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

Fixes find widget shall be shown per split terminal #155361

Merged
merged 8 commits into from Sep 2, 2022

Conversation

jeanp413
Copy link
Contributor

This PR fixes #145865

@meganrogge
Copy link
Contributor

meganrogge commented Aug 22, 2022

Thanks @jeanp413 for working on this. The code changes/clean-up look great 👍🏼

Github won't let me attach images or gifs ATM 😢

I notice a few things:

  1. You have to focus a terminal for that find widget to become active. I think focusing a find widget should set the corresponding instance as the active one

  2. When minding step 1, focusing terminal A and pressing enter in the find widget a few times results in the index 1 of 4 for example updating correctly ✅ . Leaving that find widget open and focusing terminal B and doing the same the results in the index being out of sync with the currently selected element. Investigating this issue as I implemented the index in xterm.js and could be a bug with that 😄

@jeanp413
Copy link
Contributor Author

jeanp413 commented Aug 29, 2022

@meganrogge updated the PR, last commit is optional just some 💄 to match the findwidget behavior from a normal editor
Current behavior in stable:
terminal_find_widget

@meganrogge
Copy link
Contributor

great @jeanp413 thanks. I was just working on fixing the two problems I mentioned above and will revert my fix as yours looks better 👍🏼

meganrogge
meganrogge previously approved these changes Aug 29, 2022
Copy link
Contributor

@meganrogge meganrogge left a comment

Choose a reason for hiding this comment

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

Works great 😄

@meganrogge meganrogge requested a review from Tyriar August 29, 2022 19:13
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Works great, sorry about the delay 🙂

@Tyriar Tyriar merged commit 0797f09 into microsoft:main Sep 2, 2022
@jeanp413 jeanp413 deleted the fix-145865 branch September 2, 2022 17:55
@github-actions github-actions bot locked and limited conversation to collaborators Oct 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Find widget shall be shown per split terminal
3 participants