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

Fix issue with pasting while tab renaming also sending the paste command to terminal #14669

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

marcelwgn
Copy link
Contributor

@marcelwgn marcelwgn commented Jan 11, 2023

The issue was that pasting while renaming a tab is also sending that
text to the terminal. The proposed fix is to ignore paste commands while
we are renaming a tab.

Other solutions I considered where looking for shell focus to undermine
sending paste commands (how would you paste something if the shell is
not focused) or diving deeper into the events to actually prevent it
from bubbling up so far. Due to complexity and possible problems
however, I went with just looking for renaming tabs instead.

Validation Steps Performed

  1. Start Terminal
  2. Rename tab
  3. Press Ctrl+V and see if the shell gets pasted content

Closes #14381

@ghost ghost added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Jan 11, 2023
@KalleOlaviNiemitalo
Copy link

TerminalPage::_PasteFromClipboardHandler "This function is called when the TermControl requests that we send it the clipboard's content."

Can it be fixed such that TermControl does not even receive the UI event or translate it to a paste request, if the tab is being renamed? I'm thinking, if an extension of Windows Terminal were running a script that pastes for reasons not related to UI events, then it should not be affected by whether the user is renaming a tab.

@marcelwgn
Copy link
Contributor Author

Thank you for raising this concern @KalleOlaviNiemitalo! Honestly, I'm afraid the event handling is quite complicated in that area. #12260 introduced the behavior that the KeyUp event of the tab row also get sent to the key event handler of the TerminalPage to fix some a11y issue. I think that trying to fix this issue in could potentially undo the effect of #12260.

Looking at who requests a pasting of text to the control, it seems that this is only done through UI interaction with the terminal control. It is worth noting though that this PR introduces the behavior that right clicking the terminal while renaming a tab also no longer pastes the content and instead just cancels renaming the tab. I see this as an improvement, but opinions might diverge on that matter.

@KalleOlaviNiemitalo
Copy link

How about searching for text in the terminal and pasting to the search box, does that have the same problem as in renaming the tab?

@marcelwgn
Copy link
Contributor Author

marcelwgn commented Jan 15, 2023

It seems, that the searching box seems to handle the behavior correctly.

@DHowett
Copy link
Member

DHowett commented Jan 20, 2023

I'd like to wait on this a little bit to cost out how much it would take to simply fix all of our key eventing issues. The core problem is that we're using PreviewKeyDown on TermControl, when we should be using KeyDown. Every other workaround has fallen out from there. Thanks for doing it, though, and I'm going to keep it open until we've made that assessment!

@zadjii-msft
Copy link
Member

Yo @DHowett was the cost as big as we thought?

@marcelwgn
Copy link
Contributor Author

I don't want to be pushy but I'm curious what the next steps for this PR are or if there is anything that needs adjustment 😃

@zadjii-msft zadjii-msft requested a review from DHowett August 24, 2023 10:51
@DHowett
Copy link
Member

DHowett commented Aug 24, 2023

@zadjii-msft would you mind doing that costing I mentioned? 😄

@leewilmott
Copy link

Hi guys, is there any movement on this issue?

@DHowett DHowett modified the milestones: Terminal v1.19, Terminal v1.22 Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pasting in "Rename Tab" also pastes in the shell
5 participants