Fix regressions in the AMS filament favorites dropdown

src/slic3r/GUI/AMSMaterialsSetting.cpp

@@ -16,6 +16,7 @@
 #include <wx/checklst.h>
 #include "slic3r/GUI/GUI_Utils.hpp"
 #include "slic3r/GUI/Widgets/Button.hpp"
+#include "slic3r/GUI/Widgets/DropDown.hpp"
 #include "slic3r/GUI/Widgets/Label.hpp"
 #include <utility>
 #include "slic3r/GUI/wxExtensions.hpp"
@@ -175,6 +176,24 @@ void AMSMaterialsSetting::create()
     m_comboBox_filament->Connect(wxEVT_COMMAND_COMBOBOX_SELECTED, wxCommandEventHandler(AMSMaterialsSetting::on_select_filament), NULL, this);
     m_comboBox_filament->Bind(EVT_DROPDOWN_ITEM_ACTION, &AMSMaterialsSetting::on_toggle_favorite_filament, this);
 
+    // Favorites toggled in the open list are regrouped just before it opens again, so rows never move
+    // under the cursor. These handlers run ahead of ComboBox's own, which then measure and show the
+    // rebuilt rows; wxEVT_COMBOBOX_DROPDOWN would be too late, as ComboBox sends it after showing the list.
+    auto regroup_favorites = [this]() {
+        if (!m_favorites_regroup || m_comboBox_filament->is_drop_down())
+            return;
+        const FilamentInfos* info        = selected_filament_info();
+        const std::string    filament_id = info ? info->filament_id : std::string();
+        rebuild_filament_dropdown_items(filament_id);
+    };
+    m_comboBox_filament->Bind(wxEVT_LEFT_DOWN, [regroup_favorites](wxMouseEvent& e) { regroup_favorites(); e.Skip(); });
+    m_comboBox_filament->Bind(wxEVT_LEFT_DCLICK, [regroup_favorites](wxMouseEvent& e) { regroup_favorites(); e.Skip(); });
+    m_comboBox_filament->Bind(wxEVT_KEY_DOWN, [regroup_favorites](wxKeyEvent& e) {
+        if (e.GetKeyCode() == WXK_RETURN || e.GetKeyCode() == WXK_SPACE) // the keys that open the list
+            regroup_favorites();
+        e.Skip();
+    });
+
     m_comboBox_cali_result->Connect(wxEVT_COMMAND_COMBOBOX_SELECTED, wxCommandEventHandler(AMSMaterialsSetting::on_select_cali_result), NULL, this);
 }
 
@@ -193,6 +212,8 @@ void AMSMaterialsSetting::create_panel_normal(wxWindow* parent)
     m_sizer_filament->Add(0, 0, 0, wxEXPAND, 0);
 
     m_comboBox_filament = new ::ComboBox(parent, wxID_ANY, wxEmptyString, wxDefaultPosition, AMS_MATERIALS_SETTING_COMBOX_WIDTH, 0, nullptr, wxCB_READONLY);
+    m_star_filled = ScalableBitmap(this, "score_star_light", 16);
+    m_star_empty  = ScalableBitmap(this, "score_star_dark", 16);
 
     m_sizer_filament->Add(m_comboBox_filament, 1, wxALIGN_CENTER, 0);
 
@@ -988,12 +1009,10 @@ static const char* AMS_FAVORITE_FILAMENT_IDS_KEY = "ams_favorite_filament_ids";
 
 const AMSMaterialsSetting::FilamentInfos* AMSMaterialsSetting::selected_filament_info() const
 {
-    int selection = m_comboBox_filament->GetSelection();
-    if (selection >= 0 && selection < int(m_filament_list_items.size()) && m_filament_list_items[selection].is_filament)
-        return &m_filament_list_items[selection].filament;
-
-    auto iter = map_filament_items.find(m_comboBox_filament->GetValue().ToStdString());
-    return iter == map_filament_items.end() ? nullptr : &iter->second;
+    const int selection = m_comboBox_filament->GetSelection();
+    if (selection < 0 || selection >= int(m_filament_list_items.size()) || !m_filament_list_items[selection].is_filament)
+        return nullptr;
+    return &m_filament_list_items[selection].filament;
 }
 
 std::set<std::string> AMSMaterialsSetting::load_favorite_filament_ids() const
@@ -1019,10 +1038,6 @@ void AMSMaterialsSetting::rebuild_filament_dropdown_items(const std::string& sel
     std::vector<DropDown::Item> items;
     m_filament_list_items.clear();
 
-    const std::set<std::string> favorite_ids = load_favorite_filament_ids();
-    wxBitmap star_filled = ScalableBitmap(this, "score_star_light", 16).bmp();
-    wxBitmap star_empty  = ScalableBitmap(this, "score_star_dark", 16).bmp();
-
     auto append_header = [&items, this](const wxString& label) {
         DropDown::Item item;
         item.text = label;
@@ -1031,43 +1046,32 @@ void AMSMaterialsSetting::rebuild_filament_dropdown_items(const std::string& sel
         m_filament_list_items.push_back(FilamentListItem{});
     };
 
-    auto append_filament = [&items, this, &favorite_ids, &star_filled, &star_empty](const wxString& name) {
-        auto info_it = map_filament_items.find(name.ToStdString());
+    auto append_filament = [&items, this](const wxString& name) {
+        auto info_it = map_filament_items.find(name);
         if (info_it == map_filament_items.end())
             return;
 
-        const bool is_favorite = favorite_ids.find(info_it->second.filament_id) != favorite_ids.end();
         DropDown::Item item;
         item.text = name;
-        item.action_icon = is_favorite ? star_filled : star_empty;
-        item.style = 0;
         items.push_back(item);
-
-        FilamentListItem list_item;
-        list_item.filament = info_it->second;
-        list_item.is_filament = true;
-        m_filament_list_items.push_back(list_item);
+        m_filament_list_items.push_back(FilamentListItem{info_it->second, true});
     };
 
-    bool has_favorites = false;
+    // Favorites are listed again at the top under their own header. Without any, the list is the
+    // plain filament list.
+    const std::set<std::string> favorite_ids = load_favorite_filament_ids();
+    std::vector<wxString>       favorites;
     for (const wxString& name : m_sorted_filament_items) {
-        auto info_it = map_filament_items.find(name.ToStdString());
-        if (info_it != map_filament_items.end() && favorite_ids.find(info_it->second.filament_id) != favorite_ids.end()) {
-            has_favorites = true;
-            break;
-        }
+        auto info_it = map_filament_items.find(name);
+        if (info_it != map_filament_items.end() && favorite_ids.count(info_it->second.filament_id) > 0)
+            favorites.push_back(name);
     }
-
-    if (has_favorites) {
+    if (!favorites.empty()) {
         append_header(_L("Favorite filaments"));
-        for (const wxString& name : m_sorted_filament_items) {
-            auto info_it = map_filament_items.find(name.ToStdString());
-            if (info_it != map_filament_items.end() && favorite_ids.find(info_it->second.filament_id) != favorite_ids.end())
-                append_filament(name);
-        }
+        for (const wxString& name : favorites)
+            append_filament(name);
+        append_header(_L("All filaments"));
     }
-
-    append_header(_L("All filaments"));
     for (const wxString& name : m_sorted_filament_items)
         append_filament(name);
 
@@ -1081,8 +1085,25 @@ void AMSMaterialsSetting::rebuild_filament_dropdown_items(const std::string& sel
         }
     }
 
+    // Clear() first: SetItems() keeps the old selection, and SetSelection() ignores an index equal to it,
+    // which would leave the previous filament's name on the combo.
+    m_comboBox_filament->Clear();
     m_comboBox_filament->SetItems(items);
+    update_favorite_stars();
     m_comboBox_filament->SetSelection(selection_idx);
+    m_favorites_regroup = false;
+}
+
+void AMSMaterialsSetting::update_favorite_stars()
+{
+    const std::set<std::string> favorite_ids = load_favorite_filament_ids();
+    for (size_t i = 0; i < m_filament_list_items.size(); ++i) {
+        const FilamentListItem& row = m_filament_list_items[i];
+        if (row.is_filament) {
+            const bool favorite = favorite_ids.count(row.filament.filament_id) > 0;
+            m_comboBox_filament->SetItemActionBitmap(i, favorite ? m_star_filled.bmp() : m_star_empty.bmp());
+        }
+    }
 }
 
 void AMSMaterialsSetting::toggle_favorite_filament(const std::string& filament_id)
@@ -1106,12 +1127,9 @@ void AMSMaterialsSetting::on_toggle_favorite_filament(wxCommandEvent& evt)
     if (index < 0 || index >= int(m_filament_list_items.size()) || !m_filament_list_items[index].is_filament)
         return;
 
-    std::string selected_filament_id;
-    if (const FilamentInfos* info = selected_filament_info())
-        selected_filament_id = info->filament_id;
     toggle_favorite_filament(m_filament_list_items[index].filament.filament_id);
-    rebuild_filament_dropdown_items(selected_filament_id);
-    m_comboBox_filament->RefreshDropdownIfOpen();
+    update_favorite_stars();
+    m_favorites_regroup = true;
 }
 
 void AMSMaterialsSetting::Popup(wxString filament, wxString sn, wxString temp_min, wxString temp_max, wxString k, wxString n)
@@ -1127,16 +1145,12 @@ void AMSMaterialsSetting::Popup(wxString filament, wxString sn, wxString temp_mi
     m_input_k_val->GetTextCtrl()->SetValue(k);
     m_input_n_val->GetTextCtrl()->SetValue(n);
 
-    wxArrayString filament_items;
-    map_filament_items.clear();
-    m_sorted_filament_items.clear();
-    m_filament_list_items.clear();
     wxString bambu_filament_name;
-    wxString hint_filament_name; // the hint type to be selected
     std::unordered_map<wxString, wxString> query_filament_vendors;// some information for sort
     std::unordered_map<wxString, wxString> query_filament_types;  //
 
     std::set<std::string> filament_id_set;
+    m_sorted_filament_items.clear();
     // The alias keyed map has to start empty: it is a member, so a stale alias left by an earlier
     // popup (a different printer, a different nozzle) would resolve to that printer's filament_id.
     map_filament_items.clear();
@@ -1166,7 +1180,7 @@ void AMSMaterialsSetting::Popup(wxString filament, wxString sn, wxString temp_mi
             if (alias.empty())
                 continue;
 
-            filament_items.push_back(alias);
+            m_sorted_filament_items.push_back(alias);
             _collect_filament_info(alias, *filament_it, query_filament_vendors, query_filament_types);
 
             FilamentInfos filament_infos;
@@ -1175,7 +1189,6 @@ void AMSMaterialsSetting::Popup(wxString filament, wxString sn, wxString temp_mi
             map_filament_items[alias]  = filament_infos;
 
             if (filament_it->filament_id == ams_filament_id) {
-                hint_filament_name = from_u8(alias);
                 bambu_filament_name = from_u8(alias);
 
 
@@ -1346,20 +1359,10 @@ void AMSMaterialsSetting::Popup(wxString filament, wxString sn, wxString temp_mi
             return name_compare != 0 ? name_compare < 0 : left < right;
         };
 
-        std::sort(filament_items.begin(), filament_items.end(), _filament_sorter);
+        std::sort(m_sorted_filament_items.begin(), m_sorted_filament_items.end(), _filament_sorter);
     }
 
-    std::string selection_filament_id;
-    for (int i = 0; i < int(filament_items.size()); ++i) {
-        m_sorted_filament_items.push_back(filament_items[i]);
-        if (hint_filament_name == filament_items[i]) {
-            auto info_it = map_filament_items.find(filament_items[i].ToStdString());
-            if (info_it != map_filament_items.end())
-                selection_filament_id = info_it->second.filament_id;
-        }
-    }
-
-    rebuild_filament_dropdown_items(selection_filament_id);
+    rebuild_filament_dropdown_items(ams_filament_id);
     int selection_idx = m_comboBox_filament->GetSelection();
     post_select_event(selection_idx);
 
@@ -1645,6 +1648,11 @@ void AMSMaterialsSetting::on_dpi_changed(const wxRect &suggested_rect)
     m_button_reset->Rescale(); // ORCA
     m_button_confirm->Rescale(); // ORCA
     m_button_close->Rescale(); // ORCA
+    // An open list would keep the measurements of the old stars, so close it before replacing them.
+    m_comboBox_filament->CloseDropdown();
+    m_star_filled.msw_rescale();
+    m_star_empty.msw_rescale();
+    update_favorite_stars();
     this->Refresh();
 }
 

src/slic3r/GUI/AMSMaterialsSetting.hpp

@@ -166,6 +166,7 @@ protected:
     bool is_virtual_tray();
     void update_widgets();
     void rebuild_filament_dropdown_items(const std::string& selected_filament_id);
+    void update_favorite_stars();
     std::set<std::string> load_favorite_filament_ids() const;
     void save_favorite_filament_ids(const std::set<std::string>& favorite_ids);
     void toggle_favorite_filament(const std::string& filament_id);
@@ -222,9 +223,14 @@ protected:
     };
     const FilamentInfos* selected_filament_info() const;
 
-    std::map<std::string, FilamentInfos> map_filament_items;
+    // Keyed by the same wxString the dropdown shows, so a row's name never has to be converted back.
+    std::map<wxString, FilamentInfos>    map_filament_items;
     std::vector<wxString>                m_sorted_filament_items;
     std::vector<FilamentListItem>        m_filament_list_items;
+    ScalableBitmap                       m_star_filled;
+    ScalableBitmap                       m_star_empty;
+    // Favorites changed while the list was open; their rows are regrouped before it opens again.
+    bool                                 m_favorites_regroup{false};
 };
 
 wxDECLARE_EVENT(EVT_SELECTED_COLOR, wxCommandEvent);

src/slic3r/GUI/Widgets/ComboBox.cpp

@@ -302,25 +302,10 @@ void ComboBox::ForceDropdownOpen()
     }
 }
 
