Allow statistic items in results screen to display without needing to watch a replay

[hlysine]

Re: #16483 (comment)

Refactor StatisticPanel so that each StatisticItem can specify whether a replay is needed (whether hit events are required) to display that item. When a replay hasn't been watched yet, only items that do not require a replay would be displayed, with a message below hinting that more statistics are available after watching a replay.

Screenshots

In the current state where all statistic items require hit events:

Adding a placeholder StatisticItem for illustration:

Note: I added scrolling to the statistics panel because it is definitely going to overflow when stuff like #16483 is added to the panel. There is already a VerticalScrollContainer in ResultsScreen that seems to be doing a similar job, but I have no idea what the point of its existence is, since it is basically non-functional even if the statistics panel overflows.

[hlysine]

Just some minor points that I missed. Should be ready for review now.

[peppy]

I would move these right to the top, and have two distinct flows with no Dispose calls. It's very weird first making the structure then disposing of it. Let me know if this isn't clear enough and I'll make a diff showing the general expectation.

[hlysine]

Yeah, I got it. It was just laziness on my part trying to not split the flow.

[hlysine]

Done. It will first check if all statistic items require hit events. If yes, the placeholder will be created instead of the scroll container.

[peppy]

Hmm, I'm not sure how much better this is.

Anyway, two things:

• Now that you added a scroll to the view, the statistics are never vertically centered. Maybe this is okay, but then is there any point in centering the replay button / message? It would make more sense to fix the paddings if it turns out that it looks ugly and have it scroll just like the rest of the content, reducing the complexity of the implementation.
• There's no test coverage of the new scenarios. Please add coverage so the new logic can be easily tested, at least in a manual way (TestSceneStatisticsPanel already has existing scenarios covered, so you can add new ones there).

[peppy]

As proposed.

[hlysine]

Now that you added a scroll to the view, the statistics are never vertically centered. Maybe this is okay, but then is there any point in centering the replay button / message? It would make more sense to fix the paddings if it turns out that it looks ugly and have it scroll just like the rest of the content, reducing the complexity of the implementation.

Actually, this was my initial implementation - to have the replay message top-aligned and scroll even if the panel is empty otherwise. However, I find it weird to have a huge amount of blank space below, and placeholders like this should generally be center-aligned imo.

There's no test coverage of the new scenarios. Please add coverage so the new logic can be easily tested, at least in a manual way (TestSceneStatisticsPanel already has existing scenarios covered, so you can add new ones there).

Will do.

[peppy]

I've rewritten the remaining logic which didn't sit well with me (mainly removing the panelIsComplete flag). I think this is fine to go as-is.

[peppy]

Taking a step back, I don't think the UX of this is good. Components which can't display their stats should still display, with a localised error / placeholder icon. That's what I was proposing in my original comment.

The way this is done, the layout is going to change against user expectations when events are not available.

Also this is a ruleset API breaking change, seemingly for no real reason (why does the drawable need to be a function if they already supported working with null/empty statistics?).

I'm going to fix the breaking change part and wait for a second opinion on whether this should be merged, or rewritten as above to provide a better UX.

[bdach]

Components which can't display their stats should still display, with a localised error / placeholder icon.

I agree with this sentiment. That's how I envisioned this working in the first place and that is how several other places already work (see global leaderboard, overlays - they don't just disappear out of sight of the user when logged out, but rather they show a placeholder stating what is required for the component to show).

[hlysine]

I don't mind rewriting if that's deemed better, but I don't think I see what you are envisioning there.

Right now, the drawables do support empty statistics themselves, but they all have different representations of emptiness (either with "not available" text, an empty graph, or just straight up not displaying anything), which looks messy to me. If you are proposing that each statistic item should display its own placeholder text (like wrapping each item in a container similar to OnlineViewContainer), then having multiple identical placeholder messages still looks weird imo.

One existing example of this situation is the BeatmapDetails panel. There are multiple items in that panel that require the user to log in, and when logged out, one of the boxes becomes empty (which is kinda weird in itself) while only the FailRetryGraph gets wrapped in an OnlineViewContainer, resulting in a single placeholder message. I don't think we can wrap just one of the StatisticItem in a container to display the placeholder text though, so I'm not sure how you would like this to work.

[peppy]

The message that you have in this PR is still fine to show at the bottom, but what I'm saying is that elements which can't be displayed should still take up the same region on the screen. I don't want the location of individual statistics to arbitrarily change based on the state of the local game. It is anti-user.

If we deem that there needs to be a standardised way to display the placeholder, then the statistics name can be used to generate these placeholders. If not (ie. each control should be able to show its own empty/placeholder state) then the current method is fine.

I think this PR is probably fine to get in as-is and add the better UX in a follow-up effort.The messaging in this PR will still be required regardless of that change.

The API change is more in line with how we'd write this to allow for potentially async loading the elements as a future step, so with the obsoletion path I think it's also fine.

Will leave this to someone else to merge though.