From 763ebba6a22d83e627e2a7cb14c6c16d4eee4428 Mon Sep 17 00:00:00 2001 From: David Capello Date: Wed, 7 May 2025 23:34:05 -0300 Subject: [PATCH] Fix crashes resetting the default layout + remove some hacks With this patch we've also simplified some hacks handling the populateComboBox() call, deferring the deletion of widgets/items when we are inside an event generated by those items. --- src/app/ui/layout_selector.cpp | 61 +++++++++++++++++++++------------- src/app/ui/layout_selector.h | 4 +++ src/ui/combobox.h | 3 +- 3 files changed, 43 insertions(+), 25 deletions(-) diff --git a/src/app/ui/layout_selector.cpp b/src/app/ui/layout_selector.cpp index cad752b72..807985345 100644 --- a/src/app/ui/layout_selector.cpp +++ b/src/app/ui/layout_selector.cpp @@ -105,13 +105,12 @@ public: , m_selector(selector) , m_layoutId(layoutId) { - auto* hbox = new HBox; - hbox->setTransparent(true); - addChild(hbox); + m_hbox.setTransparent(true); + addChild(&m_hbox); auto* filler = new BoxFiller(); filler->setTransparent(true); - hbox->addChild(filler); + m_hbox.addChild(filler); if (option == USER_DEFINED || ((option == DEFAULT || option == MIRRORED_DEFAULT) && !layoutId.empty())) { @@ -128,13 +127,10 @@ public: ASSERT(!m_layoutId.empty()); // TODO: Custom icons for each one would be nice here. - m_actionButton = std::unique_ptr( - new IconButton(SkinTheme::instance()->parts.iconClose())); - m_actionButton->setSizeHint( - gfx::Size(m_actionButton->textHeight(), m_actionButton->textHeight())); - + m_actionButton = new IconButton(SkinTheme::instance()->parts.iconClose()); + const int th = m_actionButton->textHeight(); + m_actionButton->setSizeHint(gfx::Size(th, th)); m_actionButton->setTransparent(true); - m_actionButton->InitTheme.connect( [this] { m_actionButton->setBgColor(gfx::rgba(0, 0, 0, 0)); }); @@ -177,7 +173,7 @@ public: }); } - children()[0]->addChild(m_actionButton.get()); + m_hbox.addChild(m_actionButton); } std::string_view getLayoutId() const { return m_layoutId; } @@ -298,15 +294,20 @@ public: private: LayoutOption m_option; - LayoutSelector* m_selector; + LayoutSelector* m_selector = nullptr; std::string m_layoutId; - std::unique_ptr m_actionButton; + HBox m_hbox; + IconButton* m_actionButton = nullptr; obs::scoped_connection m_actionConn; }; void LayoutSelector::LayoutComboBox::onChange() { ComboBox::onChange(); + + if (m_lockChange) + return; + if (auto* item = dynamic_cast(getSelectedItem())) { item->selectImmediately(); m_selected = item; @@ -316,6 +317,10 @@ void LayoutSelector::LayoutComboBox::onChange() void LayoutSelector::LayoutComboBox::onCloseListBox() { ComboBox::onCloseListBox(); + + if (m_lockChange) + return; + if (m_selected) { m_selected->selectAfterClose(); m_selected = nullptr; @@ -363,12 +368,7 @@ void LayoutSelector::addLayout(const LayoutPtr& layout) { m_layouts.addLayout(layout); - // HACK: Because this function is called from inside a LayoutItem, clearing the combobox items - // will crash. - // TODO: Is there a better way to do this? - auto* msg = new CallbackMessage([this] { populateComboBox(); }); - msg->setRecipient(this); - manager()->enqueueMessage(msg); + populateComboBox(); } void LayoutSelector::removeLayout(const LayoutPtr& layout) @@ -376,10 +376,7 @@ void LayoutSelector::removeLayout(const LayoutPtr& layout) m_layouts.removeLayout(layout); m_layouts.saveUserLayouts(); - // TODO: See addLayout - auto* msg = new CallbackMessage([this] { populateComboBox(); }); - msg->setRecipient(this); - manager()->enqueueMessage(msg); + populateComboBox(); } void LayoutSelector::removeLayout(const std::string& layoutId) @@ -523,7 +520,21 @@ void LayoutSelector::setActiveLayoutId(const std::string& layoutId) void LayoutSelector::populateComboBox() { - m_comboBox.deleteAllItems(); + // Disable combobox onChange() event processing when we are + // re-creating the combobox items. This avoids calling + // LayoutSelector::LayoutItem::selectImmediately() which could + // delete docks that generate this same event, e.g. resizing a dock + // can generate a UserResizedDock which might call this + // populateComboBox() function. + m_comboBox.setLockChange(true); + + // Defer deletion of current items because we can be inside one of + // these item callbacks. + auto itemsCopy = m_comboBox.items(); + for (auto* item : itemsCopy) { + m_comboBox.removeItem(item); + item->deferDelete(); + } m_comboBox.addItem(new SeparatorInView(Strings::main_window_layout(), HORIZONTAL)); m_comboBox.addItem( @@ -558,6 +569,8 @@ void LayoutSelector::populateComboBox() m_comboBox.setSelectedItemIndex(2); m_comboBox.getEntryWidget()->deselectText(); + + m_comboBox.setLockChange(false); } LayoutSelector::LayoutItem* LayoutSelector::getItemByLayoutId(const std::string& id) diff --git a/src/app/ui/layout_selector.h b/src/app/ui/layout_selector.h index ba3eaf443..1d65a9472 100644 --- a/src/app/ui/layout_selector.h +++ b/src/app/ui/layout_selector.h @@ -37,10 +37,14 @@ class LayoutSelector : public ui::VBox, class LayoutItem; class LayoutComboBox : public ui::ComboBox { + public: + void setLockChange(bool state) { m_lockChange = state; } + private: void onChange() override; void onCloseListBox() override; LayoutItem* m_selected = nullptr; + bool m_lockChange = false; }; public: diff --git a/src/ui/combobox.h b/src/ui/combobox.h index f311eb653..22467cf68 100644 --- a/src/ui/combobox.h +++ b/src/ui/combobox.h @@ -1,5 +1,5 @@ // Aseprite UI Library -// Copyright (C) 2019-2023 Igara Studio S.A. +// Copyright (C) 2019-2025 Igara Studio S.A. // Copyright (C) 2001-2017 David Capello // // This file is released under the terms of the MIT license. @@ -39,6 +39,7 @@ public: Items::iterator begin() { return m_items.begin(); } Items::iterator end() { return m_items.end(); } bool empty() const { return m_items.empty(); } + const Items& items() { return m_items; } void setEditable(bool state); void setClickOpen(bool state);