From 9e71cda298d18bfe43e1d2a0d594c0aa3619e2b5 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Sun, 16 Aug 2026 09:56:34 +0200 Subject: [PATCH] fix(client-linux): the library grid never got the FlowBox recursion guard, so every game click overflowed the stack MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clicking any game in the host library aborted punktfunk-client outright: thread 'main' has overflowed its stack fatal runtime error: stack overflow, aborting A click on a FlowBoxChild emits `child-activated` on the *FlowBox*, never the child's own `activate`, so a grid whose per-card handler hangs off `child.connect_activate()` has to bridge the two. The naive bridge is a cycle: FlowBoxChild's default `activate` handler re-emits `child-activated` on its parent, which re-enters the bridge, which activates the child again. 53c8eefa fixed exactly this for the host cards — but only in ui_hosts.rs. The library page had carried the same unguarded bridge since 7eea9836, and 883c3178 then added a third one for the launcher shelf. So the guard existed in one of three places, and both library grids still aborted on the first click. Rather than patch the two sites and leave the footgun loaded, the guarded bridge moves into ui_flow.rs and all three call sites go through it. The regression test moves with it, so it now covers the code both pages actually run instead of a hand-copied replica of it — which is why it never caught this. Verified in a Linux container (fmt / clippy -D warnings / build / test all green) with the display test executed for real against Xvfb rather than skipped. Removing the guard again reproduces the reported abort exactly (SIGABRT, stack overflow), so the test is known to catch the bug rather than merely pass. --- clients/linux/src/main.rs | 3 ++ clients/linux/src/ui_flow.rs | 70 +++++++++++++++++++++++++++++++++ clients/linux/src/ui_hosts.rs | 61 ++-------------------------- clients/linux/src/ui_library.rs | 11 ++---- 4 files changed, 81 insertions(+), 64 deletions(-) create mode 100644 clients/linux/src/ui_flow.rs diff --git a/clients/linux/src/main.rs b/clients/linux/src/main.rs index 7a90ff0c..dd7b5496 100644 --- a/clients/linux/src/main.rs +++ b/clients/linux/src/main.rs @@ -22,6 +22,9 @@ mod cli; mod shortcuts; #[cfg(target_os = "linux")] mod spawn; +// The guarded FlowBox `child-activated → activate` bridge every card grid needs. +#[cfg(target_os = "linux")] +mod ui_flow; #[cfg(target_os = "linux")] mod ui_hosts; #[cfg(target_os = "linux")] diff --git a/clients/linux/src/ui_flow.rs b/clients/linux/src/ui_flow.rs new file mode 100644 index 00000000..a8626b34 --- /dev/null +++ b/clients/linux/src/ui_flow.rs @@ -0,0 +1,70 @@ +//! One shared fix for a GTK4 footgun every card grid in this shell walks into. +//! +//! A pointer click (and keyboard activate) on a [`gtk::FlowBoxChild`] emits +//! `child-activated` on the *FlowBox*, never the child's own `activate` signal — so a +//! grid whose per-card handler hangs off `child.connect_activate()` has to bridge the +//! one to the other. The naive bridge is a stack overflow: `FlowBoxChild`'s default +//! `activate` handler re-emits `child-activated` on its parent, which calls the bridge, +//! which activates the child again, forever. +//! +//! [`bridge_child_activation`] is that bridge with the re-entrancy guard that breaks the +//! cycle. Use it for every `child-activated → child.activate()` hop; do not hand-roll it, +//! since a bare `flow.connect_child_activated(|_, c| c.activate())` aborts the process on +//! the first click and looks perfectly reasonable in review. + +use gtk::prelude::*; + +/// Bridge a FlowBox's `child-activated` to the activated child's own `activate` signal, +/// exactly once per click. The re-entrant emission the child's default handler bounces +/// back is swallowed rather than recursed into. +pub(crate) fn bridge_child_activation(flow: >k::FlowBox) { + let activating = std::cell::Cell::new(false); + flow.connect_child_activated(move |_, child| { + if activating.replace(true) { + return; + } + child.activate(); + activating.set(false); + }); +} + +#[cfg(test)] +mod tests { + use super::bridge_child_activation; + use gtk::prelude::*; + use std::cell::Cell; + use std::rc::Rc; + + // Reproduces the exact FlowBox/FlowBoxChild wiring the card grids use: the bridge + // calls `child.activate()`, whose own default handler re-emits `child-activated` — + // that ping-pong recursed forever (a real stack overflow on every card click/Enter, + // reported on the hosts page and then again on the library page) until the + // re-entrancy guard landed here, where both pages share it. + #[test] + #[ignore = "needs a Wayland/X display"] + fn flow_box_activation_bridge_does_not_recurse() { + assert!(gtk::init().is_ok(), "no display"); + + let flow = gtk::FlowBox::builder() + .selection_mode(gtk::SelectionMode::None) + .activate_on_single_click(true) + .build(); + bridge_child_activation(&flow); + + let child = gtk::FlowBoxChild::new(); + flow.insert(&child, -1); + let fired = Rc::new(Cell::new(0u32)); + { + let fired = fired.clone(); + child.connect_activate(move |_| fired.set(fired.get() + 1)); + } + + flow.emit_by_name::<()>("child-activated", &[&child]); + + assert_eq!( + fired.get(), + 1, + "the per-card handler should fire exactly once" + ); + } +} diff --git a/clients/linux/src/ui_hosts.rs b/clients/linux/src/ui_hosts.rs index 76e12576..28484bc0 100644 --- a/clients/linux/src/ui_hosts.rs +++ b/clients/linux/src/ui_hosts.rs @@ -781,18 +781,11 @@ impl SimpleComponent for HostsPage { // A pointer click (and keyboard activate) emits `child-activated` on the // *FlowBox*, never the child's own `activate` signal — bridge it back to the - // child, where each card wires its connect handler. The re-entrancy flag breaks - // the child-activated ↔ activate ping-pong that otherwise recurses forever - // (a real stack overflow on every card click; see the ignored display test). + // child, where each card wires its connect handler. The guard inside the bridge + // breaks the child-activated ↔ activate ping-pong that otherwise recurses forever + // (a real stack overflow on every card click; see `ui_flow`'s display test). for flow in [saved.widget(), discovered.widget()] { - let activating = std::cell::Cell::new(false); - flow.connect_child_activated(move |_, child| { - if activating.replace(true) { - return; - } - child.activate(); - activating.set(false); - }); + crate::ui_flow::bridge_child_activation(flow); } // Shown under the discovered heading while no (unsaved) advert is live yet. @@ -1466,49 +1459,3 @@ impl HostsPage { dialog.present(Some(&self.widgets.stack)); } } - -#[cfg(test)] -mod tests { - use adw::prelude::*; - use std::cell::Cell; - use std::rc::Rc; - - // Reproduces the exact FlowBox/FlowBoxChild wiring from `init()`: `child-activated` - // bridges to `child.activate()`, whose own default handler re-emits - // `child-activated` — that ping-pong recursed forever (stack overflow on every - // host-card click/Enter) until the re-entrancy guard was added. - #[test] - #[ignore = "needs a Wayland/X display"] - fn flow_box_activation_bridge_does_not_recurse() { - assert!(gtk::init().is_ok(), "no display"); - - let flow = gtk::FlowBox::builder() - .selection_mode(gtk::SelectionMode::None) - .activate_on_single_click(true) - .build(); - let activating = Cell::new(false); - flow.connect_child_activated(move |_, child| { - if activating.replace(true) { - return; - } - child.activate(); - activating.set(false); - }); - - let child = gtk::FlowBoxChild::new(); - flow.insert(&child, -1); - let fired = Rc::new(Cell::new(0u32)); - { - let fired = fired.clone(); - child.connect_activate(move |_| fired.set(fired.get() + 1)); - } - - flow.emit_by_name::<()>("child-activated", &[&child]); - - assert_eq!( - fired.get(), - 1, - "the per-card handler should fire exactly once" - ); - } -} diff --git a/clients/linux/src/ui_library.rs b/clients/linux/src/ui_library.rs index 7ec5d6d3..6574ab0b 100644 --- a/clients/linux/src/ui_library.rs +++ b/clients/linux/src/ui_library.rs @@ -116,10 +116,9 @@ fn build( .valign(gtk::Align::Start) .build(); // Click/keyboard activation fires `child-activated` on the FlowBox, not the child's own - // `activate` — bridge it so each poster's connect handler (below) runs on click. - flow.connect_child_activated(|_, child| { - child.activate(); - }); + // `activate` — bridge it so each poster's connect handler (below) runs on click. The + // bridge must be the guarded one: bare, it recurses until the stack overflows. + crate::ui_flow::bridge_child_activation(&flow); // The launcher shelf: same tile geometry as the games grid, its own FlowBox so the two // groups never interleave and each wraps on its own. let launcher_flow = gtk::FlowBox::builder() @@ -132,9 +131,7 @@ fn build( .row_spacing(18) .valign(gtk::Align::Start) .build(); - launcher_flow.connect_child_activated(|_, child| { - child.activate(); - }); + crate::ui_flow::bridge_child_activation(&launcher_flow); let launchers_heading = gtk::Label::new(Some("Launchers")); launchers_heading.add_css_class("pf-group-heading"); launchers_heading.set_halign(gtk::Align::Start); -- 2.54.0