diff options
| author | srdusr <[email protected]> | 2025-09-28 20:46:00 +0200 |
|---|---|---|
| committer | srdusr <[email protected]> | 2025-09-28 20:46:00 +0200 |
| commit | 58a368df5f7a5d579335d1cb68213baacba5bc63 (patch) | |
| tree | 6d3e765b3307c94b17bbaabbc646f324fe11fef2 | |
| parent | 9a9aa8fe9d4f4f9b15860b45244e695942c90afd (diff) | |
| download | srdwm-58a368df5f7a5d579335d1cb68213baacba5bc63.tar.gz srdwm-58a368df5f7a5d579335d1cb68213baacba5bc63.zip | |
Fix multi-selected desktop icons only ever dragging one at a time
Reported live: "try move desktop items all at once somewhere else"
didn't work. Two compounding bugs, both real: CompState::
desktop_icon_drag only ever tracked one icon id, and the click handler
that starts a drag unconditionally collapsed any existing multi-
selection down to just the grabbed icon before the drag even began.
desktop_icon_drag is now Option<DesktopIconDrag> (crates/wayland/src/
desktop_icons.rs, new type): a grab offset, the grabbed icon's own live
position, and a members list - every currently-selected icon (the
grabbed one included), each a fixed offset from the grabbed icon's own
top-left at drag start, so the group moves as one rigid unit.
input/pointer.rs's click handler now only resets to single-selection
when the grabbed icon isn't already part of the current selection --
grabbing one inside an existing multi-selection keeps the whole group
selected and dragging, matching Windows/GNOME/macOS/KDE convention.
end_desktop_icon_drag snaps every dragged icon to its own nearest free
grid cell independently, tracking newly-claimed cells across the group
so two icons landing near each other never claim the same one.
Full workspace build/test/clippy clean, built and installed. Not unit-
testable (this module has no CompState test fixture for its own
selection/drag logic, an already-documented, accepted gap) - needs a
live drag to confirm.
| -rw-r--r-- | crates/wayland/src/desktop_icons.rs | 24 | ||||
| -rw-r--r-- | crates/wayland/src/input/pointer.rs | 15 | ||||
| -rw-r--r-- | crates/wayland/src/state/desktop_icons.rs | 121 | ||||
| -rw-r--r-- | crates/wayland/src/state/mod.rs | 16 | ||||
| -rw-r--r-- | docs/TODO.md | 8 |
5 files changed, 139 insertions, 45 deletions
diff --git a/crates/wayland/src/desktop_icons.rs b/crates/wayland/src/desktop_icons.rs index 7cfcbd1..c3b573a 100644 --- a/crates/wayland/src/desktop_icons.rs +++ b/crates/wayland/src/desktop_icons.rs @@ -57,6 +57,30 @@ impl DesktopIcon { } } +/// An in-progress icon drag that may carry more than one icon along +/// together. `primary` is whichever icon the pointer actually grabbed; +/// `members` is every icon moving with it (always includes `primary`, at +/// offset `(0, 0)`), each recorded as a fixed offset from `primary`'s own +/// top-left at the moment the drag started - the whole group moves as +/// one rigid unit regardless of where each member's own cell happens to +/// be, the same way dragging one file in a multi-selection in Windows/ +/// GNOME/macOS/KDE carries every other selected file along with it. +/// Reported live as missing: "try move desktop items all at once +/// somewhere else" didn't work at all before this - `members` used to +/// not exist, a drag only ever carried the one icon it grabbed no matter +/// how many were selected. +pub(crate) struct DesktopIconDrag { + /// Pointer's grab offset from `primary`'s own top-left at drag start, + /// so the icon tracks the pointer smoothly rather than snapping its + /// top-left corner straight to the cursor. + pub(crate) grab_offset: (i32, i32), + /// `primary`'s own live top-left this frame. + pub(crate) primary_pos: (i32, i32), + /// `(icon id, fixed offset from primary's own top-left at drag + /// start)` - primary included at offset `(0, 0)`. + pub(crate) members: Vec<(String, (i32, i32))>, +} + pub(crate) struct DesktopIcons { /// Top-left of the grid's own `(0, 0)` cell, in global space, one per /// participating monitor - each monitor's own usable-area origin plus diff --git a/crates/wayland/src/input/pointer.rs b/crates/wayland/src/input/pointer.rs index 602f613..44b0bd7 100644 --- a/crates/wayland/src/input/pointer.rs +++ b/crates/wayland/src/input/pointer.rs @@ -663,7 +663,20 @@ pub(crate) fn handle_pointer_button(state: &mut CompState, pos: Point<f64, Logic state.select_desktop_icon(Some(&id)); state.open_desktop_icon(&id); } else { - state.select_desktop_icon(Some(&id)); + // Don't collapse an existing multi-selection + // just because the drag grabbed one of its own + // members - `start_desktop_icon_drag` itself + // carries every currently-selected icon along + // when the one grabbed is already selected + // (see its own doc comment), the same "drag one + // of several selected files, they all move" + // convention every real desktop uses. Grabbing + // an icon *outside* the current selection still + // replaces it, same as before. + let already_selected = state.desktop_icons.as_ref().is_some_and(|icons| icons.icons.iter().any(|i| i.id == id && i.selected)); + if !already_selected { + state.select_desktop_icon(Some(&id)); + } state.start_desktop_icon_drag(&id, origin, (pos.x as i32, pos.y as i32)); } } diff --git a/crates/wayland/src/state/desktop_icons.rs b/crates/wayland/src/state/desktop_icons.rs index f111336..81a8ed8 100644 --- a/crates/wayland/src/state/desktop_icons.rs +++ b/crates/wayland/src/state/desktop_icons.rs @@ -170,7 +170,13 @@ impl CompState { let Some(icons) = &self.desktop_icons else { return Vec::new() }; let ids: Vec<String> = icons.icons.iter().map(|i| i.id.clone()).collect(); let origins = icons.origins.clone(); - let dragging = self.desktop_icon_drag.clone(); + // `(primary_pos, members)` - see `DesktopIconDrag`'s own doc + // comment. Every dragged icon (not just the one actually grabbed) + // follows the live pointer as a rigid group; every other mirror + // (if any) and every non-dragged icon stays put at its own + // origin's cell position, same as before this could carry more + // than one icon. + let dragging = self.desktop_icon_drag.as_ref().map(|d| (d.primary_pos, d.members.clone())); let mut out = Vec::with_capacity(ids.len() * origins.len()); for id in ids { let buffer = match self.icon_buffer(&id) { @@ -179,14 +185,12 @@ impl CompState { }; let icons = self.desktop_icons.as_ref().unwrap(); let icon = icons.icons.iter().find(|i| i.id == id).unwrap(); - // The dragged copy follows the live pointer on whichever - // monitor it's actually being dragged over; every other - // mirror (if any) stays put at its own origin's cell position - // - dragging on one monitor doesn't yank the icon's other - // mirrors around mid-gesture, only the one actually grabbed. - match &dragging { - Some((drag_id, _, live_pos)) if *drag_id == id => out.push((*live_pos, buffer)), - _ => { + let drag_pos = dragging + .as_ref() + .and_then(|(primary_pos, members)| members.iter().find(|(mid, _)| *mid == id).map(|(_, offset)| (primary_pos.0 + offset.0, primary_pos.1 + offset.1))); + match drag_pos { + Some(pos) => out.push((pos, buffer)), + None => { for &origin in &origins { out.push((icon.top_left(origin), buffer.clone())); } @@ -294,54 +298,105 @@ impl CompState { /// under the pointer, not always the primary monitor's, is what makes /// the drag track the cursor instead of jumping to a different /// monitor's copy the instant the drag starts. + /// Starts a drag on `id`. If `id` is already part of a multi- + /// selection, every other selected icon comes along as a `member` + /// (see `DesktopIconDrag`'s own doc comment); otherwise this starts a + /// fresh single-icon drag and selects just `id`, the same "grabbing + /// something outside the current selection replaces it" convention + /// real desktops use. pub(crate) fn start_desktop_icon_drag(&mut self, id: &str, origin: (i32, i32), pointer: (i32, i32)) { - let Some(icons) = &self.desktop_icons else { return }; - let Some(icon) = icons.icons.iter().find(|i| i.id == id) else { return }; - let top_left = icon.top_left(origin); - let grab_offset = (pointer.0 - top_left.0, pointer.1 - top_left.1); - self.desktop_icon_drag = Some((id.to_string(), grab_offset, top_left)); + let Some(icons) = &mut self.desktop_icons else { return }; + let Some(primary) = icons.icons.iter().find(|i| i.id == id) else { return }; + let primary_top_left = primary.top_left(origin); + let primary_selected = primary.selected; + let grab_offset = (pointer.0 - primary_top_left.0, pointer.1 - primary_top_left.1); + + let mut changed = Vec::new(); + if !primary_selected { + for icon in &mut icons.icons { + let should = icon.id == id; + if icon.selected != should { + icon.selected = should; + changed.push(icon.id.clone()); + } + } + } + let members: Vec<(String, (i32, i32))> = icons + .icons + .iter() + .filter(|i| i.id == id || i.selected) + .map(|i| { + let top_left = i.top_left(origin); + (i.id.clone(), (top_left.0 - primary_top_left.0, top_left.1 - primary_top_left.1)) + }) + .collect(); + self.desktop_icon_drag = Some(crate::desktop_icons::DesktopIconDrag { grab_offset, primary_pos: primary_top_left, members }); + for id in changed { + self.rebuild_icon_buffer(&id); + } } - /// Updates the live position of whichever icon is being dragged, if + /// Updates the live position of every icon in the current drag (the + /// one actually grabbed, plus every icon carried along with it), if /// any - called from every pointer-motion event, same as /// `WindowManager::update_resize`'s own per-motion-event update. pub(crate) fn update_desktop_icon_drag(&mut self, pointer: (i32, i32)) { - if let Some((_, grab_offset, live_pos)) = &mut self.desktop_icon_drag { - *live_pos = (pointer.0 - grab_offset.0, pointer.1 - grab_offset.1); + if let Some(drag) = &mut self.desktop_icon_drag { + drag.primary_pos = (pointer.0 - drag.grab_offset.0, pointer.1 - drag.grab_offset.1); } } - /// Ends an in-progress drag (if any): snaps to the nearest free grid - /// cell (occupied cells other than the dragged icon's own previous one - /// are avoided by walking outward from the raw target, closest first) - /// and persists it. + /// Ends an in-progress drag (if any): snaps every dragged icon (the + /// one actually grabbed, plus every icon carried along with it) to + /// its own nearest free grid cell, independently but never colliding + /// with each other - walking outward from each one's own raw target, + /// closest first - and persists all of them. pub(crate) fn end_desktop_icon_drag(&mut self) { - let Some((id, _, live_pos)) = self.desktop_icon_drag.take() else { return }; + let Some(drag) = self.desktop_icon_drag.take() else { return }; // The cell math below needs the origin of whichever monitor the - // icon was actually dropped on, not always the first mirror -- + // group was actually dropped on, not always the first mirror -- // recomputed fresh (same formula `desktop_icon_origins` uses) // rather than searched for in `icons.origins`, since a drop // just past every monitor's own strict icon-grid rect (but still // on-screen) should still resolve to that monitor's grid, not fall - // through to a stale/wrong one. + // through to a stale/wrong one. Based on the primary's own drop + // position - the group drops together onto whichever monitor the + // pointer is actually over. let origin = self .wm .borrow() .monitors() .iter() - .find(|m| m.full_geometry.contains_point(live_pos.0, live_pos.1)) + .find(|m| m.full_geometry.contains_point(drag.primary_pos.0, drag.primary_pos.1)) .map(|m| (m.geometry.x + GRID_MARGIN, m.geometry.y + GRID_MARGIN)) .unwrap_or(self.desktop_icons.as_ref().map(|i| i.origins.first().copied().unwrap_or((0, 0))).unwrap_or((0, 0))); let Some(icons) = &mut self.desktop_icons else { return }; - let raw = (live_pos.0 - origin.0, live_pos.1 - origin.1); - let raw_cell = ((raw.0 as f64 / CELL_WIDTH as f64).round() as i32, (raw.1 as f64 / CELL_HEIGHT as f64).round() as i32).max_zero(); - let occupied: std::collections::HashSet<(i32, i32)> = icons.icons.iter().filter(|i| i.id != id).map(|i| i.cell).collect(); - let cell = nearest_free_cell(raw_cell, &occupied); - if let Some(icon) = icons.icons.iter_mut().find(|i| i.id == id) { - icon.cell = cell; + let dragged_ids: std::collections::HashSet<&str> = drag.members.iter().map(|(id, _)| id.as_str()).collect(); + // Each member's own nearest free cell, in recorded order (primary + // first) - `newly_occupied` accumulates across the loop so two + // dragged icons landing near each other never both claim the same + // cell, on top of every cell a *non*-dragged icon already holds. + let mut newly_occupied: std::collections::HashSet<(i32, i32)> = std::collections::HashSet::new(); + let mut placements: Vec<(String, (i32, i32))> = Vec::with_capacity(drag.members.len()); + for (id, offset) in &drag.members { + let live_pos = (drag.primary_pos.0 + offset.0, drag.primary_pos.1 + offset.1); + let raw = (live_pos.0 - origin.0, live_pos.1 - origin.1); + let raw_cell = ((raw.0 as f64 / CELL_WIDTH as f64).round() as i32, (raw.1 as f64 / CELL_HEIGHT as f64).round() as i32).max_zero(); + let occupied: std::collections::HashSet<(i32, i32)> = + icons.icons.iter().filter(|i| !dragged_ids.contains(i.id.as_str())).map(|i| i.cell).chain(newly_occupied.iter().copied()).collect(); + let cell = nearest_free_cell(raw_cell, &occupied); + newly_occupied.insert(cell); + placements.push((id.clone(), cell)); + } + for (id, cell) in &placements { + if let Some(icon) = icons.icons.iter_mut().find(|i| &i.id == id) { + icon.cell = *cell; + } + } + for (id, cell) in &placements { + self.rebuild_icon_buffer(id); + crate::desktop_icons_state::save_icon(id, *cell); } - self.rebuild_icon_buffer(&id); - crate::desktop_icons_state::save_icon(&id, cell); } pub(crate) fn open_desktop_icon(&mut self, id: &str) { diff --git a/crates/wayland/src/state/mod.rs b/crates/wayland/src/state/mod.rs index 3519297..629b58c 100644 --- a/crates/wayland/src/state/mod.rs +++ b/crates/wayland/src/state/mod.rs @@ -353,17 +353,11 @@ pub(crate) struct CompState { /// same cached-until-dirty convention as every other decoration /// buffer in this codebase. pub(crate) desktop_icon_buffers: HashMap<String, MemoryRenderBuffer>, - /// An in-progress icon drag: the icon's own id, the pointer's grab - /// offset from that icon's cell origin at the moment the drag started - /// (so the icon tracks the pointer smoothly rather than snapping its - /// top-left corner straight to the cursor), and the icon's own live - /// top-left position this frame - updated on every pointer-motion - /// event by `update_desktop_icon_drag`, read straight back by - /// `desktop_icon_render_list` with no separate "current pointer - /// position" field needed anywhere on `CompState`. `None` whenever no - /// drag is active. - #[allow(clippy::type_complexity)] - pub(crate) desktop_icon_drag: Option<(String, (i32, i32), (i32, i32))>, + /// An in-progress icon drag - see `desktop_icons::DesktopIconDrag`'s + /// own doc comment for the full shape (it may carry more than one + /// icon, when the drag started on an already-multi-selected icon). + /// `None` whenever no drag is active. + pub(crate) desktop_icon_drag: Option<crate::desktop_icons::DesktopIconDrag>, /// An active rubber-band/marquee selection drag on bare desktop -- /// `(start, current)`, both global-space pointer positions. The one /// "click and drag" desktop interaction this compositor never had at diff --git a/docs/TODO.md b/docs/TODO.md index 98deb48..e539ffb 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -13,6 +13,14 @@ that has the full story. Keep this list current as items close or open; update the source doc's own entry too, don't let this drift into a second stale copy the way `PANEL_SUPPORT_TODO.md` did. +## Real bug, root-caused and fixed: dragging a multi-selected desktop icon only ever moved that one icon (2026-08-27) + +Reported live: "try move desktop items all at once somewhere else" didn't work. Confirmed by reading the actual data, not guessed: `CompState::desktop_icon_drag` only ever held one icon id, and - the real, compounding bug - the click handler that starts a drag (`input/pointer.rs`) called `select_desktop_icon(Some(&id))` *unconditionally* before starting the drag, which collapses any existing multi-selection down to just the one icon being grabbed. Even if the drag itself had supported multiple icons, that call site would have destroyed the selection before it ever got the chance. + +Fixed both halves. `desktop_icon_drag` is now `Option<DesktopIconDrag>` (`crates/wayland/src/desktop_icons.rs`, new type): a grab offset, the grabbed icon's own live position, and a `members` list - every currently-selected icon (the grabbed one included), each recorded as a fixed offset from the grabbed icon's own top-left at drag start, so the whole group moves as one rigid unit. `input/pointer.rs`'s click handler now only resets to single-selection when the grabbed icon *isn't* already part of the current selection - grabbing an icon inside an existing multi-selection keeps the whole group selected and dragging, the same "drag one of several selected files, they all move" convention Windows/GNOME/macOS/KDE all share. `end_desktop_icon_drag` snaps every dragged icon to its own nearest free grid cell independently, tracking newly-claimed cells across the group so two dragged icons landing near each other never both claim the same one. + +Full workspace build/test/clippy clean (223 core / 141 wayland / 29 platform / 24 ctl / 28 config / 10 x11 tests). Not unit-testable the way the placement fix above was - this module has no `CompState` test fixture for its own selection/drag logic (already documented as a real, accepted gap in this same file's own "rubber-band desktop icon selection" entry, not new to this fix) - needs a live drag to confirm, same as that entry's own outstanding item. + ## Real bug, root-caused and fixed: every new window opened alone landed in the exact same spot, not at all like Windows (2026-08-27) Reported live. Root-caused by reading `SmartPlacement::place`, not guessed: it tried a grid cell first, falling back to cascade only once the grid was full. Grid's own cell count is `existing.len() + 1` - with nothing else open (the overwhelmingly common real workflow: open one app, use it, close it, open the next), that count is always `1`, so the grid is always exactly one cell, and a 1x1 grid returns the same single cell every time regardless of session history. Cascade had a second, compounding version of the same bug: its own step was `existing.len() % max_steps`, also always `0` with nothing else open, so even a from-scratch cascade calculation reset to the origin on every call. |