Better handling of layoutChanged in by-priority proxy to avoid resetting.

5 files changed, 106 insertions, 88 deletions

diff --git a/src/modlistbypriorityproxy.cpp b/src/modlistbypriorityproxy.cpp
index 9b5b2f03..a0623e8c 100644
--- a/src/modlistbypriorityproxy.cpp
+++ b/src/modlistbypriorityproxy.cpp
@@ -25,17 +25,13 @@ void ModListByPriorityProxy::setSourceModel(QAbstractItemModel* model)
   QAbstractProxyModel::setSourceModel(model);
 
   if (sourceModel()) {
-    connect(sourceModel(), &QAbstractItemModel::layoutChanged, this, &ModListByPriorityProxy::buildTree, Qt::UniqueConnection);
-    connect(sourceModel(), &QAbstractItemModel::rowsRemoved, this, &ModListByPriorityProxy::buildTree, Qt::UniqueConnection);
-    connect(sourceModel(), &QAbstractItemModel::modelReset, this, &ModListByPriorityProxy::buildTree, Qt::UniqueConnection);
-    connect(sourceModel(), &QAbstractItemModel::dataChanged, this, &ModListByPriorityProxy::modelDataChanged, Qt::UniqueConnection);
-    refresh();
-  }
-}
+    connect(sourceModel(), &QAbstractItemModel::layoutChanged, this, &ModListByPriorityProxy::onModelLayoutChanged, Qt::UniqueConnection);
+    connect(sourceModel(), &QAbstractItemModel::rowsRemoved, this, &ModListByPriorityProxy::onModelRowsRemoved, Qt::UniqueConnection);
+    connect(sourceModel(), &QAbstractItemModel::modelReset, this, &ModListByPriorityProxy::onModelReset, Qt::UniqueConnection);
+    connect(sourceModel(), &QAbstractItemModel::dataChanged, this, &ModListByPriorityProxy::onModelDataChanged, Qt::UniqueConnection);
 
-void ModListByPriorityProxy::refresh()
-{
-  buildTree();
+    onModelReset();
+  }
 }
 
 void ModListByPriorityProxy::setProfile(Profile* profile)
@@ -48,46 +44,54 @@ void ModListByPriorityProxy::setSortOrder(Qt::SortOrder order)
   m_sortOrder = order;
 }
 
+void ModListByPriorityProxy::buildMapping()
+{
+  m_IndexToItem.clear();
+  for (unsigned int index = 0; index < ModInfo::getNumMods(); ++index) {
+    m_IndexToItem[index] = std::make_unique<TreeItem>(ModInfo::getByIndex(index), index);
+  }
+}
+
 void ModListByPriorityProxy::buildTree()
 {
   if (!sourceModel()) return;
 
-  beginResetModel();
-
   // reset the root
   m_Root = { };
-  m_IndexToItem.clear();
+
+  // clear all children
+  for (auto& [index, item] : m_IndexToItem) {
+    item->children.clear();
+  }
 
   TreeItem* root = &m_Root;
-  std::unique_ptr<TreeItem> overwrite;
-  std::vector<std::unique_ptr<TreeItem>> backups;
+  TreeItem* overwrite;
+  std::vector<TreeItem*> backups;
 
   auto fn = [&](const auto& p) {
     auto& [priority, index] = p;
     ModInfo::Ptr modInfo = ModInfo::getByIndex(index);
-
-    TreeItem* item;
+    TreeItem* item = m_IndexToItem[index].get();
 
     if (modInfo->isSeparator()) {
-      m_Root.children.push_back(std::make_unique<TreeItem>(modInfo, index, &m_Root));
-      item = m_Root.children.back().get();
+      item->parent = &m_Root;
+      m_Root.children.push_back(item);
       root = item;
     }
     else if (modInfo->isOverwrite()) {
       // do not push here, because the overwrite is usually not at the right position
-      overwrite = std::make_unique<TreeItem>(modInfo, index, &m_Root);
-      item = overwrite.get();
+      item->parent = &m_Root;
+      overwrite = item;
     }
     else if (modInfo->isBackup()) {
-      backups.push_back(std::make_unique<TreeItem>(modInfo, index, &m_Root));
-      item = backups.back().get();
+      // do not push here, because backups are usually not at the right position
+      item->parent = &m_Root;
+      backups.push_back(item);
     }
     else {
-      root->children.push_back(std::make_unique<TreeItem>(modInfo, index, root));
-      item = root->children.back().get();
+      item->parent = root;
+      root->children.push_back(item);
     }
-
-    m_IndexToItem[index] = item;
   };
 
   auto& ibp = m_profile->getAllIndexesByPriority();
@@ -103,11 +107,41 @@ void ModListByPriorityProxy::buildTree()
     m_Root.children.insert(m_Root.children.end(),
       std::make_move_iterator(backups.begin()), std::make_move_iterator(backups.end()));
   }
