Skip to content

Conversation

@peppy
Copy link
Member

@peppy peppy commented Jul 25, 2025

Not sure how I feel about this. If it seems incorrect let's just not.

As proposed in #34119.

@peppy peppy added the quick fix Tasks which were taken on because they take no time to fix label Jul 25, 2025
Not sure how I feel about this. If it seems incorrect let's just not.

As proposed in ppy#34119.
@peppy peppy force-pushed the edit-from-set-panel branch from 15c9160 to 91f01ea Compare July 25, 2025 08:51
@peppy peppy moved this from Next up to Pending Review in osu! untitled project Jul 26, 2025
@bdach bdach self-requested a review July 28, 2025 06:52
if (!Expanded.Value)
if (Expanded.Value)
{
if (songSelect is SoloSongSelect soloSongSelect)
Copy link
Collaborator

@bdach bdach Jul 28, 2025

Choose a reason for hiding this comment

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

The idea of this diff makes sense and I can't seem to break it on a quick check but I have very mixed feelings about this type of cast. We have ISongSelect which nicely abstracts this type of thing for all of the other panels and now here's this sort of stuff.

Thoughts on having a ISongSelect.GetForwardActions(BeatmapSetInfo) overload?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could do that. The only caveat is it couldn't be used in the other location I abstracted the beatmap version for (the footer menu) because that would mean the edit option would appear twice.

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.

let's go with this as is for now then I guess

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:song-select quick fix Tasks which were taken on because they take no time to fix size/S

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants