Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add group support to beatmap carousel v2 #31764

Merged
merged 11 commits into from
Feb 4, 2025

Conversation

peppy
Copy link
Member

@peppy peppy commented Feb 2, 2025

This adds the basics of grouping – which is to say difficulty grouping.

Important changes:

  • 645c26c I split out keyboard and group traversal into two methods because this reduces the complexity (less conditional pathing)
  • d74939e Removed IsGroupSelectionTarget from CarouselItem. This is no longer required and is instead handled using CheckValidForGroupSelection. Less memory overhead and less complexity (basically allows behaviour to be implemented for traversal of groups without even more bool flags).
2025-02-03.02.32.23.mp4

What's next?

  • Expanded state for groups and sets (required for display purposes, mainly). Probably IsVisible becomes a ternary enum { Hidden, Contracted, Expanded }.
  • Grouping with nested sets (ie. artist / title / collection grouping). Should in theory work with no changes.

@peppy peppy force-pushed the beatmap-carousel-v2-grouping branch from 718cec1 to 6a18d18 Compare February 2, 2025 17:39
@bdach bdach self-requested a review February 3, 2025 09:27
{
newItems.Add(new CarouselItem(b.BeatmapSet!)
if (groupSetsTogether)
Copy link
Collaborator

Choose a reason for hiding this comment

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

isn't this already checked in the outermost conditional? (line 80)

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed this.

@bdach
Copy link
Collaborator

bdach commented Feb 3, 2025

Not sure how much to nitpick if at all over input handling, so please cut me off if this is out of scope for now, but this seems like a possibly unintended behaviour:

2025-02-03.13-18-07.mp4

When a group header is keyboard-preselected, attempting to navigate to previous or next group toggles the expanded state of the group instead.

@peppy
Copy link
Member Author

peppy commented Feb 3, 2025

When a group header is keyboard-preselected, attempting to navigate to previous or next group toggles the expanded state of the group instead.

I've attempted to fix this (and added test coverage), please give it another go and also check whether the code still makes logical sense. The important change was to actually confirm that keyboard selection transfer actually worked, and if it doesn't continue with normal logic.

@bdach bdach self-requested a review February 4, 2025 07:01
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.

Seems ok to go forward with

@bdach bdach merged commit 6e59cab into ppy:master Feb 4, 2025
6 of 10 checks passed
@peppy peppy deleted the beatmap-carousel-v2-grouping branch February 4, 2025 07:34
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants