8000 Rename song select v2 classes and namespaces by peppy · Pull Request #32818 · ppy/osu · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Rename song select v2 classes and namespaces #32818

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

Merged
merged 1 commit into from
Apr 16, 2025
Merged

Conversation

peppy
Copy link
Member
@peppy peppy commented Apr 16, 2025

This aims to bring some conformity to naming to make it easier to understand component structure for new components.

Renames are pulled out of the song select v2 changes and are more relevant there due to many new classes being added.

  • V2 suffix is dropped, with v2 components being moved to a relevant V2 namespace.
  • Related classes have a prefix of the area they are used.
  • Experimenting with using partial/nested classes in the song select v2 implementation. Not committing to this yet but want to see how it plays out.
  • Moved base carousel components to a generic namespace to avoid confusion with actual beatmap carousel implementation.

I'm open to any proposals here (if anything in this PR is seem as odd or unwanted, I'll probably cave to any/all requests). Just trying to set a base for moving forward from more efficiently.

One file in the diff looks like a new addition unfortunately, and that's due to the partial/nested class application.

This aims to bring some conformity to naming to make it easier to
understand component structure for new components.

Renames are pulled out of the song select v2 changes and are more
relevant there due to many new classes being added.

- `V2` suffix is dropped, with v2 components being moved to a relevant V2 namespace.
- Related classes have a prefix of the area they are used.
- Experimenting with using partial/nested classes in the song select v2 implementation.
  Not committing to this yet but want to see how it plays out.
- Moved base carousel components to a generic namespace to avoid confusion with actual beatmap carousel implementation.
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.

No major objections

{
public partial class LeaderboardScoreV2 : OsuClickableContainer, IHasContextMenu, IHasCustomTooltip<ScoreInfo>
public partial class BeatmapLeaderboardScore : OsuClickableContainer, IHasContextMenu, IHasCustomTooltip<ScoreInfo>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't really get this one, this was LeaderboardScoreV2 to distinguish against old-song-select LeaderboardScore, so kinda not sure what's with the Beatmap qualifier here. Also note that this is used in daily challenge.

Not hugely fussed though, probably can stay. I'd hope the old LeaderboardScore can be old yeller'd at some point because it's pretty horrific with the way it's been contorted to fit playlists.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why Beatmap was added here, I'm fine to drop that if it makes things better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's just go with it as is.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess it was kind of there to group this in follow-up work:

JetBrains Rider-EAP 2025-04-16 at 09 54 39

@peppy peppy merged commit 3d72644 into ppy:master Apr 16, 2025
8 of 10 checks passed
@peppy peppy deleted the rename-everything branch April 16, 2025 14:41
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.

2 participants
0