Skip to content

Commit

Permalink
No longer load content dialogs when there is already one being shown (#…
Browse files Browse the repository at this point in the history 
…12517)

## Summary of the Pull Request
Somehow, the controls v2 update caused an issue where if you as much as _load_ a content dialog when there's already one open, we get holes in the terminal window (#12447)

This commit introduces logic to `TerminalPage` to check whether there is a content dialog open before we try to load another one.

## PR Checklist
* [x] Closes #12447
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [ ] Schema updated.
* [x] I work here

## Validation Steps Performed
Can no longer repro #12447

(cherry picked from commit 788d33c)
  • Loading branch information
@PankajBhojwani @DHowett
PankajBhojwani authored and DHowett committed Mar 10, 2022
1 parent 3a42dc2 commit c4266cc
Show file tree
Hide file tree
Showing 7 changed files with 57 additions and 34 deletions.
8 changes: 8 additions & 0 deletions src/cascadia/TerminalApp/AppLogic.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,14 @@ namespace winrt::TerminalApp::implementation
}
}

// Method Description:
// - Returns true if there is no dialog currently being shown (meaning that we can show a dialog)
// - Returns false if there is a dialog currently being shown (meaning that we cannot show another dialog)
bool AppLogic::CanShowDialog()
{
return (_dialog == nullptr);
}

// Method Description:
// - Displays a dialog for errors found while loading or validating the
// settings. Uses the resources under the provided title and content keys
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalApp/AppLogic.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ namespace winrt::TerminalApp::implementation
bool GetShowTitleInTitlebar();

winrt::Windows::Foundation::IAsyncOperation<winrt::Windows::UI::Xaml::Controls::ContentDialogResult> ShowDialog(winrt::Windows::UI::Xaml::Controls::ContentDialog dialog);
bool CanShowDialog();
void DismissDialog();

Windows::Foundation::Collections::IMapView<Microsoft::Terminal::Control::KeyChord, Microsoft::Terminal::Settings::Model::Command> GlobalHotkeys();
Expand Down
7 changes: 2 additions & 5 deletions src/cascadia/TerminalApp/AppLogic.idl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ namespace TerminalApp
SystemMenuItemHandler Handler { get; };
};

// See IDialogPresenter and TerminalPage's DialogPresenter for more
// information.
[default_interface] runtimeclass AppLogic : IDirectKeyListener, IDialogPresenter
{
AppLogic();
Expand Down Expand Up @@ -106,11 +108,6 @@ namespace TerminalApp

Windows.Foundation.Collections.IMapView<Microsoft.Terminal.Control.KeyChord, Microsoft.Terminal.Settings.Model.Command> GlobalHotkeys();

// See IDialogPresenter and TerminalPage's DialogPresenter for more
// information.
Windows.Foundation.IAsyncOperation<Windows.UI.Xaml.Controls.ContentDialogResult> ShowDialog(Windows.UI.Xaml.Controls.ContentDialog dialog);
void DismissDialog();

event Windows.Foundation.TypedEventHandler<Object, Windows.UI.Xaml.UIElement> SetTitleBarContent;
event Windows.Foundation.TypedEventHandler<Object, String> TitleChanged;
event Windows.Foundation.TypedEventHandler<Object, LastTabClosedEventArgs> LastTabClosed;
Expand Down
62 changes: 33 additions & 29 deletions src/cascadia/TerminalApp/TerminalPage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -685,10 +685,7 @@ namespace winrt::TerminalApp::implementation
// Notes link, and privacy policy link.
void TerminalPage::_ShowAboutDialog()
{
if (auto presenter{ _dialogPresenter.get() })
{
presenter.ShowDialog(FindName(L"AboutDialog").try_as<WUX::Controls::ContentDialog>());
}
_ShowDialogHelper(L"AboutDialog");
}

winrt::hstring TerminalPage::ApplicationDisplayName()
Expand All @@ -708,6 +705,33 @@ namespace winrt::TerminalApp::implementation
ShellExecute(nullptr, nullptr, currentPath.c_str(), nullptr, nullptr, SW_SHOW);
}

