From d1a22562621a2768056d3604400108f0b20c5441 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 26 Jan 2022 17:48:11 +0900 Subject: [PATCH 1/2] Refactor `SkinSection` to avoid unnecessary realm queries --- .../Overlays/Settings/Sections/SkinSection.cs | 83 ++++++++++--------- 1 file changed, 42 insertions(+), 41 deletions(-) diff --git a/osu.Game/Overlays/Settings/Sections/SkinSection.cs b/osu.Game/Overlays/Settings/Sections/SkinSection.cs index 0846c023c1..441a439596 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 { @@ -51,12 +52,6 @@ namespace osu.Game.Overlays.Settings.Sections private IDisposable realmSubscription; - private IQueryable queryRealmSkins() => - realm.Realm.All() - .Where(s => !s.DeletePending) - .OrderByDescending(s => s.Protected) // protected skins should be at the top. - .ThenBy(s => s.Name, StringComparer.OrdinalIgnoreCase); - [BackgroundDependencyLoader(permitNulls: true)] private void load(OsuConfigManager config, [CanBeNull] SkinEditorOverlay skinEditor) { @@ -83,37 +78,54 @@ namespace osu.Game.Overlays.Settings.Sections skinDropdown.Current = dropdownBindable; - realmSubscription = realm.RegisterForNotifications(r => queryRealmSkins(), (sender, changes, error) => - { - // The first fire of this is a bit redundant due to the call below, - // but this is safest in case the subscription is restored after a context recycle. - updateItems(); - }); - - updateItems(); + realmSubscription = realm.RegisterForNotifications(r => realm.Realm.All() + .Where(s => !s.DeletePending) + .OrderByDescending(s => s.Protected) // protected skins should be at the top. + .ThenBy(s => s.Name, StringComparer.OrdinalIgnoreCase), skinsChanged); configBindable.BindValueChanged(id => Scheduler.AddOnce(updateSelectedSkinFromConfig)); - updateSelectedSkinFromConfig(); - dropdownBindable.BindValueChanged(skin => + dropdownBindable.BindValueChanged(dropdownSelectionChanged); + } + + private void dropdownSelectionChanged(ValueChangedEvent> skin) + { + // Only handle cases where it's clear the user has intent to change skins. + if (skin.OldValue == null) return; + + if (skin.NewValue.Equals(random_skin_info)) { - if (skin.NewValue.Equals(random_skin_info)) + var skinBefore = skins.CurrentSkinInfo.Value; + + skins.SelectRandomSkin(); + + if (skinBefore == skins.CurrentSkinInfo.Value) { - 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; + // the random selection didn't change the skin, so we should manually update the dropdown to match. + dropdownBindable.Value = skins.CurrentSkinInfo.Value; } - configBindable.Value = skin.NewValue.ID.ToString(); - }); + return; + } + + configBindable.Value = skin.NewValue.ID.ToString(); + } + + private void skinsChanged(IRealmCollection skins, ChangeSet changes, Exception error) + { + // This can only mean that realm is recycling, else we would see the protected skins. + // Because we are using `Live<>` in this class, we don't need to worry about this scenario too much. + if (!skins.Any()) + return; + + int protectedCount = skins.Count(s => s.Protected); + + skinItems = skins.ToLive(realm); + skinItems.Insert(protectedCount, random_skin_info); + + skinDropdown.Items = skinItems; + + updateSelectedSkinFromConfig(); } private void updateSelectedSkinFromConfig() @@ -126,17 +138,6 @@ namespace osu.Game.Overlays.Settings.Sections dropdownBindable.Value = skin ?? skinDropdown.Items.First(); } - private void updateItems() - { - int protectedCount = queryRealmSkins().Count(s => s.Protected); - - skinItems = queryRealmSkins().ToLive(realm); - - skinItems.Insert(protectedCount, random_skin_info); - - skinDropdown.Items = skinItems; - } - protected override void Dispose(bool isDisposing) { base.Dispose(isDisposing); From 45636ce04b224ca516b798c253f2289c357aaa5c Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 26 Jan 2022 18:25:27 +0900 Subject: [PATCH 2/2] Remove collection `ToLive` helper method to avoid confusion --- osu.Game/Database/RealmObjectExtensions.cs | 6 ------ .../Overlays/Settings/Sections/SkinSection.cs | 18 +++++++++++------- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/osu.Game/Database/RealmObjectExtensions.cs b/osu.Game/Database/RealmObjectExtensions.cs index dba8633f53..7a0ca2c85a 100644 --- a/osu.Game/Database/RealmObjectExtensions.cs +++ b/osu.Game/Database/RealmObjectExtensions.cs @@ -216,12 +216,6 @@ namespace osu.Game.Database return new RealmLiveUnmanaged(realmObject); } - public static List> ToLive(this IEnumerable realmList, RealmAccess realm) - where T : RealmObject, IHasGuidPrimaryKey - { - return realmList.Select(l => new RealmLive(l, realm)).Cast>().ToList(); - } - public static Live ToLive(this T realmObject, RealmAccess realm) where T : RealmObject, IHasGuidPrimaryKey { diff --git a/osu.Game/Overlays/Settings/Sections/SkinSection.cs b/osu.Game/Overlays/Settings/Sections/SkinSection.cs index 441a439596..1dfe49945f 100644 --- a/osu.Game/Overlays/Settings/Sections/SkinSection.cs +++ b/osu.Game/Overlays/Settings/Sections/SkinSection.cs @@ -42,7 +42,7 @@ namespace osu.Game.Overlays.Settings.Sections Name = "", }.ToLiveUnmanaged(); - private List> skinItems; + private readonly List> dropdownItems = new List>(); [Resolved] private SkinManager skins { get; set; } @@ -111,19 +111,23 @@ namespace osu.Game.Overlays.Settings.Sections configBindable.Value = skin.NewValue.ID.ToString(); } - private void skinsChanged(IRealmCollection skins, ChangeSet changes, Exception error) + private void skinsChanged(IRealmCollection sender, ChangeSet changes, Exception error) { // This can only mean that realm is recycling, else we would see the protected skins. // Because we are using `Live<>` in this class, we don't need to worry about this scenario too much. - if (!skins.Any()) + if (!sender.Any()) return; - int protectedCount = skins.Count(s => s.Protected); + int protectedCount = sender.Count(s => s.Protected); - skinItems = skins.ToLive(realm); - skinItems.Insert(protectedCount, random_skin_info); + // For simplicity repopulate the full list. + // In the future we should change this to properly handle ChangeSet events. + dropdownItems.Clear(); + foreach (var skin in sender) + dropdownItems.Add(skin.ToLive(realm)); + dropdownItems.Insert(protectedCount, random_skin_info); - skinDropdown.Items = skinItems; + skinDropdown.Items = dropdownItems; updateSelectedSkinFromConfig(); }