Merge pull request #32641 from bdach/skin-menu-checkboxes

Fix skin editor anchor/origin context menu ternary states not updating properly

Author: peppy

osu.Game.Tests/Visual/Gameplay/TestSceneSkinEditor.cs

@@ -15,6 +15,7 @@
 using osu.Framework.Input;
 using osu.Framework.Testing;
 using osu.Game.Database;
+using osu.Game.Graphics.UserInterface;
 using osu.Game.Overlays;
 using osu.Game.Overlays.Settings;
 using osu.Game.Overlays.SkinEditor;
@@ -458,6 +459,62 @@ public void TestMigrationLegacy()
             AddAssert("combo placed in ruleset target", () => rulesetHUDTarget.Components.OfType<LegacyDefaultComboCounter>().Count() == 1);
         }
 
+        [Test]
+        public void TestAnchorRadioButtonBehavior()
+        {
+            ISerialisableDrawable? selectedComponent = null;
+
+            AddStep("Select first component", () =>
+            {
+                var blueprint = skinEditor.ChildrenOfType<SkinBlueprint>().First();
+                skinEditor.SelectedComponents.Clear();
+                skinEditor.SelectedComponents.Add(blueprint.Item);
+                selectedComponent = blueprint.Item;
+            });
+
+            AddStep("Right-click to open context menu", () =>
+            {
+                if (selectedComponent != null)
+                    InputManager.MoveMouseTo(((Drawable)selectedComponent).ScreenSpaceDrawQuad.Centre);
+                InputManager.Click(MouseButton.Right);
+            });
+
+            AddStep("Click on Anchor menu", () =>
+            {
+                InputManager.MoveMouseTo(getMenuItemByText("Anchor"));
+                InputManager.Click(MouseButton.Left);
+            });
+
+            AddStep("Right-click TopLeft anchor", () =>
+            {
+                InputManager.MoveMouseTo(getMenuItemByText("TopLeft"));
+                InputManager.Click(MouseButton.Right);
+            });
+
+            AddAssert("TopLeft item checked", () => (getMenuItemByText("TopLeft").Item as TernaryStateRadioMenuItem)?.State.Value == TernaryState.True);
+
+            AddStep("Right-click Centre anchor", () =>
+            {
+                InputManager.MoveMouseTo(getMenuItemByText("Centre"));
+                InputManager.Click(MouseButton.Right);
+            });
+
+            AddAssert("Centre item checked", () => (getMenuItemByText("Centre").Item as TernaryStateRadioMenuItem)?.State.Value == TernaryState.True);
+            AddAssert("TopLeft item unchecked", () => (getMenuItemByText("TopLeft").Item as TernaryStateRadioMenuItem)?.State.Value == TernaryState.False);
+
+            AddStep("Right-click Closest anchor", () =>
+            {
+                InputManager.MoveMouseTo(getMenuItemByText("Closest"));
+                InputManager.Click(MouseButton.Right);
+            });
+
+            AddAssert("Closest item checked", () => (getMenuItemByText("Closest").Item as TernaryStateRadioMenuItem)?.State.Value == TernaryState.True);
+            AddAssert("Centre item unchecked", () => (getMenuItemByText("Centre").Item as TernaryStateRadioMenuItem)?.State.Value == TernaryState.False);
+
+            Menu.DrawableMenuItem getMenuItemByText(string text)
+                => this.ChildrenOfType<Menu.DrawableMenuItem>().First(m => m.Item.Text.ToString() == text);
+        }
+
         private Skin importSkinFromArchives(string filename)
         {
             var imported = skins.Import(new ImportTask(TestResources.OpenResource($@"Archives/{filename}"), filename)).GetResultSafely();

osu.Game/Overlays/SkinEditor/SkinEditor.cs

@@ -760,11 +760,7 @@ public RevertConfirmDialog(Action revert)
 
         #region Delegation of IEditorChangeHandler
 
-        public event Action? OnStateChange
-        {
-            add => throw new NotImplementedException();
-            remove => throw new NotImplementedException();
-        }
+        public event Action? OnStateChange;
 
         private IEditorChangeHandler? beginChangeHandler;
 
@@ -773,6 +769,9 @@ public void BeginChange()
             // Change handler may change between begin and end, which can cause unbalanced operations.
             // Let's track the one that was used when beginning the change so we can call EndChange on it specifically.
             (beginChangeHandler = changeHandler)?.BeginChange();
+
+            if (beginChangeHandler != null)
+                beginChangeHandler.OnStateChange += OnStateChange;
         }
 
         public void EndChange() => beginChangeHandler?.EndChange();

osu.Game/Overlays/SkinEditor/SkinSelectionHandler.cs

