Skip to content

Commit

Permalink
Fix dialogs not working across multiple windows (#18636)
Browse files Browse the repository at this point in the history
This can be considered "part 1" of fixing #18599: It prevents crashes
(due to unhandled exceptions) by ensuring we only create 1 content
dialog across all windows at a time. Sounds bad, but I tried it and it's
not actually _that_ bad in practice (it's still really gross though).

The bad news is that I don't have a "part 2", because I can't figure out
what's going on:
* Create 2 windows
* Open the About dialog in window 1
  and right click the text
* Close the About dialog
* Open the About dialog in window 2
  and right click the text
* WinUI will simply toss the focus to window 1

It appears as if context menus are permanently associated with the first
window that uses them. It has nothing to do with whether a ContentDialog
instance is reused (I tested that).

## Validation Steps Performed
* Open 2 windows with 2 tabs each
* Attempt to close window 1, dialog appears ✅
* Attempt to close window 2, dialog moves to window 2 ✅
  • Loading branch information
lhecker authored Feb 28, 2025
1 parent 96d1407 commit 3760cae
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 17 deletions.
57 changes: 42 additions & 15 deletions src/cascadia/TerminalApp/TerminalWindow.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,6 @@ namespace winrt::TerminalApp::implementation
{
// Now that we know we can do XAML, build our page.
_root = winrt::make_self<TerminalPage>(*_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,
Expand Down Expand Up @@ -313,6 +312,15 @@ 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. 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 };

// Method Description:
// - Show a ContentDialog with buttons to take further action. Uses the
// FrameworkElements provided as the title and content of this dialog, and
Expand All @@ -328,16 +336,32 @@ namespace winrt::TerminalApp::implementation
// - an IAsyncOperation with the dialog result
winrt::Windows::Foundation::IAsyncOperation<ContentDialogResult> 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 have 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
Expand Down Expand Up @@ -367,23 +391,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();
}
}

Expand Down
2 changes: 0 additions & 2 deletions src/cascadia/TerminalApp/TerminalWindow.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<TerminalPage> _root{ nullptr };
winrt::Windows::UI::Xaml::Controls::ContentDialog _dialog{ nullptr };
std::shared_mutex _dialogLock;

wil::com_ptr<CommandlineArgs> _appArgs{ nullptr };
bool _hasCommandLineArguments{ false };
Expand Down

0 comments on commit 3760cae

Please sign in to comment.