Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Nov 19, 2024

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 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 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
@@ -170,7 +170,6 @@ namespace winrt::TerminalApp::implementation

const auto canDragDrop = CanDragDrop();

_tabRow.PointerMoved({ get_weak(), &TerminalPage::_RestorePointerCursorHandler });
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
auto weakThis{ get_weak() };
co_await wil::resume_foreground(Dispatcher());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<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'))" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VS mangled this :(

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@DHowett
Copy link
Member

DHowett commented Nov 19, 2024

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

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

139/154

auto isCurrentlyDark = Theme::IsSystemInDarkTheme();
if (isCurrentlyDark != _currentSystemThemeIsDark)
{
_currentSystemThemeIsDark = isCurrentlyDark;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@microsoft-github-policy-service microsoft-github-policy-service bot added the Area-Windowing Window frame, quake mode, tearout label Nov 19, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the Priority-3 A description (P3) label Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Quality Stability, Performance, Etc. Area-Windowing Window frame, quake mode, tearout Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. Severity-Crash Crashes are real bad news.
Projects
None yet
2 participants