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).

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 2 commits November 23, 2024 00:42

Fix tab dragging between windows

1aafccb

Address feedback

0862e90

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

		@@ -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

lhecker commented Nov 19, 2024 •

edited

Loading