-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Implement sorting and display styles for currently online users #33649
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Using `FriendsDisplay` as a reference - that is - the "display" component is single-instance whereas the "list" component is recreated with a change in panel type. I've kept the async load part in the display component, though it doesn't have much use right now. I intend to provide the list component an initial set of users to populate with that will handle the majority of the load process, but that can come later.
I tried two other approaches here: - Using BDL to load the initial set of panels. This leads to a 10+ second initial load as we query 1000 users 50 at a time. We really shouldn't be slapping the API like this, but that will require a bit more work to get past. - Staggering loads so panels load 5ms apart. This works but looks very ugly. But, this is the best I've come up with, both from a visual and lag standpoint. There are many other things that can be done here, including pooling (probably complex to add in) and `VirtualisedListContainer` (doesn't really work because that has an internal `ScrollContainer` whereas this is being scrolled by a parent). This is just an initial step without complicating things too much.
|
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 f5b413b. As for pooling, I assume you mean to use 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. |
| // Todo: Statistics are not currently updated according to realtime user statistics, but it's also not currently displayed in the panels. | ||
| return panels.OrderByDescending(panel => panel.User.Statistics.GlobalRank.HasValue).ThenBy(panel => panel.User.Statistics.GlobalRank ?? 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The todo here is weird because the bigger problem here is that the statistics are not present at all. Every single user's global rank on this list is missing. As in, Oops It's All Nulls:
(screenshot taken using production data)
That's because the display is using UsersLookupCache, which is using LookupUsersRequest / GET api/v2/users/lookup, which doesn't return statistics (ref 1, ref 2).
You'd probably want to use something like this preload which the friends endpoint is using, but I dunno about the implications of that on web throughput. It'd be a @ppy/team-web question.
| // Todo: Last visit time is not currently updated according to realtime user presence. | ||
| return panels.OrderByDescending(panel => panel.User.LastVisit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ends up being the default sorting mode, which I don't know that we want, because it makes the list behave in a really volatile way, probably diminishing its usefulness:
Screen.Recording.2025-06-26.at.12.14.13.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is actually pretty cool, but can understand how it'd suck if you were scrolled down in the list and trying to read things.
|
I'm also skeptical about the usefulness of the brick layout here due to the sheer volatility of it: Screen.Recording.2025-06-26.at.12.15.20.movIt's kinda cool to look at, but interacting with it seems like a horrible time.
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:
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: ppy/osu-web#6647 |
|
Scope expanded again: removed background on brick panels. Also have disabled brick panels from the currently-online display in a hacky way. /shrug. |
4689517 to
79d9c32
Compare
|
@smoogipoo i've made changes here and spent more hours than i hoped. i think i made it maybe worse i dunno.
please give my changes a one-over and see if you think it's worth continuing. |
|
Your changes look fine to me. Still a lot of big issues to solve:
I think my plate is too full to tackle these right now, so my suggestion would be either take this as temporary or completely rewrite/rethink things. As we discussed the friends and currently playing tabs should probably be combined, and this should be more "stable-like". |
908c478 to
1259913
Compare
1259913 to
7fd44f2
Compare

2025-06-12.03-25-05.mp4
Coming off some recent feedback in #33426 (comment), 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.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
FriendDisplaywhich was recently overhauled. These implementations should eventually be combined somehow but that's dependent on: