From 0494f1c93b423c99563d286dc55954936fd626ac Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Thu, 22 Mar 2018 15:44:25 +0100 Subject: [PATCH] [Tree|Flat]MergeProxyModel: cleanup index mapping when removing rows --- .../src/task_list_model.cpp | 1 - .../src/task_list_model.h | 2 +- .../utils/flat_merge_proxy_model.cpp | 77 +++++++++++++++---- .../utils/tree_merge_proxy_model.cpp | 68 +++++++++++++--- .../utils/tree_merge_proxy_model.h | 2 +- 5 files changed, 122 insertions(+), 28 deletions(-) diff --git a/visualization/motion_planning_tasks/src/task_list_model.cpp b/visualization/motion_planning_tasks/src/task_list_model.cpp index e4b81f6d..9fdfda9b 100644 --- a/visualization/motion_planning_tasks/src/task_list_model.cpp +++ b/visualization/motion_planning_tasks/src/task_list_model.cpp @@ -212,7 +212,6 @@ void TaskListModel::processTaskDescriptionMessage(const std::string& id, if (!msg.stages.empty() && remote_task && (remote_task->taskFlags() & BaseTaskModel::IS_DESTROYED)) { removeModel(remote_task); - delete remote_task; created = true; // re-create remote task after it was destroyed beforehand } diff --git a/visualization/motion_planning_tasks/src/task_list_model.h b/visualization/motion_planning_tasks/src/task_list_model.h index 29950221..0500363a 100644 --- a/visualization/motion_planning_tasks/src/task_list_model.h +++ b/visualization/motion_planning_tasks/src/task_list_model.h @@ -143,7 +143,7 @@ public: Q_ASSERT(model && model->columnCount() == columnCount()); // pass on stage factory model->setStageFactory(stage_factory_); - // just wrap the base class method to further constrain the model type + // forward to base class method return FlatMergeProxyModel::insertModel(model, pos); } diff --git a/visualization/motion_planning_tasks/utils/flat_merge_proxy_model.cpp b/visualization/motion_planning_tasks/utils/flat_merge_proxy_model.cpp index 614b6fb2..22103746 100644 --- a/visualization/motion_planning_tasks/utils/flat_merge_proxy_model.cpp +++ b/visualization/motion_planning_tasks/utils/flat_merge_proxy_model.cpp @@ -51,17 +51,55 @@ public: ModelData(QAbstractItemModel* m) : model_(m) {} QAbstractItemModel* model_; - // map of proxy=source QModelIndex's internal pointer to source QModelIndex - std::map proxy_to_source_mapping_; + // map of proxy=source QModelIndex's internal pointer to source parent's QModelIndex + typedef std::map ProxyToSourceMap; + ProxyToSourceMap proxy_to_source_mapping_; + std::vector invalidated_mappings_; inline void storeMapping(void* src_internal_pointer, const QModelIndex &src_parent) { - decltype(proxy_to_source_mapping_)::value_type pair (src_internal_pointer, src_parent); + ProxyToSourceMap::value_type pair (src_internal_pointer, src_parent); auto it = proxy_to_source_mapping_.insert(std::move(pair)).first; #ifndef NDEBUG - Q_ASSERT_X(it->second == src_parent, "TreeMergeProxyModel", + Q_ASSERT_X(it->second == src_parent, "FlatMergeProxyModel", "the internal pointer must map to a unique src_parent"); #endif } + + // collect all invalidated mappings, return true if we are affected at all + bool rowsAboutToBeRemoved(const QModelIndex& src_parent, int start, int end) { + std::vector pointers; + pointers.reserve(end - start); + for (int row = start; row != end; ++row) + pointers.emplace_back(model_->index(row, 0, src_parent).internalPointer()); + + Q_ASSERT(invalidated_mappings_.empty()); + for (auto it = proxy_to_source_mapping_.begin(); it != proxy_to_source_mapping_.end(); ++it) { + QModelIndex current = it->second; + if (src_parent == current) { // it is on affected level + if (std::find(pointers.begin(), pointers.end(), it->first) != pointers.end()) + invalidated_mappings_.push_back(it); + } else { // not on affected level, check parents + while (current.isValid()) { + QModelIndex current_parent = current.parent(); + if (current_parent == src_parent) { // current on affected level + if (current.row() >= start && current.row() < end) + invalidated_mappings_.push_back(it); + break; + } + current = current_parent; + } + } + } + return !invalidated_mappings_.empty(); + } + bool rowsRemoved() { + bool affected = !invalidated_mappings_.empty(); + // remove invalidated mappings + for (auto it : invalidated_mappings_) + proxy_to_source_mapping_.erase(it); + invalidated_mappings_.clear(); + return affected; + } }; // top-level items @@ -70,6 +108,11 @@ public: public: FlatMergeProxyModelPrivate(FlatMergeProxyModel* q_ptr) : q_ptr(q_ptr) {} + std::vector::iterator find(const QObject* model) { + Q_ASSERT(model); + return std::find_if(data_.begin(), data_.end(), [model](const auto& data) { return data.model_ == model; }); + } + int accumulatedRowCount(std::vector::const_iterator start, std::vector::const_iterator end) const { return std::accumulate(start, end, 0, @@ -159,7 +202,7 @@ public: } // remove model referenced by it, call indicates that onRemoveModel() should be called - bool removeModel(std::vector::iterator& it, bool call); + bool removeModel(std::vector::iterator it, bool call); private: void _q_sourceDestroyed(QObject*model); @@ -423,9 +466,7 @@ FlatMergeProxyModel::getModel(const QModelIndex &index) const bool FlatMergeProxyModel::removeModel(QAbstractItemModel *model) { - auto it = std::find_if(d_ptr->data_.begin(), d_ptr->data_.end(), - [model](const auto& data) { return data.model_ == model; }); - return d_ptr->removeModel(it, true); + return d_ptr->removeModel(d_ptr->find(model), true); } bool FlatMergeProxyModel::removeModel(int pos) @@ -465,7 +506,7 @@ void FlatMergeProxyModel::onRemoveModel(QAbstractItemModel *model) } -bool FlatMergeProxyModelPrivate::removeModel(std::vector::iterator& it, bool call) +bool FlatMergeProxyModelPrivate::removeModel(std::vector::iterator it, bool call) { if (it == data_.end()) return false; @@ -480,9 +521,7 @@ bool FlatMergeProxyModelPrivate::removeModel(std::vector::iterator& i void FlatMergeProxyModelPrivate::_q_sourceDestroyed(QObject *model) { - auto it = std::find_if(data_.begin(), data_.end(), - [model](const auto& data) { return data.model_ == model; }); - removeModel(it, false); + removeModel(find(model), false); } void FlatMergeProxyModelPrivate::_q_sourceRowsAboutToBeInserted(const QModelIndex &parent, int start, int end) @@ -501,8 +540,12 @@ void FlatMergeProxyModelPrivate::_q_sourceRowsAboutToBeMoved(const QModelIndex & void FlatMergeProxyModelPrivate::_q_sourceRowsAboutToBeRemoved(const QModelIndex &parent, int start, int end) { - int offset = parent.isValid() ? 0 : rowOffset(q_ptr->sender()); - q_ptr->beginRemoveRows(mapFromSource(parent), start+offset, end+offset); + auto it = find(q_ptr->sender()); + Q_ASSERT(it != data_.end()); + if (it->rowsAboutToBeRemoved(parent, start, end+1)) { + int offset = parent.isValid() ? 0 : rowOffset(q_ptr->sender()); + q_ptr->beginRemoveRows(mapFromSource(parent), start+offset, end+offset); + } } void FlatMergeProxyModelPrivate::_q_sourceRowsInserted(const QModelIndex &parent, int start, int end) @@ -528,7 +571,11 @@ void FlatMergeProxyModelPrivate::_q_sourceRowsRemoved(const QModelIndex &parent, Q_UNUSED(parent) Q_UNUSED(start) Q_UNUSED(end) - q_ptr->endRemoveRows(); + + auto it = find(q_ptr->sender()); + Q_ASSERT(it != data_.end()); + if (it->rowsRemoved()) + q_ptr->endRemoveRows(); } #if QT_VERSION >= QT_VERSION_CHECK(5, 0, 0) diff --git a/visualization/motion_planning_tasks/utils/tree_merge_proxy_model.cpp b/visualization/motion_planning_tasks/utils/tree_merge_proxy_model.cpp index b6629c59..daec1799 100644 --- a/visualization/motion_planning_tasks/utils/tree_merge_proxy_model.cpp +++ b/visualization/motion_planning_tasks/utils/tree_merge_proxy_model.cpp @@ -53,17 +53,55 @@ public: QString name_; QAbstractItemModel* model_; - // map of proxy=source QModelIndex's internal pointer to source QModelIndex - std::map proxy_to_source_mapping_; + // map of proxy=source QModelIndex's internal pointer to source parent's QModelIndex + typedef std::map ProxyToSourceMap; + ProxyToSourceMap proxy_to_source_mapping_; + std::vector invalidated_mappings_; inline void storeMapping(void* src_internal_pointer, const QModelIndex &src_parent) { - decltype(proxy_to_source_mapping_)::value_type pair (src_internal_pointer, src_parent); + ProxyToSourceMap::value_type pair (src_internal_pointer, src_parent); auto it = proxy_to_source_mapping_.insert(std::move(pair)).first; #ifndef NDEBUG Q_ASSERT_X(it->second == src_parent, "TreeMergeProxyModel", "the internal pointer must map to a unique src_parent"); #endif } + + // collect all invalidated mappings, return true if we are affected at all + bool rowsAboutToBeRemoved(const QModelIndex& src_parent, int start, int end) { + std::vector pointers; + pointers.reserve(end - start); + for (int row = start; row != end; ++row) + pointers.emplace_back(model_->index(row, 0, src_parent).internalPointer()); + + Q_ASSERT(invalidated_mappings_.empty()); + for (auto it = proxy_to_source_mapping_.begin(); it != proxy_to_source_mapping_.end(); ++it) { + QModelIndex current = it->second; + if (src_parent == current) { // it is on affected level + if (std::find(pointers.begin(), pointers.end(), it->first) != pointers.end()) + invalidated_mappings_.push_back(it); + } else { // not on affected level, check parents + while (current.isValid()) { + QModelIndex current_parent = current.parent(); + if (current_parent == src_parent) { // current on affected level + if (current.row() >= start && current.row() < end) + invalidated_mappings_.push_back(it); + break; + } + current = current_parent; + } + } + } + return !invalidated_mappings_.empty(); + } + bool rowsRemoved() { + bool affected = !invalidated_mappings_.empty(); + // remove invalidated mappings + for (auto it : invalidated_mappings_) + proxy_to_source_mapping_.erase(it); + invalidated_mappings_.clear(); + return affected; + } }; // top-level items @@ -72,6 +110,11 @@ public: public: TreeMergeProxyModelPrivate(TreeMergeProxyModel* q_ptr) : q_ptr(q_ptr) {} + std::vector::iterator find(const QObject* model) { + Q_ASSERT(model); + return std::find_if(data_.begin(), data_.end(), [model](const auto& data) { return data.model_ == model; }); + } + // retrieve the source_index corresponding to proxy_index QModelIndex mapToSource(const QModelIndex &proxy_index, ModelData*& data) const { Q_ASSERT(proxy_index.isValid()); @@ -145,7 +188,7 @@ public: return data_.end(); } - bool removeModel(std::vector::iterator& it, bool call); + bool removeModel(std::vector::iterator it, bool call); private: void _q_sourceDestroyed(QObject*model); @@ -449,7 +492,7 @@ void TreeMergeProxyModel::onRemoveModel(QAbstractItemModel *model) } -bool TreeMergeProxyModelPrivate::removeModel(std::vector::iterator& it, bool call) +bool TreeMergeProxyModelPrivate::removeModel(std::vector::iterator it, bool call) { if (it == data_.end()) return false; @@ -464,9 +507,7 @@ bool TreeMergeProxyModelPrivate::removeModel(std::vector::iterator& i void TreeMergeProxyModelPrivate::_q_sourceDestroyed(QObject *model) { - auto it = std::find_if(data_.begin(), data_.end(), - [model](const auto& data) { return data.model_ == model; }); - removeModel(it, false); + removeModel(find(model), false); } void TreeMergeProxyModelPrivate::_q_sourceRowsAboutToBeInserted(const QModelIndex &parent, int start, int end) @@ -481,7 +522,10 @@ void TreeMergeProxyModelPrivate::_q_sourceRowsAboutToBeMoved(const QModelIndex & void TreeMergeProxyModelPrivate::_q_sourceRowsAboutToBeRemoved(const QModelIndex &parent, int start, int end) { - q_ptr->beginRemoveRows(mapFromSource(parent), start, end); + auto it = find(q_ptr->sender()); + Q_ASSERT(it != data_.end()); + if (it->rowsAboutToBeRemoved(parent, start, end+1)) + q_ptr->beginRemoveRows(mapFromSource(parent), start, end); } void TreeMergeProxyModelPrivate::_q_sourceRowsInserted(const QModelIndex &parent, int start, int end) @@ -507,7 +551,11 @@ void TreeMergeProxyModelPrivate::_q_sourceRowsRemoved(const QModelIndex &parent, Q_UNUSED(parent) Q_UNUSED(start) Q_UNUSED(end) - q_ptr->endRemoveRows(); + + auto it = find(q_ptr->sender()); + Q_ASSERT(it != data_.end()); + if (it->rowsRemoved()) + q_ptr->endRemoveRows(); } #if QT_VERSION >= QT_VERSION_CHECK(5, 0, 0) diff --git a/visualization/motion_planning_tasks/utils/tree_merge_proxy_model.h b/visualization/motion_planning_tasks/utils/tree_merge_proxy_model.h index 9aa5d372..1e685a49 100644 --- a/visualization/motion_planning_tasks/utils/tree_merge_proxy_model.h +++ b/visualization/motion_planning_tasks/utils/tree_merge_proxy_model.h @@ -45,7 +45,7 @@ namespace utils { class TreeMergeProxyModelPrivate; /** The TreeMergeProxyModel merges several models into one. * - * Each embedded model gets a new top-level item (with a given name) + * Each embedded model becomes a top-level item (with a given name) * and all the model's top-level items will appear as its children. */ class TreeMergeProxyModel : public QAbstractItemModel {