Remove Status and Activity bindables from APIUser
#31513
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,6 +60,7 @@ public partial class APIAccess : Component, IAPIProvider | |
|
|
||
| public IBindable<APIUser> LocalUser => localUser; | ||
| public IBindableList<APIRelation> Friends => friends; | ||
| public Bindable<UserStatus> Status { get; } = new Bindable<UserStatus>(UserStatus.Online); | ||
| public IBindable<UserActivity> Activity => activity; | ||
|
|
||
| public INotificationsClient NotificationsClient { get; } | ||
|
|
@@ -72,9 +73,6 @@ public partial class APIAccess : Component, IAPIProvider | |
|
|
||
| private Bindable<UserActivity> activity { get; } = new Bindable<UserActivity>(); | ||
|
|
||
| protected bool HasLogin => authentication.Token.Value != null || (!string.IsNullOrEmpty(ProvidedUsername) && !string.IsNullOrEmpty(password)); | ||
|
|
||
| private readonly CancellationTokenSource cancellationToken = new CancellationTokenSource(); | ||
|
|
@@ -110,7 +108,7 @@ public APIAccess(OsuGameBase game, OsuConfigManager config, EndpointConfiguratio | |
| authentication.TokenString = config.Get<string>(OsuSetting.Token); | ||
| authentication.Token.ValueChanged += onTokenChanged; | ||
|
|
||
| config.BindWith(OsuSetting.UserOnlineStatus, Status); | ||
|
|
||
| if (HasLogin) | ||
| { | ||
|
|
@@ -121,17 +119,6 @@ public APIAccess(OsuGameBase game, OsuConfigManager config, EndpointConfiguratio | |
| state.Value = APIState.Connecting; | ||
| } | ||
|
|
||
| var thread = new Thread(run) | ||
| { | ||
| Name = "APIAccess", | ||
|
|
@@ -342,10 +329,7 @@ private void attemptConnect() | |
| { | ||
| Debug.Assert(ThreadSafety.IsUpdateThread); | ||
|
|
||
| localUser.Value = me; | ||
| state.Value = me.SessionVerified ? APIState.Online : APIState.RequiresSecondFactorAuth; | ||
| failureCount = 0; | ||
| }; | ||
|
|
@@ -381,8 +365,7 @@ private void setPlaceholderLocalUser() | |
|
|
||
| localUser.Value = new APIUser | ||
| { | ||
| Username = ProvidedUsername | ||
| }; | ||
| } | ||
|
|
||
|
|
@@ -608,7 +591,7 @@ public void Logout() | |
| password = null; | ||
| SecondFactorCode = null; | ||
| authentication.Clear(); | ||
| Status.Value = UserStatus.Online; | ||
|
There was a problem hiding this comment. I dunno about this... The point of the 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. There was a problem hiding this comment.
Can you give an example of a scenario where this is important? The way I see it:
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. There was a problem hiding this comment.
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 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. There was a problem hiding this comment.
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 There was a problem hiding this comment.
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.
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. There was a problem hiding this comment. Just had a look at this PR, and I think I prefer the solution offered in 52d1842. If In addition, if There was a problem hiding this comment. I've re-applied the change with a few other fixes, exposing it as To be perfectly clear, the only solution I'm fine with is the original presented solution - exposing it as a 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:
I do not have a solution for linking the two - given that ... that intends to isolate the config manager per-test, will not work if I simply resolve an There was a problem hiding this comment. I'm wondering: can we just remove 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()!;
}
There was a problem hiding this comment. 52d1842 originally did remove the status from There was a problem hiding this comment. Well, I've done this now, along with the suggestion from #31524
|
||
|
|
||
| // Scheduled prior to state change such that the state changed event is invoked with the correct user and their friends present | ||
| Schedule(() => | ||
|
|
||
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.
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.