Add stable_id support for match restoration - #44829
Conversation
cla-bot
Bot
added
the
cla-signed
94770d9 to
be27920
Compare
SomeoneToIgnore
left a comment
SomeoneToIgnore
left a comment
There was a problem hiding this comment.
Sorry for the long wait time: had had quite a rough time and then a company-wide holiday.
Looks really great when I've tried it, thank you so much for finding a way to improve things generically.
One concern besides the listed ones: we need to test it.
At least, a test to show that the stable sorting works, and, ideally, another one to cover
Item no longer in results, clear manual selection and reset to first item
branch.
| self.delegate.set_selected_index(ix, window, cx); | ||
| let current_index = self.delegate.selected_index(); | ||
|
|
||
| // Track manually selected item to preserve across match updates |
There was a problem hiding this comment.
This comment states pretty much the same what the code below does, so not needed?
Also, can we move the whole thing into the if previous_index != current_index block?
There was a problem hiding this comment.
actually not, it subtly breaks the behavior, but I had to test it to find that out. Seems like I should add a few tests though to make sure of it.
| /// | ||
| /// Use this for user-driven selections (keyboard navigation, mouse clicks) where you want | ||
| /// the user's choice to be maintained as they continue typing. For programmatic selections | ||
| /// that should not persist, use `set_selected_index` instead. |
There was a problem hiding this comment.
For programmatic selections that should not persist, use
set_selected_indexinstead.
Curious, what is the use case for these?
If we use select_index_sticky everywhere, what does it change?
There was a problem hiding this comment.
intuitively that should be fine, I was just a bit careful to not change the default in too many places at once, because I had no good overview of what needs to be tested
| } else { | ||
| // Item no longer in results, clear manual selection and reset to first item | ||
| self.manually_selected_stable_id = None; | ||
| let ix = 0.min(match_count.saturating_sub(1)); |
There was a problem hiding this comment.
0.min(any_other_usize) is 0 or am I missing something?
Also, should we try to fall back to closest selected index instead, to avoid large list jumps?
There was a problem hiding this comment.
good catch. from my manual testing I didn't perceive this as too bad, since usually you're already down to only a handful of matches by the time this happens. I'll change this you jump to the closest
There was a problem hiding this comment.
about those large jumps. wasn't there also a ticket about the heuristic that selects the very first item when opening the picker? I can't find it right now. anyway, right now the picker already picks a "relatively random" item instead of "the closest to the cursor". maybe I find that ticket again some day
be27920 to
958bfc6
Compare
|
I've added a |
Screen.Recording.2026-02-19.at.00.24.47.movI've added it for the recent projects picker too |
e8ac6b1 to
aac841d
Compare
|
and now with correct formatting 🙄 |
|
Yes, I think I can take over from now on, if that's fine with you to wait another few weeks potentially. |
|
cool |
github-actions
Bot
added
size/XL
large-pr
📏 PR Size: 1332 lines changed (size/XL)Please note: this PR exceeds the 400 LOC soft limit.
|
66a85ad to
94665c4
Compare
SomeoneToIgnore
left a comment
SomeoneToIgnore
left a comment
There was a problem hiding this comment.
I have trimmed it down to the scope where only the pickers influenced by the change have their code adjusted.
When re-testing, I've found 2 things with the selection counter-intuitive: when I select an item manually and first get a query that mismatches the manually selected item entirely (this it's not on the list anymore), it would be good get the manually selected item re-selected on backspace
Now, it seems that backspacing leaves the selection in some odd places sometimes, even without this fallback?
Would be good to keep it closer to top by default, as we try to do now after recent PRs.
| } else { | ||
| // No manual selection - clamp current index to valid range | ||
| let current_index = self.delegate.selected_index(); | ||
| let ix = current_index.min(match_count.saturating_sub(1)); |
There was a problem hiding this comment.
I think this and the one above break recent attempts of #50594 to select the first match by default, can we ensure we try and select something closer to the top here?
|
Sorry, I see the conflicts start to appear, hence I'll close this for now: it seems that no one had upvoted the issue still, so it's not that popular, and I'm not able to find time to push the logical fixes further. I think what's left is to go over backspace interaction better, add more tests and make it look "natural" when modifying the query. |
3 participants
Closes #35440
Release Notes: