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 nearest to selection when selection was filtered #20912

Merged
merged 15 commits into from
Oct 26, 2022

Conversation

LittleEndu
Copy link
Contributor

This PR changes carousel behaviour when currently selected beatmap gets filtered. Right now the selection is random, this PR would change that to be nearest to previous selection. This PR is basically previous PR but tests fixed.

Changes

  • Make BeatmapCarousel use PerformSelection when previous selection was filtered (instead of SelectNextRandom)
  • Make CarouselGroupEagerSelect perform selection in both direction, selecting nearest to previous selection (instead of only selecting forwards)
  • Remove test case that was testing previous random behaviour
  • Write new test case for new behaviour

@peppy peppy self-requested a review October 25, 2022 03:27
@peppy
Copy link
Sponsor Member

peppy commented Oct 25, 2022

@LittleEndu I've rewritten the main method here to avoid using LINQ, mostly because the logic you had in the LINQ was basically duplicating the non-LINQ logic for the edge case scenarios.

I think this looks good otherwise, but please check my implementation to make sure I haven't (well, the tests haven't) overlooked anything.

peppy
peppy previously approved these changes Oct 25, 2022
@LittleEndu
Copy link
Contributor Author

Looks good to me.

@peppy peppy self-requested a review October 25, 2022 04:28
@peppy
Copy link
Sponsor Member

peppy commented Oct 25, 2022

I've fixed the remaining test failure but i'm seeing occasional failures on TestAddRemove. I'm not sure if it's related to this change.

JetBrains Rider-EAP 2022-10-25 at 09 59 06

@LittleEndu

This comment was marked as outdated.

@LittleEndu
Copy link
Contributor Author

Real failure seems to be that during testing/debugging, carousel root sometimes skips eager selection, and then TestAddRemove would succeed. But if it actually performs eager selection, then its last selected index would be bigger than Items.Count so the new LINQ free solution would just return null. Changing the line to be int backwardsIndex = lastSelectedIndex == Items.Count ? Items.Count - 1 : lastSelectedIndex; would fix and makes sense if deleting last beatmap of the carousel (which the test does)

return null;

int forwardsIndex = lastSelectedIndex;
int backwardsIndex = lastSelectedIndex;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
int backwardsIndex = lastSelectedIndex;
int backwardsIndex = lastSelectedIndex == Items.Count ? Items.Count - 1 : lastSelectedIndex;

@peppy peppy merged commit 7249542 into ppy:master Oct 26, 2022
@LittleEndu LittleEndu deleted the carousel-do-not-select-random branch October 26, 2022 02:45
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.

None yet

2 participants