Display up-to-date online status in user panels #31524
Conversation
smoogipoo
added
the
notable feature
osu.Game/Users/ExtendedUserPanel.cs
Outdated
| if (User.Equals(api?.LocalUser.Value)) | ||
| presence = new UserPresence { Status = api.Status.Value, Activity = api.Activity.Value }; | ||
| else | ||
| presence = metadata?.GetPresence(User.OnlineID); |
There was a problem hiding this comment.
Would you be against this following the standard server flow rather than a special case for the local user?
ie. Make it so the metadata server will always send the local user's presence updates back to them so they know when it's applied on the server side? personally i think this will feel better (i'd expect my own user card to update based on when the status is ack'd, rather than when applied).
|
We're going to need to do something about the spectator scenario, this looks weird:
|
| @@ -120,7 +118,7 @@ protected override void OnFocus(FocusEvent e) | |||
| searchTextBox.TakeFocus(); | |||
| } | |||
|
|
|||
| private void onUserPresenceUpdated(object sender, NotifyDictionaryChangedEventArgs<int, UserPresence> e) => Schedule(() => | |||
| private void onUserPresenceUpdated(object? sender, NotifyDictionaryChangedEventArgs<int, UserPresence> e) => Schedule(() => | |||
There was a problem hiding this comment.
With the recent changes to split out local user state, this is now missing self (you can no longer see your own panel on the online display). This can be fixed by decoupling the local list and inserting self, but I'm thinking maybe it makes more sense to keep self in the all-presences list? Interested in opinions on this one @ppy/team-client.
There was a problem hiding this comment.
I have a vague unsubstantiated recollection that when I worked on the user presence code I initially excluded self from the online display list, and then you added it back, but I'm not sure if that even really happened, I couldn't find any evidence of it on a few searches.
I'm relatively ambivalent on showing own panel. I don't think showing self brings any value, but also doesn't really seem to have any real downside, so I'm not sure I have a formed opinion at the end of the day. Maybe if there is some compelling argument to show or not show I could change my stance, I don't really see one for either option.
There was a problem hiding this comment.
@bdach Was this resolved by a server-side change? Is this PR ready for a second round of review?
Edit: I think ppy/osu-server-spectator#259 actually resolved the spectator issues i pointed out above..?
There was a problem hiding this comment.
Not to my knowledge. In this particular conversation I was attempting to chip in on whether being able to see self on this display was relevant or not.
The backlink here from my PR was ppy/osu-server-spectator#259 (comment), which has nothing to do with this particular review thread (but in general is relevant to this PR, namely the concern in #31524 (comment)).
The lack of any response to either point was why I didn't attempt picking up reviewing this any further because these look like valid concerns to me.
There was a problem hiding this comment.
Never mind, both issues are still present.
|
Current state of this PR, two concerns remain:
@smoogipoo shall i look at fixing the spectator case or did you want to? |
Resolves #13604
Prereqs:
StatusandActivitybindables fromAPIUser#31513I've chosen to ignore
APIUser.IsOnlinebecause it's potentially going to be out-of-date with the model cached inUserLookupCache. We're eventually going to have aMetadataClient.RequestPresenceUpdate(user)to keep it up-to-date, but this is fine for now because I can't find these panels actually displayed anywhere else.Preview (ignore error at the end -- nondescript segfault on random thread...):
2025-01-15.16-49-35.mp4