From b16edbbf524b8f95cb9a67f2bb17475dc2e34bad Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Thu, 14 Nov 2024 15:07:16 +0900 Subject: [PATCH] Make `Room.RecentParticipants` non-bindable --- .../Multiplayer/TestSceneDrawableRoom.cs | 4 +- .../TestSceneDrawableRoomParticipantsList.cs | 6 +- .../TestScenePlaylistsParticipantsList.cs | 12 ++-- .../TestScenePlaylistsRoomCreation.cs | 4 +- .../Online/Multiplayer/MultiplayerClient.cs | 6 +- osu.Game/Online/Rooms/Room.cs | 32 ++++++--- .../Components/ParticipantsDisplay.cs | 2 +- .../OnlinePlay/Components/ParticipantsList.cs | 41 ++++++++---- .../DrawableRoomParticipantsList.cs | 66 ++++++++----------- .../Screens/OnlinePlay/OnlinePlayComposite.cs | 4 -- .../OnlinePlay/TestRoomRequestsHandler.cs | 2 +- 11 files changed, 100 insertions(+), 79 deletions(-) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoom.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoom.cs index 31923b676a..d00ff72599 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoom.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoom.cs @@ -199,11 +199,11 @@ namespace osu.Game.Tests.Visual.Multiplayer if (room.RecentParticipants.Count == 0) { - room.RecentParticipants.AddRange(Enumerable.Range(0, 20).Select(i => new APIUser + room.RecentParticipants = Enumerable.Range(0, 20).Select(i => new APIUser { Id = i, Username = $"User {i}" - })); + }).ToArray(); } return new DrawableLoungeRoom(room) diff --git a/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomParticipantsList.cs b/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomParticipantsList.cs index 3d0c1be22c..b909b934b3 100644 --- a/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomParticipantsList.cs +++ b/osu.Game.Tests/Visual/Multiplayer/TestSceneDrawableRoomParticipantsList.cs @@ -138,17 +138,17 @@ namespace osu.Game.Tests.Visual.Multiplayer private void addUser(int id) { - SelectedRoom.Value.RecentParticipants.Add(new APIUser + SelectedRoom.Value.RecentParticipants = SelectedRoom.Value.RecentParticipants.Append(new APIUser { Id = id, Username = $"User {id}" - }); + }).ToArray(); SelectedRoom.Value.ParticipantCount++; } private void removeUserAt(int index) { - SelectedRoom.Value.RecentParticipants.RemoveAt(index); + SelectedRoom.Value.RecentParticipants = SelectedRoom.Value.RecentParticipants.Where(u => !u.Equals(SelectedRoom.Value.RecentParticipants[index])).ToArray(); SelectedRoom.Value.ParticipantCount--; } } diff --git a/osu.Game.Tests/Visual/Playlists/TestScenePlaylistsParticipantsList.cs b/osu.Game.Tests/Visual/Playlists/TestScenePlaylistsParticipantsList.cs index 2368a2068d..f72a2cd655 100644 --- a/osu.Game.Tests/Visual/Playlists/TestScenePlaylistsParticipantsList.cs +++ b/osu.Game.Tests/Visual/Playlists/TestScenePlaylistsParticipantsList.cs @@ -1,6 +1,7 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. +using System.Linq; using NUnit.Framework; using osu.Framework.Graphics; using osu.Game.Online.API.Requests.Responses; @@ -19,17 +20,16 @@ namespace osu.Game.Tests.Visual.Playlists AddStep("create list", () => { - SelectedRoom.Value = new Room { RoomID = 7 }; - - for (int i = 0; i < 50; i++) + SelectedRoom.Value = new Room { - SelectedRoom.Value.RecentParticipants.Add(new APIUser + RoomID = 7, + RecentParticipants = Enumerable.Range(0, 50).Select(_ => new APIUser { Username = "peppy", Statistics = new UserStatistics { GlobalRank = 1234 }, Id = 2 - }); - } + }).ToArray() + }; }); } diff --git a/osu.Game.Tests/Visual/Playlists/TestScenePlaylistsRoomCreation.cs b/osu.Game.Tests/Visual/Playlists/TestScenePlaylistsRoomCreation.cs index 031f9c272f..f75b3d7b14 100644 --- a/osu.Game.Tests/Visual/Playlists/TestScenePlaylistsRoomCreation.cs +++ b/osu.Game.Tests/Visual/Playlists/TestScenePlaylistsRoomCreation.cs @@ -62,7 +62,7 @@ namespace osu.Game.Tests.Visual.Playlists { room.Name = "my awesome room"; room.Host = API.LocalUser.Value; - room.RecentParticipants.Add(room.Host); + room.RecentParticipants = [room.Host]; room.EndDate = DateTimeOffset.Now.AddMinutes(5); room.Playlist.Add(new PlaylistItem(importedBeatmap.Beatmaps.First()) { @@ -86,7 +86,7 @@ namespace osu.Game.Tests.Visual.Playlists room.Name = "my awesome room"; room.MaxAttempts = 5; room.Host = API.LocalUser.Value; - room.RecentParticipants.Add(room.Host); + room.RecentParticipants = [room.Host]; room.EndDate = DateTimeOffset.Now.AddMinutes(5); room.Playlist.Add(new PlaylistItem(importedBeatmap.Beatmaps.First()) { diff --git a/osu.Game/Online/Multiplayer/MultiplayerClient.cs b/osu.Game/Online/Multiplayer/MultiplayerClient.cs index fe0b4c6a21..74f89c7a61 100644 --- a/osu.Game/Online/Multiplayer/MultiplayerClient.cs +++ b/osu.Game/Online/Multiplayer/MultiplayerClient.cs @@ -487,11 +487,11 @@ namespace osu.Game.Online.Multiplayer { Debug.Assert(APIRoom != null); - APIRoom.RecentParticipants.Add(user.User ?? new APIUser + APIRoom.RecentParticipants = APIRoom.RecentParticipants.Append(user.User ?? new APIUser { Id = user.UserID, Username = "[Unresolved]" - }); + }).ToArray(); APIRoom.ParticipantCount++; } @@ -506,7 +506,7 @@ namespace osu.Game.Online.Multiplayer PlayingUserIds.Remove(user.UserID); Debug.Assert(APIRoom != null); - APIRoom.RecentParticipants.RemoveAll(u => u.Id == user.UserID); + APIRoom.RecentParticipants = APIRoom.RecentParticipants.Where(u => u.Id != user.UserID).ToArray(); APIRoom.ParticipantCount--; callback?.Invoke(user); diff --git a/osu.Game/Online/Rooms/Room.cs b/osu.Game/Online/Rooms/Room.cs index 8fb3901a79..f3f8e87e54 100644 --- a/osu.Game/Online/Rooms/Room.cs +++ b/osu.Game/Online/Rooms/Room.cs @@ -137,6 +137,15 @@ namespace osu.Game.Online.Rooms set => SetField(ref participantCount, value); } + /// + /// The set of most recent participants in the room. + /// + public IReadOnlyList RecentParticipants + { + get => recentParticipants; + set => SetList(ref recentParticipants, value); + } + /// /// The match type. /// @@ -282,6 +291,9 @@ namespace osu.Game.Online.Rooms [JsonProperty("participant_count")] private int participantCount; + [JsonProperty("recent_participants")] + private IReadOnlyList recentParticipants = []; + [JsonProperty("max_attempts", DefaultValueHandling = DefaultValueHandling.Ignore)] private int? maxAttempts; @@ -324,10 +336,6 @@ namespace osu.Game.Online.Rooms [JsonProperty("playlist")] public readonly BindableList Playlist = new BindableList(); - [Cached] - [JsonProperty("recent_participants")] - public readonly BindableList RecentParticipants = new BindableList(); - /// /// Copies values from another into this one. /// @@ -369,11 +377,7 @@ namespace osu.Game.Online.Rooms Playlist.AddRange(other.Playlist); } - if (!RecentParticipants.SequenceEqual(other.RecentParticipants)) - { - RecentParticipants.Clear(); - RecentParticipants.AddRange(other.RecentParticipants); - } + RecentParticipants = other.RecentParticipants; } public void RemoveExpiredPlaylistItems() @@ -411,6 +415,16 @@ namespace osu.Game.Online.Rooms protected virtual void OnPropertyChanged([CallerMemberName] string propertyName = null!) => PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName)); + protected bool SetList(ref IReadOnlyList list, IReadOnlyList value, [CallerMemberName] string propertyName = null!) + { + if (list.SequenceEqual(value)) + return false; + + list = value; + OnPropertyChanged(propertyName); + return true; + } + protected bool SetField(ref T field, T value, [CallerMemberName] string propertyName = null!) { if (EqualityComparer.Default.Equals(field, value)) diff --git a/osu.Game/Screens/OnlinePlay/Components/ParticipantsDisplay.cs b/osu.Game/Screens/OnlinePlay/Components/ParticipantsDisplay.cs index 1ae2756514..5a040dcadb 100644 --- a/osu.Game/Screens/OnlinePlay/Components/ParticipantsDisplay.cs +++ b/osu.Game/Screens/OnlinePlay/Components/ParticipantsDisplay.cs @@ -23,7 +23,7 @@ namespace osu.Game.Screens.OnlinePlay.Components AddInternal(scroll = new OsuScrollContainer(direction) { - Child = list = new ParticipantsList() + Child = list = new ParticipantsList(room) }); switch (direction) diff --git a/osu.Game/Screens/OnlinePlay/Components/ParticipantsList.cs b/osu.Game/Screens/OnlinePlay/Components/ParticipantsList.cs index c4aefe4f99..1be23014bd 100644 --- a/osu.Game/Screens/OnlinePlay/Components/ParticipantsList.cs +++ b/osu.Game/Screens/OnlinePlay/Components/ParticipantsList.cs @@ -1,15 +1,14 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -#nullable disable - -using osu.Framework.Allocation; +using System.ComponentModel; using osu.Framework.Extensions.Color4Extensions; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; using osu.Framework.Graphics.Shapes; using osu.Framework.Threading; using osu.Game.Online.API.Requests.Responses; +using osu.Game.Online.Rooms; using osu.Game.Users.Drawables; using osuTK; @@ -57,15 +56,29 @@ namespace osu.Game.Screens.OnlinePlay.Components } } - [BackgroundDependencyLoader] - private void load() + private readonly Room room; + + public ParticipantsList(Room room) { - RecentParticipants.CollectionChanged += (_, _) => updateParticipants(); + this.room = room; + } + + protected override void LoadComplete() + { + base.LoadComplete(); + + room.PropertyChanged += onRoomPropertyChanged; updateParticipants(); } - private ScheduledDelegate scheduledUpdate; - private FillFlowContainer tiles; + private void onRoomPropertyChanged(object? sender, PropertyChangedEventArgs e) + { + if (e.PropertyName == nameof(Room.RecentParticipants)) + updateParticipants(); + } + + private ScheduledDelegate? scheduledUpdate; + private FillFlowContainer? tiles; private void updateParticipants() { @@ -83,8 +96,8 @@ namespace osu.Game.Screens.OnlinePlay.Components Spacing = Vector2.One }; - for (int i = 0; i < RecentParticipants.Count; i++) - tiles.Add(new UserTile { User = RecentParticipants[i] }); + for (int i = 0; i < room.RecentParticipants.Count; i++) + tiles.Add(new UserTile { User = room.RecentParticipants[i] }); AddInternal(tiles); @@ -92,9 +105,15 @@ namespace osu.Game.Screens.OnlinePlay.Components }); } + protected override void Dispose(bool isDisposing) + { + base.Dispose(isDisposing); + room.PropertyChanged -= onRoomPropertyChanged; + } + private partial class UserTile : CompositeDrawable { - public APIUser User + public APIUser? User { get => avatar.User; set => avatar.User = value; diff --git a/osu.Game/Screens/OnlinePlay/Lounge/Components/DrawableRoomParticipantsList.cs b/osu.Game/Screens/OnlinePlay/Lounge/Components/DrawableRoomParticipantsList.cs index 06ae939842..d989bd6c82 100644 --- a/osu.Game/Screens/OnlinePlay/Lounge/Components/DrawableRoomParticipantsList.cs +++ b/osu.Game/Screens/OnlinePlay/Lounge/Components/DrawableRoomParticipantsList.cs @@ -1,9 +1,8 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -using System.Collections.Specialized; +using System.Collections.Generic; using System.ComponentModel; -using System.Diagnostics; using System.Linq; using osu.Framework.Allocation; using osu.Framework.Graphics; @@ -165,12 +164,11 @@ namespace osu.Game.Screens.OnlinePlay.Lounge.Components { base.LoadComplete(); - RecentParticipants.BindCollectionChanged(onParticipantsChanged, true); - room.PropertyChanged += onRoomPropertyChanged; updateRoomHost(); updateRoomParticipantCount(); + updateRoomParticipants(); } private int numberOfCircles = 4; @@ -190,43 +188,38 @@ namespace osu.Game.Screens.OnlinePlay.Lounge.Components // Reinitialising the list looks janky, but this is unlikely to be used in a setting where it's visible. clearUsers(); - foreach (var u in RecentParticipants) + foreach (var u in room.RecentParticipants) addUser(u); updateHiddenUsers(); } } - private void onParticipantsChanged(object? sender, NotifyCollectionChangedEventArgs e) + private void updateRoomParticipants() { - switch (e.Action) + HashSet newUsers = room.RecentParticipants.ToHashSet(); + + avatarFlow.RemoveAll(a => { - case NotifyCollectionChangedAction.Add: - Debug.Assert(e.NewItems != null); + // Avatar with no user. Really shouldn't ever be the case but asserting it correctly is difficult. + if (a.User == null) + return false; - foreach (var added in e.NewItems.OfType()) - addUser(added); - break; + // User was previously and still is a participant. Keep them around but remove them from the new set. + // This will be useful when we add all remaining users (now just the new participants) to the flow. + if (newUsers.Contains(a.User)) + { + newUsers.Remove(a.User); + return false; + } - case NotifyCollectionChangedAction.Remove: - Debug.Assert(e.OldItems != null); + // User is no longer a participant. Remove them from the flow. + return true; + }, true); - foreach (var removed in e.OldItems.OfType()) - removeUser(removed); - break; - - case NotifyCollectionChangedAction.Reset: - clearUsers(); - break; - - case NotifyCollectionChangedAction.Replace: - case NotifyCollectionChangedAction.Move: - // Easiest is to just reinitialise the whole list. These are unlikely to ever be use cases. - clearUsers(); - foreach (var u in RecentParticipants) - addUser(u); - break; - } + // Add all remaining users to the flow. + foreach (var u in newUsers) + addUser(u); updateHiddenUsers(); } @@ -239,11 +232,6 @@ namespace osu.Game.Screens.OnlinePlay.Lounge.Components avatarFlow.Add(new CircularAvatar { User = user }); } - private void removeUser(APIUser user) - { - avatarFlow.RemoveAll(a => a.User == user, true); - } - private void clearUsers() { avatarFlow.Clear(); @@ -253,7 +241,7 @@ namespace osu.Game.Screens.OnlinePlay.Lounge.Components private void updateHiddenUsers() { int hiddenCount = 0; - if (RecentParticipants.Count > NumberOfCircles) + if (room.RecentParticipants.Count > NumberOfCircles) hiddenCount = room.ParticipantCount - NumberOfCircles + 1; hiddenUsers.Count = hiddenCount; @@ -262,7 +250,7 @@ namespace osu.Game.Screens.OnlinePlay.Lounge.Components avatarFlow.Remove(avatarFlow.Last(), true); else if (displayedCircles < NumberOfCircles) { - var nextUser = RecentParticipants.FirstOrDefault(u => avatarFlow.All(a => a.User != u)); + var nextUser = room.RecentParticipants.FirstOrDefault(u => avatarFlow.All(a => a.User != u)); if (nextUser != null) addUser(nextUser); } } @@ -278,6 +266,10 @@ namespace osu.Game.Screens.OnlinePlay.Lounge.Components case nameof(Room.ParticipantCount): updateRoomParticipantCount(); break; + + case nameof(Room.RecentParticipants): + updateRoomParticipants(); + break; } } diff --git a/osu.Game/Screens/OnlinePlay/OnlinePlayComposite.cs b/osu.Game/Screens/OnlinePlay/OnlinePlayComposite.cs index 82fc6d535c..ffe36d7609 100644 --- a/osu.Game/Screens/OnlinePlay/OnlinePlayComposite.cs +++ b/osu.Game/Screens/OnlinePlay/OnlinePlayComposite.cs @@ -4,7 +4,6 @@ using osu.Framework.Allocation; using osu.Framework.Bindables; using osu.Framework.Graphics.Containers; -using osu.Game.Online.API.Requests.Responses; using osu.Game.Online.Rooms; namespace osu.Game.Screens.OnlinePlay @@ -16,8 +15,5 @@ namespace osu.Game.Screens.OnlinePlay { [Resolved(typeof(Room))] protected BindableList Playlist { get; private set; } = null!; - - [Resolved(typeof(Room))] - protected BindableList RecentParticipants { get; private set; } = null!; } } diff --git a/osu.Game/Tests/Visual/OnlinePlay/TestRoomRequestsHandler.cs b/osu.Game/Tests/Visual/OnlinePlay/TestRoomRequestsHandler.cs index 0934c4f3dc..37325c2d6b 100644 --- a/osu.Game/Tests/Visual/OnlinePlay/TestRoomRequestsHandler.cs +++ b/osu.Game/Tests/Visual/OnlinePlay/TestRoomRequestsHandler.cs @@ -280,7 +280,7 @@ namespace osu.Game.Tests.Visual.OnlinePlay responseRoom.Password = responseRoom.HasPassword ? Guid.NewGuid().ToString() : null; if (!withParticipants) - responseRoom.RecentParticipants.Clear(); + responseRoom.RecentParticipants = []; return responseRoom; }