Remove `Status` and `Activity` bindables from `APIUser` #31513

smoogipoo · 2025-01-14T15:47:07Z

Prereqs:

Always broadcast local user's state back to themselves osu-server-spectator#261

First step towards a resolution for #13604.

As for the tests, I'm (ab)using the `IsOnline` state for the time being to restore functionality.

smoogipoo · 2025-01-14T15:55:06Z

osu.Game.Tests/Visual/Online/TestSceneUserClickableAvatar.cs

-                Status =
-                {
-                    Value = UserStatus.Online
-                },
+                IsOnline = true


As mentioned in 20108e3, this is a hack that I expect to go away soon because obviously this has no way to convey user activity & dnd state.

smoogipoo · 2025-01-14T16:01:50Z

osu.Game/Configuration/OsuConfigManager.cs

-            SetDefault<UserStatus?>(OsuSetting.UserOnlineStatus, null);
+            SetDefault(OsuSetting.UserOnlineStatus, UserStatus.Online);


Note that this will throw a non-fatal runtime error if you have the previous null default value in the config:

UserOnlineStatus =

However, the value is set to Online as soon as the user logs in, and never set back to the default value after that.

This is not easy to handle because we don't have a good way to change values from nullable to non-nullable right now (if ever?), so I left it as is and hope this is okay. Adding a migration doesn't help because those are processed after all these defaults are applied.

bdach · 2025-01-15T07:58:08Z

osu.Game/Online/API/APIAccess.cs

@@ -608,7 +591,7 @@ public void Logout()
            password = null;
            SecondFactorCode = null;
            authentication.Clear();
-            configStatus.Value = UserStatus.Online;
+            Status.Value = UserStatus.Online;


I dunno about this...

The point of the configStatus / localUserStatus separation was to better control what was getting written to the config. See #26337 and #29186. While this may be functionally equivalent, I find the face-value reading of this of "when the user logs out, their status is set to... online" rather brain-breaky... (Although you could say this was weird to begin with and the value should have been set to null instead here. Guess I didn't think about that in #29186.)

I think I'd prefer the bindable split to still exist, and for the config status to still be nullable. The absolute bare minimum here for me is an explanatory comment though because at face value this just looks completely bugged.

to better control what was getting written to the config

Can you give an example of a scenario where this is important? The way I see it:

We want the status to be read from config when logged in.

We want the status to be written to the config when it changes.

So I'm asking for case (3) where the values should deviate. Is it maybe easier to think of this in the context of the other comment, where I allude to this basically being the config value just exposed in an intuitive place?

As far as I can tell, #29186 is going in the same path as I have here.

So I'm asking for case (3) where the values should deviate

Well again I don't think I have any such scenario wherein it functionally matters, but I guess in an idealistic sense, when the user logs out, I'd prefer the "config user status" to be null over Online, because there's no user in sight.

The lack of functional difference is why I'm not gonna push for any material changes in code here, but I believe at least an inline comment is warranted, because it's a line of code that looks very odd without knowing the context of this also backing a config bindable.

I'd prefer the "config user status" to be null over Online

I disagree because the config value is really "what does the user want to appear as" instead of "what is the user's current status". I would maybe even go so far as to say the state shouldn't be reset on logout - that is, if the user "wants to appear as" offline, then when they log in again they should still appear as offline.

Would adjusting the xmldoc to the status to be broadcast for the current user help things?

I would maybe even go so far as to say the state shouldn't be reset on logout - that is, if the user "want to appear as" offline, then when they log in again they should still appear as offline

That reset behaviour was intended for cases where multiple users play on a single machine. Whether that is a niche enough use case to not warrant it, I don't know, but it was accepted at review time.

Would adjusting the xmldoc to the status to be broadcast for the current user help things?

Not really, because that's not the part I have issue with - it's the "this gets written to config" part in the logout flow that is obscured / non-obvious with these changes to me specifically. Broadcasting for current user doesn't really have anything to do with config storage in my head.

