`9eca12e`

fix fullscreen rendering and dialog stacking

- Implement chunked put_image for images exceeding X11 max request size
- Remove initial render before event loop to avoid stale dimensions
- Add surface/window size verification before blitting
- Pass parent_window to picker for transient-for hint

Authored by mfwolffe <wolffemf@dukes.jmu.edu> 3 months ago

SHA: 9eca12e8af0b9a8c60cd0a4bdbc671297e41aa0e
Parents: 7762f04
Tree: 69c65dd

3 changed files

Status	File	+	-
M	`garfield-portal/src/file_chooser.rs`	34	7
M	`garfield/src/app.rs`	127	24
M	`garfield/src/main.rs`	7	0

garfield-portal/src/file_chooser.rsmodified

+         }
+     }
 +    /// Parse X11 window ID from portal parent_window string.
 +    /// Format is "x11:<xid>" where xid is the window ID in hex.
 +    fn parse_parent_window(parent_window: &str) -> Option<u32> {
 +        if parent_window.starts_with("x11:") {
 +            let hex_str = &parent_window[4..];
 +            u32::from_str_radix(hex_str, 16).ok()
 +        } else {
 +            None
 +        }
 +    }
++
      /// Spawn garfield in picker mode and collect results.
      async fn spawn_picker(
          &self,
          multiple: bool,
          filters: Vec<String>,
          current_folder: Option<String>,
 +        parent_window: Option<u32>,
      ) -> (u32, HashMap<String, Value<'static>>) {
          // Find garfield binary - prefer ~/.cargo/bin (development), then /usr/local/bin, then PATH
          let home = std::env::var("HOME").unwrap_or_default();
              cmd.arg("--filter").arg(filters.join(";"));
+         }
 +        if let Some(parent) = parent_window {
 +            cmd.arg("--parent-window").arg(parent.to_string());
 +        }
++
          if let Some(folder) = current_folder {
              cmd.arg(&folder);
+         }
          title: &str,
          suggested_filename: Option<String>,
          current_folder: Option<String>,
 +        parent_window: Option<u32>,
      ) -> (u32, HashMap<String, Value<'static>>) {
          // Find garfield binary - prefer ~/.cargo/bin (development), then /usr/local/bin, then PATH
          let home = std::env::var("HOME").unwrap_or_default();
              cmd.arg("--title").arg(title);
+         }
 +        if let Some(parent) = parent_window {
 +            cmd.arg("--parent-window").arg(parent.to_string());
 +        }
++
          if let Some(folder) = current_folder {
              cmd.arg(&folder);
+         }
          #[zbus(object_server)] server: &zbus::ObjectServer,
          handle: ObjectPath<'_>,
          _app_id: &str,
 -        _parent_window: &str,
 +        parent_window: &str,
          title: &str,
          options: HashMap<&str, Value<'_>>,
      ) -> fdo::Result<(u32, HashMap<String, Value<'static>>)> {
 -        tracing::info!("OpenFile request: handle={}, title={}", handle, title);
 +        tracing::info!("OpenFile request: handle={}, title={}, parent_window={}", handle, title, parent_window);
          let handle_owned: OwnedObjectPath = handle.into();
 +        let parent_window_id = Self::parse_parent_window(parent_window);
          // Parse options
          let multiple = options.get("multiple")
              multiple,
              filters,
              current_folder,
 +            parent_window_id,
          ).await;
          tracing::debug!("Picker returned: {:?}", result.0);
          #[zbus(object_server)] server: &zbus::ObjectServer,
          handle: ObjectPath<'_>,
          _app_id: &str,
 -        _parent_window: &str,
 +        parent_window: &str,
          title: &str,
          options: HashMap<&str, Value<'_>>,
      ) -> fdo::Result<(u32, HashMap<String, Value<'static>>)> {
 -        tracing::info!("SaveFile request: handle={}, title={}", handle, title);
 +        tracing::info!("SaveFile request: handle={}, title={}, parent_window={}", handle, title, parent_window);
          tracing::debug!("SaveFile options: {:?}", options);
          let handle_owned: OwnedObjectPath = handle.into();
 +        let parent_window_id = Self::parse_parent_window(parent_window);
          let current_folder = Self::parse_current_folder(&options);
          let suggested_filename = Self::parse_current_name(&options);
 -        tracing::info!("SaveFile: folder={:?}, filename={:?}", current_folder, suggested_filename);
 +        tracing::info!("SaveFile: folder={:?}, filename={:?}, parent={:?}", current_folder, suggested_filename, parent_window_id);
          let request = Request::new(handle_owned.clone(), self.request_manager.clone());
          server.at(handle_owned.as_ref(), request).await
              title,
              suggested_filename,
              current_folder,
 +            parent_window_id,
          ).await;
          let _ = server.remove::<Request, _>(&handle_owned).await;
          #[zbus(object_server)] server: &zbus::ObjectServer,
          handle: ObjectPath<'_>,
          _app_id: &str,
 -        _parent_window: &str,
 +        parent_window: &str,
          title: &str,
          options: HashMap<&str, Value<'_>>,
      ) -> fdo::Result<(u32, HashMap<String, Value<'static>>)> {
 -        tracing::info!("SaveFiles request: handle={}, title={}", handle, title);
 +        tracing::info!("SaveFiles request: handle={}, title={}, parent_window={}", handle, title, parent_window);
          // SaveFiles picks a directory for saving multiple files
          let handle_owned: OwnedObjectPath = handle.into();
 +        let parent_window_id = Self::parse_parent_window(parent_window);
          let current_folder = Self::parse_current_folder(&options);
          let request = Request::new(handle_owned.clone(), self.request_manager.clone());
              false,
              Vec::new(),
              current_folder,
 +            parent_window_id,
          ).await;
          let _ = server.remove::<Request, _>(&handle_owned).await;

