Fix beatmap carousel not maintaining selection if currently selected beatmap is updated #19409
Conversation
…beatmap is updated
|
Looking at the code, doesn't this fail when the update to the beatmap is a deletion of the selected difficulty? |
|
Can you explain what you mean and which line is at fault? I can't read minds. |
| // Best effort matching. We can't use ID because in the update flow a new version will get its own GUID. | ||
| bool selectionMatches = | ||
| ((IBeatmapMetadataInfo)beatmapInfo.Metadata).Equals(SelectedBeatmapInfo.Metadata) | ||
| && beatmapInfo.DifficultyName == SelectedBeatmapInfo.DifficultyName; |
There was a problem hiding this comment.
I meant this. Isn't this true only if the difficulty also exists in the updated beatmap.
I would think if the beatmap set's metadata matches, it's OK to select any difficulty, not necessarily the one that was previously selected.
There was a problem hiding this comment.
Right. A BeatmapSet's metadata is currently just a reference to the first beatmap in the set, so it gets a bit difficult to guarantee anything there.
There are other scenarios such as if the difficulty name is changed (or the title of the beatmap is changed) which will lose selection in a similar way, so if we want to be more inclusive then a fallback selection can be added. See 24d7561.
|
I want to try this. Should I wait for tests to be added or can I test in prod? |
|
You should be able to test this in-game by modifying the beatmap however you want then update and see if it selects properly. |
|
To clarify, you can use the staging/dev environment and download a beatmap, then make a change via realm studio or the editor. |
It is possible that the import process itself marks the previous beatmaps as deleted due to an overlap in metadata or otherwise.
|
This was incorrectly blocked, should actually be okay to review. |
peppy
added
the
next release
|
@peppy Do you still plan to add tests to this PR? It's blocking merge currently. |
|
will see what i can do |
|
I've added test coverage for this (confirmed to fail on |
RFC. I've tested this to work, but adding tests is a bit involved. Want to make sure the solution seems okay before adding them.