From 9a7fdb2b7e31e0495c12e5294c89a24042f8254d Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 16 Nov 2020 16:43:17 +0900 Subject: [PATCH] Move skin deletion logic to OsuGameBase to promote thread safety `CurrentSkinInfo` is used in multiple places expecting thread safety, while ItemRemoved events are explicitly mentioning they are not thread safe. As SkinManager itself doesn't have the ability to schedule to the update thread, I've just moved the logic to `OsuGameBase`. We may want to move the current skin bindable out of the manager class in the future to match things like `BeatmapManager`. Closes https://github.com/ppy/osu/issues/10837. --- osu.Game/OsuGameBase.cs | 11 +++++++++++ osu.Game/Skinning/SkinManager.cs | 10 ---------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/osu.Game/OsuGameBase.cs b/osu.Game/OsuGameBase.cs index 193f6fe61b..1147f67ad2 100644 --- a/osu.Game/OsuGameBase.cs +++ b/osu.Game/OsuGameBase.cs @@ -194,6 +194,17 @@ namespace osu.Game dependencies.Cache(SkinManager = new SkinManager(Storage, contextFactory, Host, Audio, new NamespacedResourceStore(Resources, "Skins/Legacy"))); dependencies.CacheAs(SkinManager); + // needs to be done here rather than inside SkinManager to ensure thread safety of CurrentSkinInfo. + SkinManager.ItemRemoved.BindValueChanged(weakRemovedInfo => + { + if (weakRemovedInfo.NewValue.TryGetTarget(out var removedInfo)) + { + // check the removed skin is not the current user choice. if it is, switch back to default. + if (removedInfo.ID == SkinManager.CurrentSkinInfo.Value.ID) + Schedule(() => SkinManager.CurrentSkinInfo.Value = SkinInfo.Default); + } + }); + dependencies.CacheAs(API ??= new APIAccess(LocalConfig)); dependencies.CacheAs(spectatorStreaming = new SpectatorStreamingClient()); diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index bef3e86a4d..9b69a1eecd 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -48,16 +48,6 @@ namespace osu.Game.Skinning this.audio = audio; this.legacyDefaultResources = legacyDefaultResources; - ItemRemoved.BindValueChanged(weakRemovedInfo => - { - if (weakRemovedInfo.NewValue.TryGetTarget(out var removedInfo)) - { - // check the removed skin is not the current user choice. if it is, switch back to default. - if (removedInfo.ID == CurrentSkinInfo.Value.ID) - CurrentSkinInfo.Value = SkinInfo.Default; - } - }); - CurrentSkinInfo.ValueChanged += skin => CurrentSkin.Value = GetSkin(skin.NewValue); CurrentSkin.ValueChanged += skin => {