Skip to content
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

Allow SearchContainer implementations to manually trigger a Filter operation #5772

Merged
merged 8 commits into from May 26, 2023

Conversation

Cootz
Copy link
Contributor

@Cootz Cootz commented May 6, 2023

As discussed here this PR adds SearchContainer.Filter

Usage:

public partial class MySearchContainer : SearchContainer
{
    public string MySearchTerm
    {
        get => SearchTerm;
        set
        {
            SearchTerm = value;

            if (!IsPresent)
                Filter();
        }
    }

    //OR

    public void ForceFilter() => Filter();
}

@bdach
Copy link
Collaborator

bdach commented May 6, 2023

Context which should be in the OP but isn't:

This is supposed to facilitate the implementation of search box in the mods overlay. The problem is thus: To use SearchContainer, you would want to add a SearchContainer to each mod column in the overlay. However, mod columns that have all of their mods filtered out will hide themselves to not take up space.

The other part of the problem is that SearchContainer's filter logic needs the search container to be present to run, because it's based on invalidation and Update(). The change of the filter term only invalidates the filter, but the only thing that can re-validate the filter is Update().

So if you have a case wherein the search container filters out all children because they don't match the filter, and then have logic that says "if all children of the search container are hidden, then hide an ancestor of the search container", then if you filter out all children and attempt to unfilter them again, then the column will not unhide, because the filter doesn't run, because it can't as its Update() will never be reached.

This is supposed to be a somewhat primitive workaround for this that will allow force-executing the filter instantly on search term change if required. I considered doing this myself, but didn't really want to PR that without exploring better options which never materialised.

Also I'm not sure how accurate the sample in the OP is, because I'm pretty sure it only works if you directly hide the SearchContainer, unless you do some weird overriding. If you hide the SearchContainer's parent, then the IsPresent flag on the SearchContainer will not change and so SearchTerm changes will have no effect.


@ppy/team-client interested in thoughts on this

@peppy peppy mentioned this pull request May 25, 2023
@peppy peppy self-requested a review May 25, 2023 09:32
@peppy
Copy link
Sponsor Member

peppy commented May 25, 2023

I've reworded the xmldoc here, but have two follow-up questions:

  • Can we early return if the filter is already in a valid state, or is there a reason we need to bypass this validity check?
diff --git a/osu.Framework/Graphics/Containers/SearchContainer.cs b/osu.Framework/Graphics/Containers/SearchContainer.cs
index 77c389b48..e9de9e7e4 100644
--- a/osu.Framework/Graphics/Containers/SearchContainer.cs
+++ b/osu.Framework/Graphics/Containers/SearchContainer.cs
@@ -104,9 +104,14 @@ protected override void Update()
         ///
         /// However, if <see cref="SearchContainer{T}"/> or any of its parents are hidden this will not be run.
         /// If an implementation relies on filtering to become present / visible, this method can be used to force a filter.
+        ///
+        /// Note that this will only run if the current filter is not in an already valid state.
         /// </remarks>
         protected void Filter()
         {
+            if (filterValid.IsValid)
+                return;
+
             canBeShownBindables.Clear();
             performFilter();
             filterValid.Validate();
  • Would an alternative to any of this be an override IsPresent => base.IsPresent || !filterValid.IsValid? Not saying this is necessarily a great solution, but would it be an avenue for further exploration?

@bdach
Copy link
Collaborator

bdach commented May 25, 2023

Would an alternative to any of this be an override IsPresent => base.IsPresent || !filterValid.IsValid? Not saying this is necessarily a great solution, but would it be an avenue for further exploration?

The mod search implementation hasn't changed very much in that particular respect from how I left it as a rough branch for @Cootz to look at, and that means that overriding presence does nothing to fix it, since it is the SearchContainer's ancestor that gets hidden, meaning that an entire supertree containing the SearchContainer becomes non-present, meaning that fiddling with IsPresent in any way does nothing at all, as the presence check of the SearchContainer never even gets reached at all.

The IsUpdating solution you mentioned on discord once might work. Realistically speaking I do not have the bandwidth to go and try it.

Personally I consider this PR to be a pretty awful escape hatch (as evidenced by the game-side hack which will still be required with this PR - rather than calling Update() that weird ModSearchContainer.ForcedSearchTerm will be calling Filter()) and would pretty much accept anything even marginally better over it. But I'm not objective since I was butting my head against this issue for weeks and got nothing good to show for it.

@Cootz
Copy link
Contributor Author

Cootz commented May 26, 2023

Can we early return if the filter is already in a valid state...

Yes we can.

Would an alternative to any of this be an override IsPresent => base.IsPresent || !filterValid.IsValid?

Already answered by @bdach, just add that I've spent a lot of time working on this issue as well, and that is the only thing I came up with.

P.S. I completely missed the discussion about IsUpdating. Will give it a shot later.

@pull-request-size pull-request-size bot added size/M and removed size/S labels May 26, 2023
@peppy peppy changed the title Allow developers to invoke filtering whenever they need it Allow SearchContainer implementations to manually trigger a Filter operation May 26, 2023
@peppy peppy self-requested a review May 26, 2023 11:06
peppy added 2 commits May 26, 2023 20:07
Just in case the `Filter` method is not inlined, and for code readability I'd rather duplciate this here.
@peppy peppy enabled auto-merge May 26, 2023 11:08
@peppy peppy merged commit 9206408 into ppy:master May 26, 2023
11 checks passed
@Cootz Cootz deleted the add-search-method-for-search-container branch May 26, 2023 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants