Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upMake BeatmapSetOverlay accept nulls everywhere #2416
Conversation
naoey
referenced this pull request
Apr 18, 2018
Merged
Add beatmap ID lookup to BeatmapSetOverlay #2350
peppy
added
code quality
user interface
labels
Apr 18, 2018
peppy
added this to the April 2018 milestone
Apr 18, 2018
This comment has been minimized.
This comment has been minimized.
|
Can you split this up into multiple PRs or even just multiple commits next time? This is super difficult to go through since so many files are touched. |
This comment has been minimized.
This comment has been minimized.
|
How would you split it up? It'd just become a commit per class or class group, at which point it's the same as just reviewing a file at a time. The changes I made are basically the same in each case. Once you start reviewing you'll understand. |
This comment has been minimized.
This comment has been minimized.
|
Reviewing one class or class group at a time would make this so much simpler. The avatar change is grouped into all of this, changes to how things are displayed in SuccessRate are grouped into all of this, changes to display and interaction of things like Beatmap in ScoresContainer are grouped into all of this. None of these components have been tested individually, so there's a lot of cases I have to go through to validate the code visually. |
This comment has been minimized.
This comment has been minimized.
|
This is one of the reasons I want individual tests per component. Right now the whole overlay is one testable component so that's how I felt it correct to apply updates. |
This comment has been minimized.
This comment has been minimized.
|
Cool, sounds like something you should've done then. |
This comment has been minimized.
This comment has been minimized.
|
Can do if you'd prefer, though it's a large time investment and the changes made here are pretty straight-forward (just moving logic to a separate method). |
| @@ -31,15 +32,25 @@ public BeatmapMetrics Metrics | ||
| const int rating_range = 10; | ||
| var ratings = Metrics.Ratings.Skip(1).Take(rating_range); // adjust for API returning weird empty data at 0. | ||
| if (metrics == null) |
This comment has been minimized.
This comment has been minimized.
smoogipoo
Apr 19, 2018
Contributor
So what happens if metrics is non-null but there are 0 ratings? Won't the else-block fail? Shouldn't this account for this scenario?
| @@ -125,6 +120,15 @@ protected override void Update() | ||
| private void updatePreviewTrack(bool playing) | ||
| { | ||
| if (playing && BeatmapSet == null) |
This comment has been minimized.
This comment has been minimized.
smoogipoo
Apr 19, 2018
Contributor
When is this ever the case? Why do we have random osu!direct play buttons with null beatmap sets? Why are we allowing this?
This comment has been minimized.
This comment has been minimized.
peppy
Apr 19, 2018
Member
The play button is still being displayed in the interface in the case the interface is loading.
This whole PR makes it possible to pass down a null beatmap in this case and now have the whole interface hidden, as previously.
| clickableArea.Action = () => profile?.ShowUser(avatar.User); | ||
| clickableArea.Action = () => | ||
| { | ||
| if (avatar.User != null) profile?.ShowUser(avatar.User); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -47,9 +47,7 @@ public TestCaseBeatmapScoresContainer() | ||
| AddStep("scores pack 1", () => scoresContainer.Scores = scores); | ||
| AddStep("scores pack 2", () => scoresContainer.Scores = anotherScores); | ||
| AddStep("only top score", () => scoresContainer.Scores = new[] { topScore }); | ||
| AddStep("remove scores", scoresContainer.CleanAllScores); | ||
| AddStep("turn on loading", () => scoresContainer.IsLoading = true); | ||
| AddStep("turn off loading", () => scoresContainer.IsLoading = false); |
This comment has been minimized.
This comment has been minimized.
smoogipoo
Apr 19, 2018
Contributor
So now there is no way to test loading of scores. This needs a test.
This comment has been minimized.
This comment has been minimized.
peppy
Apr 19, 2018
Member
There is no loading any more. I removed this because it was broken (go check it before this PR).
This comment has been minimized.
This comment has been minimized.
smoogipoo
Apr 19, 2018
Contributor
Well, sure there is no more IsLoading property, but the loading animation + retrieval of scores is still there.
| private readonly ScrollContainer scroll; | ||
| private BeatmapSetInfo beatmapSet; | ||
| public BeatmapSetInfo BeatmapSet |
This comment has been minimized.
This comment has been minimized.
peppy
added some commits
Apr 19, 2018
This comment has been minimized.
This comment has been minimized.
|
◕‿◕ sure why not |
smoogipoo
merged commit 3824d61
into
ppy:master
Apr 19, 2018
1 check passed
This comment has been minimized.
This comment has been minimized.
|
At very least I think this is a step in the right direction. |
peppy commentedApr 18, 2018
•
edited
A step forward in usability of
BeatmapSetOverlay. The default state now matches what you are returned to when displaying anullBeatmapInfo.Helps with making #2350 tidy.
BeatmapSetOverlayhierarchy now acceptsnullin its settable property.updateInfo, which is also called from BDL to provide a default state.