From c528a72cccac4d94e9567c46388cd0bfeac18482 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Thu, 27 Feb 2025 15:51:13 +0100 Subject: [PATCH 1/3] Fix dialogs not working across multiple windows --- src/cascadia/TerminalApp/TerminalWindow.cpp | 56 +++++++++++++++------ src/cascadia/TerminalApp/TerminalWindow.h | 2 - 2 files changed, 41 insertions(+), 17 deletions(-) diff --git a/src/cascadia/TerminalApp/TerminalWindow.cpp b/src/cascadia/TerminalApp/TerminalWindow.cpp index 4d644666e1c..c4db653cfb2 100644 --- a/src/cascadia/TerminalApp/TerminalWindow.cpp +++ b/src/cascadia/TerminalApp/TerminalWindow.cpp @@ -144,7 +144,6 @@ namespace winrt::TerminalApp::implementation { // Now that we know we can do XAML, build our page. _root = winrt::make_self(*_WindowProperties, _manager); - _dialog = ContentDialog{}; // Pass in information about the initial state of the window. // * If we were supposed to start from serialized "content", do that, @@ -313,6 +312,14 @@ namespace winrt::TerminalApp::implementation { return _settings.GlobalSettings().CurrentTheme(); } + + // WinUI can't show 2 dialogs simultaneously. Yes, really. If you do, you get an exception. + // As such, we must dismiss whatever dialog is currently being shown. + // + // This limit is of course per-thread and not per-window. Yes... really. + // The consequence is that we use a static variable to keep track of the shown dialog. + static ContentDialog s_activeDialog{ nullptr }; + // Method Description: // - Show a ContentDialog with buttons to take further action. Uses the // FrameworkElements provided as the title and content of this dialog, and @@ -328,16 +335,32 @@ namespace winrt::TerminalApp::implementation // - an IAsyncOperation with the dialog result winrt::Windows::Foundation::IAsyncOperation TerminalWindow::ShowDialog(winrt::WUX::Controls::ContentDialog dialog) { - // DON'T release this lock in a wil::scope_exit. The scope_exit will get - // called when we await, which is not what we want. - std::unique_lock lock{ _dialogLock, std::try_to_lock }; - if (!lock) + // As mentioned on s_activeDialog, dismissing the active dialog is necessary. + // We repeat it a few times in case the resume_foreground failed to work, + // but I found that one iteration will always be enough in practice. + for (int i = 0; i < 3; ++i) + { + if (!s_activeDialog) + { + break; + } + + s_activeDialog.Hide(); + + // Wait for the current dialog to be hidden. + co_await wil::resume_foreground(_root->Dispatcher(), CoreDispatcherPriority::Low); + } + + // If two sources call ShowDialog() simultaneously, it may happen that both enter the above loop, + // but it's crucial that only one of them continues below as only 1 dialog can be shown at a time. + // Thankfully, everything runs on the UI thread, so only 1 caller will exit the above loop at a time. + // So, if s_activeDialog is still set at this point, we must've lost the race. + if (s_activeDialog) { - // Another dialog is visible. co_return ContentDialogResult::None; } - _dialog = dialog; + s_activeDialog = dialog; // IMPORTANT: This is necessary as documented in the ContentDialog MSDN docs. // Since we're hosting the dialog in a Xaml island, we need to connect it to the @@ -367,23 +390,26 @@ namespace winrt::TerminalApp::implementation } } }; - themingLambda(dialog, nullptr); // if it's already in the tree - auto loadedRevoker{ dialog.Loaded(winrt::auto_revoke, themingLambda) }; // if it's not yet in the tree + auto result = ContentDialogResult::None; - // Display the dialog. - co_return co_await dialog.ShowAsync(Controls::ContentDialogPlacement::Popup); + // Extra scope to drop the revoker before resetting the s_activeDialog to null. + { + themingLambda(dialog, nullptr); // if it's already in the tree + auto loadedRevoker{ dialog.Loaded(winrt::auto_revoke, themingLambda) }; // if it's not yet in the tree + result = co_await dialog.ShowAsync(Controls::ContentDialogPlacement::Popup); + } - // After the dialog is dismissed, the dialog lock (held by `lock`) will - // be released so another can be shown + s_activeDialog = nullptr; + co_return result; } // Method Description: // - Dismiss the (only) visible ContentDialog void TerminalWindow::DismissDialog() { - if (auto localDialog = std::exchange(_dialog, nullptr)) + if (s_activeDialog) { - localDialog.Hide(); + s_activeDialog.Hide(); } } diff --git a/src/cascadia/TerminalApp/TerminalWindow.h b/src/cascadia/TerminalApp/TerminalWindow.h index eef9efaf210..6e37e179475 100644 --- a/src/cascadia/TerminalApp/TerminalWindow.h +++ b/src/cascadia/TerminalApp/TerminalWindow.h @@ -167,8 +167,6 @@ namespace winrt::TerminalApp::implementation // ALSO: If you add any UIElements as roots here, make sure they're // updated in _ApplyTheme. The root currently is _root. winrt::com_ptr _root{ nullptr }; - winrt::Windows::UI::Xaml::Controls::ContentDialog _dialog{ nullptr }; - std::shared_mutex _dialogLock; wil::com_ptr _appArgs{ nullptr }; bool _hasCommandLineArguments{ false }; From e10a93618a648ade92dff8c8dcadf37c7a75f3bc Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Thu, 27 Feb 2025 16:04:30 +0100 Subject: [PATCH 2/3] Fix spelling --- src/cascadia/TerminalApp/TerminalWindow.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/TerminalApp/TerminalWindow.cpp b/src/cascadia/TerminalApp/TerminalWindow.cpp index c4db653cfb2..9b629a75526 100644 --- a/src/cascadia/TerminalApp/TerminalWindow.cpp +++ b/src/cascadia/TerminalApp/TerminalWindow.cpp @@ -354,7 +354,7 @@ namespace winrt::TerminalApp::implementation // If two sources call ShowDialog() simultaneously, it may happen that both enter the above loop, // but it's crucial that only one of them continues below as only 1 dialog can be shown at a time. // Thankfully, everything runs on the UI thread, so only 1 caller will exit the above loop at a time. - // So, if s_activeDialog is still set at this point, we must've lost the race. + // So, if s_activeDialog is still set at this point, we must have lost the race. if (s_activeDialog) { co_return ContentDialogResult::None; From 4ec2cd84f5a09c7974ac359a94370148a9bdc098 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Thu, 27 Feb 2025 18:55:17 +0100 Subject: [PATCH 3/3] Add more info --- src/cascadia/TerminalApp/TerminalWindow.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/cascadia/TerminalApp/TerminalWindow.cpp b/src/cascadia/TerminalApp/TerminalWindow.cpp index 9b629a75526..ff3a66acae2 100644 --- a/src/cascadia/TerminalApp/TerminalWindow.cpp +++ b/src/cascadia/TerminalApp/TerminalWindow.cpp @@ -316,7 +316,8 @@ namespace winrt::TerminalApp::implementation // WinUI can't show 2 dialogs simultaneously. Yes, really. If you do, you get an exception. // As such, we must dismiss whatever dialog is currently being shown. // - // This limit is of course per-thread and not per-window. Yes... really. + // This limit is of course per-thread and not per-window. Yes... really. See: + // https://github.com/microsoft/microsoft-ui-xaml/issues/794 // The consequence is that we use a static variable to keep track of the shown dialog. static ContentDialog s_activeDialog{ nullptr };