Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

[Core] Fix Slider Maximum , Minimum binding issue #2765

Closed
wants to merge 1 commit into from

Conversation

myroot
Copy link
Contributor

@myroot myroot commented May 18, 2018

Description of Change

Remove validateValue handler on Maximum/Minimum properties
Use propertyChanged for Maximum/Minimum integrity

Bugs Fixed

API Changes

None

Behavioral Changes

Allow set Minimum value that greater than Maximum value, after Minimum was updated, Maximum value is changed to Max(Minimum, Maximum)

Allow set Maximum value that smaller than Minimum, after Maximum was updated, Minimum value is changed to Min(Minimum, Maximum)

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense

Copy link
Member

@StephaneDelcroix StephaneDelcroix left a comment

Choose a reason for hiding this comment

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

The effects of this might be even more wrong than the current behavior.

<Slider
  Value="{Binding Value}"
  Min="{Binding Min}"
  Max="{Binding Max}" />

with a context of new {Value=-50, Min=-100, Max=-1}

the min and max will be correct, but the value will be coerced at -1

@myroot
Copy link
Contributor Author

myroot commented May 18, 2018

Binding the Value is difficult issue to solve. because It is twoway binding.
But I don't think fixed behavior is more wrong than the current behavior

- Can binding to Maximum/Minimum Can binding to Value Cause crash?
current behavior X X O
fixed behavior O X X

@samhouts samhouts added this to In Review in v3.6.0 May 18, 2018
@charlesroddie
Copy link

Looks like nothing will quite work here. With the way the current properties work, you are bound to have invalid or incorrect states.

You could add a property that is guaranteed to work, SliderValues, which has a type that contains all three floats Min, Max, and Current.

And then leave the existing three properties in a partially fixed state, where they work in more situations than they did before, but not all.

@samhouts samhouts modified the milestone: 2.3.0 Jun 27, 2018
@rmarinho rmarinho requested a review from kingces95 August 7, 2018 12:27
@kingces95 kingces95 assigned kingces95 and unassigned kingces95 Sep 12, 2018
Copy link
Contributor

@kingces95 kingces95 left a comment

Choose a reason for hiding this comment

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

Huh. Seems intractable; XAML doesn't have syntax to declare dependencies between properties to control how they are assigned. So if a dependency exists (e.g. value -> min, value -> max) then there is no way to express it in XAML and honor the dependencies. Yet Windows Phone has Min and Max properties. So maybe I'm wrong and the dependency can be expressed? Wonder why didn't they just have a single property Percentage of type double that varies between 0 and 1? Anyway, @StephaneDelcroix is the expert so if it is intractable, he'll know the least worst solution.

@charlesroddie
Copy link

charlesroddie commented Sep 12, 2018

Here is an idea:

Setting Min, Value, Max would set internal mutable variables reqMin, reqVal, reqMax. Getting Value, Min, Max gets the actual values, which are a function of the these req values. If reqMin ≤ reqVal ≤ reqMax then the actual values are (reqMin,reqVal,reqMax). If not, then the actual values take some other valid numbers, maybe (0,0,1) or (a,b,c) where
a = min(reqMin,reqVal,reqMax), c = max(...) and b = min(max(a,reqVal),c).

This would not prevent temporary glitches in Min, Value, Max (and so as a purist I would still say the correct fix is to have a single float*float*float property instead of three float properties) but it would prevent these glitches from having lasting effects (@StephaneDelcroix 's example) and also from crashing the UI (since the UI always gets valid inputs).

@kingces95 kingces95 self-assigned this Sep 13, 2018
@kingces95 kingces95 removed their assignment Oct 2, 2018
v3.6.0 automation moved this from In Review to Closed Oct 18, 2018
@myroot myroot deleted the fix-slider-min-max branch October 21, 2018 23:22
@samhouts samhouts removed this from Closed in v3.6.0 Feb 2, 2019
@samhouts samhouts added this to In Review in vCurrent (4.8.0) Jun 20, 2020
@samhouts samhouts removed this from In Review in vCurrent (4.8.0) Jul 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants