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

Add settings to warn about large or multiline pastes #6631

Merged
13 commits merged into from
Jul 1, 2020
1 change: 1 addition & 0 deletions .github/actions/spell-check/dictionary/apis.txt
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ IBind
IClass
IComparable
ICustom
IDialog
IDirect
IExplorer
IMap
Expand Down
22 changes: 12 additions & 10 deletions src/cascadia/TerminalApp/AppLogic.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
using namespace winrt::Windows::ApplicationModel;
using namespace winrt::Windows::ApplicationModel::DataTransfer;
using namespace winrt::Windows::UI::Xaml;
using namespace winrt::Windows::UI::Xaml::Controls;
using namespace winrt::Windows::UI::Core;
using namespace winrt::Windows::System;
using namespace winrt::Microsoft::Terminal;
Expand Down Expand Up @@ -231,7 +232,7 @@ namespace winrt::TerminalApp::implementation
// this as a MTA, before the app is Create()'d
WINRT_ASSERT(_loadedInitialSettings);

_root->ShowDialog({ this, &AppLogic::_ShowDialog });
_root->DialogPresenter(*this);

// In UWP mode, we cannot handle taking over the title bar for tabs,
// so this setting is overridden to false no matter what the preference is.
Expand Down Expand Up @@ -277,17 +278,18 @@ namespace winrt::TerminalApp::implementation
// - Only one dialog can be visible at a time. If another dialog is visible
// when this is called, nothing happens.
// Arguments:
// sender: unused
// dialog: the dialog object that is going to show up
fire_and_forget AppLogic::_ShowDialog(const winrt::Windows::Foundation::IInspectable& sender, winrt::Windows::UI::Xaml::Controls::ContentDialog dialog)
// - dialog: the dialog object that is going to show up
// Return value:
// - an IAsyncOperation with the dialog result
winrt::Windows::Foundation::IAsyncOperation<ContentDialogResult> AppLogic::ShowDialog(winrt::Windows::UI::Xaml::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)
{
// Another dialog is visible.
return;
co_return ContentDialogResult::None;
}

// IMPORTANT: This is necessary as documented in the ContentDialog MSDN docs.
Expand Down Expand Up @@ -321,7 +323,7 @@ namespace winrt::TerminalApp::implementation
auto loadedRevoker{ dialog.Loaded(winrt::auto_revoke, themingLambda) }; // if it's not yet in the tree

// Display the dialog.
co_await dialog.ShowAsync(Controls::ContentDialogPlacement::Popup);
return co_await dialog.ShowAsync(Controls::ContentDialogPlacement::Popup);
DHowett marked this conversation as resolved.
Show resolved Hide resolved

// After the dialog is dismissed, the dialog lock (held by `lock`) will
// be released so another can be shown
Expand All @@ -333,7 +335,7 @@ namespace winrt::TerminalApp::implementation
// as the title and first content of the dialog, then also displays a
// message for whatever exception was found while validating the settings.
// - Only one dialog can be visible at a time. If another dialog is visible
// when this is called, nothing happens. See _ShowDialog for details
// when this is called, nothing happens. See ShowDialog for details
// Arguments:
// - titleKey: The key to use to lookup the title text from our resources.
// - contentKey: The key to use to lookup the content text from our resources.
Expand Down Expand Up @@ -378,15 +380,15 @@ namespace winrt::TerminalApp::implementation
dialog.CloseButtonText(buttonText);
dialog.DefaultButton(Controls::ContentDialogButton::Close);

_ShowDialog(nullptr, dialog);
ShowDialog(dialog);
}

// Method Description:
// - Displays a dialog for warnings found while loading or validating the
// settings. Displays messages for whatever warnings were found while
// validating the settings.
// - Only one dialog can be visible at a time. If another dialog is visible
// when this is called, nothing happens. See _ShowDialog for details
// when this is called, nothing happens. See ShowDialog for details
void AppLogic::_ShowLoadWarningsDialog()
{
auto title = RS_(L"SettingsValidateErrorTitle");
Expand Down Expand Up @@ -437,7 +439,7 @@ namespace winrt::TerminalApp::implementation
dialog.CloseButtonText(buttonText);
dialog.DefaultButton(Controls::ContentDialogButton::Close);

_ShowDialog(nullptr, dialog);
ShowDialog(dialog);
}

// Method Description:
Expand Down
3 changes: 2 additions & 1 deletion src/cascadia/TerminalApp/AppLogic.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ namespace winrt::TerminalApp::implementation