// Method description:
// - Called when the user closes a content dialog
// - Tells the presenter to update its knowledge of whether there is a content dialog open
void TerminalPage::_DialogCloseClick(const IInspectable&,
const ContentDialogButtonClickEventArgs&)
{
if (auto presenter{ _dialogPresenter.get() })
{
presenter.DismissDialog();
}
}

// Method Description:
// - Helper to show a content dialog
// - We only open a content dialog if there isn't one open already
winrt::Windows::Foundation::IAsyncOperation<ContentDialogResult> TerminalPage::_ShowDialogHelper(const std::wstring_view& name)
{
if (auto presenter{ _dialogPresenter.get() })
{
if (presenter.CanShowDialog())
{
co_return co_await presenter.ShowDialog(FindName(name).try_as<WUX::Controls::ContentDialog>());
}
}
co_return ContentDialogResult::None;
}

// Method Description:
// - Displays a dialog to warn the user that they are about to close all open windows.
// Once the user clicks the OK button, shut down the application.
Expand All @@ -716,11 +740,7 @@ namespace winrt::TerminalApp::implementation
// when this is called, nothing happens. See _ShowDialog for details
winrt::Windows::Foundation::IAsyncOperation<ContentDialogResult> TerminalPage::_ShowQuitDialog()
{
if (auto presenter{ _dialogPresenter.get() })
{
co_return co_await presenter.ShowDialog(FindName(L"QuitDialog").try_as<WUX::Controls::ContentDialog>());
}
co_return ContentDialogResult::None;
return _ShowDialogHelper(L"QuitDialog");
}

// Method Description:
Expand All @@ -732,22 +752,14 @@ namespace winrt::TerminalApp::implementation
// when this is called, nothing happens. See _ShowDialog for details
winrt::Windows::Foundation::IAsyncOperation<ContentDialogResult> TerminalPage::_ShowCloseWarningDialog()
{
if (auto presenter{ _dialogPresenter.get() })
{
co_return co_await presenter.ShowDialog(FindName(L"CloseAllDialog").try_as<WUX::Controls::ContentDialog>());
}
co_return ContentDialogResult::None;
return _ShowDialogHelper(L"CloseAllDialog");
}

// Method Description:
// - Displays a dialog for warnings found while closing the terminal tab marked as read-only
winrt::Windows::Foundation::IAsyncOperation<ContentDialogResult> TerminalPage::_ShowCloseReadOnlyDialog()
{
if (auto presenter{ _dialogPresenter.get() })
{
co_return co_await presenter.ShowDialog(FindName(L"CloseReadOnlyDialog").try_as<WUX::Controls::ContentDialog>());
}
co_return ContentDialogResult::None;
return _ShowDialogHelper(L"CloseReadOnlyDialog");
}

// Method Description:
Expand All @@ -760,11 +772,7 @@ namespace winrt::TerminalApp::implementation
// when this is called, nothing happens. See _ShowDialog for details
winrt::Windows::Foundation::IAsyncOperation<ContentDialogResult> TerminalPage::_ShowMultiLinePasteWarningDialog()
{
if (auto presenter{ _dialogPresenter.get() })
{
co_return co_await presenter.ShowDialog(FindName(L"MultiLinePasteDialog").try_as<WUX::Controls::ContentDialog>());
}
co_return ContentDialogResult::None;
return _ShowDialogHelper(L"MultiLinePasteDialog");
}

// Method Description:
Expand All @@ -775,11 +783,7 @@ namespace winrt::TerminalApp::implementation
// when this is called, nothing happens. See _ShowDialog for details
winrt::Windows::Foundation::IAsyncOperation<ContentDialogResult> TerminalPage::_ShowLargePasteWarningDialog()
{
if (auto presenter{ _dialogPresenter.get() })
{
co_return co_await presenter.ShowDialog(FindName(L"LargePasteDialog").try_as<WUX::Controls::ContentDialog>());
}
co_return ContentDialogResult::None;
return _ShowDialogHelper(L"LargePasteDialog");
}

// Method Description:
Expand Down
3 changes: 3 additions & 0 deletions src/cascadia/TerminalApp/TerminalPage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,12 +215,15 @@ namespace winrt::TerminalApp::implementation
std::shared_ptr<Toast> _windowIdToast{ nullptr };
std::shared_ptr<Toast> _windowRenameFailedToast{ nullptr };

