-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Smooth animation of command palette filtering #6939
Smooth animation of command palette filtering #6939
Conversation
reviewer note: I removed the checklist, as given it was mostly unchecked it didn't feel strictly necessary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
THANK YOU. I knew there had to be a way to do this, and I'm so glad you figured it out. These are only nits,so I can't really block over them. I love this.
P.S. You'll probably need to run tools\runformat.cmd
in the root of the repo to get the CI to pass.
Thanks for the review. My official excuse for putting this together is that it's needed for accessibility -- see bullets 5 & 6 here. |
// Add any extra trailing items from the source | ||
while (_filteredActions.Size() < actions.size()) | ||
{ | ||
_filteredActions.Append(actions[_filteredActions.Size()]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I trust that this all works because you tested it, but I just can't for the life of me figure out how and why iterating over in index order doesn't explode the moment we start manipulating the array layout underneath us o_O
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, this is a process by which we successively close gaps where _filteredActions
doesn't match actions
and insert actions
that aren't in the right place in _filteredActions
. After that, we just bulk remove _filteredActions
that are off the end (because the real actions pushed them out) and bulk insert the actions
that weren't represented.
Fascinating!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The command palette is a ListView of commands. As you type into the
search box, commands are added or removed from the ListView. Currently,
each update is done by completely clearing the backing list, then adding
back any items that should be displayed. However, this defeats the
ListView's built-in animations: upon every keystroke, ListView displays
its list-clearing animation, then animates the insertion of every item
that wasn't deleted. This results in noticeable flickering.
This PR changes the update logic so that it updates the list using
(roughly) the minimum number of Insert and Remove calls, so the ListView
makes smoother transitions as you type.
Detailed Description of the Pull Request / Additional comments
I implemented it by keeping the existing code that builds the filtered
list, but I changed it to build into a scratch list. Then I grafted on
a generic delta algorithm to make the real list look like the scratch
list.
To verify the delta algorithm, I tested all 360,000 permutations of
pairs of up to 5 element lists in a toy C# app.
Validation Steps Performed
I'm not sure if my screen capture tool really caught all the flickering
here, but the screencasts below should give a rough idea of the
difference. (All the flickering was becoming a nuisance while I was
testing out the HC changes.)
Existing behavior:
New behavior: