Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 43 additions & 12 deletions src/Controls/src/Core/Slider/Slider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,33 +10,64 @@ namespace Microsoft.Maui.Controls
[DebuggerDisplay("{GetDebuggerDisplay(), nq}")]
public partial class Slider : View, ISliderController, IElementConfiguration<Slider>, ISlider
{
// Stores the value that was requested by the user, before clamping
double _requestedValue = 0d;
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The _requestedValue field is initialized to 0d, which matches the default value for the Value property. However, this creates ambiguity: when _userSetValue is false, it's unclear whether the user never set a value, or whether they explicitly set it to 0.

Consider initializing _requestedValue to double.NaN or a nullable double? to distinguish between "never set" and "set to 0". This would make the logic more robust and prevent potential issues where a user sets Value to 0, then changes the range, and the value doesn't restore properly.

Copilot uses AI. Check for mistakes.
// Tracks if the user explicitly set Value (vs it being set by recoercion)
bool _userSetValue = false;
bool _isRecoercing = false;

/// <summary>Bindable property for <see cref="Minimum"/>.</summary>
public static readonly BindableProperty MinimumProperty = BindableProperty.Create(nameof(Minimum), typeof(double), typeof(Slider), 0d, coerceValue: (bindable, value) =>
{
var slider = (Slider)bindable;
slider.Value = slider.Value.Clamp((double)value, slider.Maximum);
return value;
});
public static readonly BindableProperty MinimumProperty = BindableProperty.Create(
nameof(Minimum), typeof(double), typeof(Slider), 0d,
propertyChanged: (bindable, oldValue, newValue) =>
{
var slider = (Slider)bindable;
slider.RecoerceValue();
});

/// <summary>Bindable property for <see cref="Maximum"/>.</summary>
public static readonly BindableProperty MaximumProperty = BindableProperty.Create(nameof(Maximum), typeof(double), typeof(Slider), 1d, coerceValue: (bindable, value) =>
{
var slider = (Slider)bindable;
slider.Value = slider.Value.Clamp(slider.Minimum, (double)value);
return value;
});
public static readonly BindableProperty MaximumProperty = BindableProperty.Create(
nameof(Maximum), typeof(double), typeof(Slider), 1d,
propertyChanged: (bindable, oldValue, newValue) =>
{
var slider = (Slider)bindable;
slider.RecoerceValue();
});

/// <summary>Bindable property for <see cref="Value"/>.</summary>
public static readonly BindableProperty ValueProperty = BindableProperty.Create(nameof(Value), typeof(double), typeof(Slider), 0d, BindingMode.TwoWay, coerceValue: (bindable, value) =>
{
var slider = (Slider)bindable;
// Only store the requested value if the user is setting it (not during recoercion)
if (!slider._isRecoercing)
{
slider._requestedValue = (double)value;
slider._userSetValue = true;
}
Comment on lines +41 to +46
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential logic issue: When a Slider is created with default values, the Value property is initialized to its default (0d) by the BindableProperty system. However, it's unclear whether coerceValue is invoked during this initialization, which would set _userSetValue = true.

If coerceValue is NOT called for default values, then _userSetValue will be false, and the test ValueClampedWhenOnlyRangeChanges() will pass. But this creates inconsistent behavior:

  • A user who explicitly sets slider.Value = 0 gets value preservation
  • A user who relies on the default Value = 0 does not get value preservation

If coerceValue IS called for default values, then _userSetValue will be true, and the test ValueClampedWhenOnlyRangeChanges() at line 171 will fail because line 174 would attempt to restore the "requested" value of 0.

This ambiguity should be clarified and properly tested. Consider adding a test that explicitly verifies the behavior when Value is explicitly set to the default value (0) vs. when it's left at the default.

Copilot uses AI. Check for mistakes.
return ((double)value).Clamp(slider.Minimum, slider.Maximum);
}, propertyChanged: (bindable, oldValue, newValue) =>
{
var slider = (Slider)bindable;
slider.ValueChanged?.Invoke(slider, new ValueChangedEventArgs((double)oldValue, (double)newValue));
});
Comment on lines 38 to 52
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Inconsistency in code formatting: The Slider's MinimumProperty and MaximumProperty declarations use explicit line breaks and parameter alignment (lines 20-26, 29-35), while the ValueProperty declaration remains in a more compact format (line 38).

For consistency and readability, consider reformatting ValueProperty to match the style of MinimumProperty and MaximumProperty:

public static readonly BindableProperty ValueProperty = BindableProperty.Create(
    nameof(Value), typeof(double), typeof(Slider), 0d, BindingMode.TwoWay, 
    coerceValue: (bindable, value) =>
    {
        // ...
    }, 
    propertyChanged: (bindable, oldValue, newValue) =>
    {
        // ...
    });

Copilot uses AI. Check for mistakes.

void RecoerceValue()
{
_isRecoercing = true;
try
{
// If the user explicitly set Value, try to restore the requested value within the new range
if (_userSetValue)
Value = _requestedValue;
else
Value = Value.Clamp(Minimum, Maximum);
}
finally
{
_isRecoercing = false;
}
}
Comment on lines +54 to +69
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Potential concurrency issue: The _isRecoercing flag is not thread-safe. If two threads simultaneously call property setters (e.g., one sets Minimum while another sets Maximum), both could enter RecoerceValue() and potentially corrupt the _requestedValue or create race conditions with the _isRecoercing flag.

While MAUI controls are typically used on the UI thread, the BindableProperty system doesn't inherently enforce this, and users might update properties from background threads (especially during MVVM binding updates).

Consider:

  1. Adding thread-safety using locks or Interlocked operations
  2. Documenting that these properties must only be set from the UI thread
  3. Or accepting the risk if cross-thread access is not a supported scenario for these controls

Copilot uses AI. Check for mistakes.

/// <summary>Bindable property for <see cref="MinimumTrackColor"/>.</summary>
public static readonly BindableProperty MinimumTrackColorProperty = BindableProperty.Create(nameof(MinimumTrackColor), typeof(Color), typeof(Slider), null);

Expand Down
39 changes: 33 additions & 6 deletions src/Controls/src/Core/Stepper/Stepper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,31 +10,41 @@ namespace Microsoft.Maui.Controls
[DebuggerDisplay("{GetDebuggerDisplay(), nq}")]
public partial class Stepper : View, IElementConfiguration<Stepper>, IStepper
{
// Stores the value that was requested by the user, before clamping
double _requestedValue = 0d;
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The _requestedValue field is initialized to 0d, which matches the default value for the Value property. However, this creates ambiguity: when _userSetValue is false, it's unclear whether the user never set a value, or whether they explicitly set it to 0.

Consider initializing _requestedValue to double.NaN or a nullable double? to distinguish between "never set" and "set to 0". This would make the logic more robust and prevent potential issues where a user sets Value to 0, then changes the range, and the value doesn't restore properly.

Copilot uses AI. Check for mistakes.
// Tracks if the user explicitly set Value (vs it being set by recoercion)
bool _userSetValue = false;
bool _isRecoercing = false;

/// <summary>Bindable property for <see cref="Maximum"/>.</summary>
public static readonly BindableProperty MaximumProperty = BindableProperty.Create(nameof(Maximum), typeof(double), typeof(Stepper), 100.0,
validateValue: (bindable, value) => (double)value >= ((Stepper)bindable).Minimum,
coerceValue: (bindable, value) =>
propertyChanged: (bindable, oldValue, newValue) =>
{
var stepper = (Stepper)bindable;
stepper.Value = stepper.Value.Clamp(stepper.Minimum, (double)value);
return value;
stepper.RecoerceValue();
});

/// <summary>Bindable property for <see cref="Minimum"/>.</summary>
public static readonly BindableProperty MinimumProperty = BindableProperty.Create(nameof(Minimum), typeof(double), typeof(Stepper), 0.0,
validateValue: (bindable, value) => (double)value <= ((Stepper)bindable).Maximum,
coerceValue: (bindable, value) =>
propertyChanged: (bindable, oldValue, newValue) =>
{
var stepper = (Stepper)bindable;
stepper.Value = stepper.Value.Clamp((double)value, stepper.Maximum);
return value;
stepper.RecoerceValue();
});

/// <summary>Bindable property for <see cref="Value"/>.</summary>
public static readonly BindableProperty ValueProperty = BindableProperty.Create(nameof(Value), typeof(double), typeof(Stepper), 0.0, BindingMode.TwoWay,
coerceValue: (bindable, value) =>
{
var stepper = (Stepper)bindable;
// Only store the requested value if the user is setting it (not during recoercion)
if (!stepper._isRecoercing)
{
stepper._requestedValue = (double)value;
stepper._userSetValue = true;
}
Comment on lines +42 to +47
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential logic issue: When a Stepper is created with default values, the Value property is initialized to its default (0.0) by the BindableProperty system. However, it's unclear whether coerceValue is invoked during this initialization, which would set _userSetValue = true.

If coerceValue is NOT called for default values, then _userSetValue will be false, and the test ValueClampedWhenOnlyRangeChanges() will pass. But this creates inconsistent behavior:

  • A user who explicitly sets stepper.Value = 0 gets value preservation
  • A user who relies on the default Value = 0 does not get value preservation

If coerceValue IS called for default values, then _userSetValue will be true, and the test ValueClampedWhenOnlyRangeChanges() at line 399 will fail because line 402 would attempt to restore the "requested" value of 0.

This ambiguity should be clarified and properly tested. Consider adding a test that explicitly verifies the behavior when Value is explicitly set to the default value (0) vs. when it's left at the default.

Copilot uses AI. Check for mistakes.
return Math.Round(((double)value), stepper.digits).Clamp(stepper.Minimum, stepper.Maximum);
Comment on lines +42 to 48
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Potential logic inconsistency: The _requestedValue is stored before Math.Round is applied (line 45), but the actual Value is rounded (line 48). This means:

  1. User sets Value = 1.23456 with Increment = 0.1 (digits = 5)
  2. _requestedValue = 1.23456 (unrounded)
  3. Actual Value becomes Math.Round(1.23456, 5).Clamp(...) = 1.23456
  4. Later, Minimum changes and triggers RecoerceValue()
  5. RecoerceValue sets Value = _requestedValue (1.23456)
  6. coerceValue runs again with the SAME value, so it rounds again

This should work correctly, but it's doing unnecessary work. Consider storing the rounded value instead:

double roundedValue = Math.Round((double)value, stepper.digits);
if (!stepper._isRecoercing)
{
    stepper._requestedValue = roundedValue;
    stepper._userSetValue = true;
}
return roundedValue.Clamp(stepper.Minimum, stepper.Maximum);

This avoids redundant rounding operations and makes the logic clearer.

Suggested change
// Only store the requested value if the user is setting it (not during recoercion)
if (!stepper._isRecoercing)
{
stepper._requestedValue = (double)value;
stepper._userSetValue = true;
}
return Math.Round(((double)value), stepper.digits).Clamp(stepper.Minimum, stepper.Maximum);
double roundedValue = Math.Round((double)value, stepper.digits);
// Only store the requested value if the user is setting it (not during recoercion)
if (!stepper._isRecoercing)
{
stepper._requestedValue = roundedValue;
stepper._userSetValue = true;
}
return roundedValue.Clamp(stepper.Minimum, stepper.Maximum);

Copilot uses AI. Check for mistakes.
},
propertyChanged: (bindable, oldValue, newValue) =>
Expand All @@ -43,6 +53,23 @@ public partial class Stepper : View, IElementConfiguration<Stepper>, IStepper
stepper.ValueChanged?.Invoke(stepper, new ValueChangedEventArgs((double)oldValue, (double)newValue));
});

void RecoerceValue()
{
_isRecoercing = true;
try
{
// If the user explicitly set Value, try to restore the requested value within the new range
if (_userSetValue)
Value = _requestedValue;
else
Value = Value.Clamp(Minimum, Maximum);
}
finally
{
_isRecoercing = false;
}
}
Comment on lines +56 to +71
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Potential concurrency issue: The _isRecoercing flag is not thread-safe. If two threads simultaneously call property setters (e.g., one sets Minimum while another sets Maximum), both could enter RecoerceValue() and potentially corrupt the _requestedValue or create race conditions with the _isRecoercing flag.

While MAUI controls are typically used on the UI thread, the BindableProperty system doesn't inherently enforce this, and users might update properties from background threads (especially during MVVM binding updates).

Consider:

  1. Adding thread-safety using locks or Interlocked operations
  2. Documenting that these properties must only be set from the UI thread
  3. Or accepting the risk if cross-thread access is not a supported scenario for these controls

Copilot uses AI. Check for mistakes.

int digits = 4;
//'-log10(increment) + 4' as rounding digits gives us 4 significant decimal digits after the most significant one.
//If your increment uses more than 4 significant digits, you're holding it wrong.
Expand Down
166 changes: 166 additions & 0 deletions src/Controls/tests/Core.UnitTests/SliderUnitTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,172 @@ public void TestConstructor()
Assert.Equal(50, slider.Value);
}

// Tests for setting Min, Max, Value in all 6 possible orders
// Order: Min, Max, Value
[Theory]
[InlineData(10, 100, 50)]
[InlineData(0, 1, 0.5)]
[InlineData(-100, 100, 0)]
[InlineData(50, 150, 100)]
public void SetProperties_MinMaxValue_Order(double min, double max, double value)
{
var slider = new Slider();
slider.Minimum = min;
slider.Maximum = max;
slider.Value = value;

Assert.Equal(min, slider.Minimum);
Assert.Equal(max, slider.Maximum);
Assert.Equal(value, slider.Value);
}

// Order: Min, Value, Max
[Theory]
[InlineData(10, 100, 50)]
[InlineData(0, 1, 0.5)]
[InlineData(-100, 100, 0)]
[InlineData(50, 150, 100)]
public void SetProperties_MinValueMax_Order(double min, double max, double value)
{
var slider = new Slider();
slider.Minimum = min;
slider.Value = value;
slider.Maximum = max;

Assert.Equal(min, slider.Minimum);
Assert.Equal(max, slider.Maximum);
Assert.Equal(value, slider.Value);
}

// Order: Max, Min, Value
[Theory]
[InlineData(10, 100, 50)]
[InlineData(0, 1, 0.5)]
[InlineData(-100, 100, 0)]
[InlineData(50, 150, 100)]
public void SetProperties_MaxMinValue_Order(double min, double max, double value)
{
var slider = new Slider();
slider.Maximum = max;
slider.Minimum = min;
slider.Value = value;

Assert.Equal(min, slider.Minimum);
Assert.Equal(max, slider.Maximum);
Assert.Equal(value, slider.Value);
}

// Order: Max, Value, Min
[Theory]
[InlineData(10, 100, 50)]
[InlineData(0, 1, 0.5)]
[InlineData(-100, 100, 0)]
[InlineData(50, 150, 100)]
public void SetProperties_MaxValueMin_Order(double min, double max, double value)
{
var slider = new Slider();
slider.Maximum = max;
slider.Value = value;
slider.Minimum = min;

Assert.Equal(min, slider.Minimum);
Assert.Equal(max, slider.Maximum);
Assert.Equal(value, slider.Value);
}

// Order: Value, Min, Max
[Theory]
[InlineData(10, 100, 50)]
[InlineData(0, 1, 0.5)]
[InlineData(-100, 100, 0)]
[InlineData(50, 150, 100)]
public void SetProperties_ValueMinMax_Order(double min, double max, double value)
{
var slider = new Slider();
slider.Value = value;
slider.Minimum = min;
slider.Maximum = max;

Assert.Equal(min, slider.Minimum);
Assert.Equal(max, slider.Maximum);
Assert.Equal(value, slider.Value);
}

// Order: Value, Max, Min
[Theory]
[InlineData(10, 100, 50)]
[InlineData(0, 1, 0.5)]
[InlineData(-100, 100, 0)]
[InlineData(50, 150, 100)]
public void SetProperties_ValueMaxMin_Order(double min, double max, double value)
{
var slider = new Slider();
slider.Value = value;
slider.Maximum = max;
slider.Minimum = min;

Assert.Equal(min, slider.Minimum);
Assert.Equal(max, slider.Maximum);
Assert.Equal(value, slider.Value);
}

// Tests that _requestedValue is preserved across multiple recoercions
[Fact]
public void RequestedValuePreservedAcrossMultipleRangeChanges()
{
var slider = new Slider();
slider.Value = 50;
slider.Minimum = -10;
slider.Maximum = -1; // Value clamped to -1

Assert.Equal(-1, slider.Value);

slider.Maximum = -2; // Value should still be clamped, not corrupted

Assert.Equal(-2, slider.Value);

slider.Maximum = 100; // Now the original requested value (50) should be restored

Assert.Equal(50, slider.Value);
}

[Fact]
public void RequestedValuePreservedWhenMinimumChangesMultipleTimes()
{
var slider = new Slider();
slider.Value = 5;
slider.Maximum = 100;
slider.Minimum = 10; // Value clamped to 10

Assert.Equal(10, slider.Value);

slider.Minimum = 20; // Value clamped to 20

Assert.Equal(20, slider.Value);

slider.Minimum = 0; // Original requested value (5) should be restored

Assert.Equal(5, slider.Value);
}

[Fact]
public void ValueClampedWhenOnlyRangeChanges()
{
var slider = new Slider(); // Value defaults to 0
slider.Minimum = 10; // Value should clamp to 10
slider.Maximum = 100;

Assert.Equal(10, slider.Value);

slider.Minimum = 5; // Value stays at 10 because 10 is within [5, 100]

Assert.Equal(10, slider.Value);

slider.Minimum = 15; // Value clamps to 15

Assert.Equal(15, slider.Value);
}
Comment on lines +170 to +186
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test coverage: There's no test for the scenario where a user sets Value multiple times with range changes in between. For example:

  1. Set Value = 50
  2. Set Maximum = 40 (value clamped to 40)
  3. User explicitly sets Value = 30
  4. Set Maximum = 100

The expected behavior is that Value should be 30 (the most recent user-set value), not 50 (the original). The current implementation should handle this correctly because step 3 would update _requestedValue = 30, but this scenario should be explicitly tested to ensure the fix works as intended and to prevent regressions.

Copilot uses AI. Check for mistakes.

[Fact]
public void TestInvalidConstructor()
{
Expand Down
Loading
Loading