void WindowCloseButtonClicked();

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

// -------------------------------- WinRT Events ---------------------------------
DECLARE_EVENT_WITH_TYPED_EVENT_HANDLER(RequestedThemeChanged, _requestedThemeChangedHandlers, winrt::Windows::Foundation::IInspectable, winrt::Windows::UI::Xaml::ElementTheme);

Expand Down Expand Up @@ -79,7 +81,6 @@ namespace winrt::TerminalApp::implementation
::TerminalApp::AppCommandlineArgs _appArgs;
int _ParseArgs(winrt::array_view<const hstring>& args);

fire_and_forget _ShowDialog(const winrt::Windows::Foundation::IInspectable& sender, winrt::Windows::UI::Xaml::Controls::ContentDialog dialog);
void _ShowLoadErrorsDialog(const winrt::hstring& titleKey, const winrt::hstring& contentKey, HRESULT settingsLoadedResult);
void _ShowLoadWarningsDialog();

Expand Down
6 changes: 5 additions & 1 deletion src/cascadia/TerminalApp/AppLogic.idl
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ namespace TerminalApp
FullscreenMode,
};

[default_interface] runtimeclass AppLogic : IDirectKeyListener
[default_interface] runtimeclass AppLogic : IDirectKeyListener, IDialogPresenter
{
AppLogic();

Expand Down Expand Up @@ -50,6 +50,10 @@ namespace TerminalApp
void TitlebarClicked();
void WindowCloseButtonClicked();

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

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
14 changes: 7 additions & 7 deletions src/cascadia/TerminalApp/Resources/en-US/Resources.resw
Original file line number Diff line number Diff line change
Expand Up @@ -350,25 +350,25 @@
<data name="LargePasteDialog.CloseButtonText" xml:space="preserve">
<value>Cancel</value>
</data>
<data name="LargePasteDialog.Content" xml:space="preserve">
<data name="LargePasteDialog.Content" xml:space="preserve">
<value>You are about to paste text that is longer than 5 KiB. This can make the terminal unresponsive while the text is being received on the other end. Are you sure you want to continue?</value>
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
<value>You are about to paste text that is longer than 5 KiB. This can make the terminal unresponsive while the text is being received on the other end. Are you sure you want to continue?</value>
<value>You are about to paste text that is longer than 5 KiB. Are you sure you want to continue?</value>

@cinnamon-msft for confirmation on the wording here though.

Copy link
Contributor

Choose a reason for hiding this comment

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

@zadjii-msft's wording looks great

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay actually, can we change "Are you sure you want to continue?" to "Do you wish to continue?" to match the other message?

</data>
<data name="LargePasteDialog.PrimaryButtonText" xml:space="preserve">
<data name="LargePasteDialog.PrimaryButtonText" xml:space="preserve">
<value>Paste anyway</value>
</data>
<data name="LargePasteDialog.Title" xml:space="preserve">
<data name="LargePasteDialog.Title" xml:space="preserve">
<value>Warning</value>
</data>
<data name="MultiLinePasteDialog.CloseButtonText" xml:space="preserve">
<data name="MultiLinePasteDialog.CloseButtonText" xml:space="preserve">
<value>Cancel</value>
</data>
<data name="MultiLinePasteDialog.Content" xml:space="preserve">
<data name="MultiLinePasteDialog.Content" xml:space="preserve">
<value>You are about to paste text that contains the "new line" character. If you paste text that contains the "new line" character into a shell, one or more command(s) will be executed automatically upon paste. Are you sure you want to continue?</value>
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
<value>You are about to paste text that contains the "new line" character. If you paste text that contains the "new line" character into a shell, one or more command(s) will be executed automatically upon paste. Are you sure you want to continue?</value>
<value>You are about to paste text that contains the "new line" character. If you paste text that contains the "new line" character into a shell, one or more command(s) might be executed automatically upon paste. Are you sure you want to continue?</value>

again, @cinnamon-msft to ACK this wording

Copy link
Member

@DHowett DHowett Jun 24, 2020

Choose a reason for hiding this comment

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

I might rephrase as "You are about to paste text that contains multiple lines. If you paste this text into your shell, it may result in the unexpected execution of commands. Are you sure you want to continue?"

edit: "are you sure you wish to continue?" ("wish" stuck with me a bit more than "want")

Copy link
Member

Choose a reason for hiding this comment

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

That's my go at wordsmithing

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I like:

You are about to paste text that contains multiple lines. If you paste this text into your shell, it may result in the unexpected execution of commands. Do you wish to continue?

</data>
<data name="MultiLinePasteDialog.PrimaryButtonText" xml:space="preserve">
<data name="MultiLinePasteDialog.PrimaryButtonText" xml:space="preserve">
<value>Paste anyway</value>
</data>
<data name="MultiLinePasteDialog.Title" xml:space="preserve">
<data name="MultiLinePasteDialog.Title" xml:space="preserve">
<value>Warning</value>
</data>
</root>
102 changes: 49 additions & 53 deletions src/cascadia/TerminalApp/TerminalPage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
using namespace winrt;
using namespace winrt::Windows::Foundation::Collections;
using namespace winrt::Windows::UI::Xaml;
using namespace winrt::Windows::UI::Xaml::Controls;
using namespace winrt::Windows::UI::Core;
using namespace winrt::Windows::System;
using namespace winrt::Windows::ApplicationModel::DataTransfer;
Expand Down Expand Up @@ -246,7 +247,10 @@ namespace winrt::TerminalApp::implementation
// Notes link, and privacy policy link.
void TerminalPage::_ShowAboutDialog()
{
_showDialogHandlers(*this, FindName(L"AboutDialog").try_as<WUX::Controls::ContentDialog>());
if (auto presenter{ _dialogPresenter.get() })
{
presenter.ShowDialog(FindName(L"AboutDialog").try_as<WUX::Controls::ContentDialog>());
}
}

winrt::hstring TerminalPage::ApplicationDisplayName()
Expand Down Expand Up @@ -285,7 +289,10 @@ namespace winrt::TerminalApp::implementation
// when this is called, nothing happens. See _ShowDialog for details
void TerminalPage::_ShowCloseWarningDialog()
{
_showDialogHandlers(*this, FindName(L"CloseAllDialog").try_as<WUX::Controls::ContentDialog>());
if (auto presenter{ _dialogPresenter.get() })
{
presenter.ShowDialog(FindName(L"CloseAllDialog").try_as<WUX::Controls::ContentDialog>());
}
}

// Method Description:
Expand All @@ -296,9 +303,13 @@ namespace winrt::TerminalApp::implementation
// of a command.
// - Only one dialog can be visible at a time. If another dialog is visible
// when this is called, nothing happens. See _ShowDialog for details
void TerminalPage::_ShowMultiLinePasteWarningDialog()
winrt::Windows::Foundation::IAsyncOperation<ContentDialogResult> TerminalPage::_ShowMultiLinePasteWarningDialog()
{
_showDialogHandlers(*this, FindName(L"MultiLinePasteDialog").try_as<WUX::Controls::ContentDialog>());
if (auto presenter{ _dialogPresenter.get() })
{
co_return co_await presenter.ShowDialog(FindName(L"MultiLinePasteDialog").try_as<WUX::Controls::ContentDialog>());
DHowett marked this conversation as resolved.
Show resolved Hide resolved
}
co_return ContentDialogResult::None;
}

// Method Description:
Expand All @@ -307,9 +318,13 @@ namespace winrt::TerminalApp::implementation
// paste it but pressed the paste shortcut by accident.
// - Only one dialog can be visible at a time. If another dialog is visible
// when this is called, nothing happens. See _ShowDialog for details
void TerminalPage::_ShowLargePasteWarningDialog()
winrt::Windows::Foundation::IAsyncOperation<ContentDialogResult> TerminalPage::_ShowLargePasteWarningDialog()
{
_showDialogHandlers(*this, FindName(L"LargePasteDialog").try_as<WUX::Controls::ContentDialog>());
if (auto presenter{ _dialogPresenter.get() })
{
co_return co_await presenter.ShowDialog(FindName(L"LargePasteDialog").try_as<WUX::Controls::ContentDialog>());
}
co_return ContentDialogResult::None;
}

// Method Description:
Expand Down Expand Up @@ -1535,33 +1550,35 @@ namespace winrt::TerminalApp::implementation
}
}

co_await winrt::resume_foreground(Dispatcher());

const bool hasNewLine = std::find(text.cbegin(), text.cend(), L'\n') != text.cend();
const bool warnMultiLine = hasNewLine && _settings->GlobalSettings().WarnAboutMultiLinePaste();

constexpr const std::size_t minimumSizeForWarning = 1024 * 5; // 5 KiB
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put 5 KiB here for now, but I don't know what should be the optimal threshold for the warning.

Copy link
Member

Choose a reason for hiding this comment

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

I think five kilobytes is fine!

const bool warnLargeText = text.size() > minimumSizeForWarning &&
_settings->GlobalSettings().WarnAboutLargePaste();

if (!warnMultiLine && !warnLargeText)
if (warnMultiLine || warnLargeText)
{
eventArgs.HandleClipboardData(text);
co_return;
}
co_await winrt::resume_foreground(Dispatcher());

_acceptPaste = [=]() {
eventArgs.HandleClipboardData(text);
};
ContentDialogResult warningResult;
if (warnMultiLine)
{
warningResult = co_await _ShowMultiLinePasteWarningDialog();
}
else if (warnLargeText)
{
warningResult = co_await _ShowLargePasteWarningDialog();
}

if (warnMultiLine)
{
_ShowMultiLinePasteWarningDialog();
}
else if (warnLargeText)
{
_ShowLargePasteWarningDialog();
if (warningResult != ContentDialogResult::Primary)
{
// user rejected the paste
co_return;
}
}