-void ComboBox::RefreshDropdownIfOpen()
+void ComboBox::CloseDropdown()
 {
-    if (!drop_down)
-        return;
-
-    drop.hover_item = -1;
-    drop.offset = wxPoint();
-    if (drop.subDropDown) {
-        drop.subDropDown->group.clear();
-        drop.subDropDown->hover_item = -1;
-        drop.subDropDown->offset = wxPoint();
-        if (drop.subDropDown->IsShown())
-            drop.subDropDown->Dismiss();
-    }
-
-    drop.need_sync = true;
-    drop.messureSize();
-    drop.autoPosition();
-    drop.paintNow();
+    if (drop.IsShown())
+        drop.DismissAndNotify();
 }
 
 unsigned int ComboBox::GetCount() const { return items.size(); }
@@ -452,16 +437,18 @@ void ComboBox::keyDown(wxKeyEvent& event)
         case WXK_UP:
         case WXK_DOWN:
         case WXK_LEFT:
-        case WXK_RIGHT:
-            if ((event.GetKeyCode() == WXK_UP || event.GetKeyCode() == WXK_LEFT) && GetSelection() > 0) {
-                SetSelection(GetSelection() - 1);
-            } else if ((event.GetKeyCode() == WXK_DOWN || event.GetKeyCode() == WXK_RIGHT) && GetSelection() + 1 < items.size()) {
-                SetSelection(GetSelection() + 1);
-            } else {
+        case WXK_RIGHT: {
+            // Step over disabled rows: the mouse cannot select them either.
+            const int step = (event.GetKeyCode() == WXK_UP || event.GetKeyCode() == WXK_LEFT) ? -1 : 1;
+            int       n    = GetSelection() + step;
+            while (n >= 0 && n < int(items.size()) && (items[n].style & DD_ITEM_STYLE_DISABLED))
+                n += step;
+            if (n < 0 || n >= int(items.size()))
                 break;
-            }
+            SetSelection(n);
             sendComboBoxEvent();
             break;
+        }
         case WXK_TAB:
             HandleAsNavigationKey(event);
             break;

