Ensure at least one scale popover axis is active at any time

[bdach]

Spotted in passing when reviewing #30080. The popover would very arbitrarily revert to scaling by Y axis if both checkboxes were checked off.

Not sure how this passed review.

[OliBomby]

I agree this is how it should behave if both axes are turned off.

Maybe we should disable the ability to turn off both axes because i dont think the user ever intends to turn off both axes and make the scale tool useless. It should automatically turn on the other checkbox if you turn off the last checkbox.

[peppy]

I don't get these checkboxes. You uncheck both and it still scales Y now?

Would appreciate if someone can ping me for new UX like this. I think these checkboxes feel really bad / confusing.

[OliBomby]

I don't get these checkboxes. You uncheck both and it still scales Y now?

Would appreciate if someone can ping me for new UX like this. I think these checkboxes feel really bad / confusing.

It's not really new UX. These have been in osu! stable for ages and they're supposed to not scale if both axes are off.

[peppy]

I didn't add those and it's stupid and really shouldn't exist. There are multiple better ways of implementing this which make for a much better UX than what is there. I'd think the standard way of having two sliders for x/y scale and a single checkbox to lock adjustments to both (preserve aspect).

But for now, as you propose, one axis should always be required. ie. this should be a radio control.

[peppy]

As proposed

[bdach]

Would appreciate if someone can ping me for new UX like this

Originally this was added in #28309 which I guess I didn't ping for reviews thinking that obviously it's in stable so it's fine. But then you authored #28559, and reviewed #26311 which touched (and regressed) this. I don't like pulling receipts but that sort of response makes me a little miffed.

I don't go around making UX shotcalls without consultation and that should be pretty apparent from my track record on requesting double checks on such things.

[peppy]

It's completely on me for letting it slip through. But it's definitely the first time I've been actively aware of these checkboxes so please allow me to have a kneejerk reaction to their jank 😄.

The fact it exists on stable is also a surprise to me, but it makes more sense that it was copied across without further consideration.

[peppy]

I propose this:

diff --git a/osu.Game.Rulesets.Osu/Edit/PreciseScalePopover.cs b/osu.Game.Rulesets.Osu/Edit/PreciseScalePopover.cs
index 25515a90d5..71a7793fc9 100644
--- a/osu.Game.Rulesets.Osu/Edit/PreciseScalePopover.cs
+++ b/osu.Game.Rulesets.Osu/Edit/PreciseScalePopover.cs
@@ -136,8 +136,26 @@ protected override void LoadComplete()
             });
             scaleInput.Current.BindValueChanged(scale => scaleInfo.Value = scaleInfo.Value with { Scale = scale.NewValue });
 
-            xCheckBox.Current.BindValueChanged(x => setAxis(x.NewValue, yCheckBox.Current.Value));
-            yCheckBox.Current.BindValueChanged(y => setAxis(xCheckBox.Current.Value, y.NewValue));
+            xCheckBox.Current.BindValueChanged(_ =>
+            {
+                if (!xCheckBox.Current.Value && !yCheckBox.Current.Value)
+                {
+                    yCheckBox.Current.Value = true;
+                    return;
+                }
+
+                updateAxes();
+            });
+            yCheckBox.Current.BindValueChanged(_ =>
+            {
+                if (!xCheckBox.Current.Value && !yCheckBox.Current.Value)
+                {
+                    xCheckBox.Current.Value = true;
+                    return;
+                }
+
+                updateAxes();
+            });
 
             selectionCentreButton.Selected.Disabled = !(scaleHandler.CanScaleX.Value || scaleHandler.CanScaleY.Value);
             playfieldCentreButton.Selected.Disabled = scaleHandler.IsScalingSlider.Value && !selectionCentreButton.Selected.Disabled;
@@ -152,6 +170,12 @@ protected override void LoadComplete()
             });
         }
 
+        private void updateAxes()
+        {
+            scaleInfo.Value = scaleInfo.Value with { XAxis = xCheckBox.Current.Value, YAxis = yCheckBox.Current.Value };
+            updateMaxScale();
+        }
+
         private void updateAxisCheckBoxesEnabled()
         {
             if (scaleInfo.Value.Origin != ScaleOrigin.SelectionCentre)
@@ -234,12 +258,6 @@ private Axes getAdjustAxis(PreciseScaleInfo scale)
 
         private float getRotation(PreciseScaleInfo scale) => scale.Origin == ScaleOrigin.GridCentre ? gridToolbox.GridLinesRotation.Value : 0;
 
-        private void setAxis(bool x, bool y)
-        {
-            scaleInfo.Value = scaleInfo.Value with { XAxis = x, YAxis = y };
-            updateMaxScale();
-        }
-
         protected override void PopIn()
         {
             base.PopIn();

Although looking at this class really hurts my head. I'd probably want to lose all the with syntax ASAP.

[bdach]

I propose this:

Applied verbatim, thanks.