From e13e9abda9c051dfa2c61a45de4098f6e8349bf8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Fri, 7 Jun 2024 08:09:57 +0200 Subject: [PATCH 1/2] Disallow running save-related operations concurrently Closes https://github.com/ppy/osu/issues/25426. Different approach to prior ones, this just disables the relevant actions when something related to save/export is going on. Still ends up being convoluted because many things you wouldn't expect to touch save do touch save, so it's not just a concern between export and save specifically. --- osu.Game/Screens/Edit/BottomBar.cs | 12 +++ osu.Game/Screens/Edit/Editor.cs | 118 ++++++++++++++++++++++------- 2 files changed, 102 insertions(+), 28 deletions(-) diff --git a/osu.Game/Screens/Edit/BottomBar.cs b/osu.Game/Screens/Edit/BottomBar.cs index bc7dfaab88..612aa26c84 100644 --- a/osu.Game/Screens/Edit/BottomBar.cs +++ b/osu.Game/Screens/Edit/BottomBar.cs @@ -4,6 +4,7 @@ #nullable disable using osu.Framework.Allocation; +using osu.Framework.Bindables; using osu.Framework.Extensions.Color4Extensions; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; @@ -22,6 +23,8 @@ namespace osu.Game.Screens.Edit { public TestGameplayButton TestGameplayButton { get; private set; } + private IBindable saveInProgress; + [BackgroundDependencyLoader] private void load(OverlayColourProvider colourProvider, Editor editor) { @@ -74,6 +77,15 @@ namespace osu.Game.Screens.Edit } } }; + + saveInProgress = editor.SaveTracker.InProgress.GetBoundCopy(); + } + + protected override void LoadComplete() + { + base.LoadComplete(); + + saveInProgress.BindValueChanged(_ => TestGameplayButton.Enabled.Value = !saveInProgress.Value, true); } } } diff --git a/osu.Game/Screens/Edit/Editor.cs b/osu.Game/Screens/Edit/Editor.cs index 07c32983f5..991b92cca6 100644 --- a/osu.Game/Screens/Edit/Editor.cs +++ b/osu.Game/Screens/Edit/Editor.cs @@ -49,6 +49,7 @@ using osu.Game.Screens.Edit.GameplayTest; using osu.Game.Screens.Edit.Setup; using osu.Game.Screens.Edit.Timing; using osu.Game.Screens.Edit.Verify; +using osu.Game.Screens.OnlinePlay; using osu.Game.Screens.Play; using osu.Game.Users; using osuTK.Input; @@ -142,6 +143,8 @@ namespace osu.Game.Screens.Edit private readonly Bindable samplePlaybackDisabled = new Bindable(); private bool canSave; + private readonly List saveRelatedMenuItems = new List(); + public OngoingOperationTracker SaveTracker { get; private set; } = new OngoingOperationTracker(); protected bool ExitConfirmed { get; private set; } @@ -328,7 +331,7 @@ namespace osu.Game.Screens.Edit { new MenuItem(CommonStrings.MenuBarFile) { - Items = createFileMenuItems() + Items = createFileMenuItems().ToList() }, new MenuItem(CommonStrings.MenuBarEdit) { @@ -382,6 +385,7 @@ namespace osu.Game.Screens.Edit }, }, bottomBar = new BottomBar(), + SaveTracker, } }); changeHandler?.CanUndo.BindValueChanged(v => undoMenuItem.Action.Disabled = !v.NewValue, true); @@ -402,6 +406,12 @@ namespace osu.Game.Screens.Edit Mode.BindValueChanged(onModeChanged, true); musicController.TrackChanged += onTrackChanged; + + SaveTracker.InProgress.BindValueChanged(_ => + { + foreach (var item in saveRelatedMenuItems) + item.Action.Disabled = SaveTracker.InProgress.Value; + }, true); } protected override void Dispose(bool isDisposing) @@ -442,9 +452,14 @@ namespace osu.Game.Screens.Edit { dialogOverlay.Push(new SaveRequiredPopupDialog("The beatmap will be saved in order to test it.", () => { - if (!Save()) return; + if (SaveTracker.InProgress.Value) return; - pushEditorPlayer(); + using (SaveTracker.BeginOperation()) + { + if (!Save()) return; + + pushEditorPlayer(); + } })); } else @@ -520,7 +535,11 @@ namespace osu.Game.Screens.Edit if (e.Repeat) return false; - Save(); + if (SaveTracker.InProgress.Value) + return false; + + using (SaveTracker.BeginOperation()) + Save(); return true; } @@ -787,7 +806,13 @@ namespace osu.Game.Screens.Edit private void confirmExitWithSave() { - if (!Save()) return; + if (SaveTracker.InProgress.Value) return; + + using (SaveTracker.BeginOperation()) + { + if (!Save()) + return; + } ExitConfirmed = true; this.Exit(); @@ -1020,25 +1045,41 @@ namespace osu.Game.Screens.Edit lastSavedHash = changeHandler?.CurrentStateHash; } - private List createFileMenuItems() => new List + private IEnumerable createFileMenuItems() { - createDifficultyCreationMenu(), - createDifficultySwitchMenu(), - new OsuMenuItemSpacer(), - new EditorMenuItem(EditorStrings.DeleteDifficulty, MenuItemType.Standard, deleteDifficulty) { Action = { Disabled = Beatmap.Value.BeatmapSetInfo.Beatmaps.Count < 2 } }, - new OsuMenuItemSpacer(), - new EditorMenuItem(WebCommonStrings.ButtonsSave, MenuItemType.Standard, () => Save()), - createExportMenu(), - new OsuMenuItemSpacer(), - new EditorMenuItem(CommonStrings.Exit, MenuItemType.Standard, this.Exit) - }; + yield return createDifficultyCreationMenu(); + yield return createDifficultySwitchMenu(); + yield return new OsuMenuItemSpacer(); + yield return new EditorMenuItem(EditorStrings.DeleteDifficulty, MenuItemType.Standard, deleteDifficulty) { Action = { Disabled = Beatmap.Value.BeatmapSetInfo.Beatmaps.Count < 2 } }; + yield return new OsuMenuItemSpacer(); + + var save = new EditorMenuItem(WebCommonStrings.ButtonsSave, MenuItemType.Standard, () => + { + if (SaveTracker.InProgress.Value) return; + + using (SaveTracker.BeginOperation()) + Save(); + }); + saveRelatedMenuItems.Add(save); + yield return save; + + if (RuntimeInfo.IsDesktop) + { + var export = createExportMenu(); + saveRelatedMenuItems.AddRange(export.Items); + yield return export; + } + + yield return new OsuMenuItemSpacer(); + yield return new EditorMenuItem(CommonStrings.Exit, MenuItemType.Standard, this.Exit); + } private EditorMenuItem createExportMenu() { var exportItems = new List { - new EditorMenuItem(EditorStrings.ExportForEditing, MenuItemType.Standard, () => exportBeatmap(false)) { Action = { Disabled = !RuntimeInfo.IsDesktop } }, - new EditorMenuItem(EditorStrings.ExportForCompatibility, MenuItemType.Standard, () => exportBeatmap(true)) { Action = { Disabled = !RuntimeInfo.IsDesktop } }, + new EditorMenuItem(EditorStrings.ExportForEditing, MenuItemType.Standard, () => exportBeatmap(false)), + new EditorMenuItem(EditorStrings.ExportForCompatibility, MenuItemType.Standard, () => exportBeatmap(true)), }; return new EditorMenuItem(CommonStrings.Export) { Items = exportItems }; @@ -1050,22 +1091,35 @@ namespace osu.Game.Screens.Edit { dialogOverlay.Push(new SaveRequiredPopupDialog("The beatmap will be saved in order to export it.", () => { - if (!Save()) return; + if (SaveTracker.InProgress.Value) + return; - runExport(); + var operation = SaveTracker.BeginOperation(); + + if (!Save()) + { + operation.Dispose(); + return; + } + + runExport(operation); })); } else { - runExport(); + if (SaveTracker.InProgress.Value) + return; + + runExport(SaveTracker.BeginOperation()); } - void runExport() + void runExport(IDisposable operationInProgress) { - if (legacy) - beatmapManager.ExportLegacy(Beatmap.Value.BeatmapSetInfo); - else - beatmapManager.Export(Beatmap.Value.BeatmapSetInfo); + var task = legacy + ? beatmapManager.ExportLegacy(Beatmap.Value.BeatmapSetInfo) + : beatmapManager.Export(Beatmap.Value.BeatmapSetInfo); + + task.ContinueWith(_ => operationInProgress.Dispose()); } } @@ -1116,6 +1170,8 @@ namespace osu.Game.Screens.Edit foreach (var ruleset in rulesets.AvailableRulesets) rulesetItems.Add(new EditorMenuItem(ruleset.Name, MenuItemType.Standard, () => CreateNewDifficulty(ruleset))); + saveRelatedMenuItems.AddRange(rulesetItems); + return new EditorMenuItem(EditorStrings.CreateNewDifficulty) { Items = rulesetItems }; } @@ -1125,10 +1181,16 @@ namespace osu.Game.Screens.Edit { dialogOverlay.Push(new SaveRequiredPopupDialog("This beatmap will be saved in order to create another difficulty.", () => { - if (!Save()) + if (SaveTracker.InProgress.Value) return; - CreateNewDifficulty(rulesetInfo); + using (SaveTracker.BeginOperation()) + { + if (!Save()) + return; + + CreateNewDifficulty(rulesetInfo); + } })); return; From 1d6b7e9c9b860a4a7b8efd518b6724146e0d92ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Mon, 10 Jun 2024 10:28:10 +0200 Subject: [PATCH 2/2] Refactor further to address code quality complaints --- osu.Game/Screens/Edit/BottomBar.cs | 2 +- osu.Game/Screens/Edit/Editor.cs | 115 ++++++++++++++--------------- 2 files changed, 55 insertions(+), 62 deletions(-) diff --git a/osu.Game/Screens/Edit/BottomBar.cs b/osu.Game/Screens/Edit/BottomBar.cs index 612aa26c84..d43e675296 100644 --- a/osu.Game/Screens/Edit/BottomBar.cs +++ b/osu.Game/Screens/Edit/BottomBar.cs @@ -78,7 +78,7 @@ namespace osu.Game.Screens.Edit } }; - saveInProgress = editor.SaveTracker.InProgress.GetBoundCopy(); + saveInProgress = editor.MutationTracker.InProgress.GetBoundCopy(); } protected override void LoadComplete() diff --git a/osu.Game/Screens/Edit/Editor.cs b/osu.Game/Screens/Edit/Editor.cs index 991b92cca6..3e3e772810 100644 --- a/osu.Game/Screens/Edit/Editor.cs +++ b/osu.Game/Screens/Edit/Editor.cs @@ -6,6 +6,7 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Threading.Tasks; using JetBrains.Annotations; using osu.Framework; using osu.Framework.Allocation; @@ -35,6 +36,7 @@ using osu.Game.Graphics.UserInterface; using osu.Game.Input.Bindings; using osu.Game.Localisation; using osu.Game.Online.API; +using osu.Game.Online.Multiplayer; using osu.Game.Overlays; using osu.Game.Overlays.Notifications; using osu.Game.Overlays.OSD; @@ -144,7 +146,12 @@ namespace osu.Game.Screens.Edit private bool canSave; private readonly List saveRelatedMenuItems = new List(); - public OngoingOperationTracker SaveTracker { get; private set; } = new OngoingOperationTracker(); + + /// + /// Tracks ongoing mutually-exclusive operations related to changing the beatmap + /// (e.g. save, export). + /// + public OngoingOperationTracker MutationTracker { get; } = new OngoingOperationTracker(); protected bool ExitConfirmed { get; private set; } @@ -385,7 +392,7 @@ namespace osu.Game.Screens.Edit }, }, bottomBar = new BottomBar(), - SaveTracker, + MutationTracker, } }); changeHandler?.CanUndo.BindValueChanged(v => undoMenuItem.Action.Disabled = !v.NewValue, true); @@ -407,10 +414,10 @@ namespace osu.Game.Screens.Edit musicController.TrackChanged += onTrackChanged; - SaveTracker.InProgress.BindValueChanged(_ => + MutationTracker.InProgress.BindValueChanged(_ => { foreach (var item in saveRelatedMenuItems) - item.Action.Disabled = SaveTracker.InProgress.Value; + item.Action.Disabled = MutationTracker.InProgress.Value; }, true); } @@ -450,17 +457,13 @@ namespace osu.Game.Screens.Edit { if (HasUnsavedChanges) { - dialogOverlay.Push(new SaveRequiredPopupDialog("The beatmap will be saved in order to test it.", () => + dialogOverlay.Push(new SaveRequiredPopupDialog("The beatmap will be saved in order to test it.", () => attemptMutationOperation(() => { - if (SaveTracker.InProgress.Value) return; + if (!Save()) return false; - using (SaveTracker.BeginOperation()) - { - if (!Save()) return; - - pushEditorPlayer(); - } - })); + pushEditorPlayer(); + return true; + }))); } else { @@ -470,6 +473,26 @@ namespace osu.Game.Screens.Edit void pushEditorPlayer() => this.Push(new EditorPlayerLoader(this)); } + private bool attemptMutationOperation(Func mutationOperation) + { + if (MutationTracker.InProgress.Value) + return false; + + using (MutationTracker.BeginOperation()) + return mutationOperation.Invoke(); + } + + private bool attemptAsyncMutationOperation(Func mutationTask) + { + if (MutationTracker.InProgress.Value) + return false; + + var operation = MutationTracker.BeginOperation(); + var task = mutationTask.Invoke(); + task.FireAndForget(operation.Dispose, _ => operation.Dispose()); + return true; + } + /// /// Saves the currently edited beatmap. /// @@ -535,12 +558,7 @@ namespace osu.Game.Screens.Edit if (e.Repeat) return false; - if (SaveTracker.InProgress.Value) - return false; - - using (SaveTracker.BeginOperation()) - Save(); - return true; + return attemptMutationOperation(Save); } return false; @@ -806,13 +824,8 @@ namespace osu.Game.Screens.Edit private void confirmExitWithSave() { - if (SaveTracker.InProgress.Value) return; - - using (SaveTracker.BeginOperation()) - { - if (!Save()) - return; - } + if (!attemptMutationOperation(Save)) + return; ExitConfirmed = true; this.Exit(); @@ -1053,13 +1066,7 @@ namespace osu.Game.Screens.Edit yield return new EditorMenuItem(EditorStrings.DeleteDifficulty, MenuItemType.Standard, deleteDifficulty) { Action = { Disabled = Beatmap.Value.BeatmapSetInfo.Beatmaps.Count < 2 } }; yield return new OsuMenuItemSpacer(); - var save = new EditorMenuItem(WebCommonStrings.ButtonsSave, MenuItemType.Standard, () => - { - if (SaveTracker.InProgress.Value) return; - - using (SaveTracker.BeginOperation()) - Save(); - }); + var save = new EditorMenuItem(WebCommonStrings.ButtonsSave, MenuItemType.Standard, () => attemptMutationOperation(Save)); saveRelatedMenuItems.Add(save); yield return save; @@ -1089,37 +1096,25 @@ namespace osu.Game.Screens.Edit { if (HasUnsavedChanges) { - dialogOverlay.Push(new SaveRequiredPopupDialog("The beatmap will be saved in order to export it.", () => + dialogOverlay.Push(new SaveRequiredPopupDialog("The beatmap will be saved in order to export it.", () => attemptAsyncMutationOperation(() => { - if (SaveTracker.InProgress.Value) - return; - - var operation = SaveTracker.BeginOperation(); - if (!Save()) - { - operation.Dispose(); - return; - } + return Task.CompletedTask; - runExport(operation); - })); + return runExport(); + }))); } else { - if (SaveTracker.InProgress.Value) - return; - - runExport(SaveTracker.BeginOperation()); + attemptAsyncMutationOperation(runExport); } - void runExport(IDisposable operationInProgress) + Task runExport() { - var task = legacy - ? beatmapManager.ExportLegacy(Beatmap.Value.BeatmapSetInfo) - : beatmapManager.Export(Beatmap.Value.BeatmapSetInfo); - - task.ContinueWith(_ => operationInProgress.Dispose()); + if (legacy) + return beatmapManager.ExportLegacy(Beatmap.Value.BeatmapSetInfo); + else + return beatmapManager.Export(Beatmap.Value.BeatmapSetInfo); } } @@ -1181,16 +1176,14 @@ namespace osu.Game.Screens.Edit { dialogOverlay.Push(new SaveRequiredPopupDialog("This beatmap will be saved in order to create another difficulty.", () => { - if (SaveTracker.InProgress.Value) - return; - - using (SaveTracker.BeginOperation()) + attemptMutationOperation(() => { if (!Save()) - return; + return false; CreateNewDifficulty(rulesetInfo); - } + return true; + }); })); return;