@@ -22,7 +22,10 @@ namespace osu.Game.Overlays.SkinEditor
 {
     public partial class SkinSelectionHandler : SelectionHandler<ISerialisableDrawable>
     {
-        private OsuMenuItem originMenu = null!;
+        private OsuMenuItem? originMenu;
+
+        private TernaryStateRadioMenuItem? closestAnchor;
+        private AnchorMenuItem[]? fixedAnchors;
 
         [Resolved]
         private SkinEditor skinEditor { get; set; } = null!;
@@ -44,6 +47,38 @@ public override SelectionScaleHandler CreateScaleHandler()
             return scaleHandler;
         }
 
+        protected override void LoadComplete()
+        {
+            base.LoadComplete();
+
+            if (ChangeHandler != null)
+                ChangeHandler.OnStateChange += updateTernaryStates;
+            SelectedItems.BindCollectionChanged((_, _) => updateTernaryStates());
+        }
+
+        private void updateTernaryStates()
+        {
+            var usingClosestAnchor = GetStateFromSelection(SelectedBlueprints, c => !c.Item.UsesFixedAnchor);
+
+            if (closestAnchor != null)
+                closestAnchor.State.Value = usingClosestAnchor;
+
+            if (fixedAnchors != null)
+            {
+                foreach (var fixedAnchor in fixedAnchors)
+                    fixedAnchor.State.Value = GetStateFromSelection(SelectedBlueprints, c => c.Item.UsesFixedAnchor && ((Drawable)c.Item).Anchor == fixedAnchor.Anchor);
+            }
+
+            if (originMenu != null)
+            {
+                foreach (var origin in originMenu.Items.OfType<AnchorMenuItem>())
+                {
+                    origin.State.Value = GetStateFromSelection(SelectedBlueprints, c => ((Drawable)c.Item).Origin == origin.Anchor);
+                    origin.Action.Disabled = usingClosestAnchor == TernaryState.True;
+                }
+            }
+        }
+
         public override bool HandleFlip(Direction direction, bool flipOverOrigin)
         {
             var selectionQuad = getSelectionQuad();
@@ -102,27 +137,17 @@ protected override void DeleteItems(IEnumerable<ISerialisableDrawable> items) =>
 
         protected override IEnumerable<MenuItem> GetContextMenuItemsForSelection(IEnumerable<SelectionBlueprint<ISerialisableDrawable>> selection)
         {
-            var closestItem = new TernaryStateRadioMenuItem(SkinEditorStrings.Closest, MenuItemType.Standard, _ => applyClosestAnchors())
-            {
-                State = { Value = GetStateFromSelection(selection, c => !c.Item.UsesFixedAnchor) }
-            };
+            closestAnchor = new TernaryStateRadioMenuItem(SkinEditorStrings.Closest, MenuItemType.Standard, _ => applyClosestAnchors());
+            fixedAnchors = createAnchorItems(applyFixedAnchors).ToArray();
 
             yield return new OsuMenuItem(SkinEditorStrings.Anchor)
             {
-                Items = createAnchorItems((d, a) => d.UsesFixedAnchor && ((Drawable)d).Anchor == a, applyFixedAnchors)
-                        .Prepend(closestItem)
-                        .ToArray()
+                Items = fixedAnchors.Prepend(closestAnchor).ToArray()
             };
 
             yield return originMenu = new OsuMenuItem(SkinEditorStrings.Origin);
 
-            closestItem.State.BindValueChanged(s =>
-            {
-                // For UX simplicity, origin should only be user-editable when "closest" anchor mode is disabled.
-                originMenu.Items = s.NewValue == TernaryState.True
-                    ? Array.Empty<MenuItem>()
-                    : createAnchorItems((d, o) => ((Drawable)d).Origin == o, applyOrigins).ToArray();
-            }, true);
+            originMenu.Items = createAnchorItems(applyOrigins).ToArray();
 
             yield return new OsuMenuItemSpacer();
 
@@ -163,27 +188,37 @@ protected override IEnumerable<MenuItem> GetContextMenuItemsForSelection(IEnumer
             foreach (var item in base.GetContextMenuItemsForSelection(selection))
                 yield return item;
 
-            IEnumerable<TernaryStateMenuItem> createAnchorItems(Func<ISerialisableDrawable, Anchor, bool> checkFunction, Action<Anchor> applyFunction)
+            updateTernaryStates();
+        }
+
+        private IEnumerable<AnchorMenuItem> createAnchorItems(Action<Anchor> applyFunction)
+        {
+            var displayableAnchors = new[]
             {
-                var displayableAnchors = new[]
-                {
-                    Anchor.TopLeft,
-                    Anchor.TopCentre,
-                    Anchor.TopRight,
-                    Anchor.CentreLeft,
-                    Anchor.Centre,
-                    Anchor.CentreRight,
-                    Anchor.BottomLeft,
-                    Anchor.BottomCentre,
-                    Anchor.BottomRight,
-                };
-                return displayableAnchors.Select(a =>
-                {
-                    return new TernaryStateRadioMenuItem(a.ToString(), MenuItemType.Standard, _ => applyFunction(a))
-                    {
-                        State = { Value = GetStateFromSelection(selection, c => checkFunction(c.Item, a)) }
-                    };
-                });
+                Anchor.TopLeft,
+                Anchor.TopCentre,
+                Anchor.TopRight,
+                Anchor.CentreLeft,
+                Anchor.Centre,
+                Anchor.CentreRight,
+                Anchor.BottomLeft,
+                Anchor.BottomCentre,
+                Anchor.BottomRight,
+            };
+            return displayableAnchors.Select(a =>
+            {
+                return new AnchorMenuItem(a, _ => applyFunction(a));
+            });
+        }
+
+        private partial class AnchorMenuItem : TernaryStateRadioMenuItem
+        {
+            public readonly Anchor Anchor;
+
+            public AnchorMenuItem(Anchor anchor, Action<Anchor> applyFunction)
+                : base(anchor.ToString(), MenuItemType.Standard, _ => applyFunction(anchor))
+            {
+                Anchor = anchor;
             }
         }