-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Add notifications for when friends go online or offline #31444
base: master
Are you sure you want to change the base?
Conversation
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.
only tested behaviour, not code yet. may require some re-thinking so going to leave it here for now.
|
||
break; | ||
|
||
case NotifyDictionaryChangedAction.Remove: |
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.
When adding a friend during the current session, we end up in a bit of a predicament:
osu.2025-01-08.at.08.41.47.mp4
This is due to all states being cleared when showing / hiding the dashboard:
Schedule(() => userStates.Clear()); |
In fact, opening the dashboard breaks things even in the "simple" scenario where friends are friends from first connect.
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.
Are you intending to receive a notification immediately when adding a friend? Is that the "predicament"? If so, then that's not something that can be easily resolved, because the server will not know of changes to a user's friends list anyway until they reconnect.
As for the dashboard, I suppose the reason that's done is to save on memory which I'm not sure is super important. In any case it may be enough to only clear non-friend states.
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.
Have applied a prospective fix for the dashboard.
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 issue is only the dashboard part, correct. Showing the notification when adding a friend is optimal if it can remain that way.
It proved to be too difficult to deal with the flow that clears user states on stopping the watching of global presence updates. It's not helped in the least that friends are updated via the API, so there's a third flow to consider (and the timings therein - both server-spectator and friends are updated concurrently). Simplest is to separate the friends flow, though this does mean some logic and state duplication.
Is this series of PRs ready for re-review yet? Or are you still working on things? |
It's ready for re-review. |
Supersedes #31389
Resolves #13604
Requires ppy/osu-server-spectator#255 for proper operation, but is backwards-compatible.
Tried to match the feel and operation of the same notification in osu!stable:
2025-01-07.19-33-08.mp4