Implement sorting and display styles for currently online users
- Adds sorting and display styles.
- Saves sort/display modes to the config.
- Improves performance, especially on the 2nd+ time opening the overlay.
https://github.com/user-attachments/assets/e32b50d0-58a1-4eef-b18c-988fb497e545
Coming off some recent feedback in https://github.com/ppy/osu/discussions/33426#discussioncomment-13431275, I decided to take a bit of a detour and get a little bit more functionality in.
Sorting by rank, although it should technically work, doesn't work right now. This is because the osu!web API doesn't return user rank on /user/ lookups - it's only returned for the friends request. I'm leaving this open as a discussion topic.
- We can make osu!web return the rank and osu! will require no further changes to work correctly, or
- We can try to implement additional paths through
osu-server-spectatorwhich would blow this PR out of proportion and is best left for a task of its own.
For simplicity, I've re-implemented this display mostly as its own component for now, lifting code from FriendDisplay which was recently overhauled. These implementations should eventually be combined somehow but that's dependent on:
- Figuring out the styling - friends can display offline users for which it makes no sense to display the "spectate" button.
- Figuring out how to handle the different users/presence pathways. It's mostly a code complexity issue.
The brick layout looks absolutely silly 🤣. I think they will need to be fixed width in this context, or just disabled as non-useful (consider that the main case here is browsing users to find interesting ones – if it's only showing name there's little to gain).
On a quick read, there looks to be no pooling here. This feels like the kind of control which would highly benefit from pooling to avoid drawable churn on scroll – was there a reason for not considering this?
Although I didn't try pooling explicitly, I gave some thoughts on that and performance in general in https://github.com/ppy/osu/pull/33649/commits/f5b413b3cf4a84269aaf0ab227099491a03d6c37.
As for pooling, I assume you mean to use VirtualisedListContainer which I touch on in that commit as something that doesn't look simple to add because it has its own scroll container, whereas this container is scrolled by the overlay. I wasn't actually involved in VLC so I don't know it's limitations either, e.g. it seems to handle rows but the brick layout is not rows, and there may be other footguns I'm not aware of.
Basically what I'm getting at is if I added pooling I can only imagine it would be a custom implementation and thus subject to its own failure cases/etc and likely need testing. That doesn't seem smart to do on top of the already 1000 line diff here.
Focusing more on the functionality and code structure aspect rather than getting every drop out of performance.
A bit off-topic, but is filtering online users by gamemode not related to this? I feel like that can be included here.
I'm also skeptical about the usefulness of the brick layout here due to the sheer volatility of it:
https://github.com/user-attachments/assets/1ac2509e-2018-4247-91fc-1fd674de6191
It's kinda cool to look at, but interacting with it seems like a horrible time.
I wasn't actually involved in VLC so I don't know it's limitations either, e.g. it seems to handle rows but the brick layout is not rows, and there may be other footguns I'm not aware of.
Virtualised list really couldn't work for the brick layout, yes. Everything else it maybe could handle with some restructuring - as you say, the issue with the scrolling being provided by a higher-level drawable is a bit annoying, but maybe some redirection magic could be done to salvage it (or the display could be restructured to just have the list scroll on its own with the current header becoming sticky or something).
To save all of our times, I'm closing this PR because this is honestly effort that I can't justify putting into this. Feel free to take this PR up yourself if you believe it to be important.
Might be worth committing the minimal effort to get this merged if there's no serious technical issues? I think most of spaceman's issues could be fixed by changing / hiding sort modes until we have better support for them.
And removing the brick display completely.
I'm going to reopen this one because it was brought up once again in the latest meeting.
In doing so I'm only interested in making the least possible changes to get this merged in. Please provide a definite pathway, or close this PR once again.
Changing the toolbar to remove a button is NOT a type of change I consider simple to do. It requires:
- Multiple levels of drawable classes in order to remove that button.
- A separate enum value that does not have the brick mode.
- Separating the
OsuConfigManagerkey because currently-online/friends would have different possible display modes - consider what you expect to happen if you set "brick" mode in friends and then click on currently-online. - UNLESS the solution is to remove brick mode entirely, from both friends and currently-online.
This PR is, at its core, a refactoring to use the same structure as the friends display. This entire overlay is sorely awaiting a redesign because it is entirely inadequate.
Maybe remove the profile cover/background on the brick view to be in line with web and if the silliness is part of that. Also see why it was removed in web in the first place: https://github.com/ppy/osu-web/issues/6647
Scope expanded again: removed background on brick panels.
Also have disabled brick panels from the currently-online display in a hacky way. /shrug.