garfield/src/app.rsmodified

          let x = monitor.rect.x + (monitor.rect.width as i32 - width as i32) / 2;
          let y = monitor.rect.y + (monitor.rect.height as i32 - height as i32) / 2;
 +        tracing::debug!(
 +            "Window size: monitor={}x{}, requested={}x{} at ({}, {})",
 +            monitor.rect.width, monitor.rect.height, width, height, x, y
 +        );
++
          // Window title depends on mode
          let title = if picker_config.is_picker() {
              picker_config.title.clone().unwrap_or_else(|| {
          // Use Dialog window type for picker mode (better focus handling)
          if picker_config.is_picker() {
              window_config = window_config.window_type(gartk_x11::WindowType::Dialog);
++
 +            // Set transient-for if parent window specified (dialog belongs to parent)
 +            if let Some(parent) = picker_config.parent_window {
 +                window_config = window_config.parent_window(parent).modal(true);
 +            }
+         }
          let window = Window::create(conn.clone(), window_config)?;
      pub fn run(&mut self) -> Result<()> {
          let mut event_loop = EventLoop::new(&self.window, EventLoopConfig::default())?;
 -        // Initial render
 -        self.render()?;
 +        // Don't render before event loop - wait for ConfigureNotify/Expose
 +        // to get correct window dimensions from WM before first render
          event_loop.run(|ev, event| {
              match event {
                      ev.request_redraw();
+                 }
                  InputEvent::Resize { width, height } => {
 -                    let _ = self.renderer.resize(width, height);
 +                    tracing::debug!("ConfigureNotify resize: {}x{}", width, height);
 +                    if let Err(e) = self.renderer.resize(width, height) {
 +                        tracing::error!("Failed to resize renderer: {}", e);
 +                    }
 +                    // Update internal rect tracking (don't send X11 request - WM already sized us)
 +                    self.window.set_size(width, height);
                      self.update_layout(width, height);
                      ev.request_redraw();
+                 }
+                 }
                  PickerToolbarClick::None => {}
+             }
 -        } else {
 -            // Check normal toolbar clicks
 -            if let Some(action) = self.toolbar.on_click(pos) {
 -                self.handle_toolbar_action(action);
 -                return;
 -            }
 +        }
++
 +        // Check normal toolbar clicks (always, not just when picker_toolbar is None)
 +        if let Some(action) = self.toolbar.on_click(pos) {
 +            self.handle_toolbar_action(action);
 +            return;
+         }
          // Check pane toolbar clicks (view mode buttons)
          let sidebar_w = self.sidebar.width();
          let header_height = TAB_BAR_HEIGHT + TOOLBAR_HEIGHT + BREADCRUMB_HEIGHT;
 +        tracing::trace!("render: size={}x{}, sidebar_w={}, root_pane_bounds={:?}",
 +            size.width, size.height, sidebar_w, self.root_pane.bounds());
++
          // Clear background
          self.renderer.clear()?;
      /// Blit the rendered surface to the window.
      fn blit_surface(&mut self) -> Result<()> {
          let size = self.renderer.size();
 +        let window_size = self.window.size();
 +        let stride = self.renderer.surface().stride() as usize;
 +        let row_bytes = size.width as usize * 4; // 4 bytes per pixel (ARGB)
++
 +        // Verify surface matches window size
 +        if size.width != window_size.width || size.height != window_size.height {
 +            tracing::warn!("Surface size {}x{} doesn't match window size {}x{}, skipping blit",
 +                size.width, size.height, window_size.width, window_size.height);
 +            return Ok(());
 +        }
++
          let window_id = self.window.id();
          let depth = self.window.depth();
          let gc = self.gc;
          let conn = self.window.connection().clone();
 -        // Access surface data directly without copying, blit to X11
 -        self.renderer.surface_mut().with_data(|data| {
 -            let _ = conn.inner().put_image(
 -                ImageFormat::Z_PIXMAP,
 -                window_id,
 -                gc,
 -                size.width as u16,
 -                size.height as u16,
 -                0,
 -                0,
 -                0,
 -                depth,
 -                data,
 -            );
 -        })?;
 +        // Get maximum request size (leave some headroom for request headers)
 +        let max_request_bytes = conn.maximum_request_bytes().saturating_sub(1024);
 +        let total_bytes = row_bytes * size.height as usize;
++
 +        // Check if we need to chunk the image
 +        if total_bytes <= max_request_bytes {
 +            // Image fits in a single request
 +            tracing::trace!("blit_surface: {}x{} ({} bytes) in single request",
 +                size.width, size.height, total_bytes);
++
 +            self.renderer.surface_mut().with_data(|data| {
 +                if stride == row_bytes {
 +                    let _ = conn.inner().put_image(
 +                        ImageFormat::Z_PIXMAP,
 +                        window_id,
 +                        gc,
 +                        size.width as u16,
 +                        size.height as u16,
 +                        0,
 +                        0,
 +                        0,
 +                        depth,
 +                        data,
 +                    );
 +                } else {
 +                    let mut packed = Vec::with_capacity(total_bytes);
 +                    for y in 0..size.height as usize {
 +                        let row_start = y * stride;
 +                        let row_end = row_start + row_bytes;
 +                        if row_end <= data.len() {
 +                            packed.extend_from_slice(&data[row_start..row_end]);
 +                        }
 +                    }
 +                    let _ = conn.inner().put_image(
 +                        ImageFormat::Z_PIXMAP,
 +                        window_id,
 +                        gc,
 +                        size.width as u16,
 +                        size.height as u16,
 +                        0,
 +                        0,
 +                        0,
 +                        depth,
 +                        &packed,
 +                    );
 +                }
 +            })?;
 +        } else {
 +            // Image too large - split into horizontal bands
 +            let rows_per_chunk = max_request_bytes / row_bytes;
 +            let rows_per_chunk = rows_per_chunk.max(1); // At least 1 row per chunk
++
 +            tracing::debug!("blit_surface: {}x{} ({} bytes) exceeds max {} bytes, using {} rows per chunk",
 +                size.width, size.height, total_bytes, max_request_bytes, rows_per_chunk);
++
 +            self.renderer.surface_mut().with_data(|data| {
 +                let mut y_offset = 0u32;
 +                while y_offset < size.height {
 +                    let chunk_height = (size.height - y_offset).min(rows_per_chunk as u32);
 +                    let chunk_bytes = row_bytes * chunk_height as usize;
++
 +                    // Extract this chunk's data
 +                    let mut chunk_data = Vec::with_capacity(chunk_bytes);
 +                    for row in 0..chunk_height as usize {
 +                        let src_y = y_offset as usize + row;
 +                        let row_start = src_y * stride;
 +                        let row_end = row_start + row_bytes;
 +                        if row_end <= data.len() {
 +                            chunk_data.extend_from_slice(&data[row_start..row_end]);
 +                        }
 +                    }
++
 +                    let _ = conn.inner().put_image(
 +                        ImageFormat::Z_PIXMAP,
 +                        window_id,
 +                        gc,
 +                        size.width as u16,
 +                        chunk_height as u16,
 +                        0,
 +                        y_offset as i16,
 +                        0,
 +                        depth,
 +                        &chunk_data,
 +                    );
++
 +                    y_offset += chunk_height;
 +                }
 +            })?;
 +        }
          self.window.connection().flush()?;

garfield/src/main.rsmodified

      /// Suggested filename for save mode
      #[arg(long, requires = "save")]
      pub save_filename: Option<String>,
++
 +    /// Parent window ID for transient-for hint (X11 window ID)
 +    #[arg(long, requires = "picker")]
 +    pub parent_window: Option<u32>,
+ }
  /// Picker mode configuration parsed from CLI args.
      pub title: Option<String>,
      /// Accept button label.
      pub accept_label: String,
 +    /// Parent window ID for transient-for hint.
 +    pub parent_window: Option<u32>,
+ }
  impl PickerConfig {
              mode,
              title: args.title.clone(),
              accept_label: args.accept_label.clone().unwrap_or_else(|| default_label.to_string()),
 +            parent_window: args.parent_window,
+         }
+     }