Skip to content

Commit 3760cae

Browse files
authored
Fix dialogs not working across multiple windows (#18636)
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 ✅
1 parent 96d1407 commit 3760cae

File tree

2 files changed

+42
-17
lines changed

2 files changed

+42
-17
lines changed

src/cascadia/TerminalApp/TerminalWindow.cpp

+42-15
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,6 @@ namespace winrt::TerminalApp::implementation
144144
{
145145
// Now that we know we can do XAML, build our page.
146146
_root = winrt::make_self<TerminalPage>(*_WindowProperties, _manager);
147-
_dialog = ContentDialog{};
148147

149148
// Pass in information about the initial state of the window.
150149
// * If we were supposed to start from serialized "content", do that,
@@ -313,6 +312,15 @@ namespace winrt::TerminalApp::implementation
313312
{
314313
return _settings.GlobalSettings().CurrentTheme();
315314
}
315+
316+
// WinUI can't show 2 dialogs simultaneously. Yes, really. If you do, you get an exception.
317+
// As such, we must dismiss whatever dialog is currently being shown.
318+
//
319+
// This limit is of course per-thread and not per-window. Yes... really. See:
320+
// https://github.com/microsoft/microsoft-ui-xaml/issues/794
321+
// The consequence is that we use a static variable to keep track of the shown dialog.
322+
static ContentDialog s_activeDialog{ nullptr };
323+
316324
// Method Description:
317325
// - Show a ContentDialog with buttons to take further action. Uses the
318326
// FrameworkElements provided as the title and content of this dialog, and
@@ -328,16 +336,32 @@ namespace winrt::TerminalApp::implementation
328336
// - an IAsyncOperation with the dialog result
329337
winrt::Windows::Foundation::IAsyncOperation<ContentDialogResult> TerminalWindow::ShowDialog(winrt::WUX::Controls::ContentDialog dialog)
330338
{
331-
// DON'T release this lock in a wil::scope_exit. The scope_exit will get
332-
// called when we await, which is not what we want.
333-
std::unique_lock lock{ _dialogLock, std::try_to_lock };
334-
if (!lock)
339+
// As mentioned on s_activeDialog, dismissing the active dialog is necessary.
340+
// We repeat it a few times in case the resume_foreground failed to work,
341+
// but I found that one iteration will always be enough in practice.
342+
for (int i = 0; i < 3; ++i)
343+
{
344+
if (!s_activeDialog)
345+
{
346+
break;
347+
}
348+
349+
s_activeDialog.Hide();
350+
351+
// Wait for the current dialog to be hidden.
352+
co_await wil::resume_foreground(_root->Dispatcher(), CoreDispatcherPriority::Low);
353+
}
354+
355+
// If two sources call ShowDialog() simultaneously, it may happen that both enter the above loop,
356+
// but it's crucial that only one of them continues below as only 1 dialog can be shown at a time.
357+
// Thankfully, everything runs on the UI thread, so only 1 caller will exit the above loop at a time.
358+
// So, if s_activeDialog is still set at this point, we must have lost the race.
359+
if (s_activeDialog)
335360
{
336-
// Another dialog is visible.
337361
co_return ContentDialogResult::None;
338362
}
339363

340-
_dialog = dialog;
364+
s_activeDialog = dialog;
341365

342366
// IMPORTANT: This is necessary as documented in the ContentDialog MSDN docs.
343367
// Since we're hosting the dialog in a Xaml island, we need to connect it to the
@@ -367,23 +391,26 @@ namespace winrt::TerminalApp::implementation
367391
}
368392
} };
369393

370-
themingLambda(dialog, nullptr); // if it's already in the tree
371-
auto loadedRevoker{ dialog.Loaded(winrt::auto_revoke, themingLambda) }; // if it's not yet in the tree
394+
auto result = ContentDialogResult::None;
372395

373-
// Display the dialog.
374-
co_return co_await dialog.ShowAsync(Controls::ContentDialogPlacement::Popup);
396+
// Extra scope to drop the revoker before resetting the s_activeDialog to null.
397+
{
398+
themingLambda(dialog, nullptr); // if it's already in the tree
399+
auto loadedRevoker{ dialog.Loaded(winrt::auto_revoke, themingLambda) }; // if it's not yet in the tree
400+
result = co_await dialog.ShowAsync(Controls::ContentDialogPlacement::Popup);
401+
}
375402

376-
// After the dialog is dismissed, the dialog lock (held by `lock`) will
377-
// be released so another can be shown
403+
s_activeDialog = nullptr;
404+
co_return result;
378405
}
379406

380407
// Method Description:
381408
// - Dismiss the (only) visible ContentDialog
382409
void TerminalWindow::DismissDialog()
383410
{
384-
if (auto localDialog = std::exchange(_dialog, nullptr))
411+
if (s_activeDialog)
385412
{
386-
localDialog.Hide();
413+
s_activeDialog.Hide();
387414
}
388415
}
389416

src/cascadia/TerminalApp/TerminalWindow.h

-2
Original file line numberDiff line numberDiff line change
@@ -167,8 +167,6 @@ namespace winrt::TerminalApp::implementation
167167
// ALSO: If you add any UIElements as roots here, make sure they're
168168
// updated in _ApplyTheme. The root currently is _root.
169169
winrt::com_ptr<TerminalPage> _root{ nullptr };
170-
winrt::Windows::UI::Xaml::Controls::ContentDialog _dialog{ nullptr };
171-
std::shared_mutex _dialogLock;
172170

173171
wil::com_ptr<CommandlineArgs> _appArgs{ nullptr };
174172
bool _hasCommandLineArguments{ false };

0 commit comments

Comments
 (0)