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

Make beatmap carousel select a random beatmap when there was no previous selection #2332

Merged
merged 9 commits into from Apr 13, 2018

Conversation

3 participants
@LittleEndu
Copy link
Contributor

LittleEndu commented Mar 29, 2018

fixes #2264

@peppy

This comment has been minimized.

Copy link
Member

peppy commented Mar 30, 2018

Not sure this is the best way forward. The other scenario this same behaviour is expected (entering song select with an invalid selection) handles random in a different and better way

if (Carousel.SelectedBeatmapSet == null && !Carousel.SelectNextRandom())

The way you've added only ensures random to one level below, rather than actually selecting a random difficulty or obeying any other selection rules added by BeatmapCarousel.

LittleEndu and others added some commits Mar 30, 2018

@smoogipoo smoogipoo changed the title Make CarouselGroupEagerSelect select at random when last selection was null Make beatmap carousel select a random beatmap when there was no previous selection Apr 2, 2018

@smoogipoo

This comment has been minimized.

Copy link
Contributor

smoogipoo commented Apr 2, 2018

Made a few changes to this - extracted the logic into a CarouselRoot class, because I want to avoid polluting every single carousel beatmap set model with a BeatmapCarousel member.

Looks/works good otherwise 👍

{
private readonly BeatmapCarousel carousel;

public CarouselRoot(BeatmapCarousel carousel)

This comment has been minimized.

@peppy

peppy Apr 2, 2018

Member

probably want to pass the function here as a delegate, this is introducing outwards dependencies which we want to avoid.

This comment has been minimized.

@smoogipoo

smoogipoo Apr 2, 2018

Contributor

That's the reason why I went down the path of CarouselRoot. It's an alternative to delegate that serves the same purpose.

This comment has been minimized.

@peppy

peppy Apr 2, 2018

Member

as in i still don't want to see BeatmapCarousel passed in, but Action selectionFunction instead.

This comment has been minimized.

@smoogipoo

smoogipoo Apr 2, 2018

Contributor

As in I don't see a reason for that being required. If it would satiate you I would even make this a private nested class in BeatmapCarousel.

@smoogipoo

This comment has been minimized.

Copy link
Contributor

smoogipoo commented Apr 2, 2018

@peppy requesting review

@peppy

This comment has been minimized.

Copy link
Member

peppy commented Apr 2, 2018

Code looks okay, looks good if you've check functionality.

Could we not add a test to cover this for future regressions, though?

@LittleEndu

This comment has been minimized.

Copy link
Contributor Author

LittleEndu commented Apr 2, 2018

Umm, you might want to improve variable names again :/

@@ -151,6 +153,17 @@ private void checkInvisibleDifficultiesUnselectable()
AddAssert("Selection is visible", selectedBeatmapVisible);
}
private void checkNonmatchingFilter()
{
AddStep("Toggel non-matching filter", () =>

This comment has been minimized.

@LittleEndu

LittleEndu Apr 2, 2018

Author Contributor

Also typo in string :/

@LittleEndu

This comment has been minimized.

Copy link
Contributor Author

LittleEndu commented Apr 5, 2018

Other than the issues mentioned above the PR is ready to be merged.

LittleEndu added some commits Apr 6, 2018

@peppy

peppy approved these changes Apr 13, 2018

@peppy peppy merged commit 5f02007 into ppy:master Apr 13, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@LittleEndu LittleEndu deleted the LittleEndu:alt-eagerselect branch Apr 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.