Skip to content
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

Merged
merged 9 commits into from
Jul 27, 2021
Merged

Conversation

raymanfx
Copy link
Collaborator

A new builder method with_step(f64) is added to set the stepping value.

Signed-off-by: Christopher N. Hesse [email protected]

druid/src/widget/slider.rs Outdated Show resolved Hide resolved
druid/src/widget/slider.rs Outdated Show resolved Hide resolved
@raymanfx raymanfx force-pushed the master branch 2 times, most recently from 5679c05 to a6bbf2b Compare July 16, 2021 10:57
@raymanfx
Copy link
Collaborator Author

What should a 0.0 step value represent? Smooth (no step at all) or a single step aka binary min/max range?

@JAicewizard
Copy link
Contributor

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 0.0, but I am the wrong person to ask that.

@raymanfx
Copy link
Collaborator Author

This is good to go from my point of view now.
Paging @cmyr since he responded to my Zulip topic a few days ago.

Copy link
Collaborator

@maan2003 maan2003 left a 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?

druid/src/widget/slider.rs Outdated Show resolved Hide resolved
druid/src/widget/slider.rs Show resolved Hide resolved
druid/src/widget/slider.rs Outdated Show resolved Hide resolved
@maan2003 maan2003 added the S-waiting-on-author waits for changes from the submitter label Jul 22, 2021
@JAicewizard
Copy link
Contributor

Huh, the stepping is 10. I can see there being debate on of that should be in the form of min+n*stepping, but I'm confused how a stepping of 10 should create fractional results. (when min isn't a fraction)

With this rounding formula a stepping if 11 would create the same result as with 10.

@maan2003
Copy link
Collaborator

I'm confused how a stepping of 10 should create fractional results. (when min isn't a fraction)

because self.max was used in calculation. and self.max / step is fractional

@maan2003
Copy link
Collaborator

different implementation treats max as a separate step if it is not "well aligned"(min + n * step)

@maan2003 maan2003 removed the S-waiting-on-author waits for changes from the submitter label Jul 23, 2021
@raymanfx raymanfx force-pushed the master branch 3 times, most recently from e86bf22 to 5ddbd6c Compare July 25, 2021 15:42
raymanfx and others added 6 commits July 25, 2021 18:14
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]>
druid/src/widget/slider.rs Outdated Show resolved Hide resolved
druid/src/widget/slider.rs Show resolved Hide resolved
Signed-off-by: Christopher N. Hesse <[email protected]>
Copy link
Collaborator

@maan2003 maan2003 left a comment

Choose a reason for hiding this comment

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

Thanks!

@raymanfx
Copy link
Collaborator Author

@cmyr could you please hit the big scary button? :)

Copy link
Member

@cmyr cmyr left a 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.

Comment on lines +66 to +76
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
};
Copy link
Member

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:

Suggested change
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);
}

Copy link
Collaborator Author

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.

Copy link
Collaborator

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

@maan2003 maan2003 merged commit ac44a3a into linebender:master Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants