From c32aa3f8baea9a12557fa43cbc86209c6e99ec3d Mon Sep 17 00:00:00 2001 From: Tommaso Bianchi Date: Sun, 2 Aug 2026 12:12:50 +0200 Subject: [PATCH] Design: clicking the same face twice takes the body, and the status line moves onto the viewport A click could point at a face, an edge or a vertex, but never at the body those belong to: offer_selection_kind() can only return BodySolid when all three are clear, which a viewport click never produces. The rubber band was the only door, and the status line said "face 5 selected" while the user believed they had taken the body. A second click on the SAME sub-element now escalates to it (snaporca-gem). Not the pick cycle that was removed in bc2b741ce9 -- that one was silent and three deep, so no click had a predictable meaning. Here the status line names the next click before you make it, and a further click just takes the face under the cursor again, which needs no teaching. Double-click is untouched: wx sends Down/Up/DClick/Up and only the first Up carries a pending press, so a fast double-click still zooms to fit and picks once. The status line itself moved to the base of the viewport. In the side panel it was clipped at ~73 characters with no warning and no wrap -- set_status()'s Wrap() never took effect (snaporca-8cc) -- which silently length-limited every hint in the tab; the first version of this change lost a clause to it. m_status is kept, hidden, as the owner of the text and its colour, and the line is drawn in a bottom-left twin of the readout HUD where there is a whole window's width. Three defects found driving it on the rig, none of which the build could see: * the HUD as a wxFrame took the WM's keyboard focus every time it was raised, and the canvas then received NO key events -- every sketch shortcut silently dead. Caught with SNAPORCA_KEYTRACE: shift+S logged a line, the following R logged nothing. It is a wxPopupWindow now, which cannot be focused. SetFocus() on the canvas does not fix it: focus was on another toplevel. * zero vertical padding fits the popup tighter than the font's line box and clips the glyphs; 6 (what the readout uses) reads as a two-line box. 3 is right. * "has a caller chosen a colour?" compared the label's foreground against its PARENT's, which differ by default, so every line counted as chosen and the neutral text came out the panel's dark grey -- invisible on a dark chip. Compare against the colour the label was created with, captured before any caller writes. Verified on both rigs against fresh binaries: sketch -> extrude -> click face -> click again -> whole body tinted, offer opens with the body rows live and Create / Add material correctly greyed. Keyboard drives the whole sequence. Filed and NOT fixed here: snaporca-od0 -- a bare-plate click does not deselect the solid, so "click away, click back" escalates. Pre-existing; clearing there would also drop the face the Thicken/Shell/Draft cards hold, which needs its own pass. Refs: snaporca-gem, snaporca-8cc, snaporca-od0 --- src/slic3r/GUI/DesignCanvas.cpp | 59 +++++++++++++++++++++++++++++ src/slic3r/GUI/DesignCanvas.hpp | 15 ++++++++ src/slic3r/GUI/DesignPanel.cpp | 40 +++++++++++++++---- src/slic3r/GUI/DesignPanel.hpp | 4 ++ src/slic3r/GUI/DesignSketchTool.cpp | 32 +++++++++++++--- 5 files changed, 137 insertions(+), 13 deletions(-) diff --git a/src/slic3r/GUI/DesignCanvas.cpp b/src/slic3r/GUI/DesignCanvas.cpp index 31f036f2bd..1a36467363 100644 --- a/src/slic3r/GUI/DesignCanvas.cpp +++ b/src/slic3r/GUI/DesignCanvas.cpp @@ -132,6 +132,36 @@ DesignCanvas::DesignCanvas(wxWindow* parent) } m_sketch_tool.on_readout = [this](const std::string& s) { set_readout(s); }; + // Bottom-LEFT twin, carrying the status line. Top-level for the same reason as the readout + // (a child widget is hidden by the GL surface) but a wxPopupWindow rather than a wxFrame, + // because a popup cannot take keyboard focus. The readout gets away with a frame only + // because it appears mid-gesture and the next input is the mouse; this one is up + // permanently and is re-raised on every status change. As a frame it took the WM's focus + // each time and the canvas stopped receiving keys at all — every sketch shortcut silently + // dead, which reads as a broken tool. Do not "simplify" it back to a wxFrame. + // Its colour is set per message — the panel decides whether a line is neutral or an error. + { + wxWindow* top = wxGetTopLevelParent(m_canvas_widget); + m_status_hud = new wxPopupWindow(top, wxBORDER_NONE); + m_status_hud->SetBackgroundColour(wxColour(28, 30, 34)); + m_status_hud_label = new wxStaticText(m_status_hud, wxID_ANY, wxEmptyString); + auto* ss = new wxBoxSizer(wxHORIZONTAL); + // The line never wraps — there is a whole window's width down here — so the chip is + // ONE LINE tall. Spacers rather than a wxALL border because the two axes want + // different numbers: roomy at the sides so it reads as a label, and just enough top + // and bottom to clear the descenders. Zero vertical clips the glyphs; 6 (what the + // readout chip uses) makes it look like a two-line box. + ss->AddSpacer(10); + ss->Add(m_status_hud_label, 0, wxTOP | wxBOTTOM, 3); + ss->AddSpacer(10); + m_status_hud->SetSizerAndFit(ss); + m_status_hud->Hide(); + } + // A floating frame does not follow its parent, so the anchor has to be recomputed whenever + // the canvas changes size. The readout HUD gets away without this because it is transient; + // the status line is on screen almost permanently and would visibly detach. + m_canvas_widget->Bind(wxEVT_SIZE, [this](wxSizeEvent& e) { place_status_hud(); e.Skip(); }); + refresh_bed(); m_canvas->bind_event_handlers(); @@ -924,6 +954,35 @@ void DesignCanvas::set_readout(const std::string& text) m_hud->Raise(); } +void DesignCanvas::set_status_text(const wxString& text, const wxColour& colour) +{ + if (!m_status_hud || !m_status_hud_label || !m_canvas_widget) return; + if (text == m_status_hud_last && colour == m_status_hud_colour) return; + m_status_hud_last = text; + m_status_hud_colour = colour; + if (text.IsEmpty()) { m_status_hud->Hide(); return; } + m_status_hud_label->SetForegroundColour(colour); + m_status_hud_label->SetLabel(text); + m_status_hud->Fit(); + place_status_hud(); +} + +void DesignCanvas::place_status_hud() +{ + if (!m_status_hud || !m_canvas_widget || m_status_hud_last.IsEmpty()) return; + const wxSize cs = m_canvas_widget->GetClientSize(); + const wxSize hs = m_status_hud->GetSize(); + // Clear of the view cube and the two round view buttons, which own the bottom-left corner. + const int kLeftInset = m_canvas_widget->FromDIP(190); + const wxPoint bl = m_canvas_widget->ClientToScreen( + wxPoint(kLeftInset, cs.GetHeight() - hs.GetHeight() - 12)); + // No Raise() and no focus juggling: a popup neither takes focus nor falls behind. This was + // caught with SNAPORCA_KEYTRACE — shift+S logged a line, the following R logged nothing, and + // the only thing between them was the first status update showing this window. + if (!m_status_hud->IsShown()) m_status_hud->Show(); // Show before Move (GTK ignores pre-map Move) + m_status_hud->Move(bl); +} + void DesignCanvas::set_body_highlight(bool on) { if (m_body_selected == on) return; diff --git a/src/slic3r/GUI/DesignCanvas.hpp b/src/slic3r/GUI/DesignCanvas.hpp index d09bbbc3f6..4462de096b 100644 --- a/src/slic3r/GUI/DesignCanvas.hpp +++ b/src/slic3r/GUI/DesignCanvas.hpp @@ -2,6 +2,7 @@ #define slic3r_DesignCanvas_hpp_ #include +#include #include #include @@ -181,6 +182,10 @@ public: void set_datum_planes(std::vector planes, std::vector sizes = {}); // draw datum/reference planes (u/v extents) void set_body_highlight(bool on); // tint the solid when its feature is tree-selected + // The status line, shown along the BASE OF THE VIEWPORT rather than in the side panel: + // the panel clips it at ~73 characters with no warning (snaporca-8cc), the viewport's + // bottom margin has the whole window width to spare. Empty text hides it. + void set_status_text(const wxString& text, const wxColour& colour); void set_operand_bodies(int target_body, int tool_body); // -1,-1 clears void set_body_translucent(bool on); // render the solid see-through (fillet/chamfer preview) void set_body_hidden(bool on); // preview-only: hide base bodies, show only the result ghost @@ -300,6 +305,16 @@ private: wxStaticText* m_hud_label{nullptr}; std::string m_hud_last; void set_readout(const std::string& text); + + // Bottom-LEFT viewport HUD: the selection / tool status line, written by DesignPanel. + // A wxPopupWindow, NOT the wxFrame the readout HUD uses: a frame accepts keyboard focus, + // and this one is on screen permanently and re-raised on every status change, so it stole + // the keyboard from the canvas and killed every sketch shortcut in the tab. + wxPopupWindow* m_status_hud{nullptr}; + wxStaticText* m_status_hud_label{nullptr}; + wxString m_status_hud_last; + wxColour m_status_hud_colour; + void place_status_hud(); // re-anchors to the canvas corner (also on resize) std::function m_on_sketch_commit; std::function&, const std::vector&, diff --git a/src/slic3r/GUI/DesignPanel.cpp b/src/slic3r/GUI/DesignPanel.cpp index 75b9352624..b64c1e2b38 100644 --- a/src/slic3r/GUI/DesignPanel.cpp +++ b/src/slic3r/GUI/DesignPanel.cpp @@ -3056,6 +3056,8 @@ DesignPanel::DesignPanel(wxWindow* parent) const int cm = FromDIP(SidebarProps::ContentMargin()); m_status = new wxStaticText(m_form, wxID_ANY, ""); + m_status->Hide(); // storage only — the line is drawn over the viewport, see set_status() + m_status_default_fg = m_status->GetForegroundColour(); // capture BEFORE any caller writes root->Add(m_status, 0, wxLEFT | wxRIGHT | wxBOTTOM, 12); // DoF / constraint-state readout (P3). Dedicated line so it never clobbers the @@ -3413,10 +3415,21 @@ DesignPanel::DesignPanel(wxWindow* parent) m_status->SetForegroundColour(wxNullColour); const int nb = int(m_doc.bodies.size()); const wxString bodytag = (nb > 1) ? wxString::Format(_L("Body %d "), body + 1) : wxString(); - set_status(level == 4 ? bodytag + _L("vertex selected") - : level == 1 ? bodytag + _L("selected (whole body)") - : level == 2 ? bodytag + wxString::Format(_L("face %d selected — right-click to push/pull it"), face) - : level == 3 ? bodytag + wxString::Format(_L("edge %d selected — Fillet/Chamfer to dress it"), edge) + // Each sub-element line ends by naming the NEXT click (snaporca-gem). Escalation to the + // whole body is a gesture nothing on screen would otherwise reveal, and the status line + // is the only surface that can teach it at the moment it applies. It REPLACES the old + // per-level verb hints ("right-click to push/pull it", "Fillet/Chamfer to dress it") + // rather than joining them: the line is clipped at the panel edge past ~55 characters + // (set_status's Wrap() does not take effect — snaporca-8cc), and those verbs are shown + // with their icons in the offer anyway, while this gesture is shown nowhere else. + // Both clauses fit now that the line is drawn over the viewport instead of squeezed + // into the panel. Say "what applies to it", never "verbs" — that is this codebase's + // word for a tool-offer entry, not a word the drawing office uses, and the plane and + // bodies-list lines already say it the right way. + set_status(level == 4 ? bodytag + _L("vertex selected — click again for the whole body") + : level == 1 ? bodytag + _L("selected (whole body) — right-click for what applies to it") + : level == 2 ? bodytag + wxString::Format(_L("face %d selected — right-click to push/pull it, or click again for the whole body"), face) + : level == 3 ? bodytag + wxString::Format(_L("edge %d selected — Fillet/Chamfer to dress it, or click again for the whole body"), edge) : _L("Nothing selected")); m_status->Refresh(); }); @@ -5285,9 +5298,22 @@ void DesignPanel::set_status(const wxString& text) { if (m_status == nullptr) return; m_status->SetLabel(text); // the ONE place that may call SetLabel directly - const int w = m_status->GetParent() ? m_status->GetParent()->GetClientSize().x - 24 : 420; - m_status->Wrap(w > 120 ? w : 420); - m_status->Refresh(); + // m_status is HIDDEN and kept only as the owner of the text and its colour — every caller + // sets the colour on it just before calling here, so this stays the one place that knows + // both. What the user reads is drawn along the BASE OF THE VIEWPORT: in the panel the line + // was clipped at ~73 characters with no warning and no wrap (Wrap() never took effect — + // snaporca-8cc), which silently length-limited every hint in the tab. The viewport's bottom + // margin has the whole window width, so a sentence can be a sentence. + if (m_viewport != nullptr) { + // wxNullColour means "no opinion", and the dark default text colour is nearly invisible + // on the dark HUD; only a colour a caller actually chose (the error red, the plane-pick + // green) is carried over. Compared against the colour the label was CREATED with — + // comparing against the parent's foreground instead reported "chosen" for every line, + // and the neutral text came out the panel's grey. + const wxColour fg = m_status->GetForegroundColour(); + m_viewport->set_status_text(text, fg != m_status_default_fg ? fg + : wxColour(0xDD, 0xE1, 0xE6)); + } } wxMenuItem* DesignPanel::append_offer_item(wxMenu* menu, int id, const wxString& text, diff --git a/src/slic3r/GUI/DesignPanel.hpp b/src/slic3r/GUI/DesignPanel.hpp index 34b38f284e..37535b42c6 100644 --- a/src/slic3r/GUI/DesignPanel.hpp +++ b/src/slic3r/GUI/DesignPanel.hpp @@ -779,6 +779,10 @@ private: static int tree_icon_for(CadFeatureType t); wxStaticText* m_status{nullptr}; + // m_status's foreground as created, captured before any caller touches it. Callers signal + // "no opinion" by setting wxNullColour, which restores exactly this — so it is the only + // reliable way to tell a chosen colour (the error red) from the default. See set_status(). + wxColour m_status_default_fg; wxStaticText* m_dof_status{nullptr}; // DoF / constraint-state readout (P3) int m_feature_counter{0}; diff --git a/src/slic3r/GUI/DesignSketchTool.cpp b/src/slic3r/GUI/DesignSketchTool.cpp index 343f0f8fbb..637a36e26e 100644 --- a/src/slic3r/GUI/DesignSketchTool.cpp +++ b/src/slic3r/GUI/DesignSketchTool.cpp @@ -3007,17 +3007,17 @@ bool DesignSketchTool::handle_solid_click(GLCanvas3D& canvas, const wxMouseEvent return std::hypot(wx - t * vx, wy - t * vy); }; + // What was selected BEFORE this pick — the escalation below is the only thing that reads + // it, and everything from here on overwrites it. + const SolidSel prev_kind = m_solid_sel; + const int prev_body = m_sel_body, prev_face = m_sel_face, prev_edge = m_sel_edge; + const Vec3d prev_vtx = m_sel_vertex_pt; + m_sel_body = best_body; m_sel_face = best_face; m_sel_edge = -1; m_sel_edge_pts.clear(); - // WHOLE-BODY selection is deliberately NOT bound here. Double-click is already zoom-to-fit - // (see the LeftDClick branch at the top of on_mouse) and this pick runs on LeftUp, where - // LeftDClick() is never true — a double-click branch here would be dead code that reads as - // a working feature. The body gesture is the rubber band, which is its own piece of work; - // until it lands, bodies are selected from the Bodies list. Written down rather than - // half-done, because a selection model with a silent hole in it is how we got the cycle. { const TopoDS_Shape& bshape = (*m_solid_bodies)[m_sel_body].shape; const TopoDS_Face face = GeometryEngine::face_by_index(bshape, m_sel_face); @@ -3059,6 +3059,26 @@ bool DesignSketchTool::handle_solid_click(GLCanvas3D& canvas, const wxMouseEvent m_solid_sel = SolidSel::Face; } } + // CLICK AGAIN ON THE SAME THING -> THE WHOLE BODY (snaporca-gem). Pointing at a face and + // pointing at its body are different intents, and until now only the rubber band could + // express the second one — so the status line said "face 0 selected" while the user + // believed they had taken the body, and every body verb had to opt into the face kinds to + // stay reachable. One more click on the SAME sub-element escalates. + // + // This is not the pick cycle that was removed (bc2b741ce9). That one was silent and three + // deep, so no click had a predictable meaning. Here the escalation is announced by the + // status line BEFORE you make the click, and a further click just takes the face under the + // cursor again — the ordinary meaning of clicking a face, which needs no teaching. + // + // Double-click is safe: wx sends Down/Up/DClick/Up, and only the first Up carries a + // pending press, so a fast double-click zooms to fit and picks ONCE. Escalation needs two + // separate clicks, the same "click, pause, click" distinction a file manager uses. + if (m_solid_sel == prev_kind && m_sel_body == prev_body && m_sel_face == prev_face + && m_sel_edge == prev_edge + && (m_solid_sel != SolidSel::Vertex || (m_sel_vertex_pt - prev_vtx).norm() < 1e-9)) { + select_body(m_sel_body); // clears face/edge/vertex, tints the whole body + dp_pick_trace("re-pick -> escalated to whole body %d", m_sel_body); + } dp_pick_trace("pick -> sel=%d body=%d face=%d edge=%d", int(m_solid_sel), m_sel_body, m_sel_face, m_sel_edge); if (on_solid_selection_changed)