From 8985a387344b91ce8ec48da0bfc183db67b14b4f Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Wed, 15 Jan 2025 16:53:55 +0900 Subject: [PATCH 01/18] Display up-to-date online status in user panels --- .../Visual/Online/TestSceneUserPanel.cs | 221 +++++++++--------- osu.Game/Online/Metadata/MetadataClient.cs | 16 ++ .../Dashboard/CurrentlyOnlineDisplay.cs | 59 ++--- osu.Game/Users/ExtendedUserPanel.cs | 105 +++++---- 4 files changed, 202 insertions(+), 199 deletions(-) diff --git a/osu.Game.Tests/Visual/Online/TestSceneUserPanel.cs b/osu.Game.Tests/Visual/Online/TestSceneUserPanel.cs index 4c2e47d336..b4dafd3107 100644 --- a/osu.Game.Tests/Visual/Online/TestSceneUserPanel.cs +++ b/osu.Game.Tests/Visual/Online/TestSceneUserPanel.cs @@ -4,17 +4,18 @@ using System; using NUnit.Framework; using osu.Framework.Allocation; -using osu.Framework.Bindables; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; using osu.Framework.Utils; using osu.Game.Beatmaps; using osu.Game.Online; using osu.Game.Online.API.Requests.Responses; +using osu.Game.Online.Metadata; using osu.Game.Overlays; using osu.Game.Rulesets; using osu.Game.Scoring; using osu.Game.Tests.Beatmaps; +using osu.Game.Tests.Visual.Metadata; using osu.Game.Users; using osuTK; @@ -23,144 +24,138 @@ namespace osu.Game.Tests.Visual.Online [TestFixture] public partial class TestSceneUserPanel : OsuTestScene { - private readonly Bindable activity = new Bindable(); - private readonly Bindable status = new Bindable(); - - private UserGridPanel boundPanel1 = null!; - private TestUserListPanel boundPanel2 = null!; - [Cached] private readonly OverlayColourProvider colourProvider = new OverlayColourProvider(OverlayColourScheme.Purple); - [Cached(typeof(LocalUserStatisticsProvider))] - private readonly TestUserStatisticsProvider statisticsProvider = new TestUserStatisticsProvider(); - [Resolved] private IRulesetStore rulesetStore { get; set; } = null!; + private TestUserStatisticsProvider statisticsProvider = null!; + private TestMetadataClient metadataClient = null!; + private TestUserListPanel panel = null!; + [SetUp] public void SetUp() => Schedule(() => { - activity.Value = null; - status.Value = null; - - Remove(statisticsProvider, false); - Clear(); - Add(statisticsProvider); - - Add(new FillFlowContainer + Child = new DependencyProvidingContainer { - Anchor = Anchor.Centre, - Origin = Anchor.Centre, - AutoSizeAxes = Axes.Y, - RelativeSizeAxes = Axes.X, - Spacing = new Vector2(10f), + RelativeSizeAxes = Axes.Both, + CachedDependencies = + [ + (typeof(LocalUserStatisticsProvider), statisticsProvider = new TestUserStatisticsProvider()), + (typeof(MetadataClient), metadataClient = new TestMetadataClient()) + ], Children = new Drawable[] { - new UserBrickPanel(new APIUser + statisticsProvider, + metadataClient, + new FillFlowContainer { - Username = @"flyte", - Id = 3103765, - CoverUrl = @"https://assets.ppy.sh/user-cover-presets/1/df28696b58541a9e67f6755918951d542d93bdf1da41720fcca2fd2c1ea8cf51.jpeg", - }), - new UserBrickPanel(new APIUser - { - Username = @"peppy", - Id = 2, - Colour = "99EB47", - CoverUrl = @"https://assets.ppy.sh/user-profile-covers/8195163/4a8e2ad5a02a2642b631438cfa6c6bd7e2f9db289be881cb27df18331f64144c.jpeg", - }), - new UserGridPanel(new APIUser - { - Username = @"flyte", - Id = 3103765, - CountryCode = CountryCode.JP, - CoverUrl = @"https://assets.ppy.sh/user-cover-presets/1/df28696b58541a9e67f6755918951d542d93bdf1da41720fcca2fd2c1ea8cf51.jpeg", - IsOnline = true - }) { Width = 300 }, - boundPanel1 = new UserGridPanel(new APIUser - { - Username = @"peppy", - Id = 2, - CountryCode = CountryCode.AU, - CoverUrl = @"https://assets.ppy.sh/user-profile-covers/8195163/4a8e2ad5a02a2642b631438cfa6c6bd7e2f9db289be881cb27df18331f64144c.jpeg", - IsSupporter = true, - SupportLevel = 3, - }) { Width = 300 }, - boundPanel2 = new TestUserListPanel(new APIUser - { - Username = @"Evast", - Id = 8195163, - CountryCode = CountryCode.BY, - CoverUrl = @"https://assets.ppy.sh/user-profile-covers/8195163/4a8e2ad5a02a2642b631438cfa6c6bd7e2f9db289be881cb27df18331f64144c.jpeg", - IsOnline = false, - LastVisit = DateTimeOffset.Now - }), - new UserRankPanel(new APIUser - { - Username = @"flyte", - Id = 3103765, - CountryCode = CountryCode.JP, - CoverUrl = @"https://assets.ppy.sh/user-cover-presets/1/df28696b58541a9e67f6755918951d542d93bdf1da41720fcca2fd2c1ea8cf51.jpeg", - Statistics = new UserStatistics { GlobalRank = 12345, CountryRank = 1234 } - }) { Width = 300 }, - new UserRankPanel(new APIUser - { - Username = @"peppy", - Id = 2, - Colour = "99EB47", - CountryCode = CountryCode.AU, - CoverUrl = @"https://assets.ppy.sh/user-profile-covers/8195163/4a8e2ad5a02a2642b631438cfa6c6bd7e2f9db289be881cb27df18331f64144c.jpeg", - Statistics = new UserStatistics { GlobalRank = null, CountryRank = null } - }) { Width = 300 } + Anchor = Anchor.Centre, + Origin = Anchor.Centre, + AutoSizeAxes = Axes.Y, + RelativeSizeAxes = Axes.X, + Spacing = new Vector2(10f), + Children = new Drawable[] + { + new UserBrickPanel(new APIUser + { + Username = @"flyte", + Id = 3103765, + CoverUrl = @"https://assets.ppy.sh/user-cover-presets/1/df28696b58541a9e67f6755918951d542d93bdf1da41720fcca2fd2c1ea8cf51.jpeg", + }), + new UserBrickPanel(new APIUser + { + Username = @"peppy", + Id = 2, + Colour = "99EB47", + CoverUrl = @"https://assets.ppy.sh/user-profile-covers/8195163/4a8e2ad5a02a2642b631438cfa6c6bd7e2f9db289be881cb27df18331f64144c.jpeg", + }), + new UserGridPanel(new APIUser + { + Username = @"flyte", + Id = 3103765, + CountryCode = CountryCode.JP, + CoverUrl = @"https://assets.ppy.sh/user-cover-presets/1/df28696b58541a9e67f6755918951d542d93bdf1da41720fcca2fd2c1ea8cf51.jpeg", + IsOnline = true + }) { Width = 300 }, + new UserGridPanel(new APIUser + { + Username = @"peppy", + Id = 2, + CountryCode = CountryCode.AU, + CoverUrl = @"https://assets.ppy.sh/user-profile-covers/8195163/4a8e2ad5a02a2642b631438cfa6c6bd7e2f9db289be881cb27df18331f64144c.jpeg", + IsSupporter = true, + SupportLevel = 3, + }) { Width = 300 }, + panel = new TestUserListPanel(new APIUser + { + Username = @"peppy", + Id = 2, + CountryCode = CountryCode.AU, + CoverUrl = @"https://assets.ppy.sh/user-profile-covers/8195163/4a8e2ad5a02a2642b631438cfa6c6bd7e2f9db289be881cb27df18331f64144c.jpeg", + LastVisit = DateTimeOffset.Now + }), + new UserRankPanel(new APIUser + { + Username = @"flyte", + Id = 3103765, + CountryCode = CountryCode.JP, + CoverUrl = @"https://assets.ppy.sh/user-cover-presets/1/df28696b58541a9e67f6755918951d542d93bdf1da41720fcca2fd2c1ea8cf51.jpeg", + Statistics = new UserStatistics { GlobalRank = 12345, CountryRank = 1234 } + }) { Width = 300 }, + new UserRankPanel(new APIUser + { + Username = @"peppy", + Id = 2, + Colour = "99EB47", + CountryCode = CountryCode.AU, + CoverUrl = @"https://assets.ppy.sh/user-profile-covers/8195163/4a8e2ad5a02a2642b631438cfa6c6bd7e2f9db289be881cb27df18331f64144c.jpeg", + Statistics = new UserStatistics { GlobalRank = null, CountryRank = null } + }) { Width = 300 } + } + } } - }); + }; - boundPanel1.Status.BindTo(status); - boundPanel1.Activity.BindTo(activity); - - boundPanel2.Status.BindTo(status); - boundPanel2.Activity.BindTo(activity); + metadataClient.BeginWatchingUserPresence(); }); [Test] public void TestUserStatus() { - AddStep("online", () => status.Value = UserStatus.Online); - AddStep("do not disturb", () => status.Value = UserStatus.DoNotDisturb); - AddStep("offline", () => status.Value = UserStatus.Offline); - AddStep("null status", () => status.Value = null); + AddStep("online", () => setPresence(UserStatus.Online, null)); + AddStep("do not disturb", () => setPresence(UserStatus.DoNotDisturb, null)); + AddStep("offline", () => setPresence(UserStatus.Offline, null)); } [Test] public void TestUserActivity() { - AddStep("set online status", () => status.Value = UserStatus.Online); - - AddStep("idle", () => activity.Value = null); - AddStep("watching replay", () => activity.Value = new UserActivity.WatchingReplay(createScore(@"nats"))); - AddStep("spectating user", () => activity.Value = new UserActivity.SpectatingUser(createScore(@"mrekk"))); - AddStep("solo (osu!)", () => activity.Value = soloGameStatusForRuleset(0)); - AddStep("solo (osu!taiko)", () => activity.Value = soloGameStatusForRuleset(1)); - AddStep("solo (osu!catch)", () => activity.Value = soloGameStatusForRuleset(2)); - AddStep("solo (osu!mania)", () => activity.Value = soloGameStatusForRuleset(3)); - AddStep("choosing", () => activity.Value = new UserActivity.ChoosingBeatmap()); - AddStep("editing beatmap", () => activity.Value = new UserActivity.EditingBeatmap(new BeatmapInfo())); - AddStep("modding beatmap", () => activity.Value = new UserActivity.ModdingBeatmap(new BeatmapInfo())); - AddStep("testing beatmap", () => activity.Value = new UserActivity.TestingBeatmap(new BeatmapInfo())); + AddStep("idle", () => setPresence(UserStatus.Online, null)); + AddStep("watching replay", () => setPresence(UserStatus.Online, new UserActivity.WatchingReplay(createScore(@"nats")))); + AddStep("spectating user", () => setPresence(UserStatus.Online, new UserActivity.SpectatingUser(createScore(@"mrekk")))); + AddStep("solo (osu!)", () => setPresence(UserStatus.Online, soloGameStatusForRuleset(0))); + AddStep("solo (osu!taiko)", () => setPresence(UserStatus.Online, soloGameStatusForRuleset(1))); + AddStep("solo (osu!catch)", () => setPresence(UserStatus.Online, soloGameStatusForRuleset(2))); + AddStep("solo (osu!mania)", () => setPresence(UserStatus.Online, soloGameStatusForRuleset(3))); + AddStep("choosing", () => setPresence(UserStatus.Online, new UserActivity.ChoosingBeatmap())); + AddStep("editing beatmap", () => setPresence(UserStatus.Online, new UserActivity.EditingBeatmap(new BeatmapInfo()))); + AddStep("modding beatmap", () => setPresence(UserStatus.Online, new UserActivity.ModdingBeatmap(new BeatmapInfo()))); + AddStep("testing beatmap", () => setPresence(UserStatus.Online, new UserActivity.TestingBeatmap(new BeatmapInfo()))); } [Test] public void TestUserActivityChange() { - AddAssert("visit message is visible", () => boundPanel2.LastVisitMessage.IsPresent); - AddStep("set online status", () => status.Value = UserStatus.Online); - AddAssert("visit message is not visible", () => !boundPanel2.LastVisitMessage.IsPresent); - AddStep("set choosing activity", () => activity.Value = new UserActivity.ChoosingBeatmap()); - AddStep("set offline status", () => status.Value = UserStatus.Offline); - AddAssert("visit message is visible", () => boundPanel2.LastVisitMessage.IsPresent); - AddStep("set online status", () => status.Value = UserStatus.Online); - AddAssert("visit message is not visible", () => !boundPanel2.LastVisitMessage.IsPresent); + AddAssert("visit message is visible", () => panel.LastVisitMessage.IsPresent); + AddStep("set online status", () => setPresence(UserStatus.Online, null)); + AddAssert("visit message is not visible", () => !panel.LastVisitMessage.IsPresent); + AddStep("set choosing activity", () => setPresence(UserStatus.Online, new UserActivity.ChoosingBeatmap())); + AddStep("set offline status", () => setPresence(UserStatus.Offline, null)); + AddAssert("visit message is visible", () => panel.LastVisitMessage.IsPresent); + AddStep("set online status", () => setPresence(UserStatus.Online, null)); + AddAssert("visit message is not visible", () => !panel.LastVisitMessage.IsPresent); } [Test] @@ -185,6 +180,14 @@ namespace osu.Game.Tests.Visual.Online AddStep("set statistics to empty", () => statisticsProvider.UpdateStatistics(new UserStatistics(), Ruleset.Value)); } + private void setPresence(UserStatus status, UserActivity? activity) + { + if (status == UserStatus.Offline) + metadataClient.UserPresenceUpdated(panel.User.OnlineID, null); + else + metadataClient.UserPresenceUpdated(panel.User.OnlineID, new UserPresence { Status = status, Activity = activity }); + } + private UserActivity soloGameStatusForRuleset(int rulesetId) => new UserActivity.InSoloGame(new BeatmapInfo(), rulesetStore.GetRuleset(rulesetId)!); private ScoreInfo createScore(string name) => new ScoreInfo(new TestBeatmap(Ruleset.Value).BeatmapInfo) diff --git a/osu.Game/Online/Metadata/MetadataClient.cs b/osu.Game/Online/Metadata/MetadataClient.cs index 6578f70f74..8f1fe0641f 100644 --- a/osu.Game/Online/Metadata/MetadataClient.cs +++ b/osu.Game/Online/Metadata/MetadataClient.cs @@ -47,6 +47,22 @@ namespace osu.Game.Online.Metadata /// public abstract IBindableDictionary FriendStates { get; } + /// + /// Attempts to retrieve the presence of a user. + /// + /// The user ID. + /// The user presence, or null if not available or the user's offline. + public UserPresence? GetPresence(int userId) + { + if (FriendStates.TryGetValue(userId, out UserPresence presence)) + return presence; + + if (UserStates.TryGetValue(userId, out presence)) + return presence; + + return null; + } + /// public abstract Task UpdateActivity(UserActivity? activity); diff --git a/osu.Game/Overlays/Dashboard/CurrentlyOnlineDisplay.cs b/osu.Game/Overlays/Dashboard/CurrentlyOnlineDisplay.cs index 2ca548fdf5..ef07f4538c 100644 --- a/osu.Game/Overlays/Dashboard/CurrentlyOnlineDisplay.cs +++ b/osu.Game/Overlays/Dashboard/CurrentlyOnlineDisplay.cs @@ -1,8 +1,6 @@ // 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 System.Collections.Generic; using System.Collections.Specialized; using System.Diagnostics; @@ -40,17 +38,20 @@ namespace osu.Game.Overlays.Dashboard private readonly IBindableDictionary onlineUsers = new BindableDictionary(); private readonly Dictionary userPanels = new Dictionary(); - private SearchContainer userFlow; - private BasicSearchTextBox searchTextBox; + private SearchContainer userFlow = null!; + private BasicSearchTextBox searchTextBox = null!; [Resolved] - private IAPIProvider api { get; set; } + private IAPIProvider api { get; set; } = null!; [Resolved] - private SpectatorClient spectatorClient { get; set; } + private SpectatorClient spectatorClient { get; set; } = null!; [Resolved] - private MetadataClient metadataClient { get; set; } + private MetadataClient metadataClient { get; set; } = null!; + + [Resolved] + private UserLookupCache users { get; set; } = null!; [BackgroundDependencyLoader] private void load(OverlayColourProvider colourProvider) @@ -99,9 +100,6 @@ namespace osu.Game.Overlays.Dashboard searchTextBox.Current.ValueChanged += text => userFlow.SearchTerm = text.NewValue; } - [Resolved] - private UserLookupCache users { get; set; } - protected override void LoadComplete() { base.LoadComplete(); @@ -120,7 +118,7 @@ namespace osu.Game.Overlays.Dashboard searchTextBox.TakeFocus(); } - private void onUserUpdated(object sender, NotifyDictionaryChangedEventArgs e) => Schedule(() => + private void onUserUpdated(object? sender, NotifyDictionaryChangedEventArgs e) => Schedule(() => { switch (e.Action) { @@ -133,38 +131,13 @@ namespace osu.Game.Overlays.Dashboard users.GetUserAsync(userId).ContinueWith(task => { - APIUser user = task.GetResultSafely(); - - if (user == null) - return; - - Schedule(() => - { - userFlow.Add(userPanels[userId] = createUserPanel(user).With(p => - { - p.Status.Value = onlineUsers.GetValueOrDefault(userId).Status; - p.Activity.Value = onlineUsers.GetValueOrDefault(userId).Activity; - })); - }); + if (task.GetResultSafely() is APIUser user) + Schedule(() => userFlow.Add(userPanels[userId] = createUserPanel(user))); }); } break; - case NotifyDictionaryChangedAction.Replace: - Debug.Assert(e.NewItems != null); - - foreach (var kvp in e.NewItems) - { - if (userPanels.TryGetValue(kvp.Key, out var panel)) - { - panel.Activity.Value = kvp.Value.Activity; - panel.Status.Value = kvp.Value.Status; - } - } - - break; - case NotifyDictionaryChangedAction.Remove: Debug.Assert(e.OldItems != null); @@ -179,7 +152,7 @@ namespace osu.Game.Overlays.Dashboard } }); - private void onPlayingUsersChanged(object sender, NotifyCollectionChangedEventArgs e) + private void onPlayingUsersChanged(object? sender, NotifyCollectionChangedEventArgs e) { switch (e.Action) { @@ -219,9 +192,6 @@ namespace osu.Game.Overlays.Dashboard { public readonly APIUser User; - public readonly Bindable Status = new Bindable(); - public readonly Bindable Activity = new Bindable(); - public BindableBool CanSpectate { get; } = new BindableBool(); public IEnumerable FilterTerms { get; } @@ -268,10 +238,7 @@ namespace osu.Game.Overlays.Dashboard { RelativeSizeAxes = Axes.X, Anchor = Anchor.TopCentre, - Origin = Anchor.TopCentre, - // this is SHOCKING - Activity = { BindTarget = Activity }, - Status = { BindTarget = Status }, + Origin = Anchor.TopCentre }, new PurpleRoundedButton { diff --git a/osu.Game/Users/ExtendedUserPanel.cs b/osu.Game/Users/ExtendedUserPanel.cs index e33fb7a44e..eb1115e296 100644 --- a/osu.Game/Users/ExtendedUserPanel.cs +++ b/osu.Game/Users/ExtendedUserPanel.cs @@ -1,10 +1,8 @@ // 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 System; using osu.Framework.Allocation; -using osu.Framework.Bindables; using osu.Framework.Extensions; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; @@ -14,44 +12,56 @@ using osu.Game.Graphics.Sprites; using osu.Game.Users.Drawables; using osu.Framework.Input.Events; using osu.Framework.Localisation; +using osu.Game.Online.API; using osu.Game.Online.API.Requests.Responses; +using osu.Game.Online.Metadata; namespace osu.Game.Users { public abstract partial class ExtendedUserPanel : UserPanel { - public readonly Bindable Status = new Bindable(); + protected TextFlowContainer LastVisitMessage { get; private set; } = null!; - public readonly IBindable Activity = new Bindable(); + private StatusIcon statusIcon = null!; + private StatusText statusMessage = null!; - protected TextFlowContainer LastVisitMessage { get; private set; } + [Resolved] + private MetadataClient? metadata { get; set; } - private StatusIcon statusIcon; - private StatusText statusMessage; + [Resolved] + private IAPIProvider? api { get; set; } + + private UserStatus? lastStatus; + private UserActivity? lastActivity; + private DateTimeOffset? lastVisit; protected ExtendedUserPanel(APIUser user) : base(user) { + lastVisit = user.LastVisit; } [BackgroundDependencyLoader] private void load() { BorderColour = ColourProvider?.Light1 ?? Colours.GreyVioletLighter; - - Status.ValueChanged += status => displayStatus(status.NewValue, Activity.Value); - Activity.ValueChanged += activity => displayStatus(Status.Value, activity.NewValue); } protected override void LoadComplete() { base.LoadComplete(); - Status.TriggerChange(); + updatePresence(); // Colour should be applied immediately on first load. statusIcon.FinishTransforms(); } + protected override void Update() + { + base.Update(); + updatePresence(); + } + protected Container CreateStatusIcon() => statusIcon = new StatusIcon(); protected FillFlowContainer CreateStatusMessage(bool rightAlignedChildren) @@ -70,15 +80,6 @@ namespace osu.Game.Users text.Origin = alignment; text.AutoSizeAxes = Axes.Both; text.Alpha = 0; - - if (User.LastVisit.HasValue) - { - text.AddText(@"Last seen "); - text.AddText(new DrawableDate(User.LastVisit.Value, italic: false) - { - Shadow = false - }); - } })); statusContainer.Add(statusMessage = new StatusText @@ -91,37 +92,53 @@ namespace osu.Game.Users return statusContainer; } - private void displayStatus(UserStatus? status, UserActivity activity = null) + private void updatePresence() { - if (status != null) + UserPresence? presence; + + if (User.Equals(api?.LocalUser.Value)) + presence = new UserPresence { Status = api.Status.Value, Activity = api.Activity.Value }; + else + presence = metadata?.GetPresence(User.OnlineID); + + UserStatus status = presence?.Status ?? UserStatus.Offline; + UserActivity? activity = presence?.Activity; + + if (status == lastStatus && activity == lastActivity) + return; + + if (status == UserStatus.Offline && lastVisit != null) { - LastVisitMessage.FadeTo(status == UserStatus.Offline && User.LastVisit.HasValue ? 1 : 0); - - // Set status message based on activity (if we have one) and status is not offline - if (activity != null && status != UserStatus.Offline) + LastVisitMessage.FadeTo(1); + LastVisitMessage.Clear(); + LastVisitMessage.AddText(@"Last seen "); + LastVisitMessage.AddText(new DrawableDate(lastVisit.Value, italic: false) { - statusMessage.Text = activity.GetStatus(); - statusMessage.TooltipText = activity.GetDetails(); - statusIcon.FadeColour(activity.GetAppropriateColour(Colours), 500, Easing.OutQuint); - return; - } + Shadow = false + }); + } + else + LastVisitMessage.FadeTo(0); - // Otherwise use only status + // Set status message based on activity (if we have one) and status is not offline + if (activity != null && status != UserStatus.Offline) + { + statusMessage.Text = activity.GetStatus(); + statusMessage.TooltipText = activity.GetDetails() ?? string.Empty; + statusIcon.FadeColour(activity.GetAppropriateColour(Colours), 500, Easing.OutQuint); + } + + // Otherwise use only status + else + { statusMessage.Text = status.GetLocalisableDescription(); statusMessage.TooltipText = string.Empty; - statusIcon.FadeColour(status.Value.GetAppropriateColour(Colours), 500, Easing.OutQuint); - - return; + statusIcon.FadeColour(status.GetAppropriateColour(Colours), 500, Easing.OutQuint); } - // Fallback to web status if local one is null - if (User.IsOnline) - { - Status.Value = UserStatus.Online; - return; - } - - Status.Value = UserStatus.Offline; + lastStatus = status; + lastActivity = activity; + lastVisit = status != UserStatus.Offline ? DateTimeOffset.Now : lastVisit; } protected override bool OnHover(HoverEvent e) From 2763cb0b4e9febbfc7f9d185c4acc737214e9a58 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Wed, 15 Jan 2025 17:14:16 +0900 Subject: [PATCH 02/18] Fix inspection --- osu.Game/Overlays/Dashboard/CurrentlyOnlineDisplay.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Overlays/Dashboard/CurrentlyOnlineDisplay.cs b/osu.Game/Overlays/Dashboard/CurrentlyOnlineDisplay.cs index ef07f4538c..e6e1850721 100644 --- a/osu.Game/Overlays/Dashboard/CurrentlyOnlineDisplay.cs +++ b/osu.Game/Overlays/Dashboard/CurrentlyOnlineDisplay.cs @@ -196,8 +196,8 @@ namespace osu.Game.Overlays.Dashboard public IEnumerable FilterTerms { get; } - [Resolved(canBeNull: true)] - private IPerformFromScreenRunner performer { get; set; } + [Resolved] + private IPerformFromScreenRunner? performer { get; set; } public bool FilteringActive { set; get; } From aa3ae8324e19769df585bb45fe8af3935f830113 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Thu, 16 Jan 2025 17:29:43 +0900 Subject: [PATCH 03/18] Add test for local user presence --- .../Visual/Online/TestSceneUserPanel.cs | 31 ++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/osu.Game.Tests/Visual/Online/TestSceneUserPanel.cs b/osu.Game.Tests/Visual/Online/TestSceneUserPanel.cs index b4dafd3107..684e8b7b86 100644 --- a/osu.Game.Tests/Visual/Online/TestSceneUserPanel.cs +++ b/osu.Game.Tests/Visual/Online/TestSceneUserPanel.cs @@ -9,6 +9,7 @@ using osu.Framework.Graphics.Containers; using osu.Framework.Utils; using osu.Game.Beatmaps; using osu.Game.Online; +using osu.Game.Online.API; using osu.Game.Online.API.Requests.Responses; using osu.Game.Online.Metadata; using osu.Game.Overlays; @@ -112,7 +113,11 @@ namespace osu.Game.Tests.Visual.Online CountryCode = CountryCode.AU, CoverUrl = @"https://assets.ppy.sh/user-profile-covers/8195163/4a8e2ad5a02a2642b631438cfa6c6bd7e2f9db289be881cb27df18331f64144c.jpeg", Statistics = new UserStatistics { GlobalRank = null, CountryRank = null } - }) { Width = 300 } + }) { Width = 300 }, + new UserGridPanel(API.LocalUser.Value) + { + Width = 300 + } } } } @@ -180,6 +185,23 @@ namespace osu.Game.Tests.Visual.Online AddStep("set statistics to empty", () => statisticsProvider.UpdateStatistics(new UserStatistics(), Ruleset.Value)); } + [Test] + public void TestLocalUserActivity() + { + AddStep("idle", () => setLocalUserPresence(UserStatus.Online, null)); + AddStep("watching replay", () => setLocalUserPresence(UserStatus.Online, new UserActivity.WatchingReplay(createScore(@"nats")))); + AddStep("spectating user", () => setLocalUserPresence(UserStatus.Online, new UserActivity.SpectatingUser(createScore(@"mrekk")))); + AddStep("solo (osu!)", () => setLocalUserPresence(UserStatus.Online, soloGameStatusForRuleset(0))); + AddStep("solo (osu!taiko)", () => setLocalUserPresence(UserStatus.Online, soloGameStatusForRuleset(1))); + AddStep("solo (osu!catch)", () => setLocalUserPresence(UserStatus.Online, soloGameStatusForRuleset(2))); + AddStep("solo (osu!mania)", () => setLocalUserPresence(UserStatus.Online, soloGameStatusForRuleset(3))); + AddStep("choosing", () => setLocalUserPresence(UserStatus.Online, new UserActivity.ChoosingBeatmap())); + AddStep("editing beatmap", () => setLocalUserPresence(UserStatus.Online, new UserActivity.EditingBeatmap(new BeatmapInfo()))); + AddStep("modding beatmap", () => setLocalUserPresence(UserStatus.Online, new UserActivity.ModdingBeatmap(new BeatmapInfo()))); + AddStep("testing beatmap", () => setLocalUserPresence(UserStatus.Online, new UserActivity.TestingBeatmap(new BeatmapInfo()))); + AddStep("set offline status", () => setLocalUserPresence(UserStatus.Offline, null)); + } + private void setPresence(UserStatus status, UserActivity? activity) { if (status == UserStatus.Offline) @@ -188,6 +210,13 @@ namespace osu.Game.Tests.Visual.Online metadataClient.UserPresenceUpdated(panel.User.OnlineID, new UserPresence { Status = status, Activity = activity }); } + private void setLocalUserPresence(UserStatus status, UserActivity? activity) + { + DummyAPIAccess dummyAPI = (DummyAPIAccess)API; + dummyAPI.Status.Value = status; + dummyAPI.Activity.Value = activity; + } + private UserActivity soloGameStatusForRuleset(int rulesetId) => new UserActivity.InSoloGame(new BeatmapInfo(), rulesetStore.GetRuleset(rulesetId)!); private ScoreInfo createScore(string name) => new ScoreInfo(new TestBeatmap(Ruleset.Value).BeatmapInfo) From 65b88ab365df223e07a4b7c56794b75b42d4b338 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Thu, 16 Jan 2025 19:38:14 +0900 Subject: [PATCH 04/18] Use MetadataClient for local user status --- .../Visual/Online/TestSceneUserPanel.cs | 38 ++++++++----------- osu.Game/Users/ExtendedUserPanel.cs | 8 +--- 2 files changed, 16 insertions(+), 30 deletions(-) diff --git a/osu.Game.Tests/Visual/Online/TestSceneUserPanel.cs b/osu.Game.Tests/Visual/Online/TestSceneUserPanel.cs index 684e8b7b86..f4fc15da20 100644 --- a/osu.Game.Tests/Visual/Online/TestSceneUserPanel.cs +++ b/osu.Game.Tests/Visual/Online/TestSceneUserPanel.cs @@ -9,7 +9,6 @@ using osu.Framework.Graphics.Containers; using osu.Framework.Utils; using osu.Game.Beatmaps; using osu.Game.Online; -using osu.Game.Online.API; using osu.Game.Online.API.Requests.Responses; using osu.Game.Online.Metadata; using osu.Game.Overlays; @@ -188,33 +187,26 @@ namespace osu.Game.Tests.Visual.Online [Test] public void TestLocalUserActivity() { - AddStep("idle", () => setLocalUserPresence(UserStatus.Online, null)); - AddStep("watching replay", () => setLocalUserPresence(UserStatus.Online, new UserActivity.WatchingReplay(createScore(@"nats")))); - AddStep("spectating user", () => setLocalUserPresence(UserStatus.Online, new UserActivity.SpectatingUser(createScore(@"mrekk")))); - AddStep("solo (osu!)", () => setLocalUserPresence(UserStatus.Online, soloGameStatusForRuleset(0))); - AddStep("solo (osu!taiko)", () => setLocalUserPresence(UserStatus.Online, soloGameStatusForRuleset(1))); - AddStep("solo (osu!catch)", () => setLocalUserPresence(UserStatus.Online, soloGameStatusForRuleset(2))); - AddStep("solo (osu!mania)", () => setLocalUserPresence(UserStatus.Online, soloGameStatusForRuleset(3))); - AddStep("choosing", () => setLocalUserPresence(UserStatus.Online, new UserActivity.ChoosingBeatmap())); - AddStep("editing beatmap", () => setLocalUserPresence(UserStatus.Online, new UserActivity.EditingBeatmap(new BeatmapInfo()))); - AddStep("modding beatmap", () => setLocalUserPresence(UserStatus.Online, new UserActivity.ModdingBeatmap(new BeatmapInfo()))); - AddStep("testing beatmap", () => setLocalUserPresence(UserStatus.Online, new UserActivity.TestingBeatmap(new BeatmapInfo()))); - AddStep("set offline status", () => setLocalUserPresence(UserStatus.Offline, null)); + AddStep("idle", () => setPresence(UserStatus.Online, null, API.LocalUser.Value.OnlineID)); + AddStep("watching replay", () => setPresence(UserStatus.Online, new UserActivity.WatchingReplay(createScore(@"nats")), API.LocalUser.Value.OnlineID)); + AddStep("spectating user", () => setPresence(UserStatus.Online, new UserActivity.SpectatingUser(createScore(@"mrekk")), API.LocalUser.Value.OnlineID)); + AddStep("solo (osu!)", () => setPresence(UserStatus.Online, soloGameStatusForRuleset(0), API.LocalUser.Value.OnlineID)); + AddStep("solo (osu!taiko)", () => setPresence(UserStatus.Online, soloGameStatusForRuleset(1), API.LocalUser.Value.OnlineID)); + AddStep("solo (osu!catch)", () => setPresence(UserStatus.Online, soloGameStatusForRuleset(2), API.LocalUser.Value.OnlineID)); + AddStep("solo (osu!mania)", () => setPresence(UserStatus.Online, soloGameStatusForRuleset(3), API.LocalUser.Value.OnlineID)); + AddStep("choosing", () => setPresence(UserStatus.Online, new UserActivity.ChoosingBeatmap(), API.LocalUser.Value.OnlineID)); + AddStep("editing beatmap", () => setPresence(UserStatus.Online, new UserActivity.EditingBeatmap(new BeatmapInfo()), API.LocalUser.Value.OnlineID)); + AddStep("modding beatmap", () => setPresence(UserStatus.Online, new UserActivity.ModdingBeatmap(new BeatmapInfo()), API.LocalUser.Value.OnlineID)); + AddStep("testing beatmap", () => setPresence(UserStatus.Online, new UserActivity.TestingBeatmap(new BeatmapInfo()), API.LocalUser.Value.OnlineID)); + AddStep("set offline status", () => setPresence(UserStatus.Offline, null, API.LocalUser.Value.OnlineID)); } - private void setPresence(UserStatus status, UserActivity? activity) + private void setPresence(UserStatus status, UserActivity? activity, int? userId = null) { if (status == UserStatus.Offline) - metadataClient.UserPresenceUpdated(panel.User.OnlineID, null); + metadataClient.UserPresenceUpdated(userId ?? panel.User.OnlineID, null); else - metadataClient.UserPresenceUpdated(panel.User.OnlineID, new UserPresence { Status = status, Activity = activity }); - } - - private void setLocalUserPresence(UserStatus status, UserActivity? activity) - { - DummyAPIAccess dummyAPI = (DummyAPIAccess)API; - dummyAPI.Status.Value = status; - dummyAPI.Activity.Value = activity; + metadataClient.UserPresenceUpdated(userId ?? panel.User.OnlineID, new UserPresence { Status = status, Activity = activity }); } private UserActivity soloGameStatusForRuleset(int rulesetId) => new UserActivity.InSoloGame(new BeatmapInfo(), rulesetStore.GetRuleset(rulesetId)!); diff --git a/osu.Game/Users/ExtendedUserPanel.cs b/osu.Game/Users/ExtendedUserPanel.cs index eb1115e296..2fc2a97b47 100644 --- a/osu.Game/Users/ExtendedUserPanel.cs +++ b/osu.Game/Users/ExtendedUserPanel.cs @@ -94,13 +94,7 @@ namespace osu.Game.Users private void updatePresence() { - UserPresence? presence; - - if (User.Equals(api?.LocalUser.Value)) - presence = new UserPresence { Status = api.Status.Value, Activity = api.Activity.Value }; - else - presence = metadata?.GetPresence(User.OnlineID); - + UserPresence? presence = metadata?.GetPresence(User.OnlineID); UserStatus status = presence?.Status ?? UserStatus.Offline; UserActivity? activity = presence?.Activity; From 94db39317b626a90c6dd60c2c9dee530bdc58fe2 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Thu, 16 Jan 2025 20:43:22 +0900 Subject: [PATCH 05/18] Add xmldoc --- osu.Game/Configuration/SessionStatics.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/osu.Game/Configuration/SessionStatics.cs b/osu.Game/Configuration/SessionStatics.cs index bdfb0217ad..d2069e4027 100644 --- a/osu.Game/Configuration/SessionStatics.cs +++ b/osu.Game/Configuration/SessionStatics.cs @@ -95,6 +95,9 @@ namespace osu.Game.Configuration /// DailyChallengeIntroPlayed, + /// + /// The activity for the current user to broadcast to other players. + /// UserOnlineActivity, } } From 626be9d7806b44434bb773cb9afe002c0639356b Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Fri, 17 Jan 2025 16:01:11 +0900 Subject: [PATCH 06/18] Return local user state where appropriate --- osu.Game/Online/Metadata/MetadataClient.cs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/osu.Game/Online/Metadata/MetadataClient.cs b/osu.Game/Online/Metadata/MetadataClient.cs index a72377721a..1b6f96d91b 100644 --- a/osu.Game/Online/Metadata/MetadataClient.cs +++ b/osu.Game/Online/Metadata/MetadataClient.cs @@ -4,8 +4,10 @@ using System; using System.Linq; using System.Threading.Tasks; +using osu.Framework.Allocation; using osu.Framework.Bindables; using osu.Framework.Graphics; +using osu.Game.Online.API; using osu.Game.Users; namespace osu.Game.Online.Metadata @@ -14,6 +16,9 @@ namespace osu.Game.Online.Metadata { public abstract IBindable IsConnected { get; } + [Resolved] + private IAPIProvider api { get; set; } = null!; + #region Beatmap metadata updates public abstract Task GetChangesSince(int queueId); @@ -59,6 +64,9 @@ namespace osu.Game.Online.Metadata /// The user presence, or null if not available or the user's offline. public UserPresence? GetPresence(int userId) { + if (userId == api.LocalUser.Value.OnlineID) + return LocalUserState; + if (FriendStates.TryGetValue(userId, out UserPresence presence)) return presence; From 61419ec9c840fe55886c338f0eb53a8dd919be89 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Tue, 4 Feb 2025 17:54:03 +0900 Subject: [PATCH 07/18] Refactor user presence watching to be tokenised --- .../Online/TestSceneMetadataClient.cs | 52 +++++++++++++++ .../Online/TestSceneCurrentlyOnlineDisplay.cs | 12 ++-- osu.Game/Online/Metadata/MetadataClient.cs | 59 ++++++++++++++--- .../Online/Metadata/OnlineMetadataClient.cs | 63 +++++++++---------- osu.Game/Overlays/DashboardOverlay.cs | 9 ++- .../Visual/Metadata/TestMetadataClient.cs | 20 +++--- 6 files changed, 156 insertions(+), 59 deletions(-) create mode 100644 osu.Game.Tests/Online/TestSceneMetadataClient.cs diff --git a/osu.Game.Tests/Online/TestSceneMetadataClient.cs b/osu.Game.Tests/Online/TestSceneMetadataClient.cs new file mode 100644 index 0000000000..8c738eeca6 --- /dev/null +++ b/osu.Game.Tests/Online/TestSceneMetadataClient.cs @@ -0,0 +1,52 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using System; +using NUnit.Framework; +using osu.Framework.Testing; +using osu.Game.Tests.Visual; +using osu.Game.Tests.Visual.Metadata; + +namespace osu.Game.Tests.Online +{ + [TestFixture] + [HeadlessTest] + public class TestSceneMetadataClient : OsuTestScene + { + private TestMetadataClient client = null!; + + [SetUp] + public void Setup() => Schedule(() => + { + Child = client = new TestMetadataClient(); + }); + + [Test] + public void TestWatchingMultipleTimesInvokesServerMethodsOnce() + { + int countBegin = 0; + int countEnd = 0; + + IDisposable token1 = null!; + IDisposable token2 = null!; + + AddStep("setup", () => + { + client.OnBeginWatchingUserPresence += () => countBegin++; + client.OnEndWatchingUserPresence += () => countEnd++; + }); + + AddStep("begin watching presence (1)", () => token1 = client.BeginWatchingUserPresence()); + AddAssert("server method invoked once", () => countBegin, () => Is.EqualTo(1)); + + AddStep("begin watching presence (2)", () => token2 = client.BeginWatchingUserPresence()); + AddAssert("server method not invoked a second time", () => countBegin, () => Is.EqualTo(1)); + + AddStep("end watching presence (1)", () => token1.Dispose()); + AddAssert("server method not invoked", () => countEnd, () => Is.EqualTo(0)); + + AddStep("end watching presence (2)", () => token2.Dispose()); + AddAssert("server method invoked once", () => countEnd, () => Is.EqualTo(1)); + } + } +} diff --git a/osu.Game.Tests/Visual/Online/TestSceneCurrentlyOnlineDisplay.cs b/osu.Game.Tests/Visual/Online/TestSceneCurrentlyOnlineDisplay.cs index b696c5d8ca..2e53ec2ba4 100644 --- a/osu.Game.Tests/Visual/Online/TestSceneCurrentlyOnlineDisplay.cs +++ b/osu.Game.Tests/Visual/Online/TestSceneCurrentlyOnlineDisplay.cs @@ -65,7 +65,9 @@ namespace osu.Game.Tests.Visual.Online [Test] public void TestBasicDisplay() { - AddStep("Begin watching user presence", () => metadataClient.BeginWatchingUserPresence()); + IDisposable token = null!; + + AddStep("Begin watching user presence", () => token = metadataClient.BeginWatchingUserPresence()); AddStep("Add online user", () => metadataClient.UserPresenceUpdated(streamingUser.Id, new UserPresence { Status = UserStatus.Online, Activity = new UserActivity.ChoosingBeatmap() })); AddUntilStep("Panel loaded", () => currentlyOnline.ChildrenOfType().FirstOrDefault()?.User.Id == 2); AddAssert("Spectate button disabled", () => currentlyOnline.ChildrenOfType().First().Enabled.Value, () => Is.False); @@ -78,14 +80,16 @@ namespace osu.Game.Tests.Visual.Online AddStep("Remove playing user", () => metadataClient.UserPresenceUpdated(streamingUser.Id, null)); AddUntilStep("Panel no longer present", () => !currentlyOnline.ChildrenOfType().Any()); - AddStep("End watching user presence", () => metadataClient.EndWatchingUserPresence()); + AddStep("End watching user presence", () => token.Dispose()); } [Test] public void TestUserWasPlayingBeforeWatchingUserPresence() { + IDisposable token = null!; + AddStep("User began playing", () => spectatorClient.SendStartPlay(streamingUser.Id, 0)); - AddStep("Begin watching user presence", () => metadataClient.BeginWatchingUserPresence()); + AddStep("Begin watching user presence", () => token = metadataClient.BeginWatchingUserPresence()); AddStep("Add online user", () => metadataClient.UserPresenceUpdated(streamingUser.Id, new UserPresence { Status = UserStatus.Online, Activity = new UserActivity.ChoosingBeatmap() })); AddUntilStep("Panel loaded", () => currentlyOnline.ChildrenOfType().FirstOrDefault()?.User.Id == 2); AddAssert("Spectate button enabled", () => currentlyOnline.ChildrenOfType().First().Enabled.Value, () => Is.True); @@ -93,7 +97,7 @@ namespace osu.Game.Tests.Visual.Online AddStep("User finished playing", () => spectatorClient.SendEndPlay(streamingUser.Id)); AddAssert("Spectate button disabled", () => currentlyOnline.ChildrenOfType().First().Enabled.Value, () => Is.False); AddStep("Remove playing user", () => metadataClient.UserPresenceUpdated(streamingUser.Id, null)); - AddStep("End watching user presence", () => metadataClient.EndWatchingUserPresence()); + AddStep("End watching user presence", () => token.Dispose()); } internal partial class TestUserLookupCache : UserLookupCache diff --git a/osu.Game/Online/Metadata/MetadataClient.cs b/osu.Game/Online/Metadata/MetadataClient.cs index 356c50bcc0..1da245e80d 100644 --- a/osu.Game/Online/Metadata/MetadataClient.cs +++ b/osu.Game/Online/Metadata/MetadataClient.cs @@ -3,11 +3,13 @@ using System; using System.Linq; +using System.Threading; using System.Threading.Tasks; using osu.Framework.Allocation; using osu.Framework.Bindables; using osu.Framework.Graphics; using osu.Game.Online.API; +using osu.Game.Online.Multiplayer; using osu.Game.Users; namespace osu.Game.Online.Metadata @@ -37,11 +39,6 @@ namespace osu.Game.Online.Metadata #region User presence updates - /// - /// Whether the client is currently receiving user presence updates from the server. - /// - public abstract IBindable IsWatchingUserPresence { get; } - /// /// The information about the current user. /// @@ -82,11 +79,36 @@ namespace osu.Game.Online.Metadata /// public abstract Task UpdateStatus(UserStatus? status); - /// - public abstract Task BeginWatchingUserPresence(); + private int userPresenceWatchCount; + + protected bool IsWatchingUserPresence + => Interlocked.CompareExchange(ref userPresenceWatchCount, userPresenceWatchCount, userPresenceWatchCount) > 0; + + /// + public IDisposable BeginWatchingUserPresence() + => new UserPresenceWatchToken(this); /// - public abstract Task EndWatchingUserPresence(); + Task IMetadataServer.BeginWatchingUserPresence() + { + if (Interlocked.Increment(ref userPresenceWatchCount) == 1) + return BeginWatchingUserPresenceInternal(); + + return Task.CompletedTask; + } + + /// + Task IMetadataServer.EndWatchingUserPresence() + { + if (Interlocked.Decrement(ref userPresenceWatchCount) == 0) + return EndWatchingUserPresenceInternal(); + + return Task.CompletedTask; + } + + protected abstract Task BeginWatchingUserPresenceInternal(); + + protected abstract Task EndWatchingUserPresenceInternal(); /// public abstract Task UserPresenceUpdated(int userId, UserPresence? presence); @@ -94,6 +116,27 @@ namespace osu.Game.Online.Metadata /// public abstract Task FriendPresenceUpdated(int userId, UserPresence? presence); + private class UserPresenceWatchToken : IDisposable + { + private readonly IMetadataServer server; + private bool isDisposed; + + public UserPresenceWatchToken(IMetadataServer server) + { + this.server = server; + server.BeginWatchingUserPresence().FireAndForget(); + } + + public void Dispose() + { + if (isDisposed) + return; + + server.EndWatchingUserPresence().FireAndForget(); + isDisposed = true; + } + } + #endregion #region Daily Challenge diff --git a/osu.Game/Online/Metadata/OnlineMetadataClient.cs b/osu.Game/Online/Metadata/OnlineMetadataClient.cs index 5aeeb04d11..c7c7dfc58b 100644 --- a/osu.Game/Online/Metadata/OnlineMetadataClient.cs +++ b/osu.Game/Online/Metadata/OnlineMetadataClient.cs @@ -20,9 +20,6 @@ namespace osu.Game.Online.Metadata { public override IBindable IsConnected { get; } = new Bindable(); - public override IBindable IsWatchingUserPresence => isWatchingUserPresence; - private readonly BindableBool isWatchingUserPresence = new BindableBool(); - public override UserPresence LocalUserPresence => localUserPresence; private UserPresence localUserPresence; @@ -109,15 +106,18 @@ namespace osu.Game.Online.Metadata { Schedule(() => { - isWatchingUserPresence.Value = false; userPresences.Clear(); friendPresences.Clear(); dailyChallengeInfo.Value = null; localUserPresence = default; }); + return; } + if (IsWatchingUserPresence) + BeginWatchingUserPresenceInternal(); + if (localUser.Value is not GuestUser) { UpdateActivity(userActivity.Value); @@ -201,6 +201,31 @@ namespace osu.Game.Online.Metadata return connection.InvokeAsync(nameof(IMetadataServer.UpdateStatus), status); } + protected override Task BeginWatchingUserPresenceInternal() + { + if (connector?.IsConnected.Value != true) + return Task.CompletedTask; + + Logger.Log($@"{nameof(OnlineMetadataClient)} began watching user presence", LoggingTarget.Network); + + Debug.Assert(connection != null); + return connection.InvokeAsync(nameof(IMetadataServer.BeginWatchingUserPresence)); + } + + protected override Task EndWatchingUserPresenceInternal() + { + if (connector?.IsConnected.Value != true) + return Task.CompletedTask; + + Logger.Log($@"{nameof(OnlineMetadataClient)} stopped watching user presence", LoggingTarget.Network); + + // must be scheduled before any remote calls to avoid mis-ordering. + Schedule(() => userPresences.Clear()); + + Debug.Assert(connection != null); + return connection.InvokeAsync(nameof(IMetadataServer.EndWatchingUserPresence)); + } + public override Task UserPresenceUpdated(int userId, UserPresence? presence) { Schedule(() => @@ -237,36 +262,6 @@ namespace osu.Game.Online.Metadata return Task.CompletedTask; } - public override async Task BeginWatchingUserPresence() - { - if (connector?.IsConnected.Value != true) - throw new OperationCanceledException(); - - Debug.Assert(connection != null); - await connection.InvokeAsync(nameof(IMetadataServer.BeginWatchingUserPresence)).ConfigureAwait(false); - Schedule(() => isWatchingUserPresence.Value = true); - Logger.Log($@"{nameof(OnlineMetadataClient)} began watching user presence", LoggingTarget.Network); - } - - public override async Task EndWatchingUserPresence() - { - try - { - if (connector?.IsConnected.Value != true) - throw new OperationCanceledException(); - - // must be scheduled before any remote calls to avoid mis-ordering. - Schedule(() => userPresences.Clear()); - Debug.Assert(connection != null); - await connection.InvokeAsync(nameof(IMetadataServer.EndWatchingUserPresence)).ConfigureAwait(false); - Logger.Log($@"{nameof(OnlineMetadataClient)} stopped watching user presence", LoggingTarget.Network); - } - finally - { - Schedule(() => isWatchingUserPresence.Value = false); - } - } - public override Task DailyChallengeUpdated(DailyChallengeInfo? info) { Schedule(() => dailyChallengeInfo.Value = info); diff --git a/osu.Game/Overlays/DashboardOverlay.cs b/osu.Game/Overlays/DashboardOverlay.cs index 1861f892bd..1912736135 100644 --- a/osu.Game/Overlays/DashboardOverlay.cs +++ b/osu.Game/Overlays/DashboardOverlay.cs @@ -6,7 +6,6 @@ using osu.Framework.Allocation; using osu.Framework.Bindables; using osu.Framework.Graphics.Containers; using osu.Game.Online.Metadata; -using osu.Game.Online.Multiplayer; using osu.Game.Overlays.Dashboard; using osu.Game.Overlays.Dashboard.Friends; @@ -18,6 +17,7 @@ namespace osu.Game.Overlays private MetadataClient metadataClient { get; set; } = null!; private IBindable metadataConnected = null!; + private IDisposable? userPresenceWatchToken; public DashboardOverlay() : base(OverlayColourScheme.Purple) @@ -61,9 +61,12 @@ namespace osu.Game.Overlays return; if (State.Value == Visibility.Visible) - metadataClient.BeginWatchingUserPresence().FireAndForget(); + userPresenceWatchToken ??= metadataClient.BeginWatchingUserPresence(); else - metadataClient.EndWatchingUserPresence().FireAndForget(); + { + userPresenceWatchToken?.Dispose(); + userPresenceWatchToken = null; + } } } } diff --git a/osu.Game/Tests/Visual/Metadata/TestMetadataClient.cs b/osu.Game/Tests/Visual/Metadata/TestMetadataClient.cs index d14cbd7743..dca1b0e468 100644 --- a/osu.Game/Tests/Visual/Metadata/TestMetadataClient.cs +++ b/osu.Game/Tests/Visual/Metadata/TestMetadataClient.cs @@ -16,9 +16,6 @@ namespace osu.Game.Tests.Visual.Metadata public override IBindable IsConnected => isConnected; private readonly BindableBool isConnected = new BindableBool(true); - public override IBindable IsWatchingUserPresence => isWatchingUserPresence; - private readonly BindableBool isWatchingUserPresence = new BindableBool(); - public override UserPresence LocalUserPresence => localUserPresence; private UserPresence localUserPresence; @@ -34,15 +31,18 @@ namespace osu.Game.Tests.Visual.Metadata [Resolved] private IAPIProvider api { get; set; } = null!; - public override Task BeginWatchingUserPresence() + public event Action? OnBeginWatchingUserPresence; + public event Action? OnEndWatchingUserPresence; + + protected override Task BeginWatchingUserPresenceInternal() { - isWatchingUserPresence.Value = true; + OnBeginWatchingUserPresence?.Invoke(); return Task.CompletedTask; } - public override Task EndWatchingUserPresence() + protected override Task EndWatchingUserPresenceInternal() { - isWatchingUserPresence.Value = false; + OnEndWatchingUserPresence?.Invoke(); return Task.CompletedTask; } @@ -50,7 +50,7 @@ namespace osu.Game.Tests.Visual.Metadata { localUserPresence = localUserPresence with { Activity = activity }; - if (isWatchingUserPresence.Value) + if (IsWatchingUserPresence) { if (userPresences.ContainsKey(api.LocalUser.Value.Id)) userPresences[api.LocalUser.Value.Id] = localUserPresence; @@ -63,7 +63,7 @@ namespace osu.Game.Tests.Visual.Metadata { localUserPresence = localUserPresence with { Status = status }; - if (isWatchingUserPresence.Value) + if (IsWatchingUserPresence) { if (userPresences.ContainsKey(api.LocalUser.Value.Id)) userPresences[api.LocalUser.Value.Id] = localUserPresence; @@ -74,7 +74,7 @@ namespace osu.Game.Tests.Visual.Metadata public override Task UserPresenceUpdated(int userId, UserPresence? presence) { - if (isWatchingUserPresence.Value) + if (IsWatchingUserPresence) { if (presence?.Status != null) { From 2f90bb4d6793475835d1d51bef92b2c40f69112c Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Tue, 4 Feb 2025 17:55:50 +0900 Subject: [PATCH 08/18] Watch global user presence while in spectator screen --- osu.Game/Screens/Spectate/SpectatorScreen.cs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/osu.Game/Screens/Spectate/SpectatorScreen.cs b/osu.Game/Screens/Spectate/SpectatorScreen.cs index ddc638b7c5..84b5889751 100644 --- a/osu.Game/Screens/Spectate/SpectatorScreen.cs +++ b/osu.Game/Screens/Spectate/SpectatorScreen.cs @@ -12,6 +12,7 @@ using osu.Framework.Extensions.ObjectExtensions; using osu.Game.Beatmaps; using osu.Game.Database; using osu.Game.Online.API.Requests.Responses; +using osu.Game.Online.Metadata; using osu.Game.Online.Spectator; using osu.Game.Replays; using osu.Game.Rulesets; @@ -38,6 +39,9 @@ namespace osu.Game.Screens.Spectate [Resolved] private SpectatorClient spectatorClient { get; set; } = null!; + [Resolved] + private MetadataClient metadataClient { get; set; } = null!; + [Resolved] private UserLookupCache userLookupCache { get; set; } = null!; @@ -50,6 +54,7 @@ namespace osu.Game.Screens.Spectate private readonly Dictionary gameplayStates = new Dictionary(); private IDisposable? realmSubscription; + private IDisposable? userWatchToken; /// /// Creates a new . @@ -64,6 +69,8 @@ namespace osu.Game.Screens.Spectate { base.LoadComplete(); + userWatchToken = metadataClient.BeginWatchingUserPresence(); + userLookupCache.GetUsersAsync(users.ToArray()).ContinueWith(task => Schedule(() => { var foundUsers = task.GetResultSafely(); @@ -282,6 +289,7 @@ namespace osu.Game.Screens.Spectate } realmSubscription?.Dispose(); + userWatchToken?.Dispose(); } } } From 0ad97c1fad6c85b2e95864ba007ba670de00b3ba Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Tue, 4 Feb 2025 18:24:57 +0900 Subject: [PATCH 09/18] Fix inspection --- osu.Game.Tests/Online/TestSceneMetadataClient.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Tests/Online/TestSceneMetadataClient.cs b/osu.Game.Tests/Online/TestSceneMetadataClient.cs index 8c738eeca6..04e1d91edf 100644 --- a/osu.Game.Tests/Online/TestSceneMetadataClient.cs +++ b/osu.Game.Tests/Online/TestSceneMetadataClient.cs @@ -11,7 +11,7 @@ namespace osu.Game.Tests.Online { [TestFixture] [HeadlessTest] - public class TestSceneMetadataClient : OsuTestScene + public partial class TestSceneMetadataClient : OsuTestScene { private TestMetadataClient client = null!; From 14273824dcce96bf5c0e59a344a10305fc2bf253 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 4 Feb 2025 19:37:00 +0900 Subject: [PATCH 10/18] Fix `Carousel.FilterAsync` not working when called from a non-update thread I was trying to be smart about things and make use of our `SynchronisationContext` setup, but it turns out to not work in all cases due to the context being missing depending on where you are calling the method from. For now let's prefer the "works everywhere" method of scheduling the final work back to update. --- .../SongSelect/BeatmapCarouselV2TestScene.cs | 2 +- osu.Game/Screens/SelectV2/Carousel.cs | 19 ++++++++++--------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs b/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs index 72c9611fdb..b29394c55d 100644 --- a/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs +++ b/osu.Game.Tests/Visual/SongSelect/BeatmapCarouselV2TestScene.cs @@ -130,7 +130,7 @@ namespace osu.Game.Tests.Visual.SongSelect }); } - protected void SortBy(FilterCriteria criteria) => AddStep($"sort {criteria.Sort} group {criteria.Group}", () => Carousel.Filter(criteria)); + protected void SortBy(FilterCriteria criteria) => AddStep($"sort:{criteria.Sort} group:{criteria.Group}", () => Carousel.Filter(criteria)); protected void WaitForDrawablePanels() => AddUntilStep("drawable panels loaded", () => Carousel.ChildrenOfType().Count(), () => Is.GreaterThan(0)); protected void WaitForSorting() => AddUntilStep("sorting finished", () => Carousel.IsFiltering, () => Is.False); diff --git a/osu.Game/Screens/SelectV2/Carousel.cs b/osu.Game/Screens/SelectV2/Carousel.cs index 608ef207d9..78c2c99d99 100644 --- a/osu.Game/Screens/SelectV2/Carousel.cs +++ b/osu.Game/Screens/SelectV2/Carousel.cs @@ -228,8 +228,6 @@ namespace osu.Game.Screens.SelectV2 private async Task performFilter() { - Debug.Assert(SynchronizationContext.Current != null); - Stopwatch stopwatch = Stopwatch.StartNew(); var cts = new CancellationTokenSource(); @@ -266,19 +264,22 @@ namespace osu.Game.Screens.SelectV2 { log("Cancelled due to newer request arriving"); } - }, cts.Token).ConfigureAwait(true); + }, cts.Token).ConfigureAwait(false); if (cts.Token.IsCancellationRequested) return; - log("Items ready for display"); - carouselItems = items.ToList(); - displayedRange = null; + Schedule(() => + { + log("Items ready for display"); + carouselItems = items.ToList(); + displayedRange = null; - // Need to call this to ensure correct post-selection logic is handled on the new items list. - HandleItemSelected(currentSelection.Model); + // Need to call this to ensure correct post-selection logic is handled on the new items list. + HandleItemSelected(currentSelection.Model); - refreshAfterSelection(); + refreshAfterSelection(); + }); void log(string text) => Logger.Log($"Carousel[op {cts.GetHashCode().ToString()}] {stopwatch.ElapsedMilliseconds} ms: {text}"); } From 5c9e84caf0350760c1f7d78cbe80024aed7661de Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 5 Feb 2025 17:31:54 +0900 Subject: [PATCH 11/18] Add lock object --- osu.Game/Screens/SelectV2/Carousel.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/osu.Game/Screens/SelectV2/Carousel.cs b/osu.Game/Screens/SelectV2/Carousel.cs index 78c2c99d99..681da84390 100644 --- a/osu.Game/Screens/SelectV2/Carousel.cs +++ b/osu.Game/Screens/SelectV2/Carousel.cs @@ -226,12 +226,14 @@ namespace osu.Game.Screens.SelectV2 private Task filterTask = Task.CompletedTask; private CancellationTokenSource cancellationSource = new CancellationTokenSource(); + private readonly object cancellationLock = new object(); + private async Task performFilter() { Stopwatch stopwatch = Stopwatch.StartNew(); var cts = new CancellationTokenSource(); - lock (this) + lock (cancellationLock) { cancellationSource.Cancel(); cancellationSource = cts; From b7aa71c9759dc7d69249948591ebb60de34e2750 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 5 Feb 2025 18:24:07 +0900 Subject: [PATCH 12/18] Adjust xmldoc slightly to convey the disposal pattern --- osu.Game/Online/Metadata/MetadataClient.cs | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/osu.Game/Online/Metadata/MetadataClient.cs b/osu.Game/Online/Metadata/MetadataClient.cs index 1da245e80d..9885419b65 100644 --- a/osu.Game/Online/Metadata/MetadataClient.cs +++ b/osu.Game/Online/Metadata/MetadataClient.cs @@ -73,10 +73,8 @@ namespace osu.Game.Online.Metadata return null; } - /// public abstract Task UpdateActivity(UserActivity? activity); - /// public abstract Task UpdateStatus(UserStatus? status); private int userPresenceWatchCount; @@ -84,11 +82,12 @@ namespace osu.Game.Online.Metadata protected bool IsWatchingUserPresence => Interlocked.CompareExchange(ref userPresenceWatchCount, userPresenceWatchCount, userPresenceWatchCount) > 0; - /// - public IDisposable BeginWatchingUserPresence() - => new UserPresenceWatchToken(this); + /// + /// Signals to the server that we want to begin receiving status updates for all users. + /// + /// An which will end the session when disposed. + public IDisposable BeginWatchingUserPresence() => new UserPresenceWatchToken(this); - /// Task IMetadataServer.BeginWatchingUserPresence() { if (Interlocked.Increment(ref userPresenceWatchCount) == 1) @@ -97,7 +96,6 @@ namespace osu.Game.Online.Metadata return Task.CompletedTask; } - /// Task IMetadataServer.EndWatchingUserPresence() { if (Interlocked.Decrement(ref userPresenceWatchCount) == 0) @@ -110,10 +108,8 @@ namespace osu.Game.Online.Metadata protected abstract Task EndWatchingUserPresenceInternal(); - /// public abstract Task UserPresenceUpdated(int userId, UserPresence? presence); - /// public abstract Task FriendPresenceUpdated(int userId, UserPresence? presence); private class UserPresenceWatchToken : IDisposable @@ -143,7 +139,6 @@ namespace osu.Game.Online.Metadata public abstract IBindable DailyChallengeInfo { get; } - /// public abstract Task DailyChallengeUpdated(DailyChallengeInfo? info); #endregion From c5deb9f36b067f03bd9d597967ac17f8502ade27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 5 Feb 2025 10:28:09 +0100 Subject: [PATCH 13/18] Use alternative lockless solution for atomic cancellation token recreation --- osu.Game/Screens/SelectV2/Carousel.cs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/osu.Game/Screens/SelectV2/Carousel.cs b/osu.Game/Screens/SelectV2/Carousel.cs index 681da84390..0b706b4bb8 100644 --- a/osu.Game/Screens/SelectV2/Carousel.cs +++ b/osu.Game/Screens/SelectV2/Carousel.cs @@ -226,18 +226,13 @@ namespace osu.Game.Screens.SelectV2 private Task filterTask = Task.CompletedTask; private CancellationTokenSource cancellationSource = new CancellationTokenSource(); - private readonly object cancellationLock = new object(); - private async Task performFilter() { Stopwatch stopwatch = Stopwatch.StartNew(); var cts = new CancellationTokenSource(); - lock (cancellationLock) - { - cancellationSource.Cancel(); - cancellationSource = cts; - } + var previousCancellationSource = Interlocked.Exchange(ref cancellationSource, cts); + await previousCancellationSource.CancelAsync().ConfigureAwait(false); if (DebounceDelay > 0) { From e5943e460d657cb12545078d10b89ca58f6456f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 5 Feb 2025 10:28:23 +0100 Subject: [PATCH 14/18] Unify `ConfigureAwait()` calls across method --- osu.Game/Screens/SelectV2/Carousel.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Screens/SelectV2/Carousel.cs b/osu.Game/Screens/SelectV2/Carousel.cs index 0b706b4bb8..3371e45453 100644 --- a/osu.Game/Screens/SelectV2/Carousel.cs +++ b/osu.Game/Screens/SelectV2/Carousel.cs @@ -237,7 +237,7 @@ namespace osu.Game.Screens.SelectV2 if (DebounceDelay > 0) { log($"Filter operation queued, waiting for {DebounceDelay} ms debounce"); - await Task.Delay(DebounceDelay, cts.Token).ConfigureAwait(true); + await Task.Delay(DebounceDelay, cts.Token).ConfigureAwait(false); } // Copy must be performed on update thread for now (see ConfigureAwait above). From abce42b1c8fa48dc9fc64ff5e756b90f66d947a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 5 Feb 2025 15:28:27 +0100 Subject: [PATCH 15/18] Improve bookmark controls - Bookmark menu items get disabled when they would do nothing. - Bookmark deletion only deletes the closest bookmark instead of all of them within the proximity of 2 seconds to current clock time. Action is only however *enabled* within 2 seconds of a bookmark. Additionally, logic was moved out of `Editor` because it's a huge class and I dislike huge classes if they can be at all avoided. --- osu.Game/Screens/Edit/BookmarkController.cs | 148 ++++++++++++++++++++ osu.Game/Screens/Edit/Editor.cs | 66 +-------- 2 files changed, 152 insertions(+), 62 deletions(-) create mode 100644 osu.Game/Screens/Edit/BookmarkController.cs diff --git a/osu.Game/Screens/Edit/BookmarkController.cs b/osu.Game/Screens/Edit/BookmarkController.cs new file mode 100644 index 0000000000..8c048ba871 --- /dev/null +++ b/osu.Game/Screens/Edit/BookmarkController.cs @@ -0,0 +1,148 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using System; +using System.Linq; +using osu.Framework.Allocation; +using osu.Framework.Bindables; +using osu.Framework.Graphics; +using osu.Framework.Graphics.UserInterface; +using osu.Framework.Input.Bindings; +using osu.Framework.Input.Events; +using osu.Game.Graphics.UserInterface; +using osu.Game.Input.Bindings; +using osu.Game.Localisation; +using osu.Game.Overlays; +using osu.Game.Screens.Edit.Components.Menus; + +namespace osu.Game.Screens.Edit +{ + public class BookmarkController : Component, IKeyBindingHandler + { + public EditorMenuItem Menu { get; private set; } + + [Resolved] + private EditorClock clock { get; set; } = null!; + + [Resolved] + private EditorBeatmap editorBeatmap { get; set; } = null!; + + [Resolved] + private IDialogOverlay? dialogOverlay { get; set; } + + private readonly BindableList bookmarks = new BindableList(); + + private readonly EditorMenuItem removeBookmarkMenuItem; + private readonly EditorMenuItem seekToPreviousBookmarkMenuItem; + private readonly EditorMenuItem seekToNextBookmarkMenuItem; + private readonly EditorMenuItem resetBookmarkMenuItem; + + public BookmarkController() + { + Menu = new EditorMenuItem(EditorStrings.Bookmarks) + { + Items = new MenuItem[] + { + new EditorMenuItem(EditorStrings.AddBookmark, MenuItemType.Standard, addBookmarkAtCurrentTime) + { + Hotkey = new Hotkey(GlobalAction.EditorAddBookmark), + }, + removeBookmarkMenuItem = new EditorMenuItem(EditorStrings.RemoveClosestBookmark, MenuItemType.Destructive, removeClosestBookmark) + { + Hotkey = new Hotkey(GlobalAction.EditorRemoveClosestBookmark) + }, + seekToPreviousBookmarkMenuItem = new EditorMenuItem(EditorStrings.SeekToPreviousBookmark, MenuItemType.Standard, () => seekBookmark(-1)) + { + Hotkey = new Hotkey(GlobalAction.EditorSeekToPreviousBookmark) + }, + seekToNextBookmarkMenuItem = new EditorMenuItem(EditorStrings.SeekToNextBookmark, MenuItemType.Standard, () => seekBookmark(1)) + { + Hotkey = new Hotkey(GlobalAction.EditorSeekToNextBookmark) + }, + resetBookmarkMenuItem = new EditorMenuItem(EditorStrings.ResetBookmarks, MenuItemType.Destructive, () => dialogOverlay?.Push(new BookmarkResetDialog(editorBeatmap))) + } + }; + } + + protected override void LoadComplete() + { + base.LoadComplete(); + + bookmarks.BindTo(editorBeatmap.Bookmarks); + } + + protected override void Update() + { + base.Update(); + + bool hasAnyBookmark = bookmarks.Count > 0; + bool hasBookmarkCloseEnoughForDeletion = bookmarks.Any(b => Math.Abs(b - clock.CurrentTimeAccurate) < 2000); + + removeBookmarkMenuItem.Action.Disabled = !hasBookmarkCloseEnoughForDeletion; + seekToPreviousBookmarkMenuItem.Action.Disabled = !hasAnyBookmark; + seekToNextBookmarkMenuItem.Action.Disabled = !hasAnyBookmark; + resetBookmarkMenuItem.Action.Disabled = !hasAnyBookmark; + } + + private void addBookmarkAtCurrentTime() + { + int bookmark = (int)clock.CurrentTimeAccurate; + int idx = bookmarks.BinarySearch(bookmark); + if (idx < 0) + bookmarks.Insert(~idx, bookmark); + } + + private void removeClosestBookmark() + { + if (removeBookmarkMenuItem.Action.Disabled) + return; + + int closestBookmark = bookmarks.MinBy(b => Math.Abs(b - clock.CurrentTimeAccurate)); + bookmarks.Remove(closestBookmark); + } + + private void seekBookmark(int direction) + { + int? targetBookmark = direction < 1 + ? bookmarks.Cast().LastOrDefault(b => b < clock.CurrentTimeAccurate) + : bookmarks.Cast().FirstOrDefault(b => b > clock.CurrentTimeAccurate); + + if (targetBookmark != null) + clock.SeekSmoothlyTo(targetBookmark.Value); + } + + public bool OnPressed(KeyBindingPressEvent e) + { + switch (e.Action) + { + case GlobalAction.EditorSeekToPreviousBookmark: + seekBookmark(-1); + return true; + + case GlobalAction.EditorSeekToNextBookmark: + seekBookmark(1); + return true; + } + + if (e.Repeat) + return false; + + switch (e.Action) + { + case GlobalAction.EditorAddBookmark: + addBookmarkAtCurrentTime(); + return true; + + case GlobalAction.EditorRemoveClosestBookmark: + removeClosestBookmark(); + return true; + } + + return false; + } + + public void OnReleased(KeyBindingReleaseEvent e) + { + } + } +} diff --git a/osu.Game/Screens/Edit/Editor.cs b/osu.Game/Screens/Edit/Editor.cs index 3302fafbb8..a5dfda9c95 100644 --- a/osu.Game/Screens/Edit/Editor.cs +++ b/osu.Game/Screens/Edit/Editor.cs @@ -317,6 +317,9 @@ namespace osu.Game.Screens.Edit workingBeatmapUpdated = true; }); + var bookmarkController = new BookmarkController(); + AddInternal(bookmarkController); + OsuMenuItem undoMenuItem; OsuMenuItem redoMenuItem; @@ -442,29 +445,7 @@ namespace osu.Game.Screens.Edit Items = new MenuItem[] { new EditorMenuItem(EditorStrings.SetPreviewPointToCurrent, MenuItemType.Standard, SetPreviewPointToCurrentTime), - new EditorMenuItem(EditorStrings.Bookmarks) - { - Items = new MenuItem[] - { - new EditorMenuItem(EditorStrings.AddBookmark, MenuItemType.Standard, addBookmarkAtCurrentTime) - { - Hotkey = new Hotkey(GlobalAction.EditorAddBookmark), - }, - new EditorMenuItem(EditorStrings.RemoveClosestBookmark, MenuItemType.Destructive, removeBookmarksInProximityToCurrentTime) - { - Hotkey = new Hotkey(GlobalAction.EditorRemoveClosestBookmark) - }, - new EditorMenuItem(EditorStrings.SeekToPreviousBookmark, MenuItemType.Standard, () => seekBookmark(-1)) - { - Hotkey = new Hotkey(GlobalAction.EditorSeekToPreviousBookmark) - }, - new EditorMenuItem(EditorStrings.SeekToNextBookmark, MenuItemType.Standard, () => seekBookmark(1)) - { - Hotkey = new Hotkey(GlobalAction.EditorSeekToNextBookmark) - }, - new EditorMenuItem(EditorStrings.ResetBookmarks, MenuItemType.Destructive, () => dialogOverlay?.Push(new BookmarkResetDialog(editorBeatmap))) - } - } + bookmarkController.Menu, } } } @@ -800,14 +781,6 @@ namespace osu.Game.Screens.Edit case GlobalAction.EditorSeekToNextSamplePoint: seekSamplePoint(1); return true; - - case GlobalAction.EditorSeekToPreviousBookmark: - seekBookmark(-1); - return true; - - case GlobalAction.EditorSeekToNextBookmark: - seekBookmark(1); - return true; } if (e.Repeat) @@ -815,14 +788,6 @@ namespace osu.Game.Screens.Edit switch (e.Action) { - case GlobalAction.EditorAddBookmark: - addBookmarkAtCurrentTime(); - return true; - - case GlobalAction.EditorRemoveClosestBookmark: - removeBookmarksInProximityToCurrentTime(); - return true; - case GlobalAction.EditorCloneSelection: Clone(); return true; @@ -855,19 +820,6 @@ namespace osu.Game.Screens.Edit return false; } - private void addBookmarkAtCurrentTime() - { - int bookmark = (int)clock.CurrentTimeAccurate; - int idx = editorBeatmap.Bookmarks.BinarySearch(bookmark); - if (idx < 0) - editorBeatmap.Bookmarks.Insert(~idx, bookmark); - } - - private void removeBookmarksInProximityToCurrentTime() - { - editorBeatmap.Bookmarks.RemoveAll(b => Math.Abs(b - clock.CurrentTimeAccurate) < 2000); - } - public void OnReleased(KeyBindingReleaseEvent e) { } @@ -1202,16 +1154,6 @@ namespace osu.Game.Screens.Edit clock.SeekSmoothlyTo(found.StartTime); } - private void seekBookmark(int direction) - { - int? targetBookmark = direction < 1 - ? editorBeatmap.Bookmarks.Cast().LastOrDefault(b => b < clock.CurrentTimeAccurate) - : editorBeatmap.Bookmarks.Cast().FirstOrDefault(b => b > clock.CurrentTimeAccurate); - - if (targetBookmark != null) - clock.SeekSmoothlyTo(targetBookmark.Value); - } - private void seekSamplePoint(int direction) { double currentTime = clock.CurrentTimeAccurate; From 4cbfb5170790c301ecfa08214df9795e82b754e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 5 Feb 2025 15:30:11 +0100 Subject: [PATCH 16/18] Fix undoing bookmark operations potentially making them unsorted Found in testing of previous commit. This would break seeking between bookmarks. Reproduction steps on `master`: - open map with bookmark - delete the first bookmark - undo the deletion of the first bookmark - seek to previous bookmark will now always seek to the first bookmark rather than closest preceding regardless of current clock time --- osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs b/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs index f3d58a3c3c..e84b6bfc72 100644 --- a/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs +++ b/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs @@ -115,7 +115,9 @@ namespace osu.Game.Screens.Edit if (editorBeatmap.Bookmarks.Contains(newBookmark)) continue; - editorBeatmap.Bookmarks.Add(newBookmark); + int idx = editorBeatmap.Bookmarks.BinarySearch(newBookmark); + if (idx < 0) + editorBeatmap.Bookmarks.Insert(~idx, newBookmark); } } From 10711e5e2721ab11b28c4fc5f00e6769d9aad3ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 5 Feb 2025 15:39:36 +0100 Subject: [PATCH 17/18] Add missing `partial` --- osu.Game/Screens/Edit/BookmarkController.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Screens/Edit/BookmarkController.cs b/osu.Game/Screens/Edit/BookmarkController.cs index 8c048ba871..3d2cb4663f 100644 --- a/osu.Game/Screens/Edit/BookmarkController.cs +++ b/osu.Game/Screens/Edit/BookmarkController.cs @@ -17,7 +17,7 @@ using osu.Game.Screens.Edit.Components.Menus; namespace osu.Game.Screens.Edit { - public class BookmarkController : Component, IKeyBindingHandler + public partial class BookmarkController : Component, IKeyBindingHandler { public EditorMenuItem Menu { get; private set; } From d9b370e3a1d384758dee89ef704dd0c38a694ec8 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 6 Feb 2025 13:41:16 +0900 Subject: [PATCH 18/18] Add xmldoc for menu implying external consumption --- osu.Game/Screens/Edit/BookmarkController.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/osu.Game/Screens/Edit/BookmarkController.cs b/osu.Game/Screens/Edit/BookmarkController.cs index 3d2cb4663f..80e77364e5 100644 --- a/osu.Game/Screens/Edit/BookmarkController.cs +++ b/osu.Game/Screens/Edit/BookmarkController.cs @@ -19,6 +19,9 @@ namespace osu.Game.Screens.Edit { public partial class BookmarkController : Component, IKeyBindingHandler { + /// + /// Bookmarks menu item (with submenu containing options). Should be added to the 's global menu. + /// public EditorMenuItem Menu { get; private set; } [Resolved]