Merge pull request #11672 from RickiNano/fix/11368-marquee-moves-outline-after-move

Marquee and Lasso: a drag after moving pixels moves only the outline (#11368)

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,

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<bool> {
     // 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;

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");
     }
 }

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<String> = 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");

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