From eb66e45b7b9dc910e79e5347f81279de8390986a Mon Sep 17 00:00:00 2001 From: Tommaso Bianchi Date: Thu, 20 Aug 2026 18:37:01 +0200 Subject: [PATCH] Design: confine the mapped-frame workaround to GTK, so macOS stops hanging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported on PR #15238: on macOS, drawing a corner rectangle and pressing Enter on the first dimension opens the second field and then wedges the entire application — no typing, no Escape, dead menu bar, no tab switching, and the field stays composited over the desktop after minimising. That last detail is what identifies it: an already-drawn window keeps being composited by the WindowServer once the process stops answering. The main thread is stuck. 4c8c93b512 is the only commit that ever changed the second-queued-field path. It stopped unmapping the value field between two queued dimensions, and its whole justification is mutter: focus-stealing prevention refuses keyboard focus to a window that was just re-mapped, which left the second field visible but dead on GNOME (snaporca-p8uw). It was applied with no platform guard, so macOS re-activates an already-visible borderless NSWindow at NSModalPanelWindowLevel from inside wxOSX's pending-event drain — which on macOS runs from a CFRunLoopObserver at kCFRunLoopBeforeTimers (evtloop_cf.cpp) over an unbounded `while (!m_handlersWithPendingEvents.IsEmpty())` (appbase.cpp). One constexpr now carries that choice, and both halves of the contract read it, because the failure mode of this fix is the two halves drifting apart: open()'s reuse-if-shown and do_commit()'s deferred hide are one decision, not two. Not a macOS guess I could not check: the non-GTK branch was exercised on the Linux rig by forcing the constant to false and rebuilding. A corner rectangle drew, its first field committed at 60 mm, the SECOND field opened, committed at 40 mm, and the sketch ended at 60.0 x 40.0 mm with the field closed and no freeze — so the map-afresh path is functionally complete, not merely different. On GTK the constant is true and every generated instruction is unchanged. What is still unproven is the exact line where macOS wedges; the reporter has been asked for a `sample` of the hung process. This fixes the cause the evidence points at without waiting for that, and cannot regress the GTK behaviour. Co-Authored-By: Claude Opus 5 (1M context) --- src/slic3r/GUI/CAD/SketchInlineEditor.cpp | 39 ++++++++++++++++++----- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/src/slic3r/GUI/CAD/SketchInlineEditor.cpp b/src/slic3r/GUI/CAD/SketchInlineEditor.cpp index c9a85e7ef7..de81eecf32 100644 --- a/src/slic3r/GUI/CAD/SketchInlineEditor.cpp +++ b/src/slic3r/GUI/CAD/SketchInlineEditor.cpp @@ -72,6 +72,23 @@ void present_toplevel(wxFrame* frame) } #endif +// Between two queued dimensions (a rectangle's Width then Height) the frame is either kept +// MAPPED and merely re-titled, or unmapped and mapped again. That is a per-toolkit choice, not +// a preference: +// GTK/mutter keep it mapped. Focus-stealing prevention refuses keyboard focus to a window +// that was just re-mapped, so hiding between the two fields left the second one +// visible but dead (snaporca-p8uw). +// elsewhere map it afresh. This is what shipped before that workaround, which was applied +// with no platform guard — and it is the only difference between the first queued +// field (works everywhere) and the second (macOS wedges the whole app, PR #15238). +// A workaround for one window manager must not become a contract for all of them. +constexpr bool keep_mapped_between_fields = +#ifdef __WXGTK__ + true; +#else + false; +#endif + void trace_inline_focus(wxFrame* frame, const std::string& title) { if (!std::getenv("SNAPORCA_KEYTRACE")) return; @@ -136,9 +153,11 @@ void SketchInlineEditor::open(const wxPoint& screen_px, double value, std::function on_cancel) { if (m_frame == nullptr || m_ctrl == nullptr) { if (on_cancel) on_cancel(); return; } - // Never close/unmap on the way in: the previous queued dimension left this frame mapped - // (see do_commit), and re-mapping a hidden toplevel is exactly what mutter refuses to - // focus. Reuse the still-mapped frame and just re-title/re-position it. + // Where the frame is kept mapped, never close/unmap on the way in: the previous queued + // dimension left it mapped (see do_commit) and re-mapping is what mutter refuses to focus, + // so reuse it and just re-title/re-position. Elsewhere, force a fresh map. + if (!keep_mapped_between_fields && m_frame->IsShown()) + m_frame->Hide(); m_commit = std::move(on_commit); m_cancel = std::move(on_cancel); m_ctrl->ChangeValue(en_format(value)); @@ -195,14 +214,18 @@ void SketchInlineEditor::do_commit() return; } auto cb = m_commit; - m_open = false; // logically closed: the frame stays MAPPED + m_open = false; // logically closed; whether it stays MAPPED is per-toolkit m_commit = nullptr; m_cancel = nullptr; + // Unmap BEFORE the callback where we are not keeping it mapped, so the reopen the callback + // schedules starts from a hidden frame — the ordering that shipped before the workaround. + if (!keep_mapped_between_fields) + m_frame->Hide(); if (cb) cb(v); - // The callback either re-opens us for the next queued dimension (via its own CallAfter, - // queued during cb(v), therefore BEFORE the one below) or it does not. Hiding here would - // unmap the window and mutter would refuse to focus the re-map; so hide only after the - // reopen has had its turn. + // Kept mapped: the callback either re-opens us for the next queued dimension (via its own + // CallAfter, queued during cb(v), therefore BEFORE the one below) or it does not. Hiding + // here would unmap the window and mutter would refuse to focus the re-map; so hide only + // after the reopen has had its turn. Harmless on the unmapped path — already hidden. m_frame->CallAfter([this] { if (!m_open && m_frame) m_frame->Hide(); }); }