From aa7b4a89e220bd87b03a117c3b6e004b78b496b9 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Fri, 14 Aug 2026 23:34:14 +0200 Subject: [PATCH 01/24] fix: restore multi-shape history at recorded indices Create redo added an enumerate offset on top of already-final indices, so pasting or duplicating two shapes and then undoing/redoing panicked. Delete undo used the same offset, which restacked non-adjacent shapes in the wrong order. --- src/draw/frame/history/frame/apply.rs | 19 ++++--- src/draw/frame/tests/history/basics.rs | 75 ++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 8 deletions(-) diff --git a/src/draw/frame/history/frame/apply.rs b/src/draw/frame/history/frame/apply.rs index 699d07a6..12ff9768 100644 --- a/src/draw/frame/history/frame/apply.rs +++ b/src/draw/frame/history/frame/apply.rs @@ -1,13 +1,11 @@ use super::super::super::core::Frame; -use super::super::super::types::{ShapeId, UndoAction}; +use super::super::super::types::{DrawnShape, ShapeId, UndoAction}; impl Frame { pub(super) fn apply_action(&mut self, action: &UndoAction) { match action { UndoAction::Create { shapes } => { - for (offset, (index, shape)) in shapes.iter().enumerate() { - self.insert_existing(index + offset, shape.clone()); - } + self.restore_shapes_at_recorded_indices(shapes); } UndoAction::Delete { shapes } => { for (_, shape) in shapes { @@ -50,10 +48,7 @@ impl Frame { } } UndoAction::Delete { shapes } => { - for (offset, (index, shape)) in shapes.iter().enumerate() { - let insert_at = (index + offset).min(self.shapes.len()); - self.insert_existing(insert_at, shape.clone()); - } + self.restore_shapes_at_recorded_indices(shapes); } UndoAction::Modify { shape_id, before, .. @@ -79,6 +74,14 @@ impl Frame { } } + fn restore_shapes_at_recorded_indices(&mut self, shapes: &[(usize, DrawnShape)]) { + let mut items: Vec<_> = shapes.iter().collect(); + items.sort_by_key(|(index, _)| *index); + for (index, shape) in items { + self.insert_existing((*index).min(self.shapes.len()), shape.clone()); + } + } + fn move_shape_to(&mut self, shape_id: ShapeId, target: usize) { if let Some(index) = self.find_index(shape_id) { if index == target { diff --git a/src/draw/frame/tests/history/basics.rs b/src/draw/frame/tests/history/basics.rs index 6367e3b7..ac51a26d 100644 --- a/src/draw/frame/tests/history/basics.rs +++ b/src/draw/frame/tests/history/basics.rs @@ -1,6 +1,18 @@ use crate::draw::frame::{Frame, ImageBoundsSnapshot, UndoAction}; use crate::draw::{EmbeddedImage, Shape, color::BLACK}; +fn rect_at(x: i32) -> Shape { + Shape::Rect { + x, + y: 0, + w: 10, + h: 10, + fill: false, + color: BLACK, + thick: 2.0, + } +} + #[test] fn undo_and_redo_cycle_shapes() { let mut frame = Frame::new(); @@ -34,6 +46,69 @@ fn undo_and_redo_cycle_shapes() { assert_eq!(frame.shapes.len(), 1); } +#[test] +fn redo_restores_multi_shape_create_in_recorded_order() { + let mut frame = Frame::new(); + let first_id = frame.add_shape(rect_at(0)); + let second_id = frame.add_shape(rect_at(20)); + frame.push_undo_action( + UndoAction::Create { + shapes: vec![ + (0, frame.shape(first_id).unwrap().clone()), + (1, frame.shape(second_id).unwrap().clone()), + ], + }, + 10, + ); + + assert!(frame.undo_last().is_some()); + assert!(frame.shapes.is_empty()); + + assert!(frame.redo_last().is_some()); + assert_eq!( + frame + .shapes + .iter() + .map(|shape| shape.id) + .collect::>(), + vec![first_id, second_id] + ); +} + +#[test] +fn undo_restores_gapped_delete_in_original_z_order() { + let mut frame = Frame::new(); + let ids: Vec<_> = (0..4) + .map(|index| frame.add_shape(rect_at(index * 20))) + .collect(); + let removed = vec![ + (0, frame.shape(ids[0]).unwrap().clone()), + (2, frame.shape(ids[2]).unwrap().clone()), + ]; + frame.shapes.remove(2); + frame.shapes.remove(0); + frame.push_undo_action(UndoAction::Delete { shapes: removed }, 10); + + assert_eq!( + frame + .shapes + .iter() + .map(|shape| shape.id) + .collect::>(), + vec![ids[1], ids[3]] + ); + + assert!(frame.undo_last().is_some()); + assert_eq!( + frame + .shapes + .iter() + .map(|shape| shape.id) + .collect::>(), + ids + ); +} + #[test] fn adding_new_shape_clears_redo_stack() { let mut frame = Frame::new(); From 3fb90ffbb73d2c2ef204862b47c401c6267fcbfd Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Fri, 14 Aug 2026 23:36:42 +0200 Subject: [PATCH 02/24] fix: keep original text when persisting during an in-place edit In-place text edit clears the stored string and keeps the live buffer in DrawingState. Autosave, shutdown, and output-switch snapshots read the frame directly, so a pause or quit during editing wrote empty text. Snapshot through the existing cancel-and-restore helper, and defer autosave while text input is active. --- .../backend/event_loop/session_save.rs | 13 +++- .../backend/event_loop/session_save/tests.rs | 9 ++- .../wayland/state/core/output/transition.rs | 2 +- src/input/state/core/session.rs | 73 +++++++++++++++++++ 4 files changed, 89 insertions(+), 8 deletions(-) diff --git a/src/backend/wayland/backend/event_loop/session_save.rs b/src/backend/wayland/backend/event_loop/session_save.rs index 8ab515cd..c3d6876e 100644 --- a/src/backend/wayland/backend/event_loop/session_save.rs +++ b/src/backend/wayland/backend/event_loop/session_save.rs @@ -78,7 +78,7 @@ fn persist_final_session_direct(state: &mut WaylandState) -> Result<(), anyhow:: if should_skip_protected_session_save(state, &options) { return Ok(()); } - let snapshot = session::snapshot_from_input(&state.input_state, &options); + let snapshot = state.input_state.snapshot_for_persistence(&options); let has_board_data = snapshot .as_ref() .is_some_and(session::SessionSnapshot::has_board_data); @@ -138,7 +138,7 @@ fn persist_final_session(state: &mut WaylandState) -> Result<(), anyhow::Error> options.session_file_path().display() ); let snapshot_started = Instant::now(); - let snapshot = session::snapshot_from_input(&state.input_state, &options); + let snapshot = state.input_state.snapshot_for_persistence(&options); log_snapshot_capture( SessionSaveReason::Shutdown, &options, @@ -239,7 +239,7 @@ pub(super) fn autosave_if_due(state: &mut WaylandState, now: Instant) -> Result< let started = Instant::now(); let snapshot_started = Instant::now(); - let snapshot = session::snapshot_from_input(&state.input_state, &options); + let snapshot = state.input_state.snapshot_for_persistence(&options); log_snapshot_capture( SessionSaveReason::Autosave, &options, @@ -537,6 +537,10 @@ pub(in crate::backend::wayland) fn should_defer_for_interaction(state: &WaylandS state.board_panning_active(), state.zoom_panning_active(), stylus_tip_down(state), + matches!( + state.input_state.state, + crate::input::DrawingState::TextInput { .. } + ), ) } @@ -547,8 +551,9 @@ fn persistence_interaction_active( board_pan: bool, zoom_pan: bool, stylus_tip: bool, + text_editing: bool, ) -> bool { - pointer || toolbar_drag || move_drag || board_pan || zoom_pan || stylus_tip + pointer || toolbar_drag || move_drag || board_pan || zoom_pan || stylus_tip || text_editing } pub(in crate::backend::wayland) fn interaction_defer_interval() -> Duration { diff --git a/src/backend/wayland/backend/event_loop/session_save/tests.rs b/src/backend/wayland/backend/event_loop/session_save/tests.rs index ed643ec9..4076e9ec 100644 --- a/src/backend/wayland/backend/event_loop/session_save/tests.rs +++ b/src/backend/wayland/backend/event_loop/session_save/tests.rs @@ -22,13 +22,16 @@ fn final_save_does_not_retry_through_an_unhealthy_worker() { #[test] fn pointer_and_stylus_both_gate_persistence_transitions() { assert!(persistence_interaction_active( - true, false, false, false, false, false + true, false, false, false, false, false, false )); assert!(persistence_interaction_active( - false, false, false, false, false, true + false, false, false, false, false, true, false + )); + assert!(persistence_interaction_active( + false, false, false, false, false, false, true )); assert!(!persistence_interaction_active( - false, false, false, false, false, false + false, false, false, false, false, false, false )); } diff --git a/src/backend/wayland/state/core/output/transition.rs b/src/backend/wayland/state/core/output/transition.rs index a7ee7a1e..b3335a3c 100644 --- a/src/backend/wayland/state/core/output/transition.rs +++ b/src/backend/wayland/state/core/output/transition.rs @@ -244,7 +244,7 @@ impl WaylandState { if self.should_skip_protected_session_save(options) { return Ok(()); } - let snapshot = session::snapshot_from_input(&self.input_state, options); + let snapshot = self.input_state.snapshot_for_persistence(options); if self.should_skip_unloaded_contentless_session_save(options, snapshot.as_ref())? { return Ok(()); } diff --git a/src/input/state/core/session.rs b/src/input/state/core/session.rs index 2b004acc..7b9d7abf 100644 --- a/src/input/state/core/session.rs +++ b/src/input/state/core/session.rs @@ -206,6 +206,16 @@ impl InputState { rollback.restore(self); value } + + /// Snapshot boards for persistence without writing in-progress edits as empty text. + pub(crate) fn snapshot_for_persistence( + &mut self, + options: &crate::session::SessionOptions, + ) -> Option { + self.with_active_interaction_canceled_for_capture(|input| { + crate::session::snapshot_from_input(input, options) + }) + } } #[cfg(test)] @@ -356,6 +366,69 @@ mod tests { assert!(!state.has_active_pointer_interaction()); } + #[test] + fn persistence_snapshot_keeps_original_text_during_in_place_edit() { + use crate::session::SessionOptions; + use std::path::PathBuf; + + let mut options = SessionOptions::new(PathBuf::from("/tmp"), "text-edit-snapshot"); + options.persist_transparent = true; + + let mut state = make_test_input_state(); + let shape_id = state.boards.active_frame_mut().add_shape(Shape::Text { + x: 40, + y: 80, + text: "Original".to_string(), + color: state.current_color, + size: state.current_font_size, + font_descriptor: state.font_descriptor.clone(), + background_enabled: state.text_background_enabled, + wrap_width: Some(180), + }); + state.set_selection(vec![shape_id]); + assert!(state.edit_selected_text()); + let DrawingState::TextInput { buffer, .. } = &mut state.state else { + panic!("expected text input"); + }; + buffer.push_str(" unsaved edit"); + + let live = crate::session::snapshot_from_input(&state, &options).expect("live snapshot"); + assert_eq!(first_snapshot_text(&live), ""); + + let persisted = state + .snapshot_for_persistence(&options) + .expect("persistence snapshot"); + assert_eq!(first_snapshot_text(&persisted), "Original"); + + let DrawingState::TextInput { buffer, .. } = &state.state else { + panic!("edit should still be active after persistence snapshot"); + }; + assert_eq!(buffer, "Original unsaved edit"); + let Shape::Text { text, .. } = &state + .boards + .active_frame() + .shape(shape_id) + .expect("text shape") + .shape + else { + panic!("expected text shape"); + }; + assert!(text.is_empty()); + } + + fn first_snapshot_text(snapshot: &crate::session::SessionSnapshot) -> &str { + for board in &snapshot.boards { + for page in &board.pages.pages { + for drawn in &page.shapes { + if let Shape::Text { text, .. } = &drawn.shape { + return text; + } + } + } + } + panic!("expected a text shape in the snapshot"); + } + fn test_shape_snapshot() -> ShapeSnapshot { ShapeSnapshot { shape: Shape::Freehand { From d405c6936f254b08a63c76c58fb59b0bcdd2ad66 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Fri, 14 Aug 2026 23:44:13 +0200 Subject: [PATCH 03/24] fix: crop portal freeze/zoom using desktop screenshot origin Portal captures are full-desktop images. Cropping with logical * this output's scale picks the wrong pixels on mixed-DPI layouts, and zoom kept the uncropped desktop when that crop missed. --- src/backend/wayland/frozen/state.rs | 84 ++++++++++++++++++- src/backend/wayland/frozen_geometry.rs | 29 ++++++- src/backend/wayland/handlers/compositor.rs | 18 +--- src/backend/wayland/handlers/layer.rs | 11 ++- src/backend/wayland/handlers/output.rs | 3 +- src/backend/wayland/handlers/xdg.rs | 11 ++- src/backend/wayland/state/capture/backdrop.rs | 30 +++++++ src/backend/wayland/state/capture/pdf.rs | 14 ---- src/backend/wayland/zoom/portal.rs | 33 ++++---- src/capture/tests/desktop_backdrop.rs | 9 ++ 10 files changed, 184 insertions(+), 58 deletions(-) diff --git a/src/backend/wayland/frozen/state.rs b/src/backend/wayland/frozen/state.rs index 6f5f9651..6830b02f 100644 --- a/src/backend/wayland/frozen/state.rs +++ b/src/backend/wayland/frozen/state.rs @@ -394,14 +394,14 @@ impl FrozenState { } let (origin_x, origin_y) = source_geometry .or(self.active_geometry.as_ref()) - .map(|geo| geo.physical_origin()) + .map(OutputGeometry::portal_crop_origin) .unwrap_or((0, 0)); let (width, height, data) = crop_argb( &image.data, image.width, image.height, - origin_x.max(0) as u32, - origin_y.max(0) as u32, + origin_x, + origin_y, target_width, target_height, )?; @@ -639,6 +639,84 @@ mod tests { assert!(!input_state.frozen_active()); } + fn desktop_geometry( + logical_x: i32, + logical_y: i32, + logical_width: u32, + logical_height: u32, + scale: i32, + screenshot_origin: Option<(u32, u32)>, + ) -> OutputGeometry { + OutputGeometry { + logical_x, + logical_y, + logical_width, + logical_height, + scale, + transform: wl_output::Transform::Normal, + screenshot_origin, + } + } + + #[test] + fn desktop_capture_crops_from_screenshot_origin_not_logical_times_scale() { + let mut state = FrozenState::new(None); + let mut input_state = make_test_input_state(); + let mut data = Vec::new(); + for pixel in 0u8..10 { + data.extend_from_slice(&[pixel, pixel, pixel, 255]); + } + let geometry = desktop_geometry(0, 0, 4, 1, 1, Some((6, 0))); + assert_eq!(geometry.physical_origin(), (0, 0)); + + state.set_pending_desktop_image( + FrozenImage { + width: 10, + height: 1, + stride: 40, + data, + }, + None, + Some(geometry), + ); + + state + .activate_pending_image(4, 1, &mut input_state) + .expect("mixed-scale screenshot origin should crop the active output"); + + let image = state.image().expect("the frozen image should be active"); + assert_eq!( + image.data, + vec![6, 6, 6, 255, 7, 7, 7, 255, 8, 8, 8, 255, 9, 9, 9, 255] + ); + } + + #[test] + fn desktop_capture_crops_at_buffer_origin_when_screenshot_origin_is_unknown() { + let mut state = FrozenState::new(None); + let mut input_state = make_test_input_state(); + let geometry = desktop_geometry(10, 20, 2, 1, 2, None); + assert_eq!(geometry.physical_origin(), (20, 40)); + assert_eq!(geometry.portal_crop_origin(), (0, 0)); + + state.set_pending_desktop_image( + FrozenImage { + width: 4, + height: 2, + stride: 16, + data: vec![7; 4 * 2 * 4], + }, + None, + Some(geometry), + ); + + state + .activate_pending_image(4, 2, &mut input_state) + .expect("a single-output desktop shot should crop from the buffer origin"); + assert!(state.image().is_some()); + assert!(input_state.frozen_active()); + } + #[test] fn pending_capture_is_discarded_if_output_changes_before_activation() { let mut state = FrozenState::new(None); diff --git a/src/backend/wayland/frozen_geometry.rs b/src/backend/wayland/frozen_geometry.rs index 05782f48..765ca7b0 100644 --- a/src/backend/wayland/frozen_geometry.rs +++ b/src/backend/wayland/frozen_geometry.rs @@ -9,6 +9,9 @@ pub struct OutputGeometry { pub logical_height: u32, pub scale: i32, pub transform: wl_output::Transform, + /// Origin of this output inside a full-desktop screenshot, walking every + /// known output so mixed-DPI layouts are not `logical * this_output.scale`. + pub screenshot_origin: Option<(u32, u32)>, } impl OutputGeometry { @@ -31,6 +34,7 @@ impl OutputGeometry { logical_height: lh as u32, scale, transform, + screenshot_origin: None, }) } } @@ -57,6 +61,12 @@ mod tests { assert_eq!(geo.transform, wl_output::Transform::_270); assert_eq!(geo.physical_size(), (3840, 2160)); assert_eq!(geo.physical_origin(), (20, 40)); + assert_eq!(geo.portal_crop_origin(), (0, 0)); + assert_eq!( + geo.with_screenshot_origin(Some((6, 0))) + .portal_crop_origin(), + (6, 0) + ); } #[test] @@ -106,11 +116,28 @@ impl OutputGeometry { ) } - /// Returns physical pixel origin. + /// Returns physical pixel origin of the logical position on this output. + /// + /// Portal desktop screenshots must use [`Self::portal_crop_origin`] instead: + /// mixed-scale layouts are not `logical * this output's scale`. pub fn physical_origin(&self) -> (i32, i32) { ( self.logical_x.saturating_mul(self.scale), self.logical_y.saturating_mul(self.scale), ) } + + pub fn with_screenshot_origin(mut self, origin: Option<(u32, u32)>) -> Self { + self.screenshot_origin = origin; + self + } + + /// Crop origin inside a portal/desktop screenshot. + /// + /// Unknown origin falls back to the buffer origin. A full-desktop shot of a + /// single output starts at `(0, 0)` even when that output's logical position + /// is not the layout origin. + pub fn portal_crop_origin(&self) -> (u32, u32) { + self.screenshot_origin.unwrap_or((0, 0)) + } } diff --git a/src/backend/wayland/handlers/compositor.rs b/src/backend/wayland/handlers/compositor.rs index cdf1b297..ebd61c47 100644 --- a/src/backend/wayland/handlers/compositor.rs +++ b/src/backend/wayland/handlers/compositor.rs @@ -136,7 +136,7 @@ impl CompositorHandler for WaylandState { .logical_size .unwrap_or((self.surface.width() as i32, self.surface.height() as i32)); let (logical_x, logical_y) = info.logical_position.unwrap_or((0, 0)); - self.frozen.set_active_geometry(Some( + self.set_freeze_zoom_geometry(Some( crate::backend::wayland::frozen_geometry::OutputGeometry { logical_x, logical_y, @@ -144,20 +144,11 @@ impl CompositorHandler for WaylandState { logical_height: logical_h.max(0) as u32, scale, transform: info.transform, + screenshot_origin: None, }, )); self.frozen .set_active_output(Some(output.clone()), Some(info.id)); - self.zoom.set_active_geometry(Some( - crate::backend::wayland::frozen_geometry::OutputGeometry { - logical_x, - logical_y, - logical_width: logical_w.max(0) as u32, - logical_height: logical_h.max(0) as u32, - scale, - transform: info.transform, - }, - )); self.zoom .set_active_output(Some(output.clone()), Some(info.id)); } @@ -203,10 +194,9 @@ impl CompositorHandler for WaylandState { } self.refresh_active_output_label(); self.frozen.set_active_output(None, None); - self.frozen.set_active_geometry(None); - self.frozen.unfreeze(&mut self.input_state); self.zoom.set_active_output(None, None); - self.zoom.set_active_geometry(None); + self.set_freeze_zoom_geometry(None); + self.frozen.unfreeze(&mut self.input_state); self.zoom.deactivate(&mut self.input_state); } } diff --git a/src/backend/wayland/handlers/layer.rs b/src/backend/wayland/handlers/layer.rs index e98b1b0d..d3a23f34 100644 --- a/src/backend/wayland/handlers/layer.rs +++ b/src/backend/wayland/handlers/layer.rs @@ -79,15 +79,20 @@ impl LayerShellHandler for WaylandState { .and_then(|output| self.output_state.info(output)) .map(|info| info.transform) .unwrap_or(wayland_client::protocol::wl_output::Transform::Normal); + let logical_position = self + .surface + .current_output() + .as_ref() + .and_then(|output| self.output_state.info(output)) + .and_then(|info| info.logical_position); if let Some(geo) = crate::backend::wayland::frozen_geometry::OutputGeometry::update_from( - None, // logical position is not available here + logical_position, Some((self.surface.width() as i32, self.surface.height() as i32)), (self.surface.width(), self.surface.height()), self.surface.scale(), output_transform, ) { - self.frozen.set_active_geometry(Some(geo.clone())); - self.zoom.set_active_geometry(Some(geo)); + self.set_freeze_zoom_geometry(Some(geo)); } } diff --git a/src/backend/wayland/handlers/output.rs b/src/backend/wayland/handlers/output.rs index 71e329a8..ead5c8d3 100644 --- a/src/backend/wayland/handlers/output.rs +++ b/src/backend/wayland/handlers/output.rs @@ -45,10 +45,9 @@ impl OutputHandler for WaylandState { info.scale_factor.max(1), info.transform, ) { - self.frozen.set_active_geometry(Some(geo.clone())); + self.set_freeze_zoom_geometry(Some(geo)); self.frozen .set_active_output(Some(output.clone()), Some(info.id)); - self.zoom.set_active_geometry(Some(geo)); self.zoom .set_active_output(Some(output.clone()), Some(info.id)); } diff --git a/src/backend/wayland/handlers/xdg.rs b/src/backend/wayland/handlers/xdg.rs index c1bd64c2..d21b7eb5 100644 --- a/src/backend/wayland/handlers/xdg.rs +++ b/src/backend/wayland/handlers/xdg.rs @@ -130,15 +130,20 @@ impl WindowHandler for WaylandState { .and_then(|output| self.output_state.info(output)) .map(|info| info.transform) .unwrap_or(wayland_client::protocol::wl_output::Transform::Normal); + let logical_position = self + .surface + .current_output() + .as_ref() + .and_then(|output| self.output_state.info(output)) + .and_then(|info| info.logical_position); if let Some(geo) = crate::backend::wayland::frozen_geometry::OutputGeometry::update_from( - None, // logical position is not available here + logical_position, Some((self.surface.width() as i32, self.surface.height() as i32)), (self.surface.width(), self.surface.height()), self.surface.scale(), output_transform, ) { - self.frozen.set_active_geometry(Some(geo.clone())); - self.zoom.set_active_geometry(Some(geo)); + self.set_freeze_zoom_geometry(Some(geo)); } if self.xdg_frozen_fullscreen_requested() && self.frozen.has_pending_image() { if self.xdg_frozen_fullscreen_pending_configure() && !configure.is_fullscreen() { diff --git a/src/backend/wayland/state/capture/backdrop.rs b/src/backend/wayland/state/capture/backdrop.rs index 480c4095..35cfcd52 100644 --- a/src/backend/wayland/state/capture/backdrop.rs +++ b/src/backend/wayland/state/capture/backdrop.rs @@ -1,4 +1,5 @@ use super::super::*; +use crate::backend::wayland::frozen_geometry::OutputGeometry; pub(super) fn desktop_backdrop_output_geometry_from_info( info: &smithay_client_toolkit::output::OutputInfo, @@ -61,6 +62,35 @@ fn transformed_output_size(width: u32, height: u32, transform: wl_output::Transf } } +impl WaylandState { + pub(in crate::backend::wayland) fn desktop_backdrop_geometry( + &self, + ) -> Option { + let output = self.surface.current_output()?; + let active_info = self.output_state.info(&output)?; + let active = desktop_backdrop_output_geometry_from_info(&active_info)?; + let mut outputs = Vec::new(); + for output in self.output_state.outputs() { + let info = self.output_state.info(&output)?; + outputs.push(desktop_backdrop_output_geometry_from_info(&info)?); + } + + DesktopBackdropGeometry::from_outputs(active, &outputs, active_info.scale_factor.max(1)) + } + + pub(in crate::backend::wayland) fn set_freeze_zoom_geometry( + &mut self, + geometry: Option, + ) { + let screenshot_origin = self + .desktop_backdrop_geometry() + .and_then(DesktopBackdropGeometry::physical_origin); + let geometry = geometry.map(|geo| geo.with_screenshot_origin(screenshot_origin)); + self.frozen.set_active_geometry(geometry.clone()); + self.zoom.set_active_geometry(geometry); + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/backend/wayland/state/capture/pdf.rs b/src/backend/wayland/state/capture/pdf.rs index f6af30cd..f8b1111e 100644 --- a/src/backend/wayland/state/capture/pdf.rs +++ b/src/backend/wayland/state/capture/pdf.rs @@ -1,5 +1,4 @@ use super::super::*; -use super::backdrop::desktop_backdrop_output_geometry_from_info; use crate::input::state::{Toast, ToastPriority}; impl WaylandState { @@ -203,17 +202,4 @@ impl WaylandState { operation, } } - - fn desktop_backdrop_geometry(&self) -> Option { - let output = self.surface.current_output()?; - let active_info = self.output_state.info(&output)?; - let active = desktop_backdrop_output_geometry_from_info(&active_info)?; - let mut outputs = Vec::new(); - for output in self.output_state.outputs() { - let info = self.output_state.info(&output)?; - outputs.push(desktop_backdrop_output_geometry_from_info(&info)?); - } - - DesktopBackdropGeometry::from_outputs(active, &outputs, active_info.scale_factor.max(1)) - } } diff --git a/src/backend/wayland/zoom/portal.rs b/src/backend/wayland/zoom/portal.rs index f7c7e453..4fd87568 100644 --- a/src/backend/wayland/zoom/portal.rs +++ b/src/backend/wayland/zoom/portal.rs @@ -47,25 +47,22 @@ impl ZoomState { if let Some(geo) = geo { let (phys_w, phys_h) = geo.physical_size(); - let (origin_x, origin_y) = geo.physical_origin(); - if origin_x >= 0 - && origin_y >= 0 - && phys_w > 0 - && phys_h > 0 - && let Some((cropped_w, cropped_h, cropped)) = crop_argb( - &data, - width, - height, - origin_x as u32, - origin_y as u32, - phys_w, - phys_h, - ) - { - data = cropped; - width = cropped_w; - height = cropped_h; + let (origin_x, origin_y) = geo.portal_crop_origin(); + let Some((cropped_w, cropped_h, cropped)) = + crop_argb(&data, width, height, origin_x, origin_y, phys_w, phys_h) + else { + return Err(CaptureError::ImageError( + "Zoom capture does not contain the active output".to_string(), + )); + }; + if cropped_w != phys_w || cropped_h != phys_h { + return Err(CaptureError::ImageError( + "Zoom capture does not contain the active output".to_string(), + )); } + data = cropped; + width = cropped_w; + height = cropped_h; } Ok(( diff --git a/src/capture/tests/desktop_backdrop.rs b/src/capture/tests/desktop_backdrop.rs index 358f20c4..b06a91ed 100644 --- a/src/capture/tests/desktop_backdrop.rs +++ b/src/capture/tests/desktop_backdrop.rs @@ -126,6 +126,15 @@ fn desktop_backdrop_crops_mixed_scale_output_using_screenshot_origin() { ); } +#[test] +fn desktop_backdrop_single_output_at_nonzero_logical_origin_crops_at_zero() { + let outputs = [output(10, 20, 4, 2, 8, 4)]; + let geometry = + DesktopBackdropGeometry::from_outputs(outputs[0], &outputs, 2).expect("single output"); + + assert_eq!(geometry.physical_origin(), Some((0, 0))); +} + #[test] fn desktop_backdrop_normalizes_negative_output_origins() { let outputs = [output(-2, 0, 2, 2, 2, 2), output(0, 0, 3, 2, 3, 2)]; From 25d481e4d552458dd00a81097c7a9c24be591b0d Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Fri, 14 Aug 2026 23:46:43 +0200 Subject: [PATCH 04/24] fix: hit-test one-point pressure strokes as painted cap circles The renderer fills a circle at each stylus sample, but FreehandPressure hit testing only walked segments, so a single-point tap could not be selected. --- src/input/hit_test/mod.rs | 19 +------------------ src/input/hit_test/shapes.rs | 27 +++++++++++++++++++++++++++ src/input/hit_test/tests.rs | 20 ++++++++++++++++++++ 3 files changed, 48 insertions(+), 18 deletions(-) diff --git a/src/input/hit_test/mod.rs b/src/input/hit_test/mod.rs index e127d20e..63718d90 100644 --- a/src/input/hit_test/mod.rs +++ b/src/input/hit_test/mod.rs @@ -30,24 +30,7 @@ pub fn hit_test(shape: &DrawnShape, point: (i32, i32), tolerance: f64) -> bool { shapes::freehand_hit(points, point, *thick, tolerance) } Shape::FreehandPressure { points, .. } => { - // Check hit against variable width segments - if points.is_empty() { - return false; - } - // Simplified hit test: check each segment against max thickness or per-segment thickness - // For precise hit testing, we'd check each segment with its specific width. - // Using max thickness is a safe over-approximation for now, or we can iterate. - - // Let's iterate segments for better precision - for i in 0..points.len() - 1 { - let (x1, y1, t1) = points[i]; - let (x2, y2, t2) = points[i + 1]; - let max_segment_thick = t1.max(t2) as f64; - if shapes::segment_hit(x1, y1, x2, y2, max_segment_thick, point, tolerance) { - return true; - } - } - false + shapes::freehand_pressure_hit(points, point, tolerance) } Shape::Line { x1, diff --git a/src/input/hit_test/shapes.rs b/src/input/hit_test/shapes.rs index ddd90cdc..024d4721 100644 --- a/src/input/hit_test/shapes.rs +++ b/src/input/hit_test/shapes.rs @@ -28,6 +28,33 @@ pub(super) fn freehand_hit( false } +pub(super) fn freehand_pressure_hit( + points: &[(i32, i32, f32)], + point: (i32, i32), + tolerance: f64, +) -> bool { + if points.is_empty() { + return false; + } + + for &(x, y, thick) in points { + let padded = tolerance.max(thick as f64 / 2.0); + if distance_point_to_point((x, y), point) <= padded { + return true; + } + } + + for window in points.windows(2) { + let (x1, y1, t1) = window[0]; + let (x2, y2, t2) = window[1]; + if segment_hit(x1, y1, x2, y2, t1.max(t2) as f64, point, tolerance) { + return true; + } + } + + false +} + pub(super) fn segment_hit( x1: i32, y1: i32, diff --git a/src/input/hit_test/tests.rs b/src/input/hit_test/tests.rs index f43cf5eb..7a6db9ce 100644 --- a/src/input/hit_test/tests.rs +++ b/src/input/hit_test/tests.rs @@ -368,3 +368,23 @@ fn rect_outline_hit_treats_a_zero_rect_as_a_point() { assert!(shapes::rect_outline_hit(10, 20, 0, 0, 2.0, (11, 21), 3.0)); assert!(!shapes::rect_outline_hit(10, 20, 0, 0, 2.0, (30, 40), 3.0)); } + +#[test] +fn pressure_stroke_hit_includes_one_point_stylus_dots() { + let dot = DrawnShape::with_metadata( + 1, + Shape::FreehandPressure { + points: vec![(50, 50, 20.0)], + color: BLACK, + }, + 0, + false, + ); + + assert!( + hit_test(&dot, (50, 50), 1.0), + "a one-point pressure stroke paints a cap circle and must be selectable" + ); + assert!(hit_test(&dot, (55, 50), 1.0)); + assert!(!hit_test(&dot, (80, 80), 1.0)); +} From b6e86b7098187989805bcd07b71620d50022384f Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Fri, 14 Aug 2026 23:53:51 +0200 Subject: [PATCH 05/24] fix: keep capture and export saves inside the target directory Filename templates and format strings were joined onto the save directory, so a value with / or .. could write outside it. Require a single path component, allowlist the extension, and reject the same escapes in the configurator. --- config.example.toml | 5 +- configurator/src/models/config/tests.rs | 17 ++++ .../src/models/config/to_config/capture.rs | 20 ++++- .../src/models/config/to_config/export.rs | 37 ++++++-- docs/CONFIG.md | 6 +- src/capture/file.rs | 87 +++++++++++++++++-- src/config/mod.rs | 3 +- src/config/tests/validate.rs | 19 ++++ src/config/types/capture.rs | 41 +++++++++ src/config/types/mod.rs | 5 +- src/config/validate/capture.rs | 17 ++++ src/config/validate/export.rs | 25 +++++- src/config/validate/mod.rs | 2 + src/paths/mod.rs | 24 +++++ src/paths/tests.rs | 14 +++ 15 files changed, 299 insertions(+), 23 deletions(-) create mode 100644 src/config/validate/capture.rs diff --git a/config.example.toml b/config.example.toml index fad4f761..93741d6c 100644 --- a/config.example.toml +++ b/config.example.toml @@ -1381,10 +1381,11 @@ enabled = true # Directory where PNG files are saved (supports ~ expansion) save_directory = "~/Pictures/Wayscriber" -# Filename template (strftime-like subset: %Y, %m, %d, %H, %M, %S) +# Filename template (strftime-like subset: %Y, %m, %d, %H, %M, %S). +# Must be a single file name, not a path. filename_template = "screenshot_%Y-%m-%d_%H%M%S" -# Image format for saved screenshots +# Image format for saved screenshots: png, jpg, or jpeg format = "png" # Copy screenshots to clipboard by default diff --git a/configurator/src/models/config/tests.rs b/configurator/src/models/config/tests.rs index d0175bfb..d6abc730 100644 --- a/configurator/src/models/config/tests.rs +++ b/configurator/src/models/config/tests.rs @@ -89,6 +89,23 @@ fn config_draft_to_config_reports_errors() { assert!(fields.contains(&"drawing.default_color")); } +#[test] +fn config_draft_rejects_path_escaping_save_names() { + let mut draft = ConfigDraft::from_config(&Config::default()); + draft.capture_filename_template = "../evil_%Y".to_string(); + draft.capture_format = "png/../../x".to_string(); + draft.export_pdf_filename_template = "foo/bar".to_string(); + + let errors = draft + .to_config(&Config::default()) + .expect_err("path-escaping save names must not save"); + let fields: Vec<&str> = errors.iter().map(|err| err.field.as_str()).collect(); + + assert!(fields.contains(&"capture.filename_template")); + assert!(fields.contains(&"capture.format")); + assert!(fields.contains(&"export.pdf.filename_template")); +} + #[test] fn sparse_configurator_no_op_save_remains_byte_for_byte_sparse() { let temp = crate::test_temp::tempdir().expect("create temp directory"); diff --git a/configurator/src/models/config/to_config/capture.rs b/configurator/src/models/config/to_config/capture.rs index bdb80e68..d9e65aef 100644 --- a/configurator/src/models/config/to_config/capture.rs +++ b/configurator/src/models/config/to_config/capture.rs @@ -1,13 +1,27 @@ use super::super::draft::ConfigDraft; use crate::models::error::FormError; -use wayscriber::config::{Config, validate_ocr_languages}; +use wayscriber::config::{ + Config, validate_capture_format, validate_filename_template, validate_ocr_languages, +}; impl ConfigDraft { pub(super) fn apply_capture(&self, config: &mut Config, errors: &mut Vec) { config.capture.enabled = self.capture_enabled; config.capture.save_directory = self.capture_save_directory.clone(); - config.capture.filename_template = self.capture_filename_template.clone(); - config.capture.format = self.capture_format.clone(); + match validate_filename_template(&self.capture_filename_template) { + Ok(()) => config.capture.filename_template = self.capture_filename_template.clone(), + Err(reason) => errors.push(FormError::new( + "capture.filename_template", + format!("Filename template: {reason}."), + )), + } + match validate_capture_format(&self.capture_format) { + Ok(format) => config.capture.format = format, + Err(reason) => errors.push(FormError::new( + "capture.format", + format!("Image format: {reason}."), + )), + } config.capture.copy_to_clipboard = self.capture_copy_to_clipboard; config.capture.exit_after_capture = self.capture_exit_after; match validate_ocr_languages(&self.capture_ocr_languages) { diff --git a/configurator/src/models/config/to_config/export.rs b/configurator/src/models/config/to_config/export.rs index 1a34daff..4a9c45d2 100644 --- a/configurator/src/models/config/to_config/export.rs +++ b/configurator/src/models/config/to_config/export.rs @@ -1,13 +1,22 @@ use super::super::draft::ConfigDraft; use crate::models::error::FormError; use crate::models::util::parse_f64; -use wayscriber::config::{Config, PdfLabelContentMode, validate_pdf_label_template}; +use wayscriber::config::{ + Config, PdfLabelContentMode, validate_filename_template, validate_pdf_label_template, +}; impl ConfigDraft { pub(super) fn apply_export(&self, config: &mut Config, errors: &mut Vec) { - config.export.pdf.filename_template = non_empty(self.export_pdf_filename_template.clone()); - config.export.pdf.all_boards_filename_template = - non_empty(self.export_pdf_all_boards_filename_template.clone()); + config.export.pdf.filename_template = optional_filename_template( + &self.export_pdf_filename_template, + "export.pdf.filename_template", + errors, + ); + config.export.pdf.all_boards_filename_template = optional_filename_template( + &self.export_pdf_all_boards_filename_template, + "export.pdf.all_boards_filename_template", + errors, + ); config.export.pdf.page_size = self.export_pdf_page_size.to_config(); config.export.pdf.orientation = self.export_pdf_orientation.to_config(); config.export.pdf.fit = self.export_pdf_fit.to_config(); @@ -105,9 +114,25 @@ impl ConfigDraft { } } -fn non_empty(value: String) -> Option { +fn optional_filename_template( + value: &str, + field: &'static str, + errors: &mut Vec, +) -> Option { let trimmed = value.trim(); - (!trimmed.is_empty()).then(|| trimmed.to_string()) + if trimmed.is_empty() { + return None; + } + match validate_filename_template(trimmed) { + Ok(()) => Some(trimmed.to_string()), + Err(reason) => { + errors.push(FormError::new( + field, + format!("Filename template: {reason}."), + )); + None + } + } } fn validate_pdf_label_template_for_content( diff --git a/docs/CONFIG.md b/docs/CONFIG.md index a9720ea9..8a725927 100644 --- a/docs/CONFIG.md +++ b/docs/CONFIG.md @@ -1377,10 +1377,11 @@ enabled = true # Directory for saved screenshots (supports ~ expansion) save_directory = "~/Pictures/Wayscriber" -# Filename template (strftime-like subset: %Y, %m, %d, %H, %M, %S) +# Filename template (strftime-like subset: %Y, %m, %d, %H, %M, %S). +# Must be a single file name, not a path. filename_template = "screenshot_%Y-%m-%d_%H%M%S" -# Image format (currently "png") +# Image format: png, jpg, or jpeg format = "png" # Copy captures to clipboard in addition to saving files @@ -1399,6 +1400,7 @@ ocr_languages = "eng" **Tips:** - Set `copy_to_clipboard = false` if you prefer file-only captures. - Clipboard-only shortcuts ignore the save directory automatically. +- `filename_template` must be a single file name (no `/` or `..`). `format` is `png`, `jpg`, or `jpeg`. - `wl-clipboard`, `grim`, and `slurp` are installed automatically by deb/rpm/AUR packages. For source/tarball installs, add them manually; otherwise wayscriber falls back to `xdg-desktop-portal`. #### Copy text from screen (OCR) diff --git a/src/capture/file.rs b/src/capture/file.rs index ff82216e..235384a2 100644 --- a/src/capture/file.rs +++ b/src/capture/file.rs @@ -45,25 +45,71 @@ pub fn generate_filename(template: &str, format: &str) -> String { format!("{}.{}", filename, format) } +fn sanitize_save_extension(format: &str) -> Option { + let normalized = format.trim().to_ascii_lowercase(); + matches!(normalized.as_str(), "png" | "jpg" | "jpeg" | "pdf").then_some(normalized) +} + +fn save_file_name(template: &str, format: &str) -> Result { + let stem = format_with_template(now_local(), template); + let extension = sanitize_save_extension(format).ok_or_else(|| { + CaptureError::SaveError(std::io::Error::other("unsupported screenshot file format")) + })?; + if !crate::paths::is_single_path_component(&stem) { + return Err(CaptureError::SaveError(std::io::Error::other( + "filename template must expand to a single file name", + ))); + } + let filename = format!("{stem}.{extension}"); + if !crate::paths::is_single_path_component(&filename) { + return Err(CaptureError::SaveError(std::io::Error::other( + "filename template must expand to a single file name", + ))); + } + Ok(filename) +} + const UNIQUE_NAME_ATTEMPTS: u32 = 100; -fn generate_file_path(directory: &Path, template: &str, format: &str) -> PathBuf { - let filename = generate_filename(template, format); +fn generate_file_path( + directory: &Path, + template: &str, + format: &str, +) -> Result { + let filename = save_file_name(template, format)?; let base = Path::new(&filename) .file_stem() .and_then(|stem| stem.to_str()) - .unwrap_or("screenshot"); + .ok_or_else(|| { + CaptureError::SaveError(std::io::Error::other( + "filename template must expand to a single file name", + )) + })?; + let extension = Path::new(&filename) + .extension() + .and_then(|ext| ext.to_str()) + .ok_or_else(|| { + CaptureError::SaveError(std::io::Error::other("unsupported screenshot file format")) + })?; let mut path = directory.join(&filename); + if path.parent() != Some(directory) { + return Err(CaptureError::SaveError(std::io::Error::other( + "filename template must expand to a single file name", + ))); + } if path.exists() { for suffix in 1..=UNIQUE_NAME_ATTEMPTS { - let candidate = directory.join(format!("{}-{}.{}", base, suffix, format)); + let candidate = directory.join(format!("{base}-{suffix}.{extension}")); + if candidate.parent() != Some(directory) { + continue; + } if !candidate.exists() { log::info!( "Screenshot filename exists; using {} instead", candidate.display() ); path = candidate; - return path; + return Ok(path); } } log::warn!( @@ -71,7 +117,7 @@ fn generate_file_path(directory: &Path, template: &str, format: &str) -> PathBuf path.display() ); } - path + Ok(path) } /// Ensure the save directory exists, creating it if necessary. @@ -111,7 +157,7 @@ pub fn save_screenshot( let directory = ensure_directory_exists(&config.save_directory)?; // Generate filename - let file_path = generate_file_path(&directory, &config.filename_template, &config.format); + let file_path = generate_file_path(&directory, &config.filename_template, &config.format)?; log::info!( "Saving screenshot to: {} ({} bytes)", @@ -190,4 +236,31 @@ mod tests { assert!(target.exists()); assert_eq!(resolved, target.canonicalize().unwrap()); } + + #[test] + fn save_file_name_rejects_path_escapes_and_unknown_formats() { + for template in ["../evil", "foo/bar", "/tmp/x", "..", ".", ""] { + assert!( + save_file_name(template, "png").is_err(), + "{template:?} must stay inside the save directory" + ); + } + assert!(save_file_name("shot", "png/../../x").is_err()); + assert!(save_file_name("shot", "exe").is_err()); + assert_eq!(save_file_name("shot", "png").unwrap(), "shot.png"); + assert_eq!(save_file_name("shot", "JPEG").unwrap(), "shot.jpeg"); + } + + #[test] + fn generate_file_path_stays_inside_the_save_directory() { + let temp = crate::test_temp::tempdir().unwrap(); + let directory = temp.path(); + let path = generate_file_path(directory, "shot", "png").expect("safe name"); + assert_eq!(path.parent(), Some(directory)); + assert_eq!( + path.file_name().and_then(|name| name.to_str()), + Some("shot.png") + ); + assert!(generate_file_path(directory, "../evil", "png").is_err()); + } } diff --git a/src/config/mod.rs b/src/config/mod.rs index 502d8158..18588ee6 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -79,7 +79,8 @@ pub use types::{ UiConfig, UpdatesConfig, ZoomChipDisplay, default_quick_color_for_index, fold_legacy_section_flags, resolve_section_visibility, section_flag_for_item, set_section_visibility, toolbar_item_definitions, toolbar_item_ids, toolbar_item_order_group, - validate_ocr_languages, validate_pdf_label_template, + validate_capture_format, validate_filename_template, validate_ocr_languages, + validate_pdf_label_template, }; #[cfg(feature = "tablet-input")] #[allow(unused_imports)] diff --git a/src/config/tests/validate.rs b/src/config/tests/validate.rs index 5c9d8e97..483eb8e6 100644 --- a/src/config/tests/validate.rs +++ b/src/config/tests/validate.rs @@ -416,6 +416,25 @@ fn validate_export_pdf_sanitizes_numbers_colors_and_bad_templates() { ); } +#[test] +fn validate_and_clamp_rejects_path_escaping_save_names() { + let mut config = Config::default(); + config.capture.filename_template = "../evil_%Y".to_string(); + config.capture.format = "png/../../x".to_string(); + config.export.pdf.filename_template = Some("foo/bar".to_string()); + config.export.pdf.all_boards_filename_template = Some("/tmp/x".to_string()); + + config.validate_and_clamp(); + + assert_eq!( + config.capture.filename_template, + Config::default().capture.filename_template + ); + assert_eq!(config.capture.format, "png"); + assert_eq!(config.export.pdf.filename_template, None); + assert_eq!(config.export.pdf.all_boards_filename_template, None); +} + #[test] fn validate_export_pdf_ignores_template_when_label_content_is_not_custom() { let mut config = Config::default(); diff --git a/src/config/types/capture.rs b/src/config/types/capture.rs index f6cf910e..78f39390 100644 --- a/src/config/types/capture.rs +++ b/src/config/types/capture.rs @@ -105,6 +105,27 @@ pub fn validate_ocr_languages(raw: &str) -> Result { Ok(tokens.join("+")) } +/// Filename templates must expand to a file name inside the save directory. +pub fn validate_filename_template(template: &str) -> Result<(), String> { + let trimmed = template.trim(); + if trimmed.is_empty() { + return Err("value is empty".to_string()); + } + if !crate::paths::is_single_path_component(trimmed) { + return Err("must be a single file name, not a path".to_string()); + } + Ok(()) +} + +/// Screenshot save format. PDF exports use a separate extension at save time. +pub fn validate_capture_format(format: &str) -> Result { + let normalized = format.trim().to_ascii_lowercase(); + match normalized.as_str() { + "png" | "jpg" | "jpeg" => Ok(normalized), + _ => Err("must be png, jpg, or jpeg".to_string()), + } +} + fn default_capture_ocr_languages() -> String { DEFAULT_OCR_LANGUAGES.to_string() } @@ -181,4 +202,24 @@ mod tests { assert_eq!(config.resolved_ocr_languages(), DEFAULT_OCR_LANGUAGES); } + + #[test] + fn filename_template_must_be_a_single_component() { + validate_filename_template("screenshot_%Y-%m-%d_%H%M%S").unwrap(); + for template in ["", " ", "../evil", "foo/bar", "/tmp/x"] { + assert!( + validate_filename_template(template).is_err(), + "{template:?} must be rejected" + ); + } + } + + #[test] + fn capture_format_is_allowlisted() { + assert_eq!(validate_capture_format(" PNG ").unwrap(), "png"); + assert_eq!(validate_capture_format("jpeg").unwrap(), "jpeg"); + assert!(validate_capture_format("pdf").is_err()); + assert!(validate_capture_format("png/../../x").is_err()); + assert!(validate_capture_format("exe").is_err()); + } } diff --git a/src/config/types/mod.rs b/src/config/types/mod.rs index 4ed016df..c270cd75 100644 --- a/src/config/types/mod.rs +++ b/src/config/types/mod.rs @@ -28,7 +28,10 @@ mod updates; pub use arrow::ArrowConfig; pub use board::BoardConfig; pub use boards::{BoardBackgroundConfig, BoardColorConfig, BoardItemConfig, BoardsConfig}; -pub use capture::{CaptureConfig, DEFAULT_OCR_LANGUAGES, validate_ocr_languages}; +pub use capture::{ + CaptureConfig, DEFAULT_OCR_LANGUAGES, validate_capture_format, validate_filename_template, + validate_ocr_languages, +}; pub use click_highlight::ClickHighlightConfig; pub use context_menu::ContextMenuUiConfig; pub use drawing::{ diff --git a/src/config/validate/capture.rs b/src/config/validate/capture.rs new file mode 100644 index 00000000..08ab19f2 --- /dev/null +++ b/src/config/validate/capture.rs @@ -0,0 +1,17 @@ +use super::super::{CaptureConfig, Config, validate_capture_format, validate_filename_template}; + +impl Config { + pub(super) fn validate_capture(&mut self) { + if let Err(reason) = validate_filename_template(&self.capture.filename_template) { + log::warn!("Invalid capture.filename_template ({reason}); resetting to default"); + self.capture.filename_template = CaptureConfig::default().filename_template; + } + match validate_capture_format(&self.capture.format) { + Ok(format) => self.capture.format = format, + Err(reason) => { + log::warn!("Invalid capture.format ({reason}); resetting to png"); + self.capture.format = CaptureConfig::default().format; + } + } + } +} diff --git a/src/config/validate/export.rs b/src/config/validate/export.rs index 6b3d4fb5..3ee4dd5a 100644 --- a/src/config/validate/export.rs +++ b/src/config/validate/export.rs @@ -1,6 +1,6 @@ use super::super::{ Config, PDF_LABEL_DEFAULT_TEMPLATE, PdfLabelConfig, PdfLabelContentMode, - validate_pdf_label_template, + validate_filename_template, validate_pdf_label_template, }; const PDF_DIMENSION_MIN: f64 = 1.0; @@ -35,6 +35,29 @@ impl Config { "export.pdf.content_source_padding", ); validate_pdf_labels(&mut self.export.pdf.labels); + sanitize_optional_filename_template( + &mut self.export.pdf.filename_template, + "export.pdf.filename_template", + ); + sanitize_optional_filename_template( + &mut self.export.pdf.all_boards_filename_template, + "export.pdf.all_boards_filename_template", + ); + } +} + +fn sanitize_optional_filename_template(template: &mut Option, path: &str) { + let Some(value) = template.as_deref() else { + return; + }; + let trimmed = value.trim(); + if trimmed.is_empty() { + *template = None; + return; + } + if let Err(reason) = validate_filename_template(trimmed) { + log::warn!("Invalid {path} ({reason}); ignoring so capture.filename_template is used"); + *template = None; } } diff --git a/src/config/validate/mod.rs b/src/config/validate/mod.rs index 87c331fe..8fc36c17 100644 --- a/src/config/validate/mod.rs +++ b/src/config/validate/mod.rs @@ -3,6 +3,7 @@ use super::Config; mod arrow; mod board; mod boards; +mod capture; mod drawing; mod export; mod fonts; @@ -81,6 +82,7 @@ impl Config { self.validate_ui(); self.validate_render_profiles(); self.validate_export(); + self.validate_capture(); let keybindings = self.validate_keybindings(); self.validate_session(); self.validate_updates(); diff --git a/src/paths/mod.rs b/src/paths/mod.rs index 500c2e63..411655ce 100644 --- a/src/paths/mod.rs +++ b/src/paths/mod.rs @@ -82,6 +82,30 @@ pub fn expand_tilde(path: &str) -> PathBuf { PathBuf::from(path) } +/// True when `name` is a single relative file name, not a path. +/// +/// Rejects empty names, `.`, `..`, NULs, slashes, and anything `Path::join` +/// would treat as leaving or replacing the parent directory. +pub fn is_single_path_component(name: &str) -> bool { + if name.is_empty() || name.contains('\0') || name.contains('/') || name.contains('\\') { + return false; + } + let path = std::path::Path::new(name); + if path.is_absolute() { + return false; + } + let mut components = path.components(); + match components.next() { + Some(std::path::Component::Normal(os_name)) => { + components.next().is_none() + && os_name.to_str().is_some_and(|component| { + component == name && component != "." && component != ".." + }) + } + _ => false, + } +} + fn fallback_runtime_root() -> PathBuf { std::env::temp_dir().join("wayscriber") } diff --git a/src/paths/tests.rs b/src/paths/tests.rs index ce2d5fcc..b790558b 100644 --- a/src/paths/tests.rs +++ b/src/paths/tests.rs @@ -301,3 +301,17 @@ fn expand_tilde_replaces_home() { None => unsafe { env::remove_var(HOME_ENV) }, } } + +#[test] +fn is_single_path_component_rejects_path_escapes() { + assert!(is_single_path_component("screenshot_%Y-%m-%d_%H%M%S")); + assert!(is_single_path_component("shot.png")); + assert!(!is_single_path_component("")); + assert!(!is_single_path_component(".")); + assert!(!is_single_path_component("..")); + assert!(!is_single_path_component("../evil")); + assert!(!is_single_path_component("foo/bar")); + assert!(!is_single_path_component("/abs")); + assert!(!is_single_path_component("foo\\bar")); + assert!(!is_single_path_component("foo\0bar")); +} From 7aef05464682a466b750cb49c2ef2c266b97bfaf Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Fri, 14 Aug 2026 23:57:18 +0200 Subject: [PATCH 06/24] fix: confine portal screenshot reads to trusted temp files Portal capture accepted any file:// URI, followed symlinks, and deleted the target. Read only regular files under $XDG_RUNTIME_DIR or the portal cache, with O_NOFOLLOW and a 256 MiB cap. --- src/capture/sources/reader.rs | 303 ++++++++++++++++++++++++++++------ 1 file changed, 253 insertions(+), 50 deletions(-) diff --git a/src/capture/sources/reader.rs b/src/capture/sources/reader.rs index 1046ce30..412d33e1 100644 --- a/src/capture/sources/reader.rs +++ b/src/capture/sources/reader.rs @@ -1,69 +1,187 @@ #![cfg(feature = "portal")] use crate::capture::types::CaptureError; +use crate::env_vars::XDG_RUNTIME_DIR_ENV; use crate::file_uri; -use std::path::PathBuf; -use std::{fs, thread, time::Duration}; +use std::fs::{self, File}; +use std::io::Read; +use std::path::{Path, PathBuf}; +use std::{thread, time::Duration}; -/// Read image data from a file:// URI. +const MAX_PORTAL_IMAGE_BYTES: u64 = 256 * 1024 * 1024; +const MAX_ATTEMPTS: usize = 60; +const ATTEMPT_DELAY_MS: u64 = 50; + +/// Read image data from a portal `file://` URI. /// -/// This properly decodes percent-encoded URIs (spaces, non-ASCII characters, etc.) -/// and cleans up the temporary file after reading. +/// The path must live under `$XDG_RUNTIME_DIR` or the xdg-desktop-portal cache +/// directory. The file is opened without following symlinks, must be a regular +/// file, and is deleted only after those checks pass. pub fn read_image_from_uri(uri: &str) -> Result, CaptureError> { + read_image_from_uri_with_limit(uri, MAX_PORTAL_IMAGE_BYTES) +} + +fn read_image_from_uri_with_limit(uri: &str, max_bytes: u64) -> Result, CaptureError> { let path = decode_file_uri(uri)?; + if !is_allowed_portal_path(&path) { + return Err(CaptureError::InvalidResponse( + "Portal screenshot path is not in a trusted directory".to_string(), + )); + } log::debug!("Reading screenshot from the portal temporary file"); - // Wait briefly for portal to flush the file to disk (some portals write asynchronously) - const MAX_ATTEMPTS: usize = 60; // up to 3 seconds total - const ATTEMPT_DELAY_MS: u64 = 50; - - let mut data = Vec::new(); + let mut opened = None; for attempt in 0..MAX_ATTEMPTS { - match fs::read(&path) { - Ok(bytes) if !bytes.is_empty() => { - data = bytes; + match open_portal_image(&path, max_bytes) { + Ok(file) => { + opened = Some(file); break; } - Ok(_) => { - log::trace!( - "Portal screenshot file still empty (attempt {}/{})", - attempt + 1, - MAX_ATTEMPTS - ); + Err(PortalOpenError::Rejected { message, delete }) => { + if delete { + remove_allowed_portal_file(&path); + } + return Err(CaptureError::ImageError(message)); + } + Err(PortalOpenError::NotReady(err)) if attempt + 1 == MAX_ATTEMPTS => { + return Err(CaptureError::ImageError(format!( + "Portal screenshot file was not ready after {MAX_ATTEMPTS} attempts: {err}" + ))); } - Err(e) => { + Err(PortalOpenError::NotReady(err)) => { log::trace!( - "Portal screenshot file not ready yet (attempt {}/{}): {}", - attempt + 1, - MAX_ATTEMPTS, - e + "Portal screenshot file not ready yet (attempt {}/{MAX_ATTEMPTS}): {err}", + attempt + 1 ); } } - - if attempt + 1 == MAX_ATTEMPTS { - return Err(CaptureError::ImageError(format!( - "Portal screenshot file was not ready after {MAX_ATTEMPTS} attempts" - ))); - } - thread::sleep(Duration::from_millis(ATTEMPT_DELAY_MS)); } + let file = opened.ok_or_else(|| { + CaptureError::ImageError("Portal screenshot file was not ready".to_string()) + })?; + let mut data = Vec::new(); + file.take(max_bytes.saturating_add(1)) + .read_to_end(&mut data) + .map_err(|err| { + CaptureError::ImageError(format!("Failed to read portal screenshot: {err}")) + })?; + if data.is_empty() { + return Err(CaptureError::ImageError( + "Portal screenshot file was empty".to_string(), + )); + } + if data.len() as u64 > max_bytes { + remove_allowed_portal_file(&path); + return Err(CaptureError::ImageError( + "Portal screenshot file exceeds the size limit".to_string(), + )); + } + log::info!( "Successfully read {} bytes from portal screenshot", data.len() ); + remove_allowed_portal_file(&path); + Ok(data) +} - // Clean up portal temp file to prevent accumulation - if let Err(e) = fs::remove_file(&path) { - log::warn!("Failed to remove portal screenshot temporary file: {e}"); +fn remove_allowed_portal_file(path: &Path) { + if let Err(err) = fs::remove_file(path) { + log::warn!("Failed to remove portal screenshot temporary file: {err}"); } else { log::debug!("Removed portal screenshot temporary file"); } +} - Ok(data) +fn is_allowed_portal_path(path: &Path) -> bool { + let Some(name) = path.file_name().and_then(|name| name.to_str()) else { + return false; + }; + if !crate::paths::is_single_path_component(name) { + return false; + } + let Some(parent) = path.parent() else { + return false; + }; + let Ok(parent) = parent.canonicalize() else { + return false; + }; + allowed_portal_roots().into_iter().any(|root| { + root.canonicalize() + .is_ok_and(|root| parent.starts_with(root)) + }) +} + +fn allowed_portal_roots() -> Vec { + let mut roots = Vec::new(); + if let Some(runtime) = std::env::var_os(XDG_RUNTIME_DIR_ENV) + && !runtime.is_empty() + { + roots.push(PathBuf::from(runtime)); + } + if let Some(cache) = crate::paths::cache_dir() { + roots.push(cache.join("xdg-desktop-portal")); + } + roots +} + +enum PortalOpenError { + NotReady(String), + Rejected { message: String, delete: bool }, +} + +fn open_portal_image(path: &Path, max_bytes: u64) -> Result { + let file = open_portal_file(path).map_err(|err| { + if err.kind() == std::io::ErrorKind::NotFound { + PortalOpenError::NotReady(err.to_string()) + } else { + PortalOpenError::Rejected { + message: err.to_string(), + delete: false, + } + } + })?; + let metadata = file.metadata().map_err(|err| PortalOpenError::Rejected { + message: err.to_string(), + delete: false, + })?; + if !metadata.file_type().is_file() { + return Err(PortalOpenError::Rejected { + message: "portal screenshot is not a regular file".to_string(), + delete: false, + }); + } + if metadata.len() > max_bytes { + return Err(PortalOpenError::Rejected { + message: "portal screenshot file exceeds the size limit".to_string(), + delete: true, + }); + } + if metadata.len() == 0 { + return Err(PortalOpenError::NotReady( + "portal screenshot file is empty".to_string(), + )); + } + Ok(file) +} + +#[cfg(unix)] +fn open_portal_file(path: &Path) -> std::io::Result { + use std::fs::OpenOptions; + use std::os::unix::fs::OpenOptionsExt; + + OpenOptions::new() + .read(true) + .custom_flags(libc::O_NOFOLLOW | libc::O_CLOEXEC | libc::O_NONBLOCK) + .open(path) +} + +#[cfg(not(unix))] +fn open_portal_file(path: &Path) -> std::io::Result { + File::open(path) } fn decode_file_uri(uri: &str) -> Result { @@ -73,27 +191,99 @@ fn decode_file_uri(uri: &str) -> Result { #[cfg(test)] mod tests { use super::*; + use crate::env_vars::XDG_RUNTIME_DIR_ENV; use crate::test_temp::TempDir; - #[test] - fn reads_and_removes_file() { - let temp = TempDir::new().unwrap(); - let file_path = temp.path().join("capture file.png"); - std::fs::write(&file_path, b"portal-bytes").unwrap(); - let uri = format!( + fn file_uri_for(path: &Path) -> String { + format!( "file://{}", - file_path - .to_string_lossy() + path.to_string_lossy() .replace('%', "%25") .replace(' ', "%20") - ); + ) + } + + fn with_runtime_dir(dir: &Path, body: impl FnOnce() -> T) -> T { + crate::test_env::with_env_var(XDG_RUNTIME_DIR_ENV, Some(dir.as_os_str()), body) + } + + #[test] + fn reads_and_removes_file() { + let temp = TempDir::new().unwrap(); + with_runtime_dir(temp.path(), || { + let file_path = temp.path().join("capture file.png"); + std::fs::write(&file_path, b"portal-bytes").unwrap(); + + let data = read_image_from_uri(&file_uri_for(&file_path)).expect("read succeeds"); + assert_eq!(data, b"portal-bytes"); + assert!( + !file_path.exists(), + "read_image_from_uri should delete the portal temp file" + ); + }); + } + + #[test] + fn rejects_paths_outside_trusted_directories() { + let temp = TempDir::new().unwrap(); + let outside = temp.path().join("outside.png"); + std::fs::write(&outside, b"secret").unwrap(); + let trusted = temp.path().join("runtime"); + std::fs::create_dir(&trusted).unwrap(); - let data = read_image_from_uri(&uri).expect("read succeeds"); - assert_eq!(data, b"portal-bytes"); - assert!( - !file_path.exists(), - "read_image_from_uri should delete the portal temp file" - ); + with_runtime_dir(&trusted, || { + let err = read_image_from_uri(&file_uri_for(&outside)).expect_err("outside path"); + match err { + CaptureError::InvalidResponse(msg) => { + assert!(msg.contains("trusted directory"), "{msg}"); + } + other => panic!("unexpected error variant: {other:?}"), + } + assert!( + outside.exists(), + "rejected portal URIs must not delete the target file" + ); + }); + } + + #[test] + fn rejects_files_over_the_size_limit_and_still_deletes_the_temp() { + let temp = TempDir::new().unwrap(); + with_runtime_dir(temp.path(), || { + let file_path = temp.path().join("huge.png"); + std::fs::write(&file_path, b"12345").unwrap(); + let err = read_image_from_uri_with_limit(&file_uri_for(&file_path), 4) + .expect_err("oversize file"); + match err { + CaptureError::ImageError(msg) => { + assert!(msg.contains("size limit"), "{msg}"); + } + other => panic!("unexpected error variant: {other:?}"), + } + assert!( + !file_path.exists(), + "an oversized portal temp file should still be removed" + ); + }); + } + + #[test] + #[cfg(unix)] + fn rejects_symlinks_without_following_them() { + let temp = TempDir::new().unwrap(); + with_runtime_dir(temp.path(), || { + let target = temp.path().join("target.png"); + std::fs::write(&target, b"secret").unwrap(); + let link = temp.path().join("link.png"); + std::os::unix::fs::symlink(&target, &link).unwrap(); + + read_image_from_uri(&file_uri_for(&link)).expect_err("symlink"); + assert!(target.exists()); + assert!( + link.exists(), + "failed symlink reads must not delete the link" + ); + }); } #[test] @@ -104,4 +294,17 @@ mod tests { other => panic!("unexpected error variant: {other:?}"), } } + + #[test] + fn allowed_portal_path_requires_runtime_or_cache_root() { + let temp = TempDir::new().unwrap(); + let file_path = temp.path().join("shot.png"); + std::fs::write(&file_path, b"x").unwrap(); + with_runtime_dir(temp.path(), || { + assert!(is_allowed_portal_path(&file_path)); + }); + crate::test_env::with_env_var(XDG_RUNTIME_DIR_ENV, None, || { + assert!(!is_allowed_portal_path(&file_path)); + }); + } } From d625afeaa6fa5c49565caed9141bf71dceef211f Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Fri, 14 Aug 2026 23:58:42 +0200 Subject: [PATCH 07/24] fix: open clipboard file pastes without following symlinks URI-list paste used metadata() and a following open, so a symlink to a regular file was treated as the image. lstat and O_NOFOLLOW keep the paste on the named file. --- src/backend/wayland/clipboard/file_list.rs | 29 ++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/src/backend/wayland/clipboard/file_list.rs b/src/backend/wayland/clipboard/file_list.rs index 383cc7ce..592283d4 100644 --- a/src/backend/wayland/clipboard/file_list.rs +++ b/src/backend/wayland/clipboard/file_list.rs @@ -122,7 +122,7 @@ fn map_clipboard_file_read_error(path: &Path, err: ClipboardReadError) -> Clipbo } fn ensure_regular_clipboard_path(path: &Path) -> Result<(), ClipboardReadError> { - let metadata = fs::metadata(path).map_err(|err| { + let metadata = fs::symlink_metadata(path).map_err(|err| { ClipboardReadError::Other(format!( "Failed to inspect clipboard file {}: {}", path.display(), @@ -154,7 +154,7 @@ fn validate_clipboard_file_metadata( fn open_regular_clipboard_file(path: &Path) -> Result { let file = OpenOptions::new() .read(true) - .custom_flags(libc::O_NONBLOCK) + .custom_flags(libc::O_NOFOLLOW | libc::O_NONBLOCK) .open(path) .map_err(|err| { ClipboardReadError::Other(format!( @@ -280,6 +280,31 @@ mod tests { } } + #[test] + #[cfg(unix)] + fn uri_list_paste_rejects_symlink_without_following_it() { + let temp = TempDir::new().unwrap(); + let image_path = temp.path().join("cat.png"); + fs::write(&image_path, tiny_png()).unwrap(); + let link_path = temp.path().join("link.png"); + std::os::unix::fs::symlink(&image_path, &link_path).unwrap(); + let uri = file_uri_for_path(&link_path); + let offered = vec![TEXT_URI_LIST_MIME.to_string()]; + + let result = decode_clipboard_uri_list(TEXT_URI_LIST_MIME, uri.into_bytes(), offered); + + match result { + ClipboardPasteResult::DecodeFailed(err) => { + assert!( + err.contains("not a regular file") || err.contains("could not be read"), + "{err}" + ); + } + other => panic!("expected decode failure, got {other:?}"), + } + assert!(image_path.exists()); + } + #[test] fn uri_list_paste_treats_missing_file_as_decode_failure() { let temp = TempDir::new().unwrap(); From 0b346dd666626c001286a90184122642f613f725 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Sat, 15 Aug 2026 00:00:59 +0200 Subject: [PATCH 08/24] fix: refuse oversized config.toml loads Config loading read the whole file into memory. Cap both the typed load and the lossless document path at 2 MiB, matching the session snapshot size-guard pattern. --- src/config/document.rs | 6 +++++- src/config/io.rs | 18 ++++++++++++++++++ src/config/tests/document.rs | 11 +++++++++++ src/config/tests/load.rs | 15 +++++++++++++++ 4 files changed, 49 insertions(+), 1 deletion(-) diff --git a/src/config/document.rs b/src/config/document.rs index a9590ed8..c74f9eb7 100644 --- a/src/config/document.rs +++ b/src/config/document.rs @@ -5,7 +5,9 @@ mod merge; pub use lock::ConfigWriteLockTimeout; -use super::io::{create_config_backup, prepare_config_parent, write_config_text_atomic}; +use super::io::{ + create_config_backup, ensure_config_file_size, prepare_config_parent, write_config_text_atomic, +}; use super::keybindings::KeybindingAuthorship; use super::{Config, ConfigSource}; use crate::durable_io::{ @@ -171,8 +173,10 @@ impl SourceRevision { destination.display() ); } + ensure_config_file_size(metadata.len(), &destination)?; let bytes = fs::read(&destination) .with_context(|| format!("Failed to read config from {}", destination.display()))?; + ensure_config_file_size(bytes.len() as u64, &destination)?; Ok(Self::Present { bytes, identity: FileIdentity::of(&metadata), diff --git a/src/config/io.rs b/src/config/io.rs index ba0e906d..53edf875 100644 --- a/src/config/io.rs +++ b/src/config/io.rs @@ -17,6 +17,20 @@ use std::fs; use std::io::ErrorKind; use std::path::{Path, PathBuf}; +/// Config files larger than this are refused so a truncated or hostile +/// `config.toml` cannot be pulled entirely into memory. +pub(crate) const MAX_CONFIG_FILE_BYTES: u64 = 2 * 1024 * 1024; + +pub(crate) fn ensure_config_file_size(len: u64, path: &Path) -> Result<()> { + if len > MAX_CONFIG_FILE_BYTES { + bail!( + "Config file {} is {len} bytes; the maximum is {MAX_CONFIG_FILE_BYTES}", + path.display() + ); + } + Ok(()) +} + /// Represents the source used to load configuration data. #[derive(Debug, Clone)] pub enum ConfigSource { @@ -151,8 +165,12 @@ impl Config { /// The presence set is taken from the same text serde sees, so resolution /// can tell an authored shortcut from an offer (#293). fn read_unvalidated_from(config_path: &Path) -> Result<(Self, Vec)> { + let metadata = fs::metadata(config_path) + .with_context(|| format!("Failed to read config from {}", config_path.display()))?; + ensure_config_file_size(metadata.len(), config_path)?; let config_str = fs::read_to_string(config_path) .with_context(|| format!("Failed to read config from {}", config_path.display()))?; + ensure_config_file_size(config_str.len() as u64, config_path)?; let (mut config, section_errors) = match toml::from_str::(&config_str) { Ok(config) => (config, Vec::new()), diff --git a/src/config/tests/document.rs b/src/config/tests/document.rs index 6cbe7b8f..3b065997 100644 --- a/src/config/tests/document.rs +++ b/src/config/tests/document.rs @@ -41,6 +41,17 @@ fn diagnostic_paths(document: &ConfigDocument) -> Vec<&str> { .collect() } +#[test] +fn load_from_path_rejects_oversized_config_file() { + let temp = TempConfig::new("oversized"); + let oversized = vec![b'x'; (crate::config::io::MAX_CONFIG_FILE_BYTES as usize) + 1]; + fs::write(&temp.path, oversized).expect("write oversized config"); + + let err = ConfigDocument::load_from_path(&temp.path).expect_err("oversized config"); + let message = format!("{err:#}"); + assert!(message.contains("maximum"), "unexpected error: {message}"); +} + fn diagnostic_kinds(document: &ConfigDocument) -> Vec<(ConfigDiagnosticKind, &str)> { document .diagnostics() diff --git a/src/config/tests/load.rs b/src/config/tests/load.rs index fb2967a1..fe5f87a7 100644 --- a/src/config/tests/load.rs +++ b/src/config/tests/load.rs @@ -1319,3 +1319,18 @@ fn tablet_stylus_button_bindings_parse_custom_actions() { assert_eq!(config.tablet.stylus_button.action, Some(Action::Undo)); assert_eq!(config.tablet.stylus_button2.action, Some(Action::Redo)); } + +#[test] +fn load_rejects_oversized_config_file() { + with_temp_config_home(|config_root| { + let primary_dir = config_root.join(PRIMARY_CONFIG_DIR); + fs::create_dir_all(&primary_dir).unwrap(); + let config_path = primary_dir.join("config.toml"); + let oversized = vec![b'x'; (crate::config::io::MAX_CONFIG_FILE_BYTES as usize) + 1]; + fs::write(&config_path, oversized).unwrap(); + + let err = Config::load().expect_err("oversized config must not load"); + let message = format!("{err:#}"); + assert!(message.contains("maximum"), "unexpected error: {message}"); + }); +} From 111ad01b17dfdd4a4a30d70a7146267bae3e4ed2 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Sat, 15 Aug 2026 00:02:40 +0200 Subject: [PATCH 09/24] fix: preserve board pan settings when saving from the configurator BoardsDraft rebuilt BoardsConfig from defaults, so a save dropped pan_enabled and show_pan_badge. Keep those fields on the draft so an existing false value survives a round trip. --- configurator/src/models/config/boards.rs | 2 ++ .../src/models/config/boards/mapping.rs | 4 ++++ configurator/src/models/config/tests.rs | 20 +++++++++++++++++++ 3 files changed, 26 insertions(+) diff --git a/configurator/src/models/config/boards.rs b/configurator/src/models/config/boards.rs index d61a2ae1..05692e33 100644 --- a/configurator/src/models/config/boards.rs +++ b/configurator/src/models/config/boards.rs @@ -64,6 +64,8 @@ pub struct BoardsDraft { pub max_count: String, pub auto_create: bool, pub show_board_badge: bool, + pub pan_enabled: bool, + pub show_pan_badge: bool, pub persist_customizations: bool, pub default_board: String, pub items: Vec, diff --git a/configurator/src/models/config/boards/mapping.rs b/configurator/src/models/config/boards/mapping.rs index cce8c02c..746a7ce8 100644 --- a/configurator/src/models/config/boards/mapping.rs +++ b/configurator/src/models/config/boards/mapping.rs @@ -116,6 +116,8 @@ impl BoardsDraft { max_count: boards.max_count.to_string(), auto_create: boards.auto_create, show_board_badge: boards.show_board_badge, + pan_enabled: boards.pan_enabled, + show_pan_badge: boards.show_pan_badge, persist_customizations: boards.persist_customizations, default_board: boards.default_board.clone(), items: boards @@ -133,6 +135,8 @@ impl BoardsDraft { }); config.auto_create = self.auto_create; config.show_board_badge = self.show_board_badge; + config.pan_enabled = self.pan_enabled; + config.show_pan_badge = self.show_pan_badge; config.persist_customizations = self.persist_customizations; let default_board = self.default_board.trim(); diff --git a/configurator/src/models/config/tests.rs b/configurator/src/models/config/tests.rs index d6abc730..43768208 100644 --- a/configurator/src/models/config/tests.rs +++ b/configurator/src/models/config/tests.rs @@ -667,6 +667,26 @@ fn config_draft_round_trips_render_profiles() { ); } +#[test] +fn config_draft_round_trips_board_pan_settings() { + let mut config = Config::default(); + let mut boards = wayscriber::config::BoardsConfig::default(); + boards.pan_enabled = false; + boards.show_pan_badge = false; + config.boards = Some(boards); + + let draft = ConfigDraft::from_config(&config); + assert!(!draft.boards.pan_enabled); + assert!(!draft.boards.show_pan_badge); + + let round_trip = draft + .to_config(&config) + .expect("board pan settings should round trip"); + let boards = round_trip.boards.expect("boards section should be saved"); + assert!(!boards.pan_enabled); + assert!(!boards.show_pan_badge); +} + #[test] fn config_draft_round_trips_pdf_export() { let mut config = Config::default(); From 37b4ab37479ae6a5b78d2bcce444cf5392031deb Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Sat, 15 Aug 2026 00:11:33 +0200 Subject: [PATCH 10/24] fix: refuse out-of-range configurator values instead of clamping them Save used to parse any number and let validate_and_clamp rewrite it. Drawing, arrow, and history fields now reject the same ranges the UI already shows, and a remaining clamp still blocks the write so keybinding arbitration can keep saving. --- configurator/src/app/update/config/save.rs | 14 ++++ configurator/src/app/update/config/tests.rs | 21 +++++ configurator/src/models/config/parse.rs | 83 ++++++++++++++++++- configurator/src/models/config/tests.rs | 17 ++++ .../src/models/config/to_config/drawing.rs | 59 +++++++++---- .../src/models/config/to_config/history.rs | 31 +++++-- 6 files changed, 202 insertions(+), 23 deletions(-) diff --git a/configurator/src/app/update/config/save.rs b/configurator/src/app/update/config/save.rs index 76112aed..57a99c5c 100644 --- a/configurator/src/app/update/config/save.rs +++ b/configurator/src/app/update/config/save.rs @@ -80,7 +80,15 @@ impl ConfiguratorApp { document: &ConfigDocument, ) -> Result> { let mut config = self.draft.to_config(document.config())?; + let before_clamp = config.clone(); self.pending_save_validation = config.validate_and_clamp(); + if save_clamped_non_keybinding_fields(&before_clamp, &config) { + self.pending_save_validation = Default::default(); + return Err(vec![FormError::new( + "config", + "Some values are outside their allowed ranges and would be changed on save. Fix them before saving.", + )]); + } Ok(config) } @@ -138,3 +146,9 @@ impl ConfiguratorApp { Vec::new() } } + +fn save_clamped_non_keybinding_fields(before: &Config, after: &Config) -> bool { + let mut before = before.clone(); + before.keybindings = after.keybindings.clone(); + format!("{before:?}") != format!("{after:?}") +} diff --git a/configurator/src/app/update/config/tests.rs b/configurator/src/app/update/config/tests.rs index 39ed0334..a20f37ce 100644 --- a/configurator/src/app/update/config/tests.rs +++ b/configurator/src/app/update/config/tests.rs @@ -368,6 +368,27 @@ fn a_draft_the_converter_rejects_keeps_the_document() { )); } +/// Out-of-range numbers used to parse, then `validate_and_clamp` wrote the +/// clamped value. The converter must refuse them so Save cannot change them. +#[test] +fn a_draft_with_out_of_range_numbers_keeps_the_document() { + let (mut app, _dir, _path) = app_with_config_file(""); + app.draft.drawing_default_thickness = "99".to_string(); + + let effects = app.handle_save_requested(); + + assert!(effects.is_empty()); + assert!(!app.is_saving); + assert!( + app.base_document.is_some(), + "a refused save must not take the document with it" + ); + assert!(status_contains( + &app.status, + "drawing.default_thickness: Expected 1-50" + )); +} + /// Hex text the parser rejects was never applied to the draft, so a save /// would write the last value that did parse and the reload would replace /// the text with it. The Save is refused instead. diff --git a/configurator/src/models/config/parse.rs b/configurator/src/models/config/parse.rs index 624e46b5..2de92372 100644 --- a/configurator/src/models/config/parse.rs +++ b/configurator/src/models/config/parse.rs @@ -8,8 +8,34 @@ pub(super) fn parse_field( apply: F, ) where F: FnOnce(f64), +{ + parse_field_in_range( + value, + field, + f64::NEG_INFINITY, + f64::INFINITY, + errors, + apply, + ); +} + +pub(super) fn parse_field_in_range( + value: &str, + field: &'static str, + min: f64, + max: f64, + errors: &mut Vec, + apply: F, +) where + F: FnOnce(f64), { match parse_f64(value.trim()) { + Ok(parsed) if !parsed.is_finite() => { + errors.push(FormError::new(field, "Expected a finite numeric value")); + } + Ok(parsed) if parsed < min || parsed > max => { + errors.push(FormError::new(field, format!("Expected {min}-{max}"))); + } Ok(parsed) => apply(parsed), Err(err) => errors.push(FormError::new(field, err)), } @@ -22,22 +48,61 @@ pub(super) fn parse_usize_field( apply: F, ) where F: FnOnce(usize), +{ + parse_usize_in_range(value, field, 0, usize::MAX, errors, apply); +} + +pub(super) fn parse_usize_in_range( + value: &str, + field: &'static str, + min: usize, + max: usize, + errors: &mut Vec, + apply: F, +) where + F: FnOnce(usize), +{ + match value.trim().parse::() { + Ok(parsed) if parsed < min || parsed > max => { + errors.push(FormError::new(field, format!("Expected {min}-{max}"))); + } + Ok(parsed) => apply(parsed), + Err(err) => errors.push(FormError::new(field, err.to_string())), + } +} + +pub(super) fn parse_usize_at_least( + value: &str, + field: &'static str, + min: usize, + errors: &mut Vec, + apply: F, +) where + F: FnOnce(usize), { match value.trim().parse::() { + Ok(parsed) if parsed < min => { + errors.push(FormError::new(field, format!("Expected at least {min}"))); + } Ok(parsed) => apply(parsed), Err(err) => errors.push(FormError::new(field, err.to_string())), } } -pub(super) fn parse_u8_field( +pub(super) fn parse_u8_in_range( value: &str, field: &'static str, + min: u8, + max: u8, errors: &mut Vec, apply: F, ) where F: FnOnce(u8), { match value.trim().parse::() { + Ok(parsed) if parsed < min || parsed > max => { + errors.push(FormError::new(field, format!("Expected {min}-{max}"))); + } Ok(parsed) => apply(parsed), Err(err) => errors.push(FormError::new(field, err.to_string())), } @@ -112,8 +177,24 @@ pub(super) fn parse_u64_field( apply: F, ) where F: FnOnce(u64), +{ + parse_u64_in_range(value, field, 0, u64::MAX, errors, apply); +} + +pub(super) fn parse_u64_in_range( + value: &str, + field: &'static str, + min: u64, + max: u64, + errors: &mut Vec, + apply: F, +) where + F: FnOnce(u64), { match value.trim().parse::() { + Ok(parsed) if parsed < min || parsed > max => { + errors.push(FormError::new(field, format!("Expected {min}-{max}"))); + } Ok(parsed) => apply(parsed), Err(err) => errors.push(FormError::new(field, err.to_string())), } diff --git a/configurator/src/models/config/tests.rs b/configurator/src/models/config/tests.rs index 43768208..4f70b38f 100644 --- a/configurator/src/models/config/tests.rs +++ b/configurator/src/models/config/tests.rs @@ -89,6 +89,23 @@ fn config_draft_to_config_reports_errors() { assert!(fields.contains(&"drawing.default_color")); } +#[test] +fn config_draft_to_config_rejects_out_of_range_numbers() { + let mut draft = ConfigDraft::from_config(&Config::default()); + draft.drawing_default_thickness = "99".to_string(); + draft.arrow_angle = "90".to_string(); + draft.history_undo_all_delay_ms = "1".to_string(); + + let errors = draft + .to_config(&Config::default()) + .expect_err("out-of-range numbers must not convert"); + let fields: Vec<&str> = errors.iter().map(|err| err.field.as_str()).collect(); + + assert!(fields.contains(&"drawing.default_thickness")); + assert!(fields.contains(&"arrow.angle_degrees")); + assert!(fields.contains(&"history.undo_all_delay_ms")); +} + #[test] fn config_draft_rejects_path_escaping_save_names() { let mut draft = ConfigDraft::from_config(&Config::default()); diff --git a/configurator/src/models/config/to_config/drawing.rs b/configurator/src/models/config/to_config/drawing.rs index 8d0fedf9..610e4b30 100644 --- a/configurator/src/models/config/to_config/drawing.rs +++ b/configurator/src/models/config/to_config/drawing.rs @@ -1,7 +1,11 @@ use super::super::draft::ConfigDraft; -use super::super::parse::{parse_field, parse_u8_field, parse_usize_field}; +use super::super::parse::{ + parse_field_in_range, parse_u8_in_range, parse_usize_at_least, parse_usize_in_range, +}; use crate::models::error::FormError; use wayscriber::config::Config; +use wayscriber::draw::{REGULAR_POLYGON_MAX_SIDES, REGULAR_POLYGON_MIN_SIDES}; +use wayscriber::input::state::{MAX_STROKE_THICKNESS, MIN_STROKE_THICKNESS}; use wayscriber::input::{DragBindableTool, DragTool}; impl ConfigDraft { @@ -14,34 +18,44 @@ impl ConfigDraft { } self.drawing_quick_colors .apply_to_config(&mut config.drawing.quick_colors, errors); - parse_field( + parse_field_in_range( &self.drawing_default_thickness, "drawing.default_thickness", + MIN_STROKE_THICKNESS, + MAX_STROKE_THICKNESS, errors, |value| config.drawing.default_thickness = value, ); - parse_field( + parse_field_in_range( &self.drawing_default_eraser_size, "drawing.default_eraser_size", + MIN_STROKE_THICKNESS, + MAX_STROKE_THICKNESS, errors, |value| config.drawing.default_eraser_size = value, ); config.drawing.default_eraser_mode = self.drawing_default_eraser_mode.to_mode(); - parse_field( + parse_field_in_range( &self.drawing_default_font_size, "drawing.default_font_size", + 8.0, + 72.0, errors, |value| config.drawing.default_font_size = value, ); - parse_u8_field( + parse_u8_in_range( &self.drawing_polygon_sides, "drawing.polygon_sides", + REGULAR_POLYGON_MIN_SIDES, + REGULAR_POLYGON_MAX_SIDES, errors, |value| config.drawing.polygon_sides = value, ); - parse_field( + parse_field_in_range( &self.drawing_marker_opacity, "drawing.marker_opacity", + 0.05, + 0.9, errors, |value| config.drawing.marker_opacity = value, ); @@ -71,31 +85,46 @@ impl ConfigDraft { DragBindableTool::Ellipse, ); config.drawing.drag_tools = materialize_drag_tools.then(|| self.drawing_drag_tools.clone()); - parse_field( + parse_field_in_range( &self.drawing_hit_test_tolerance, "drawing.hit_test_tolerance", + 1.0, + 20.0, errors, |value| config.drawing.hit_test_tolerance = value, ); - parse_usize_field( + parse_usize_at_least( &self.drawing_hit_test_linear_threshold, "drawing.hit_test_linear_threshold", + 1, errors, |value| config.drawing.hit_test_linear_threshold = value, ); - parse_usize_field( + parse_usize_in_range( &self.drawing_undo_stack_limit, "drawing.undo_stack_limit", + 10, + 1000, errors, |value| config.drawing.undo_stack_limit = value, ); - parse_field(&self.arrow_length, "arrow.length", errors, |value| { - config.arrow.length = value - }); - parse_field(&self.arrow_angle, "arrow.angle_degrees", errors, |value| { - config.arrow.angle_degrees = value - }); + parse_field_in_range( + &self.arrow_length, + "arrow.length", + 5.0, + 50.0, + errors, + |value| config.arrow.length = value, + ); + parse_field_in_range( + &self.arrow_angle, + "arrow.angle_degrees", + 15.0, + 60.0, + errors, + |value| config.arrow.angle_degrees = value, + ); config.arrow.head_at_end = self.arrow_head_at_end; } } diff --git a/configurator/src/models/config/to_config/history.rs b/configurator/src/models/config/to_config/history.rs index 4a43ad4a..6543950e 100644 --- a/configurator/src/models/config/to_config/history.rs +++ b/configurator/src/models/config/to_config/history.rs @@ -1,44 +1,61 @@ use super::super::draft::ConfigDraft; -use super::super::parse::{parse_u64_field, parse_usize_field}; +use super::super::parse::{parse_u64_in_range, parse_usize_in_range}; use crate::models::error::FormError; use wayscriber::config::Config; +const HISTORY_DELAY_MS_MIN: u64 = 50; +const HISTORY_DELAY_MS_MAX: u64 = 5_000; +const HISTORY_STEPS_MIN: usize = 1; +const HISTORY_STEPS_MAX: usize = 500; + impl ConfigDraft { pub(super) fn apply_history(&self, config: &mut Config, errors: &mut Vec) { - parse_u64_field( + parse_u64_in_range( &self.history_undo_all_delay_ms, "history.undo_all_delay_ms", + HISTORY_DELAY_MS_MIN, + HISTORY_DELAY_MS_MAX, errors, |value| config.history.undo_all_delay_ms = value, ); - parse_u64_field( + parse_u64_in_range( &self.history_redo_all_delay_ms, "history.redo_all_delay_ms", + HISTORY_DELAY_MS_MIN, + HISTORY_DELAY_MS_MAX, errors, |value| config.history.redo_all_delay_ms = value, ); config.history.custom_section_enabled = self.history_custom_section_enabled; - parse_u64_field( + parse_u64_in_range( &self.history_custom_undo_delay_ms, "history.custom_undo_delay_ms", + HISTORY_DELAY_MS_MIN, + HISTORY_DELAY_MS_MAX, errors, |value| config.history.custom_undo_delay_ms = value, ); - parse_u64_field( + parse_u64_in_range( &self.history_custom_redo_delay_ms, "history.custom_redo_delay_ms", + HISTORY_DELAY_MS_MIN, + HISTORY_DELAY_MS_MAX, errors, |value| config.history.custom_redo_delay_ms = value, ); - parse_usize_field( + parse_usize_in_range( &self.history_custom_undo_steps, "history.custom_undo_steps", + HISTORY_STEPS_MIN, + HISTORY_STEPS_MAX, errors, |value| config.history.custom_undo_steps = value, ); - parse_usize_field( + parse_usize_in_range( &self.history_custom_redo_steps, "history.custom_redo_steps", + HISTORY_STEPS_MIN, + HISTORY_STEPS_MAX, errors, |value| config.history.custom_redo_steps = value, ); From bc355e86f2c48f155b1262747b25aff5c9302c86 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Sat, 15 Aug 2026 00:13:58 +0200 Subject: [PATCH 11/24] fix: validate configurator keybinding text with the real parser Comma-splitting accepted any token, so an unparseable shortcut reached save and was dropped by validate_and_clamp. Parse each entry with KeyBinding and mark the row so Save cannot write a binding the overlay would ignore. --- configurator/src/app/pages/keybindings.rs | 22 +++++++++++++++++ configurator/src/models/keybindings/mod.rs | 1 + configurator/src/models/keybindings/parse.rs | 10 +++++--- configurator/src/models/keybindings/tests.rs | 25 ++++++++++++++++++++ 4 files changed, 55 insertions(+), 3 deletions(-) diff --git a/configurator/src/app/pages/keybindings.rs b/configurator/src/app/pages/keybindings.rs index 54a56521..126548ea 100644 --- a/configurator/src/app/pages/keybindings.rs +++ b/configurator/src/app/pages/keybindings.rs @@ -11,6 +11,7 @@ use relm4::{adw, gtk}; use adw::prelude::*; use crate::messages::Message; +use crate::models::keybindings::parse_keybinding_list; use crate::models::{KeybindingField, KeybindingsTabId, TabId}; use super::super::search::AppSearchSummary; @@ -136,6 +137,27 @@ fn binding_row( // value back as a user edit clears that load's diagnostics. set_text_blocked(&row, &handler, value); + let parse_error = parse_keybinding_list(value).err(); + let has_error_class = row.has_css_class("error"); + match parse_error { + Some(message) => { + if !has_error_class { + row.add_css_class("error"); + } + if row.tooltip_text().as_deref() != Some(message.as_str()) { + row.set_tooltip_text(Some(&message)); + } + } + None => { + if has_error_class { + row.remove_css_class("error"); + } + if row.tooltip_text().is_some() { + row.set_tooltip_text(None); + } + } + } + let default = app .defaults .keybindings diff --git a/configurator/src/models/keybindings/mod.rs b/configurator/src/models/keybindings/mod.rs index 03b015a0..e7275afb 100644 --- a/configurator/src/models/keybindings/mod.rs +++ b/configurator/src/models/keybindings/mod.rs @@ -4,6 +4,7 @@ mod parse; pub use draft::KeybindingsDraft; pub use field::KeybindingField; +pub(crate) use parse::parse_keybinding_list; #[cfg(test)] mod tests; diff --git a/configurator/src/models/keybindings/parse.rs b/configurator/src/models/keybindings/parse.rs index bc4cf722..e4da185d 100644 --- a/configurator/src/models/keybindings/parse.rs +++ b/configurator/src/models/keybindings/parse.rs @@ -1,11 +1,15 @@ -pub fn parse_keybinding_list(value: &str) -> Result, String> { +use wayscriber::config::KeyBinding; + +pub(crate) fn parse_keybinding_list(value: &str) -> Result, String> { let mut entries = Vec::new(); for part in value.split(',') { let trimmed = part.trim(); - if !trimmed.is_empty() { - entries.push(trimmed.to_string()); + if trimmed.is_empty() { + continue; } + KeyBinding::parse(trimmed)?; + entries.push(trimmed.to_string()); } Ok(entries) diff --git a/configurator/src/models/keybindings/tests.rs b/configurator/src/models/keybindings/tests.rs index 96a11be9..ceb83565 100644 --- a/configurator/src/models/keybindings/tests.rs +++ b/configurator/src/models/keybindings/tests.rs @@ -12,6 +12,31 @@ fn parse_keybinding_list_trims_and_ignores_empty() { assert_eq!(parsed, vec!["Ctrl+Z".to_string(), "Alt+K".to_string()]); } +#[test] +fn parse_keybinding_list_rejects_unparseable_shortcuts() { + let error = parse_keybinding_list("Ctrl+Shift, Escape").expect_err("modifiers-only is invalid"); + assert!( + error.contains("No key specified"), + "parser error should name the problem: {error}" + ); +} + +#[test] +fn keybindings_draft_to_config_rejects_unparseable_shortcuts() { + let mut draft = KeybindingsDraft::from_config(&KeybindingsConfig::default()); + draft.set(KeybindingField::Exit, "Ctrl+Shift".to_string()); + + let errors = draft + .to_config() + .expect_err("unparseable shortcuts must not save"); + assert!( + errors + .iter() + .any(|err| err.field == "keybindings.exit" && err.message.contains("No key specified")), + "expected a field error for exit, got {errors:?}" + ); +} + #[test] fn keybindings_draft_to_config_updates_fields() { let mut draft = KeybindingsDraft::from_config(&KeybindingsConfig::default()); From d682a77ee66f91367639e75e3907b3f4f3f23810 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Sat, 15 Aug 2026 00:14:25 +0200 Subject: [PATCH 12/24] fix: reject an empty daemon shortcut instead of inventing one An empty Apply Shortcut field used to write Super+g or Ctrl+Shift+g. Treat blank input as an error so a cleared field cannot bind a default the user did not type. --- configurator/src/app/daemon_setup/shortcut.rs | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/configurator/src/app/daemon_setup/shortcut.rs b/configurator/src/app/daemon_setup/shortcut.rs index 6a8ffeea..9dca2102 100644 --- a/configurator/src/app/daemon_setup/shortcut.rs +++ b/configurator/src/app/daemon_setup/shortcut.rs @@ -282,11 +282,7 @@ fn normalize_shortcut_for_portal(input: &str) -> Result { fn normalize_shortcut(input: &str, gnome_style: bool) -> Result { let trimmed = input.trim(); if trimmed.is_empty() { - return Ok(if gnome_style { - "g".to_string() - } else { - "g".to_string() - }); + return Err("Shortcut cannot be empty.".to_string()); } if trimmed.contains('<') && trimmed.contains('>') { return Ok(trimmed.to_string()); @@ -461,6 +457,18 @@ mod tests { assert!(error.contains("Unsupported modifier")); } + #[test] + fn normalize_shortcut_rejects_empty_input() { + assert_eq!( + normalize_shortcut_for_gnome("").expect_err("empty gnome shortcut"), + "Shortcut cannot be empty." + ); + assert_eq!( + normalize_shortcut_for_portal(" ").expect_err("blank portal shortcut"), + "Shortcut cannot be empty." + ); + } + #[test] fn shell_quote_leaves_simple_paths_unquoted() { assert_eq!(shell_quote("/usr/bin/wayscriber"), "/usr/bin/wayscriber"); From 91072589207fea8bf749f37e56d709af9812ed86 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Sat, 15 Aug 2026 00:14:41 +0200 Subject: [PATCH 13/24] fix: reveal UI subtabs before hiding the current stack child Hiding a GtkStackPage before selecting the next one lets GTK fall back to another child. Match the keybindings page: show needed pages, select the model tab, then hide the rest. --- configurator/src/app/pages/ui.rs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/configurator/src/app/pages/ui.rs b/configurator/src/app/pages/ui.rs index 4b627ce8..880a2df1 100644 --- a/configurator/src/app/pages/ui.rs +++ b/configurator/src/app/pages/ui.rs @@ -105,13 +105,14 @@ pub(super) fn build(sender: &ComponentSender) -> BuiltPage { let ui_summary = summary.tab(TabId::Ui); let searching = summary.is_active(); let mut any_visible = false; + // Reveal before switching and hide after: the stack drops a + // visible child that goes invisible, and picking the section the + // model asks for first keeps that fallback out of the way. for (tab, stack_page) in &stack_pages { - // A hidden `GtkStackPage` also drops its switcher button, - // which is how the Iced view narrowed the sub-tab row. let visible = !searching || ui_summary.is_some_and(|summary| summary.ui_tab_visible(*tab)); - if stack_page.is_visible() != visible { - stack_page.set_visible(visible); + if visible && !stack_page.is_visible() { + stack_page.set_visible(true); } any_visible |= visible; } @@ -126,6 +127,13 @@ pub(super) fn build(sender: &ComponentSender) -> BuiltPage { if stack.visible_child_name().as_deref() != Some(name) { stack.set_visible_child_name(name); } + for (tab, stack_page) in &stack_pages { + let visible = + !searching || ui_summary.is_some_and(|summary| summary.ui_tab_visible(*tab)); + if !visible && stack_page.is_visible() { + stack_page.set_visible(false); + } + } })); } From a84b2aa66160e7d4b47a2c2665de4adbec159f80 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Sat, 15 Aug 2026 00:16:54 +0200 Subject: [PATCH 14/24] fix: take configurator keybinding labels from action_meta KeybindingField kept its own title strings, so overlay and configurator names drifted. Look up each field's Action and use action_label, and search the same metadata aliases so terms like eyedropper still find the row. --- configurator/src/app/search/summary.rs | 18 +- .../src/models/keybindings/field/labels.rs | 163 ++---------------- configurator/src/models/keybindings/tests.rs | 24 +++ 3 files changed, 52 insertions(+), 153 deletions(-) diff --git a/configurator/src/app/search/summary.rs b/configurator/src/app/search/summary.rs index 83b52c95..3d52ade7 100644 --- a/configurator/src/app/search/summary.rs +++ b/configurator/src/app/search/summary.rs @@ -1,7 +1,7 @@ use crate::app::state::ConfiguratorApp; use crate::models::{KeybindingsTabId, SearchQuery, TabId, UiTabId}; use wayscriber::config::{ - PERFORMANCE_FIELD_METADATA, PerformanceFieldGroup, toolbar_item_definitions, + PERFORMANCE_FIELD_METADATA, PerformanceFieldGroup, action_meta, toolbar_item_definitions, }; use super::terms::*; @@ -369,13 +369,27 @@ fn keybinding_matches(app: &ConfiguratorApp, query: &SearchQuery, summary: &mut .keybindings .value_for(entry.field) .unwrap_or(""); - let text = format!( + let mut text = format!( "keybindings keybinding shortcut hotkey keyboard shortcut list {} {} {} {}", entry.field.tab().title(), entry.field.label(), entry.field.field_key(), entry.value, ); + if let Some(meta) = entry + .field + .action() + .and_then(wayscriber::config::action_meta) + { + text.push(' '); + text.push_str(meta.description); + text.push(' '); + text.push_str(meta.short_label()); + for alias in meta.search_aliases { + text.push(' '); + text.push_str(alias); + } + } if query.matches_text(&text) || query.matches_text(default_value) { summary.add_keybinding_field(entry.field); } diff --git a/configurator/src/models/keybindings/field/labels.rs b/configurator/src/models/keybindings/field/labels.rs index 6422c956..bf964dcc 100644 --- a/configurator/src/models/keybindings/field/labels.rs +++ b/configurator/src/models/keybindings/field/labels.rs @@ -1,158 +1,19 @@ use super::KeybindingField; +use wayscriber::config::{Action, KeybindingsConfig, action_label}; impl KeybindingField { + pub fn action(self) -> Option { + let key = self.field_key(); + KeybindingsConfig::configurable_actions() + .iter() + .copied() + .find(|action| KeybindingsConfig::config_key_for_action(*action) == Some(key)) + } + pub fn label(&self) -> &'static str { - match self { - Self::Exit => "Exit", - Self::EnterTextMode => "Enter text mode", - Self::EnterStickyNoteMode => "Enter sticky note mode", - Self::ClearCanvas => "Clear canvas", - Self::Undo => "Undo", - Self::Redo => "Redo", - Self::UndoAll => "Undo all", - Self::RedoAll => "Redo all", - Self::UndoAllDelayed => "Undo all (delayed)", - Self::RedoAllDelayed => "Redo all (delayed)", - Self::DuplicateSelection => "Duplicate selection", - Self::CopySelection => "Copy selection", - Self::PasteSelection => "Paste selection", - Self::SelectAll => "Select all", - Self::MoveSelectionToFront => "Move selection to front", - Self::MoveSelectionToBack => "Move selection to back", - Self::NudgeSelectionUp => "Nudge selection up", - Self::NudgeSelectionDown => "Nudge selection down", - Self::NudgeSelectionLeft => "Nudge selection left", - Self::NudgeSelectionRight => "Nudge selection right", - Self::NudgeSelectionUpLarge => "Nudge selection up (large)", - Self::NudgeSelectionDownLarge => "Nudge selection down (large)", - Self::MoveSelectionToStart => "Move selection to start", - Self::MoveSelectionToEnd => "Move selection to end", - Self::MoveSelectionToTop => "Move selection to top", - Self::MoveSelectionToBottom => "Move selection to bottom", - Self::DeleteSelection => "Delete selection", - Self::IncreaseThickness => "Increase thickness", - Self::DecreaseThickness => "Decrease thickness", - Self::IncreaseMarkerOpacity => "Increase marker opacity", - Self::DecreaseMarkerOpacity => "Decrease marker opacity", - Self::SelectSelectionTool => "Select selection tool", - Self::SelectPenTool => "Select pen tool", - Self::SelectEraserTool => "Select eraser tool", - Self::ToggleEraserMode => "Toggle eraser mode", - Self::SelectMarkerTool => "Select marker tool", - Self::SelectStepMarkerTool => "Select step marker tool", - Self::SelectLineTool => "Select line tool", - Self::SelectRectTool => "Select rectangle tool", - Self::SelectEllipseTool => "Select ellipse tool", - Self::SelectArrowTool => "Select arrow tool", - Self::SelectTriangleTool => "Select triangle tool", - Self::SelectParallelogramTool => "Select parallelogram tool", - Self::SelectRhombusTool => "Select rhombus tool", - Self::SelectRegularPolygonTool => "Select regular polygon tool", - Self::SelectFreeformPolygonTool => "Select freeform polygon tool", - Self::SelectBlurTool => "Select blur tool", - Self::SelectSpotlightTool => "Select spotlight tool", - Self::CycleBlurStyle => "Cycle blur style", - Self::SelectHighlightTool => "Select highlight tool", - Self::IncreaseFontSize => "Increase font size", - Self::DecreaseFontSize => "Decrease font size", - Self::ToggleWhiteboard => "Toggle whiteboard", - Self::ToggleBlackboard => "Toggle blackboard", - Self::ReturnToTransparent => "Return to transparent", - Self::PagePrev => "Page: previous", - Self::PageNext => "Page: next", - Self::PageNew => "Page: new", - Self::PageDuplicate => "Page: duplicate", - Self::PageDelete => "Page: delete", - Self::Board1 => "Board: 1", - Self::Board2 => "Board: 2", - Self::Board3 => "Board: 3", - Self::Board4 => "Board: 4", - Self::Board5 => "Board: 5", - Self::Board6 => "Board: 6", - Self::Board7 => "Board: 7", - Self::Board8 => "Board: 8", - Self::Board9 => "Board: 9", - Self::FocusNextOutput => "Focus next output", - Self::FocusPrevOutput => "Focus previous output", - Self::ToggleLightMode => "Toggle light mode", - Self::ToggleLightModeDrawing => "Toggle light mode drawing", - Self::ToggleRadialMenu => "Toggle radial menu", - Self::BoardNext => "Board: next", - Self::BoardPrev => "Board: previous", - Self::BoardNew => "Board: new", - Self::BoardDuplicate => "Board: duplicate", - Self::BoardDelete => "Board: delete", - Self::BoardPicker => "Board: picker", - Self::ToggleHelp => "Toggle help", - Self::ToggleQuickHelp => "Toggle quick help", - Self::ToggleStatusBar => "Toggle status bar", - Self::ToggleFloatingBadge => "Toggle board/page badge", - Self::ToggleZoomChip => "Toggle zoom chip", - Self::ToggleFocusMode => "Focus mode (hide all UI)", - Self::ToggleClickHighlight => "Toggle click highlight", - Self::ToggleInputHud => "Toggle input HUD", - Self::ToggleToolbar => "Toggle toolbar", - Self::CycleToolbarDisplay => "Cycle toolbar display", - Self::TogglePresenterMode => "Toggle presenter mode", - Self::RenderProfileNext => "Render profile: next", - Self::RenderProfilePrevious => "Render profile: previous", - Self::RenderProfileOff => "Render profile: off", - Self::ToggleFill => "Toggle fill", - Self::ToggleHighlightTool => "Toggle highlight tool", - Self::ToggleSelectionProperties => "Toggle selection properties", - Self::OpenContextMenu => "Open context menu", - Self::OpenConfigurator => "Open configurator", - Self::OpenAbout => "Open About window", - Self::ToggleCommandPalette => "Toggle command palette", - Self::SetColorRed => "Quick color 1 (red default)", - Self::SetColorGreen => "Quick color 2 (green default)", - Self::SetColorBlue => "Quick color 3 (blue default)", - Self::SetColorYellow => "Quick color 4 (yellow default)", - Self::SetColorOrange => "Quick color 5 (orange default)", - Self::SetColorPink => "Quick color 6 (pink default)", - Self::SetColorWhite => "Quick color 7 (white default)", - Self::SetColorBlack => "Quick color 8 (black default)", - Self::PickScreenColor => "Screen eyedropper (pick color)", - Self::CaptureFullScreen => "Capture full screen", - Self::CaptureActiveWindow => "Capture active window", - Self::CaptureSelection => "Capture selection", - Self::CaptureClipboardFull => "Clipboard full screen", - Self::CaptureFileFull => "File full screen", - Self::CaptureClipboardSelection => "Clipboard selection", - Self::CaptureFileSelection => "File selection", - Self::CaptureClipboardRegion => "Clipboard region", - Self::CaptureFileRegion => "File region", - Self::ExportCanvasFile => "Export canvas: file", - Self::ExportCanvasClipboard => "Export canvas: clipboard", - Self::ExportCanvasClipboardAndFile => "Export canvas: clipboard and file", - Self::ExportBoardPdfFile => "Export board: PDF file", - Self::ExportAllBoardsPdfFile => "Export all boards: PDF file", - Self::OpenCaptureFolder => "Open capture folder", - Self::CopyTextFromScreen => "Copy text from screen (OCR)", - Self::ToggleFrozenMode => "Toggle freeze", - Self::ZoomIn => "Zoom in", - Self::ZoomOut => "Zoom out", - Self::ResetZoom => "Reset zoom", - Self::ToggleZoomLock => "Toggle zoom lock", - Self::RefreshZoomCapture => "Refresh zoom snapshot", - Self::ResetArrowLabels => "Reset arrow labels", - Self::ResetStepMarkers => "Reset step markers", - Self::ApplyPreset1 => "Apply preset 1", - Self::ApplyPreset2 => "Apply preset 2", - Self::ApplyPreset3 => "Apply preset 3", - Self::ApplyPreset4 => "Apply preset 4", - Self::ApplyPreset5 => "Apply preset 5", - Self::SavePreset1 => "Save preset 1", - Self::SavePreset2 => "Save preset 2", - Self::SavePreset3 => "Save preset 3", - Self::SavePreset4 => "Save preset 4", - Self::SavePreset5 => "Save preset 5", - Self::ClearPreset1 => "Clear preset 1", - Self::ClearPreset2 => "Clear preset 2", - Self::ClearPreset3 => "Clear preset 3", - Self::ClearPreset4 => "Clear preset 4", - Self::ClearPreset5 => "Clear preset 5", - } + self.action() + .map(action_label) + .unwrap_or_else(|| self.field_key()) } pub fn field_key(&self) -> &'static str { diff --git a/configurator/src/models/keybindings/tests.rs b/configurator/src/models/keybindings/tests.rs index ceb83565..93909ecc 100644 --- a/configurator/src/models/keybindings/tests.rs +++ b/configurator/src/models/keybindings/tests.rs @@ -1,3 +1,4 @@ +use wayscriber::config::action_label; use wayscriber::config::action_meta_iter; use wayscriber::config::keybindings::KeybindingsConfig; @@ -227,6 +228,29 @@ fn every_configurable_config_key_resolves_to_a_field() { ); } +#[test] +fn every_keybinding_field_label_matches_action_meta() { + let mut unlabeled = Vec::new(); + let mut mismatched = Vec::new(); + for field in KeybindingField::all() { + let Some(action) = field.action() else { + unlabeled.push(field.field_key()); + continue; + }; + if field.label() != action_label(action) { + mismatched.push(field.field_key()); + } + } + assert!( + unlabeled.is_empty(), + "these fields have no Action, so their labels cannot follow action_meta: {unlabeled:?}" + ); + assert!( + mismatched.is_empty(), + "these fields still use a private label instead of action_meta: {mismatched:?}" + ); +} + #[test] fn blur_style_and_spotlight_bindings_read_and_write_config() { let mut config = KeybindingsConfig::default(); From 7c2dcc5b204bb727e424dab177401cefaa616012 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Sat, 15 Aug 2026 00:19:50 +0200 Subject: [PATCH 15/24] fix: name every configurator tab as an --open destination Capture, Performance, Daemon, Arrow, Render Profiles, Tablet, Help Overlay, and Presenter Mode had no screen token, so overlay and tray could not land there. Add the missing destinations and keep help, parse, and the configurator match in sync. --- configurator/README.md | 5 ++- configurator/src/app/startup.rs | 21 +++++++++++ src/configurator_destination.rs | 66 ++++++++++++++++++++++++++++++++- 3 files changed, 89 insertions(+), 3 deletions(-) diff --git a/configurator/README.md b/configurator/README.md index 9fe7c57e..dbc69f37 100644 --- a/configurator/README.md +++ b/configurator/README.md @@ -37,8 +37,9 @@ wayscriber-configurator --open 'drawing?search=Quick Colors' ``` Destinations are `ui/toolbar`, `ui/toolbar-visibility`, `ui/status-bar`, `ui/click-highlight`, -`ui/input-hud`, `drawing`, `presets`, `boards`, `history`, `session`, `keybindings`, and -`keybindings/
` for `general`, `drawing`, `tools`, `selection`, `history`, `boards`, +`ui/input-hud`, `ui/help-overlay`, `ui/presenter-mode`, `drawing`, `presets`, `boards`, `history`, +`session`, `capture`, `performance`, `daemon`, `arrow`, `render-profiles`, `tablet`, `keybindings`, +and `keybindings/
` for `general`, `drawing`, `tools`, `selection`, `history`, `boards`, `ui-modes`, `capture-view`, and `presets`. Append `?search=` to any of them to open with the search box filled. `--help` prints the same list. An unknown destination falls back to the normal initial screen and says so in the status banner. diff --git a/configurator/src/app/startup.rs b/configurator/src/app/startup.rs index c4276817..448cda66 100644 --- a/configurator/src/app/startup.rs +++ b/configurator/src/app/startup.rs @@ -60,11 +60,24 @@ impl ConfiguratorApp { ConfiguratorScreen::UiStatusBar => self.show_ui_tab(UiTabId::StatusBar), ConfiguratorScreen::UiClickHighlight => self.show_ui_tab(UiTabId::ClickHighlight), ConfiguratorScreen::UiInputHud => self.show_ui_tab(UiTabId::InputHud), + ConfiguratorScreen::UiHelpOverlay => self.show_ui_tab(UiTabId::HelpOverlay), + ConfiguratorScreen::UiPresenterMode => self.show_ui_tab(UiTabId::PresenterMode), ConfiguratorScreen::Drawing => self.active_tab = TabId::Drawing, ConfiguratorScreen::Presets => self.active_tab = TabId::Presets, ConfiguratorScreen::Boards => self.active_tab = TabId::Boards, ConfiguratorScreen::History => self.active_tab = TabId::History, ConfiguratorScreen::Session => self.active_tab = TabId::Session, + ConfiguratorScreen::Capture => self.active_tab = TabId::Capture, + ConfiguratorScreen::Performance => self.active_tab = TabId::Performance, + ConfiguratorScreen::Daemon => self.active_tab = TabId::Daemon, + ConfiguratorScreen::Arrow => self.active_tab = TabId::Arrow, + ConfiguratorScreen::RenderProfiles => self.active_tab = TabId::RenderProfiles, + ConfiguratorScreen::Tablet => { + #[cfg(feature = "tablet-input")] + { + self.active_tab = TabId::Tablet; + } + } ConfiguratorScreen::Keybindings(section) => { self.active_tab = TabId::Keybindings; // No section means the tab, on whichever subtab it already @@ -163,6 +176,8 @@ mod tests { ("ui/status-bar", UiTabId::StatusBar), ("ui/click-highlight", UiTabId::ClickHighlight), ("ui/input-hud", UiTabId::InputHud), + ("ui/help-overlay", UiTabId::HelpOverlay), + ("ui/presenter-mode", UiTabId::PresenterMode), ] { let (app, _dir, _path) = app_launched_with(&["--open", argument]); @@ -179,6 +194,12 @@ mod tests { ("presets", TabId::Presets), ("drawing", TabId::Drawing), ("session", TabId::Session), + ("capture", TabId::Capture), + ("performance", TabId::Performance), + ("daemon", TabId::Daemon), + ("arrow", TabId::Arrow), + ("render-profiles", TabId::RenderProfiles), + ("tablet", TabId::Tablet), ("keybindings", TabId::Keybindings), ] { let (app, _dir, _path) = app_launched_with(&["--open", argument]); diff --git a/src/configurator_destination.rs b/src/configurator_destination.rs index 95fdc45b..dc79838e 100644 --- a/src/configurator_destination.rs +++ b/src/configurator_destination.rs @@ -276,28 +276,44 @@ pub enum ConfiguratorScreen { UiStatusBar, UiClickHighlight, UiInputHud, + UiHelpOverlay, + UiPresenterMode, Drawing, Presets, Boards, History, Session, + Capture, + Performance, + Daemon, + Arrow, + RenderProfiles, + Tablet, /// The Keybindings tab, optionally on a named section. Keybindings(Option), } impl ConfiguratorScreen { /// Every nameable screen, in the order help text lists them. - pub const ALL: [Self; 20] = [ + pub const ALL: [Self; 28] = [ Self::UiToolbar, Self::UiToolbarVisibility, Self::UiStatusBar, Self::UiClickHighlight, Self::UiInputHud, + Self::UiHelpOverlay, + Self::UiPresenterMode, Self::Drawing, Self::Presets, Self::Boards, Self::History, Self::Session, + Self::Capture, + Self::Performance, + Self::Daemon, + Self::Arrow, + Self::RenderProfiles, + Self::Tablet, Self::Keybindings(None), Self::Keybindings(Some(KeybindingsSection::General)), Self::Keybindings(Some(KeybindingsSection::Drawing)), @@ -318,11 +334,19 @@ impl ConfiguratorScreen { Self::UiStatusBar => "ui/status-bar", Self::UiClickHighlight => "ui/click-highlight", Self::UiInputHud => "ui/input-hud", + Self::UiHelpOverlay => "ui/help-overlay", + Self::UiPresenterMode => "ui/presenter-mode", Self::Drawing => "drawing", Self::Presets => "presets", Self::Boards => "boards", Self::History => "history", Self::Session => "session", + Self::Capture => "capture", + Self::Performance => "performance", + Self::Daemon => "daemon", + Self::Arrow => "arrow", + Self::RenderProfiles => "render-profiles", + Self::Tablet => "tablet", Self::Keybindings(None) => "keybindings", Self::Keybindings(Some(KeybindingsSection::General)) => "keybindings/general", Self::Keybindings(Some(KeybindingsSection::Drawing)) => "keybindings/drawing", @@ -344,11 +368,19 @@ impl ConfiguratorScreen { "ui/status-bar" => Some(Self::UiStatusBar), "ui/click-highlight" => Some(Self::UiClickHighlight), "ui/input-hud" => Some(Self::UiInputHud), + "ui/help-overlay" => Some(Self::UiHelpOverlay), + "ui/presenter-mode" => Some(Self::UiPresenterMode), "drawing" => Some(Self::Drawing), "presets" => Some(Self::Presets), "boards" => Some(Self::Boards), "history" => Some(Self::History), "session" => Some(Self::Session), + "capture" => Some(Self::Capture), + "performance" => Some(Self::Performance), + "daemon" => Some(Self::Daemon), + "arrow" => Some(Self::Arrow), + "render-profiles" => Some(Self::RenderProfiles), + "tablet" => Some(Self::Tablet), "keybindings" => Some(Self::Keybindings(None)), "keybindings/general" => Some(Self::Keybindings(Some(KeybindingsSection::General))), "keybindings/drawing" => Some(Self::Keybindings(Some(KeybindingsSection::Drawing))), @@ -531,6 +563,38 @@ mod tests { ConfiguratorDestination::new(ConfiguratorScreen::UiInputHud).as_arg(), "ui/input-hud" ); + assert_eq!( + ConfiguratorDestination::new(ConfiguratorScreen::UiHelpOverlay).as_arg(), + "ui/help-overlay" + ); + assert_eq!( + ConfiguratorDestination::new(ConfiguratorScreen::UiPresenterMode).as_arg(), + "ui/presenter-mode" + ); + assert_eq!( + ConfiguratorDestination::new(ConfiguratorScreen::Capture).as_arg(), + "capture" + ); + assert_eq!( + ConfiguratorDestination::new(ConfiguratorScreen::Performance).as_arg(), + "performance" + ); + assert_eq!( + ConfiguratorDestination::new(ConfiguratorScreen::Daemon).as_arg(), + "daemon" + ); + assert_eq!( + ConfiguratorDestination::new(ConfiguratorScreen::Arrow).as_arg(), + "arrow" + ); + assert_eq!( + ConfiguratorDestination::new(ConfiguratorScreen::RenderProfiles).as_arg(), + "render-profiles" + ); + assert_eq!( + ConfiguratorDestination::new(ConfiguratorScreen::Tablet).as_arg(), + "tablet" + ); assert_eq!( ConfiguratorDestination::new(ConfiguratorScreen::History).as_arg(), "history" From c806f6ed35c747d9d1cd3e561b4101d59a89a586 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Sat, 15 Aug 2026 00:21:58 +0200 Subject: [PATCH 16/24] fix: do not detach the GTK toolbar thread on shutdown timeout Dropping the JoinHandle after the deadline detaches GTK's main loop. Leak the handle instead so a stuck thread is not abandoned while the process is still running. --- src/toolbar_gtk/bridge.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/toolbar_gtk/bridge.rs b/src/toolbar_gtk/bridge.rs index a9eb4d26..21535e83 100644 --- a/src/toolbar_gtk/bridge.rs +++ b/src/toolbar_gtk/bridge.rs @@ -427,7 +427,12 @@ fn finish_thread_within( std::thread::yield_now(); } if !handle.is_finished() { - thread.take(); + // Dropping a JoinHandle detaches the thread. GTK's main loop is not + // safe to abandon that way, so leak the handle and let process exit + // collect the thread if it never finishes. + if let Some(handle) = thread.take() { + std::mem::forget(handle); + } return ThreadShutdownOutcome::TimedOut; } let handle = thread @@ -515,7 +520,7 @@ impl Drop for GtkToolbarBridge { } ThreadShutdownOutcome::TimedOut => { log::warn!( - "GTK toolbar thread did not stop within {:?}; detaching it safely", + "GTK toolbar thread did not stop within {:?}; leaving it running until process exit", GTK_THREAD_SHUTDOWN_TIMEOUT ); } @@ -883,7 +888,7 @@ mod tests { } #[test] - fn stuck_thread_is_detached_at_the_bounded_deadline() { + fn stuck_thread_is_leaked_not_detached_at_the_bounded_deadline() { let (completion_tx, completion_rx) = mpsc::channel(); let (release_tx, release_rx) = mpsc::channel(); let mut thread = Some(std::thread::spawn(move || { From ecd2e33f62ec39fee5b4287b5cb2803b2d768153 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Sat, 15 Aug 2026 00:23:02 +0200 Subject: [PATCH 17/24] fix: fail closed on portal output mismatch and crop overflow A capture with no target output used to apply to whichever output was active, and ARGB crop sizes could wrap before the allocation. Require both sides of the output id, and use checked arithmetic for the crop buffer. --- src/backend/wayland/portal_capture.rs | 39 +++++++++++++++++++-------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/src/backend/wayland/portal_capture.rs b/src/backend/wayland/portal_capture.rs index 42ac6131..8e9ca74a 100644 --- a/src/backend/wayland/portal_capture.rs +++ b/src/backend/wayland/portal_capture.rs @@ -40,8 +40,7 @@ pub(crate) const fn portal_output_matches(target: Option, current: Option target_output == current_output, (None, None) => true, - (None, Some(_)) => true, - (Some(_), None) => false, + (None, Some(_)) | (Some(_), None) => false, } } @@ -65,14 +64,18 @@ pub(crate) fn crop_argb( return None; } - let mut out = vec![0u8; (cw * ch * 4) as usize]; - let src_stride = (width * 4) as usize; - let dst_stride = (cw * 4) as usize; - for row in 0..ch as usize { - let src_offset = ((y as usize + row) * src_stride) + (x as usize * 4); - let dst_offset = row * dst_stride; - let end = src_offset + dst_stride; - if end > data.len() || dst_offset + dst_stride > out.len() { + let pixel_count = cw.checked_mul(ch)?; + let byte_count = pixel_count.checked_mul(4)?; + let mut out = vec![0u8; usize::try_from(byte_count).ok()?]; + let src_stride = usize::try_from(width.checked_mul(4)?).ok()?; + let dst_stride = usize::try_from(cw.checked_mul(4)?).ok()?; + for row in 0..usize::try_from(ch).ok()? { + let src_offset = (usize::try_from(y).ok()? + row) + .checked_mul(src_stride)? + .checked_add(usize::try_from(x).ok()?.checked_mul(4)?)?; + let dst_offset = row.checked_mul(dst_stride)?; + let end = src_offset.checked_add(dst_stride)?; + if end > data.len() || dst_offset.checked_add(dst_stride)? > out.len() { return None; } out[dst_offset..dst_offset + dst_stride] @@ -83,7 +86,7 @@ pub(crate) fn crop_argb( #[cfg(test)] mod tests { - use super::crop_argb; + use super::{crop_argb, portal_output_matches}; #[test] fn crop_argb_respects_bounds() { @@ -104,4 +107,18 @@ mod tests { // y beyond height. assert!(crop_argb(&[0u8; 4], 1, 1, 0, 2, 1, 1).is_none()); } + + #[test] + fn crop_argb_rejects_dimensions_that_would_overflow() { + assert!(crop_argb(&[0u8; 4], u32::MAX, 1, 0, 0, u32::MAX, 1).is_none()); + } + + #[test] + fn portal_output_matches_requires_both_sides_or_neither() { + assert!(portal_output_matches(Some(1), Some(1))); + assert!(!portal_output_matches(Some(1), Some(2))); + assert!(portal_output_matches(None, None)); + assert!(!portal_output_matches(None, Some(1))); + assert!(!portal_output_matches(Some(1), None)); + } } From 67a2bd2ecd0379691ce379536d8d65776c8ef997 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Sat, 15 Aug 2026 00:26:22 +0200 Subject: [PATCH 18/24] fix: log onboarding save failures and document remaining unsafe buffers Stale Phase 2 dead_code allows hid live capture types, and silent onboarding saves hid migrate/recover failures. Document the Cairo buffer casts and keep mixed-DPI geometry helpers available for tests. --- configurator/src/app/search/summary.rs | 6 +----- src/about_window/render/mod.rs | 3 +++ src/backend/wayland/frozen_geometry.rs | 3 +++ src/backend/wayland/state/render/canvas/background.rs | 7 +++++-- src/backend/wayland/toolbar/surfaces/render.rs | 3 +++ src/capture/types.rs | 5 ----- src/onboarding.rs | 8 +++++--- 7 files changed, 20 insertions(+), 15 deletions(-) diff --git a/configurator/src/app/search/summary.rs b/configurator/src/app/search/summary.rs index 3d52ade7..be5281e8 100644 --- a/configurator/src/app/search/summary.rs +++ b/configurator/src/app/search/summary.rs @@ -376,11 +376,7 @@ fn keybinding_matches(app: &ConfiguratorApp, query: &SearchQuery, summary: &mut entry.field.field_key(), entry.value, ); - if let Some(meta) = entry - .field - .action() - .and_then(wayscriber::config::action_meta) - { + if let Some(meta) = entry.field.action().and_then(action_meta) { text.push(' '); text.push_str(meta.description); text.push(' '); diff --git a/src/about_window/render/mod.rs b/src/about_window/render/mod.rs index 85a4a4ed..03d57827 100644 --- a/src/about_window/render/mod.rs +++ b/src/about_window/render/mod.rs @@ -38,6 +38,9 @@ impl AboutWindowState { ) .context("Failed to create about window buffer")?; + // SAFETY: `canvas` is the SlotPool buffer for this frame. Cairo wraps + // those bytes as ARgb32 with stride `phys_w * 4`. The surface and + // context are dropped before the buffer is attached to Wayland. let surface = unsafe { cairo::ImageSurface::create_for_data_unsafe( canvas.as_mut_ptr(), diff --git a/src/backend/wayland/frozen_geometry.rs b/src/backend/wayland/frozen_geometry.rs index 765ca7b0..0fe2e3e5 100644 --- a/src/backend/wayland/frozen_geometry.rs +++ b/src/backend/wayland/frozen_geometry.rs @@ -3,7 +3,9 @@ use wayland_client::protocol::wl_output; /// Geometry and scale details for the active output, used for cropping fallback captures. #[derive(Clone, Debug)] pub struct OutputGeometry { + #[allow(dead_code)] // part of the output snapshot; tests read origin via physical_origin pub logical_x: i32, + #[allow(dead_code)] // part of the output snapshot; tests read origin via physical_origin pub logical_y: i32, pub logical_width: u32, pub logical_height: u32, @@ -120,6 +122,7 @@ impl OutputGeometry { /// /// Portal desktop screenshots must use [`Self::portal_crop_origin`] instead: /// mixed-scale layouts are not `logical * this output's scale`. + #[allow(dead_code)] // used by geometry tests; portal crop uses screenshot_origin pub fn physical_origin(&self) -> (i32, i32) { ( self.logical_x.saturating_mul(self.scale), diff --git a/src/backend/wayland/state/render/canvas/background.rs b/src/backend/wayland/state/render/canvas/background.rs index 7aa5485a..d98128d0 100644 --- a/src/backend/wayland/state/render/canvas/background.rs +++ b/src/backend/wayland/state/render/canvas/background.rs @@ -53,8 +53,11 @@ impl WaylandState { }); if let Some((image, cache_key, zoom_render_active)) = background_image { - // SAFETY: we create a Cairo surface borrowing our owned buffer; it is dropped - // before commit, and we hold the buffer alive via `image.data`. + // SAFETY: Cairo borrows `image.data` for this surface. The buffer + // is owned by `image` and stays alive until the surface is dropped + // (before Wayland commit). The API wants `*mut u8` even though this + // path only reads pixels; we never write through the pointer, and + // no other alias mutates the buffer while Cairo holds it. let surface = unsafe { cairo::ImageSurface::create_for_data_unsafe( image.data.as_ptr() as *mut u8, diff --git a/src/backend/wayland/toolbar/surfaces/render.rs b/src/backend/wayland/toolbar/surfaces/render.rs index 6ec76220..8935c724 100644 --- a/src/backend/wayland/toolbar/surfaces/render.rs +++ b/src/backend/wayland/toolbar/surfaces/render.rs @@ -75,6 +75,9 @@ impl ToolbarSurface { ) .map_err(|err| anyhow!("failed to create a {phys_w}x{phys_h} buffer: {err}"))?; + // SAFETY: `canvas` is the SlotPool buffer for this toolbar surface. + // Cairo wraps those bytes as ARgb32 with stride `phys_w * 4`. The + // surface is flushed and dropped before the buffer is committed. let surface = unsafe { cairo::ImageSurface::create_for_data_unsafe( canvas.as_mut_ptr(), diff --git a/src/capture/types.rs b/src/capture/types.rs index c0110ac0..240572a3 100644 --- a/src/capture/types.rs +++ b/src/capture/types.rs @@ -392,7 +392,6 @@ pub enum CaptureType { /// Capture the currently focused window. ActiveWindow, /// Capture a user-selected rectangular region. - #[allow(dead_code)] // Will be used in Phase 2 for region selection Selection { x: i32, y: i32, @@ -405,14 +404,12 @@ pub enum CaptureType { #[derive(Debug, Clone)] pub struct CaptureResult { /// Raw image data (PNG format). - #[allow(dead_code)] // Will be used in Phase 2 for annotation compositing pub image_data: Vec, pub operation: ImageOperationKind, pub fallback_format_override: Option, /// Path where the image was saved (if saved). pub saved_path: Option, /// Whether the image was copied to clipboard. - #[allow(dead_code)] // Will be used in Phase 2 for status notifications pub copied_to_clipboard: bool, /// Why a requested file save produced no file, when the operation still /// succeeded through another destination. A successful clipboard copy must @@ -439,7 +436,6 @@ pub enum CaptureOutcome { /// Where the captured image should be delivered. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum CaptureDestination { - #[allow(dead_code)] // Will be used by upcoming clipboard-only actions ClipboardOnly, FileOnly, ClipboardAndFile, @@ -448,7 +444,6 @@ pub enum CaptureDestination { /// Errors that can occur during screenshot capture. #[derive(Debug)] pub enum CaptureError { - #[allow(dead_code)] // Will be used in Phase 2 for capability checks PortalUnavailable, #[cfg_attr(not(feature = "portal"), allow(dead_code))] diff --git a/src/onboarding.rs b/src/onboarding.rs index 068f2e34..3c67eabb 100644 --- a/src/onboarding.rs +++ b/src/onboarding.rs @@ -297,8 +297,8 @@ impl OnboardingStore { path: Some(path), persistence_available: true, }; - if needs_save { - let _ = store.save(); + if needs_save && let Err(err) = store.save() { + warn!("Failed to persist migrated onboarding state: {err}"); } return store; } @@ -691,7 +691,9 @@ fn recover_onboarding_file(path: PathBuf, _raw: Option<&str>) -> OnboardingStore path: Some(path), persistence_available: true, }; - let _ = store.save(); + if let Err(err) = store.save() { + warn!("Failed to persist recovered onboarding state: {err}"); + } store } From 1732761c4aaa18c09554b6fcca24185ce35a0e97 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Sat, 15 Aug 2026 01:27:25 +0200 Subject: [PATCH 19/24] fix: gate the tablet configurator destination on tablet-input --open tablet and the help list advertised a tab that does not exist without the feature. Keep the destination, help text, and parse table behind the same gate. --- configurator/README.md | 9 +++++---- configurator/src/app/startup.rs | 19 +++++++++++++------ src/configurator_destination.rs | 23 ++++++++++++++++++++--- 3 files changed, 38 insertions(+), 13 deletions(-) diff --git a/configurator/README.md b/configurator/README.md index dbc69f37..01e10a08 100644 --- a/configurator/README.md +++ b/configurator/README.md @@ -38,11 +38,12 @@ wayscriber-configurator --open 'drawing?search=Quick Colors' Destinations are `ui/toolbar`, `ui/toolbar-visibility`, `ui/status-bar`, `ui/click-highlight`, `ui/input-hud`, `ui/help-overlay`, `ui/presenter-mode`, `drawing`, `presets`, `boards`, `history`, -`session`, `capture`, `performance`, `daemon`, `arrow`, `render-profiles`, `tablet`, `keybindings`, +`session`, `capture`, `performance`, `daemon`, `arrow`, `render-profiles`, `keybindings`, and `keybindings/
` for `general`, `drawing`, `tools`, `selection`, `history`, `boards`, -`ui-modes`, `capture-view`, and `presets`. Append `?search=` to any of them to open with the -search box filled. `--help` prints the same list. An unknown destination falls back to the normal -initial screen and says so in the status banner. +`ui-modes`, `capture-view`, and `presets`. Builds with tablet input also accept `tablet`. +Append `?search=` to any of them to open with the search box filled. `--help` prints the +same list. An unknown destination falls back to the normal initial screen and says so in the +status banner. Toolbar pin, top placement offsets, the top strip's display form, item visibility/order, and board pin controls are labeled as configured defaults because the running overlay can store later diff --git a/configurator/src/app/startup.rs b/configurator/src/app/startup.rs index 448cda66..c955f09d 100644 --- a/configurator/src/app/startup.rs +++ b/configurator/src/app/startup.rs @@ -72,12 +72,8 @@ impl ConfiguratorApp { ConfiguratorScreen::Daemon => self.active_tab = TabId::Daemon, ConfiguratorScreen::Arrow => self.active_tab = TabId::Arrow, ConfiguratorScreen::RenderProfiles => self.active_tab = TabId::RenderProfiles, - ConfiguratorScreen::Tablet => { - #[cfg(feature = "tablet-input")] - { - self.active_tab = TabId::Tablet; - } - } + #[cfg(feature = "tablet-input")] + ConfiguratorScreen::Tablet => self.active_tab = TabId::Tablet, ConfiguratorScreen::Keybindings(section) => { self.active_tab = TabId::Keybindings; // No section means the tab, on whichever subtab it already @@ -199,6 +195,7 @@ mod tests { ("daemon", TabId::Daemon), ("arrow", TabId::Arrow), ("render-profiles", TabId::RenderProfiles), + #[cfg(feature = "tablet-input")] ("tablet", TabId::Tablet), ("keybindings", TabId::Keybindings), ] { @@ -314,6 +311,16 @@ mod tests { assert!(!app.startup_search_focus_pending); } + #[test] + #[cfg(not(feature = "tablet-input"))] + fn tablet_is_an_unknown_destination_without_tablet_input() { + let (app, _dir, _path) = app_launched_with(&["--open", "tablet"]); + + assert_eq!(app.active_tab, TabId::Daemon); + let text = status_text(&app.status); + assert!(text.contains("Unknown destination: tablet"), "{text}"); + } + #[test] fn an_unrecognized_argument_still_opens_its_destination() { let (app, _dir, _path) = app_launched_with(&["--frobnicate", "--open", "session"]); diff --git a/src/configurator_destination.rs b/src/configurator_destination.rs index dc79838e..a900d647 100644 --- a/src/configurator_destination.rs +++ b/src/configurator_destination.rs @@ -288,6 +288,7 @@ pub enum ConfiguratorScreen { Daemon, Arrow, RenderProfiles, + #[cfg(feature = "tablet-input")] Tablet, /// The Keybindings tab, optionally on a named section. Keybindings(Option), @@ -295,7 +296,7 @@ pub enum ConfiguratorScreen { impl ConfiguratorScreen { /// Every nameable screen, in the order help text lists them. - pub const ALL: [Self; 28] = [ + pub const ALL: &'static [Self] = &[ Self::UiToolbar, Self::UiToolbarVisibility, Self::UiStatusBar, @@ -313,6 +314,7 @@ impl ConfiguratorScreen { Self::Daemon, Self::Arrow, Self::RenderProfiles, + #[cfg(feature = "tablet-input")] Self::Tablet, Self::Keybindings(None), Self::Keybindings(Some(KeybindingsSection::General)), @@ -346,6 +348,7 @@ impl ConfiguratorScreen { Self::Daemon => "daemon", Self::Arrow => "arrow", Self::RenderProfiles => "render-profiles", + #[cfg(feature = "tablet-input")] Self::Tablet => "tablet", Self::Keybindings(None) => "keybindings", Self::Keybindings(Some(KeybindingsSection::General)) => "keybindings/general", @@ -380,6 +383,7 @@ impl ConfiguratorScreen { "daemon" => Some(Self::Daemon), "arrow" => Some(Self::Arrow), "render-profiles" => Some(Self::RenderProfiles), + #[cfg(feature = "tablet-input")] "tablet" => Some(Self::Tablet), "keybindings" => Some(Self::Keybindings(None)), "keybindings/general" => Some(Self::Keybindings(Some(KeybindingsSection::General))), @@ -498,7 +502,7 @@ mod tests { #[test] fn every_screen_round_trips_without_a_search_term() { - for screen in ConfiguratorScreen::ALL { + for &screen in ConfiguratorScreen::ALL { let destination = ConfiguratorDestination::new(screen); assert_eq!( ConfiguratorDestination::parse(&destination.as_arg()), @@ -511,7 +515,7 @@ mod tests { #[test] fn every_screen_round_trips_with_a_search_term() { - for screen in ConfiguratorScreen::ALL { + for &screen in ConfiguratorScreen::ALL { for term in SAMPLE_TERMS { let destination = ConfiguratorDestination::with_search(screen, term); assert_eq!(destination.search(), Some(term)); @@ -591,6 +595,7 @@ mod tests { ConfiguratorDestination::new(ConfiguratorScreen::RenderProfiles).as_arg(), "render-profiles" ); + #[cfg(feature = "tablet-input")] assert_eq!( ConfiguratorDestination::new(ConfiguratorScreen::Tablet).as_arg(), "tablet" @@ -638,6 +643,18 @@ mod tests { ); assert_eq!(ConfiguratorDestination::parse("drawing?focus=colors"), None); assert_eq!(ConfiguratorDestination::parse("drawing?"), None); + #[cfg(not(feature = "tablet-input"))] + assert_eq!(ConfiguratorDestination::parse("tablet"), None); + } + + #[test] + #[cfg(feature = "tablet-input")] + fn tablet_is_a_destination_when_the_feature_is_on() { + assert_eq!( + ConfiguratorScreen::parse("tablet"), + Some(ConfiguratorScreen::Tablet) + ); + assert!(ConfiguratorScreen::ALL.contains(&ConfiguratorScreen::Tablet)); } #[test] From 0273d409ccf33f9b22b97efeebc9a4b2735c4329 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Sat, 15 Aug 2026 01:27:30 +0200 Subject: [PATCH 20/24] test: assert a parser-rejected shortcut cannot save Save now rejects unparseable keybindings before writing. Cover that the document stays and the field error is shown, instead of only checking a warning status. --- configurator/src/app/update/config/tests.rs | 21 ++++++++++++--------- configurator/src/models/config/tests.rs | 9 +++++---- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/configurator/src/app/update/config/tests.rs b/configurator/src/app/update/config/tests.rs index a20f37ce..9e3ac277 100644 --- a/configurator/src/app/update/config/tests.rs +++ b/configurator/src/app/update/config/tests.rs @@ -768,8 +768,7 @@ fn a_save_without_shortcut_trouble_stays_a_plain_success() { } /// A shortcut the editor accepts as text but the parser rejects never -/// reaches the file either, so the save status is the only place it can be -/// reported. +/// reaches a write. Save reports the field error instead of asking for one. #[test] fn a_typed_shortcut_the_parser_rejects_is_reported_by_the_save() { let (mut app, _dir, _path) = app_with_config_file(&format!( @@ -779,16 +778,20 @@ fn a_typed_shortcut_the_parser_rejects_is_reported_by_the_save() { .keybindings .set(KeybindingField::ClearCanvas, "Ctrl+Shift".to_string()); - let _ = save_draft(&mut app); + let effects = app.handle_save_requested(); + assert!(effects.is_empty()); + assert!(!app.is_saving); assert!( - matches!(app.status, StatusMessage::Warning(_)), - "unexpected status: {:?}", - app.status + app.base_document.is_some(), + "a refused save must not take the document with it" ); - assert!(status_contains(&app.status, "Ctrl+Shift")); - assert!(status_contains(&app.status, "Clear Canvas")); - assert!(status_contains(&app.status, "could not be parsed")); + assert!(status_contains( + &app.status, + "Cannot save due to validation" + )); + assert!(status_contains(&app.status, "keybindings.clear_canvas")); + assert!(status_contains(&app.status, "No key specified")); } /// A reload replaces the draft and base document when it lands, so a save diff --git a/configurator/src/models/config/tests.rs b/configurator/src/models/config/tests.rs index 4f70b38f..aed3b089 100644 --- a/configurator/src/models/config/tests.rs +++ b/configurator/src/models/config/tests.rs @@ -687,10 +687,11 @@ fn config_draft_round_trips_render_profiles() { #[test] fn config_draft_round_trips_board_pan_settings() { let mut config = Config::default(); - let mut boards = wayscriber::config::BoardsConfig::default(); - boards.pan_enabled = false; - boards.show_pan_badge = false; - config.boards = Some(boards); + config.boards = Some(wayscriber::config::BoardsConfig { + pan_enabled: false, + show_pan_badge: false, + ..Default::default() + }); let draft = ConfigDraft::from_config(&config); assert!(!draft.boards.pan_enabled); From 31b73b49b382c3ec01575c9711b500542937f973 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Sat, 15 Aug 2026 01:27:39 +0200 Subject: [PATCH 21/24] fix: read portal screenshots from any regular file:// URI The Screenshot portal does not promise a temp directory, so refusing paths outside $XDG_RUNTIME_DIR broke legitimate captures. Keep O_NOFOLLOW and the size cap, and delete only files under the runtime dir or portal cache. --- src/capture/sources/reader.rs | 91 +++++++++++++++++++++-------------- 1 file changed, 54 insertions(+), 37 deletions(-) diff --git a/src/capture/sources/reader.rs b/src/capture/sources/reader.rs index 412d33e1..f854a5e0 100644 --- a/src/capture/sources/reader.rs +++ b/src/capture/sources/reader.rs @@ -14,22 +14,18 @@ const ATTEMPT_DELAY_MS: u64 = 50; /// Read image data from a portal `file://` URI. /// -/// The path must live under `$XDG_RUNTIME_DIR` or the xdg-desktop-portal cache -/// directory. The file is opened without following symlinks, must be a regular -/// file, and is deleted only after those checks pass. +/// The Screenshot portal only promises a URI, not a particular directory, so +/// this reads any regular file without following symlinks and caps the size. +/// Files under `$XDG_RUNTIME_DIR` or the xdg-desktop-portal cache are deleted +/// after a successful read; other locations (Pictures, `/tmp`) are left in place. pub fn read_image_from_uri(uri: &str) -> Result, CaptureError> { read_image_from_uri_with_limit(uri, MAX_PORTAL_IMAGE_BYTES) } fn read_image_from_uri_with_limit(uri: &str, max_bytes: u64) -> Result, CaptureError> { let path = decode_file_uri(uri)?; - if !is_allowed_portal_path(&path) { - return Err(CaptureError::InvalidResponse( - "Portal screenshot path is not in a trusted directory".to_string(), - )); - } - log::debug!("Reading screenshot from the portal temporary file"); + log::debug!("Reading screenshot from the portal file"); let mut opened = None; for attempt in 0..MAX_ATTEMPTS { @@ -40,7 +36,7 @@ fn read_image_from_uri_with_limit(uri: &str, max_bytes: u64) -> Result, } Err(PortalOpenError::Rejected { message, delete }) => { if delete { - remove_allowed_portal_file(&path); + remove_portal_temp_file(&path); } return Err(CaptureError::ImageError(message)); } @@ -74,7 +70,7 @@ fn read_image_from_uri_with_limit(uri: &str, max_bytes: u64) -> Result, )); } if data.len() as u64 > max_bytes { - remove_allowed_portal_file(&path); + remove_portal_temp_file(&path); return Err(CaptureError::ImageError( "Portal screenshot file exceeds the size limit".to_string(), )); @@ -84,11 +80,15 @@ fn read_image_from_uri_with_limit(uri: &str, max_bytes: u64) -> Result, "Successfully read {} bytes from portal screenshot", data.len() ); - remove_allowed_portal_file(&path); + remove_portal_temp_file(&path); Ok(data) } -fn remove_allowed_portal_file(path: &Path) { +fn remove_portal_temp_file(path: &Path) { + if !is_deletable_portal_temp_path(path) { + log::debug!("Leaving portal screenshot in place; path is not a portal temp file"); + return; + } if let Err(err) = fs::remove_file(path) { log::warn!("Failed to remove portal screenshot temporary file: {err}"); } else { @@ -96,26 +96,20 @@ fn remove_allowed_portal_file(path: &Path) { } } -fn is_allowed_portal_path(path: &Path) -> bool { - let Some(name) = path.file_name().and_then(|name| name.to_str()) else { - return false; - }; - if !crate::paths::is_single_path_component(name) { - return false; - } +fn is_deletable_portal_temp_path(path: &Path) -> bool { let Some(parent) = path.parent() else { return false; }; let Ok(parent) = parent.canonicalize() else { return false; }; - allowed_portal_roots().into_iter().any(|root| { + allowed_portal_temp_roots().into_iter().any(|root| { root.canonicalize() .is_ok_and(|root| parent.starts_with(root)) }) } -fn allowed_portal_roots() -> Vec { +fn allowed_portal_temp_roots() -> Vec { let mut roots = Vec::new(); if let Some(runtime) = std::env::var_os(XDG_RUNTIME_DIR_ENV) && !runtime.is_empty() @@ -224,24 +218,21 @@ mod tests { } #[test] - fn rejects_paths_outside_trusted_directories() { + fn reads_and_leaves_files_outside_portal_temp_roots() { let temp = TempDir::new().unwrap(); - let outside = temp.path().join("outside.png"); - std::fs::write(&outside, b"secret").unwrap(); + let outside = temp.path().join("Pictures"); + std::fs::create_dir(&outside).unwrap(); + let file_path = outside.join("shot.png"); + std::fs::write(&file_path, b"portal-bytes").unwrap(); let trusted = temp.path().join("runtime"); std::fs::create_dir(&trusted).unwrap(); with_runtime_dir(&trusted, || { - let err = read_image_from_uri(&file_uri_for(&outside)).expect_err("outside path"); - match err { - CaptureError::InvalidResponse(msg) => { - assert!(msg.contains("trusted directory"), "{msg}"); - } - other => panic!("unexpected error variant: {other:?}"), - } + let data = read_image_from_uri(&file_uri_for(&file_path)).expect("read succeeds"); + assert_eq!(data, b"portal-bytes"); assert!( - outside.exists(), - "rejected portal URIs must not delete the target file" + file_path.exists(), + "portal screenshots outside temp roots must not be deleted" ); }); } @@ -267,6 +258,32 @@ mod tests { }); } + #[test] + fn oversized_files_outside_temp_roots_are_not_deleted() { + let temp = TempDir::new().unwrap(); + let outside = temp.path().join("Pictures"); + std::fs::create_dir(&outside).unwrap(); + let file_path = outside.join("huge.png"); + std::fs::write(&file_path, b"12345").unwrap(); + let trusted = temp.path().join("runtime"); + std::fs::create_dir(&trusted).unwrap(); + + with_runtime_dir(&trusted, || { + let err = read_image_from_uri_with_limit(&file_uri_for(&file_path), 4) + .expect_err("oversize file"); + match err { + CaptureError::ImageError(msg) => { + assert!(msg.contains("size limit"), "{msg}"); + } + other => panic!("unexpected error variant: {other:?}"), + } + assert!( + file_path.exists(), + "an oversized portal file outside temp roots must not be deleted" + ); + }); + } + #[test] #[cfg(unix)] fn rejects_symlinks_without_following_them() { @@ -296,15 +313,15 @@ mod tests { } #[test] - fn allowed_portal_path_requires_runtime_or_cache_root() { + fn deletable_portal_path_requires_runtime_or_cache_root() { let temp = TempDir::new().unwrap(); let file_path = temp.path().join("shot.png"); std::fs::write(&file_path, b"x").unwrap(); with_runtime_dir(temp.path(), || { - assert!(is_allowed_portal_path(&file_path)); + assert!(is_deletable_portal_temp_path(&file_path)); }); crate::test_env::with_env_var(XDG_RUNTIME_DIR_ENV, None, || { - assert!(!is_allowed_portal_path(&file_path)); + assert!(!is_deletable_portal_temp_path(&file_path)); }); } } From d2f77704fb9ec9da1888de360a6af20f0371839a Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Sat, 15 Aug 2026 01:27:39 +0200 Subject: [PATCH 22/24] fix: detach the GTK toolbar thread when shutdown times out Forgetting the JoinHandle at the deadline leaked a handle that could never be joined. Drop it so the OS can reap the thread if it later finishes. --- src/toolbar_gtk/bridge.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/toolbar_gtk/bridge.rs b/src/toolbar_gtk/bridge.rs index 21535e83..fecfd305 100644 --- a/src/toolbar_gtk/bridge.rs +++ b/src/toolbar_gtk/bridge.rs @@ -427,12 +427,10 @@ fn finish_thread_within( std::thread::yield_now(); } if !handle.is_finished() { - // Dropping a JoinHandle detaches the thread. GTK's main loop is not - // safe to abandon that way, so leak the handle and let process exit - // collect the thread if it never finishes. - if let Some(handle) = thread.take() { - std::mem::forget(handle); - } + // Bounded shutdown: drop the JoinHandle to detach. GTK may keep + // running until process exit; detach lets the OS reap the thread if + // it later finishes. + let _ = thread.take(); return ThreadShutdownOutcome::TimedOut; } let handle = thread @@ -520,7 +518,7 @@ impl Drop for GtkToolbarBridge { } ThreadShutdownOutcome::TimedOut => { log::warn!( - "GTK toolbar thread did not stop within {:?}; leaving it running until process exit", + "GTK toolbar thread did not stop within {:?}; detaching it and leaving it running until process exit", GTK_THREAD_SHUTDOWN_TIMEOUT ); } @@ -888,7 +886,7 @@ mod tests { } #[test] - fn stuck_thread_is_leaked_not_detached_at_the_bounded_deadline() { + fn stuck_thread_is_detached_at_the_bounded_deadline() { let (completion_tx, completion_rx) = mpsc::channel(); let (release_tx, release_rx) = mpsc::channel(); let mut thread = Some(std::thread::spawn(move || { From 5ff5a91184c3b2577cfb2e4ae4b5b6201169d5a1 Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Sat, 15 Aug 2026 01:27:39 +0200 Subject: [PATCH 23/24] fix: refresh freeze/zoom crop origin on every output change A monitor added, moved, or destroyed to the left used to leave a stale screenshot origin. Recompute on add and update, and exclude the dying output in output_destroyed because SCTK 0.20 still lists it. --- src/backend/wayland/handlers/output.rs | 37 ++++++++------- src/backend/wayland/state/capture/backdrop.rs | 46 +++++++++++++++++-- src/capture/tests/desktop_backdrop.rs | 12 +++++ 3 files changed, 76 insertions(+), 19 deletions(-) diff --git a/src/backend/wayland/handlers/output.rs b/src/backend/wayland/handlers/output.rs index ead5c8d3..2d26c6ff 100644 --- a/src/backend/wayland/handlers/output.rs +++ b/src/backend/wayland/handlers/output.rs @@ -18,6 +18,7 @@ impl OutputHandler for WaylandState { ) { debug!("New output detected"); self.refresh_active_output_label(); + self.refresh_freeze_zoom_screenshot_origin(); } fn update_output( @@ -30,28 +31,28 @@ impl OutputHandler for WaylandState { if self.surface.current_output().as_ref() == Some(&output) { self.refresh_active_output_label(); } - // Refresh geometry only for the active output so fallback cropping stays correct. - if let Some(info) = self.output_state.info(&output) { - if !self.frozen.active_output_matches(info.id) - && !self.zoom.active_output_matches(info.id) - { - return; - } - - if let Some(geo) = crate::backend::wayland::frozen_geometry::OutputGeometry::update_from( + if let Some(info) = self.output_state.info(&output) + && (self.frozen.active_output_matches(info.id) + || self.zoom.active_output_matches(info.id)) + && let Some(geo) = crate::backend::wayland::frozen_geometry::OutputGeometry::update_from( info.logical_position, info.logical_size, (self.surface.width(), self.surface.height()), info.scale_factor.max(1), info.transform, - ) { - self.set_freeze_zoom_geometry(Some(geo)); - self.frozen - .set_active_output(Some(output.clone()), Some(info.id)); - self.zoom - .set_active_output(Some(output.clone()), Some(info.id)); - } + ) + { + self.set_freeze_zoom_geometry(Some(geo)); + self.frozen + .set_active_output(Some(output.clone()), Some(info.id)); + self.zoom + .set_active_output(Some(output.clone()), Some(info.id)); + return; } + // Screenshot origin walks every output, so a non-active monitor that + // is added, moved, scaled, or given logical geometry still has to + // refresh the active crop. + self.refresh_freeze_zoom_screenshot_origin(); } fn output_destroyed( @@ -66,5 +67,9 @@ impl OutputHandler for WaylandState { self.set_has_seen_surface_enter(false); } self.refresh_active_output_label(); + // SCTK 0.20 calls this before removing the output from OutputState, so + // a walk of current outputs would still include it. Exclude it here; + // there is no later callback after the removal. + self.refresh_freeze_zoom_screenshot_origin_excluding(Some(&output)); } } diff --git a/src/backend/wayland/state/capture/backdrop.rs b/src/backend/wayland/state/capture/backdrop.rs index 35cfcd52..1d646969 100644 --- a/src/backend/wayland/state/capture/backdrop.rs +++ b/src/backend/wayland/state/capture/backdrop.rs @@ -65,13 +65,26 @@ fn transformed_output_size(width: u32, height: u32, transform: wl_output::Transf impl WaylandState { pub(in crate::backend::wayland) fn desktop_backdrop_geometry( &self, + ) -> Option { + self.desktop_backdrop_geometry_excluding(None) + } + + pub(in crate::backend::wayland) fn desktop_backdrop_geometry_excluding( + &self, + exclude: Option<&wl_output::WlOutput>, ) -> Option { let output = self.surface.current_output()?; + if exclude.is_some_and(|destroyed| destroyed == &output) { + return None; + } let active_info = self.output_state.info(&output)?; let active = desktop_backdrop_output_geometry_from_info(&active_info)?; let mut outputs = Vec::new(); - for output in self.output_state.outputs() { - let info = self.output_state.info(&output)?; + for candidate in self.output_state.outputs() { + if exclude.is_some_and(|destroyed| destroyed == &candidate) { + continue; + } + let info = self.output_state.info(&candidate)?; outputs.push(desktop_backdrop_output_geometry_from_info(&info)?); } @@ -81,14 +94,41 @@ impl WaylandState { pub(in crate::backend::wayland) fn set_freeze_zoom_geometry( &mut self, geometry: Option, + ) { + self.set_freeze_zoom_geometry_excluding(geometry, None); + } + + pub(in crate::backend::wayland) fn set_freeze_zoom_geometry_excluding( + &mut self, + geometry: Option, + exclude: Option<&wl_output::WlOutput>, ) { let screenshot_origin = self - .desktop_backdrop_geometry() + .desktop_backdrop_geometry_excluding(exclude) .and_then(DesktopBackdropGeometry::physical_origin); let geometry = geometry.map(|geo| geo.with_screenshot_origin(screenshot_origin)); self.frozen.set_active_geometry(geometry.clone()); self.zoom.set_active_geometry(geometry); } + + pub(in crate::backend::wayland) fn refresh_freeze_zoom_screenshot_origin(&mut self) { + self.refresh_freeze_zoom_screenshot_origin_excluding(None); + } + + pub(in crate::backend::wayland) fn refresh_freeze_zoom_screenshot_origin_excluding( + &mut self, + exclude: Option<&wl_output::WlOutput>, + ) { + let Some(geometry) = self + .frozen + .active_geometry() + .cloned() + .or_else(|| self.zoom.active_geometry().cloned()) + else { + return; + }; + self.set_freeze_zoom_geometry_excluding(Some(geometry), exclude); + } } #[cfg(test)] diff --git a/src/capture/tests/desktop_backdrop.rs b/src/capture/tests/desktop_backdrop.rs index b06a91ed..4a1b21e2 100644 --- a/src/capture/tests/desktop_backdrop.rs +++ b/src/capture/tests/desktop_backdrop.rs @@ -135,6 +135,18 @@ fn desktop_backdrop_single_output_at_nonzero_logical_origin_crops_at_zero() { assert_eq!(geometry.physical_origin(), Some((0, 0))); } +#[test] +fn desktop_backdrop_origin_shifts_when_a_left_output_is_removed() { + let left = output(-4, 0, 4, 1, 6, 1); + let right = output(0, 0, 4, 1, 4, 1); + let with_left = + DesktopBackdropGeometry::from_outputs(right, &[left, right], 1).expect("with left"); + let without_left = DesktopBackdropGeometry::from_outputs(right, &[right], 1).expect("removed"); + + assert_eq!(with_left.physical_origin(), Some((6, 0))); + assert_eq!(without_left.physical_origin(), Some((0, 0))); +} + #[test] fn desktop_backdrop_normalizes_negative_output_origins() { let outputs = [output(-2, 0, 2, 2, 2, 2), output(0, 0, 3, 2, 3, 2)]; From 18abc65df6be2f4a260262ebd0a9b41b19746d1b Mon Sep 17 00:00:00 2001 From: devmobasa <4170275+devmobasa@users.noreply.github.com> Date: Sat, 15 Aug 2026 01:27:39 +0200 Subject: [PATCH 24/24] fix: reject portal freeze/zoom captures after the layout changes Portal tasks snapshot crop geometry before the screenshot returns, and output-id matching misses a monitor added or moved to the left. Track a layout generation, fail closed on an unknown origin unless the image is this output's size, and discard stale results. --- src/backend/wayland/frozen/mod.rs | 1 + src/backend/wayland/frozen/portal.rs | 77 ++++++++++++++++++++-- src/backend/wayland/frozen/state.rs | 59 +++++++++++++++-- src/backend/wayland/frozen_geometry.rs | 30 +++++---- src/backend/wayland/zoom/mod.rs | 6 +- src/backend/wayland/zoom/portal.rs | 89 ++++++++++++++++++++++++-- src/backend/wayland/zoom/state.rs | 11 ++++ 7 files changed, 245 insertions(+), 28 deletions(-) diff --git a/src/backend/wayland/frozen/mod.rs b/src/backend/wayland/frozen/mod.rs index 4f0980cb..f51803c2 100644 --- a/src/backend/wayland/frozen/mod.rs +++ b/src/backend/wayland/frozen/mod.rs @@ -12,6 +12,7 @@ pub use state::FrozenState; type PortalCaptureResult = Result< ( Option, + u64, Option, self::image::FrozenImage, ), diff --git a/src/backend/wayland/frozen/portal.rs b/src/backend/wayland/frozen/portal.rs index dee4d17c..f5e6b397 100644 --- a/src/backend/wayland/frozen/portal.rs +++ b/src/backend/wayland/frozen/portal.rs @@ -33,6 +33,7 @@ impl FrozenState { let source_geometry = self.active_geometry.clone(); let target_output_id = self.active_output_id; + let layout_generation = self.output_layout_generation; // Notify user that portal fallback is in progress crate::notification::send_notification_async( tokio_handle, @@ -49,6 +50,7 @@ impl FrozenState { Ok(( target_output_id, + layout_generation, source_geometry, FrozenImage { width, @@ -94,13 +96,18 @@ impl FrozenState { .map(PortalTask::poll) .unwrap_or(PortalPoll::Disconnected); match poll { - PortalPoll::Ready(Ok((target_output, source_geometry, image))) => { + PortalPoll::Ready(Ok((target_output, layout_generation, source_geometry, image))) => { let output_matches = portal_output_matches(target_output, self.active_output_id); + let layout_matches = layout_generation == self.output_layout_generation; - if output_matches { + if output_matches && layout_matches { self.set_pending_desktop_image(image, target_output, source_geometry); } else { - warn!("Portal capture for inactive output discarded"); + if !layout_matches { + warn!("Portal capture discarded after the output layout changed"); + } else { + warn!("Portal capture for inactive output discarded"); + } self.capture_done = true; } @@ -173,6 +180,20 @@ mod tests { } } + fn crop_geometry( + origin: (u32, u32), + ) -> crate::backend::wayland::frozen_geometry::OutputGeometry { + crate::backend::wayland::frozen_geometry::OutputGeometry { + logical_x: 0, + logical_y: 0, + logical_width: 2, + logical_height: 1, + scale: 1, + transform: wayland_client::protocol::wl_output::Transform::Normal, + screenshot_origin: Some(origin), + } + } + async fn poll_until_finished( frozen: &mut FrozenState, input: &mut InputState, @@ -196,7 +217,7 @@ mod tests { frozen.portal_task = Some(PortalTask::spawn( &tokio::runtime::Handle::current(), wake.handle(), - async { Ok((None, None, image(0))) }, + async { Ok((None, 0, None, image(0))) }, )); frozen.portal_in_progress = true; poll_until_finished(&mut frozen, &mut input).await?; @@ -312,7 +333,7 @@ mod tests { frozen.portal_task = Some(PortalTask::spawn( &tokio::runtime::Handle::current(), wake.handle(), - async { Ok((Some(1), None, image(9))) }, + async { Ok((Some(1), 0, None, image(9))) }, )); frozen.portal_in_progress = true; @@ -324,6 +345,52 @@ mod tests { Ok(()) } + #[tokio::test] + async fn stale_layout_is_discarded_without_a_pending_image() -> anyhow::Result<()> { + let wake = crate::backend::wayland::RuntimeWakeSource::new()?; + let mut frozen = FrozenState::new_with_runtime_wake(None, wake.handle()); + let mut input = make_test_input_state(); + frozen.set_active_geometry(Some(crop_geometry((0, 0)))); + let layout_generation = frozen.output_layout_generation; + frozen.portal_task = Some(PortalTask::spawn( + &tokio::runtime::Handle::current(), + wake.handle(), + async move { Ok((None, layout_generation, None, image(9))) }, + )); + frozen.portal_in_progress = true; + frozen.set_active_geometry(Some(crop_geometry((6, 0)))); + + poll_until_finished(&mut frozen, &mut input).await?; + + assert!(!frozen.has_pending_image()); + assert!(frozen.take_capture_done()); + Ok(()) + } + + #[tokio::test] + async fn matching_layout_still_applies_after_an_identical_geometry_refresh() + -> anyhow::Result<()> { + let wake = crate::backend::wayland::RuntimeWakeSource::new()?; + let mut frozen = FrozenState::new_with_runtime_wake(None, wake.handle()); + let mut input = make_test_input_state(); + let geometry = crop_geometry((0, 0)); + frozen.set_active_geometry(Some(geometry.clone())); + let layout_generation = frozen.output_layout_generation; + frozen.portal_task = Some(PortalTask::spawn( + &tokio::runtime::Handle::current(), + wake.handle(), + async move { Ok((None, layout_generation, None, image(0))) }, + )); + frozen.portal_in_progress = true; + frozen.set_active_geometry(Some(geometry)); + + poll_until_finished(&mut frozen, &mut input).await?; + + assert!(frozen.has_pending_image()); + assert!(!frozen.take_capture_done()); + Ok(()) + } + #[tokio::test] async fn supersession_is_ignored_and_explicit_cancel_owns_task_cancellation() -> anyhow::Result<()> { diff --git a/src/backend/wayland/frozen/state.rs b/src/backend/wayland/frozen/state.rs index 6830b02f..add54a1b 100644 --- a/src/backend/wayland/frozen/state.rs +++ b/src/backend/wayland/frozen/state.rs @@ -115,6 +115,9 @@ pub struct FrozenState { pub(super) active_output: Option, pub(super) active_output_id: Option, pub(super) active_geometry: Option, + /// Bumped when freeze/zoom crop geometry actually changes so in-flight + /// portal captures can be rejected after a layout change. + pub(super) output_layout_generation: u64, pub(super) direct_capture: Option, pub(super) image: Option, image_target_dimensions: Option<(u32, u32)>, @@ -165,6 +168,7 @@ impl FrozenState { active_output: None, active_output_id: None, active_geometry: None, + output_layout_generation: 0, direct_capture: None, image: None, image_target_dimensions: None, @@ -201,9 +205,16 @@ impl FrozenState { } pub fn set_active_geometry(&mut self, geometry: Option) { + if self.active_geometry != geometry { + self.output_layout_generation = self.output_layout_generation.wrapping_add(1); + } self.active_geometry = geometry; } + pub fn active_geometry(&self) -> Option<&OutputGeometry> { + self.active_geometry.as_ref() + } + pub fn active_output_matches(&self, info_id: u32) -> bool { self.active_output_id == Some(info_id) } @@ -392,10 +403,8 @@ impl FrozenState { if target_width == 0 || target_height == 0 { return None; } - let (origin_x, origin_y) = source_geometry - .or(self.active_geometry.as_ref()) - .map(OutputGeometry::portal_crop_origin) - .unwrap_or((0, 0)); + let geometry = source_geometry.or(self.active_geometry.as_ref())?; + let (origin_x, origin_y) = geometry.portal_crop_origin(image.width, image.height)?; let (width, height, data) = crop_argb( &image.data, image.width, @@ -658,6 +667,19 @@ mod tests { } } + #[test] + fn output_layout_generation_bumps_only_when_geometry_changes() { + let mut state = FrozenState::new(None); + assert_eq!(state.output_layout_generation, 0); + let first = desktop_geometry(0, 0, 4, 1, 1, Some((0, 0))); + state.set_active_geometry(Some(first.clone())); + assert_eq!(state.output_layout_generation, 1); + state.set_active_geometry(Some(first)); + assert_eq!(state.output_layout_generation, 1); + state.set_active_geometry(Some(desktop_geometry(0, 0, 4, 1, 1, Some((6, 0))))); + assert_eq!(state.output_layout_generation, 2); + } + #[test] fn desktop_capture_crops_from_screenshot_origin_not_logical_times_scale() { let mut state = FrozenState::new(None); @@ -697,7 +719,8 @@ mod tests { let mut input_state = make_test_input_state(); let geometry = desktop_geometry(10, 20, 2, 1, 2, None); assert_eq!(geometry.physical_origin(), (20, 40)); - assert_eq!(geometry.portal_crop_origin(), (0, 0)); + assert_eq!(geometry.portal_crop_origin(4, 2), Some((0, 0))); + assert_eq!(geometry.portal_crop_origin(10, 2), None); state.set_pending_desktop_image( FrozenImage { @@ -717,6 +740,32 @@ mod tests { assert!(input_state.frozen_active()); } + #[test] + fn desktop_capture_fails_when_screenshot_origin_is_unknown_and_image_is_larger() { + let mut state = FrozenState::new(None); + let mut input_state = make_test_input_state(); + let geometry = desktop_geometry(10, 20, 2, 1, 2, None); + + state.set_pending_desktop_image( + FrozenImage { + width: 10, + height: 2, + stride: 40, + data: vec![7; 10 * 2 * 4], + }, + None, + Some(geometry), + ); + + assert!( + state + .activate_pending_image(4, 2, &mut input_state) + .is_err() + ); + assert!(state.image().is_none()); + assert!(!input_state.frozen_active()); + } + #[test] fn pending_capture_is_discarded_if_output_changes_before_activation() { let mut state = FrozenState::new(None); diff --git a/src/backend/wayland/frozen_geometry.rs b/src/backend/wayland/frozen_geometry.rs index 0fe2e3e5..6a6244ab 100644 --- a/src/backend/wayland/frozen_geometry.rs +++ b/src/backend/wayland/frozen_geometry.rs @@ -1,7 +1,7 @@ use wayland_client::protocol::wl_output; /// Geometry and scale details for the active output, used for cropping fallback captures. -#[derive(Clone, Debug)] +#[derive(Clone, Debug, PartialEq, Eq)] pub struct OutputGeometry { #[allow(dead_code)] // part of the output snapshot; tests read origin via physical_origin pub logical_x: i32, @@ -63,11 +63,12 @@ mod tests { assert_eq!(geo.transform, wl_output::Transform::_270); assert_eq!(geo.physical_size(), (3840, 2160)); assert_eq!(geo.physical_origin(), (20, 40)); - assert_eq!(geo.portal_crop_origin(), (0, 0)); + assert_eq!(geo.portal_crop_origin(3840, 2160), Some((0, 0))); + assert_eq!(geo.portal_crop_origin(8000, 2160), None); assert_eq!( geo.with_screenshot_origin(Some((6, 0))) - .portal_crop_origin(), - (6, 0) + .portal_crop_origin(8000, 2160), + Some((6, 0)) ); } @@ -121,7 +122,9 @@ impl OutputGeometry { /// Returns physical pixel origin of the logical position on this output. /// /// Portal desktop screenshots must use [`Self::portal_crop_origin`] instead: - /// mixed-scale layouts are not `logical * this output's scale`. + /// mixed-scale layouts are not `logical * this output's scale`, and an + /// unknown origin must not be treated as `(0, 0)` unless the capture is + /// already this output's physical size. #[allow(dead_code)] // used by geometry tests; portal crop uses screenshot_origin pub fn physical_origin(&self) -> (i32, i32) { ( @@ -135,12 +138,17 @@ impl OutputGeometry { self } - /// Crop origin inside a portal/desktop screenshot. + /// Crop origin inside a portal/desktop screenshot of `image_width` × + /// `image_height`. /// - /// Unknown origin falls back to the buffer origin. A full-desktop shot of a - /// single output starts at `(0, 0)` even when that output's logical position - /// is not the layout origin. - pub fn portal_crop_origin(&self) -> (u32, u32) { - self.screenshot_origin.unwrap_or((0, 0)) + /// Unknown origin is `(0, 0)` only when those dimensions are this output's + /// physical size (a single-output capture). Otherwise the origin stays + /// unknown so a multi-output shot is not cropped from the first monitor. + pub fn portal_crop_origin(&self, image_width: u32, image_height: u32) -> Option<(u32, u32)> { + if let Some(origin) = self.screenshot_origin { + return Some(origin); + } + let (phys_width, phys_height) = self.physical_size(); + (image_width == phys_width && image_height == phys_height).then_some((0, 0)) } } diff --git a/src/backend/wayland/zoom/mod.rs b/src/backend/wayland/zoom/mod.rs index ac649571..9249afb2 100644 --- a/src/backend/wayland/zoom/mod.rs +++ b/src/backend/wayland/zoom/mod.rs @@ -9,6 +9,10 @@ const MIN_ZOOM_SCALE: f64 = 1.0; const MAX_ZOOM_SCALE: f64 = 8.0; type PortalCaptureResult = Result< - (Option, crate::backend::wayland::frozen::FrozenImage), + ( + Option, + u64, + crate::backend::wayland::frozen::FrozenImage, + ), crate::capture::types::CaptureError, >; diff --git a/src/backend/wayland/zoom/portal.rs b/src/backend/wayland/zoom/portal.rs index 4fd87568..047ef2d7 100644 --- a/src/backend/wayland/zoom/portal.rs +++ b/src/backend/wayland/zoom/portal.rs @@ -32,6 +32,7 @@ impl ZoomState { let geo = self.active_geometry.clone(); let target_output_id = self.active_output_id; + let layout_generation = self.output_layout_generation; crate::notification::send_notification_async( tokio_handle, "Zoom capture".to_string(), @@ -47,7 +48,11 @@ impl ZoomState { if let Some(geo) = geo { let (phys_w, phys_h) = geo.physical_size(); - let (origin_x, origin_y) = geo.portal_crop_origin(); + let Some((origin_x, origin_y)) = geo.portal_crop_origin(width, height) else { + return Err(CaptureError::ImageError( + "Zoom capture does not contain the active output".to_string(), + )); + }; let Some((cropped_w, cropped_h, cropped)) = crop_argb(&data, width, height, origin_x, origin_y, phys_w, phys_h) else { @@ -67,6 +72,7 @@ impl ZoomState { Ok(( target_output_id, + layout_generation, FrozenImage { width, height, @@ -109,13 +115,18 @@ impl ZoomState { .map(PortalTask::poll) .unwrap_or(PortalPoll::Disconnected); match poll { - PortalPoll::Ready(Ok((target_output, image))) => { + PortalPoll::Ready(Ok((target_output, layout_generation, image))) => { let output_matches = portal_output_matches(target_output, self.active_output_id); + let layout_matches = layout_generation == self.output_layout_generation; - if output_matches { + if output_matches && layout_matches { self.set_image(image); } else { - warn!("Portal zoom capture for inactive output discarded"); + if !layout_matches { + warn!("Portal zoom capture discarded after the output layout changed"); + } else { + warn!("Portal zoom capture for inactive output discarded"); + } self.finish_failed_portal_task(input_state); return; } @@ -191,6 +202,20 @@ mod tests { } } + fn crop_geometry( + origin: (u32, u32), + ) -> crate::backend::wayland::frozen_geometry::OutputGeometry { + crate::backend::wayland::frozen_geometry::OutputGeometry { + logical_x: 0, + logical_y: 0, + logical_width: 2, + logical_height: 1, + scale: 1, + transform: wayland_client::protocol::wl_output::Transform::Normal, + screenshot_origin: Some(origin), + } + } + async fn poll_until_finished(zoom: &mut ZoomState, input: &mut InputState) { for _ in 0..100 { zoom.poll_portal_capture(input, Instant::now()); @@ -211,7 +236,7 @@ mod tests { zoom.portal_task = Some(PortalTask::spawn( &tokio::runtime::Handle::current(), wake.handle(), - async { Ok((None, image(3))) }, + async { Ok((None, 0, image(3))) }, )); zoom.portal_in_progress = true; @@ -293,7 +318,7 @@ mod tests { zoom.portal_task = Some(PortalTask::spawn( &tokio::runtime::Handle::current(), wake.handle(), - async { Ok((Some(1), image(9))) }, + async { Ok((Some(1), 0, image(9))) }, )); zoom.portal_in_progress = true; @@ -306,6 +331,58 @@ mod tests { assert!(zoom.take_capture_done()); } + #[tokio::test] + async fn stale_layout_preserves_the_current_zoom_image_and_activation() { + let wake = crate::backend::wayland::RuntimeWakeSource::new().unwrap(); + let mut zoom = ZoomState::new_with_runtime_wake(None, wake.handle()); + let mut input = make_test_input_state(); + zoom.set_image(image(4)); + let generation = zoom.image_generation(); + zoom.set_active_geometry(Some(crop_geometry((0, 0)))); + let layout_generation = zoom.output_layout_generation; + zoom.request_activation(); + zoom.portal_task = Some(PortalTask::spawn( + &tokio::runtime::Handle::current(), + wake.handle(), + async move { Ok((None, layout_generation, image(9))) }, + )); + zoom.portal_in_progress = true; + zoom.set_active_geometry(Some(crop_geometry((6, 0)))); + + poll_until_finished(&mut zoom, &mut input).await; + + assert!(!zoom.active); + assert!(!zoom.pending_activation); + assert_eq!(zoom.image_generation(), generation); + assert_eq!(zoom.image().unwrap().data, vec![4; 8]); + assert!(zoom.take_capture_done()); + } + + #[tokio::test] + async fn matching_layout_still_applies_after_an_identical_geometry_refresh() { + let wake = crate::backend::wayland::RuntimeWakeSource::new().unwrap(); + let mut zoom = ZoomState::new_with_runtime_wake(None, wake.handle()); + let mut input = make_test_input_state(); + let geometry = crop_geometry((0, 0)); + zoom.set_active_geometry(Some(geometry.clone())); + let layout_generation = zoom.output_layout_generation; + zoom.request_activation(); + zoom.portal_task = Some(PortalTask::spawn( + &tokio::runtime::Handle::current(), + wake.handle(), + async move { Ok((None, layout_generation, image(3))) }, + )); + zoom.portal_in_progress = true; + zoom.set_active_geometry(Some(geometry)); + + poll_until_finished(&mut zoom, &mut input).await; + + assert!(zoom.active); + assert!(!zoom.pending_activation); + assert_eq!(zoom.image().unwrap().data, vec![3; 8]); + assert!(zoom.take_capture_done()); + } + #[tokio::test] async fn supersession_is_ignored_and_explicit_abort_owns_task_cancellation() { let wake = crate::backend::wayland::RuntimeWakeSource::new().unwrap(); diff --git a/src/backend/wayland/zoom/state.rs b/src/backend/wayland/zoom/state.rs index 846c083d..092fcf43 100644 --- a/src/backend/wayland/zoom/state.rs +++ b/src/backend/wayland/zoom/state.rs @@ -16,6 +16,9 @@ pub struct ZoomState { pub(super) active_output: Option, pub(super) active_output_id: Option, pub(super) active_geometry: Option, + /// Bumped when freeze/zoom crop geometry actually changes so in-flight + /// portal captures can be rejected after a layout change. + pub(super) output_layout_generation: u64, pub(super) capture: Option, pub(super) image: Option, image_generation: u64, @@ -57,6 +60,7 @@ impl ZoomState { active_output: None, active_output_id: None, active_geometry: None, + output_layout_generation: 0, capture: None, image: None, image_generation: 0, @@ -87,9 +91,16 @@ impl ZoomState { } pub fn set_active_geometry(&mut self, geometry: Option) { + if self.active_geometry != geometry { + self.output_layout_generation = self.output_layout_generation.wrapping_add(1); + } self.active_geometry = geometry; } + pub fn active_geometry(&self) -> Option<&OutputGeometry> { + self.active_geometry.as_ref() + } + pub fn active_output_matches(&self, info_id: u32) -> bool { self.active_output_id == Some(info_id) }