Adjust search container logic to address game-side issues with hiding its children#5530
Merged
peppy merged 8 commits intoppy:masterfrom Nov 29, 2022
Merged
Conversation
A notable exception is `TestGroupButton`, which no longer inherits from `IHasFilterableChildren`, but its `FilterableChildren` member has not been deleted. This is because `TestBrowser` relies on it.
The test was previously checking that all the `HeaderContainer`s are not present. This however breaks if one of them is caught early as `CanBeShown = false`, as then the search algorithm will not descend into that subtree at all, and *technically* leave some `HeaderContainer`s present - but the ones we actually care about, i.e. the topmost-level ones, will be hidden correctly.
smoogipoo
approved these changes
Nov 29, 2022
peppy
reviewed
Nov 29, 2022
|
|
||
| if (drawable is IConditionalFilterable conditionalFilterable) | ||
| { | ||
| var canBeShownBindable = conditionalFilterable.CanBeShown.GetBoundCopy(); |
Member
There was a problem hiding this comment.
I'm a touch worried about the bindable overhead here if many filter operations happen over a short period (there will be a lot of bindable churn). Probably okay, but here's a quick comparison of loading SettingsOverlay and performing 10 filters + unfilters:
Before:
After:
The overhead is definitely visible, but maybe fine to eat? We can probably further optimise this at a later point (ie. avoiding clearing the list altogether and reusing existing bindings) if required.
Member
|
Let's go with this and see how things play out. |
peppy
approved these changes
Nov 29, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an RFC.
SearchContainerfiltering operation and fix potential flaws #5154Contents
IHasFilterableChildrenis removed; hierarchical lookups now leverageIContainerEnumerable<T>This is the same as in the original PR. Rather than rely on
IHasFilterableChildrento descend down complex hierarchies for filtering purposes,IContainerEnumerable<T>.Childrenis used, which results in less boilerplate as you don't need to remember to implement it whenever needed.IConditionalFilterable(?) is introducedAt first I tried to resurrect the aforementioned PR rather than rewrite, but the invalidation issues pointed out therein ended up being caused by a fatal contradiction in its logic that I describe in more detail here. As a result, this PR takes a different, explicit approach of introducing a new
IConditionalFilterableinterface.The name is bad, I know (suggestions welcome), but the gist of it is that upon implementing
IConditionalFilterable, you get one extra bindable to play with. If the value of said bindable istrue, then the item is included in textual search as usual. If it isfalse, however, it - and its entire subtree - is excluded from further search on the premise that the item does not meet external criteria. In practical terms, you would set it tofalsewhen you want to hide some items inside aSearchContainerbecause they're unavailable due to other settings, which is precisely the use case that needs fixing game-side.A branch for game-side adjustments for this framework change, that end up fixing ppy/osu#18003, is available for preview at https://github.com/ppy/osu/compare/master...bdach:filtering-framework-change-adjustments?expand=1.
I also recommend examining the commit messages of d0d90e9 and b790cd3.