From 7268b2e077ab95347a12d5374cbdf505ff8538d1 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Thu, 9 Jan 2025 17:31:01 +0900 Subject: [PATCH] Add separate path for friend presence notifications 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. --- .../TestSceneFriendPresenceNotifier.cs | 14 +- osu.Game/Online/API/APIAccess.cs | 19 ++- osu.Game/Online/API/DummyAPIAccess.cs | 3 - osu.Game/Online/API/IAPIProvider.cs | 7 - osu.Game/Online/FriendPresenceNotifier.cs | 121 ++++++++++++------ osu.Game/Online/Metadata/IMetadataClient.cs | 5 + osu.Game/Online/Metadata/MetadataClient.cs | 8 ++ .../Online/Metadata/OnlineMetadataClient.cs | 34 +++-- .../Visual/Metadata/TestMetadataClient.cs | 16 ++- 9 files changed, 148 insertions(+), 79 deletions(-) diff --git a/osu.Game.Tests/Visual/Components/TestSceneFriendPresenceNotifier.cs b/osu.Game.Tests/Visual/Components/TestSceneFriendPresenceNotifier.cs index 851c1141db..2fe2326508 100644 --- a/osu.Game.Tests/Visual/Components/TestSceneFriendPresenceNotifier.cs +++ b/osu.Game.Tests/Visual/Components/TestSceneFriendPresenceNotifier.cs @@ -56,16 +56,16 @@ namespace osu.Game.Tests.Visual.Components [Test] public void TestNotifications() { - AddStep("bring friend 1 online", () => metadataClient.UserPresenceUpdated(1, new UserPresence { Status = UserStatus.Online })); + AddStep("bring friend 1 online", () => metadataClient.FriendPresenceUpdated(1, new UserPresence { Status = UserStatus.Online })); AddUntilStep("wait for notification", () => notificationOverlay.AllNotifications.Count(), () => Is.EqualTo(1)); - AddStep("bring friend 1 offline", () => metadataClient.UserPresenceUpdated(1, null)); + AddStep("bring friend 1 offline", () => metadataClient.FriendPresenceUpdated(1, null)); AddUntilStep("wait for notification", () => notificationOverlay.AllNotifications.Count(), () => Is.EqualTo(2)); } [Test] public void TestSingleUserNotificationOpensChat() { - AddStep("bring friend 1 online", () => metadataClient.UserPresenceUpdated(1, new UserPresence { Status = UserStatus.Online })); + AddStep("bring friend 1 online", () => metadataClient.FriendPresenceUpdated(1, new UserPresence { Status = UserStatus.Online })); AddUntilStep("wait for notification", () => notificationOverlay.AllNotifications.Count(), () => Is.EqualTo(1)); AddStep("click notification", () => @@ -83,8 +83,8 @@ namespace osu.Game.Tests.Visual.Components { AddStep("bring friends 1 & 2 online", () => { - metadataClient.UserPresenceUpdated(1, new UserPresence { Status = UserStatus.Online }); - metadataClient.UserPresenceUpdated(2, new UserPresence { Status = UserStatus.Online }); + metadataClient.FriendPresenceUpdated(1, new UserPresence { Status = UserStatus.Online }); + metadataClient.FriendPresenceUpdated(2, new UserPresence { Status = UserStatus.Online }); }); AddUntilStep("wait for notification", () => notificationOverlay.AllNotifications.Count(), () => Is.EqualTo(1)); @@ -112,7 +112,7 @@ namespace osu.Game.Tests.Visual.Components AddStep("bring friends 1-10 online", () => { for (int i = 1; i <= 10; i++) - metadataClient.UserPresenceUpdated(i, new UserPresence { Status = UserStatus.Online }); + metadataClient.FriendPresenceUpdated(i, new UserPresence { Status = UserStatus.Online }); }); AddUntilStep("wait for notification", () => notificationOverlay.AllNotifications.Count(), () => Is.EqualTo(1)); @@ -120,7 +120,7 @@ namespace osu.Game.Tests.Visual.Components AddStep("bring friends 1-10 offline", () => { for (int i = 1; i <= 10; i++) - metadataClient.UserPresenceUpdated(i, null); + metadataClient.FriendPresenceUpdated(i, null); }); AddUntilStep("wait for notification", () => notificationOverlay.AllNotifications.Count(), () => Is.EqualTo(2)); diff --git a/osu.Game/Online/API/APIAccess.cs b/osu.Game/Online/API/APIAccess.cs index 39c09f2a5d..46476ab7f0 100644 --- a/osu.Game/Online/API/APIAccess.cs +++ b/osu.Game/Online/API/APIAccess.cs @@ -6,6 +6,7 @@ using System; using System.Collections.Generic; using System.Diagnostics; +using System.Linq; using System.Net; using System.Net.Http; using System.Net.Sockets; @@ -18,6 +19,7 @@ using osu.Framework.Extensions.ObjectExtensions; using osu.Framework.Graphics; using osu.Framework.Logging; using osu.Game.Configuration; +using osu.Game.Extensions; using osu.Game.Localisation; using osu.Game.Online.API.Requests; using osu.Game.Online.API.Requests.Responses; @@ -75,7 +77,6 @@ namespace osu.Game.Online.API protected bool HasLogin => authentication.Token.Value != null || (!string.IsNullOrEmpty(ProvidedUsername) && !string.IsNullOrEmpty(password)); - private readonly Dictionary friendsMapping = new Dictionary(); private readonly CancellationTokenSource cancellationToken = new CancellationTokenSource(); private readonly Logger log; @@ -404,8 +405,6 @@ namespace osu.Game.Online.API public IChatClient GetChatClient() => new WebSocketChatClient(this); - public APIRelation GetFriend(int userId) => friendsMapping.GetValueOrDefault(userId); - public RegistrationRequest.RegistrationRequestErrors CreateAccount(string email, string username, string password) { Debug.Assert(State.Value == APIState.Offline); @@ -597,8 +596,6 @@ namespace osu.Game.Online.API Schedule(() => { setLocalUser(createGuestUser()); - - friendsMapping.Clear(); friends.Clear(); }); @@ -615,12 +612,14 @@ namespace osu.Game.Online.API friendsReq.Failure += _ => state.Value = APIState.Failing; friendsReq.Success += res => { - friendsMapping.Clear(); - friends.Clear(); + // Add new friends into local list. + HashSet friendsSet = friends.Select(f => f.TargetID).ToHashSet(); + friends.AddRange(res.Where(f => !friendsSet.Contains(f.TargetID))); - foreach (var u in res) - friendsMapping[u.TargetID] = u; - friends.AddRange(res); + // Remove non-friends from local lists. + friendsSet.Clear(); + friendsSet.AddRange(res.Select(f => f.TargetID)); + friends.RemoveAll(f => !friendsSet.Contains(f.TargetID)); }; Queue(friendsReq); diff --git a/osu.Game/Online/API/DummyAPIAccess.cs b/osu.Game/Online/API/DummyAPIAccess.cs index ca4edb3d8f..5d63c04925 100644 --- a/osu.Game/Online/API/DummyAPIAccess.cs +++ b/osu.Game/Online/API/DummyAPIAccess.cs @@ -2,7 +2,6 @@ // See the LICENCE file in the repository root for full licence text. using System; -using System.Linq; using System.Threading; using System.Threading.Tasks; using osu.Framework.Bindables; @@ -195,8 +194,6 @@ namespace osu.Game.Online.API public IChatClient GetChatClient() => new TestChatClientConnector(this); - public APIRelation? GetFriend(int userId) => Friends.FirstOrDefault(r => r.TargetID == userId); - public RegistrationRequest.RegistrationRequestErrors? CreateAccount(string email, string username, string password) { Thread.Sleep(200); diff --git a/osu.Game/Online/API/IAPIProvider.cs b/osu.Game/Online/API/IAPIProvider.cs index 4655b26f84..1c4b2da742 100644 --- a/osu.Game/Online/API/IAPIProvider.cs +++ b/osu.Game/Online/API/IAPIProvider.cs @@ -152,13 +152,6 @@ namespace osu.Game.Online.API /// IChatClient GetChatClient(); - /// - /// Retrieves a friend from a given user ID. - /// - /// The friend's user ID. - /// The object representing the friend, if any. - APIRelation? GetFriend(int userId); - /// /// Create a new user account. This is a blocking operation. /// diff --git a/osu.Game/Online/FriendPresenceNotifier.cs b/osu.Game/Online/FriendPresenceNotifier.cs index 655a004d3e..330e0a908f 100644 --- a/osu.Game/Online/FriendPresenceNotifier.cs +++ b/osu.Game/Online/FriendPresenceNotifier.cs @@ -2,6 +2,7 @@ // See the LICENCE file in the repository root for full licence text. using System.Collections.Generic; +using System.Collections.Specialized; using System.Linq; using osu.Framework.Allocation; using osu.Framework.Bindables; @@ -43,7 +44,10 @@ namespace osu.Game.Online private OsuConfigManager config { get; set; } = null!; private readonly Bindable notifyOnFriendPresenceChange = new BindableBool(); - private readonly IBindableDictionary userStates = new BindableDictionary(); + + private readonly IBindableList friends = new BindableList(); + private readonly IBindableDictionary friendStates = new BindableDictionary(); + private readonly HashSet onlineAlertQueue = new HashSet(); private readonly HashSet offlineAlertQueue = new HashSet(); @@ -56,42 +60,11 @@ namespace osu.Game.Online config.BindWith(OsuSetting.NotifyOnFriendPresenceChange, notifyOnFriendPresenceChange); - userStates.BindTo(metadataClient.UserStates); - userStates.BindCollectionChanged((_, args) => - { - switch (args.Action) - { - case NotifyDictionaryChangedAction.Add: - foreach ((int userId, var _) in args.NewItems!) - { - if (api.GetFriend(userId)?.TargetUser is APIUser user) - { - if (!offlineAlertQueue.Remove(user)) - { - onlineAlertQueue.Add(user); - lastOnlineAlertTime ??= Time.Current; - } - } - } + friends.BindTo(api.Friends); + friends.BindCollectionChanged(onFriendsChanged, true); - break; - - case NotifyDictionaryChangedAction.Remove: - foreach ((int userId, var _) in args.OldItems!) - { - if (api.GetFriend(userId)?.TargetUser is APIUser user) - { - if (!onlineAlertQueue.Remove(user)) - { - offlineAlertQueue.Add(user); - lastOfflineAlertTime ??= Time.Current; - } - } - } - - break; - } - }); + friendStates.BindTo(metadataClient.FriendStates); + friendStates.BindCollectionChanged(onFriendStatesChanged, true); } protected override void Update() @@ -102,6 +75,82 @@ namespace osu.Game.Online alertOfflineUsers(); } + private void onFriendsChanged(object? sender, NotifyCollectionChangedEventArgs e) + { + switch (e.Action) + { + case NotifyCollectionChangedAction.Add: + foreach (APIRelation friend in e.NewItems!.Cast()) + { + if (friend.TargetUser is not APIUser user) + continue; + + if (friendStates.TryGetValue(friend.TargetID, out _)) + markUserOnline(user); + } + + break; + + case NotifyCollectionChangedAction.Remove: + foreach (APIRelation friend in e.OldItems!.Cast()) + { + if (friend.TargetUser is not APIUser user) + continue; + + onlineAlertQueue.Remove(user); + offlineAlertQueue.Remove(user); + } + + break; + } + } + + private void onFriendStatesChanged(object? sender, NotifyDictionaryChangedEventArgs e) + { + switch (e.Action) + { + case NotifyDictionaryChangedAction.Add: + foreach ((int friendId, _) in e.NewItems!) + { + APIRelation? friend = friends.FirstOrDefault(f => f.TargetID == friendId); + + if (friend?.TargetUser is APIUser user) + markUserOnline(user); + } + + break; + + case NotifyDictionaryChangedAction.Remove: + foreach ((int friendId, _) in e.OldItems!) + { + APIRelation? friend = friends.FirstOrDefault(f => f.TargetID == friendId); + + if (friend?.TargetUser is APIUser user) + markUserOffline(user); + } + + break; + } + } + + private void markUserOnline(APIUser user) + { + if (!offlineAlertQueue.Remove(user)) + { + onlineAlertQueue.Add(user); + lastOnlineAlertTime ??= Time.Current; + } + } + + private void markUserOffline(APIUser user) + { + if (!onlineAlertQueue.Remove(user)) + { + offlineAlertQueue.Add(user); + lastOfflineAlertTime ??= Time.Current; + } + } + private void alertOnlineUsers() { if (onlineAlertQueue.Count == 0) diff --git a/osu.Game/Online/Metadata/IMetadataClient.cs b/osu.Game/Online/Metadata/IMetadataClient.cs index 97c1bbde5f..a4251fae80 100644 --- a/osu.Game/Online/Metadata/IMetadataClient.cs +++ b/osu.Game/Online/Metadata/IMetadataClient.cs @@ -21,6 +21,11 @@ namespace osu.Game.Online.Metadata /// Task UserPresenceUpdated(int userId, UserPresence? status); + /// + /// Delivers and update of the of a friend with the supplied . + /// + Task FriendPresenceUpdated(int userId, UserPresence? presence); + /// /// Delivers an update of the current "daily challenge" status. /// Null value means there is no "daily challenge" currently active. diff --git a/osu.Game/Online/Metadata/MetadataClient.cs b/osu.Game/Online/Metadata/MetadataClient.cs index 8a5fe1733e..6578f70f74 100644 --- a/osu.Game/Online/Metadata/MetadataClient.cs +++ b/osu.Game/Online/Metadata/MetadataClient.cs @@ -42,6 +42,11 @@ namespace osu.Game.Online.Metadata /// public abstract IBindableDictionary UserStates { get; } + /// + /// Dictionary keyed by user ID containing all of the information about currently online friends received from the server. + /// + public abstract IBindableDictionary FriendStates { get; } + /// public abstract Task UpdateActivity(UserActivity? activity); @@ -57,6 +62,9 @@ namespace osu.Game.Online.Metadata /// public abstract Task UserPresenceUpdated(int userId, UserPresence? presence); + /// + public abstract Task FriendPresenceUpdated(int userId, UserPresence? presence); + #endregion #region Daily Challenge diff --git a/osu.Game/Online/Metadata/OnlineMetadataClient.cs b/osu.Game/Online/Metadata/OnlineMetadataClient.cs index ef748f0b49..a8a14b1c78 100644 --- a/osu.Game/Online/Metadata/OnlineMetadataClient.cs +++ b/osu.Game/Online/Metadata/OnlineMetadataClient.cs @@ -3,7 +3,6 @@ using System; using System.Diagnostics; -using System.Linq; using System.Threading; using System.Threading.Tasks; using Microsoft.AspNetCore.SignalR.Client; @@ -27,14 +26,14 @@ namespace osu.Game.Online.Metadata public override IBindableDictionary UserStates => userStates; private readonly BindableDictionary userStates = new BindableDictionary(); + public override IBindableDictionary FriendStates => friendStates; + private readonly BindableDictionary friendStates = new BindableDictionary(); + public override IBindable DailyChallengeInfo => dailyChallengeInfo; private readonly Bindable dailyChallengeInfo = new Bindable(); private readonly string endpoint; - [Resolved] - private IAPIProvider api { get; set; } = null!; - private IHubClientConnector? connector; private Bindable lastQueueId = null!; private IBindable localUser = null!; @@ -49,7 +48,7 @@ namespace osu.Game.Online.Metadata } [BackgroundDependencyLoader] - private void load(OsuConfigManager config) + private void load(IAPIProvider api, OsuConfigManager config) { // Importantly, we are intentionally not using MessagePack here to correctly support derived class serialization. // More information on the limitations / reasoning can be found in osu-server-spectator's initialisation code. @@ -63,6 +62,7 @@ namespace osu.Game.Online.Metadata // https://github.com/dotnet/aspnetcore/issues/15198 connection.On(nameof(IMetadataClient.BeatmapSetsUpdated), ((IMetadataClient)this).BeatmapSetsUpdated); connection.On(nameof(IMetadataClient.UserPresenceUpdated), ((IMetadataClient)this).UserPresenceUpdated); + connection.On(nameof(IMetadataClient.FriendPresenceUpdated), ((IMetadataClient)this).FriendPresenceUpdated); connection.On(nameof(IMetadataClient.DailyChallengeUpdated), ((IMetadataClient)this).DailyChallengeUpdated); connection.On(nameof(IMetadataClient.MultiplayerRoomScoreSet), ((IMetadataClient)this).MultiplayerRoomScoreSet); connection.On(nameof(IStatefulUserHubClient.DisconnectRequested), ((IMetadataClient)this).DisconnectRequested); @@ -108,6 +108,7 @@ namespace osu.Game.Online.Metadata { isWatchingUserPresence.Value = false; userStates.Clear(); + friendStates.Clear(); dailyChallengeInfo.Value = null; }); return; @@ -209,6 +210,19 @@ namespace osu.Game.Online.Metadata return Task.CompletedTask; } + public override Task FriendPresenceUpdated(int userId, UserPresence? presence) + { + Schedule(() => + { + if (presence?.Status != null) + friendStates[userId] = presence.Value; + else + friendStates.Remove(userId); + }); + + return Task.CompletedTask; + } + public override async Task BeginWatchingUserPresence() { if (connector?.IsConnected.Value != true) @@ -228,15 +242,7 @@ namespace osu.Game.Online.Metadata throw new OperationCanceledException(); // must be scheduled before any remote calls to avoid mis-ordering. - Schedule(() => - { - foreach (int userId in userStates.Keys.ToArray()) - { - if (api.GetFriend(userId) == null) - userStates.Remove(userId); - } - }); - + Schedule(() => userStates.Clear()); Debug.Assert(connection != null); await connection.InvokeAsync(nameof(IMetadataServer.EndWatchingUserPresence)).ConfigureAwait(false); Logger.Log($@"{nameof(OnlineMetadataClient)} stopped watching user presence", LoggingTarget.Network); diff --git a/osu.Game/Tests/Visual/Metadata/TestMetadataClient.cs b/osu.Game/Tests/Visual/Metadata/TestMetadataClient.cs index 6dd6392b3a..36f79a5adc 100644 --- a/osu.Game/Tests/Visual/Metadata/TestMetadataClient.cs +++ b/osu.Game/Tests/Visual/Metadata/TestMetadataClient.cs @@ -2,7 +2,6 @@ // See the LICENCE file in the repository root for full licence text. using System; -using System.Linq; using System.Threading.Tasks; using osu.Framework.Allocation; using osu.Framework.Bindables; @@ -23,6 +22,9 @@ namespace osu.Game.Tests.Visual.Metadata public override IBindableDictionary UserStates => userStates; private readonly BindableDictionary userStates = new BindableDictionary(); + public override IBindableDictionary FriendStates => friendStates; + private readonly BindableDictionary friendStates = new BindableDictionary(); + public override Bindable DailyChallengeInfo => dailyChallengeInfo; private readonly Bindable dailyChallengeInfo = new Bindable(); @@ -67,7 +69,7 @@ namespace osu.Game.Tests.Visual.Metadata public override Task UserPresenceUpdated(int userId, UserPresence? presence) { - if (isWatchingUserPresence.Value || api.Friends.Any(f => f.TargetID == userId)) + if (isWatchingUserPresence.Value) { if (presence.HasValue) userStates[userId] = presence.Value; @@ -78,6 +80,16 @@ namespace osu.Game.Tests.Visual.Metadata return Task.CompletedTask; } + public override Task FriendPresenceUpdated(int userId, UserPresence? presence) + { + if (presence.HasValue) + friendStates[userId] = presence.Value; + else + friendStates.Remove(userId); + + return Task.CompletedTask; + } + public override Task GetChangesSince(int queueId) => Task.FromResult(new BeatmapUpdates(Array.Empty(), queueId));