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 #16183
Closes #16221
Closes #16487
Closes #16532
Closes #16733
Closes #16755
Closes #17015
Closes #17360
Closes #17420
Closes #17457
Closes #17799
Closes #17976
Closes #18057
Closes #18084
Closes #18169
Closes #18176
Closes #18191

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

src/cascadia/TerminalApp/Remoting.idl

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

DHowett · 2024-12-05T17:39:08Z

src/cascadia/WindowsTerminal/AppHost.cpp

+
+    if (const auto manager = *s_desktopManager.lock_shared())
+    {
+        return manager;


but this will still return a dead RPC object if we had one stored, right? there's no way to communicate failure and invalidate it

I tried it by killing explorer.exe but it still worked for me. Does the client perhaps reconnect automatically?

src/cascadia/WindowsTerminal/WindowEmperor.cpp

DHowett

Classic Leonard code - linear, straightforward, and readable. Another tour-de-force.

DHowett · 2024-12-05T17:57:42Z

src/cascadia/WindowsTerminal/WindowEmperor.cpp

-        const wil::unique_environstrings_ptr strings{ GetEnvironmentStringsW() };
-        const auto beg = strings.get();
-        auto end = beg;
+        wil::unique_mutex mutex{ CreateMutexW(nullptr, TRUE, className) };


fwiw unique_mutex_nothrow et al have .try_create which returns false if it fails and populates an out param boolean

src/cascadia/WindowsTerminal/WindowEmperor.cpp

zadjii-msft

116/164, but the baby's awake

zadjii-msft · 2024-12-11T12:00:36Z

src/cascadia/TerminalApp/App.cpp

-        Exit();
-        {
-            MSG msg = {};
-            while (PeekMessageW(&msg, nullptr, 0, 0, PM_REMOVE))


📝 did this get moved somewhere into like, main()

zadjii-msft · 2024-12-11T12:00:57Z

src/cascadia/TerminalApp/App.cpp

-            // us. On Windows 10, that CoreWindow will show up as a visible
-            // window on the taskbar, unless we hide it manually. So, go get it
-            // and do the SW_HIDE thing on it.
-            if (const auto& coreWindow{ winrt::Windows::UI::Core::CoreWindow::GetForCurrentThread() })


I know this sounds dumb - but we tested this on a Win10 VM right?

I did test it and from what I can tell it seems to work.
Better yet, I just tested #16183 and that's fixed now.

zadjii-msft · 2024-12-11T12:14:08Z

src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp

@@ -5,16 +5,18 @@
 ********************************************************/
 #include "pch.h"
 #include "NonClientIslandWindow.h"
+
+#include <dwmapi.h>


Due to the removal of some other files, this .cpp file now lacks the necessary imports for the DWM API without this.

zadjii-msft · 2024-12-11T12:22:48Z

src/cascadia/WindowsTerminal/WindowEmperor.cpp

+                break;
+            }
+            case WM_CONTEXTMENU:
+                if (const auto menu = CreatePopupMenu())


this is definitely grosser to read now. I kinda liked the idea that the logic for the tray icon was all encapsulated into its own file. Even if it's not a separate class, just anything that's not deep in a switch/case...

We can also encapsulate it into its own function. That seems like a good trade-off to me. It's better than using a separate class IMO and sharing a HWND and any other implicit state across class boundaries.

zadjii-msft · 2024-12-11T12:23:33Z

src/cascadia/WindowsTerminal/WindowEmperor.cpp

+
+                            if (const auto title = logic.Title(); !title.empty())
+                            {
+                                fmt::format_to(std::back_inserter(displayText), L": {}", title);


... like look here - this line starts with 32 columns of whitespace 😨

zadjii-msft · 2024-12-11T12:48:21Z

src/cascadia/TerminalApp/TerminalPage.h

@@ -496,12 +480,6 @@ namespace winrt::TerminalApp::implementation

        void _TryMoveTab(const uint32_t currentTabIndex, const int32_t suggestedNewTabIndex);

-        bool _shouldMouseVanish{ false };


📝where'd this go

➡️TermControl.cpp

zadjii-msft · 2024-12-11T14:05:55Z

src/cascadia/TerminalApp/TerminalPage.cpp

-                pointerPosition.Y - windowOrigin.Y - tabPosition.Y,
-            };
+            const auto inverseScale = 1.0f / static_cast<float>(eventTab.XamlRoot().RasterizationScale());
+            POINT cursorPos;


📝 IIRC the old one gets the offest within the tab, right? so that we create the new window s.t. the tab itself doesn't seem to move from where we dropped it.

Now we're stashing the relative position within the old window of the mouse pointer. I'd guess for a tab farther to the right in the row, that causes us to spawn the new window to the left of where we'd expect...

From what I can tell, the new logic works correct on my 150% scale display.

src/cascadia/TerminalApp/TerminalWindow.cpp

zadjii-msft · 2024-12-11T14:30:24Z

src/cascadia/TerminalApp/TerminalWindow.cpp

-    // - <none>
-    // Return Value:
-    // - true iff we should exit the application before even starting the window
-    bool TerminalWindow::ShouldExitEarly()


📝 check that we still handle this scenario

This is handled by the following code in WindowEmperor.cpp:

// If we created no windows, e.g. because the args are "/?" we can just exit now. _postQuitMessageIfNeeded();

But thank you for bringing this up, because I just noticed that wtd /? does not work.

lhecker · 2024-12-11T18:58:57Z

src/cascadia/WindowsTerminal/WindowEmperor.cpp

+    // Once the loop exits, the app exits, and the message box will be closed.
+    _messageBoxCount += 1;
+    const auto decrement = wil::scope_exit([hwnd = _window.get()]() noexcept {
+        PostMessageW(hwnd, WM_MESSAGE_BOX_CLOSED, 0, 0);


FWIW the reason I went with PostMessageW instead of resume_foreground is because I think it's crucial that this does not fail so that the ref-count does not go out of sync. I think PostMessageW is fundamentally more robust in that regard.

zadjii-msft

161/164, but alas, the burrito screams

sometimes when I get that deep into a PR as big as this, I just assume the last couple files must be good too

zadjii-msft · 2024-12-11T17:49:50Z

src/cascadia/WindowsTerminal/WindowEmperor.cpp

 }

-void WindowEmperor::WaitForWindows()
+void WindowEmperor::CreateNewWindow(winrt::TerminalApp::WindowRequestedArgs args)


zadjii-msft · 2024-12-11T19:03:43Z

src/cascadia/WindowsTerminal/WindowEmperor.cpp

+    {
+        if (const auto interop = coreWindow.try_as<ICoreWindowInterop>())
+        {
+            HWND coreHandle = nullptr;
+            if (SUCCEEDED(interop->get_WindowHandle(&coreHandle)) && coreHandle)


Do we need to do this before we start spawning windows (and CLI processes?)

I think we could do this at any point. Do you have something in mind?

zadjii-msft · 2024-12-12T14:15:41Z

src/cascadia/WindowsTerminal/WindowEmperor.cpp

+    else
+    {
+        winrt::TerminalApp::WindowRequestedArgs request{ windowId, std::move(args) };
+        request.WindowName(std::move(windowName));


Entirely sytlistically, it's a little weird that this isn't like window = _mostRecentWindowCurrentDesktop(); break;. It's the only early return in what is otherwise a very methodical function

That's unfortunately necessary because _mostRecentWindowCurrentDesktop is a coroutine. We could of course turn this function into a coroutine as well and then do

window = co_await _mostRecentWindowCurrentDesktop(); break;

It's mostly a coincidence that I drew the boundary like that.

zadjii-msft · 2024-12-12T14:25:27Z

src/cascadia/WindowsTerminal/WindowEmperor.cpp

+        }
+    }
+    else if (!args.WindowName.empty())
+    {


📝: you know, the code is cleaner if it's one for loop with the if(args.WindowID) / else if (!args.WindowName.empty()) statement on the inside, but it's less efficient then. 🤷

Nah, it'd would not really be a big difference (branch prediction + it usually never loops >10 times anyway). This is also not really intentional, and I'll just move the if condition inside.

Eh, you know what, I'll leave this as-is for now, because it makes the window = _mostRecentWindow(); branch at the end easier to express. If anyone ever happens to change this code I would not mind at all.

zadjii-msft · 2024-12-12T14:34:12Z

src/cascadia/WindowsTerminal/WindowEmperor.cpp

+                wchar_t guidStr[39];
+                guidStr[0] = L'{';
+                memcpy(&guidStr[1], name.data() + 7, 36 * sizeof(wchar_t));
+                guidStr[37] = L'}';


of course it can't be, that would be too easy

In my defense, when I wrote that GuidFromPlainString didn't exist yet. 😅
It's quite ugly code indeed.

zadjii-msft

zadjii-msft · 2024-12-12T14:59:38Z

src/cascadia/WindowsTerminal/BaseWindow.h

 template<typename T>
 class BaseWindow
 {
 public:
+    static constexpr UINT CM_UPDATE_TITLE = WM_USER + 0;


feels weird that this guy isnt with the others in WindowEmperor. ik its lt a message "for the emperor", just threw me

WM_USER messages are technically per-window-class from what I know, so I felt like they should be defined in their respective window classes as well.

zadjii-msft · 2024-12-12T15:01:19Z

src/cascadia/WindowsTerminal/AppHost.cpp

+    QueryPerformanceFrequency(&freq);
+    QueryPerformanceCounter(&counter);
+
+    const auto period = freq.QuadPart * 4;


oh intriguing, all windows will have the same frame xolor now cool

Oh, woops, I wanted to back out this change and submit it as a separate PR.
And yeah, that, and fixing floating imprecision once WT has run for a long time, were my intentions with this.

zadjii-msft · 2024-12-12T15:10:37Z

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

Probably some number of: (in decreasing likelyhood)

Probably not:

Explorer.exe reset/restart causes WT tray icon to disappear for the current instance #18057
Subsequent wt.exe /min to an existing window restores a minimized window #18076

lhecker · 2024-12-12T21:21:12Z

Thanks for linking all the issues! I tested them and all but the last one seems fixed.

Forgetti di spaghetti in #18215. Closes #18324 ## Validation Steps Performed * Launch through the start menu * Explicitly minimize * Then... * Launch through the start menu again ✅ * Launch via wtd.exe in Win+R ✅ * Launch via wtd.exe in another Terminal ✅ * Launch via handoff ✅

Fixes * Cursor vanishing, because ```cpp CoreWindow::GetForCurrentThread().PointerCursor() ``` returned a non-null, but still invisible cursor. * Alt/F7 not dispatching to newly created windows, because the `WM_ACTIVATE` message now arrives before the `AppHost` is initialized. This caused the messages to be delivered to old windows. * Windows Terminal blocking expedited shutdown, because we return `FALSE` on non-`ENDSESSION_CLOSEAPP` messages. Closes #18331 Closes #18335 ## Validation Steps Performed * Cursor still doesn't really vanish for me in the first place ✅ * Alt/F7 work in new windows without triggering old ones ✅

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

lhecker added 2 commits December 5, 2024 17:56

Fix WindowEmperor not reacing HWND_BROADCAST messages

c9151e1

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

53ec62d

DHowett reviewed Dec 5, 2024

View reviewed changes

DHowett approved these changes Dec 5, 2024

View reviewed changes

lhecker added 2 commits December 10, 2024 18:10

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

314d8c2

Address feedback

5ca8264

DHowett approved these changes Dec 11, 2024

View reviewed changes

zadjii-msft reviewed Dec 11, 2024

View reviewed changes

lhecker added 3 commits December 11, 2024 19:50

Reduce nesting in the window message handler

26877d4

Fix typo

90cbe94

Fix the message box

fea2660

DHowett approved these changes Dec 11, 2024

View reviewed changes

lhecker commented Dec 11, 2024

View reviewed changes

zadjii-msft reviewed Dec 12, 2024

View reviewed changes

zadjii-msft approved these changes Dec 12, 2024

View reviewed changes

lhecker merged commit 8bbf00e into main Dec 12, 2024
20 checks passed

lhecker deleted the dev/lhecker/dethronement branch December 12, 2024 21:21

microsoft-github-policy-service bot added Area-DefApp Area-Remoting Communication layer between windows. Often for windowing behavior, quake mode, etc. Area-UserInterface Issues pertaining to the user interface of the Console or Terminal labels Dec 12, 2024

lhecker mentioned this pull request Dec 13, 2024

Pass foreground rights to the emperor #18325

Merged

inoperable mentioned this pull request Dec 17, 2024

Mouse pointer "vanishes" when moved over Terminal Window #18335

Closed

lhecker added a commit that referenced this pull request Dec 20, 2024

Fix multiple regressions since #18215

83dadd1

lhecker added a commit that referenced this pull request Dec 20, 2024

Fix multiple regressions since #18215

6efee6c

lhecker added a commit that referenced this pull request Dec 20, 2024

Fix multiple regressions since #18215

6ac55f6

e82eric mentioned this pull request Dec 27, 2024

Unable to set alt+space as keybinding on lastest canary build #18371

Closed

		@@ -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");

		@@ -496,12 +480,6 @@ namespace winrt::TerminalApp::implementation

		void _TryMoveTab(const uint32_t currentTabIndex, const int32_t suggestedNewTabIndex);

		bool _shouldMouseVanish{ false };

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

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

This comment has been minimized.

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

Choose a reason for hiding this comment

zadjii-msft 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

lhecker Dec 11, 2024 • edited Loading

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

zadjii-msft 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

zadjii-msft 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

zadjii-msft commented Dec 12, 2024

lhecker commented Dec 12, 2024

lhecker commented Nov 19, 2024 •

edited

Loading

lhecker Dec 11, 2024 •

edited

Loading