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

Fix settings overlay components not invalidating presence on filter change #17937

Merged
merged 5 commits into from
Apr 24, 2022

Conversation

frenzibyte
Copy link
Member

@frenzibyte frenzibyte commented Apr 22, 2022

Fixes #17929, again

Prior to #17823, filtering would update the Alpha value of the settings overlay components (sections & items), which would invalidate Presence along the way, to note to parenting flow containers that re-performing layout is necessary.

But now on master, filtering would instead update the IsPresent state directly without an invalidation, resulting in the settings panels not laying out properly and get into a completely weird state.

Added test coverage by filtering after load, as that's when the settings overlay starts relying on invalidation to reflow the sections.

Note that this also reverts #17931 as it's still no longer necessary, but I will suggest quadruple-checking the overlay in all ways possible to ensure nothing has regressed yet again. I've tested enough and can't find anything broken now.

bdach
bdach previously requested changes Apr 22, 2022
Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requesting changes due to actual test failures and doubts about direction

osu.Game/Overlays/Settings/SettingsItem.cs Outdated Show resolved Hide resolved
Comment on lines +114 to +115
if (IsPresent != wasPresent)
Invalidate(Invalidation.Presence);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know about this. In an ideal world, this sort of thing should be handled by framework. There are several other IsPresent overrides in the game alone even and as far as I understand every single one is susceptible to this same bug of invalidations not firing on custom presence criteria changing. And not every one of those will be so easily fixable as this one (the ones that check Scheduler.HasPendingTasks certainly aren't going to be...)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sure, but I'm not really sure how it should be approached framework-side. If anything, we probably don't want to allow IsPresent to be overridable in the first place, to avoid this weirdness happening.

If I were to guess of a proper fix, it might be to have something similar to the ShouldBeAlive and IsAlive flow (in which the former is an overridable property, while the latter is updated internally to the state of the former).

But for now, every override of IsPresent must invalidate Presence, and I'm unsure how that should be achieved for all overrides, so I sticked into fixing SettingsOverlay which is the most important here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the kind of case we want to use AddLayout instead?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AddLayout solves the inverse problem doesn't it? LayoutMember is for when you want to recompute something when an invalidation happens. But the issue here is that due to IsPresent being a computed read-only property, framework can't know when it changes to fire off an invalidation without adding some flow that would check whether the value changed between consecutive reads.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yep, that makes sense.

I guess this is okay for now?

@bdach bdach dismissed their stale review April 23, 2022 18:36

the blocker is resolved, i suppose

@peppy peppy merged commit 21665f6 into ppy:master Apr 24, 2022
@frenzibyte frenzibyte deleted the settings-filter-regression-fix-2 branch April 24, 2022 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Searching in settings does not show/hide sections correctly
3 participants