eventArgs.HandleClipboardData(text);
}
CATCH_LOG();
}
Expand Down Expand Up @@ -1765,36 +1782,6 @@ namespace winrt::TerminalApp::implementation
_CloseAllTabs();
}

// Method Description:
// - Called when the user wants to paste the text anyway after a paste
// warning.
// - Sends the clipboard's text to the `TermControl`.
// Arguments:
// - sender: unused
// - ContentDialogButtonClickEventArgs: unused
void TerminalPage::_AcceptPasteButtonOnClick(WUX::Controls::ContentDialog /* sender */,
WUX::Controls::ContentDialogButtonClickEventArgs /* eventArgs*/)
{
if (_acceptPaste)
{
_acceptPaste();
_acceptPaste = nullptr;
}
}

// Method Description:
// - Called when the user closes a warning dialog about pasting text.
// - Destroys the callback to send the clipboard's text to the
// `TermControl` because the user cancelled the paste operation.
// Arguments:
// - sender: unused
// - ContentDialogButtonClickEventArgs: unused
void TerminalPage::_RejectPasteButtonOnClick(WUX::Controls::ContentDialog /* sender */,
WUX::Controls::ContentDialogButtonClickEventArgs /* eventArgs*/)
{
_acceptPaste = nullptr;
}

// Method Description:
// - Hook up keybindings, and refresh the UI of the terminal.
// This includes update the settings of all the tabs according
Expand Down Expand Up @@ -1867,6 +1854,16 @@ namespace winrt::TerminalApp::implementation
_startupActions = actions;
}

winrt::TerminalApp::IDialogPresenter TerminalPage::DialogPresenter() const
{
return _dialogPresenter.get();
}

void TerminalPage::DialogPresenter(winrt::TerminalApp::IDialogPresenter dialogPresenter)
{
_dialogPresenter = dialogPresenter;
}

// Method Description:
// - This is the method that App will call when the titlebar
// has been clicked. It dismisses any open flyouts.
Expand Down Expand Up @@ -2096,6 +2093,5 @@ namespace winrt::TerminalApp::implementation
DEFINE_EVENT_WITH_TYPED_EVENT_HANDLER(TerminalPage, TitleChanged, _titleChangeHandlers, winrt::Windows::Foundation::IInspectable, winrt::hstring);
DEFINE_EVENT_WITH_TYPED_EVENT_HANDLER(TerminalPage, LastTabClosed, _lastTabClosedHandlers, winrt::Windows::Foundation::IInspectable, winrt::TerminalApp::LastTabClosedEventArgs);
DEFINE_EVENT_WITH_TYPED_EVENT_HANDLER(TerminalPage, SetTitleBarContent, _setTitleBarContentHandlers, winrt::Windows::Foundation::IInspectable, UIElement);
DEFINE_EVENT_WITH_TYPED_EVENT_HANDLER(TerminalPage, ShowDialog, _showDialogHandlers, winrt::Windows::Foundation::IInspectable, WUX::Controls::ContentDialog);
DEFINE_EVENT_WITH_TYPED_EVENT_HANDLER(TerminalPage, ToggleFullscreen, _toggleFullscreenHandlers, winrt::Windows::Foundation::IInspectable, winrt::TerminalApp::ToggleFullscreenEventArgs);
}
15 changes: 8 additions & 7 deletions src/cascadia/TerminalApp/TerminalPage.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,13 @@ namespace winrt::TerminalApp::implementation

