xamarin / Xamarin.Forms

Xamarin.Forms is no longer supported. Migrate your apps to .NET MAUI.
https://aka.ms/xamarin-upgrade
Other
5.62k stars 1.87k forks source link

Slider crashes when Minimum value greater than zero and set before Max value in XAML #1943

Open Paul-Brenner-Tangoe opened 6 years ago

Paul-Brenner-Tangoe commented 6 years ago

Description

Migrating https://bugzilla.xamarin.com/show_bug.cgi?id=43435 here

The difference between the working and crashing versions is the order of the Minimum and Maximum properties in the XAML. Note that if Minimum is set to zero, the order is no longer an issue.

Steps to Reproduce

Run app, note that it works fine Uncomment line #8 in MainPage.xaml Run app, note it crashes with "System.ArgumentException: Value was an invalid value for Minimum"

Expected Behavior

App does not crash

Actual Behavior

App crashes

Basic Information

Reproduction Link

https://www.dropbox.com/s/qdweqnvh1hsj4wh/slidercrashsample.zip?dl=0

CZEMacLeod commented 6 years ago

I had a similar issue in pure code with something like

Slider s = new Slider() { Minimum = 1000, Maximum = 2000 };

I had to change it to

Slider s = new Slider() { Maximum = 2000, Minimum = 1000 };

I have a feeling that the blocks/errors should not be raised until it is fully initialized. Also what if you want a slider going from 100->0?

sayedihashimi commented 6 years ago

A customer I'm working with has also run into this. He said it took him about an hour to figure out he needed to set Maximum before Minimum. Also he stated that when developing in XAML with UWP there was no such requirement for Maximum to be before Minimum.

charlesroddie commented 6 years ago

The current behavior is absurd. https://docs.microsoft.com/en-us/xamarin/xamarin-forms/user-interface/slider To set (Minimum, Maximum) = (2,3), you have to set maximum first, then minimum. To set (Minimum, Maximum) = (-3,-2), you have to set minimum first, then maximum.

My suggestion (expanded from https://github.com/xamarin/Xamarin.Forms/pull/2765#issuecomment-420835011):

Min, Value, Max are valid when Min <= Value <= Max. A temporary glitch happens when in the process of setting the properties in some order, there is a momentary incorrectness in one value. Temporary glitches are OK but invalid state (this issue) is not since it may crash the UI. Permanent glitches (https://github.com/xamarin/Xamarin.Forms/issues/3521) are also not OK.

Setting Min, Value, Max should set internal mutable variables representing the currently requested values. Getting Value, Min, Max gets a valid function of the these requested values. This would not prevent temporary glitches but does prevent permanent glitches and invalid state.

Glitch-free operation can be done with an extra property containing all three values.

type SliderValues(minimum:float, value:float, maximum:float) =
    member t.Minimum = minimum
    member t.Value = value
    member t.Maximum = maximum
    member t.ForceValid =
        SliderValues(
            Math.Min(Math.Min(minimum,value),maximum),
            Math.Min(Math.Max(minimum,value),maximum),
            Math.Max(Math.Max(minimum,value),maximum)
            )
    member t.IsValid = minimum <= value && value <= maximum

type Slider() =

    let mutable reqValues = SliderValues(0.,0.,1.)
    let mutable validValues = SliderValues(0.,0.,1.)

    /// Moving from old valid values to new valid values, changes UI/binding properties
    /// one at a time, keeping validity at each update.
    let updateSliderValues(newValues:SliderValues) =
        validValues <- newValues
        let o1,o2,o3 = validValues.Minimum, validValues.Value, validValues.Maximum
        let n1,n2,n3 = newValues.Minimum, newValues.Value, newValues.Maximum
        if   n3 >= o3 && n2 >= o2 then () // update UI with 3 then 2 then 1
        elif n3 >= o3 && n2 < o2 then () // 3 then 1 then 2
        elif n1 < o1 && n2 < o2 then () // 1 then 2 then 3
        elif n1 < o1 && n2 >= o2 then () // 1 then 3 then 2
        else // n1 >= o1 && n3 < o3
            () // 2 then 1 and 3

    member t.Minimum
        with get() = validValues.Minimum
        and set v =
            reqValues <- SliderValues(v,reqValues.Value,reqValues.Maximum)
            updateSliderValues(reqValues.ForceValid)
    member t.Value
        with get() = validValues.Value
        and set v =
            reqValues <- SliderValues(reqValues.Minimum,v,reqValues.Maximum)
            updateSliderValues(reqValues.ForceValid)
    member t.Maximum
        with get() = validValues.Maximum
        and set v =
            reqValues <- SliderValues(reqValues.Minimum,reqValues.Value,v)
            updateSliderValues(reqValues.ForceValid)
    // set all values in a glitch free way with proper error handling
    member t.SliderValues
        with get() = validValues
        and set (v:SliderValues) =
            if v.IsValid then
                reqValues <- v
                updateSliderValues(v)
            else failwith "Incorrectly ordered slider values"
charlesroddie commented 5 years ago

I have updated my proposal for clarity. @samhouts @StephaneDelcroix any comments?

jwosty commented 5 years ago

I would even suggest that minimum be allowed to be greater than maximum. It does make sense to have a slider that directly outputs values from, say, 10 to 0, without having to map it myself in an unnecessary step. Honestly, what kinds of bugs is throwing this error even preventing, anyway?

charlesroddie commented 5 years ago

@jwosty I agree somewhat but to do that would require renaming Minimum and Maximum and avoiding usage of native platform controls, i.e. a change in both api and philisophy. Probably a good change, but not a quick fix.

krdmllr commented 5 years ago

I would suggest to simply set the max value to the min value if the min value is greater then the max value and vise versa to avoid the crash when setting up the control.

chucker commented 4 years ago

@krdmllr yes, well, it's not very intuitive that this will throw:

<Slider Grid.Column="1" Grid.Row="1" Minimum="1" Maximum="10" Value="3" />

But this won't:

<Slider Grid.Column="1" Grid.Row="1" Maximum="10" Minimum="1" Value="3" />

I would even argue the former is a more natural way of writing it.

tompi commented 4 years ago

Just wasted some time on this as well(latest forms version)...

thomasgalliker commented 4 years ago

Is there anyone working on this issue? Or is there at least a cheap workaround for this problem?

charlesroddie commented 4 years ago

@thomasgalliker people have been complaining about this for many years but it just gets ignored. Using my method above you can workaround the bugs by creating a safe version of a slider:

/// A slider with SafeMinimum, SafeValue, SafeMaximum properties which allows setting these values without crashing.
type SafeSlider() =
    inherit Slider()

    let mutable reqValues = SliderValues(0.,0.,1.)
    let mutable validValues = SliderValues(0.,0.,1.)

    /// Moving from old valid values to new valid values, changes UI/binding properties
    /// one at a time, keeping validity at each update.
    let updateSliderValues(newValues:SliderValues, s:Slider) =
        let o1,o2,o3 = validValues.Minimum, validValues.Value, validValues.Maximum
        validValues <- newValues
        let n1,n2,n3 = newValues.Minimum, newValues.Value, newValues.Maximum
        let set1() = s.Minimum <- n1
        let set2() = s.Value <- n2
        let set3() = s.Maximum <- n3
        if   n3 >= o3 && n2 >= o2 then set3(); set2(); set1()
        elif n3 >= o3 && n2 < o2 then set3(); set1(); set2()
        elif n1 < o1 && n2 < o2 then set1(); set2(); set3()
        elif n1 < o1 && n2 >= o2 then set1(); set3(); set2()
        else // n1 >= o1 && n3 < o3
            set2(); set1(); set3()

    member t.SafeMinimum
        with get() = validValues.Minimum
        and set v =
            reqValues <- SliderValues(v,reqValues.Value,reqValues.Maximum)
            updateSliderValues(reqValues.ForceValid, t)
    member t.SafeValue
        with get() = validValues.Value
        and set v =
            reqValues <- SliderValues(reqValues.Minimum,v,reqValues.Maximum)
            updateSliderValues(reqValues.ForceValid, t)
    member t.SafeMaximum
        with get() = validValues.Maximum
        and set v =
            reqValues <- SliderValues(reqValues.Minimum,reqValues.Value,v)
            updateSliderValues(reqValues.ForceValid, t)
    /// Set all values in a glitch free way with proper error handling
    member t.SliderValues
        with get() = validValues
        and set (v:SliderValues) =
            if v.IsValid then
                reqValues <- v
                updateSliderValues(v, t)
            else failwith "Incorrectly ordered slider values"
janusw commented 3 years ago

I also got bitten by this. Is there any way this will get fixed at some point? It's a pretty basic bug and it should be rather easy to fix this, I guess.

(Also the issue deserves a label "a/slider" at least, so that it can be found easier.)

madaraUchiha3094 commented 2 years ago

Issue still seen in Xamarin 5.0.0.2083. System.ArgumentException: Value was an invalid value for Minimum Parameter name: value

Tried setting the max value before minimum in xaml as suggested. Still not working