mirror of
https://github.com/storytold/lightcraft.git
synced 2026-10-24 10:09:01 -06:00
Merge PR #1402: Fix slider wheel edits after leaving and returning to the Edit panel
crates/ui-egui/src/widgets/tests_numeric_entry.rs: keep both sides' tests. docs/parity-checklist.md: newer header (2026-10-21); KEY-SLIDER-NUDGE from #1402, KEY-SLIDER-TYPE from #1687; both revision rows.
This commit is contained in:
@@ -1631,6 +1631,36 @@ fn a_selected_develop_slider_accepts_wheel_nudges() {
|
||||
}
|
||||
let after = lightcraft_develop::controls::get(&develop(&h), "light.exposure").expect("exposure");
|
||||
assert!(after > before, "selected exposure did not increase: {before} -> {after}");
|
||||
for _ in 0..40 {
|
||||
h.step();
|
||||
}
|
||||
let before_leaving = develop(&h).light.exposure;
|
||||
// Leaving the row disarms wheel edits, but must not take keyboard arrows away.
|
||||
h.request("ui.move", json!({"x": panel.left() - 10.0, "y": track.center().y}), T);
|
||||
h.request("ui.hoverWidget", json!({"id": name}), T);
|
||||
// More than a full notch: disarming dropped the wheel remainder, so this alone would edit it.
|
||||
h.request("ui.scroll", json!({"dy": -240.0}), T);
|
||||
for _ in 0..40 {
|
||||
h.step();
|
||||
}
|
||||
assert_eq!(develop(&h).light.exposure, before_leaving, "returning to a slider while scrolling must not edit it (issue #1355)");
|
||||
h.request("ui.move", json!({"x": panel.left() - 10.0, "y": track.center().y}), T);
|
||||
h.request("ui.key", json!({"key": "ArrowUp"}), T);
|
||||
assert!(develop(&h).light.exposure > before_leaving, "keyboard adjustment survives leaving the slider");
|
||||
|
||||
// Scroll back to the top, then explicitly select it again to re-enable wheel nudges.
|
||||
h.request("ui.move", json!({"x": panel.right() - 8.0, "y": panel.center().y}), T);
|
||||
h.request("ui.scroll", json!({"dy": 600.0}), T);
|
||||
for _ in 0..40 {
|
||||
h.step();
|
||||
}
|
||||
click(&mut h, name);
|
||||
let before_reselecting = develop(&h).light.exposure;
|
||||
h.request("ui.scroll", json!({"dy": 120.0}), T);
|
||||
for _ in 0..40 {
|
||||
h.step();
|
||||
}
|
||||
assert!(develop(&h).light.exposure > before_reselecting, "a new click re-enables wheel adjustment");
|
||||
}
|
||||
|
||||
/// Scrolling sideways over a slider adjusts it without selecting it first: a trackpad swipe
|
||||
|
||||
@@ -255,16 +255,45 @@ pub fn slider(ui: &mut Ui, spec: &ControlSpec, value: f64, enabled: bool, label_
|
||||
let track_rect = Rect::from_min_max(pos2(row.left() + pad_l, row.top() + 22.0), pos2(row.right() - pad_r, row.top() + 40.0));
|
||||
let id = ui.id().with(spec.id);
|
||||
let resp = ui.interact(track_rect.expand2(vec2(8.0, 2.0)), id, if enabled { Sense::click_and_drag() } else { Sense::hover() });
|
||||
// A click selects the slider for arrow/wheel adjustments. Keep scroll-to-adjust
|
||||
// opt-in so hovering an unfocused slider still scrolls the containing panel.
|
||||
// Tab reaching the slider selects it as a click does: the arrow keys then adjust it
|
||||
// Keyboard focus survives leaving the row; vertical wheel edits need a fresh click or drag
|
||||
// in it (at most one slider is armed: `wheel_slider_key`).
|
||||
let wheel_id = id.with("wheel-rem");
|
||||
let mut wheel_armed = ui.data(|d| d.get_temp::<egui::Id>(wheel_slider_key())) == Some(id);
|
||||
let pointer_in_row = ui.rect_contains_pointer(row);
|
||||
// Every move this frame counts, not only where the pointer ended up: a slow frame (a slider
|
||||
// drag, soft proofing) can merge leaving the row and coming back into one.
|
||||
let left_row = wheel_armed
|
||||
&& ui.input(|i| {
|
||||
i.events.iter().any(|e| match e {
|
||||
egui::Event::PointerMoved(p) => !row.contains(*p),
|
||||
egui::Event::PointerGone => true,
|
||||
_ => false,
|
||||
})
|
||||
});
|
||||
// Tab reaching the slider selects it for arrow keys, not vertical wheel edits.
|
||||
if enabled && resp.gained_focus() {
|
||||
ui.data_mut(|d| d.insert_temp(focused_slider_key(), id));
|
||||
}
|
||||
if enabled && (resp.clicked() || resp.drag_started()) {
|
||||
let selected = enabled && (resp.clicked() || resp.drag_started());
|
||||
if selected {
|
||||
resp.request_focus();
|
||||
ui.data_mut(|d| d.insert_temp(focused_slider_key(), id));
|
||||
}
|
||||
// A drag that leaves the row disarms too (it keeps dragging: that needs no wheel selection),
|
||||
// and one that only starts once the pointer is outside doesn't arm it again.
|
||||
if selected && pointer_in_row && !left_row {
|
||||
ui.data_mut(|d| {
|
||||
d.insert_temp(wheel_slider_key(), id);
|
||||
d.remove::<f32>(wheel_id);
|
||||
});
|
||||
wheel_armed = true;
|
||||
} else if wheel_armed && (!enabled || !resp.has_focus() || !pointer_in_row || left_row) {
|
||||
ui.data_mut(|d| {
|
||||
d.remove::<egui::Id>(wheel_slider_key());
|
||||
d.remove::<f32>(wheel_id);
|
||||
});
|
||||
wheel_armed = false;
|
||||
}
|
||||
// screen readers: a slider named after its control, with its value
|
||||
let label_text = crate::i18n::tr(label_override.unwrap_or(spec.label)).to_string();
|
||||
resp.widget_info(|| egui::WidgetInfo::slider(enabled, value, label_text.clone()));
|
||||
@@ -464,7 +493,7 @@ pub fn slider(ui: &mut Ui, spec: &ControlSpec, value: f64, enabled: bool, label_
|
||||
})
|
||||
});
|
||||
// Arrow keys work with either a hovered or a focused slider.
|
||||
if enabled && out.value.is_none() && typing.is_none() && (ui.rect_contains_pointer(row) || resp.has_focus() || inc || dec) {
|
||||
if enabled && out.value.is_none() && typing.is_none() && (pointer_in_row || resp.has_focus() || inc || dec) {
|
||||
let (up, down, shift) = ui.input_mut(|i| {
|
||||
let shift = i.modifiers.shift;
|
||||
let m = if shift { egui::Modifiers::SHIFT } else { egui::Modifiers::NONE };
|
||||
@@ -487,7 +516,7 @@ pub fn slider(ui: &mut Ui, spec: &ControlSpec, value: f64, enabled: bool, label_
|
||||
// leaves the sliders it passes alone. Right raises the value, as dragging the thumb right does.
|
||||
let side_id = id.with("side-rem");
|
||||
let (dx, dy) = ui.input(|i| (i.smooth_scroll_delta.x, i.smooth_scroll_delta.y));
|
||||
if !enabled || typing.is_some() || out.value.is_some() || !ui.rect_contains_pointer(row) || dx.abs() <= dy.abs() {
|
||||
if !enabled || typing.is_some() || out.value.is_some() || !pointer_in_row || dx.abs() <= dy.abs() {
|
||||
ui.data_mut(|m| m.remove::<f32>(side_id));
|
||||
} else {
|
||||
// the vertical part would scroll the panel and move the slider out from under the pointer
|
||||
@@ -505,10 +534,8 @@ pub fn slider(ui: &mut Ui, spec: &ControlSpec, value: f64, enabled: bool, label_
|
||||
}
|
||||
// Wheel nudges require an explicitly selected slider, unlike regular panel scrolling.
|
||||
// Consume only the vertical component so a trackpad's horizontal scroll still works.
|
||||
let wheel_id = id.with("wheel-rem");
|
||||
if !resp.has_focus() {
|
||||
ui.data_mut(|m| m.remove::<f32>(wheel_id));
|
||||
} else if enabled && typing.is_none() && out.value.is_none() && ui.rect_contains_pointer(row) {
|
||||
// (`wheel_armed` is cleared above with its remainder whenever focus or the pointer leaves.)
|
||||
if wheel_armed && enabled && typing.is_none() && out.value.is_none() && pointer_in_row {
|
||||
let delta = ui.input_mut(|i| {
|
||||
let dy = i.smooth_scroll_delta.y;
|
||||
if dy != 0.0 {
|
||||
@@ -1031,8 +1058,13 @@ fn focused_slider_key() -> egui::Id {
|
||||
egui::Id::new("lightcraft-focused-slider")
|
||||
}
|
||||
|
||||
/// Whether the widget with keyboard focus is a develop slider a click selected. It owns only the
|
||||
/// arrow keys (and the wheel): every other shortcut still works, as in Lightroom after a drag.
|
||||
/// The slider a click or drag armed for vertical wheel edits, if any.
|
||||
fn wheel_slider_key() -> egui::Id {
|
||||
egui::Id::new("lightcraft-wheel-slider")
|
||||
}
|
||||
|
||||
/// Whether the widget with keyboard focus is a develop slider selected by a click, drag or Tab.
|
||||
/// It owns only the arrow keys: every other shortcut still works. Wheel selection is separate.
|
||||
pub fn slider_has_focus(ctx: &egui::Context) -> bool {
|
||||
let focused = ctx.memory(|m| m.focused());
|
||||
focused.is_some() && focused == ctx.data(|d| d.get_temp::<egui::Id>(focused_slider_key()))
|
||||
|
||||
@@ -195,3 +195,134 @@ fn a_long_click_keeps_the_field_open() {
|
||||
h.frame(vec![Event::Text("2".into())]);
|
||||
assert_eq!(h.key(Key::Enter).value, Some(2.0));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn leaving_a_slider_disarms_wheel_edits_until_it_is_selected_again() {
|
||||
let mut h = Harness::new();
|
||||
let inside = h.track.center();
|
||||
let outside = egui::pos2(5.0, 80.0);
|
||||
let button = |pressed| Event::PointerButton { pos: inside, button: PointerButton::Primary, pressed, modifiers: Modifiers::NONE };
|
||||
let wheel = || Event::MouseWheel {
|
||||
unit: egui::MouseWheelUnit::Point,
|
||||
delta: vec2(0.0, 120.0),
|
||||
phase: egui::TouchPhase::Move,
|
||||
modifiers: Modifiers::NONE,
|
||||
};
|
||||
h.frame(vec![Event::PointerMoved(inside)]);
|
||||
h.frame(vec![button(true)]);
|
||||
h.frame(vec![button(false)]);
|
||||
let selected = h.value;
|
||||
h.frame(vec![wheel()]);
|
||||
for _ in 0..40 {
|
||||
h.frame(vec![]);
|
||||
}
|
||||
assert!(h.value > selected, "selecting a slider enables wheel nudges");
|
||||
h.frame(vec![Event::PointerMoved(outside)]);
|
||||
let before_return = h.value;
|
||||
h.frame(vec![Event::PointerMoved(inside), wheel()]);
|
||||
for _ in 0..40 {
|
||||
h.frame(vec![]);
|
||||
}
|
||||
assert_eq!(h.value, before_return, "scrolling back over a previously selected slider must not edit it");
|
||||
h.frame(vec![Event::PointerMoved(outside)]);
|
||||
let before_arrow = h.value;
|
||||
h.key(Key::ArrowUp);
|
||||
assert!(h.value > before_arrow, "leaving must preserve keyboard arrows");
|
||||
// Sideways scrolling remains independent of vertical wheel selection (#1465).
|
||||
let before_sideways = h.value;
|
||||
h.frame(vec![
|
||||
Event::PointerMoved(inside),
|
||||
Event::MouseWheel { unit: egui::MouseWheelUnit::Point, delta: vec2(120.0, 0.0), phase: egui::TouchPhase::Move, modifiers: Modifiers::NONE },
|
||||
]);
|
||||
for _ in 0..40 {
|
||||
h.frame(vec![]);
|
||||
}
|
||||
let after_sideways = h.value;
|
||||
assert!(after_sideways > before_sideways, "a disarmed slider still accepts sideways nudges");
|
||||
h.frame(vec![wheel()]);
|
||||
for _ in 0..40 {
|
||||
h.frame(vec![]);
|
||||
}
|
||||
assert_eq!(h.value, after_sideways, "sideways nudges do not re-arm vertical wheel edits");
|
||||
h.frame(vec![Event::PointerMoved(inside)]);
|
||||
h.frame(vec![button(true)]);
|
||||
h.frame(vec![button(false)]);
|
||||
let selected_again = h.value;
|
||||
h.frame(vec![wheel()]);
|
||||
for _ in 0..40 {
|
||||
h.frame(vec![]);
|
||||
}
|
||||
assert!(h.value > selected_again, "selecting it again restores wheel nudges");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn leaving_the_row_during_a_drag_keeps_dragging_but_release_disarms_the_wheel() {
|
||||
let mut h = Harness::new();
|
||||
let inside = h.track.center();
|
||||
let outside = egui::pos2(h.track.right() - 20.0, 80.0);
|
||||
let button = |pos, pressed| Event::PointerButton { pos, button: PointerButton::Primary, pressed, modifiers: Modifiers::NONE };
|
||||
h.frame(vec![Event::PointerMoved(inside)]);
|
||||
h.frame(vec![button(inside, true)]);
|
||||
let initial = h.value;
|
||||
h.frame(vec![Event::PointerMoved(outside)]);
|
||||
assert!(h.value > initial, "an owned drag keeps editing outside the row");
|
||||
h.frame(vec![button(outside, false)]);
|
||||
let released = h.value;
|
||||
h.frame(vec![
|
||||
Event::PointerMoved(inside),
|
||||
Event::MouseWheel { unit: egui::MouseWheelUnit::Point, delta: vec2(0.0, -120.0), phase: egui::TouchPhase::Move, modifiers: Modifiers::NONE },
|
||||
]);
|
||||
for _ in 0..40 {
|
||||
h.frame(vec![]);
|
||||
}
|
||||
assert_eq!(h.value, released, "releasing outside disarms subsequent wheel edits");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_drag_that_leaves_and_ends_back_in_the_row_disarms_the_wheel() {
|
||||
let mut h = Harness::new();
|
||||
let inside = h.track.center();
|
||||
let outside = egui::pos2(h.track.right() - 20.0, 80.0);
|
||||
let button = |pos, pressed| Event::PointerButton { pos, button: PointerButton::Primary, pressed, modifiers: Modifiers::NONE };
|
||||
h.frame(vec![Event::PointerMoved(inside)]);
|
||||
h.frame(vec![button(inside, true)]);
|
||||
h.frame(vec![Event::PointerMoved(outside)]);
|
||||
h.frame(vec![Event::PointerMoved(inside)]);
|
||||
h.frame(vec![button(inside, false)]);
|
||||
let released = h.value;
|
||||
h.frame(vec![Event::MouseWheel {
|
||||
unit: egui::MouseWheelUnit::Point,
|
||||
delta: vec2(0.0, 120.0),
|
||||
phase: egui::TouchPhase::Move,
|
||||
modifiers: Modifiers::NONE,
|
||||
}]);
|
||||
for _ in 0..40 {
|
||||
h.frame(vec![]);
|
||||
}
|
||||
assert_eq!(h.value, released, "the drag left the row, so the wheel needs a fresh selection");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn leaving_and_returning_within_one_frame_still_disarms_the_wheel() {
|
||||
let wheel = || Event::MouseWheel {
|
||||
unit: egui::MouseWheelUnit::Point,
|
||||
delta: vec2(0.0, 120.0),
|
||||
phase: egui::TouchPhase::Move,
|
||||
modifiers: Modifiers::NONE,
|
||||
};
|
||||
// a slow frame merges the moves out of the row and back (or out of the window and back)
|
||||
for away in [Event::PointerMoved(egui::pos2(5.0, 80.0)), Event::PointerGone] {
|
||||
let mut h = Harness::new();
|
||||
let inside = h.track.center();
|
||||
let button = |pressed| Event::PointerButton { pos: inside, button: PointerButton::Primary, pressed, modifiers: Modifiers::NONE };
|
||||
h.frame(vec![Event::PointerMoved(inside)]);
|
||||
h.frame(vec![button(true)]);
|
||||
h.frame(vec![button(false)]);
|
||||
let selected = h.value;
|
||||
h.frame(vec![away.clone(), Event::PointerMoved(inside), wheel()]);
|
||||
for _ in 0..40 {
|
||||
h.frame(vec![]);
|
||||
}
|
||||
assert_eq!(h.value, selected, "the pointer left the row between two frames ({away:?})");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
# Lightroom parity checklist
|
||||
|
||||
> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-20 · **Change:** minor (LR-PRE-PANEL: the applied preset stays marked per photo) · **Target:** Adobe Lightroom Classic 15.5 / Lightroom 9.6
|
||||
> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-21 · **Change:** patch (KEY-SLIDER-NUDGE: vertical wheel nudges need a click or drag in the row; leaving it disarms them) · **Target:** Adobe Lightroom Classic 15.5 / Lightroom 9.6
|
||||
|
||||
The measured, row-by-row checklist: one row per Lightroom feature, menu item and shortcut, what we
|
||||
have and what is missing. It answers *does the feature exist?* (feature breadth). Whether it is good
|
||||
@@ -673,7 +673,7 @@ From `06-shortcuts.md` part 1. Evidence is our binding; conflicts are explained
|
||||
| KEY-CROP-RESET | Reset crop — ⌥⌘R | P0 | ✅ | `cmd:crop.reset` | |
|
||||
| KEY-STRAIGHTEN | Straighten while held — ⌘ drag | P1 | ✅ | `cmd:crop.straighten`, `crates/ui-egui/src/panels/detail.rs` (`straighten_overlay`) | ⌘-drag in Crop draws a level line; the crop tool stays active |
|
||||
| KEY-SLIDER-RESET | Reset slider — double-click | P0 | ✅ | `crates/ui-egui/src/widgets.rs`, `crates/ui-egui/src/tests_slider_reset.rs` | double-click a slider's name or thumb: back to its default as one undo step (a click on the thumb no longer moves it first, which made it two); resting the pointer on either says so (“Exposure — double-click to reset it”, as the curve channels), the value keeps “Click to type a value” (issue #1436); a disabled slider's name doesn't reset it. No per-slider reset button (Lightroom has none); no ⌥ panel headings turning into Reset buttons yet (`cmd:develop.resetSection` is command-only) |
|
||||
| KEY-SLIDER-NUDGE | Nudge slider — ↑/↓ | P1 | ✅ | `crates/ui-egui/src/widgets.rs` (`nudged`), `cmd:develop.adjust` | ↑/↓ over any slider: ≈ 1/200 of its range (exposure 0.05, most sliders 1, temperature 50 K); ⇧ ×5; one undo step each. Scrolling sideways over any slider (trackpad swipe, tilting wheel, ⇧+wheel) nudges it the same way, one wheel notch a step, without selecting it first; only a mostly sideways movement counts, so scrolling the panel leaves sliders alone (`crates/ui-egui/src/tests_masking.rs`, `sideways_scrolling_adjusts_the_slider_under_the_pointer`) |
|
||||
| KEY-SLIDER-NUDGE | Nudge slider — ↑/↓ | P1 | ✅ | `crates/ui-egui/src/widgets.rs` (`nudged`), `cmd:develop.adjust` | ↑/↓ over any slider: ≈ 1/200 of its range (exposure 0.05, most sliders 1, temperature 50 K); ⇧ ×5; one undo step each. Scrolling sideways over any slider (trackpad swipe, tilting wheel, ⇧+wheel) nudges it the same way, one wheel notch a step, without selecting it first; only a mostly sideways movement counts, so scrolling the panel leaves sliders alone (`crates/ui-egui/src/tests_masking.rs`, `sideways_scrolling_adjusts_the_slider_under_the_pointer`). The vertical wheel nudges only a slider a click or drag selected, while the pointer stays in its row: leaving the row (even within one frame) disarms it until the next click or drag, and Tab focus arms only the arrow keys (issue #1355, `crates/ui-egui/src/widgets/tests_numeric_entry.rs`) |
|
||||
| KEY-SLIDER-TYPE | Type a slider value | P1 | ✅ | `crates/ui-egui/src/widgets.rs` (`typed_value`), `crates/ui-egui/src/text_field.rs`, `crates/ui-egui/src/widgets/tests_numeric_entry.rs` (`opening_release_honors_return_and_escape_immediately`, `a_long_click_keeps_the_field_open`), `crates/ui-egui/src/tests_keymap.rs` (`opening_numeric_release_keeps_keys_in_the_editor`) | click the value next to any slider's name and type one (`1.5`, `+0,25`, `-20`, `5600 K`): Return or clicking away applies it on the control's steps and range as one undo step, Esc keeps the old value; issue #322; Return/Escape take effect on the opening release, retaining the stored value on Escape; typing stays in the editor without triggering global shortcuts; the field stays open however long the click is held (a release over the value keeps it, issue #1435) |
|
||||
| KEY-SLIDER-SCRUB | Scrub a slider value — drag the number | P1 | ✅ | `crates/ui-egui/src/widgets.rs` (`slider`, `SCRUB_START`), `crates/ui-egui/src/widgets/tests_numeric_entry.rs` (`dragging_the_value_sideways_scrubs_it_like_the_thumb`, `scrubbing_back_and_with_shift`, `a_press_that_barely_moves_still_types`) | drag the value next to any slider's name sideways: it moves as dragging the thumb would (the pointer's travel over the track's width, ⇧ a tenth as far), on the control's steps and range, as one undo step; a press that moves less than 3 points sideways stays a click that types a value (KEY-SLIDER-TYPE); the pointer over the value is the horizontal-resize cursor, as Lightroom's scrubby value |
|
||||
| KEY-SHORTCUTS | Shortcut list — ⌘/ | P1 | ✅ | `cmd:app.shortcuts` | |
|
||||
@@ -797,6 +797,7 @@ Latin stays upright. Advanced Japanese composition remains open. Regression cove
|
||||
|
||||
| Date | Change | Summary |
|
||||
|---|---|---|
|
||||
| 2026-10-21 | patch | KEY-SLIDER-NUDGE: vertical wheel nudges need a click or drag in the slider's row; leaving it (even between two frames) or Tab focus alone does not arm them (issue #1355); status unchanged |
|
||||
| 2026-10-20 | minor | LR-PRE-PANEL: the preset applied to a photo is marked again (with its Amount slider) on returning to the photo, until the photo is edited further (#2392); status unchanged (✅) |
|
||||
| 2026-10-20 | patch | LR-VIEW-ZOOM: the 20 ms rule clears a slow-drag verdict only on a drag timed at full size; a drag that drafts small leaves it unchanged, as the code does; status unchanged |
|
||||
| 2026-10-19 | patch | LR-VIEW-ZOOM: the slow slider-drag verdict is described as per photo and control, as the code keys it; status unchanged |
|
||||
|
||||
@@ -1,5 +1,15 @@
|
||||
# What's new in LightCraft
|
||||
|
||||
## 2026-10-21
|
||||
|
||||
### Scrolling past editing sliders
|
||||
- Leaving a selected slider disarms its vertical mouse-wheel adjustment (issue #1355), even when the pointer leaves
|
||||
and comes back between two screen updates. Scrolling back over it now scrolls the panel instead of unexpectedly
|
||||
changing the photo. Click or drag the slider again to adjust it with the vertical wheel; keyboard arrow
|
||||
adjustments and an ongoing drag still work. A slider reached with Tab takes the arrow keys but not the wheel:
|
||||
click it to scroll-adjust it. Sideways scrolling still adjusts the slider under the pointer without selecting it
|
||||
first.
|
||||
|
||||
## 2026-10-19
|
||||
|
||||
### Hidden Local locations come back one at a time
|
||||
|
||||
Reference in New Issue
Block a user