winrt::Windows::Foundation::IAsyncOperation<winrt::Windows::UI::Xaml::Controls::ContentDialogResult> _ShowDialogHelper(const std::wstring_view& name);

void _ShowAboutDialog();
winrt::Windows::Foundation::IAsyncOperation<winrt::Windows::UI::Xaml::Controls::ContentDialogResult> _ShowQuitDialog();
winrt::Windows::Foundation::IAsyncOperation<winrt::Windows::UI::Xaml::Controls::ContentDialogResult> _ShowCloseWarningDialog();
winrt::Windows::Foundation::IAsyncOperation<winrt::Windows::UI::Xaml::Controls::ContentDialogResult> _ShowCloseReadOnlyDialog();
winrt::Windows::Foundation::IAsyncOperation<winrt::Windows::UI::Xaml::Controls::ContentDialogResult> _ShowMultiLinePasteWarningDialog();
winrt::Windows::Foundation::IAsyncOperation<winrt::Windows::UI::Xaml::Controls::ContentDialogResult> _ShowLargePasteWarningDialog();
void _DialogCloseClick(const IInspectable& sender, const Windows::UI::Xaml::Controls::ContentDialogButtonClickEventArgs& eventArgs);

void _CreateNewTabFlyout();
void _OpenNewTabDropdown();
Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalApp/TerminalPage.idl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ namespace TerminalApp
interface IDialogPresenter
{
Windows.Foundation.IAsyncOperation<Windows.UI.Xaml.Controls.ContentDialogResult> ShowDialog(Windows.UI.Xaml.Controls.ContentDialog dialog);
Boolean CanShowDialog();
void DismissDialog();
};

[default_interface] runtimeclass TerminalPage : Windows.UI.Xaml.Controls.Page, Windows.UI.Xaml.Data.INotifyPropertyChanged
Expand Down
8 changes: 8 additions & 0 deletions src/cascadia/TerminalApp/TerminalPage.xaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@
<ContentDialog x:Name="AboutDialog"
x:Uid="AboutDialog"
x:Load="False"
CloseButtonClick="_DialogCloseClick"
DefaultButton="Close">
<StackPanel Orientation="Vertical">
<TextBlock IsTextSelectionEnabled="True">
Expand All @@ -96,21 +97,25 @@
<ContentDialog x:Name="QuitDialog"
x:Uid="QuitDialog"
x:Load="False"
CloseButtonClick="_DialogCloseClick"
DefaultButton="Primary" />

<ContentDialog x:Name="CloseAllDialog"
x:Uid="CloseAllDialog"
x:Load="False"
CloseButtonClick="_DialogCloseClick"
DefaultButton="Primary" />

<ContentDialog x:Name="CloseReadOnlyDialog"
x:Uid="CloseReadOnlyDialog"
x:Load="False"
CloseButtonClick="_DialogCloseClick"
DefaultButton="Close" />

<ContentDialog x:Name="MultiLinePasteDialog"
x:Uid="MultiLinePasteDialog"
x:Load="False"
CloseButtonClick="_DialogCloseClick"
DefaultButton="Primary">
<StackPanel>
<TextBlock x:Uid="MultiLineWarningText"
Expand All @@ -130,11 +135,13 @@
<ContentDialog x:Name="LargePasteDialog"
x:Uid="LargePasteDialog"
x:Load="False"
CloseButtonClick="_DialogCloseClick"
DefaultButton="Primary" />

<ContentDialog x:Name="ControlNoticeDialog"
x:Uid="ControlNoticeDialog"
x:Load="False"
CloseButtonClick="_DialogCloseClick"
DefaultButton="Primary">
<TextBlock IsTextSelectionEnabled="True"
TextWrapping="WrapWholeWords">
Expand All @@ -145,6 +152,7 @@
<ContentDialog x:Name="CouldNotOpenUriDialog"
x:Uid="CouldNotOpenUriDialog"
x:Load="False"
CloseButtonClick="_DialogCloseClick"
DefaultButton="Primary">
<TextBlock IsTextSelectionEnabled="True"
TextWrapping="WrapWholeWords">
Expand Down

0 comments on commit c4266cc

Please sign in to comment.