Implement search in Settings UI#19519
Conversation
- SearchMetadata is unsused, but it is designed to hold information
so that we can navigate to a setting
- Updated all pages to...
- have an x:Name on relevant setting containers
- have a NavigateToXArgs used in the OnNavigatedTo() function
- update the NavigateToXArgs to include the name of an element
to scroll down to
- Add BringIntoViewWhenLoaded() to HasScrollViewer<T> which
scrolls down to the element with a given name
These components aren't fully hooked up together yet and there's a
few TODO CARLOS's throughout. Main upcoming work:
- indexing
- runtime indexing
- search box UI
- search results UI
based on https://github.com/microsoft/PowerToys/blob/079c69b8beced55211485c352f41308426930e47/doc/specs/settings-search.md
- Adds referential XAML names to components of interest. - Adds a script that generates the build time entries to be loaded - Invokes the script in Editor.vcxproj
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
lhecker
left a comment
There was a problem hiding this comment.
Why do we need multiple runtimeclass NavigateTo classes? Why not use one and it has an IInspectable navigate-to argument?
src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
Honestly, that's a good idea now that we need one of these for every page we navigate to. I'll add a |
This comment has been minimized.
This comment has been minimized.
DHowett
left a comment
There was a problem hiding this comment.
58/62 down. only the hard ones left!
|
|
||
| WINRT_OBSERVABLE_PROPERTY(ColorSchemesSubPage, CurrentPage, _propertyChangedHandlers, ColorSchemesSubPage::Base); | ||
| WINRT_OBSERVABLE_PROPERTY(Windows::Foundation::Collections::IObservableVector<Editor::ColorSchemeViewModel>, AllColorSchemes, _propertyChangedHandlers, nullptr); | ||
| WINRT_PROPERTY(hstring, ElementToFocus); |
There was a problem hiding this comment.
I noticed a couple of these were added, but I can't see where they're used. Do we store one on the Page VM and one on the Nav Args? When do we move from the Nav Args to the VM?
There was a problem hiding this comment.
AddProfile was wrong/unnecessary, so I just fixed that one.
ColorSchemesPageViewModel and ProfileViewModel still need it because they have subpages. Specifically, elementToFocus goes to the main ColorSchemes/Profiles_Base page, then gets consumed in OnNavigatedTo() there. However, the subpage navigation happens from setting VM.CurrentPage(), so when we get to the property changed handler, the original elementToFocus went out of scope and is lost.
As a workaround, I'm using these two VMs as a staging area for elementToFocus.
Actions doesn't have this problem because although it has subpages, we don't actually try to focus any of the controls in them.
That said, we've talked about refactoring the navigation code. I bet we may be able to remove these ElementToFocus members as a part of that. I suspect that if we manage the breadcrumbs and VMs properly, we may be able to just call contentFrame().Navigate() once and just include elementToFocus there like with the other pages. I'm tracking that over in #19866
| Inputs="@(Page);$(OpenConsoleDir)tools\GenerateSettingsIndex.ps1" | ||
| Outputs="$(GeneratedFilesDir)GeneratedSettingsIndex.g.h;$(GeneratedFilesDir)GeneratedSettingsIndex.g.cpp" |
There was a problem hiding this comment.
very cool - nice specification for files to consider during "out of date" calculation
DHowett
left a comment
There was a problem hiding this comment.
Very clever how you've handled weighting Defaults above other things, and making sure runtime-generated objects match on their names. I haven't read the powershell yet, but I am through everything else. Only minor Qs - but one could be a perf sink.
This is amazing excellent amazing work.
| // Snapshot the current index so that Reset() can safely swap | ||
| // it without invalidating pointers used by this search. | ||
| const auto index = _index.load(); | ||
| wil::hide_name _index; |
|
@DHowett FYI I found a bug, but it's fixed over in PR #19889 (targets this one). In case you're curious:
Cleaning up the navigation code made this easier to fix. Plus that PR includes fixes to several follow-ups. |
DHowett
left a comment
There was a problem hiding this comment.
Carlos, this is amazing work. I found a number of issues in the PowerShell script, but given it's your first ever at-scale powershell there's no chance you might have known all the caveats. I'm not blocking over this because it works today, but it is going to break in the near future for pretty straightforward reasons (like, somebody puts one too many spaces in a XAML file or something)
It can generate tighter, more constexpr-able code, and make even less of a hit on loading TSE. :)
| 'KeyChordListener.xaml', | ||
| 'NullableColorPicker.xaml', | ||
| 'SettingContainerStyle.xaml', | ||
| 'AISettings.xaml', |
| const std::array<IndexEntry, $($ntmEntries.Count)>& LoadNTMFolderIndex() | ||
| { | ||
| static std::array<IndexEntry, $($ntmEntries.Count)> entries = |
There was a problem hiding this comment.
if std::array has a deduction guide, you might be able to do this (just to simplify the actual code and the generator)
const auto& LoadNTMFolderIndex()
{
static std::array<IndexEntry> entries =oh, it looks like you have to drop the ENTIRE template specification to do that. then you need to put the Type{} on each member. okay it would look like this
const auto& LoadNTMFolderIndex()
{
static std::array entries = {
IndexEntry{ a, b, c, d, },
IndexEntry{ a, b, c, d, },
IndexEntry{ a, b, c, d, },
}
}## Summary of the Pull Request Consolidates the navigation functions in `MainPage` for the settings UI. This involved: - combining all separate `_Navigate()` functions into one big one - deduplicating `SettingsNav().SelectedItem()` calls - removing the unnecessary variable for a crumb (just inline creation and registration) - removing the `elementToFocus` staging behavior for color schemes and profile sub pages ## References and Relevant Issues Builds off the work done in #19519 and #19831 ## Validation Steps Performed Navigate to... ✅ simple top-level pages: Startup, Interaction, Appearance, Rendering, Compatibility, Add profile ✅ Color schemes and subpages ✅ Actions, subpages ✅ New Tab Menu and folder subpages ✅ Extensions, subpages, and profile/scheme navigation ✅ defaults profile and subpages ✅ specific profile and subpages Also tested discarding changes on these pages. ✅ search still works and navigates to the correct element ## PR Checklist Closes #19866
## Summary of the Pull Request Cleans up GenerateSettingsIndex.ps1 by addressing the feedback Dustin left in #19519 ## Validation Steps Performed ✅ SUI search works
Summary of the Pull Request
Adds search functionality to the settings UI. This is added to an
AutoSuggestBoxin the mainNavigationView. Invoking a result navigates to the proper location in the settings UI and focuses the setting, when possible.References and Relevant Issues
Based on microsoft/PowerToys#41285
Detailed Description of the Pull Request / Additional comments
x:Nameso that we can navigate to them and bring them into view.BringIntoViewWhenLoaded()which navigates to the relevant part of the UI. This is called inOnNavigatedTo()for each page.MainPage:MainPage::_UpdateSearchIndex()|SearchIndex::Reset(): loads the search index generated byGenerateSettingsIndex.ps1; provides additional localization, if neededMainPage::SettingsSearchBox_TextChanged:SearchIndex::SearchAsync(). This is a HEFTY async function that can be cancelled. It needs a lot of context passed in to expand the search index appropriately (i.e. build awareness of "PowerShell" profile and generate results appropriately). This is also where fzf is used to perform weighted matching.SettingsSearchBox_QuerySubmitted: extract the search index metadata and call the correct_Navigate()functionValidation Steps Performed
Search for...
To test fzf matching and weighted results, I specifically tested these scenarios:
PR Checklist
Closes #12949
Follow-ups
GetSearchableFields()should make the rest pretty easy.GetSearchableFields()should make the rest pretty easy.