Merge pull request #13404 from peppy/fix-beatmap-skin-disables

Fix `FindProvider` calls on `SkinProvidingContainer` not considering disable flags

osu.Game.Tests/Skins/TestSceneBeatmapSkinLookupDisables.cs

@@ -0,0 +1,116 @@
+// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
+// See the LICENCE file in the repository root for full licence text.
+
+using System;
+using NUnit.Framework;
+using osu.Framework.Allocation;
+using osu.Framework.Audio.Sample;
+using osu.Framework.Bindables;
+using osu.Framework.Graphics;
+using osu.Framework.Graphics.Containers;
+using osu.Framework.Graphics.OpenGL.Textures;
+using osu.Framework.Graphics.Textures;
+using osu.Framework.Testing;
+using osu.Game.Audio;
+using osu.Game.Configuration;
+using osu.Game.Rulesets.Osu;
+using osu.Game.Skinning;
+using osu.Game.Tests.Beatmaps;
+using osu.Game.Tests.Visual;
+
+namespace osu.Game.Tests.Skins
+{
+    [TestFixture]
+    [HeadlessTest]
+    public class TestSceneBeatmapSkinLookupDisables : OsuTestScene
+    {
+        private UserSkinSource userSource;
+        private BeatmapSkinSource beatmapSource;
+        private SkinRequester requester;
+
+        [Resolved]
+        private OsuConfigManager config { get; set; }
+
+        [SetUp]
+        public void SetUp() => Schedule(() =>
+        {
+            Add(new SkinProvidingContainer(userSource = new UserSkinSource())
+                .WithChild(new BeatmapSkinProvidingContainer(beatmapSource = new BeatmapSkinSource())
+                    .WithChild(requester = new SkinRequester())));
+        });
+
+        [TestCase(false)]
+        [TestCase(true)]
+        public void TestDrawableLookup(bool allowBeatmapLookups)
+        {
+            AddStep($"Set beatmap skin enabled to {allowBeatmapLookups}", () => config.SetValue(OsuSetting.BeatmapSkins, allowBeatmapLookups));
+
+            string expected = allowBeatmapLookups ? "beatmap" : "user";
+
+            AddAssert($"Check lookup is from {expected}", () => requester.GetDrawableComponent(new TestSkinComponent())?.Name == expected);
+        }
+
+        [TestCase(false)]
+        [TestCase(true)]
+        public void TestProviderLookup(bool allowBeatmapLookups)
+        {
+            AddStep($"Set beatmap skin enabled to {allowBeatmapLookups}", () => config.SetValue(OsuSetting.BeatmapSkins, allowBeatmapLookups));
+
+            ISkin expected() => allowBeatmapLookups ? (ISkin)beatmapSource : userSource;
+
+            AddAssert("Check lookup is from correct source", () => requester.FindProvider(s => s.GetDrawableComponent(new TestSkinComponent()) != null) == expected());
+        }
+
+        public class UserSkinSource : LegacySkin
+        {
+            public UserSkinSource()
+                : base(new SkinInfo(), null, null, string.Empty)
+            {
+            }
+
+            public override Drawable GetDrawableComponent(ISkinComponent component)
+            {
+                return new Container { Name = "user" };
+            }
+        }
+
+        public class BeatmapSkinSource : LegacyBeatmapSkin
+        {
+            public BeatmapSkinSource()
+                : base(new TestBeatmap(new OsuRuleset().RulesetInfo).BeatmapInfo, null, null)
+            {
+            }
+
+            public override Drawable GetDrawableComponent(ISkinComponent component)
+            {
+                return new Container { Name = "beatmap" };
+            }
+        }
+
+        public class SkinRequester : Drawable, ISkin
+        {
+            private ISkinSource skin;
+
+            [BackgroundDependencyLoader]
+            private void load(ISkinSource skin)
+            {
+                this.skin = skin;
+            }
+
+            public Drawable GetDrawableComponent(ISkinComponent component) => skin.GetDrawableComponent(component);
+
+            public Texture GetTexture(string componentName, WrapMode wrapModeS, WrapMode wrapModeT) => skin.GetTexture(componentName, wrapModeS, wrapModeT);
+
+            public ISample GetSample(ISampleInfo sampleInfo) => skin.GetSample(sampleInfo);
+
+            public IBindable<TValue> GetConfig<TLookup, TValue>(TLookup lookup) => skin.GetConfig<TLookup, TValue>(lookup);
+
+            public ISkin FindProvider(Func<ISkin, bool> lookupFunction) => skin.FindProvider(lookupFunction);
+        }
+
+        private class TestSkinComponent : ISkinComponent
+        {
+            public string LookupName => string.Empty;
+        }
+    }
+}

osu.Game/Skinning/LegacySkinTransformer.cs

@@ -53,8 +53,8 @@ namespace osu.Game.Skinning
 
         public event Action SourceChanged
         {
-            add { throw new NotSupportedException(); }
+            add => Source.SourceChanged += value;
-            remove { }
+            remove => Source.SourceChanged -= value;
         }
     }
 }

osu.Game/Skinning/SkinProvidingContainer.cs

@@ -27,6 +27,8 @@ namespace osu.Game.Skinning
         [CanBeNull]
         private ISkinSource fallbackSource;
 
+        private readonly NoFallbackProxy noFallbackLookupProxy;
+
         protected virtual bool AllowDrawableLookup(ISkinComponent component) => true;
 
         protected virtual bool AllowTextureLookup(string componentName) => true;
@@ -42,6 +44,11 @@ namespace osu.Game.Skinning
             this.skin = skin;
 
             RelativeSizeAxes = Axes.Both;
+
+            noFallbackLookupProxy = new NoFallbackProxy(this);
+
+            if (skin is ISkinSource source)
+                source.SourceChanged += TriggerSourceChanged;
         }
 
         public ISkin FindProvider(Func<ISkin, bool> lookupFunction)
@@ -53,7 +60,8 @@ namespace osu.Game.Skinning
             }
             else if (skin != null)
             {
-                if (lookupFunction(skin))
+                // a proxy must be used here to correctly pass through the "Allow" checks without implicitly falling back to the fallbackSource.
+                if (lookupFunction(noFallbackLookupProxy))
                     return skin;
             }
 
@@ -61,46 +69,70 @@ namespace osu.Game.Skinning
         }
 
         public Drawable GetDrawableComponent(ISkinComponent component)
+            => GetDrawableComponent(component, true);
+
+        public Drawable GetDrawableComponent(ISkinComponent component, bool fallback)
         {
             Drawable sourceDrawable;
             if (AllowDrawableLookup(component) && (sourceDrawable = skin?.GetDrawableComponent(component)) != null)
                 return sourceDrawable;
 
+            if (!fallback)
+                return null;
+
             return fallbackSource?.GetDrawableComponent(component);
         }
 
         public Texture GetTexture(string componentName, WrapMode wrapModeS, WrapMode wrapModeT)
+            => GetTexture(componentName, wrapModeS, wrapModeT, true);
+
+        public Texture GetTexture(string componentName, WrapMode wrapModeS, WrapMode wrapModeT, bool fallback)
         {
             Texture sourceTexture;
             if (AllowTextureLookup(componentName) && (sourceTexture = skin?.GetTexture(componentName, wrapModeS, wrapModeT)) != null)
                 return sourceTexture;
 
+            if (!fallback)
+                return null;
+
             return fallbackSource?.GetTexture(componentName, wrapModeS, wrapModeT);
         }
 
         public ISample GetSample(ISampleInfo sampleInfo)
+            => GetSample(sampleInfo, true);
+
+        public ISample GetSample(ISampleInfo sampleInfo, bool fallback)
         {
             ISample sourceChannel;
             if (AllowSampleLookup(sampleInfo) && (sourceChannel = skin?.GetSample(sampleInfo)) != null)
                 return sourceChannel;
 
+            if (!fallback)
+                return null;
+
             return fallbackSource?.GetSample(sampleInfo);
         }
 
         public IBindable<TValue> GetConfig<TLookup, TValue>(TLookup lookup)
+            => GetConfig<TLookup, TValue>(lookup, true);
+
+        public IBindable<TValue> GetConfig<TLookup, TValue>(TLookup lookup, bool fallback)
         {
             if (skin != null)
             {
                 if (lookup is GlobalSkinColours || lookup is SkinCustomColourLookup)
-                    return lookupWithFallback<TLookup, TValue>(lookup, AllowColourLookup);
+                    return lookupWithFallback<TLookup, TValue>(lookup, AllowColourLookup, fallback);
 
-                return lookupWithFallback<TLookup, TValue>(lookup, AllowConfigurationLookup);
+                return lookupWithFallback<TLookup, TValue>(lookup, AllowConfigurationLookup, fallback);
             }
 
+            if (!fallback)
+                return null;
+
             return fallbackSource?.GetConfig<TLookup, TValue>(lookup);
         }
 
-        private IBindable<TValue> lookupWithFallback<TLookup, TValue>(TLookup lookup, bool canUseSkinLookup)
+        private IBindable<TValue> lookupWithFallback<TLookup, TValue>(TLookup lookup, bool canUseSkinLookup, bool canUseFallback)
         {
             if (canUseSkinLookup)
             {
@@ -109,6 +141,9 @@ namespace osu.Game.Skinning
                     return bindable;
             }
 
+            if (!canUseFallback)
+                return null;
+
             return fallbackSource?.GetConfig<TLookup, TValue>(lookup);
         }
 
@@ -136,6 +171,40 @@ namespace osu.Game.Skinning
 
             if (fallbackSource != null)
                 fallbackSource.SourceChanged -= TriggerSourceChanged;
+
+            if (skin is ISkinSource source)
+                source.SourceChanged -= TriggerSourceChanged;
+        }
+
+        private class NoFallbackProxy : ISkinSource
+        {
+            private readonly SkinProvidingContainer provider;
+
+            public NoFallbackProxy(SkinProvidingContainer provider)
+            {
+                this.provider = provider;
+            }
+
+            public Drawable GetDrawableComponent(ISkinComponent component)
+                => provider.GetDrawableComponent(component, false);
+
+            public Texture GetTexture(string componentName, WrapMode wrapModeS, WrapMode wrapModeT)
+                => provider.GetTexture(componentName, wrapModeS, wrapModeT, false);
+
+            public ISample GetSample(ISampleInfo sampleInfo)
+                => provider.GetSample(sampleInfo, false);
+
+            public IBindable<TValue> GetConfig<TLookup, TValue>(TLookup lookup)
+                => provider.GetConfig<TLookup, TValue>(lookup, false);
+
+            public event Action SourceChanged
+            {
+                add => provider.SourceChanged += value;
+                remove => provider.SourceChanged -= value;
+            }
+
+            public ISkin FindProvider(Func<ISkin, bool> lookupFunction) =>
+                provider.FindProvider(lookupFunction);
         }
     }
 }