From 7e5994b8cb235820b7382eeb6b97dcc4abfb3d3f Mon Sep 17 00:00:00 2001 From: Tommaso Bianchi Date: Sun, 2 Aug 2026 10:31:21 +0200 Subject: [PATCH] Design: right-click a body row opens the offer, and taking a body always means the same thing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The third door onto the offer, after the viewport right-click and the Menu key. A body ROW is an unambiguous body, so the offer reports BodySolid and the body verbs act on the row you can see highlighted — the confirmation a face pick cannot give, since pointing at a face lights the face and never the body the verb will change. The status line has been promising exactly this ("Body N selected — right-click for what applies to it") since before any handler existed on that list; the product was advertising a gesture that did nothing. WHAT THE RIG CAUGHT THAT THE BUILD DID NOT. The first version hung the state normalisation off wxEVT_TREE_SEL_CHANGED. But SelectItem() on a row that is ALREADY selected fires no selection event, so a stale vertex from an earlier viewport pick survived — and offer_selection_kind() tests vertex FIRST, so right-clicking the body row served the VERTEX offer while the row sat highlighted: Fillet/chamfer/draft greyed, Mirror standing where Repeat belongs, "vertex selected" still in the status line and the cyan marker still on screen. The happy path (fresh row, nothing else picked) looked perfect, which is why only the deliberate stale-state sequence exposed it. Reading the code would not have shown it — SelectItem looks like it selects. So the normalisation is no longer a selection handler. apply_body_row() is called UNCONDITIONALLY by both doors, because taking a body from the list means the same state change however it was asked for. It also clears m_sel_solid_vertex, which the original handler never did — latent while nothing opened the offer from that list, and immediately fatal once something did. Verified on both rigs with the failing sequence itself: pick a vertex, then right-click the already-selected row. Fillet/chamfer/draft enabled, Repeat back in place, status reads "Body 1 selected", vertex marker gone. Does NOT touch the feature tree. That needs new selection kinds (offer_selection_kind has no notion of "a feature is selected") plus verbs the atlas does not contain — Suppress, Rename, Reorder, Roll back — and is filed separately. --- src/slic3r/GUI/DesignPanel.cpp | 41 ++++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/src/slic3r/GUI/DesignPanel.cpp b/src/slic3r/GUI/DesignPanel.cpp index 305f255f3d..75b9352624 100644 --- a/src/slic3r/GUI/DesignPanel.cpp +++ b/src/slic3r/GUI/DesignPanel.cpp @@ -2970,21 +2970,48 @@ DesignPanel::DesignPanel(wxWindow* parent) // tree rebuild — until then an empty box would sit under the header. m_parts->Hide(); m_parts_label->Hide(); - m_parts->Bind(wxEVT_TREE_SEL_CHANGED, [this](wxTreeEvent&) { - if (!m_viewport) return; - const int b = tree_body_selection(); - if (b < 0) return; + // Taking a body from the list means the SAME state change however it was asked for, so the + // normalisation lives here and not inside a selection handler. That distinction is not + // pedantry: SelectItem() on a row that is ALREADY selected fires no SEL_CHANGED at all, so a + // version of this that only ran on selection left a stale vertex/edge from an earlier + // viewport pick in place — and offer_selection_kind() tests vertex FIRST, so right-clicking + // the body row served the VERTEX offer (Fillet greyed, Mirror in place of Repeat) while the + // row sat highlighted. Measured on the rig 2026-08-02; it is invisible from the code alone. + auto apply_body_row = [this](int b) { + if (!m_viewport || b < 0) return; // One selection at a time: a body row and a feature row mean different things to the // op bar, so clear the feature tree's highlight when a body takes over. if (m_tree) m_tree->UnselectAll(); m_viewport->set_body_highlight(false); // the per-body overlay does the tint - m_viewport->select_body(b); - m_sel_solid_body = b; - m_sel_solid_face = m_sel_solid_edge = -1; + m_viewport->select_body(b); // also drops the vertex/edge marker + m_sel_solid_body = b; + m_sel_solid_face = m_sel_solid_edge = -1; + m_sel_solid_vertex = false; m_pick_face = m_pick_face_body = -1; // chosen from the list, no face was pointed at m_status->SetForegroundColour(wxNullColour); set_status(wxString::Format(_L("Body %d selected — right-click for what applies to it"), b + 1)); m_status->Refresh(); + }; + m_parts->Bind(wxEVT_TREE_SEL_CHANGED, [this, apply_body_row](wxTreeEvent&) { + apply_body_row(tree_body_selection()); + }); + // The third door onto the offer, after the viewport right-click and the Menu key. A body ROW + // is an unambiguous body, so the offer reports BodySolid and the body verbs act on the row you + // can see highlighted. That is the confirmation a face pick cannot give: pointing at a face + // lights the face, never the body the verb will actually change. The status line above has + // been promising this right-click since before it existed. + m_parts->Bind(wxEVT_TREE_ITEM_MENU, [this, apply_body_row](wxTreeEvent& e) { + if (e.GetItem().IsOk()) + m_parts->SelectItem(e.GetItem()); // the row under the cursor, never a stale one + apply_body_row(tree_body_selection()); // unconditional — see above, SelectItem on an + // already-selected row raises no event + // GetPoint() is tree-client; it is (-1,-1) when the KEYBOARD menu key raised this, so fall + // back to the shared anchor rather than popping the menu at a garbage coordinate. + const wxPoint p = e.GetPoint(); + const wxPoint screen = (p.x >= 0 && p.y >= 0) ? m_parts->ClientToScreen(p) : offer_anchor(); + // Let the modal menu take the loop after this handler returns — same CallAfter as the + // sketch path, which learned it the hard way. + CallAfter([this, screen] { show_offer_menu(screen); }); }); // --- Variables (document-scope named expressions) ---