From 9c5f4ebe479f73bdfa7addb294d413feb510d735 Mon Sep 17 00:00:00 2001 From: Clifford Garwood Date: Fri, 2 Oct 2026 22:55:53 -0400 Subject: [PATCH] Stop duplicate mode names from trapping a plate's mode cycle The editor only replaced an empty name, so a row could be given a name another row already had. A plate stores its mode by name and find_imex_mode() takes the first row with it, so the second row was unreachable, and the plate's mode list repeated the name: left-click stuck on it, or looped without getting back to Primary. An edited name that another row already carries, or the reserved Primary name in any case, is now replaced when the edit is committed: "copy" becomes "copy 2". The edited row yields, so plates keep resolving to the row they meant, and tabbing through a field without changing it checks nothing. Resetting a row to a saved name that another row has since taken does the same. The plate's mode list comes from imex_plate_mode_choices(), which lists each name once, so a profile that already has duplicates still cycles. Co-Authored-By: Claude Opus 5.5 --- src/libslic3r/IMEXHelpers.cpp | 9 +++++ src/libslic3r/IMEXHelpers.hpp | 9 +++-- src/slic3r/GUI/IMEXModesCtrl.cpp | 50 +++++++++++++++++++++++---- src/slic3r/GUI/IMEXModesCtrl.hpp | 16 ++++++--- src/slic3r/GUI/Plater.cpp | 9 ++--- tests/libslic3r/test_imex_helpers.cpp | 8 +++++ 6 files changed, 82 insertions(+), 19 deletions(-) diff --git a/src/libslic3r/IMEXHelpers.cpp b/src/libslic3r/IMEXHelpers.cpp index 57162638d4..ec3319fb04 100644 --- a/src/libslic3r/IMEXHelpers.cpp +++ b/src/libslic3r/IMEXHelpers.cpp @@ -651,6 +651,15 @@ std::vector imex_mode_table(const ConfigBase& cfg) return table; } +std::vector imex_plate_mode_choices(const ConfigBase& cfg) +{ + std::vector choices{ kImexPrimaryMode }; + for (const ImexMode& m : imex_mode_table(cfg)) + if (!m.name.empty() && std::find(choices.begin(), choices.end(), m.name) == choices.end()) + choices.push_back(m.name); + return choices; +} + bool imex_hull_violates_zones(const std::vector& zones, const Polygon& hull) { if (hull.points.empty()) diff --git a/src/libslic3r/IMEXHelpers.hpp b/src/libslic3r/IMEXHelpers.hpp index eb54b6ee0f..96bf1e211c 100644 --- a/src/libslic3r/IMEXHelpers.hpp +++ b/src/libslic3r/IMEXHelpers.hpp @@ -40,8 +40,8 @@ inline constexpr const char* kImexPrimaryMode = "primary"; // Resolution rule. Applied by find_imex_mode() / imex_mode_table() and NOWHERE else; // every consumer in the tree goes through one of the two: // * `imex_mode_names` is the roster. A mode exists iff a row of that array carries its -// name, and the FIRST such row wins. (The modes editor uniquifies names on entry, so -// duplicates only reach here from a hand-edited profile; first-match is what a +// name, and the FIRST such row wins. (The modes editor makes a name unique when it is +// committed, so duplicates reach here from a hand-edited profile; first-match is what a // std::find over the names array already did, and what the editor's own row order // means.) // * A sibling array too short to reach that row yields an EMPTY string for that field @@ -87,6 +87,11 @@ ImexMode find_imex_mode(const ConfigBase& cfg, const std::string& name); // wants every mode (the modes editor, the plate's mode menu) rather than one by name. std::vector imex_mode_table(const ConfigBase& cfg); +// The modes a plate can be set to, in the order its mode button cycles them: kImexPrimaryMode, +// then each name in the table once. A repeated name is listed at its first row only, the one +// find_imex_mode() resolves it to, and empty names and rows named kImexPrimaryMode are left out. +std::vector imex_plate_mode_choices(const ConfigBase& cfg); + // ============================================================================= // PHYSICAL vs LOGICAL extruder indices — read this before adding a new IMEX call site // ============================================================================= diff --git a/src/slic3r/GUI/IMEXModesCtrl.cpp b/src/slic3r/GUI/IMEXModesCtrl.cpp index 47c103d03c..a0cfd6e91d 100644 --- a/src/slic3r/GUI/IMEXModesCtrl.cpp +++ b/src/slic3r/GUI/IMEXModesCtrl.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include "slic3r/GUI/EditGCodeDialog.hpp" @@ -262,7 +263,7 @@ std::map IMEXModesCtrl::roles_for_mode(const std::string& active_ return roles; } -std::string IMEXModesCtrl::unique_mode_name(const std::vector& also_taken) const { +std::string IMEXModesCtrl::unique_mode_name(const std::vector& also_taken, const std::string& base) const { auto is_taken = [&](const std::string& cand) { if (std::find(also_taken.begin(), also_taken.end(), cand) != also_taken.end()) return true; @@ -272,12 +273,35 @@ std::string IMEXModesCtrl::unique_mode_name(const std::vector& also return false; }; for (int n = 2; ; ++n) { - std::string cand = "Mode " + std::to_string(n); + std::string cand = base + " " + std::to_string(n); if (!is_taken(cand)) return cand; } } +bool IMEXModesCtrl::fix_row_name(Row& r) { + if (r.is_primary || !r.name) + return false; + const std::string name = into_u8(r.name->GetTextCtrl()->GetValue()); + // Case-insensitive: a row named "Primary" works, but its menu entry reads the same as the + // built-in Primary's. + const bool reserved = boost::iequals(name, kImexPrimaryMode); + bool taken = false; + for (const Row& other : m_rows) + if (&other != &r && !other.is_primary && other.name && into_u8(other.name->GetTextCtrl()->GetValue()) == name) + taken = true; + if (!name.empty() && !reserved && !taken) + return false; + std::string base = name; + // "copy 2" taken becomes "copy 3", not "copy 2 2". + if (const size_t sp = base.find_last_of(' '); sp != std::string::npos && sp > 0 && sp + 1 < base.size() && + base.find_first_not_of("0123456789", sp + 1) == std::string::npos) + base.erase(sp); + // ChangeValue(), not SetValue(): no nested wxEVT_TEXT. + r.name->GetTextCtrl()->ChangeValue(from_u8(name.empty() || reserved ? unique_mode_name({}) : unique_mode_name({}, base))); + return true; +} + IMEXModesCtrl::RoleStyle IMEXModesCtrl::role_style(std::optional role) { // Gray / "Inactive" is the no-role answer; every role gets an explicit case so a new // one is a compile-time -Wswitch prompt rather than a tile that silently renders gray. @@ -501,15 +525,24 @@ void IMEXModesCtrl::add_row(const std::string& name, // Restore a name rather than let the row reach get_mode_data() unnamed. // Row is located by panel pointer (stable across add/remove) so a // kill-focus delivered while the rows are being torn down is a no-op. + r.name->GetTextCtrl()->Bind(wxEVT_SET_FOCUS, [this, panel = r.panel](wxFocusEvent& e) { + e.Skip(); + for (auto& row_ref : m_rows) + if (row_ref.panel == panel && row_ref.name) + row_ref.focus_name = into_u8(row_ref.name->GetTextCtrl()->GetValue()); + }); r.name->GetTextCtrl()->Bind(wxEVT_KILL_FOCUS, [this, panel = r.panel](wxFocusEvent& e) { e.Skip(); if (m_clearing_rows) return; // focus-out emitted while the rows are being deleted for (auto& row_ref : m_rows) { if (row_ref.panel != panel) continue; - if (!row_ref.name || !row_ref.name->GetTextCtrl()->GetValue().empty()) return; - // ChangeValue(), not SetValue(): no nested wxEVT_TEXT. - row_ref.name->GetTextCtrl()->ChangeValue(from_u8(unique_mode_name({}))); - notify(); + // Only an edit is checked. Tabbing through a hand-edited profile's duplicate + // leaves it alone rather than renaming the row plates resolve to. + const wxString value = row_ref.name ? row_ref.name->GetTextCtrl()->GetValue() : wxString(); + if (!value.empty() && into_u8(value) == row_ref.focus_name) + return; + if (fix_row_name(row_ref)) + notify(); return; } }); @@ -806,8 +839,11 @@ void IMEXModesCtrl::reset_row_to_parent(wxPanel* panel) { Row& r = m_rows[i]; if (r.panel != panel) continue; if (!p_names || i >= p_names->values.size()) return; - if (!r.is_primary && r.name) + if (!r.is_primary && r.name) { r.name->GetTextCtrl()->ChangeValue(from_u8(p_names->values[i])); + // The saved name may since have been given to another row, which keeps it. + fix_row_name(r); + } if (p_gcodes && i < p_gcodes->values.size()) r.gcode->ChangeValue(from_u8(p_gcodes->values[i])); if (p_tools && i < p_tools->values.size()) { diff --git a/src/slic3r/GUI/IMEXModesCtrl.hpp b/src/slic3r/GUI/IMEXModesCtrl.hpp index 354891f26a..74589dd6ae 100644 --- a/src/slic3r/GUI/IMEXModesCtrl.hpp +++ b/src/slic3r/GUI/IMEXModesCtrl.hpp @@ -121,14 +121,14 @@ private: // outside the currently visible rows × cols without losing data on save. static std::map roles_for_mode(const std::string& active_tools); - // "Mode N" for the lowest N >= 2 not already used by a row or by `also_taken` - // (N == 1 is conceptually the fixed Primary row). Deterministic for a given set - // of rows, so get_mode_data() is stable across calls and matches_config() stays + // `base` + " N" for the lowest N >= 2 not already used by a row or by `also_taken` + // (N == 1 is the fixed Primary row, or the name being made unique). Deterministic for a + // given set of rows, so get_mode_data() is stable across calls and matches_config() stays // honest. Intentionally NOT translated: mode names are identifiers — objects // store one in `imex_parallel_mode` and GCode.cpp matches it against // `imex_mode_names` by string — so a locale-dependent name would break a project // opened under a different language. - std::string unique_mode_name(const std::vector& also_taken) const; + std::string unique_mode_name(const std::vector& also_taken, const std::string& base = "Mode") const; // Everything the editor draws for one role, in ONE place: a new role needs a color and // a legend name here and nowhere else in this file. nullopt is Inactive (gray). @@ -172,6 +172,9 @@ private: // the plates a delete strands. Rebuilding the rows (load_from_config / set_grid_size) // re-seeds it, which is correct: config and rows agree again at that point. std::string orig_name; + // The name when the field last took focus, so a commit that leaves it unchanged is not + // treated as an edit. + std::string focus_name; bool is_primary {false}; ScalableButton* reset_btn {nullptr}; // nullptr when row has no parent counterpart bool reset_dirty_cached {false}; @@ -209,6 +212,11 @@ private: std::string active_tools_string(const Row& r) const; + // Replaces an edited row's name when it is empty, the reserved Primary name in any case, or + // one another row already carries: a plate stores the name and find_imex_mode() takes its + // first row, so the edited row yields. Returns whether it changed the name; the caller notifies. + bool fix_row_name(Row& r); + void notify(); // Update each row's reset bitmap to reflect current dirty state. Cached so we diff --git a/src/slic3r/GUI/Plater.cpp b/src/slic3r/GUI/Plater.cpp index 44d277c1ab..702f4b2c7c 100644 --- a/src/slic3r/GUI/Plater.cpp +++ b/src/slic3r/GUI/Plater.cpp @@ -22909,12 +22909,9 @@ int Plater::select_plate_by_hover_id(int hover_id, bool right_click, bool isModi ret = select_plate(plate_index); if (!ret) { PartPlate* curr_plate = p->partplate_list.get_curr_plate(); - // Build ordered mode list: kImexPrimaryMode first, then all named modes. - std::vector modes; - modes.push_back(kImexPrimaryMode); - const DynamicPrintConfig& printer_cfg = wxGetApp().preset_bundle->printers.get_edited_preset().config; - for (const ImexMode& m : imex_mode_table(printer_cfg)) - if (!m.name.empty() && m.name != kImexPrimaryMode) modes.push_back(m.name); + // Ordered mode list: kImexPrimaryMode first, then each named mode once. + const std::vector modes = + imex_plate_mode_choices(wxGetApp().preset_bundle->printers.get_edited_preset().config); if (right_click) { // Show a popup menu with all modes. diff --git a/tests/libslic3r/test_imex_helpers.cpp b/tests/libslic3r/test_imex_helpers.cpp index e27630a78a..cdc7fabaca 100644 --- a/tests/libslic3r/test_imex_helpers.cpp +++ b/tests/libslic3r/test_imex_helpers.cpp @@ -1330,6 +1330,14 @@ TEST_CASE("imex_mode_table - every row agrees with find_imex_mode on that name", } } +TEST_CASE("A plate offers Primary then each mode name once in table order", "[IMEX]") { + // A profile can carry the same name twice; a plate stores a mode by name, so only the + // first row of a name is reachable, and listing it twice traps the plate's mode cycle. + const DynamicPrintConfig cfg = mode_cfg({ "copy", "mirror", "copy", "", "primary", "iq-copy" }, {}, {}); + REQUIRE(imex_plate_mode_choices(cfg) == std::vector{ kImexPrimaryMode, "copy", "mirror", "iq-copy" }); + REQUIRE(imex_plate_mode_choices(mode_cfg({}, {}, {})) == std::vector{ kImexPrimaryMode }); +} + // --------------------------------------------------------------------------- // imex_resolve_routing — the one derivation shared by the hard block and the warning // ---------------------------------------------------------------------------