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
2 changes: 2 additions & 0 deletions doc/cascadia/SettingsSchema.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ Properties listed below affect the entire window, regardless of the profile sett
| `alwaysShowTabs` | _Required_ | Boolean | `true` | When set to `true`, tabs are always displayed. When set to `false` and `showTabsInTitlebar` is set to `false`, tabs only appear after typing <kbd>Ctrl</kbd> + <kbd>T</kbd>. |
| `copyOnSelect` | Optional | Boolean | `false` | When set to `true`, a selection is immediately copied to your clipboard upon creation. When set to `false`, the selection persists and awaits further action. |
| `copyFormatting` | Optional | Boolean | `false` | When set to `true`, the color and font formatting of selected text is also copied to your clipboard. When set to `false`, only plain text is copied to your clipboard. |
| `largePasteWarning` | Optional | Boolean | `true` | When set to `true`, trying to paste text with more than 5 KiB of characters will display a warning asking you whether to continue or not with the paste. |
| `multiLinePasteWarning` | Optional | Boolean | `true` | When set to `true`, trying to paste text with a _new line_ character will display a warning asking you whether to continue or not with the paste. |
Comment on lines +12 to +13
Copy link
Member

Choose a reason for hiding this comment

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

For Discussion:
I'm wondering if we'll be adding more "warning" features over time. If so, what's everybody's thoughts on having a "warning." prefix? So these settings would be called warning.largePaste and warning.multiLinePaste.

And we could override some others like confirmCloseAllTabs to be warning.closeWithMultipleTabs.

Copy link
Member

Choose a reason for hiding this comment

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

oh wow, I actually like that a lot. I'd love another ACK on that design, but I'm on board.

Copy link
Member

Choose a reason for hiding this comment

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

I'll ACK that design. Should we do it in post so we can let greg904 in peace to merge his PR?

Making it a separate PR allows us to also add a compatibility lookup for warning.closeAllTabs

Copy link
Contributor

Choose a reason for hiding this comment

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

I third this :)

