Search all SelectingItemsControl items with TextSearch on key input, not just realized ones #17506
Conversation
jonko0493
changed the title
|
You can test this PR using the following package version. |
|
|
@cla-avalonia agree |
|
Everything was passing prior to me merging master... not sure what changed, could be transient failures? |
|
@jonko0493 don't worry tests are a bit flaky atm |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
|
Hi folks! Is there something more I need to do to get this reviewed? It is needed for the application I'm developing and it's been a month since it was opened. Thanks! |
|
You can test this PR using the following package version. |
|
@timunie I'm very sorry to ping you, but I'm wondering if there's anything else I need to do to get this reviewed. It's needed to make my project usable and I'd prefer not to take a dependency on the PR package. Please let me know if there's anything I can do! |
|
If you have paid support please ask for priority |
|
You can test this PR using the following package version. |
|
@timunie I don't have paid support as we're a tiny open source project, so fair enough, I'll wait my turn. This PR has been open three months -- how long should I expect to wait before getting a review without paid support? |
|
Thanks for bearing with us. To clarify, this PR was submitted on 14th November, so it’s been open for less than two months, not three. As an open-source project, we greatly value community contributions, but our priority is supporting paying customers. While we can’t guarantee review times for community PRs, we aim to review them as quickly as possible. We’re planning to allocate time this week to address outstanding contributions, though I can’t promise which specific PRs will be reviewed. |
There was a problem hiding this comment.
Thank you for your contribution.
While the implementation has its merits, we now have different behaviors for standard panels and for VirtualizingPanel.
The non-virtualized implementation searches inside item containers (controls) directly. The new implementation for VirtualizingPanel searches only inside Items.
We should have the same implementation for both panels here, for consistency.
Looking at WPF, it always searches in Items, so I propose that we do the same, whatever the panel.
The search should look for:
TextSearch.Text(current)IContentControl.Content.ToString()(current)item.ToString()
(Ideally we'd also consider DisplayMemberBinding, like WPF does, but this is out of scope of this PR and can be added later).
| /// </summary> | ||
| /// <param name="textSearchTerm">The beginning of a string (case-insensitive) to search for in the panel</param> | ||
| /// <returns>The index of the first IContentControl item contained in the panel</returns> | ||
| protected internal int GetIndexFromTextSearch(string textSearchTerm) |
There was a problem hiding this comment.
Please remove the protected modifier, this method shouldn't be exposed as public API.
| /// <returns>The index of the first IContentControl item contained in the panel</returns> | ||
| protected internal int GetIndexFromTextSearch(string textSearchTerm) | ||
| { | ||
| var matchingControl = Items.FirstOrDefault(i => i is Control c && ItemsPresenter.ControlMatchesTextSearch(c, textSearchTerm)); |
There was a problem hiding this comment.
Use a single loop, and check each item for ControlMatchesText then ToString(), keeping the old behavior.
|
@MikeCodesDotNET Thanks so much for letting me know and I'm sorry for misrepresenting how long the PR had been open -- that was truly a mistake on my part (I can't do date math well in my head, apparently lol). @MrJul Thank you for the review! The changes you've suggested make good sense. I'll make them in the next few days. |
What does the pull request do?
This PR brings the behavior of
SelectingItemControls withVirtualizingPanelpresenters into parity with those controls without virtualizing presenters.What is the current behavior?
Currently,
SelectingItemsControls withVirtualizingPanelitem presenters (such asComboBox) do not search the full contents of their boxes with text search, only the currently realized controls. This meansthat if a(this was slightly wrong!) it will only jump to items immediately in the popup.ComboBoxis closed, typing does nothing, and even when opened,What is the updated/expected behavior with this PR?
Now,
ComboBoxes and otherSelectingItemsControls with virtualizing presenters will have properTextSearchfor their entire item sets.How was the solution implemented (if it's not obvious)?
The anonymous method that existed in the
SelectingItemsControlclass previously has been moved into a static method inItemsPresenter. It's now used there and inVirtualizingPanelto check search their items/children for text search. (Note: I don't know if there is a preferred place for a static method like this; happy to move as appropriate.)Unit test case was added to an existing test and the test was adapted so that ComboBoxes in tests will use a
VirtualizingStackPanellike the real controls do to ensure the tests fail without this change.Checklist
Breaking changes
Obsoletions / Deprecations
Fixed issues
Fixes #12490.