Just had a look at this PR, and I think I prefer the solution offered in 52d1842. If APIAccess can avoid exposing this config setting at all, then that works the best. I don't see an issue with DiscordRichPresence and OnlineMetadataClient reading from the config rather than api.State.

In addition, if Status is still exposed via IAPIProvider, it should be an IBindable. Every write usage should request it from the configuration directly. I've tested and this should work fine.

I've re-applied the change with a few other fixes, exposing it as IBindable now.

To be perfectly clear, the only solution I'm fine with is the original presented solution - exposing it as a Bindable directly. I have many reasons for this, which I cannot convey so I'm not going to try anymore.

However, do be aware of one particular footgun. Because there are now two controllers exposing the same thing - one as read-only and one as "write-only", if you ever want to test components that utilise BOTH forms then you will need to both:

Create your own OsuConfigManager (or use the testscene's global one - up to you I guess), and set the config value directly, AND...

Propagate any values you've set also to DummyAPIAccess.Status (Bindable<>) directly.

I do not have a solution for linking the two - given that DummyAPIAccess is global for the entire test scene, code like:

new DependencyProvidingContainer { CachedDependencies = [ (typeof(OsuConfigManager), new OsuConfigManager(LocalStorage)) ] }

... that intends to isolate the config manager per-test, will not work if I simply resolve an OsuConfigManager into DummyAPIAccess like APIAccess does. For now I've exposed it from DummyAPIAccess in some usable way just so that if a case comes up then we immediately have something to work with, even if it is hacky imo. I use this in the subsequent PR here.

I'm wondering: can we just remove Status from IAPIProvider completely?

diff --git a/osu.Desktop/DiscordRichPresence.cs b/osu.Desktop/DiscordRichPresence.cs index 6c7e7d393f..cedc34dc56 100644 --- a/osu.Desktop/DiscordRichPresence.cs +++ b/osu.Desktop/DiscordRichPresence.cs @@ -54,7 +54,7 @@ internal partial class DiscordRichPresence : Component [Resolved] private OsuConfigManager config { get; set; } = null!; - private readonly IBindable<UserStatus> status = new Bindable<UserStatus>(); + private readonly Bindable<UserStatus> status = new Bindable<UserStatus>(); private readonly IBindable<UserActivity?> activity = new Bindable<UserActivity?>(); private readonly Bindable<DiscordRichPresenceMode> privacyMode = new Bindable<DiscordRichPresenceMode>(); @@ -106,9 +106,9 @@ protected override void LoadComplete() base.LoadComplete(); config.BindWith(OsuSetting.DiscordRichPresence, privacyMode); + config.BindWith(OsuSetting.UserOnlineStatus, status); user = api.LocalUser.GetBoundCopy(); - status.BindTo(api.Status); activity.BindTo(api.Activity); ruleset.BindValueChanged(_ => schedulePresenceUpdate()); diff --git a/osu.Game/Online/API/DummyAPIAccess.cs b/osu.Game/Online/API/DummyAPIAccess.cs index 3fef2b59cf..b338f4e8cb 100644 --- a/osu.Game/Online/API/DummyAPIAccess.cs +++ b/osu.Game/Online/API/DummyAPIAccess.cs @@ -197,7 +197,6 @@ public void UpdateLocalFriends() IBindable<APIUser> IAPIProvider.LocalUser => LocalUser; IBindableList<APIRelation> IAPIProvider.Friends => Friends; - IBindable<UserStatus> IAPIProvider.Status => Status; IBindable<UserActivity?> IAPIProvider.Activity => Activity; /// <summary> diff --git a/osu.Game/Online/API/IAPIProvider.cs b/osu.Game/Online/API/IAPIProvider.cs index 9ac7343885..4b87db4ecc 100644 --- a/osu.Game/Online/API/IAPIProvider.cs +++ b/osu.Game/Online/API/IAPIProvider.cs @@ -24,11 +24,6 @@ public interface IAPIProvider /// </summary> IBindableList<APIRelation> Friends { get; } - /// <summary> - /// The status for the current user that's broadcast to other players. - /// </summary> - IBindable<UserStatus> Status { get; } - /// <summary> /// The activity for the current user that's broadcast to other players. /// </summary> diff --git a/osu.Game/Online/Metadata/OnlineMetadataClient.cs b/osu.Game/Online/Metadata/OnlineMetadataClient.cs index 101307636a..26367c9c5c 100644 --- a/osu.Game/Online/Metadata/OnlineMetadataClient.cs +++ b/osu.Game/Online/Metadata/OnlineMetadataClient.cs @@ -73,9 +73,9 @@ private void load(IAPIProvider api, OsuConfigManager config) } lastQueueId = config.GetBindable<int>(OsuSetting.LastProcessedMetadataId); + userStatus = config.GetBindable<UserStatus>(OsuSetting.UserOnlineStatus); localUser = api.LocalUser.GetBoundCopy(); - userStatus = api.Status.GetBoundCopy(); userActivity = api.Activity.GetBoundCopy()!; }

52d1842 originally did remove the status from IAPIProvider completely, for whatever it's worth.

Well, I've done this now, along with the suggestion from #31524

UserStatus moved to OsuConfigManager.

UserActivity moved to SessionStatics, as I proposed above.

osu.Game/Overlays/Dashboard/CurrentlyOnlineDisplay.cs

osu.Game/Online/API/IAPIProvider.cs

peppy · 2025-01-16T07:13:14Z

osu.Game/Online/API/APIAccess.cs

@@ -608,7 +591,7 @@ public void Logout()
            password = null;
            SecondFactorCode = null;
            authentication.Clear();
-            configStatus.Value = UserStatus.Online;
+            Status.Value = UserStatus.Online;


Just had a look at this PR, and I think I prefer the solution offered in 52d1842. If APIAccess can avoid exposing this config setting at all, then that works the best. I don't see an issue with DiscordRichPresence and OnlineMetadataClient reading from the config rather than api.State.

In addition, if Status is still exposed via IAPIProvider, it should be an IBindable. Every write usage should request it from the configuration directly. I've tested and this should work fine.

peppy

Looks good apart from a small concern.

peppy · 2025-01-17T06:54:52Z

osu.Game/Online/Metadata/OnlineMetadataClient.cs

+                    if (hadLocalUserState)
+                        userStates[api.LocalUser.Value.OnlineID] = presence;


As mentioned IRL, let's keep the local user state as a separate field to simplify this.

… with changes

peppy · 2025-01-17T07:31:48Z

@smoogipoo can you check 311f08b is in line with expectations?

smoogipoo · 2025-01-17T07:35:14Z

Seems okay.

smoogipoo added 2 commits January 14, 2025 23:45

Remove Status and Activity bindables from APIUser

20108e3

As for the tests, I'm (ab)using the `IsOnline` state for the time being to restore functionality.

Make config the definitive status value

b7a9b77

smoogipoo added type:code-quality type:online labels Jan 14, 2025

pull-request-size bot added the size/L label Jan 14, 2025

smoogipoo commented Jan 14, 2025

View reviewed changes

bdach self-requested a review January 15, 2025 07:48

bdach reviewed Jan 15, 2025

View reviewed changes

bdach requested a review from peppy January 15, 2025 08:04

smoogipoo mentioned this pull request Jan 15, 2025

Display up-to-date online status in user panels #31524

Merged

1 task

Clear Discord presence when logged out

7ca3a6f

smoogipoo force-pushed the remove-status-from-apiuser branch from 52d1842 to 7ca3a6f Compare January 15, 2025 11:24

peppy requested changes Jan 16, 2025

View reviewed changes

smoogipoo added 3 commits January 16, 2025 17:05

Expose as IBindable from IAPIProvider, writes via config

b54d959

Allow setting of DummyAPIAccess status

c1f0c47

Move bindables to OsuConfigManager & SessionStatics

8400726

peppy self-requested a review January 17, 2025 06:38

smoogipoo added 2 commits January 17, 2025 15:42

Fix tests

ae7e4be

Merge branch 'master' into remove-status-from-apiuser

5425d62

peppy reviewed Jan 17, 2025

View reviewed changes

Separate the local user state

a51938f

peppy self-requested a review January 17, 2025 07:20

peppy added 2 commits January 17, 2025 16:25

Rename fields from State to Presence when presence is involved

3bb4b0c

Update TestMetadataClient to correctly set local user state in line…

311f08b

… with changes

Fix double-retrieval of user presence from dictionary in online display

41c603b

peppy approved these changes Jan 17, 2025

View reviewed changes

peppy merged commit a8456ce into ppy:master Jan 17, 2025
7 of 9 checks passed

smoogipoo mentioned this pull request Feb 21, 2025

Offline players stay on the online tab at the dashboard #31966

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `Status` and `Activity` bindables from `APIUser` #31513

Remove `Status` and `Activity` bindables from `APIUser` #31513

smoogipoo commented Jan 14, 2025 •

edited by peppy

Loading

smoogipoo Jan 14, 2025 •

edited

Loading

smoogipoo Jan 14, 2025 •

edited

Loading

bdach Jan 15, 2025

smoogipoo Jan 15, 2025 •

edited

Loading

bdach Jan 15, 2025 •

edited

Loading

smoogipoo Jan 15, 2025 •

edited

Loading

bdach Jan 15, 2025

peppy Jan 16, 2025

smoogipoo Jan 16, 2025 •

edited

Loading

peppy Jan 16, 2025

bdach Jan 16, 2025

smoogipoo Jan 16, 2025 •

edited

Loading

peppy Jan 16, 2025

peppy left a comment

peppy Jan 17, 2025

peppy commented Jan 17, 2025

smoogipoo commented Jan 17, 2025

		SetDefault<UserStatus?>(OsuSetting.UserOnlineStatus, null);
		SetDefault(OsuSetting.UserOnlineStatus, UserStatus.Online);

		if (hadLocalUserState)
		userStates[api.LocalUser.Value.OnlineID] = presence;

Remove Status and Activity bindables from APIUser #31513

Remove Status and Activity bindables from APIUser #31513

Conversation

smoogipoo commented Jan 14, 2025 • edited by peppy Loading

smoogipoo Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

smoogipoo Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

bdach Jan 15, 2025

Choose a reason for hiding this comment

smoogipoo Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

bdach Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

smoogipoo Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

bdach Jan 15, 2025

Choose a reason for hiding this comment

peppy Jan 16, 2025

Choose a reason for hiding this comment

smoogipoo Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

peppy Jan 16, 2025

Choose a reason for hiding this comment

bdach Jan 16, 2025

Choose a reason for hiding this comment

smoogipoo Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

peppy Jan 16, 2025

Choose a reason for hiding this comment

peppy left a comment

Choose a reason for hiding this comment

peppy Jan 17, 2025

Choose a reason for hiding this comment

peppy commented Jan 17, 2025

smoogipoo commented Jan 17, 2025

Remove `Status` and `Activity` bindables from `APIUser` #31513

Remove `Status` and `Activity` bindables from `APIUser` #31513

smoogipoo commented Jan 14, 2025 •

edited by peppy

Loading

smoogipoo Jan 14, 2025 •

edited

Loading

smoogipoo Jan 14, 2025 •

edited

Loading

smoogipoo Jan 15, 2025 •

edited

Loading

bdach Jan 15, 2025 •

edited

Loading

smoogipoo Jan 15, 2025 •

edited

Loading

smoogipoo Jan 16, 2025 •

edited

Loading

smoogipoo Jan 16, 2025 •

edited

Loading