Don't copy before pasting in copyOnSelect mode #14635

zadjii-msft · 2023-01-05T20:21:39Z

Don't try to copy if we're in copyOnSelect mode, and have already copied this selection.

#14464 demonstrates a scenario where the buffer contents might have changed since the selection was made, and copying here would cause weirdness.

Also adds a couple tests for copyOnSelect.

Closes #14464

…ests

KalleOlaviNiemitalo · 2023-01-05T23:41:13Z

This changes only the handling of mouse right-clicks, right?

I have copy-on-select enabled. Occasionally, I select some text in the terminal and paste it to another program; then copy some other text from a third program and paste that too; then want to paste the text from the terminal again. In that situation, I have been able to switch back to the terminal, where the original text remains selected, and press Ctrl+Insert to copy the same text again.

Now, this PR apparently prevents copying with the right mouse button in that scenario -- well, that's okay but please don't hurt my precious Ctrl+Insert.

carlos-zamora

Bug:

ensure copy on select is enabled
enter mark mode
make a selection with mark mode
right-click to copy/paste

expected: selection is copied and pasted immediately

NOTE: similar thing happens when you make a mouse selection, then modify it using quick-edit keys (i.e. shift+right). The selected text is not copied and pasted immediately.

zadjii-msft · 2023-01-13T21:06:44Z

Lemme make sure I get this straight - when you modify the selection with mark mode (with copyOnSelect:true), the clipboard contents don't get immediately updated? They only get updated on the right-click (which we turn around and use to immediately paste)?

Hmm. Because TermControl::_TryHandleKeyBinding calls _core.TryMarkModeKeybinding then early-returns, without going through Interactivity so that _selectionNeedsToBeCopied can be updated. Hmm.

…pyOnSelect-moving-text

github-actions · 2023-01-17T15:12:20Z

@check-spelling-bot Report

🔴 Please review

See the 📂 files view or the 📜action log for details.

Unrecognized words (1)

occured

Previously acknowledged words that are now absent

Hirots inthread reingest :arrow_right:

To accept ✔️ these unrecognized words as correct and remove the previously acknowledged and now absent words, run the following commands

... in a clone of the [email protected]:microsoft/terminal.git repository
on the dev/migrie/b/14464-copyOnSelect-moving-text branch (ℹ️ how do I use this?):

curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/v0.0.21/apply.pl' |
perl - 'https://github.com/microsoft/terminal/actions/runs/3940577809/attempts/1'

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. If it doesn't work for you, you can manually add (one word per line) / remove items to expect.txt and the excludes.txt files.

If the listed items are:

... misspelled, then please correct them instead of using the command.
... names, please add them to .github/actions/spelling/allow/names.txt.
... APIs, you can add them to a file in .github/actions/spelling/allow/.
... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

If the flagged items are 🤯 false positives

If items relate to a ...

binary file (or some other file you wouldn't want to check at all).

Please add a file path to the excludes.txt file matching the containing file.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).
well-formed pattern.

If you can write a pattern that would match it,
try adding it to the patterns.txt file.

Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

Note that patterns can't match multiline strings.

ozinka · 2023-05-30T08:00:56Z

Sorry, I'm not very experienced in feature implementation workflow. Could someone explain to me which stage this issue is on?
I waited for the new release and fix but unfortunately, it's still not implemented :(

zadjii-msft · 2023-05-30T10:13:09Z

Sorry, I'm not very experienced in feature implementation workflow. Could someone explain to me which stage this issue is on? I waited for the new release and fix but unfortunately, it's still not implemented :(

No worries - this one's just in a bit of a holding pattern. There was an edge case I didn't account for in my original fix. I haven't had time to loop back on this one since. So this isn't fixed in 1.18 yet. Hopefully I can double back on this in July. (If someone else wants to help sort out the edge case above, I'd be happy to have the help)

carlos-zamora · 2023-06-10T00:07:49Z

Lemme make sure I get this straight - when you modify the selection with mark mode (with copyOnSelect:true), the clipboard contents don't get immediately updated? They only get updated on the right-click (which we turn around and use to immediately paste)?

Correct! Because if the clipboard contents got updated whenever you move the cursor, you'd copy so many " " to your clipboard! 😆

lhecker · 2023-07-10T20:55:37Z

src/cascadia/TerminalControl/ControlInteractivity.cpp

+            // copied this selection. GH#14464 demonstrates a scenario where the
+            // buffer contents might have changed since the selection was made,
+            // and copying here would cause weirdness.
+            if (_selectionNeedsToBeCopied || !copyOnSelect)


I don't quite get why this depends on CopyOnSelect... When would we ever want to copy text more than once?

ozinka · 2024-01-30T21:06:03Z

I waited for a long time this issue will be fixed and no. New version 1.20 doesn't contain fix for it. I wander why so small number of users confirmed this bug. This feature works ideal on putty, why it can't be fixed on MS Terminal? I believe it's pretty easy to fix.
I'm sorry, I'm not experienced developer, otherwhise I'd also contribute. If there is any volonteer to explain how I can do that, please guide me. I believe I can read this code to fix it.

zadjii-msft · 2024-01-30T21:14:30Z

I've been working on another project for the last 6 months and have solidly cached this out. I'll probably loop around on all these open PRs after that gets out the door, but I honestly don't remember exactly the interaction that was weird about this fix. I know that it was related to #14635 (review) but I don't recall how the code flowed to cause that anymore.

ozinka · 2024-01-30T21:25:06Z

I've been working on another project for the last 6 months and have solidly cached this out. I'll probably loop around on all these open PRs after that gets out the door, but I honestly don't remember exactly the interaction that was weird about this fix. I know that it was related to #14635 (review) but I don't recall how the code flowed to cause that anymore.

In the nutshell the problem is that when copyOnSelect is active it does it twise: onSelect and onPaste (right click). It shouldn't do copy during onPaste operation. In putty, when you select anything, putty shows selection. When you select anything somewhre else and copy it, putty clears it's own selection and after that buffer contains new selection. Which is used for paste on right click.

I've read release notes and that's a huge of work. Just hope new version also include fix for this bug. )

zadjii-msft added 3 commits January 5, 2023 12:01

this is a basic copyOnSelect test, for reference, for starting more t…

2ec30ef

…ests

some tests that fail, intentionally

45b3e8d

Fix the bug

ca658ba

ghost added Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. labels Jan 5, 2023

This comment has been minimized.

Sign in to view

zadjii-msft added this to the Terminal v1.17 milestone Jan 10, 2023

carlos-zamora requested changes Jan 11, 2023

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 11, 2023

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 13, 2023

Merge remote-tracking branch 'origin/main' into dev/migrie/b/14464-co…

eaf7215

…pyOnSelect-moving-text

carlos-zamora added a commit that referenced this pull request Jan 14, 2023

PRE-MERGE #14635 Don't copy before pasting in copyOnSelect mode

e96f6e6

zadjii-msft mentioned this pull request Jan 23, 2023

"copy on select" doesn't work good #14713

Closed

zadjii-msft self-assigned this Jan 29, 2023

zadjii-msft modified the milestones: Terminal v1.17, Terminal v1.19 May 12, 2023

lhecker reviewed Jul 10, 2023

View reviewed changes

zadjii-msft modified the milestones: Terminal v1.19, Terminal 1.21 Jan 30, 2024

dandrei mentioned this pull request Sep 13, 2024

Clipboard gets overridden when pasting to a tab with a previous selection with right click or Shift+Insert, when copy-on-select is enabled #17925

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't copy before pasting in copyOnSelect mode #14635

Don't copy before pasting in copyOnSelect mode #14635

zadjii-msft commented Jan 5, 2023

This comment has been minimized.

KalleOlaviNiemitalo commented Jan 5, 2023

carlos-zamora left a comment

zadjii-msft commented Jan 13, 2023 •

edited

Loading

github-actions bot commented Jan 17, 2023

ozinka commented May 30, 2023

zadjii-msft commented May 30, 2023

carlos-zamora commented Jun 10, 2023

lhecker Jul 10, 2023

ozinka commented Jan 30, 2024

zadjii-msft commented Jan 30, 2024

ozinka commented Jan 30, 2024 •

edited

Loading

Don't copy before pasting in copyOnSelect mode #14635

Are you sure you want to change the base?

Don't copy before pasting in copyOnSelect mode #14635

Conversation

zadjii-msft commented Jan 5, 2023

This comment has been minimized.

KalleOlaviNiemitalo commented Jan 5, 2023

carlos-zamora left a comment

Choose a reason for hiding this comment

zadjii-msft commented Jan 13, 2023 • edited Loading

github-actions bot commented Jan 17, 2023

@check-spelling-bot Report

🔴 Please review

Unrecognized words (1)

ozinka commented May 30, 2023

zadjii-msft commented May 30, 2023

carlos-zamora commented Jun 10, 2023

lhecker Jul 10, 2023

Choose a reason for hiding this comment

ozinka commented Jan 30, 2024

zadjii-msft commented Jan 30, 2024

ozinka commented Jan 30, 2024 • edited Loading

zadjii-msft commented Jan 13, 2023 •

edited

Loading

ozinka commented Jan 30, 2024 •

edited

Loading