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 now playing playlist not highlighting selected item on initial open #22950

Merged
merged 3 commits into from
May 2, 2023

Conversation

Joehuu
Copy link
Member

@Joehuu Joehuu commented Mar 26, 2023

Before:
AnWtIGyEe2

After:
zI9dufmSUO

Comment on lines 77 to 80
updateSelectionState(false);
});
}, true);

updateSelectionState(true);
Copy link
Collaborator

@bdach bdach Mar 28, 2023

Choose a reason for hiding this comment

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

This is really convoluted and haphazard. Note that after the BindValueChanged() call, which calls updateSelectionState(instant: false), updateSelectionState(instant: true) is called. There's no reason why you would issue a non-instant update and then an instant update. This fix only appears to work because the (..., true) change happens to update the initial state of the selected field which is not valid on master. There's also the part where even if updateSelectionState() is hit, the text sprites of the title part may not have been created yet. So this pretty much only works by chance.

If you ask me, this pull should look like this:

diff --git a/osu.Game/Overlays/Music/PlaylistItem.cs b/osu.Game/Overlays/Music/PlaylistItem.cs
index 00c5ce8002..4a39cc06c8 100644
--- a/osu.Game/Overlays/Music/PlaylistItem.cs
+++ b/osu.Game/Overlays/Music/PlaylistItem.cs
@@ -56,7 +56,7 @@ protected override void LoadComplete()
                 var artist = new RomanisableString(metadata.ArtistUnicode, metadata.Artist);
 
                 titlePart = text.AddText(title, sprite => sprite.Font = OsuFont.GetFont(weight: FontWeight.Regular));
-                titlePart.DrawablePartsRecreated += _ => updateSelectionState(true);
+                titlePart.DrawablePartsRecreated += _ => updateSelectionState(SelectedSet.Value, instant: true);
 
                 text.AddText(@"  "); // to separate the title from the artist.
                 text.AddText(artist, sprite =>
@@ -66,25 +66,22 @@ protected override void LoadComplete()
                     sprite.Padding = new MarginPadding { Top = 1 };
                 });
 
-                SelectedSet.BindValueChanged(set =>
-                {
-                    bool newSelected = set.NewValue?.Equals(Model) == true;
-
-                    if (newSelected == selected)
-                        return;
-
-                    selected = newSelected;
-                    updateSelectionState(false);
-                });
-
-                updateSelectionState(true);
+                SelectedSet.BindValueChanged(set => updateSelectionState(set.NewValue, instant: false));
+                updateSelectionState(SelectedSet.Value, instant: true);
             });
         }
 
         private bool selected;
 
-        private void updateSelectionState(bool instant)
+        private void updateSelectionState(Live<BeatmapSetInfo> selectedSet, bool instant)
         {
+            bool newSelected = selectedSet?.Equals(Model) == true;
+
+            if (newSelected == selected && !instant) // instant updates should forcibly set correct state regardless of previous state.
+                return;
+
+            selected = newSelected;
+
             foreach (Drawable s in titlePart.Drawables)
                 s.FadeColour(selected ? colours.Yellow : Color4.White, instant ? 0 : FADE_DURATION);
         }

Co-Authored-By: Bartłomiej Dach <dach.bartlomiej@gmail.com>
@Joehuu Joehuu self-assigned this Apr 20, 2023
@peppy peppy self-requested a review May 2, 2023 05:07
@peppy peppy merged commit ce4a6c3 into ppy:master May 2, 2023
@Joehuu Joehuu deleted the fix-initial-playlist-highlight branch May 2, 2023 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants