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.

Sign up for GitHub

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
Merged

Add settings to warn about large or multiline pastes #6631

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
618a4c4
Add settings to warn when pasting text too large or with a new line c…
Jun 22, 2020
0e515f5
Fix formatting in Resources.resw
Jun 24, 2020
2c5e78c
Make dialog presentation waitable
Jun 24, 2020
6385175
Use the IAsyncOperation returned by ShowDialog
Jun 24, 2020
5d73dc7
Use a weak reference to prevent circular dependency
Jun 24, 2020
6006394
Add 'IDialog' to spell checker's apis.txt
Jun 24, 2020
21eb079
Only `resume_foreground` if we actually need to display a warning dialog
Jun 24, 2020
b8152fc
Fix invalid name in `FindName`
Jun 24, 2020
aaca27c
Replace `return` by `co_return` to fix warning
Jun 25, 2020
0861584
Fix DialogPresenter comment
Jun 25, 2020
0f083cf
Change wording
Jun 27, 2020
33196af
Merge branch 'master' of https://github.com/microsoft/terminal into p…
Jun 27, 2020
b7dfde2
Add missing close tag in Resources.resw
Jun 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/actions/spell-check/dictionary/apis.txt
View file
Open in desktop
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
2 changes: 2 additions & 0 deletions doc/cascadia/SettingsSchema.md
View file
Open in desktop
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
View file
Open in desktop
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
22 changes: 12 additions & 10 deletions src/cascadia/TerminalApp/AppLogic.cpp
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
6 changes: 6 additions & 0 deletions src/cascadia/TerminalApp/GlobalAppSettings.cpp
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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">
<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">
<value>Paste anyway</value>
</data>
<data name="LargePasteDialog.Title" xml:space="preserve">
<value>Warning</value>
</data>
<data name="MultiLinePasteDialog.CloseButtonText" xml:space="preserve">
<value>Cancel</value>
</data>
<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 •
edited
Loading

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">
<value>Paste anyway</value>
</data>
<data name="MultiLinePasteDialog.Title" xml:space="preserve">
<value>Warning</value>
</data>
</root>
Loading