Fix performance issues adding/deleting widgets (related to #3281)

src/app/commands/cmd_keyboard_shortcuts.cpp

@@ -607,8 +607,7 @@ private:
     if (m_headerItem.parent() == listbox)
       listbox->removeChild(&m_headerItem);
 
-    while (listbox->lastChild()) {
+    while (auto item = listbox->lastChild()) {
-      Widget* item = listbox->lastChild();
       listbox->removeChild(item);
       delete item;
     }

src/app/commands/cmd_options.cpp

@@ -1185,8 +1185,8 @@ private:
   }
 
   void reloadThemes() {
-    while (themeList()->firstChild())
+    while (auto child = themeList()->lastChild())
-      delete themeList()->lastChild();
+      delete child;
 
     loadThemes();
   }

src/app/commands/cmd_undo_history.cpp

@@ -1,5 +1,5 @@
 // Aseprite
-// Copyright (C) 2020  Igara Studio S.A.
+// Copyright (C) 2020-2022  Igara Studio S.A.
 // Copyright (C) 2015-2018  David Capello
 //
 // This program is distributed under the terms of
@@ -194,7 +194,7 @@ private:
 
   void clearList() {
     ui::Widget* child;
-    while ((child = actions()->firstChild()))
+    while ((child = actions()->lastChild()))
       delete child;
 
     actions()->layout();

src/app/commands/debugger.cpp

@@ -310,7 +310,7 @@ public:
   }
 
   void clear() {
-    while (auto item = firstChild()) {
+    while (auto item = lastChild()) {
       removeChild(item);
       item->deferDelete();
     }
@@ -730,7 +730,7 @@ private:
   }
 
   void clearLocals() {
-    while (auto item = locals()->firstChild()) {
+    while (auto item = locals()->lastChild()) {
       locals()->removeChild(item);
       item->deferDelete();
     }

src/app/ui/browser_view.cpp

@@ -273,8 +273,8 @@ private:
 
   void clear() {
     // Delete all children
-    while (firstChild())
+    while (auto child = lastChild())
-      delete firstChild();
+      delete child;
   }
 
   void processNode(cmark_node* root,

src/app/ui/news_listbox.cpp

@@ -204,8 +204,8 @@ void NewsListBox::reload()
   if (m_loader || m_timer.isRunning())
     return;
 
-  while (lastChild())
+  while (auto child = lastChild())
-    removeChild(lastChild());
+    removeChild(child);
 
   View* view = View::getView(this);
   if (view)

src/app/ui/recent_listbox.cpp

@@ -242,8 +242,7 @@ RecentListBox::RecentListBox()
 
 void RecentListBox::rebuildList()
 {
-  while (lastChild()) {
+  while (auto child = lastChild()) {
-    auto child = lastChild();
     removeChild(child);
     child->deferDelete();
   }

src/ui/combobox.cpp

@@ -1,5 +1,5 @@
 // Aseprite UI Library
-// Copyright (C) 2018-2020  Igara Studio S.A.
+// Copyright (C) 2018-2022  Igara Studio S.A.
 // Copyright (C) 2001-2017  David Capello
 //
 // This file is released under the terms of the MIT license.
@@ -202,8 +202,12 @@ void ComboBox::deleteItem(int itemIndex)
 
 void ComboBox::deleteAllItems()
 {
-  for (Widget* item : m_items)
+  // Delete all items back to front, in this way Widget::removeChild()
-    delete item;                // widget
+  // doesn't have to use linear search to update m_parentIndex of all
+  // other children.
+  auto end = m_items.rend();
+  for (auto it=m_items.rbegin(); it != end; ++it)
+    delete *it;                // widget
 
   m_items.clear();
   m_selected = -1;

src/ui/intern.cpp

@@ -1,5 +1,5 @@
 // Aseprite UI Library
-// Copyright (C) 2020  Igara Studio S.A.
+// Copyright (C) 2020-2022  Igara Studio S.A.
 // Copyright (C) 2001-2017  David Capello
 //
 // This file is released under the terms of the MIT license.
@@ -15,32 +15,31 @@
 #include "ui/widget.h"
 #include "ui/window.h"
 
-#include <list>
+#include <memory>
+#include <set>
 
 namespace ui {
 namespace details {
 
-static std::list<Widget*>* widgets;
+static std::unique_ptr<std::set<Widget*>> widgets;
 
 void initWidgets()
 {
   assert_ui_thread();
-
+  widgets = std::make_unique<std::set<Widget*>>();
-  widgets = new std::list<Widget*>;
 }
 
 void exitWidgets()
 {
   assert_ui_thread();
-
+  widgets.reset();
-  delete widgets;
 }
 
 void addWidget(Widget* widget)
 {
   assert_ui_thread();
 
-  widgets->push_back(widget);
+  widgets->insert(widget);
 }
 
 void removeWidget(Widget* widget)
@@ -49,11 +48,12 @@ void removeWidget(Widget* widget)
 
   ASSERT(!Manager::widgetAssociatedToManager(widget));
 
-  auto it = std::find(widgets->begin(), widgets->end(), widget);
+  widgets->erase(widget);
-  if (it != widgets->end())
-    widgets->erase(it);
 }
 
+// TODO we should be able to re-initialize all widgets without using
+//      this global "widgets" set, so we don't have to keep track of
+//      all widgets globally
 void reinitThemeForAllWidgets()
 {
   assert_ui_thread();

src/ui/listbox.cpp

@@ -1,5 +1,5 @@
 // Aseprite UI Library
-// Copyright (C) 2019-2020  Igara Studio S.A.
+// Copyright (C) 2019-2022  Igara Studio S.A.
 // Copyright (C) 2001-2018  David Capello
 //
 // This file is released under the terms of the MIT license.
@@ -70,25 +70,6 @@ int ListBox::getSelectedIndex()
   return -1;
 }
 
-int ListBox::getChildIndex(Widget* item)
-{
-  const WidgetsList& children = this->children();
-  auto it = std::find(children.begin(), children.end(), item);
-  if (it != children.end())
-    return it - children.begin();
-  else
-    return -1;
-}
-
-Widget* ListBox::getChildByIndex(int index)
-{
-  const WidgetsList& children = this->children();
-  if (index >= 0 && index < int(children.size()))
-    return children[index];
-  else
-    return nullptr;
-}
-
 void ListBox::selectChild(Widget* item, Message* msg)
 {
   bool didChange = false;
@@ -152,7 +133,10 @@ void ListBox::selectChild(Widget* item, Message* msg)
 
 void ListBox::selectIndex(int index, Message* msg)
 {
-  Widget* child = getChildByIndex(index);
+  if (index < 0 || index >= int(children().size()))
+    return;
+
+  Widget* child = at(index);
   if (child)
     selectChild(child, msg);
 }
@@ -456,8 +440,8 @@ int ListBox::advanceIndexThroughVisibleItems(
       index = 0-sgn;
       cycle = true;
     }
-    else {
+    else if (index >= 0 && index < children().size()) {
-      Widget* item = getChildByIndex(index);
+      Widget* item = at(index);
       if (item &&
           !item->hasFlags(HIDDEN) &&
           // We can completely ignore separators from navigation

src/ui/listbox.h

@@ -1,5 +1,5 @@
 // Aseprite UI Library
-// Copyright (C) 2020  Igara Studio S.A.
+// Copyright (C) 2020-2022  Igara Studio S.A.
 // Copyright (C) 2001-2017  David Capello
 //
 // This file is released under the terms of the MIT license.
@@ -49,9 +49,6 @@ namespace ui {
     virtual void onChange();
     virtual void onDoubleClickItem();
 
-    int getChildIndex(Widget* item);
-    Widget* getChildByIndex(int index);
-
     int advanceIndexThroughVisibleItems(
       int startIndex, int delta, const bool loop);
 

src/ui/listitem.cpp

@@ -1,5 +1,5 @@
 // Aseprite UI Library
-// Copyright (C) 2019  Igara Studio S.A.
+// Copyright (C) 2019-2022  Igara Studio S.A.
 // Copyright (C) 2001-2017  David Capello
 //
 // This file is released under the terms of the MIT license.
@@ -60,8 +60,16 @@ void ListItem::onSizeHint(SizeHintEvent& ev)
   int w = 0, h = 0;
   Size maxSize;
 
-  if (hasText())
+  if (hasText()) {
-    maxSize = textSize();
+    if (m_textLength >= 0) {
+      maxSize.w = m_textLength;
+      maxSize.h = textHeight();
+    }
+    else {
+      maxSize = textSize();
+      m_textLength = maxSize.w;
+    }
+  }
   else
     maxSize.w = maxSize.h = 0;
 
@@ -78,4 +86,16 @@ void ListItem::onSizeHint(SizeHintEvent& ev)
   ev.setSizeHint(Size(w, h));
 }
 
+void ListItem::onInitTheme(InitThemeEvent& ev)
+{
+  Widget::onInitTheme(ev);
+  m_textLength = -1;
+}
+
+void ListItem::onSetText()
+{
+  Widget::onSetText();
+  m_textLength = -1;
+}
+
 } // namespace ui

src/ui/listitem.h

@@ -1,4 +1,5 @@
 // Aseprite UI Library
+// Copyright (C) 2022  Igara Studio S.A.
 // Copyright (C) 2001-2017  David Capello
 //
 // This file is released under the terms of the MIT license.
@@ -26,9 +27,12 @@ namespace ui {
     bool onProcessMessage(Message* msg) override;
     void onResize(ResizeEvent& ev) override;
     void onSizeHint(SizeHintEvent& ev) override;
+    void onInitTheme(InitThemeEvent& ev) override;
+    void onSetText() override;
 
   private:
     std::string m_value;
+    int m_textLength = -1;
   };
 
 } // namespace ui

src/ui/widget.cpp

@@ -66,6 +66,7 @@ Widget::Widget(WidgetType type)
   , m_bgColor(gfx::ColorNone)
   , m_bounds(0, 0, 0, 0)
   , m_parent(nullptr)
+  , m_parentIndex(-1)
   , m_sizeHint(nullptr)
   , m_mnemonic(0)
   , m_minSize(0, 0)
@@ -424,11 +425,17 @@ Manager* Widget::manager() const
 
 int Widget::getChildIndex(Widget* child)
 {
-  auto it = std::find(m_children.begin(), m_children.end(), child);
+  if (!child)
-  if (it != m_children.end())
-    return it - m_children.begin();
-  else
     return -1;
+
+#ifdef _DEBUG
+  ASSERT(child->parent() == this);
+  auto it = std::find(m_children.begin(), m_children.end(), child);
+  ASSERT(it != m_children.end());
+  ASSERT(child->parentIndex() == (it - m_children.begin()));
+#endif
+
+  return child->parentIndex();
 }
 
 Widget* Widget::nextSibling()
@@ -438,12 +445,9 @@ Widget* Widget::nextSibling()
   if (!m_parent)
     return nullptr;
 
-  WidgetsList::iterator begin = m_parent->m_children.begin();
+  auto begin = m_parent->m_children.begin();
-  WidgetsList::iterator end = m_parent->m_children.end();
+  auto end = m_parent->m_children.end();
-  WidgetsList::iterator it = std::find(begin, end, this);
+  auto it = begin + m_parentIndex;
-
-  if (it == end)
-    return nullptr;
 
   if (++it == end)
     return nullptr;
@@ -458,11 +462,10 @@ Widget* Widget::previousSibling()
   if (!m_parent)
     return nullptr;
 
-  WidgetsList::iterator begin = m_parent->m_children.begin();
+  auto begin = m_parent->m_children.begin();
-  WidgetsList::iterator end = m_parent->m_children.end();
+  auto it = begin + m_parentIndex;
-  WidgetsList::iterator it = std::find(begin, end, this);
 
-  if (it == begin || it == end)
+  if (it == begin)
     return nullptr;
 
   return *(--it);
@@ -536,32 +539,38 @@ void Widget::addChild(Widget* child)
   ASSERT_VALID_WIDGET(this);
   ASSERT_VALID_WIDGET(child);
 
+  int i = int(m_children.size());
   m_children.push_back(child);
   child->m_parent = this;
+  child->m_parentIndex = i;
 }
 
-void Widget::removeChild(WidgetsList::iterator& it)
+void Widget::removeChild(const WidgetsList::iterator& it)
 {
   Widget* child = *it;
 
   ASSERT(it != m_children.end());
-  if (it != m_children.end())
+  if (it != m_children.end()) {
-    m_children.erase(it);
+    auto it2 = m_children.erase(it);
+    for (auto end=m_children.end(); it2!=end; ++it2)
+      --(*it2)->m_parentIndex;
+  }
 
   // Free from manager
   if (auto man = manager())
     man->freeWidget(child);
 
   child->m_parent = nullptr;
+  child->m_parentIndex = -1;
 }
 
 void Widget::removeChild(Widget* child)
 {
   ASSERT_VALID_WIDGET(this);
   ASSERT_VALID_WIDGET(child);
-
+  ASSERT(child->parent() == this);
-  WidgetsList::iterator it = std::find(m_children.begin(), m_children.end(), child);
+  if (child->parent() == this)
-  removeChild(it);
+    removeChild(m_children.begin() + child->m_parentIndex);
 }
 
 void Widget::removeAllChildren()
@@ -575,18 +584,29 @@ void Widget::replaceChild(Widget* oldChild, Widget* newChild)
   ASSERT_VALID_WIDGET(oldChild);
   ASSERT_VALID_WIDGET(newChild);
 
-  WidgetsList::iterator before =
+#if _DEBUG
-    std::find(m_children.begin(), m_children.end(), oldChild);
+  {
-  if (before == m_children.end()) {
+    auto it = std::find(m_children.begin(), m_children.end(), oldChild);
+    ASSERT(it != m_children.end());
+    ASSERT(oldChild->m_parentIndex == (it - m_children.begin()));
+  }
+#endif
+
+  if (oldChild->parent() != this) {
     ASSERT(false);
     return;
   }
-  int index = before - m_children.begin();
+  int index = oldChild->m_parentIndex;
 
   removeChild(oldChild);
 
-  m_children.insert(m_children.begin()+index, newChild);
+  auto it = m_children.begin() + index;
+  it = m_children.insert(it, newChild);
+  for (auto end=m_children.end(); it!=end; ++it)
+    ++(*it)->m_parentIndex;
+
   newChild->m_parent = this;
+  newChild->m_parentIndex = index;
 }
 
 void Widget::insertChild(int index, Widget* child)
@@ -594,21 +614,37 @@ void Widget::insertChild(int index, Widget* child)
   ASSERT_VALID_WIDGET(this);
   ASSERT_VALID_WIDGET(child);
 
-  m_children.insert(m_children.begin()+index, child);
+  index = base::clamp(index, 0, int(m_children.size()));
+
+  auto it = m_children.begin() + index;
+  it = m_children.insert(it, child);
+  ++it;
+  for (auto end=m_children.end(); it!=end; ++it)
+    ++(*it)->m_parentIndex;
+
   child->m_parent = this;
+  child->m_parentIndex = index;
 }
 
 void Widget::moveChildTo(Widget* thisChild, Widget* toThisPosition)
 {
-  auto itA = std::find(m_children.begin(), m_children.end(), thisChild);
+  ASSERT(thisChild->parent() == this);
-  auto itB = std::find(m_children.begin(), m_children.end(), toThisPosition);
+  ASSERT(toThisPosition->parent() == this);
-  if (itA == m_children.end()) {
+
-    ASSERT(false);
+  const int from = thisChild->m_parentIndex;
-    return;
+  const int to = toThisPosition->m_parentIndex;
-  }
+
-  int index = itB - m_children.begin();
+  auto it = m_children.begin() + from;
-  m_children.erase(itA);
+  it = m_children.erase(it);
-  m_children.insert(m_children.begin() + index, thisChild);
+  auto end = m_children.end();
+  for (; it!=end; ++it)
+    --(*it)->m_parentIndex;
+
+  it = m_children.begin() + to;
+  it = m_children.insert(it, thisChild);
+  thisChild->m_parentIndex = to;
+  for (++it, end=m_children.end(); it!=end; ++it)
+    ++(*it)->m_parentIndex;
 }
 
 // ===============================================================

src/ui/widget.h

@@ -160,6 +160,7 @@ namespace ui {
 
     Window* window() const;
     Widget* parent() const { return m_parent; }
+    int parentIndex() const { return m_parentIndex; }
     Manager* manager() const;
 
     // Returns a list of children.
@@ -392,7 +393,7 @@ namespace ui {
     virtual double onGetTextDouble() const;
 
   private:
-    void removeChild(WidgetsList::iterator& it);
+    void removeChild(const WidgetsList::iterator& it);
     void paint(Graphics* graphics,
                const gfx::Region& drawRegion,
                const bool isBg);
@@ -412,6 +413,7 @@ namespace ui {
     gfx::Region m_updateRegion;   // Region to be redrawed.
     WidgetsList m_children;       // Sub-widgets
     Widget* m_parent;             // Who is the parent?
+    int m_parentIndex;            // Location/index of this widget in the parent's Widget::m_children vector
     gfx::Size* m_sizeHint;
     int m_mnemonic;               // Keyboard shortcut to access this widget like Alt+mnemonic
 

src/ui/widget_ui_tests.cpp

@@ -0,0 +1,57 @@
+// Aseprite UI Library
+// Copyright (C) 2022  Igara Studio S.A.
+//
+// This file is released under the terms of the MIT license.
+// Read LICENSE.txt for more information.
+
+#define TEST_GUI
+#include "tests/app_test.h"
+
+using namespace ui;
+
+TEST(Widget, ParentIndex)
+{
+  Widget a, b, c, d, e;
+  EXPECT_EQ(-1, b.parentIndex());
+
+  a.addChild(&b);
+  a.addChild(&c);
+  a.addChild(&d);
+  EXPECT_EQ(0, b.parentIndex());
+  EXPECT_EQ(1, c.parentIndex());
+  EXPECT_EQ(2, d.parentIndex());
+
+  a.removeChild(&c);
+  EXPECT_EQ(0, b.parentIndex());
+  EXPECT_EQ(1, d.parentIndex());
+  EXPECT_EQ(-1, c.parentIndex());
+
+  a.replaceChild(&b, &c);
+  EXPECT_EQ(0, c.parentIndex());
+  EXPECT_EQ(1, d.parentIndex());
+  EXPECT_EQ(-1, b.parentIndex());
+
+  a.insertChild(1, &e);
+  EXPECT_EQ(0, c.parentIndex());
+  EXPECT_EQ(1, e.parentIndex());
+  EXPECT_EQ(2, d.parentIndex());
+  EXPECT_EQ(-1, b.parentIndex());
+
+  a.insertChild(10, &b);
+  EXPECT_EQ(0, c.parentIndex());
+  EXPECT_EQ(1, e.parentIndex());
+  EXPECT_EQ(2, d.parentIndex());
+  EXPECT_EQ(3, b.parentIndex());
+
+  a.moveChildTo(&c, &b);
+  EXPECT_EQ(0, e.parentIndex());
+  EXPECT_EQ(1, d.parentIndex());
+  EXPECT_EQ(2, b.parentIndex());
+  EXPECT_EQ(3, c.parentIndex());
+
+  a.moveChildTo(&b, &e);
+  EXPECT_EQ(0, b.parentIndex());
+  EXPECT_EQ(1, e.parentIndex());
+  EXPECT_EQ(2, d.parentIndex());
+  EXPECT_EQ(3, c.parentIndex());
+}