Remove Monarch/Peasant & Make UI single-threaded #18215

lhecker · 2024-11-19T00:15:21Z

As before, a minor refactor:

I started off by removing the Monarch/Peasant with the goal of moving
it into and deduplicating its functionality with WindowEmperor.
Since I needed a replacement for the Monarch (= ensures that there's
a single instance), I wrote single-instance code with a NT mutex
and by yeeting data across processes with WM_COPYDATA.
This resulted in severe threading issues, because it now started up
way faster. The more I tried to solve them the deeper I had to dig,
because you can't just put a mutex around CascadiaSettings.
I then tried to seeif WinUI can run multiple windows on a single
thread and, as it turns out, it can.
So, I removed the multi- from the window threading.
At this point I had dig about 1 mile deep and brought no ladder.
So, to finish it up, I had to clean up the entire eventing system
around WindowEmperor, cleaned up all the coroutines,
and cleaned up all the callbacks.

Closes #16733
Closes #17799
Closes #18176
Closes #18191
TODO: Find other issues

Validation Steps Performed

It does not crash ✅
New/close tab ✅
New/close window ✅
Move tabs between windows ✅
Split tab into new window ✅
Persist windows on exit / restore startup ✅

lhecker · 2024-11-19T00:19:11Z

src/cascadia/TerminalApp/TerminalPage.cpp

@@ -170,7 +170,6 @@ namespace winrt::TerminalApp::implementation

        const auto canDragDrop = CanDragDrop();

-        _tabRow.PointerMoved({ get_weak(), &TerminalPage::_RestorePointerCursorHandler });


Moved into TerminalControl as a different workaround is needed for this functionality.

lhecker · 2024-11-19T00:20:19Z