| `defaultProfile` | _Required_ | String | PowerShell guid | Sets the default profile. Opens by typing <kbd>Ctrl</kbd> + <kbd>T</kbd> or by clicking the '+' icon. The guid of the desired default profile is used as the value. |
| `initialCols` | _Required_ | Integer | `120` | The number of columns displayed in the window upon first load. |
| `initialPosition` | Optional | String | `","` | The position of the top left corner of the window upon first load. On a system with multiple displays, these coordinates are relative to the top left of the primary display. If `launchMode` is set to `"maximized"`, the window will be maximized on the monitor specified by those coordinates. |
Expand Down
10 changes: 10 additions & 0 deletions doc/cascadia/profiles.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,16 @@
"description": "When set to `true`, the color and font formatting of selected text is also copied to your clipboard. When set to `false`, only plain text is copied to your clipboard.",
"type": "boolean"
},
"largePasteWarning": {
"default": true,
"description": "When set to true, trying to paste text with more than 5 KiB of characters will display a warning asking you whether to continue or not with the paste.",
"type": "boolean"
},
"multiLinePasteWarning": {
"default": true,
"description": "When set to true, trying to paste text with a \"new line\" character will display a warning asking you whether to continue or not with the paste.",
"type": "boolean"
},
"defaultProfile": {
"description": "Sets the default profile. Opens by clicking the \"+\" icon or typing the key binding assigned to \"newTab\".",
"type": "string"
Expand Down
6 changes: 6 additions & 0 deletions src/cascadia/TerminalApp/GlobalAppSettings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ static constexpr std::string_view ShowTabsInTitlebarKey{ "showTabsInTitlebar" };
static constexpr std::string_view WordDelimitersKey{ "wordDelimiters" };
static constexpr std::string_view CopyOnSelectKey{ "copyOnSelect" };
static constexpr std::string_view CopyFormattingKey{ "copyFormatting" };
static constexpr std::string_view WarnAboutLargePasteKey{ "largePasteWarning" };
static constexpr std::string_view WarnAboutMultiLinePasteKey{ "multiLinePasteWarning" };
static constexpr std::string_view LaunchModeKey{ "launchMode" };
static constexpr std::string_view ConfirmCloseAllKey{ "confirmCloseAllTabs" };
static constexpr std::string_view SnapToGridOnResizeKey{ "snapToGridOnResize" };
Expand Down Expand Up @@ -190,6 +192,10 @@ void GlobalAppSettings::LayerJson(const Json::Value& json)

JsonUtils::GetBool(json, CopyFormattingKey, _CopyFormatting);

JsonUtils::GetBool(json, WarnAboutLargePasteKey, _WarnAboutLargePaste);

JsonUtils::GetBool(json, WarnAboutMultiLinePasteKey, _WarnAboutMultiLinePaste);

if (auto launchMode{ json[JsonKey(LaunchModeKey)] })
{
_LaunchMode = _ParseLaunchMode(GetWstringFromJson(launchMode));
Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalApp/GlobalAppSettings.h
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ class TerminalApp::GlobalAppSettings final
GETSET_PROPERTY(std::wstring, WordDelimiters); // default value set in constructor
GETSET_PROPERTY(bool, CopyOnSelect, false);
GETSET_PROPERTY(bool, CopyFormatting, false);
GETSET_PROPERTY(bool, WarnAboutLargePaste, true);
GETSET_PROPERTY(bool, WarnAboutMultiLinePaste, true);
Comment on lines +78 to +79
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should these be enabled by default?

Copy link
Member

Choose a reason for hiding this comment

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

Yep 😊

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 I was initially reluctant to enabled by default for both, but I've sat on it for a bit and I concur, enabled by default seems sensible.

GETSET_PROPERTY(LaunchPosition, InitialPosition);
GETSET_PROPERTY(winrt::TerminalApp::LaunchMode, LaunchMode, winrt::TerminalApp::LaunchMode::DefaultMode);
GETSET_PROPERTY(bool, SnapToGridOnResize, true);
Expand Down
24 changes: 24 additions & 0 deletions src/cascadia/TerminalApp/Resources/en-US/Resources.resw
Original file line number Diff line number Diff line change
Expand Up @@ -347,4 +347,28 @@
<value>Command Prompt</value>
<comment>This is the name of "Command Prompt", as localized in Windows. The localization here should match the one in the Windows product for "Command Prompt"</comment>
</data>
<data name="LargePasteDialog.CloseButtonText" xml:space="preserve">
<value>Cancel</value>
</data>
<data name="LargePasteDialog.Content" xml:space="preserve">
beviu marked this conversation as resolved.
Show resolved Hide resolved
<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">
beviu marked this conversation as resolved.
Show resolved Hide resolved
<value>Paste anyway</value>
</data>
<data name="LargePasteDialog.Title" xml:space="preserve">
beviu marked this conversation as resolved.
Show resolved Hide resolved
<value>Warning</value>
</data>
<data name="MultiLinePasteDialog.CloseButtonText" xml:space="preserve">
beviu marked this conversation as resolved.
Show resolved Hide resolved
<value>Cancel</value>
</data>
<data name="MultiLinePasteDialog.Content" xml:space="preserve">
beviu marked this conversation as resolved.
Show resolved Hide resolved
<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">
beviu marked this conversation as resolved.
Show resolved Hide resolved
<value>Paste anyway</value>
</data>
<data name="MultiLinePasteDialog.Title" xml:space="preserve">
beviu marked this conversation as resolved.
Show resolved Hide resolved
<value>Warning</value>
</data>
</root>
110 changes: 92 additions & 18 deletions src/cascadia/TerminalApp/TerminalPage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,30 @@ namespace winrt::TerminalApp::implementation
_showDialogHandlers(*this, FindName(L"CloseAllDialog").try_as<WUX::Controls::ContentDialog>());
}

// Method Description:
// - Displays a dialog to warn the user about the fact that the text that
// they are trying to paste contains the "new line" character which can
// have the effect of starting commands without the user's knowledge if
// it is pasted on a shell where the "new line" character marks the end
// 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()
{
_showDialogHandlers(*this, FindName(L"MultiLinePasteDialog").try_as<WUX::Controls::ContentDialog>());
}

// Method Description:
// - Displays a dialog to warn the user about the fact that the text that
// they are trying to paste is very long, in case they did not mean to
// 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()
{
_showDialogHandlers(*this, FindName(L"LargePasteDialog").try_as<WUX::Controls::ContentDialog>());
}

// Method Description:
// - Builds the flyout (dropdown) attached to the new tab button, and
// attaches it to the button. Populates the flyout with one entry per
Expand Down Expand Up @@ -1472,26 +1496,19 @@ namespace winrt::TerminalApp::implementation
CATCH_LOG();
}

