From 7d6c83b37f820550d12a4aef2fcbbea32f2e1015 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Tue, 17 Sep 2024 14:44:20 +0200 Subject: [PATCH] Fix `DragValue` range clamping (#5118) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When using a `DragValue`, there are three common modes of range clamping that the user may want: A) no clamping B) clamping only user input (dragging or editing text), but leave existing value intact C) always clamp The difference between mode B and C is: ```rs let mut x = 42.0; ui.add(DragValue::new(&mut x).range(0.0..=1.0)); // What will `x` be here? ``` With this PR, we now can get the three behaviors with: * A): don't call `.range()` (or use `-Inf..=Inf`) * B) call `.range()` and `.clamp_existing_to_range(false)` * C) call `.range()` ## Slider clamping Slider clamping is slightly different, since a slider always has a range. For a slider, there are these three cases to consider: A) no clamping B) clamp any value that the user enters, but leave existing values intact C) always clamp all values Out of this, C should probably be the default. I'm not sure what the best API is for this yet. Maybe an `enum` 🤔 I'll take a pass on that in a future PR. ## Related * https://github.com/emilk/egui/pull/4728 * https://github.com/emilk/egui/issues/4881 * https://github.com/emilk/egui/pull/4882 --- crates/egui/src/widgets/drag_value.rs | 80 ++++++++++++++++++++------- crates/egui/src/widgets/slider.rs | 2 +- 2 files changed, 60 insertions(+), 22 deletions(-) diff --git a/crates/egui/src/widgets/drag_value.rs b/crates/egui/src/widgets/drag_value.rs index 5391c72d..2ae3585f 100644 --- a/crates/egui/src/widgets/drag_value.rs +++ b/crates/egui/src/widgets/drag_value.rs @@ -41,7 +41,7 @@ pub struct DragValue<'a> { prefix: String, suffix: String, range: RangeInclusive, - clamp_to_range: bool, + clamp_existing_to_range: bool, min_decimals: usize, max_decimals: Option, custom_formatter: Option>, @@ -72,7 +72,7 @@ impl<'a> DragValue<'a> { prefix: Default::default(), suffix: Default::default(), range: f64::NEG_INFINITY..=f64::INFINITY, - clamp_to_range: true, + clamp_existing_to_range: true, min_decimals: 0, max_decimals: None, custom_formatter: None, @@ -93,35 +93,75 @@ impl<'a> DragValue<'a> { /// Sets valid range for the value. /// /// By default all values are clamped to this range, even when not interacted with. - /// You can change this behavior by passing `false` to [`crate::Slider::clamp_to_range`]. + /// You can change this behavior by passing `false` to [`Self::clamp_existing_to_range`]. #[deprecated = "Use `range` instead"] #[inline] - pub fn clamp_range(mut self, range: RangeInclusive) -> Self { - self.range = range.start().to_f64()..=range.end().to_f64(); - self + pub fn clamp_range(self, range: RangeInclusive) -> Self { + self.range(range) } /// Sets valid range for dragging the value. /// /// By default all values are clamped to this range, even when not interacted with. - /// You can change this behavior by passing `false` to [`crate::Slider::clamp_to_range`]. + /// You can change this behavior by passing `false` to [`Self::clamp_existing_to_range`]. #[inline] pub fn range(mut self, range: RangeInclusive) -> Self { self.range = range.start().to_f64()..=range.end().to_f64(); self } - /// If set to `true`, all incoming and outgoing values will be clamped to the sliding [`Self::range`] (if any). + /// If set to `true`, existing values will be clamped to [`Self::range`]. /// - /// If set to `false`, a value outside of the range that is set programmatically or by user input will not be changed. - /// Dragging will be restricted to the range regardless of this setting. - /// Default: `true`. + /// If `false`, only values entered by the user (via dragging or text editing) + /// will be clamped to the range. + /// + /// ### Without calling `range` + /// ``` + /// # egui::__run_test_ui(|ui| { + /// let mut my_value: f32 = 1337.0; + /// ui.add(egui::DragValue::new(&mut my_value)); + /// assert_eq!(my_value, 1337.0, "No range, no clamp"); + /// # }); + /// ``` + /// + /// ### With `.clamp_existing_to_range(true)` (default) + /// ``` + /// # egui::__run_test_ui(|ui| { + /// let mut my_value: f32 = 1337.0; + /// ui.add(egui::DragValue::new(&mut my_value).range(0.0..=1.0)); + /// assert!(0.0 <= my_value && my_value <= 1.0, "Existing values should be clamped"); + /// # }); + /// ``` + /// + /// ### With `.clamp_existing_to_range(false)` + /// ``` + /// # egui::__run_test_ui(|ui| { + /// let mut my_value: f32 = 1337.0; + /// let response = ui.add( + /// egui::DragValue::new(&mut my_value).range(0.0..=1.0) + /// .clamp_existing_to_range(false) + /// ); + /// if response.dragged() { + /// // The user edited the value, so it should be clamped to the range + /// assert!(0.0 <= my_value && my_value <= 1.0); + /// } else { + /// // The user didn't edit, so our original value should still be here: + /// assert_eq!(my_value, 1337.0); + /// } + /// # }); + /// ``` #[inline] - pub fn clamp_to_range(mut self, clamp_to_range: bool) -> Self { - self.clamp_to_range = clamp_to_range; + pub fn clamp_existing_to_range(mut self, clamp_existing_to_range: bool) -> Self { + self.clamp_existing_to_range = clamp_existing_to_range; self } + #[inline] + #[deprecated = "Renamed clamp_existing_to_range"] + pub fn clamp_to_range(self, clamp_to_range: bool) -> Self { + self.clamp_existing_to_range(clamp_to_range) + } + /// Show a prefix before the number, e.g. "x: " #[inline] pub fn prefix(mut self, prefix: impl ToString) -> Self { @@ -392,7 +432,7 @@ impl<'a> Widget for DragValue<'a> { mut get_set_value, speed, range, - clamp_to_range, + clamp_existing_to_range, prefix, suffix, min_decimals, @@ -470,7 +510,7 @@ impl<'a> Widget for DragValue<'a> { }); } - if clamp_to_range { + if clamp_existing_to_range { value = clamp_value_to_range(value, range.clone()); } @@ -501,9 +541,8 @@ impl<'a> Widget for DragValue<'a> { // Make sure we applied the last text value: let parsed_value = parse(&custom_parser, &value_text); if let Some(mut parsed_value) = parsed_value { - if clamp_to_range { - parsed_value = clamp_value_to_range(parsed_value, range.clone()); - } + // User edits always clamps: + parsed_value = clamp_value_to_range(parsed_value, range.clone()); set(&mut get_set_value, parsed_value); } } @@ -537,9 +576,8 @@ impl<'a> Widget for DragValue<'a> { if update { let parsed_value = parse(&custom_parser, &value_text); if let Some(mut parsed_value) = parsed_value { - if clamp_to_range { - parsed_value = clamp_value_to_range(parsed_value, range.clone()); - } + // User edits always clamps: + parsed_value = clamp_value_to_range(parsed_value, range.clone()); set(&mut get_set_value, parsed_value); } } diff --git a/crates/egui/src/widgets/slider.rs b/crates/egui/src/widgets/slider.rs index 91aa4f1b..f06d5924 100644 --- a/crates/egui/src/widgets/slider.rs +++ b/crates/egui/src/widgets/slider.rs @@ -822,7 +822,7 @@ impl<'a> Slider<'a> { let mut dv = DragValue::new(&mut value) .speed(speed) .range(self.range.clone()) - .clamp_to_range(self.clamp_to_range) + .clamp_existing_to_range(self.clamp_to_range) .min_decimals(self.min_decimals) .max_decimals_opt(self.max_decimals) .suffix(self.suffix.clone())