diff --git a/crates/ui-egui/src/canvas/selection.rs b/crates/ui-egui/src/canvas/selection.rs index b3e37d1f18..5845e4bcf1 100644 --- a/crates/ui-egui/src/canvas/selection.rs +++ b/crates/ui-egui/src/canvas/selection.rs @@ -11,7 +11,8 @@ pub enum SelCursor { Cut, /// Arrow with a second arrow: ⌥ lifts a copy of them instead. Copy, - /// Plain arrow: a floating piece, moved again by a plain drag until it is dropped. + /// Plain arrow: a floating piece, moved again by the Move tool (⌘ with a marquee) until it is + /// dropped. Piece, /// Hollow arrowhead: the selected pixels are being dragged. Dragging, diff --git a/crates/ui-egui/src/canvas/tool_input.rs b/crates/ui-egui/src/canvas/tool_input.rs index 08aff7dd70..d678929e47 100644 --- a/crates/ui-egui/src/canvas/tool_input.rs +++ b/crates/ui-egui/src/canvas/tool_input.rs @@ -109,6 +109,11 @@ fn preflight_tool_event(app: &mut PhotocraftApp, ev: ToolEvent, mods: egui::Modi Some((armed, mods)) } +/// The handlers a pointer event meets before the tool: dropping a floating piece the press won't +/// drag, snapping, and the overlays and modifier gestures (Free Transform, sticky keys, brush +/// resize, the ⌥ eyedropper, path and shape widgets…) that take the event for themselves. `None`: +/// one of them handled it; otherwise the event as it came and as filtered (snapped, the Move +/// tool's axis lock), and the modifiers with the sticky keys applied. fn filter_event_before_tool(app: &mut PhotocraftApp, ev: ToolEvent, mods: egui::Modifiers, armed: bool) -> Option<(ToolEvent, ToolEvent, egui::Modifiers)> { // View › Snap / Snap To and smart guides (snap_ui.rs). let raw = ev; @@ -116,8 +121,9 @@ fn filter_event_before_tool(app: &mut PhotocraftApp, ev: ToolEvent, mods: egui:: if crate::move_box::pointer(app, raw) { return None; } - // A press anywhere but on the floating piece (or with ⇧ / ⌥, to draw) drops it first; the - // Move tool (⌘ with a painting tool too, `event_tool`) drags it from anywhere. + // A press that doesn't drag the floating piece drops it first: off it, with ⇧ / ⌥ to draw, or a + // plain selection-tool press on it, which moves the outline only (#11368). The Move tool (⌘ + // with a painting tool too, `event_tool`) drags it from anywhere. if let ToolEvent::Down { x, y, .. } = raw && app.session.active().is_some_and(|st| photocraft_engine::float_cmds::floating(st).is_some()) && !crate::move_ui::moves_selected_pixels_with(app, event_tool(app, mods)) @@ -673,12 +679,13 @@ fn covers(sel: &photocraft_raster::Surface, p: [f64; 2]) -> bool { } /// Does a press with `tool` at `p` drag the selection rather than draw a new one? `Some(true)` -/// moves the floating piece (⌘ cuts one first, ⌘⌥ copies; a floating piece moves with a plain -/// drag; the Polygonal Lasso and Magic Wand need ⌘, as a plain press there places or samples), -/// `Some(false)` just the outline (no ⇧ / ⌥ and the options bar on New Selection, so a combining -/// drag still draws). The Single Row / Single Column Marquee drag the outline too (#11392); a click -/// without moving, inside the selection or on a floating piece (dropped first), still selects the -/// line under the pointer (`finish_gesture`). +/// moves the floating piece (⌘ cuts one first, ⌘⌥ copies; the Polygonal Lasso and Magic Wand need +/// ⌘, as a plain press there places or samples), `Some(false)` just the outline (no ⇧ / ⌥ and the +/// options bar on New Selection, so a combining drag still draws). A plain drag on a floating piece +/// moves the outline too, as in Photoshop: the press drops the piece first (#11368). The Single Row +/// / Single Column Marquee drag the outline too (#11392); a click without moving, inside the +/// selection or on a floating piece (dropped first), still selects the line under the pointer +/// (`finish_gesture`). pub fn selection_drag_kind(app: &PhotocraftApp, tool: Tool, p: [f64; 2], mods: egui::Modifiers) -> Option { // Click-driven selection tools (a press places a point or samples): only ⌘ drags the // selection, and not while a polygon is being drawn. @@ -690,11 +697,18 @@ pub fn selection_drag_kind(app: &PhotocraftApp, tool: Tool, p: [f64; 2], mods: e // ⌘ cuts, ⌘⌥ copies (`select.float`'s `copy`). let cut = mods.command && !mods.shift; if let Some(f) = app.session.active().and_then(photocraft_engine::float_cmds::floating) { - // A press on the piece's outline (where its ants are) drags it; one in the margin the cut - // left of the selection starts a new selection (#7538). + // A ⌘ press on the piece's outline (where its ants are) drags it; a plain one drags the + // outline, dropping the piece where it is first (`filter_event_before_tool`, #11368); one + // in the margin the cut left of the selection starts a new selection (#7538). let at = [p[0] - f64::from(f.offset.0), p[1] - f64::from(f.offset.1)]; let on = f.parts.outline().map_or_else(|| inside_selection(app, at), |o| covers(o, at)); - return (on && !mods.shift && (!mods.alt || mods.command) && (cut || !clicky)).then_some(true); + if !on { + return None; + } + if cut { + return Some(true); + } + return (!clicky && !mods.command && selection_mode(app, mods) == "replace").then_some(false); } if !inside_selection(app, p) { return None; diff --git a/crates/ui-egui/src/canvas/transform_controls_tests.rs b/crates/ui-egui/src/canvas/transform_controls_tests.rs index 8eb7dbf821..ecfbfe056d 100644 --- a/crates/ui-egui/src/canvas/transform_controls_tests.rs +++ b/crates/ui-egui/src/canvas/transform_controls_tests.rs @@ -89,14 +89,16 @@ fn floating_square() -> PhotocraftApp { app } -/// #7538: a selection tool's press on a floating piece's ants drags it; one in the margin its -/// cut left of the loose marquee draws a new selection, as the ants say. +/// #7538: a selection tool's press on a floating piece's ants drags its outline (#11368), a ⌘ +/// press the piece; one in the margin its cut left of the loose marquee draws a new selection, as +/// the ants say. #[test] fn a_press_in_the_margin_of_a_floating_piece_starts_a_new_selection() { let app = floating_square(); let none = egui::Modifiers::NONE; for tool in [Tool::RectMarquee, Tool::Lasso] { - assert_eq!(selection_drag_kind(&app, tool, [70.0, 60.0], none), Some(true), "{tool:?}: on the square drags it"); + assert_eq!(selection_drag_kind(&app, tool, [70.0, 60.0], none), Some(false), "{tool:?}: on the square drags its outline"); + assert_eq!(selection_drag_kind(&app, tool, [70.0, 60.0], egui::Modifiers::COMMAND), Some(true), "{tool:?}: ⌘ drags the square"); assert_eq!(selection_drag_kind(&app, tool, [33.0, 28.0], none), None, "{tool:?}: in the old marquee's margin"); } } diff --git a/crates/ui-egui/src/marquee_tests.rs b/crates/ui-egui/src/marquee_tests.rs index 6cf286b52b..5a42aab0fc 100644 --- a/crates/ui-egui/src/marquee_tests.rs +++ b/crates/ui-egui/src/marquee_tests.rs @@ -803,7 +803,7 @@ fn shift_constrains_a_floating_selection_drag() { } /// ⌘-drag cuts the selected pixels into a floating piece (`select.float`): shown at the pointer -/// while dragging, moved again by plain drags, put back by Undo, dropped by Deselect. +/// while dragging, moved again by ⌘-drags, put back by Undo, dropped by Deselect. #[test] fn cmd_drag_floats_the_selected_pixels() { use crate::canvas::{ToolEvent, tool_event}; @@ -819,8 +819,8 @@ fn cmd_drag_floats_the_selected_pixels() { tool_event(&mut app, ToolEvent::Up { x: 35.0, y: 20.0 }, Modifiers::COMMAND); assert_eq!(offset(&app), Some((15, 0))); assert_eq!(alpha(&doc(&app), 12, 20), 1.0, "the document waits for the drop"); - // A plain drag on the piece moves it again. - drag(&mut app, [30.0, 20.0], [30.0, 30.0], Modifiers::NONE); + // A ⌘-drag on the piece moves it again. + drag(&mut app, [30.0, 20.0], [30.0, 30.0], Modifiers::COMMAND); assert_eq!(offset(&app), Some((15, 10))); assert_eq!(app.session.active().unwrap().history.past_len(), steps); // Undo puts it back. @@ -836,6 +836,29 @@ fn cmd_drag_floats_the_selected_pixels() { assert_eq!(labels.first().map(String::as_str), Some("Move"), "{labels:?}"); } +/// #11368: after the Move tool moves the selected pixels, a plain marquee drag inside the +/// selection moves only the outline, as in Photoshop: the pixels are dropped where they are (one +/// Move step), then the outline moves on its own (one Move Selection step). +#[test] +fn a_marquee_drag_after_moving_pixels_moves_only_the_outline() { + let (mut app, layer) = painted(); + let alpha = |app: &PhotocraftApp, x, y| app.session.active().unwrap().doc.layer(layer).unwrap().surface().unwrap().rgba(x, y)[3]; + let sel = |app: &PhotocraftApp| app.session.active().unwrap().doc.selection.as_ref().map(|s| s.content_bounds()); + let floating = |app: &PhotocraftApp| photocraft_engine::float_cmds::floating(app.session.active().unwrap()).is_some(); + let steps = app.session.active().unwrap().history.past_len(); + app.ui.tool = Tool::Move; + drag(&mut app, [20.0, 20.0], [35.0, 20.0], Modifiers::NONE); + assert!(floating(&app), "the Move tool leaves the moved pixels floating"); + app.ui.tool = Tool::RectMarquee; + drag(&mut app, [30.0, 20.0], [30.0, 30.0], Modifiers::NONE); + assert!(!floating(&app), "the press dropped the piece"); + assert_eq!(sel(&app), Some(Rect::new(25, 20, 45, 40)), "the outline moved down"); + assert!(alpha(&app, 40, 15) == 1.0 && alpha(&app, 40, 35) == 0.0, "the pixels stay where the Move tool left them"); + assert_eq!(alpha(&app, 12, 20), 0.0, "the cut stays cut"); + let labels: Vec = app.session.active().unwrap().history.entries().into_iter().skip(steps + 1).map(|e| e.to_string()).collect(); + assert_eq!(labels, ["Move", "Move Selection"]); +} + /// Through the real canvas (mouse events): a drag inside the ants moves the selection. #[test] fn mouse_drag_inside_the_selection_moves_it() { @@ -1070,7 +1093,8 @@ fn selected_pixels_move_off_the_background_but_not_off_a_fully_locked_layer() { /// Photoshop's cursor over a selection: the move cursor where a marquee press moves the outline, /// scissors where ⌘ (or the Move tool) cuts the selected pixels, a double arrow where ⌥ copies -/// them, a hollow arrowhead while they are dragged, and a plain arrow over the floating piece. +/// them, a hollow arrowhead while they are dragged, and a plain arrow over the floating piece where +/// a press would drag it (the Move tool, ⌘). #[test] fn selection_cursor_follows_the_modifiers_and_the_floating_piece() { use crate::canvas::{SelCursor, selection_cursor}; @@ -1094,7 +1118,8 @@ fn selection_cursor_follows_the_modifiers_and_the_floating_piece() { tool_event(&mut app, ToolEvent::Move { x: 35.0, y: 20.0, pressure: 1.0 }, Modifiers::COMMAND); assert_eq!(selection_cursor(&app, Tool::RectMarquee, [35.0, 20.0], Modifiers::COMMAND), Some(SelCursor::Dragging)); tool_event(&mut app, ToolEvent::Up { x: 35.0, y: 20.0 }, Modifiers::COMMAND); - assert_eq!(selection_cursor(&app, Tool::RectMarquee, [35.0, 20.0], none), Some(SelCursor::Piece)); + assert_eq!(selection_cursor(&app, Tool::RectMarquee, [35.0, 20.0], none), Some(SelCursor::Outline), "a plain drag moves the outline (#11368)"); + assert_eq!(selection_cursor(&app, Tool::RectMarquee, [35.0, 20.0], Modifiers::COMMAND), Some(SelCursor::Piece)); assert_eq!(selection_cursor(&app, Tool::Move, [35.0, 20.0], none), Some(SelCursor::Piece)); assert_eq!(selection_cursor(&app, Tool::RectMarquee, [35.0, 20.0], cmd_alt), Some(SelCursor::Copy), "a copy of the piece"); assert_eq!(selection_cursor(&app, Tool::RectMarquee, inside, none), None, "where it was cut from"); diff --git a/crates/ui-egui/src/single_line_marquee_move_tests.rs b/crates/ui-egui/src/single_line_marquee_move_tests.rs index 01c2286137..1f6037cc82 100644 --- a/crates/ui-egui/src/single_line_marquee_move_tests.rs +++ b/crates/ui-egui/src/single_line_marquee_move_tests.rs @@ -50,6 +50,12 @@ fn steps(app: &PhotocraftApp) -> usize { app.session.active().unwrap().history.entries().len() } +/// The active layer's pixel at `(x, y)`. +fn pixel(app: &PhotocraftApp, x: i32, y: i32) -> [f32; 4] { + let st = app.session.active().unwrap(); + st.doc.layer(st.active_layer.unwrap()).unwrap().surface().unwrap().rgba(x, y) +} + /// Rows 150 and 250 (New, then ⇧ adds). fn two_rows(app: &mut PhotocraftApp) { click(app, Tool::SingleRowMarquee, [300.0, 250.5], Modifiers::NONE); @@ -148,7 +154,8 @@ fn click_inside_with_snapping_on_selects_the_line() { /// A drag from inside that snapping pulls back to where it started (the selection's edge on a /// guide, 2 px away) is not a click: the selection stays, with no step. It took the click's path: -/// one line replaced the rows, a floating piece was dropped, and the Rectangular Marquee deselected. +/// one line replaced the rows, and the Rectangular Marquee deselected. A floating piece is dropped +/// where it is by the press, which drags the outline (#11368). #[test] fn a_drag_snapped_back_to_its_start_leaves_the_selection() { let floats = |app: &PhotocraftApp| app.session.active().is_some_and(|st| photocraft_engine::float_cmds::floating(st).is_some()); @@ -170,14 +177,15 @@ fn a_drag_snapped_back_to_its_start_leaves_the_selection() { app.run("edit.fill", json!({"color": "#ff0000"})).unwrap(); app.run("select.float", json!({"dx": 0, "dy": 0})).unwrap(); drag(&mut app, Tool::SingleRowMarquee, [200.0, 200.5], [200.0, 202.5], Modifiers::NONE); - assert!(floats(&app), "the piece still floats"); + assert!(!floats(&app), "the press dropped the piece where it was (#11368)"); assert_eq!(bounds(&app), Rect::new(100, 150, 300, 250)); + assert_eq!(pixel(&app, 200, 200), [1.0, 0.0, 0.0, 1.0], "the dropped pixels are where they were"); } /// A drag from inside that goes away and comes back to the press is not a click either: the /// selection stays where it was, with no step. Only the release was checked, so it took the -/// click's path (one line replaced the rows, the piece was dropped, the Rectangular Marquee -/// deselected). +/// click's path (one line replaced the rows, the Rectangular Marquee deselected). A floating piece +/// is dropped where it is by the press, which drags the outline (#11368). #[test] fn a_drag_that_returns_to_its_start_leaves_the_selection() { let floats = |app: &PhotocraftApp| app.session.active().is_some_and(|st| photocraft_engine::float_cmds::floating(st).is_some()); @@ -208,8 +216,9 @@ fn a_drag_that_returns_to_its_start_leaves_the_selection() { app.run("edit.fill", json!({"color": "#ff0000"})).unwrap(); app.run("select.float", json!({"dx": 0, "dy": 0})).unwrap(); out_and_back(&mut app, Tool::SingleRowMarquee, [200.0, 200.5]); - assert!(floats(&app), "the piece still floats"); + assert!(!floats(&app), "the press dropped the piece where it was (#11368)"); assert_eq!(bounds(&app), Rect::new(100, 150, 300, 250)); + assert_eq!(pixel(&app, 200, 200), [1.0, 0.0, 0.0, 1.0], "the dropped pixels are where they were"); } /// A click whose release drifts under a pixel from the press (a zoomed-in hand jitter) is still a