+}
+
+void ModListByPriorityProxy::onModelRowsRemoved(const QModelIndex& parent, int first, int last)
+{
+  onModelReset();
+}
+
+void ModListByPriorityProxy::onModelLayoutChanged(const QList<QPersistentModelIndex>&, LayoutChangeHint hint)
+{
+  MOBase::log::debug("LAYOUT");
+
+  emit layoutAboutToBeChanged();
+  auto persistent = persistentIndexList();
+  buildTree();
+
+  QModelIndexList toPersistent;
+  for (auto& idx : persistent) {
+    // we can still access the TreeItem* because we did not destroy them
+    auto* item = static_cast<TreeItem*>(idx.internalPointer());
+    toPersistent.append(createIndex(item->parent->childIndex(item), idx.column(), item));
+  }
+  changePersistentIndexList(persistent, toPersistent);
+
+  emit layoutChanged({}, hint);
+}
 
+void ModListByPriorityProxy::onModelReset()
+{
+  beginResetModel();
+  buildMapping();
+  buildTree();
   endResetModel();
 }
 
-void ModListByPriorityProxy::modelDataChanged(const QModelIndex& topLeft, const QModelIndex& bottomRight, const QVector<int>& roles)
+void ModListByPriorityProxy::onModelDataChanged(const QModelIndex& topLeft, const QModelIndex& bottomRight, const QVector<int>& roles)
 {
   QModelIndex proxyTopLeft = mapFromSource(topLeft);
   if (!proxyTopLeft.isValid()) {
@@ -129,7 +163,7 @@ QModelIndex ModListByPriorityProxy::mapFromSource(const QModelIndex& sourceIndex
     return QModelIndex();
   }
 
-  auto* item = m_IndexToItem.at(sourceIndex.row());
+  auto* item = m_IndexToItem.at(sourceIndex.row()).get();
   return createIndex(item->parent->childIndex(item), sourceIndex.column(), item);
 }
 
@@ -175,7 +209,7 @@ QModelIndex ModListByPriorityProxy::parent(const QModelIndex& child) const
     return QModelIndex();
   }
 
-  return createIndex(item->parent->parent->childIndex(item->parent), child.column(), item->parent);
+  return createIndex(item->parent->parent->childIndex(item->parent), 0, item->parent);
 }
 
 bool ModListByPriorityProxy::hasChildren(const QModelIndex& parent) const
@@ -343,7 +377,7 @@ bool ModListByPriorityProxy::dropMimeData(const QMimeData* data, Qt::DropAction
       // in descending priority, we take the separator itself if it's empty or
       // its last children
       else {
-        auto* item = m_Root.children[parent.row()].get();
+        auto* item = m_Root.children[parent.row()];
         sourceRow = item->children.empty() ? item->index : item->children.back()->index;
       }
     }
@@ -366,7 +400,7 @@ QModelIndex ModListByPriorityProxy::index(int row, int column, const QModelIndex
     parentItem = static_cast<TreeItem*>(parent.internalPointer());
   }
 
-  return createIndex(row, column, parentItem->children[row].get());
+  return createIndex(row, column, parentItem->children[row]);
 }
 
 void ModListByPriorityProxy::onDropEnter(const QMimeData*, bool dropExpanded, ModListView::DropPosition dropPosition)
diff --git a/src/modlistbypriorityproxy.h b/src/modlistbypriorityproxy.h
index 380e257b..77e0fe3d 100644
--- a/src/modlistbypriorityproxy.h
+++ b/src/modlistbypriorityproxy.h
@@ -33,10 +33,6 @@ public:
   //
   void setSortOrder(Qt::SortOrder order);
 
-  // refresh the proxy by rebuilding the inner structure
-  //
-  void refresh();
-
   void setSourceModel(QAbstractItemModel* sourceModel) override;
 
   int rowCount(const QModelIndex& parent = QModelIndex()) const override;
