feat: add slider widget with horizontal and vertical support #853
Conversation
- Implement Slider widget with customizable colors, step size, and radii - Add horizontal and vertical axis support with proper coordinate handling - Include hover effects and accessibility features - Add comprehensive tests for interaction and rendering - Integrate slider into widget gallery example with direction switching - Add documentation and example usage - Fix edge cases for min/max values and step adjustments
|
|
||
| #[must_use] | ||
| /// Set the callback for editing state changes. | ||
| pub fn on_editing_changed( |
There was a problem hiding this comment.
I was hoping to see some use case for this.
Also "editing state changes" is extremely vague terminology.
- Move slider-related constants from Slider impl to theme module - Change color fields from Color to Brush type for better flexibility - Rename color-related methods to thumb_color for clarity - Remove macro-based property handling in favor of explicit updates
There was a problem hiding this comment.
I'm happy to approve the Xilem side. I would still like some documentation for why you'd set on_editing_changed, but I won't block on it.
@PoignardAzur can you please review the Masonry/widget code?
There was a problem hiding this comment.
The code calculating the slider values and the tests checking that code are pretty intense, and I didn't inspect them in detail. I think it may be possible to write a simpler version of that code, but I won't block on that.
The Widget code itself seems mostly fine and idiomatic. I'm fine with merging it once the problems I've pointed out are fixed.
| // Copyright 2023 the Xilem Authors and the Druid Authors | ||
| // SPDX-License-Identifier: Apache-2.0 |
There was a problem hiding this comment.
| // Copyright 2023 the Xilem Authors and the Druid Authors | |
| // SPDX-License-Identifier: Apache-2.0 | |
| // Copyright 2025 the Xilem Authors | |
| // SPDX-License-Identifier: Apache-2.0 |
| use vello::{ | ||
| kurbo::{Affine, Point, Rect, RoundedRect, RoundedRectRadii, Shape as _, Size}, | ||
| peniko::{Brush, Color}, | ||
| Scene, | ||
| }; | ||
|
|
||
| use crate::{ | ||
| core::{ | ||
| AccessCtx, AccessEvent, Action, BoxConstraints, EventCtx, LayoutCtx, PaintCtx, | ||
| PointerButton, PointerEvent, QueryCtx, RegisterCtx, TextEvent, Update, UpdateCtx, Widget, | ||
| WidgetId, WidgetMut, | ||
| }, | ||
| theme, | ||
| }; | ||
|
|
||
| use super::Axis; |
There was a problem hiding this comment.
Nitpick: I'd rather this code follow the import conventions of other files. That is: no nested group imports, and no super:: imports.
| pub fn set_value(this: &mut WidgetMut<'_, Self>, value: f64) { | ||
| this.widget.value = value.clamp(this.widget.min, this.widget.max); | ||
| this.ctx.request_paint_only(); | ||
| } | ||
|
|
||
| /// Sets the slider's minimum value. | ||
| pub fn set_min(this: &mut WidgetMut<'_, Self>, min: f64) { | ||
| this.widget.min = min; | ||
| this.widget.value = this.widget.value.clamp(this.widget.min, this.widget.max); | ||
| this.ctx.request_paint_only(); | ||
| } | ||
|
|
||
| /// Sets the slider's maximum value. | ||
| pub fn set_max(this: &mut WidgetMut<'_, Self>, max: f64) { | ||
| this.widget.max = max; | ||
| this.widget.value = this.widget.value.clamp(this.widget.min, this.widget.max); | ||
| this.ctx.request_paint_only(); | ||
| } |
There was a problem hiding this comment.
These three setters affect the accessibility node, and should therefore call request_render(). Not sure about the others.
| self.is_dragging = true; | ||
| self.editing = true; |
There was a problem hiding this comment.
Is there any difference between is_dragging and is_editing? They seem to be updated in tandem.
|
|
||
| fn accessibility(&mut self, _ctx: &mut AccessCtx, node: &mut Node) { | ||
| node.set_value(self.value.to_string()); | ||
| node.add_action(accesskit::Action::SetValue); |
There was a problem hiding this comment.
This action needs to be processed in on_access_event.
| if ctx.is_disabled() { | ||
| CursorIcon::Wait | ||
| } else if self.is_dragging { | ||
| CursorIcon::Grabbing | ||
| } else if self.is_hovered { | ||
| CursorIcon::Grab | ||
| } else { | ||
| CursorIcon::Default | ||
| } |
There was a problem hiding this comment.
IIRC disabled widgets don't affect the cursor.
Also, if your get_cursor methods can return different values based on status, you should call request_cursor_icon_change when the status changes. (Otherwise, you may have cases where the icon doesn't change until the mouse moves.)
EDIT: Though honestly, we already trigger a pointer pass in most relevant cases, and we could generalize it to all pointer events. So I guess it's fine if you ignore this.
| #[test] | ||
| fn vertical_slider() { | ||
| let [slider_id] = widget_ids(); | ||
| let slider = Slider::new(Axis::Vertical, 0.0, 100.0, 75.0).with_id(slider_id); | ||
|
|
||
| let mut harness = TestHarness::create_with_size(slider, Size::new(40.0, 200.0)); | ||
| assert_render_snapshot!(harness, "vertical_slider"); | ||
| } |
There was a problem hiding this comment.
It's great that you added lots of screenshot tests, but the screenshots haven't been added to source control.
There is still an issue where my tests are not outputting images for some reason. Can anyone help me with this?