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

search-in-workspace: focus on next and previous search results #12703

Merged
merged 12 commits into from
Feb 8, 2024
Merged

search-in-workspace: focus on next and previous search results #12703

merged 12 commits into from
Feb 8, 2024

Conversation

vladarama
Copy link
Contributor

@vladarama vladarama commented Jul 12, 2023 •
edited
Loading

What it does

Fixes: #12692
Adds Search: Focus on Next Result and Search: Focus on Previous Result commands to be able to quickly cycle through search results using F4 and Shift+F4 keybindings respectively. This allows the user to go through search results without having to focus on the search-in-workspace view and needing to use up/down keybindings.

How to test

  1. Open Theia Application
  2. Search for a string that has multiple results in the search-in-workspace view
  3. Use the command palette (Ctrl+Shift+P) and search for Search: Focus on Next Result or Search: Focus on Previous Result
  4. The search-in-workspace view should appear and the next/previous search result should be focused and selected.

OR

  1. Open Theia Application
  2. Search for a string that has multiple results in the search-in-workspace view
  3. Use the keybindings F4 and Shift+F4 for the Search: Focus on Next Result and Search: Focus on Previous Result respectively
  4. The search-in-workspace view should appear and the next/previous search result should be focused and selected.

Review checklist

Reminder for reviewers

@vince-fugnitto vince-fugnitto added the search in workspace issues related to the search-in-workspace label Jul 12, 2023
@vladarama vladarama requested a review from vince-fugnitto July 12, 2023 15:23
vince-fugnitto
vince-fugnitto reviewed Jul 13, 2023
View reviewed changes
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The implementation is starting to look good but there are a couple of things to address:

  • the focus for previous and next results should only focus on actual results, not the file nodes themselves
  • the focus for previous and next results should support the results being collapsed

Here's a quick video of what I mean:

siw-focus.mp4

packages/search-in-workspace/src/browser/search-in-workspace-frontend-contribution.ts Outdated Show resolved Hide resolved
@msujew msujew mentioned this pull request Jul 15, 2023
Closed
1 task
@vince-fugnitto vince-fugnitto self-requested a review August 1, 2023 17:27
@vladarama
Merge branch 'master' of https://github.com/vladarama/theia into sear…
fe57af6 
…ch-focus-result
@vladarama
fix review comments
3367c5f 
- Simplify the `isEnabled` command.
- Add `when` context to the keybinding.
@vladarama vladarama marked this pull request as draft August 10, 2023 13:06
@vladarama
add unit tests to the selectable trees
bdec8b1 
- Added a new mock tree model that contains selectable nodes
Unit tests cover:
- `getNextNode`, `getPrevNode`
- `getNextSelectableNode`, `getPrevSelectableNode`
- `selectNext`, `selectPrev`
- `selectNextNode`, `selectPrevNode`
@vladarama
refactor and simplify unit tests
10728e9 
This commit simplifies the code by significantly reducing the repetition
in the tests.
@vladarama vladarama marked this pull request as ready for review August 11, 2023 12:39
@vince-fugnitto
Copy link
Member

I'll perform a review of the pull-request tomorrow 👍

@JonasHelming
Copy link
Contributor

@vince-fugnitto ping :-)

@vince-fugnitto
Copy link
Member

@msujew I helped Vlad a bit during the implementation, would you mind performing a review of the feature whenever you get a chance? There's no rush :)

@JonasHelming JonasHelming requested a review from msujew December 14, 2023 23:40
msujew
msujew approved these changes Feb 8, 2024
View reviewed changes
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

I'm happy with the changes in the PR. I have a misgiving in regards to the preview behavior for editors, but we can address this in a separate PR:

When focusing the next search result in the same file, the editor switches from the preview mode to the non-preview mode. Consequently the editor tabs stick in the main area, whereas in vscode these editors stay as preview editors.

@msujew msujew merged commit b7ed5e9 into eclipse-theia:master Feb 8, 2024
1 check passed
@jfaltermeier jfaltermeier added this to the 1.47.0 milestone Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msujew msujew msujew approved these changes

@vince-fugnitto vince-fugnitto Awaiting requested review from vince-fugnitto

Assignees

@msujew msujew

Labels
search in workspace issues related to the search-in-workspace
Projects
PR Backlog
Archived in project
Milestone
1.47.0
Development

Successfully merging this pull request may close these issues.

search-in-workspace: focus on next and previous search results
5 participants
@vladarama @vince-fugnitto @JonasHelming @msujew @jfaltermeier