Fix #206511: Shortcuts now visit the correct search entry after editing #209228
Conversation
|
@microsoft-github-policy-service agree |
…ter editing
Bug (previous behavior):
Press Ctrl+Shift+F to perform a search.
Press F4 a few times.
Edit the selected result so it no longer matches the query.
The active position in the search pane disappears.
Press F4. Editor location jumps to the first found item.
Solution:
When the search results change, the tree in src/vs/workbench/contrib/search/browser/searchView.ts now correctly updates the selected result if it is no longer present.
This is accomplished by keeping a reference to the Match immediately before the selection.
If the tree's selection is deleted, we will find this reference and make this the new selection.
Testing:
This is tested via a smoke test, which attempts to perform deletions via the keyboard shortcuts in order to keep only the results in the first FileMatch.
|
Hi! I’m sorry, I’m a bit new to the open source development community and didn’t expect my own smoke test to fail when I opened the PR. It was running fine on my codespace. The actual fix works perfectly fine though. |
The test now uses the editor testing api to change the text in the files. It also uses waitForTextContent on the selected plain match to make sure the selection is correct.
|
Hello @andreamah! Sorry for the mess, once again, I'm still a bit inexperienced. The fix and tests should be working now :) |
| this.tree.expand(beforePrevious); | ||
| } | ||
|
|
||
| beforePrevious = navigator.previous(); |
There was a problem hiding this comment.
Before before this step, is the navigator's current selection still on the node that is either now non-existing or isn't a match? Just want to know without running the debugger 😅
There was a problem hiding this comment.
The navigator.current() result after navigating to selection (line 750) will point to the node that is now non-existing. However, because the tree results aren't updated yet, I can get the previous node (which still exists) and then use it to set the selection when the current is deleted.
There was a problem hiding this comment.
Makes sense. Can you check the event variable for removed: true for this so that it doesn't run every time we refresh the tree?
|
I wonder: would we expect the next entry to be highlighted instead of the previous? This might be more expected, and we already have logic for highlighting next match here vscode/src/vs/workbench/contrib/search/browser/searchActionsRemoveReplace.ts Lines 360 to 374 in 777ea45 |
Yes, this definitely makes sense. I decided to do it this way (selecting the previous) so that, when the person clicks f4 again, the editor will jump to the result after the deleted selected node. I did it this way because of my interpretation of the original issue.
If you prefer, I can also do it in the way you suggested :) |
No, I think it makes sense to progress to the next one. I think I originally said this because clicking 'x' to remove a search result went to the result before, but I think I'd accept the inconsistency with that here. |
| this.tree.expand(beforePrevious); | ||
| } | ||
|
|
||
| beforePrevious = navigator.previous(); |
There was a problem hiding this comment.
Makes sense. Can you check the event variable for removed: true for this so that it doesn't run every time we refresh the tree?
| if (this.tree.isCollapsed(beforePrevious)) { | ||
| this.tree.expand(beforePrevious); | ||
| } |
There was a problem hiding this comment.
Not sure if this logic is correct.
When you expand an element, you might want to go previous from what was after it in order to get the true previous match.
Bug steps:
- Collapse the node before the target search result.
- remove the target search result by editing it
- 🐛 the result goes to the node before the collapsed node, and still expands the collapsed node.
There was a problem hiding this comment.
Might also be good to test in the tree view for search.
There was a problem hiding this comment.
The logic is more correct than what we have now, at sure, it virtually doesn't matter if the focus jumps one item up or down on deletion, important is that it doesn't jump to the first item.
Can you pls merge this PR, the current state is much worse than this PR.
Fixes #206511
Bug (previous behavior):
Press Ctrl+Shift+F to perform a search.
Press F4 a few times.
Edit the selected result so it no longer matches the query.
The active position in the search pane disappears.
Press F4. Editor location jumps to the first found item.
Solution:
When the search results change, the tree in src/vs/workbench/contrib/search/browser/searchView.ts now correctly updates the selected result if it is no longer present. This is accomplished by keeping a reference to the Match immediately before the selection. If the tree's selection is deleted, we will find this reference and make this the new selection.
Testing:
This is tested via a smoke test, which attempts to perform deletions via the keyboard shortcuts in order to keep only the results in the first FileMatch.