@@ -57,21 +53,33 @@ public slots:
 
 protected slots:
 
-  void modelDataChanged(const QModelIndex& topLeft, const QModelIndex& bottomRight, const QVector<int>& roles = QVector<int>());
+  void onModelRowsRemoved(const QModelIndex& parent, int first, int last);
+  void onModelLayoutChanged(const QList<QPersistentModelIndex>& parents, LayoutChangeHint hint);
+  void onModelReset();
+  void onModelDataChanged(const QModelIndex& topLeft, const QModelIndex& bottomRight, const QVector<int>& roles = QVector<int>());
 
 private:
 
+  // fill the mapping from index to item (required by buildTree), should
+  // only be used for full model reset because it destroys existing references
+  // to tree items
+  //
+  void buildMapping();
+
+  // create the actual tree by creating parent/child associations, requires
+  // the mapping to be created (by a previous buildMapping call)
+  //
   void buildTree();
 
   struct TreeItem {
     ModInfo::Ptr mod;
     unsigned int index;
-    std::vector<std::unique_ptr<TreeItem>> children;
+    std::vector<TreeItem*> children;
     TreeItem* parent;
 
     std::size_t childIndex(TreeItem* child) const {
       for (std::size_t i = 0; i < children.size(); ++i) {
-        if (children[i].get() == child) {
+        if (children[i] == child) {
           return i;
         }
       }
@@ -84,7 +92,7 @@ private:
   };
 
   TreeItem m_Root;
-  std::map<unsigned int, TreeItem*> m_IndexToItem;
+  std::map<unsigned int, std::unique_ptr<TreeItem>> m_IndexToItem;
 
 private:
   OrganizerCore& m_core;
diff --git a/src/modlistview.cpp b/src/modlistview.cpp
index 763e852a..8e599e59 100644
--- a/src/modlistview.cpp
+++ b/src/modlistview.cpp
@@ -657,10 +657,6 @@ bool ModListView::moveSelection(int key)
   }

 

   m_core->modList()->shiftModsPriority(sourceRows, offset);

-

-  // reset the selection and the index

-  setSelected(cindex, sourceRows);

-

   return true;

 }

 

@@ -682,27 +678,25 @@ void ModListView::updateGroupByProxy()
 {

   int groupIndex = ui.groupBy->currentIndex();

   auto* previousModel = m_sortProxy->sourceModel();

-  QAbstractProxyModel* nextProxy = nullptr;

 

+  QAbstractItemModel* nextModel = m_core->modList();

   if (groupIndex == GroupBy::CATEGORY) {

-    nextProxy = m_byCategoryProxy;

+    nextModel = m_byCategoryProxy;

   }

   else if (groupIndex == GroupBy::NEXUS_ID) {

-    nextProxy = m_byNexusIdProxy;

+    nextModel = m_byNexusIdProxy;

   }

   else if (m_core->settings().interface().collapsibleSeparators(m_sortProxy->sortOrder())

     && m_sortProxy->sortColumn() == ModList::COL_PRIORITY) {

     m_byPriorityProxy->setSortOrder(m_sortProxy->sortOrder());

-    nextProxy = m_byPriorityProxy;

-  }

-

-  QAbstractItemModel* nextModel = m_core->modList();

-  if (nextProxy) {

-    nextProxy->setSourceModel(m_core->modList());

-    nextModel = nextProxy;

+    nextModel = m_byPriorityProxy;

   }

 

   if (nextModel != previousModel) {

+

+    if (auto* proxy = dynamic_cast<QAbstractProxyModel*>(nextModel)) {

+      proxy->setSourceModel(m_core->modList());

+    }

     m_sortProxy->setSourceModel(nextModel);

 

     // reset the source model of the old proxy because we do not want to

@@ -711,17 +705,18 @@ void ModListView::updateGroupByProxy()
     if (auto* proxy = qobject_cast<QAbstractProxyModel*>(previousModel)) {

       proxy->setSourceModel(nullptr);

     }

-  }

 

-  // expand items previously expanded

-  refreshExpandedItems();

+    // expand items previously expanded

+    refreshExpandedItems();

+

+    if (hasCollapsibleSeparators()) {

+      ui.filterSeparators->setCurrentIndex(ModListSortProxy::SeparatorFilter);

+      ui.filterSeparators->setEnabled(false);

+    }

+    else {

+      ui.filterSeparators->setEnabled(true);

+    }

 

-  if (hasCollapsibleSeparators()) {

-    ui.filterSeparators->setCurrentIndex(ModListSortProxy::SeparatorFilter);

-    ui.filterSeparators->setEnabled(false);

-  }

-  else {

-    ui.filterSeparators->setEnabled(true);

   }

 }

 

@@ -1356,17 +1351,10 @@ void ModListView::dropEvent(QDropEvent* event)
   // is no way to deduce this except using dropIndicatorPosition())

   emit dropEntered(event->mimeData(), isExpanded(index), static_cast<DropPosition>(dropIndicatorPosition()));

 

-  ModListDropInfo dropInfo(event->mimeData(), *m_core);

-

   // see selectedIndexes()

-  auto [current, selected] = this->selected();

   m_inDragMoveEvent = true;

   QTreeView::dropEvent(event);

   m_inDragMoveEvent = false;

-

-  if (dropInfo.isModDrop()) {

-    setSelected(current, selected);

-  }

 }

 

 void ModListView::timerEvent(QTimerEvent* event)

diff --git a/src/modlistviewactions.cpp b/src/modlistviewactions.cpp
index 5af79b3b..123fbf75 100644
--- a/src/modlistviewactions.cpp
+++ b/src/modlistviewactions.cpp
@@ -530,21 +530,14 @@ void ModListViewActions::displayModInformation(ModInfo::Ptr modInfo, unsigned in
   }
 }
 
-void ModListViewActions::setModsPriority(const QModelIndexList& indexes, int priority) const
-{
-  auto [current, selected] = m_view->selected();
-  m_core.modList()->changeModsPriority(indexes, priority);
-  m_view->setSelected(current, selected);
-}
-
 void ModListViewActions::sendModsToTop(const QModelIndexList& indexes) const
 {
-  setModsPriority(indexes, 0);
+  m_core.modList()->changeModsPriority(indexes, 0);
 }
 
 void ModListViewActions::sendModsToBottom(const QModelIndexList& indexes) const
 {
-  setModsPriority(indexes, std::numeric_limits<int>::max());
+  m_core.modList()->changeModsPriority(indexes, std::numeric_limits<int>::max());
 }
 
 void ModListViewActions::sendModsToPriority(const QModelIndexList& indexes) const
@@ -555,7 +548,7 @@ void ModListViewActions::sendModsToPriority(const QModelIndexList& indexes) cons
     0, 0, std::numeric_limits<int>::max(), 1, &ok);
   if (!ok) return;
 
-  setModsPriority(indexes, priority);
+  m_core.modList()->changeModsPriority(indexes, priority);
 }
 
 void ModListViewActions::sendModsToSeparator(const QModelIndexList& indexes) const
@@ -599,7 +592,7 @@ void ModListViewActions::sendModsToSeparator(const QModelIndexList& indexes) con
         --newPriority;
       }
 
-      setModsPriority(indexes, newPriority);
+      m_core.modList()->changeModsPriority(indexes, newPriority);
     }
   }
 }
@@ -622,7 +615,7 @@ void ModListViewActions::sendModsToFirstConflict(const QModelIndexList& indexes)
   });
 
   if (!priorities.empty()) {
-    setModsPriority(indexes, *priorities.begin());
+    m_core.modList()->changeModsPriority(indexes, *priorities.begin());
   }
 }
 
@@ -644,7 +637,7 @@ void ModListViewActions::sendModsToLastConflict(const QModelIndexList& indexes)
   });
 
   if (!priorities.empty()) {
-    setModsPriority(indexes, *priorities.rbegin());
+    m_core.modList()->changeModsPriority(indexes, *priorities.rbegin());
   }
 }
 
diff --git a/src/modlistviewactions.h b/src/modlistviewactions.h
index 3c225fbc..1914a8aa 100644
--- a/src/modlistviewactions.h
+++ b/src/modlistviewactions.h
@@ -150,11 +150,6 @@ private:
   //
   void checkModsForUpdates(std::multimap<QString, int> const& IDs) const;
 
-  // set the priorities of the given mods while maintaining the
-  // mod list selection (which may be different from the list of mods)
-  //
-  void setModsPriority(const QModelIndexList& indexes, int priority) const;
-
 private:
 
   OrganizerCore& m_core;