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

Added: Copy text without dismissing the selection #15552

Merged
merged 8 commits into from Jul 6, 2023
Merged

Added: Copy text without dismissing the selection #15552

merged 8 commits into from Jul 6, 2023

Conversation

gonzalo-garcian
Copy link
Contributor

@gonzalo-garcian gonzalo-garcian commented Jun 14, 2023 •
edited by carlos-zamora

Summary of the Pull Request

Adds a dismiss selection option to the "copy" action.

PR Checklist

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Task It's a feature request, but it doesn't really need a major design. Area-Settings Issues related to settings and customizability, for console or terminal Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal. labels Jun 14, 2023
@gonzalo-garcian gonzalo-garcian marked this pull request as ready for review June 14, 2023 23:55
@gonzalo-garcian
Copy link
Contributor Author

@microsoft-github-policy-service agree

carlos-zamora
carlos-zamora requested changes Jun 23, 2023
View reviewed changes
Copy link
Member

@carlos-zamora carlos-zamora left a comment •
edited

Choose a reason for hiding this comment

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

Looks great! I even tested it out myself. Thanks! There's a few more things you need to do before I sign off on this though:

src/cascadia/TerminalSettingsModel/ActionArgs.cpp Outdated
Comment on lines 190 to 198
if (DismissSelection())
{
ss << RS_(L"DismissSelectionCommandKey").c_str();
}
else
{
ss << RS_(L"DismissSelectionFalseCommandKey").c_str();
}

Copy link
Member

Choose a reason for hiding this comment

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

You're going to have to move this somewhere else in this method. So, here's how this function works:

  • if singleLine: true --> the command palette displays the action as "Copy text as a single line"
  • otherwise --> "Copy text"
  • if copyFormatting is set, we append "copyFormatting: "

Honestly, the easiest approach I recommend is appending "dismissSelection: false" to the string if dismissSelection: false.

@DHowett @nguyen-dows Thoughts? Y'all are generally good at wordsmithing stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The truth is that I have programmed it a bit by intuition. Where do these strings appear/be used for? I haven't had much time to investigate. I guess I'll wait for others to comment on this.

Copy link
Member

Choose a reason for hiding this comment

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

No problem! haha GenerateName() spits out the localized string version of the action. These get displayed in the command palette and in the actions page of the settings UI.

src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw Outdated
Comment on lines 702 to 709
<data name="DismissSelectionCommandKey" xml:space="preserve">
<value>DismissSelectionCommandKey</value>
<comment>DismissSelectionCommandKey</comment>
</data>
<data name="DismissSelectionFalseCommandKey" xml:space="preserve">
<value>DismissSelectionFalseCommandKey</value>
<comment>DismissSelectionFalseCommandKey</comment>
</data>
Copy link
Member

Choose a reason for hiding this comment

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

Building off of my comments in ActionArgs.cpp.

If you go with my suggestion of dismissSelection: false, you can delete these changes/keys.
If we end up going with an approach that'll be translated (i.e. how the CopyText key is translated to "Copy text"), you'll want to keep these keys, but replace the content of <value> with the translated string.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 23, 2023
@carlos-zamora
Copy link
Member

Also, what's the repro for this?

The first Ctrl + C throw an exeception.

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 25, 2023
@gonzalo-garcian
Copy link
Contributor Author

Also, what's the repro for this?

The first Ctrl + C throw an exeception.

I think it's a Visual Studio thing because I've done some testing and it only happens if you start typing in the terminal when Visual Studio hasn't fully loaded the debug menus yet. When I have time I'll upload some gifs showing it.

carlos-zamora
carlos-zamora approved these changes Jul 5, 2023
View reviewed changes
Copy link
Member

@carlos-zamora carlos-zamora left a comment

Choose a reason for hiding this comment

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

Thanks! I'll go ahead and resolve the comment I made and clean up the PR body before merging it. Nice work 😊

src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw Outdated Show resolved Hide resolved
DHowett
DHowett previously requested changes Jul 5, 2023
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.

taking some time to look at this myself

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 5, 2023
zadjii-msft
zadjii-msft approved these changes Jul 6, 2023
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.

Exactly what I would have done, thanks!

@zadjii-msft
Copy link
Member

image

@zadjii-msft zadjii-msft dismissed DHowett’s stale review July 6, 2023 16:37

cause you said so

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 6, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot merged commit 0c64b8a into microsoft:main Jul 6, 2023
12 checks passed
tusharsnx pushed a commit to tusharsnx/terminal that referenced this pull request Jul 12, 2023
@carlos-zamora @tusharsnx
Added: Copy text without dismissing the selection (microsoft#15552)
b37dc8d 
## Summary of the Pull Request
Adds a dismiss selection option to the "copy" action.

## PR Checklist
- [x] Closes microsoft#15371
- [x] Tests added/passed
- [x] Documentation updated
- If checked, please file a pull request on [our docs
repo](https://github.com/MicrosoftDocs/terminal) and link it here:
MicrosoftDocs/terminal#686
- [x] Schema updated (if necessary)

---------

Co-authored-by: Carlos Zamora <carlos.zamora@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Delano5000 Delano5000 Delano5000 left review comments

@lhecker lhecker lhecker approved these changes

@carlos-zamora carlos-zamora carlos-zamora approved these changes

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

@DHowett DHowett DHowett left review comments

Assignees
No one assigned
Labels
Area-Settings Issues related to settings and customizability, for console or terminal Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) 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. Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Copy text without dismissing the selection
6 participants
@gonzalo-garcian @carlos-zamora @zadjii-msft @DHowett @lhecker @Delano5000