// Method Description:
// - Fires an async event to get data from the clipboard, and paste it to
// the terminal. Triggered when the Terminal Control requests clipboard
// data with it's PasteFromClipboard event.
// Arguments:
// - eventArgs: the PasteFromClipboard event sent from the TermControl
winrt::fire_and_forget TerminalPage::_PasteFromClipboardHandler(const IInspectable /*sender*/,
const PasteFromClipboardEventArgs eventArgs)
{
co_await winrt::resume_foreground(Dispatcher(), CoreDispatcherPriority::High);

TerminalPage::PasteFromClipboard(eventArgs);
}

// Function Description:
// - Copies and processes the text data from the Windows Clipboard.
// Does some of this in a background thread, as to not hang/crash the UI thread.
// - This function is called when the `TermControl` requests that we send
// it the clipboard's content.
// - Retrieves the data from the Windows Clipboard and converts it to text.
// - Shows warnings if the clipboard is too big or contains multiple lines
// of text.
// - Sends the text back to the TermControl through the event's
// `HandleClipboardData` member function.
// - Does some of this in a background thread, as to not hang/crash the UI thread.
// Arguments:
// - eventArgs: the PasteFromClipboard event sent from the TermControl
fire_and_forget TerminalPage::PasteFromClipboard(PasteFromClipboardEventArgs eventArgs)
fire_and_forget TerminalPage::_PasteFromClipboardHandler(const IInspectable /*sender*/,
const PasteFromClipboardEventArgs eventArgs)
{
const DataPackageView data = Clipboard::GetContent();

Expand All @@ -1517,7 +1534,34 @@ namespace winrt::TerminalApp::implementation
text = item.Path();
}
}
eventArgs.HandleClipboardData(text);

co_await winrt::resume_foreground(Dispatcher());
beviu marked this conversation as resolved.
Show resolved Hide resolved

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)
{
eventArgs.HandleClipboardData(text);
co_return;
}

_acceptPaste = [=]() {
beviu marked this conversation as resolved.
Show resolved Hide resolved
eventArgs.HandleClipboardData(text);
beviu marked this conversation as resolved.
Show resolved Hide resolved
};

if (warnMultiLine)
{
_ShowMultiLinePasteWarningDialog();
}
else if (warnLargeText)
{
_ShowLargePasteWarningDialog();
}
}
CATCH_LOG();
}
Expand Down Expand Up @@ -1721,6 +1765,36 @@ 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
7 changes: 6 additions & 1 deletion src/cascadia/TerminalApp/TerminalPage.h
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,12 @@ namespace winrt::TerminalApp::implementation
std::deque<winrt::TerminalApp::ActionAndArgs> _startupActions;
winrt::fire_and_forget _ProcessStartupActions();

std::function<void()> _acceptPaste;
beviu marked this conversation as resolved.
Show resolved Hide resolved

void _ShowAboutDialog();
void _ShowCloseWarningDialog();
void _ShowMultiLinePasteWarningDialog();
void _ShowLargePasteWarningDialog();

void _CreateNewTabFlyout();
void _OpenNewTabDropdown();
Expand All @@ -108,6 +112,8 @@ 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 Expand Up @@ -149,7 +155,6 @@ namespace winrt::TerminalApp::implementation
const Microsoft::Terminal::TerminalControl::PasteFromClipboardEventArgs eventArgs);
bool _CopyText(const bool trimTrailingWhitespace);
void _PasteText();
static fire_and_forget PasteFromClipboard(winrt::Microsoft::Terminal::TerminalControl::PasteFromClipboardEventArgs eventArgs);

fire_and_forget _LaunchSettings(const winrt::TerminalApp::SettingsTarget target);

Expand Down
18 changes: 18 additions & 0 deletions src/cascadia/TerminalApp/TerminalPage.xaml
Original file line number Diff line number Diff line change
Expand Up @@ -53,5 +53,23 @@ the MIT License. See LICENSE in the project root for license information. -->
DefaultButton="Primary"
PrimaryButtonClick="_CloseWarningPrimaryButtonOnClick">
</ContentDialog>

<ContentDialog
x:Load="False"
x:Name="MultiLinePasteDialog"
x:Uid="MultiLinePasteDialog"
DefaultButton="Primary"
PrimaryButtonClick="_AcceptPasteButtonOnClick"
CloseButtonClick="_RejectPasteButtonOnClick">
</ContentDialog>

<ContentDialog
x:Load="False"
x:Name="LargePasteDialog"
x:Uid="LargePasteDialog"
DefaultButton="Primary"
PrimaryButtonClick="_AcceptPasteButtonOnClick"
CloseButtonClick="_RejectPasteButtonOnClick">
</ContentDialog>
</Grid>
</Page>