Fix random selection and avoid using legacy events for handling skin import/deletion

osu.Game/Overlays/Settings/Sections/SkinSection.cs

@@ -18,6 +18,7 @@
 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 @@ public class SkinSection : SettingsSection
 
         private List<ILive<SkinInfo>> 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<SkinInfo> realmSkins;
+
         [BackgroundDependencyLoader(permitNulls: true)]
         private void load(OsuConfigManager config, [CanBeNull] SkinEditorOverlay skinEditor)
         {
@@ -75,9 +70,6 @@ private void load(OsuConfigManager config, [CanBeNull] SkinEditorOverlay skinEdi
                 new ExportSkinButton(),
             };
 
-            skins.ItemUpdated += itemUpdated;
-            skins.ItemRemoved += itemRemoved;
-
             config.BindWith(OsuSetting.Skin, configBindable);
         }
 
@@ -86,6 +78,21 @@ protected override void LoadComplete()
             base.LoadComplete();
 
             skinDropdown.Current = dropdownBindable;
+
+            realmSkins = realmFactory.Context.All<SkinInfo>()
+                                     .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 @@ protected override void LoadComplete()
             {
                 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 @@ private void updateSelectedSkinFromConfig()
 
             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 @@ private void updateSelectedSkinFromConfig()
 
         private void updateItems()
         {
-            skinItems = skins.GetAllUsableSkins();
+            skinItems = realmSkins.ToLive();
-            skinItems.Insert(firstNonDefaultSkinIndex, random_skin_info);
+
-            sortUserSkins(skinItems);
+            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<SkinInfo> item)
-        {
-            List<ILive<SkinInfo>> 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<ILive<SkinInfo>> skinsList)
-        {
-            try
-            {
-                // Sort user skins separately from built-in skins
-                skinsList.Sort(firstNonDefaultSkinIndex, skinsList.Count - firstNonDefaultSkinIndex,
-                    Comparer<ILive<SkinInfo>>.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)
+            realmSubscription?.Dispose();
-            {
-                skins.ItemUpdated -= itemUpdated;
-                skins.ItemRemoved -= itemRemoved;
-            }
         }
 
         private class SkinSettingsDropdown : SettingsDropdown<ILive<SkinInfo>>
@@ -192,6 +182,11 @@ private void load()
             {
                 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);

osu.Game/Skinning/SkinManager.cs

@@ -38,6 +38,8 @@ public class SkinManager : ISkinSource, IStorageResourceProvider, IModelImporter
     {
         private readonly AudioManager audio;
 
+        private readonly Scheduler scheduler;
+
         private readonly GameHost host;
 
         private readonly IResourceStore<byte[]> resources;
@@ -64,6 +66,7 @@ public SkinManager(Storage storage, RealmContextFactory contextFactory, GameHost
         {
             this.contextFactory = contextFactory;
             this.audio = audio;
+            this.scheduler = scheduler;
             this.host = host;
             this.resources = resources;
 
@@ -84,15 +87,6 @@ public SkinManager(Storage storage, RealmContextFactory contextFactory, GameHost
 
                 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;
-            });
         }
 
         /// <summary>
@@ -283,23 +277,18 @@ public Task<ILive<SkinInfo>> Import(SkinInfo item, ArchiveReader archive = null,
 
         #region Implementation of IModelManager<SkinInfo>
 
-        public event Action<SkinInfo> ItemUpdated
+        public void Delete([CanBeNull] Expression<Func<SkinInfo, bool>> filter = null, bool silent = false)
-        {
-            add => skinModelManager.ItemUpdated += value;
-            remove => skinModelManager.ItemUpdated -= value;
-        }
-
-        public event Action<SkinInfo> ItemRemoved
-        {
-            add => skinModelManager.ItemRemoved += value;
-            remove => skinModelManager.ItemRemoved -= value;
-        }
-
-        public void Delete(Expression<Func<SkinInfo, bool>> filter, bool silent = false)
         {
             using (var context = contextFactory.CreateContext())
             {
                 var items = context.All<SkinInfo>().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);
             }
         }

osu.Game/Stores/RealmArchiveModelManager.cs

@@ -24,8 +24,21 @@ namespace osu.Game.Stores
     public abstract class RealmArchiveModelManager<TModel> : RealmArchiveModelImporter<TModel>, IModelManager<TModel>, IModelFileManager<TModel, RealmNamedFileUsage>
         where TModel : RealmObject, IHasRealmFiles, IHasGuidPrimaryKey, ISoftDelete
     {
-        public event Action<TModel>? ItemUpdated;
+        public event Action<TModel>? ItemUpdated
-        public event Action<TModel>? 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();
+        }
+
+        public event Action<TModel>? 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 @@ public void AddFile(TModel item, Stream stream, string filename, Realm realm)
 
         public override async Task<ILive<TModel>?> Import(TModel item, ArchiveReader? archive = null, bool lowPriority = false, CancellationToken cancellationToken = default)
         {
-            var imported = await base.Import(item, archive, lowPriority, cancellationToken).ConfigureAwait(false);
+            return await base.Import(item, archive, lowPriority, cancellationToken).ConfigureAwait(false);
-
-            imported?.PerformRead(i => ItemUpdated?.Invoke(i.Detach()));
-
-            return imported;
         }
 
         /// <summary>
@@ -150,7 +159,6 @@ public bool Delete(TModel item)
                 return false;
 
             item.Realm.Write(r => item.DeletePending = true);
-            ItemRemoved?.Invoke(item.Detach());
             return true;
         }
 
@@ -160,10 +168,9 @@ public void Undelete(TModel item)
                 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)
         {