From 4ecc724841ac075f24f8d5695fb277672ccceca8 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 27 Jun 2023 18:18:58 +0900 Subject: [PATCH 1/8] Add test coverage of save failure when beatmap is detached from set --- .../Visual/Editing/TestSceneEditorSaving.cs | 31 +++++++++++++++++-- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs b/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs index 64c48e74cf..7191ae6a57 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs +++ b/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.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.Linq; using NUnit.Framework; using osu.Framework.Allocation; @@ -96,6 +94,33 @@ namespace osu.Game.Tests.Visual.Editing AddAssert("Beatmap still has correct timeline zoom", () => EditorBeatmap.BeatmapInfo.TimelineZoom == changedTimelineZoom); } + [Test] + public void TestSaveWithDetachedBeatmap() + { + AddStep("Set overall difficulty", () => EditorBeatmap.Difficulty.OverallDifficulty = 7); + + SaveEditor(); + + AddStep("Detach beatmap from set", () => + { + Realm.Write(r => + { + BeatmapSetInfo? beatmapSet = r.Find(EditorBeatmap.BeatmapInfo.BeatmapSet!.ID); + BeatmapInfo? beatmap = r.Find(EditorBeatmap.BeatmapInfo.ID); + + beatmapSet.Beatmaps.Remove(beatmap); + }); + }); + + SaveEditor(); + + AddAssert("Beatmap has correct overall difficulty", () => EditorBeatmap.Difficulty.OverallDifficulty == 7); + + ReloadEditorToSameBeatmap(); + + AddAssert("Beatmap still has correct overall difficulty", () => EditorBeatmap.Difficulty.OverallDifficulty == 7); + } + [Test] public void TestDifficulty() { @@ -130,7 +155,7 @@ namespace osu.Game.Tests.Visual.Editing [Test] public void TestLengthAndStarRatingUpdated() { - WorkingBeatmap working = null; + WorkingBeatmap working = null!; double lastStarRating = 0; double lastLength = 0; From 8e80e2fa323337f885ae8119a5a14b5b54e68596 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 27 Jun 2023 18:19:59 +0900 Subject: [PATCH 2/8] Fix incorrect realm copy logic when a beatmap becomes detached from its set The code here was assuming that if the beatmap which is having changes copied across does not exist within the `BeatmapSet.Beatmaps` list, it was not yet persisted to realm. In some edge case, it can happen that the beatmap *is* persisted to realm but not correctly attached to the beatmap set. I don't yet know how this occurs, but it has caused loss of data for at least two users. The fix here is to check realm-wide for the beatmap (using its primary key) rather than only in the list. We then handle the scenario where the beatmap needs to be reattached to the set as a seprate step. --- This does raise others questions like "are we even structuring this correctly? couldn't a single beatmap exist in two different sets?" Maybe, but let's deal with that if/when it comes up. --- osu.Game/Database/RealmObjectExtensions.cs | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/osu.Game/Database/RealmObjectExtensions.cs b/osu.Game/Database/RealmObjectExtensions.cs index a771aa04df..888c1cf8ce 100644 --- a/osu.Game/Database/RealmObjectExtensions.cs +++ b/osu.Game/Database/RealmObjectExtensions.cs @@ -52,10 +52,19 @@ namespace osu.Game.Database { foreach (var beatmap in s.Beatmaps) { - var existing = d.Beatmaps.FirstOrDefault(b => b.ID == beatmap.ID); + // Importantly, search all of realm for the beatmap (not just the set's beatmaps). + // It may have gotten detached, and if that's the case let's use this opportunity to fix + // things up. + var existingBeatmap = d.Realm.Find(beatmap.ID); - if (existing != null) - copyChangesToRealm(beatmap, existing); + if (existingBeatmap != null) + { + // As above, reattach if it happens to not be in the set's beatmaps. + if (!d.Beatmaps.Contains(existingBeatmap)) + d.Beatmaps.Add(existingBeatmap); + + copyChangesToRealm(beatmap, existingBeatmap); + } else { var newBeatmap = new BeatmapInfo @@ -64,6 +73,7 @@ namespace osu.Game.Database BeatmapSet = d, Ruleset = d.Realm.Find(beatmap.Ruleset.ShortName) }; + d.Beatmaps.Add(newBeatmap); copyChangesToRealm(beatmap, newBeatmap); } From 99e55bb9c03972fdee0569ee4ae7a5c37eac84a9 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 28 Jun 2023 12:21:05 +0900 Subject: [PATCH 3/8] Add logging and `Debug.Fail` on detached beatmap detection --- osu.Game/Database/RealmObjectExtensions.cs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/osu.Game/Database/RealmObjectExtensions.cs b/osu.Game/Database/RealmObjectExtensions.cs index 888c1cf8ce..5a6c2e3232 100644 --- a/osu.Game/Database/RealmObjectExtensions.cs +++ b/osu.Game/Database/RealmObjectExtensions.cs @@ -3,10 +3,12 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Linq; using System.Runtime.Serialization; using AutoMapper; using AutoMapper.Internal; +using osu.Framework.Logging; using osu.Game.Beatmaps; using osu.Game.Input.Bindings; using osu.Game.Models; @@ -61,7 +63,11 @@ namespace osu.Game.Database { // As above, reattach if it happens to not be in the set's beatmaps. if (!d.Beatmaps.Contains(existingBeatmap)) + { + Debug.Fail("Beatmaps should never become detached under normal circumstances. If this ever triggers, it should be investigated further."); + Logger.Log("WARNING: One of the difficulties in a beatmap was detached from its set. Please save a copy of logs and report this to devs.", LoggingTarget.Database, LogLevel.Important); d.Beatmaps.Add(existingBeatmap); + } copyChangesToRealm(beatmap, existingBeatmap); } From 29376ffcc0c7a52620ddbbe6096261b7ca6dd3ab Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 28 Jun 2023 12:54:12 +0900 Subject: [PATCH 4/8] Trigger state change when flipping via hotkey in the editor This will trigger a change even if nothing happens. But I think that's okay (not easy to avoid) because the change handler should be aware that nothing changed, if anything. Closes https://github.com/ppy/osu/issues/24065. --- .../Edit/Compose/Components/SelectionHandler.cs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/osu.Game/Screens/Edit/Compose/Components/SelectionHandler.cs b/osu.Game/Screens/Edit/Compose/Components/SelectionHandler.cs index 5cedf1ca42..9ec59cf833 100644 --- a/osu.Game/Screens/Edit/Compose/Components/SelectionHandler.cs +++ b/osu.Game/Screens/Edit/Compose/Components/SelectionHandler.cs @@ -163,10 +163,18 @@ namespace osu.Game.Screens.Edit.Compose.Components switch (e.Action) { case GlobalAction.EditorFlipHorizontally: - return HandleFlip(Direction.Horizontal, true); + ChangeHandler?.BeginChange(); + HandleFlip(Direction.Horizontal, true); + ChangeHandler?.EndChange(); + + return true; case GlobalAction.EditorFlipVertically: - return HandleFlip(Direction.Vertical, true); + ChangeHandler?.BeginChange(); + HandleFlip(Direction.Vertical, true); + ChangeHandler?.EndChange(); + + return true; } return false; From 6ce0ca832e57f2b9d4e96d9a3b9a908d49896f28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 28 Jun 2023 19:52:08 +0200 Subject: [PATCH 5/8] Delete test case covering beatmap detach scenario Due to being fundamentally incompatible with the `Debug.Fail()` call added in 99e55bb9c03972fdee0569ee4ae7a5c37eac84a9. This reverts commit 4ecc724841ac075f24f8d5695fb277672ccceca8. --- .../Visual/Editing/TestSceneEditorSaving.cs | 31 ++----------------- 1 file changed, 3 insertions(+), 28 deletions(-) diff --git a/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs b/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs index 7191ae6a57..64c48e74cf 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs +++ b/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs @@ -1,6 +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.Linq; using NUnit.Framework; using osu.Framework.Allocation; @@ -94,33 +96,6 @@ namespace osu.Game.Tests.Visual.Editing AddAssert("Beatmap still has correct timeline zoom", () => EditorBeatmap.BeatmapInfo.TimelineZoom == changedTimelineZoom); } - [Test] - public void TestSaveWithDetachedBeatmap() - { - AddStep("Set overall difficulty", () => EditorBeatmap.Difficulty.OverallDifficulty = 7); - - SaveEditor(); - - AddStep("Detach beatmap from set", () => - { - Realm.Write(r => - { - BeatmapSetInfo? beatmapSet = r.Find(EditorBeatmap.BeatmapInfo.BeatmapSet!.ID); - BeatmapInfo? beatmap = r.Find(EditorBeatmap.BeatmapInfo.ID); - - beatmapSet.Beatmaps.Remove(beatmap); - }); - }); - - SaveEditor(); - - AddAssert("Beatmap has correct overall difficulty", () => EditorBeatmap.Difficulty.OverallDifficulty == 7); - - ReloadEditorToSameBeatmap(); - - AddAssert("Beatmap still has correct overall difficulty", () => EditorBeatmap.Difficulty.OverallDifficulty == 7); - } - [Test] public void TestDifficulty() { @@ -155,7 +130,7 @@ namespace osu.Game.Tests.Visual.Editing [Test] public void TestLengthAndStarRatingUpdated() { - WorkingBeatmap working = null!; + WorkingBeatmap working = null; double lastStarRating = 0; double lastLength = 0; From 0940ab1e11cb246975908f8f00974511f1864fa4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 28 Jun 2023 20:47:00 +0200 Subject: [PATCH 6/8] Add failing tests covering correct flip handling --- .../Editing/TestSceneComposerSelection.cs | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/osu.Game.Tests/Visual/Editing/TestSceneComposerSelection.cs b/osu.Game.Tests/Visual/Editing/TestSceneComposerSelection.cs index 4d99c47f77..d6934a3770 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneComposerSelection.cs +++ b/osu.Game.Tests/Visual/Editing/TestSceneComposerSelection.cs @@ -133,6 +133,32 @@ namespace osu.Game.Tests.Visual.Editing AddAssert("objects reverted to original position", () => addedObjects[0].Position == new Vector2(0)); } + [Test] + public void TestGlobalFlipHotkeys() + { + HitCircle addedObject = null; + + AddStep("add hitobjects", () => EditorBeatmap.Add(addedObject = new HitCircle { StartTime = 100 })); + + AddStep("select objects", () => EditorBeatmap.SelectedHitObjects.Add(addedObject)); + + AddStep("flip horizontally across playfield", () => + { + InputManager.PressKey(Key.ControlLeft); + InputManager.Key(Key.H); + InputManager.ReleaseKey(Key.ControlLeft); + }); + AddAssert("objects flipped horizontally", () => addedObject.Position == new Vector2(OsuPlayfield.BASE_SIZE.X, 0)); + + AddStep("flip vertically across playfield", () => + { + InputManager.PressKey(Key.ControlLeft); + InputManager.Key(Key.J); + InputManager.ReleaseKey(Key.ControlLeft); + }); + AddAssert("objects flipped vertically", () => addedObject.Position == OsuPlayfield.BASE_SIZE); + } + [Test] public void TestBasicSelect() { From e4e08c0f5fb787091d28a38b4e67bc59b2b0b846 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 28 Jun 2023 20:48:22 +0200 Subject: [PATCH 7/8] Fix selection handlers eating hotkey presses they didn't handle --- .../Edit/Compose/Components/SelectionHandler.cs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/osu.Game/Screens/Edit/Compose/Components/SelectionHandler.cs b/osu.Game/Screens/Edit/Compose/Components/SelectionHandler.cs index 9ec59cf833..052cb18a5d 100644 --- a/osu.Game/Screens/Edit/Compose/Components/SelectionHandler.cs +++ b/osu.Game/Screens/Edit/Compose/Components/SelectionHandler.cs @@ -160,21 +160,23 @@ namespace osu.Game.Screens.Edit.Compose.Components if (e.Repeat) return false; + bool handled; + switch (e.Action) { case GlobalAction.EditorFlipHorizontally: ChangeHandler?.BeginChange(); - HandleFlip(Direction.Horizontal, true); + handled = HandleFlip(Direction.Horizontal, true); ChangeHandler?.EndChange(); - return true; + return handled; case GlobalAction.EditorFlipVertically: ChangeHandler?.BeginChange(); - HandleFlip(Direction.Vertical, true); + handled = HandleFlip(Direction.Vertical, true); ChangeHandler?.EndChange(); - return true; + return handled; } return false; From ea8700053917628b69aa37a64d508753cac63f11 Mon Sep 17 00:00:00 2001 From: Bastian Pedersen Date: Wed, 28 Jun 2023 21:11:56 +0200 Subject: [PATCH 8/8] Localise chat related notifications --- osu.Game/Localisation/NotificationsStrings.cs | 10 ++++++++++ osu.Game/Online/Chat/MessageNotifier.cs | 5 +++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/osu.Game/Localisation/NotificationsStrings.cs b/osu.Game/Localisation/NotificationsStrings.cs index b6f2a55e37..44e440e8d9 100644 --- a/osu.Game/Localisation/NotificationsStrings.cs +++ b/osu.Game/Localisation/NotificationsStrings.cs @@ -88,6 +88,16 @@ Please try changing your audio device to a working setting."); /// public static LocalisableString LinkTypeNotSupported => new TranslatableString(getKey(@"unsupported_link_type"), @"This link type is not yet supported!"); + /// + /// "You received a private message from '{0}'. Click to read it!" + /// + public static LocalisableString PrivateMessageReceived(string username) => new TranslatableString(getKey(@"private_message_received"), @"You received a private message from '{0}'. Click to read it!", username); + + /// + /// "Your name was mentioned in chat by '{0}'. Click to find out why!" + /// + public static LocalisableString YourNameWasMentioned(string username) => new TranslatableString(getKey(@"your_name_was_mentioned"), @"Your name was mentioned in chat by '{0}'. Click to find out why!", username); + private static string getKey(string key) => $@"{prefix}:{key}"; } } diff --git a/osu.Game/Online/Chat/MessageNotifier.cs b/osu.Game/Online/Chat/MessageNotifier.cs index 52bdd36169..ae249d1b7f 100644 --- a/osu.Game/Online/Chat/MessageNotifier.cs +++ b/osu.Game/Online/Chat/MessageNotifier.cs @@ -15,6 +15,7 @@ using osu.Framework.Graphics.Sprites; using osu.Framework.Platform; using osu.Game.Configuration; using osu.Game.Graphics; +using osu.Game.Localisation; using osu.Game.Online.API; using osu.Game.Online.API.Requests.Responses; using osu.Game.Overlays; @@ -154,7 +155,7 @@ namespace osu.Game.Online.Chat : base(message, channel) { Icon = FontAwesome.Solid.Envelope; - Text = $"You received a private message from '{message.Sender.Username}'. Click to read it!"; + Text = NotificationsStrings.PrivateMessageReceived(message.Sender.Username); } } @@ -164,7 +165,7 @@ namespace osu.Game.Online.Chat : base(message, channel) { Icon = FontAwesome.Solid.At; - Text = $"Your name was mentioned in chat by '{message.Sender.Username}'. Click to find out why!"; + Text = NotificationsStrings.YourNameWasMentioned(message.Sender.Username); } }