src/cascadia/TerminalApp/TerminalPage.cpp

    {
-        auto weakThis{ get_weak() };
-        co_await wil::resume_foreground(Dispatcher());


No multi-threading = No problem. (Suppress whitespace in the diff.)

DHowett · 2024-11-19T15:15:32Z

src/cascadia/WindowsTerminal/WindowsTerminal.vcxproj

-      <CurrentXamlPackage Include="@(AppxPackageRegistration)"
-        Condition="'%(AppxPackageRegistration.Architecture)'=='$(Platform)' AND
-        $([System.String]::new('%(AppxPackageRegistration.Filename)').StartsWith('Microsoft.UI.Xaml'))" />
+      <CurrentXamlPackage Include="@(AppxPackageRegistration)" Condition="'%(AppxPackageRegistration.Architecture)'=='$(Platform)' AND&#xD;&#xA;        $([System.String]::new('%(AppxPackageRegistration.Filename)').StartsWith('Microsoft.UI.Xaml'))" />


VS mangled this :(

DHowett

86/154! It's easy to review deletions ;)

DHowett · 2024-11-19T15:21:06Z

different branding versions of terminal do not interact
unpackaged terminal doesn't interact with:
- any other unpackaged terminal
- any other packaged terminal

DHowett

139/154

DHowett · 2024-11-19T18:36:04Z

src/cascadia/WindowsTerminal/IslandWindow.cpp

-                auto isCurrentlyDark = Theme::IsSystemInDarkTheme();
-                if (isCurrentlyDark != _currentSystemThemeIsDark)
-                {
-                    _currentSystemThemeIsDark = isCurrentlyDark;


📝 DH - Guessing this moved to the Message Window; confirm

DHowett · 2024-11-19T19:43:25Z

src/cascadia/TerminalApp/Remoting.idl

+{
+    runtimeclass CommandlineArgs
+    {
+        CommandlineArgs();


FWIW, these constructors are only necessary if they will be created from outside the DLL.

If not, you can remove the ctor from the IDL, and the FACTORY from the header.

It's instantiated in the WindowEmperor. Honestly, we could just move WindowEmperor into TerminalApp and call it from main(). That may make things simpler, especially now that it's way smaller.

DHowett · 2024-11-19T19:44:26Z

src/cascadia/TerminalApp/Remoting.idl

+        WindowRequestedArgs(String window, String content, Windows.Foundation.IReference<Windows.Foundation.Rect> bounds);
+
+        UInt64 Id;
+        String WindowName;


I saw you had to make a change to make WindowName writable. Is it possible to restrict that to the internal interface, so that it is not writable in the winmd and idl?

It's being written the WindowEmperor which is external. I was hoping to improve this design at a later time.

DHowett · 2024-11-19T19:45:45Z

src/cascadia/WindowsTerminal/IslandWindow.cpp

-    // On the foreground thread:
-    co_await wil::resume_foreground(_rootGrid.Dispatcher());
-    _summonWindowRoutineBody(args);
+    _summonWindowRoutineBody(std::move(args));
 }

 // Method Description:
 // - As above.
 //   BODGY: ARM64 BUILD FAILED WITH fatal error C1001: Internal compiler error


now that we got a new compiler, is this still valid

it's not even a coroutine any longer, so we can probably dispense with the separation entirely...

DHowett · 2024-11-19T19:46:50Z

src/cascadia/WindowsTerminal/NotificationIcon.cpp

-    ScopedResourceLoader loader{ L"TerminalApp/ContextMenu" };
-    const auto appNameLoc = loader.GetLocalizedString(L"AppName");


📝 DH - Loading up somebody else's resources - probably moved to Emperor

DHowett · 2024-11-19T19:47:48Z

src/cascadia/WindowsTerminal/WindowEmperor.h

+    HWND GetMainWindow() const noexcept;
+    AppHost* GetWindowById(uint64_t id) const noexcept;
+    AppHost* GetWindowByName(std::wstring_view name) const noexcept;
+    void CreateNewWindow(winrt::TerminalApp::WindowRequestedArgs args);


nit: why not const&?

DHowett · 2024-11-19T19:48:50Z

src/cascadia/WindowsTerminal/WindowEmperor.h

-    bool _registerHotKey(const int index, const winrt::Microsoft::Terminal::Control::KeyChord& hotkey) noexcept;
-    void _unregisterHotKey(const int index) noexcept;
-    safe_void_coroutine _setupGlobalHotkeys();
+    enum class TriBool : uint8_t


HAHAHAHAHAHAHAHAHAHAH

hahahahahaha

hahah

std::optional<bool>?

std::optional<bool> is two bools in a trench coat, so... meh.

...but yeah I could do that.

DHowett · 2024-11-19T19:53:11Z

src/cascadia/TerminalControl/TermControl.cpp

-        const auto absoluteWindowOrigin = CoreWindow::GetForCurrentThread().Bounds();
-        // Get the offset (margin + tabs, etc..) of the control within the window
-        const auto controlOrigin = TransformToVisual(nullptr).TransformPoint({});
+        const auto inverseScale = 1.0f / static_cast<float>(XamlRoot().RasterizationScale());


these don't seem related to regicide

CoreWindow::GetForCurrentThread().Bounds() won't work anymore, so this had to be rewritten. I also chose to use the technically correct way to get the rasterization scale (we do it incorrectly everywhere, according to the docs).

github-actions · 2024-11-26T20:56:22Z

@check-spelling-bot Report

🔴 Please review

See the 📂 files view, the 📜action log, or 📝 job summary for details.

Unrecognized words (2)

IInput
inputpaneinterop

Previously acknowledged words that are now absent

barbaz DISPATCHNOTIFY Ralph stb 🫥

To accept these unrecognized words as correct and remove the previously acknowledged and now absent words, you could run the following commands

... in a clone of the [email protected]:microsoft/terminal.git repository
on the dev/lhecker/dethronement branch (ℹ️ how do I use this?):

curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/v0.0.22/apply.pl' |
perl - 'https://github.com/microsoft/terminal/actions/runs/12038853251/attempts/1'

Available 📚 dictionaries could cover words (expected and unrecognized) not in the 📘 dictionary

This includes both expected items (2229) from .github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt and unrecognized words (2)

Dictionary	Entries	Covers	Uniquely
cspell:cpp/src/lang-jargon.txt	11	1	1
cspell:swift/src/swift.txt	53	1	1
cspell:gaming-terms/dict/gaming-terms.txt	59	1	1
cspell:monkeyc/src/monkeyc_keywords.txt	123	1	1
cspell:cryptocurrencies/cryptocurrencies.txt	125	1	1

Consider adding them (in .github/workflows/spelling2.yml) for uses: check-spelling/[email protected] in its with:

      with:
        extra_dictionaries:
          cspell:cpp/src/lang-jargon.txt
          cspell:swift/src/swift.txt
          cspell:gaming-terms/dict/gaming-terms.txt
          cspell:monkeyc/src/monkeyc_keywords.txt
          cspell:cryptocurrencies/cryptocurrencies.txt

To stop checking additional dictionaries, add (in .github/workflows/spelling2.yml) for uses: check-spelling/[email protected] in its with:

check_extra_dictionaries: ''

Errors (1)

See the 📂 files view, the 📜action log, or 📝 job summary for details.

❌ Errors	Count
❌ ignored-expect-variant	6

See ❌ Event descriptions for more information.

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

If the listed items are:

... misspelled, then please correct them instead of using the command.
... names, please add them to .github/actions/spelling/allow/names.txt.
... APIs, you can add them to a file in .github/actions/spelling/allow/.
... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

If the flagged items are 🤯 false positives

If items relate to a ...

binary file (or some other file you wouldn't want to check at all).

Please add a file path to the excludes.txt file matching the containing file.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).
well-formed pattern.

If you can write a pattern that would match it,
try adding it to the patterns.txt file.

Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

Note that patterns can't match multiline strings.

lhecker changed the title ~~Remove Monarch/Peasant, Single-threaded UI, and Windowing cleanup~~ Remove Monarch/Peasant & Make UI single-threaded Nov 19, 2024

microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Quality Stability, Performance, Etc. Product-Terminal The new Windows Terminal. Severity-Crash Crashes are real bad news. labels Nov 19, 2024

List of monarchs who lost their thrones in the 21st century

7853853

lhecker force-pushed the dev/lhecker/dethronement branch from b39c5a7 to 7853853 Compare November 19, 2024 00:29

lhecker commented Nov 19, 2024

View reviewed changes

DHowett reviewed Nov 19, 2024

View reviewed changes

lhecker added 4 commits November 19, 2024 18:09

Revert WindowsTerminal.vcxproj

3dee5c0

Fix window singleton for unpackaged builds

16900fd

Dismiss tray icon on exit

25d1c71

Fix _quake summoning

c7d1393

DHowett reviewed Nov 19, 2024

View reviewed changes

microsoft-github-policy-service bot added the Area-Windowing Window frame, quake mode, tearout label Nov 19, 2024

Fix formatting

5faf0f6

microsoft-github-policy-service bot added the Priority-3 A description (P3) label Nov 20, 2024

lhecker added 2 commits November 22, 2024 01:56

Merge remote-tracking branch 'origin/main' into dev/lhecker/dethronement

82902f2

Fix wt /?

06066d5

DHowett reviewed Nov 22, 2024

View reviewed changes

lhecker added 7 commits November 23, 2024 00:42

Fix tab dragging between windows

1aafccb

Address feedback

0862e90

Don't include the pch in header files

c66ad24

Remove unused code

f0449d7

Prevent the CoreWindow from getting destroyed

f665c2f

Fix the touch input panel

88a5302

A more sensible timeout

8682ae1

This comment has been minimized.

Sign in to view

Fix tab drag window position

d84aa79

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Monarch/Peasant & Make UI single-threaded #18215

Remove Monarch/Peasant & Make UI single-threaded #18215

lhecker commented Nov 19, 2024 •

edited

Loading

lhecker Nov 19, 2024

lhecker Nov 19, 2024

DHowett Nov 19, 2024

DHowett left a comment

DHowett commented Nov 19, 2024

DHowett left a comment

DHowett Nov 19, 2024

DHowett Nov 19, 2024

lhecker Nov 22, 2024

DHowett Nov 19, 2024

lhecker Nov 22, 2024

DHowett Nov 19, 2024

DHowett Nov 19, 2024

DHowett Nov 19, 2024

DHowett Nov 19, 2024

DHowett Nov 19, 2024

lhecker Nov 22, 2024

DHowett Nov 19, 2024

lhecker Nov 22, 2024

This comment has been minimized.

github-actions bot commented Nov 26, 2024

		@@ -170,7 +170,6 @@ namespace winrt::TerminalApp::implementation

		const auto canDragDrop = CanDragDrop();

		_tabRow.PointerMoved({ get_weak(), &TerminalPage::_RestorePointerCursorHandler });

		ScopedResourceLoader loader{ L"TerminalApp/ContextMenu" };
		const auto appNameLoc = loader.GetLocalizedString(L"AppName");

Remove Monarch/Peasant & Make UI single-threaded #18215

Are you sure you want to change the base?

Remove Monarch/Peasant & Make UI single-threaded #18215

Conversation

lhecker commented Nov 19, 2024 • edited Loading

Validation Steps Performed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented Nov 19, 2024

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

github-actions bot commented Nov 26, 2024

@check-spelling-bot Report

🔴 Please review

Unrecognized words (2)

lhecker commented Nov 19, 2024 •

edited

Loading