Fix several issues in Command Palette#18486
Conversation
|
That failed test seems unrelated to my changes. Can you take a look @yair100 ? |
|
The test is passing now. That being said, this PR seems to include a lot of changes that weren't discussed beforehand. It will speed up the review process if this focuses on the fix needed for #18395. We can discuss the other changes as well, but they shouldn't be part of this PR. |
True. I described them in the description of this PR.
I made #18496. It fixes the one issue.
I described them in the description of this PR. Feel free to discuss. IMHO it's easier to discuss changes when you see them, instead of when they're still an abstract concept. |
Resolved / Related Issues
Closes #18395
NavigationToolbarViewModel.UpdateCommandPaletteSuggestions, more specifically in theelsebranch loop. Thatelsebranch was completely removed. We now always replace all items in theOmnibarCommandPaletteModeSuggestionItemscollection. This also results in simpler and more efficient code.NavigationToolbarViewModel.UpdateCommandPaletteSuggestionshas become so simple that it was inlined into its caller,PopulateOmnibarSuggestionsForCommandPaletteMode. Because of that change, we no longer need the intermediatenewSuggestionscollection, saving memory and resulting in a more responsive UI update.await Task.Yield()calls is reduced. There were too many, resulting is too much pointless overhead.commandsToProcesslist has been optimized, especially for the case where theOmnibarCommandPaletteModeTextsearch string is empty, i.e. the first list to be calculated when the command palette is opened.commandsToProcesslist happens on a background thread, we now test whetherOmnibarCommandPaletteModeTexthas not changed by the time the list calculation is finished, and prevent further processing of stale results if it did.NavigationBarSuggestionItemobjects are now immutable. They were never mutated anyway. This also simplifies their construction.NavigationBarSuggestionItem.ActionIconSourceproperty was removed as it was never assigned and always had the valuenull.NavigationBarSuggestionItem.PrimaryDisplayproperty was removed as it always had the same value as theTextproperty.NavigationBarSuggestionItem.SearchTextproperty was removed as it was never read.NavigationBarSuggestionItem.Commandproperty was added, storing the command used to construct theNavigationBarSuggestionItem. That property is used inNavigationToolbar.Omnibar_QuerySubmitted, eliminating the need to search for the command.NavigationBarSuggestionItemis now immutable, it no longer inherits fromObservableObjectand it no longer tries to raise pointlessPropertyChangingandPropertyChangedevents during its construction.NavigationBarSuggestionItemis now immutable, data binding in NavigationToolbar.xaml now usesMode=OneTimeinstead ofMode=OneWay. As a result, the UI no longer needs to wire up listeners for events that will never be raised anyway.Omnibar.AutoSuggestBox_TextChanged, we set_textChangeReasontoOmnibarTextChangeReason.UserInputwhen it's notOmnibarTextChangeReason.ProgrammaticChangeand_userInput.Length == 0. This fixes the bug where clicking theXbutton at the right of the textbox to clear it did not cause a recalculation of the suggestions list.As a result of the above
Steps used to test these changes
terminalone character at a time, as well as removing characters using Backspace, and observed that suggestions are now correct.xyz. Observed that theNoCommandsFoundmessage is still displayed correctly.xyz. Observed that the dialog box is still displayed correctly.Xbutton in the textbox to clear it. Observed that this now correctly causes suggestions to be recalculated.