-
Notifications
You must be signed in to change notification settings - Fork 566
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
widget: slider: Add stepping functionality #1875
Conversation
5679c05
to
a6bbf2b
Compare
What should a |
No step at all is reasonable. It makes logical sense to me, you have infinite steps. Maybe its more rust-like use an option and disallow |
This is good to go from my point of view now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- What will/should happen if initial value is not aligned with the step?
- Should we show the available points(as little dots) on the slider?
Huh, the stepping is 10. I can see there being debate on of that should be in the form of With this rounding formula a stepping if 11 would create the same result as with 10. |
because self.max was used in calculation. and self.max / step is fractional |
different implementation treats max as a separate step if it is not "well aligned"(min + n * step) |
e86bf22
to
5ddbd6c
Compare
A new builder method `with_step(f64)` is added to set the stepping value. Signed-off-by: Christopher N. Hesse <[email protected]>
In case of max % step != 0, the last step is smaller than the others. Signed-off-by: Christopher N. Hesse <[email protected]>
Handle the edge case where we need an extra step separately. This occurs whenever max % step != 0. Signed-off-by: Christopher N. Hesse <[email protected]>
We now handle that case and allow for self.max to be reached. Signed-off-by: Christopher N. Hesse <[email protected]>
Signed-off-by: Christopher N. Hesse <[email protected]>
Signed-off-by: Christopher N. Hesse <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@cmyr could you please hit the big scary button? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me! As always this stuff is trickier than expected, but I think it's also fair for us to expect the user to provide sane values.
if step < 0.0 { | ||
warn!("bad stepping (must be positive): {}", step); | ||
return self; | ||
} | ||
self.step = if step > 0.0 { | ||
Some(step) | ||
} else { | ||
// A stepping value of 0.0 would yield an infinite amount of steps. | ||
// Enforce no stepping instead. | ||
None | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to change it, but just for posterity I'd have just written:
if step < 0.0 { | |
warn!("bad stepping (must be positive): {}", step); | |
return self; | |
} | |
self.step = if step > 0.0 { | |
Some(step) | |
} else { | |
// A stepping value of 0.0 would yield an infinite amount of steps. | |
// Enforce no stepping instead. | |
None | |
}; | |
if step < 0.0 { | |
warn!("bad stepping (must be positive): {}", step); | |
} else { | |
self.step = Some(step); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That has a drawback IMO: you'd actually need to check for step <= 0
since otherwise we would divide by zero elsewhere iirc.
We decided to allow 0
as a valid step value though, indicating smooth stepping (no intervals) - but we have to explicitly model this as None
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets change that step > 0.0
to step != 0.0
to make it more explicit
A new builder method
with_step(f64)
is added to set the stepping value.Signed-off-by: Christopher N. Hesse [email protected]