src/slic3r/GUI/Widgets/ComboBox.hpp

@@ -101,7 +101,7 @@ public:
     void     DeleteOneItem(unsigned int pos) { DoDeleteOneItem(pos); }
 
     void ForceDropdownOpen();
-    void RefreshDropdownIfOpen();
+    void CloseDropdown();
 
 protected:
     virtual int  DoInsertItems(const wxArrayStringsAdapter &items,

src/slic3r/GUI/Widgets/DropDown.cpp

@@ -243,27 +243,17 @@ static wxSize GetBmpSize(wxBitmap & bmp)
 #endif
 }
 
-wxSize DropDown::actionIconSize() const
+// The x-range [first, second) of the column at the right of the rows that holds the items' action
+// icons, empty when no item has one. render() draws the icons in it and mouseReleased() hit-tests
+// clicks against it, so the drawn icon and its clickable area always match.
+std::pair<int, int> DropDown::actionColumn() const
 {
-    wxSize size;
-    std::set<wxString> groups;
-    for (const Item& item : items) {
-        if (group.IsEmpty()) {
-            if (!item.group_key.IsEmpty()) {
-                if (groups.find(item.group_key) != groups.end())
-                    continue;
-                groups.insert(item.group_key);
-            }
-        } else if (item.group_key != group) {
-            continue;
-        }
-
-        wxBitmap icon = item.action_icon;
-        wxSize icon_size = GetBmpSize(icon);
-        size.x = std::max(size.x, icon_size.x);
-        size.y = std::max(size.y, icon_size.y);
-    }
-    return size;
+    if (actionIconWidth <= 0)
+        return {0, 0};
+    int right = rowSize.x;
+    if (rowSize.y * count > GetSize().y) // position bar
+        right -= 6;
+    return {right - actionIconWidth - FromDIP(16), right};
 }
 
 static void _DrawSplitItem(const wxWindow* w, wxDC& dc, wxString split_text, wxPoint start_pt, int item_width, int item_height)
@@ -395,8 +385,9 @@ void DropDown::render(wxDC &dc)
         rcContent.width -= szBmp.x + 5;
     }
 
-    const wxSize action_icon_size = actionIconSize();
-    const int action_column_width = action_icon_size.x > 0 ? action_icon_size.x + FromDIP(16) : 0;
+    const auto action_column = actionColumn();
+    if (action_column.second > action_column.first)
+        rcContent.width = action_column.first - rcContent.x;
 
     std::set<wxString> groups;
     // draw texts & icons
@@ -436,7 +427,6 @@ void DropDown::render(wxDC &dc)
         }
         if (rcContent.y > size.y) break;
         wxPoint pt   = rcContent.GetLeftTop();
-        int rcContent_width = rcContent.width;
 
         if (item.style & DD_ITEM_STYLE_SPLIT_ITEM) {
             _DrawSplitItem(this, dc, item.text, pt, rowSize.GetWidth(), rowSize.GetHeight());
@@ -462,12 +452,9 @@ void DropDown::render(wxDC &dc)
         auto text = group.IsEmpty()
                         ? (item.group_key.IsEmpty() ? item.text : item.group_label)
                         : item.text;
-        if (action_column_width > 0)
-            rcContent.width -= action_column_width;
-
         if (item.action_icon.IsOk()) {
             auto action_size = GetBmpSize(item.action_icon);
-            wxPoint action_pt(rcContent.x + rcContent_width - action_size.x - FromDIP(8),
+            wxPoint action_pt(action_column.second - action_size.x - FromDIP(8),
                               rcContent.y + (rcContent.height - action_size.y) / 2);
             dc.DrawBitmap(item.action_icon, action_pt);
         }
@@ -498,7 +485,6 @@ void DropDown::render(wxDC &dc)
                 dc.DrawBitmap(arrow_bitmap.bmp(), pt);
             }
         }
-        rcContent.width = rcContent_width;
         rcContent.y += rowSize.y;
     }
 }
@@ -572,6 +558,7 @@ void DropDown::messureSize()
     if (!need_sync) return;
     textSize = wxSize();
     iconSize = wxSize();
+    actionIconWidth = 0;
     count = 0;
     wxClientDC dc(GetParent() ? GetParent() : this);
     dc.SetFont(GetFont());
@@ -609,9 +596,12 @@ void DropDown::messureSize()
             }
         }
         if (item.action_icon.IsOk())
-            size1.x += GetBmpSize(item.action_icon).x + FromDIP(16);
+            actionIconWidth = std::max(actionIconWidth, GetBmpSize(item.action_icon).x);
         if (size1.x > textSize.x) textSize = size1;
     }
+    // Every row leaves room for the action column, as render() and actionColumn() lay it out.
+    if (actionIconWidth > 0)
+        textSize.x += actionIconWidth + FromDIP(16);
     if (!align_icon) iconSize.x = 0;
     wxSize szContent = textSize;
     if (szContent.x < FromDIP(120) && !use_content_width)
@@ -766,12 +756,8 @@ void DropDown::mouseReleased(wxMouseEvent& event)
         if (hover_item >= 0 && (subDropDown == nullptr || subDropDown->group.empty())) { // not moved
             int index = hoverIndex();
             if (index >= 0 && items[index].action_icon.IsOk()) {
-                wxSize action_size = actionIconSize();
-                int action_right = GetSize().x;
-                if (rowSize.y * count > GetSize().y)
-                    action_right -= 6;
-                int action_left = action_right - action_size.x - FromDIP(16);
-                if (event.GetX() >= action_left && event.GetX() <= action_right) {
+                const auto action_column = actionColumn();
+                if (event.GetX() >= action_column.first && event.GetX() < action_column.second) {
                     sendDropDownActionEvent(index);
                     paintNow();
                     return;

src/slic3r/GUI/Widgets/DropDown.hpp

@@ -5,6 +5,7 @@
 #include <wx/event.h>
 #include <vector>
 #include <cstddef>
+#include <utility>
 #include <wx/gdicmn.h>
 #include "slic3r/GUI/Widgets/StateColor.hpp"
 #include <boost/date_time/posix_time/ptime.hpp>
@@ -67,6 +68,7 @@ private:
 
     wxSize textSize;
     wxSize iconSize;
+    int    actionIconWidth = 0;
     wxSize rowSize;
 
     StateHandler state_handler;
@@ -137,7 +139,7 @@ private:
     int hoverIndex();
 
     int selectedItem();
-    wxSize actionIconSize() const;
+    std::pair<int, int> actionColumn() const;
 
     friend class ComboBox;
     void messureSize();