Split out beatmap and set panels in beatmap carousel v2 #31652
Conversation
| carouselPanel.Item = null; | ||
| carouselPanel.Selected.Value = false; | ||
| carouselPanel.KeyboardSelected.Value = false; |
There was a problem hiding this comment.
What's the significance of these operations? Should I be worrying that CarouselYPosition isn't reset here?
There was a problem hiding this comment.
I just see it as sane defaults for animation purposes. ie. new panel comes alive, if it gets selected i'd hope it goes from not-selected to selected, rather than starting with selected==true.
Should I be worrying that CarouselYPosition isn't reset here?
I assume you mean DrawYPosition? I don't know what a sane default for that would be. We could set it back to 0 but I think it's kinda up to carousel to manage that (it already transfers when retrieving from pool immediately)
There was a problem hiding this comment.
I just noticed that you moved this logic from BeatmapPanel.FreeAfterUse() which I guess gives this change more of a point. I can agree with it in that light.
|
|
||
| protected override bool OnClick(ClickEvent e) | ||
| { | ||
| carousel.CurrentSelection = Item!.Model; |
There was a problem hiding this comment.
To continue the theme above, this weirds me out. Locally the set panel select itself but then that gets hijacked to beatmap selection at Carousel level? It's a bit strange, is it supposed to be like that?
There was a problem hiding this comment.
Not too sure. How would you see it working? I'm trying to keep the knowlege of "what selects what" all in one place (BeatmapCarousel) and keep these panels as simple as possible. The other direction would be to make them very self aware.
For this specific case I guess it can already do that by setting its first available beatmap, which sounds fine and maybe preferable, until we add something like a recommendation step to the process. Once we have an external entity choosing which beatmap should be selected, I think it's going to be better to have this selection logic in BeatmapCarousel.
There was a problem hiding this comment.
I dunno... maybe at least an inline comment or something that this gets magically handled elsewhere?
The other suggestion would be to invert the flow and just give panels a delegate to request selection from the carousel.
There was a problem hiding this comment.
Hold that though until I get groups working, because selection is a bit obtuse there and i'm still finding a solution.
There was a problem hiding this comment.
I'll get back to this later then... if I remember to I guess 😅
Looks good 👍 |
In preparation for getting grouping working, let's do sets/beatmaps "properly".