void SetStartupActions(std::deque<winrt::TerminalApp::ActionAndArgs>& actions);

winrt::TerminalApp::IDialogPresenter DialogPresenter() const;
void DialogPresenter(winrt::TerminalApp::IDialogPresenter dialogPresenter);

// -------------------------------- WinRT Events ---------------------------------
DECLARE_EVENT_WITH_TYPED_EVENT_HANDLER(TitleChanged, _titleChangeHandlers, winrt::Windows::Foundation::IInspectable, winrt::hstring);
DECLARE_EVENT_WITH_TYPED_EVENT_HANDLER(LastTabClosed, _lastTabClosedHandlers, winrt::Windows::Foundation::IInspectable, winrt::TerminalApp::LastTabClosedEventArgs);
DECLARE_EVENT_WITH_TYPED_EVENT_HANDLER(SetTitleBarContent, _setTitleBarContentHandlers, winrt::Windows::Foundation::IInspectable, winrt::Windows::UI::Xaml::UIElement);
DECLARE_EVENT_WITH_TYPED_EVENT_HANDLER(ShowDialog, _showDialogHandlers, winrt::Windows::Foundation::IInspectable, winrt::Windows::UI::Xaml::Controls::ContentDialog);
DECLARE_EVENT_WITH_TYPED_EVENT_HANDLER(ToggleFullscreen, _toggleFullscreenHandlers, winrt::Windows::Foundation::IInspectable, winrt::TerminalApp::ToggleFullscreenEventArgs);
TYPED_EVENT(Initialized, winrt::Windows::Foundation::IInspectable, winrt::Windows::UI::Xaml::RoutedEventArgs);

Expand Down Expand Up @@ -87,6 +89,9 @@ namespace winrt::TerminalApp::implementation
std::optional<int> _rearrangeFrom;
std::optional<int> _rearrangeTo;

// use a weak reference to prevent circular dependency with AppLogic
winrt::weak_ref<winrt::TerminalApp::IDialogPresenter> _dialogPresenter;

winrt::com_ptr<ShortcutActionDispatch> _actionDispatch{ winrt::make_self<ShortcutActionDispatch>() };

winrt::Windows::UI::Xaml::Controls::Grid::LayoutUpdated_revoker _layoutUpdatedRevoker;
Expand All @@ -95,12 +100,10 @@ namespace winrt::TerminalApp::implementation
std::deque<winrt::TerminalApp::ActionAndArgs> _startupActions;
winrt::fire_and_forget _ProcessStartupActions();

std::function<void()> _acceptPaste;

void _ShowAboutDialog();
void _ShowCloseWarningDialog();
void _ShowMultiLinePasteWarningDialog();
void _ShowLargePasteWarningDialog();
winrt::Windows::Foundation::IAsyncOperation<winrt::Windows::UI::Xaml::Controls::ContentDialogResult> _ShowMultiLinePasteWarningDialog();
winrt::Windows::Foundation::IAsyncOperation<winrt::Windows::UI::Xaml::Controls::ContentDialogResult> _ShowLargePasteWarningDialog();

void _CreateNewTabFlyout();
void _OpenNewTabDropdown();
Expand All @@ -112,8 +115,6 @@ namespace winrt::TerminalApp::implementation
void _FeedbackButtonOnClick(const IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& eventArgs);
void _AboutButtonOnClick(const IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& eventArgs);
void _CloseWarningPrimaryButtonOnClick(Windows::UI::Xaml::Controls::ContentDialog sender, Windows::UI::Xaml::Controls::ContentDialogButtonClickEventArgs eventArgs);
void _AcceptPasteButtonOnClick(Windows::UI::Xaml::Controls::ContentDialog sender, Windows::UI::Xaml::Controls::ContentDialogButtonClickEventArgs eventArgs);
void _RejectPasteButtonOnClick(Windows::UI::Xaml::Controls::ContentDialog sender, Windows::UI::Xaml::Controls::ContentDialogButtonClickEventArgs eventArgs);
void _ThirdPartyNoticesOnClick(const IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& eventArgs);

void _HookupKeyBindings(TerminalApp::AppKeyBindings bindings) noexcept;
Expand Down
Loading