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

Jump to bottom

Show clipboard contents in multiple line paste warning dialog #8744

Merged
6 commits merged into from Jan 12, 2021
Merged

Show clipboard contents in multiple line paste warning dialog #8744

6 commits merged into from Jan 12, 2021

Conversation

PankajBhojwani
Copy link
Contributor

@PankajBhojwani PankajBhojwani commented Jan 11, 2021 •
edited by DHowett

When we display a dialog to warn the user that they are doing a
multi-line paste, we show the clipboard contents

The contents are shown in a scroll viewer with a fixed maximum height.

Closes #7997

@ghost ghost added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Jan 11, 2021
DHowett
DHowett requested changes Jan 11, 2021
View reviewed changes
src/cascadia/TerminalApp/Resources/en-US/Resources.resw Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.xaml Show resolved Hide resolved
src/cascadia/TerminalApp/Resources/en-US/Resources.resw
@@ -405,8 +405,8 @@
<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 multiple lines. If you paste this text into your shell, it may result in the unexpected execution of commands. Do you wish to continue?</value>
<data name="MultiLineWarningText.Text" xml:space="preserve">
Copy link
Member

Choose a reason for hiding this comment

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

i wish we didn't have to change the key for this. I know why we have to, but this will trigger a re-localization for a string that is almost exactly the same.

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 11, 2021
@DHowett
Copy link
Member

DHowett commented Jan 11, 2021

Should we introduce a limit, say 10 lines, after which we insert a (...content truncated...) line at the end?

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 11, 2021
@PankajBhojwani
Copy link
Contributor Author

PankajBhojwani commented Jan 11, 2021 •
edited

Should we introduce a limit, say 10 lines, after which we insert a (...content truncated...) line at the end?

Hm I feel like since we have the scrollbar we don't need to truncate the content (and if the user does not want to see their large pastes they could just use the setting to disable multi line paste warnings)

@DHowett DHowett mentioned this pull request Jan 12, 2021
Closed
zadjii-msft
zadjii-msft approved these changes Jan 12, 2021
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

okay yea this seems good to me

DHowett
DHowett reviewed Jan 12, 2021
View reviewed changes
src/cascadia/TerminalApp/TerminalPage.xaml
</TextBlock>
<ScrollViewer
Margin="0,8,0,0"
x:Name="ClipboardContentScrollViewer"
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to name infrastructural components unless we refer to them in code 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We actually refer to this one! To reset the scroll offset (the scroll offset does not reset automatically in between calls to open the dialog)

src/cascadia/TerminalApp/TerminalPage.xaml Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.xaml Outdated Show resolved Hide resolved
DHowett
DHowett approved these changes Jan 12, 2021
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

love it!

@DHowett
Copy link
Member

DHowett commented Jan 12, 2021

Moved from body:

multiline

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 12, 2021
@ghost
Copy link

ghost commented Jan 12, 2021

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit aaf2395 into main Jan 12, 2021
@ghost ghost deleted the dev/pabhoj/show_clipboard branch January 12, 2021 23:00
@skyline75489 skyline75489 mentioned this pull request Jan 26, 2021
Closed
6 tasks
@ghost
Copy link

ghost commented Jan 28, 2021

🎉Windows Terminal Preview v1.6.10272.0 has been released which incorporates this pull request.:tada:

Handy links:

mpela81 pushed a commit to mpela81/terminal that referenced this pull request Jan 28, 2021
@PankajBhojwani @mpela81
Show clipboard contents in multiple line paste warning dialog (micros…
634a336 
…oft#8744)

When we display a dialog to warn the user that they are doing a
multi-line paste, we show the clipboard contents

The contents are shown in a scroll viewer with a fixed maximum height.

Closes microsoft#7997
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DHowett DHowett DHowett approved these changes

@zadjii-msft zadjii-msft zadjii-msft approved these changes

Assignees

@DHowett DHowett

Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Display clipboard contents in the multiple line warning dialog box when pasting into Terminal
3 participants
@PankajBhojwani @DHowett @zadjii-msft