maxDistance and minDistance

[finalevil]

• main change
  add the minDistance and maxDistance to keep the distance between two point.
• other change
  • move the minLabel, maxLabel to public (I think some people need to change the font or size etc.)
  • move the leftHandleSelected, rightHandleSelected to public (need the property to check the value change by user's touch or not)
  • move the leftHandle, rightHandle to public

[TomThorpe]

Hey! Thanks a lot for helping out and submitting the PR, but I think I can't merge it at the moment.

There's a lot of duplication in these two blocks, and also I feel like this way of ensuring the min or max difference properties are kept doesn't fit in with the method. If you check out the lines at the start of the refresh method, this is how we make sure the slider doesn't go off the end of the scale, by accessing the property values directly we make sure that the refresh method isn't called again.

Also we make sure the changes are made before the delegate is updated. I think with how this PR is at the moment, any delegates will be updated with the wrong values first, and then the whole refresh method will be called again. I'm also not completely sure I understand the whole if((self.minDistance - diff) < 0.001) offset thing?

If this could maybe be tidied up though to be in the same pattern as the minValue and maxValue stuff at the start of the refresh method, and remove as much duplication as possible, I would be happy to accept and merge it :-)

thanks again!

[TomThorpe]

Hey!

Thank a lot for your PR. I do genuinely really appreciate PRs and people helping out! I've added a comment with some thoughts. I think the min and max value properties is a great idea.

However I'm not too sure about making the other properties public though. I think it would be better if people need to set fonts or selected values etc. to expose methods to do this work rather than the actual properties themselves?

[finalevil]

ok, thank you for your review. I will modify code and pull request again : )

[finalevil]

Hi, I did some change about property and refresh method.
But the duplication in these two blocks, I am not sure how to reduce it.
Do you have any suggestion?

[TomThorpe]

Hey!

Thanks for looking at this again.

Can I suggest that maybe we ignore the extra properties for now? Theres a few potential issues I think I can spot by looking at the code (although admittedly I haven't run it yet) regarding for example

• minLabelText and maxLabelText properties, if we set those they will get overwritten when the slider position changes.
• leftHandleFrame and rightHandleFrame should be readonly seeing as setting them does nothing, this could be confusing to people. I'm also not sure what need people would have for getting the actual frame of the left and right handle?
• I like the idea of being able to change the font! Although I'm not sure why you need the bridge when setting it? (this might be my ignorance, sorry if so!). Also maybe it would be better to have those as a single property, I'm not sure why people would need to have different fonts for each label?

Sorry if I'm being overly critical but I want to make sure things are right before doing any merges that could break or complicate things. Thats why I think it might be best to leave those out for now, and concentrate on just the max and min distance properties? Then we could add the other ones later.

Also there's a few times where I think your IDE has added spaces on blank lines (like 154, 189 etc.) Would you mind taking them out so it cleans up the diff?

Regarding the code for min and max distance, what you wrote is pretty much right really. I just thought you could move the diff calculation out of the conditionals, and I'm not sure you need the offset stuff? Although correct me if I'm wrong! Something like this (which still has some duplication but its definitely a balance for readability :D) :

 float diff = self.selectedMaximum - self.selectedMinimum;
    if (minDiff > 0 && diff < minDiff){
        if (self.leftHandleSelected){
            _selectedMinimum = self.selectedMaximum - minDiff;
        } else {
            _selectedMaximum = self.selectedMinimum + minDiff;
        }
    } else if (maxDiff > 0 && diff > maxDiff){
        if (self.leftHandleSelected){
            _selectedMinimum = self.selectedMaximum - maxDiff;
        } else {
            _selectedMaximum = self.selectedMinimum + maxDiff;
        }
    }

Sorry again for being picky and thanks for your work!

[finalevil]

Hi,

It's ok. Thank you for your review. And I worry that waste your time to review. :-)

If drag the right handle and over the maxDistance.
In your code, the right handle will stop and can't to move. You need move the left handle first.
In my code, the left handle will shift to keep the maxDistance.
I'm not sure which one is better for user experience. What do you think?

As you say, focus on the minDistance and maxDistance.
So I have remove the other code, keep the minDistance and maxDistance.

[TomThorpe]

Ah, I see. Yeah I didn't think about moving the other slider to maintain the distance. That's a pretty good idea!

Ok this looks good now :-) I'll merge it! Thanks again!

[finalevil]

Hi,
So what is your choice about minDistacne and maxDistance code? if you need, I can change the code again.

And I forgot one thing. You are right that people wouldn't need to have different fonts. I will add the code to change font for two label. Thank you : )

[TomThorpe]

It's up to you, you're welcome to change the code again if you have the time :-) Just submit another PR