Remove Monarch/Peasant & Make UI single-threaded by lhecker · Pull Request #18215 · microsoft/terminal

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


        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

+    void TerminalPage::IdentifyWindow()
    {
-        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


 // 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 · 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

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

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

zadjii-msft mentioned this pull request Feb 19, 2025

[1.18+] On Windows 10, Terminal reserves ~15 MB of memory per window opened at the same time #15384

Closed

zadjii-msft linked an issue Feb 19, 2025 that may be closed by this pull request

[1.18+] On Windows 10, Terminal reserves ~15 MB of memory per window opened at the same time #15384

Closed

This was referenced Mar 26, 2025

"Attach to the most recently used window on this desktop" continues to open in instance on other desktop #18652

Closed

New tab doesn't inherit working directory for -d #18604

Closed

carlos-zamora mentioned this pull request May 2, 2025

Allowing headless mode (running in the background) prevents Windows Terminal from opening again #18845

Closed

DHowett mentioned this pull request May 28, 2025

Ensure we are in the foreground when closed with multiple tabs open #15713

Closed

4 tasks

lhecker mentioned this pull request Jul 9, 2025

Window doesn't close when it's dragged WHILE CLI app exits #12951

Closed

DHowett mentioned this pull request Feb 4, 2026

"Ghost" windows appear on every launch when background run is enabled #19823

Open


		const auto canDragDrop = CanDragDrop();

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

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

Conversation

lhecker commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Validation Steps Performed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

DHowett commented Nov 19, 2024

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zadjii-msft commented Dec 12, 2024

Uh oh!

lhecker commented Dec 12, 2024

Uh oh!

lhecker commented Nov 19, 2024 •

edited

Loading