-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Plot widget - allow disabling zoom and drag for x and y separately #2901
Conversation
@@ -402,9 +402,19 @@ impl Plot { | |||
self | |||
} | |||
|
|||
/// Whether to allow zooming in the plot. Default: `true`. | |||
pub fn allow_zoom(mut self, on: bool) -> Self { |
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.
It would be very easy to maintain backwards compatibility here by keeping these methods and having them set both x and y at the same time.
Question to the reviewers: what's the preference - keep backwards compatibility here or go with a more minimal API?
I'll update the changelog once this is decided.
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.
I vote keeping the old one. Not just for backwards compatibility, but also ergonomics: in most cases, you want to control both the same
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.
Yeah, good point. It occurs to me now that having it be generic in T
where T : Into<AxisBools>
would produce the best of both worlds. That way there's only the one method needed, and it maintains the current ergonomics for setting both at once, while allowing the new flexibility.
The main cost I see there is that it requires exposing AxisBools, which has been private until now. I guess it will also be a little more verbose to disable just one of the axes, as you'll have to do something like .allow_zoom(AxisBools::new(false, true))
(unless we keep the allow_zoom_x/y
methods, which doesn't seem worth it). I'll go ahead and update with the new signature, and probably drop the x/y-specific methods.
The referenced issue #1164 wasn't exactly asking for this - should I open a separate issue asking for this exactly? |
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.
Looks good, but please reintroduce the old functions as a simplified interface
@@ -402,9 +402,19 @@ impl Plot { | |||
self | |||
} | |||
|
|||
/// Whether to allow zooming in the plot. Default: `true`. | |||
pub fn allow_zoom(mut self, on: bool) -> Self { |
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.
I vote keeping the old one. Not just for backwards compatibility, but also ergonomics: in most cases, you want to control both the same
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
Currently,
allow_zoom
andallow_drag
apply to both axes at the same time. This PR makes it possible to specify whether zooming and dragging is allowed per axis. This helps in a scenario where the range of one axis is fixed for the whole range-- like a sinusoid. In that case, it's nice if zooming and dragging is only possible on the X axis.This partially addresses #1164, but isn't exactly what was asked for there.
Egui.Plot.zoom.drag.demo.webm