From c629a7a36fe1acdd9e1143fb9c6692b5152c38a3 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 29 Nov 2021 16:18:34 +0900 Subject: [PATCH] Fix random selection and avoid using legacy events for handling skin import/deletion --- .../Overlays/Settings/Sections/SkinSection.cs | 95 +++++++++---------- osu.Game/Skinning/SkinManager.cs | 33 +++---- osu.Game/Stores/RealmArchiveModelManager.cs | 27 ++++-- 3 files changed, 73 insertions(+), 82 deletions(-) diff --git a/osu.Game/Overlays/Settings/Sections/SkinSection.cs b/osu.Game/Overlays/Settings/Sections/SkinSection.cs index 9c5a2dff47..88f60a6004 100644 --- a/osu.Game/Overlays/Settings/Sections/SkinSection.cs +++ b/osu.Game/Overlays/Settings/Sections/SkinSection.cs @@ -18,6 +18,7 @@ using osu.Game.Graphics.UserInterface; using osu.Game.Localisation; using osu.Game.Skinning; using osu.Game.Skinning.Editor; +using Realms; namespace osu.Game.Overlays.Settings.Sections { @@ -43,21 +44,15 @@ namespace osu.Game.Overlays.Settings.Sections private List> skinItems; - private int firstNonDefaultSkinIndex - { - get - { - int index = skinItems.FindIndex(s => s.ID == SkinInfo.CLASSIC_SKIN); - if (index < 0) - index = skinItems.Count; - - return index; - } - } - [Resolved] private SkinManager skins { get; set; } + [Resolved] + private RealmContextFactory realmFactory { get; set; } + + private IDisposable realmSubscription; + private IQueryable realmSkins; + [BackgroundDependencyLoader(permitNulls: true)] private void load(OsuConfigManager config, [CanBeNull] SkinEditorOverlay skinEditor) { @@ -75,9 +70,6 @@ namespace osu.Game.Overlays.Settings.Sections new ExportSkinButton(), }; - skins.ItemUpdated += itemUpdated; - skins.ItemRemoved += itemRemoved; - config.BindWith(OsuSetting.Skin, configBindable); } @@ -86,6 +78,21 @@ namespace osu.Game.Overlays.Settings.Sections base.LoadComplete(); skinDropdown.Current = dropdownBindable; + + realmSkins = realmFactory.Context.All() + .Where(s => !s.DeletePending) + .OrderBy(s => s.Name, StringComparer.OrdinalIgnoreCase); + + realmSubscription = realmSkins + .SubscribeForNotifications((sender, changes, error) => + { + if (changes == null) + return; + + // Eventually this should be handling the individual changes rather than refreshing the whole dropdown. + updateItems(); + }); + updateItems(); // Todo: This should not be necessary when OsuConfigManager is databased @@ -97,7 +104,16 @@ namespace osu.Game.Overlays.Settings.Sections { if (skin.NewValue.Equals(random_skin_info)) { + var skinBefore = skins.CurrentSkinInfo.Value; + skins.SelectRandomSkin(); + + if (skinBefore == skins.CurrentSkinInfo.Value) + { + // the random selection didn't change the skin, so we should manually update the dropdown to match. + dropdownBindable.Value = skins.CurrentSkinInfo.Value; + } + return; } @@ -111,12 +127,13 @@ namespace osu.Game.Overlays.Settings.Sections var skin = skinDropdown.Items.FirstOrDefault(s => s.ID == configId); + // TODO: i don't think this will be required any more. if (skin == null) { // there may be a thread race condition where an item is selected that hasn't yet been added to the dropdown. // to avoid adding complexity, let's just ensure the item is added so we can perform the selection. skin = skins.Query(s => s.ID == configId); - addItem(skin); + updateItems(); } dropdownBindable.Value = skin; @@ -124,47 +141,20 @@ namespace osu.Game.Overlays.Settings.Sections private void updateItems() { - skinItems = skins.GetAllUsableSkins(); - skinItems.Insert(firstNonDefaultSkinIndex, random_skin_info); - sortUserSkins(skinItems); + skinItems = realmSkins.ToLive(); + + skinItems.Insert(0, SkinInfo.Default.ToLive()); + skinItems.Insert(1, DefaultLegacySkin.Info.ToLive()); + skinItems.Insert(2, random_skin_info); + skinDropdown.Items = skinItems; } - private void itemUpdated(SkinInfo item) => Schedule(() => addItem(item.ToLive())); - - private void addItem(ILive item) - { - List> newDropdownItems = skinDropdown.Items.Where(i => !i.Equals(item)).Append(item).ToList(); - sortUserSkins(newDropdownItems); - skinDropdown.Items = newDropdownItems; - } - - private void itemRemoved(SkinInfo item) => Schedule(() => skinDropdown.Items = skinDropdown.Items.Where(i => !i.ID.Equals(item.ID)).ToArray()); - - private void sortUserSkins(List> skinsList) - { - try - { - // Sort user skins separately from built-in skins - skinsList.Sort(firstNonDefaultSkinIndex, skinsList.Count - firstNonDefaultSkinIndex, - Comparer>.Create((a, b) => - { - // o_________________________o - return a.PerformRead(ai => b.PerformRead(bi => string.Compare(ai.Name, bi.Name, StringComparison.OrdinalIgnoreCase))); - })); - } - catch { } - } - protected override void Dispose(bool isDisposing) { base.Dispose(isDisposing); - if (skins != null) - { - skins.ItemUpdated -= itemUpdated; - skins.ItemRemoved -= itemRemoved; - } + realmSubscription?.Dispose(); } private class SkinSettingsDropdown : SettingsDropdown> @@ -192,6 +182,11 @@ namespace osu.Game.Overlays.Settings.Sections { Text = SkinSettingsStrings.ExportSkinButton; Action = export; + } + + protected override void LoadComplete() + { + base.LoadComplete(); currentSkin = skins.CurrentSkin.GetBoundCopy(); currentSkin.BindValueChanged(skin => Enabled.Value = skin.NewValue.SkinInfo.PerformRead(s => s.IsManaged), true); diff --git a/osu.Game/Skinning/SkinManager.cs b/osu.Game/Skinning/SkinManager.cs index 2ec934ac32..bcad5277ab 100644 --- a/osu.Game/Skinning/SkinManager.cs +++ b/osu.Game/Skinning/SkinManager.cs @@ -38,6 +38,8 @@ namespace osu.Game.Skinning { private readonly AudioManager audio; + private readonly Scheduler scheduler; + private readonly GameHost host; private readonly IResourceStore resources; @@ -64,6 +66,7 @@ namespace osu.Game.Skinning { this.contextFactory = contextFactory; this.audio = audio; + this.scheduler = scheduler; this.host = host; this.resources = resources; @@ -84,15 +87,6 @@ namespace osu.Game.Skinning SourceChanged?.Invoke(); }; - - // needs to be done here rather than inside SkinManager to ensure thread safety of CurrentSkinInfo. - ItemRemoved += item => scheduler.Add(() => - { - // TODO: fix. - // check the removed skin is not the current user choice. if it is, switch back to default. - // if (item.Equals(CurrentSkinInfo.Value)) - // CurrentSkinInfo.Value = SkinInfo.Default; - }); } /// @@ -283,23 +277,18 @@ namespace osu.Game.Skinning #region Implementation of IModelManager - public event Action ItemUpdated - { - add => skinModelManager.ItemUpdated += value; - remove => skinModelManager.ItemUpdated -= value; - } - - public event Action ItemRemoved - { - add => skinModelManager.ItemRemoved += value; - remove => skinModelManager.ItemRemoved -= value; - } - - public void Delete(Expression> filter, bool silent = false) + public void Delete([CanBeNull] Expression> filter = null, bool silent = false) { using (var context = contextFactory.CreateContext()) { var items = context.All().Where(filter).ToList(); + + // check the removed skin is not the current user choice. if it is, switch back to default. + Guid currentUserSkin = CurrentSkinInfo.Value.ID; + + if (items.Any(s => s.ID == currentUserSkin)) + scheduler.Add(() => CurrentSkinInfo.Value = SkinInfo.Default.ToLive()); + skinModelManager.Delete(items, silent); } } diff --git a/osu.Game/Stores/RealmArchiveModelManager.cs b/osu.Game/Stores/RealmArchiveModelManager.cs index 14b7077dd2..ae40bcac79 100644 --- a/osu.Game/Stores/RealmArchiveModelManager.cs +++ b/osu.Game/Stores/RealmArchiveModelManager.cs @@ -24,8 +24,21 @@ namespace osu.Game.Stores public abstract class RealmArchiveModelManager : RealmArchiveModelImporter, IModelManager, IModelFileManager where TModel : RealmObject, IHasRealmFiles, IHasGuidPrimaryKey, ISoftDelete { - public event Action? ItemUpdated; - public event Action? ItemRemoved; + public event Action? ItemUpdated + { + // This may be brought back for beatmaps to ease integration. + // The eventual goal would be not requiring this and using realm subscriptions in its place. + add => throw new NotImplementedException(); + remove => throw new NotImplementedException(); + } + + public event Action? ItemRemoved + { + // This may be brought back for beatmaps to ease integration. + // The eventual goal would be not requiring this and using realm subscriptions in its place. + add => throw new NotImplementedException(); + remove => throw new NotImplementedException(); + } private readonly RealmFileStore realmFileStore; @@ -64,11 +77,7 @@ namespace osu.Game.Stores public override async Task?> Import(TModel item, ArchiveReader? archive = null, bool lowPriority = false, CancellationToken cancellationToken = default) { - var imported = await base.Import(item, archive, lowPriority, cancellationToken).ConfigureAwait(false); - - imported?.PerformRead(i => ItemUpdated?.Invoke(i.Detach())); - - return imported; + return await base.Import(item, archive, lowPriority, cancellationToken).ConfigureAwait(false); } /// @@ -150,7 +159,6 @@ namespace osu.Game.Stores return false; item.Realm.Write(r => item.DeletePending = true); - ItemRemoved?.Invoke(item.Detach()); return true; } @@ -160,10 +168,9 @@ namespace osu.Game.Stores return; item.Realm.Write(r => item.DeletePending = false); - ItemUpdated?.Invoke(item); } - public virtual bool IsAvailableLocally(TModel model) => false; // TODO: implement + public virtual bool IsAvailableLocally(TModel model) => false; // Not relevant for skins since they can't be downloaded yet. public void Update(TModel skin) {