Remove overrides of PopIn/PopOut in FocusedOverlayContainer #5834
+20
−14
Conversation
This file contains 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
jai-x
commented
Jun 11, 2023
Comment on lines
+22
to
+33
| public abstract partial class Popover : OverlayContainer | ||
| { | ||
| protected override bool BlockPositionalInput => true; | ||
|
|
||
| public override bool ReceivePositionalInputAt(Vector2 screenSpacePos) => Body.ReceivePositionalInputAt(screenSpacePos) || Arrow.ReceivePositionalInputAt(screenSpacePos); | ||
|
|
||
| public override bool HandleNonPositionalInput => State.Value == Visibility.Visible; | ||
|
|
||
| public override bool RequestsFocus => State.Value == Visibility.Visible; | ||
|
|
||
| public override bool AcceptsFocus => State.Value == Visibility.Visible; | ||
|
|
There was a problem hiding this comment.
This is a bit of an odd change.
Since PopOver was overriding PopIn/PopOut and FocusedOverlayContainer used those methods for automatic getting focus, it meant that PopOver would never automatically gain focus when shown.
Since moving the focus gain to UpdateState in FocusedOverlayContainer, this changed the focus behaviour of PopOver and broke tests.
The solution here is to not inherit from FocusedOverlayContainer and allow PopOver to manage it's own focus state.
peppy
added a commit
to peppy/osu
that referenced
this pull request
Jun 12, 2023
…`abstract` change See ppy/osu-framework#5834
Merged
2 tasks
|
osu! side changes: ppy/osu#23882 Will wait for one more review on this one. |
peppy
added a commit
to peppy/osu
that referenced
this pull request
Jun 12, 2023
…`abstract` change See ppy/osu-framework#5834
peppy
added a commit
to peppy/osu
that referenced
this pull request
Jun 16, 2023
…`abstract` change See ppy/osu-framework#5834
peppy
added a commit
to ppy/osu
that referenced
this pull request
Jun 17, 2023
…`abstract` change See ppy/osu-framework#5834
peppy
added a commit
to peppy/osu
that referenced
this pull request
Jun 17, 2023
…`abstract` change See ppy/osu-framework#5834
peppy
added a commit
to peppy/osu
that referenced
this pull request
Jun 18, 2023
…`abstract` change See ppy/osu-framework#5834
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Resolves #5621
Moves focus logic outside of
PopIn/PopOutmethods inFocusedOverlayContainerForces non-abstract inheritors of
FocusedOverlayContainerto correctly implement these methods.Breaking changes
Subclasses of
FocusedOverlayContainermust now implementPop{In,Out}()Previously, subclasses of
FocusedOverlayContainerwere not forced to implementPop{In,Out}themselves, despite actually needing to do so for the overlay to actually play transitions out correctly. This has now been changed and inheritors of the class must always implementPop{In,Out}.In practice this should not break any reasonable consumers of the class, except for the need to remove any
base.Pop{In,Out}()calls in classes that directly inheritFocusedOverlayContainerand call base inPop{In,Out}().