Commit b09c59a8 authored by Nicolas Carion's avatar Nicolas Carion

More defensive coding in the tree structure and more tests

parent 06cdf66e
......@@ -25,6 +25,9 @@
#include <QDebug>
#include <utility>
#include <unordered_set>
#include <queue>
int AbstractTreeModel::currentTreeId = 0;
AbstractTreeModel::AbstractTreeModel(QObject *parent)
: QAbstractItemModel(parent)
......@@ -34,7 +37,7 @@ AbstractTreeModel::AbstractTreeModel(QObject *parent)
std::shared_ptr<AbstractTreeModel> AbstractTreeModel::construct(QObject *parent)
{
std::shared_ptr<AbstractTreeModel> self(new AbstractTreeModel(parent));
self->rootItem.reset(new TreeItem(QList<QVariant>(), self, true));
self->rootItem = TreeItem::construct(QList<QVariant>(), self, true);
return self;
}
......@@ -194,3 +197,70 @@ std::shared_ptr<TreeItem> AbstractTreeModel::getRoot() const
{
return rootItem;
}
bool AbstractTreeModel::checkConsistency()
{
// first check that the root is all good
if (!rootItem || !rootItem->m_isRoot || !rootItem->isInModel()
|| m_allItems.count(rootItem->getId()) == 0){
qDebug() << !rootItem->m_isRoot << !rootItem->isInModel()
<< (m_allItems.count(rootItem->getId()) == 0);
qDebug() << "ERROR: Model is not valid because root is not properly constructed";
return false;
}
// Then we traverse the tree from the root, checking the infos on the way
std::unordered_set<int> seenIDs;
std::queue<std::pair<int, std::pair<int, int>> > queue; // store (id, (depth, parentId))
queue.push({rootItem->getId(), {0, rootItem->getId()}});
while (!queue.empty()) {
auto current = queue.front();
int currentId = current.first, currentDepth = current.second.first;
int parentId = current.second.second;
queue.pop();
if (seenIDs.count(currentId) != 0) {
qDebug() << "ERROR: Invalid tree: Id found twice."
<< "It either a cycle or a clash in id attribution";
return false;
}
if (m_allItems.count(currentId) == 0) {
qDebug() << "ERROR: Invalid tree: Id not found. Item is not registered";
return false;
}
auto currentItem = m_allItems[currentId].lock();
if (currentItem->depth() != currentDepth) {
qDebug() << "ERROR: Invalid tree: invalid depth info found";
return false;
}
if (!currentItem->isInModel()) {
qDebug() << "ERROR: Invalid tree: item thinks it is not in a model";
return false;
}
if (currentId != rootItem->getId()) {
if((currentDepth == 0 || currentItem->m_isRoot)) {
qDebug() << "ERROR: Invalid tree: duplicate root";
return false;
}
if (auto ptr = currentItem->parentItem().lock()) {
if (ptr->getId() != parentId ||
ptr->child(currentItem->row())->getId() != currentItem->getId()) {
qDebug() << "ERROR: Invalid tree: invalid parent link";
return false;
}
} else {
qDebug() << "ERROR: Invalid tree: invalid parent";
return false;
}
}
// propagate to children
int i = 0;
for (const auto &child : currentItem->m_childItems) {
if (currentItem->child(i) != child) {
qDebug() << "ERROR: Invalid tree: invalid child ordering";
return false;
}
queue.push({child->getId(), {currentDepth + 1, currentId}});
i++;
}
}
return true;
}
......@@ -100,6 +100,9 @@ protected:
*/
void notifyRowDeleted();
/* @brief This is a convenience function that helps check if the tree is in a valid state */
bool checkConsistency();
protected:
std::shared_ptr<TreeItem> rootItem;
......
......@@ -45,7 +45,6 @@ std::shared_ptr<TreeItem> TreeItem::construct(const QList<QVariant> &data, std::
// static
void TreeItem::baseFinishConstruct(const std::shared_ptr<TreeItem> &self)
{
qDebug() << "FINISHED constructing " << self->getId();
if (self->m_isRoot) {
self->registerSelf();
}
......@@ -68,8 +67,22 @@ std::shared_ptr<TreeItem> TreeItem::appendChild(const QList<QVariant> &data)
return std::shared_ptr<TreeItem>();
}
void TreeItem::appendChild(std::shared_ptr<TreeItem> child)
bool TreeItem::appendChild(std::shared_ptr<TreeItem> child)
{
if (hasAncestor(child->getId())) {
// in that case, we are trying to create a cycle, abort
return false;
}
if (auto oldParent = child->parentItem().lock()) {
if (oldParent->getId() == m_id) {
// no change needed
return true;
} else {
// in that case a call to removeChild should have been carried out
qDebug() << "ERROR: trying to append a child that alrealdy has a parent";
return false;
}
}
if (auto ptr = m_model.lock()) {
ptr->notifyRowAboutToAppend(shared_from_this());
child->m_depth = m_depth + 1;
......@@ -79,10 +92,11 @@ void TreeItem::appendChild(std::shared_ptr<TreeItem> child)
m_iteratorTable[id] = it;
child->registerSelf();
ptr->notifyRowAppended(child);
} else {
qDebug() << "ERROR: Something went wrong when appending child in TreeItem. Model is not available anymore";
Q_ASSERT(false);
return true;
}
qDebug() << "ERROR: Something went wrong when appending child in TreeItem. Model is not available anymore";
Q_ASSERT(false);
return false;
}
void TreeItem::moveChild(int ix, std::shared_ptr<TreeItem> child)
......@@ -99,7 +113,6 @@ void TreeItem::moveChild(int ix, std::shared_ptr<TreeItem> child)
ptr->notifyRowAboutToAppend(shared_from_this());
child->m_depth = m_depth + 1;
child->m_parentItem = shared_from_this();
qDebug() << "appending child2" << child->getId() << "to " << m_id;
int id = child->getId();
auto pos = m_childItems.begin();
std::advance(pos, ix);
......@@ -116,9 +129,9 @@ void TreeItem::moveChild(int ix, std::shared_ptr<TreeItem> child)
void TreeItem::removeChild(const std::shared_ptr<TreeItem> &child)
{
if (auto ptr = m_model.lock()) {
qDebug() << "removing child" << child->getId() << "from " << m_id;
ptr->notifyRowAboutToDelete(shared_from_this(), child->row());
// get iterator corresponding to child
Q_ASSERT(m_iteratorTable.count(child->getId()) > 0);
auto it = m_iteratorTable[child->getId()];
// deletion of child
m_childItems.erase(it);
......@@ -134,17 +147,26 @@ void TreeItem::removeChild(const std::shared_ptr<TreeItem> &child)
}
}
void TreeItem::changeParent(std::shared_ptr<TreeItem> newParent)
bool TreeItem::changeParent(std::shared_ptr<TreeItem> newParent)
{
Q_ASSERT(!m_isRoot);
qDebug() << "changing parent of " << m_id;
if (auto ptr = m_parentItem.lock()) {
ptr->removeChild(shared_from_this());
if (m_isRoot) return false;
std::shared_ptr<TreeItem> oldParent;
if (oldParent = m_parentItem.lock()) {
oldParent->removeChild(shared_from_this());
}
bool res = true;
if (newParent) {
newParent->appendChild(shared_from_this());
m_parentItem = newParent;
res = newParent->appendChild(shared_from_this());
if (res) {
m_parentItem = newParent;
} else if (oldParent){
// something went wrong, we have to reset the parent.
bool reverse = oldParent->appendChild(shared_from_this());
Q_ASSERT(reverse);
}
}
return res;
}
std::shared_ptr<TreeItem> TreeItem::child(int row) const
......@@ -226,3 +248,13 @@ void TreeItem::deregisterSelf()
}
}
}
bool TreeItem::hasAncestor(int id) {
if (m_id == id) {
return true;
}
if (auto ptr = m_parentItem.lock()) {
return ptr->hasAncestor(id);
}
return false;
}
......@@ -71,8 +71,9 @@ public:
/* @brief Appends an already created child
Useful for example if the child should be a subclass of TreeItem
@return true on success. Otherwise, nothing is modified.
*/
void appendChild(std::shared_ptr<TreeItem> child);
bool appendChild(std::shared_ptr<TreeItem> child);
void moveChild(int ix, std::shared_ptr<TreeItem> child);
/* @brief Remove given child from children list. The parent of the child is updated
......@@ -82,7 +83,7 @@ public:
/* @brief Change the parent of the current item. Structures are modified accordingly
*/
virtual void changeParent(std::shared_ptr<TreeItem> newParent);
virtual bool changeParent(std::shared_ptr<TreeItem> newParent);
/* @brief Retrieves a child of the current item
@param row is the index of the child to retrieve
......@@ -125,8 +126,8 @@ public:
*/
template <class T, class BinaryOperation> T accumulate(T init, BinaryOperation op);
/* @brief Returns true if the model has been notified about the existence of this object
*/
/* @brief Return true if the current item has the item with given id as an ancestor */
bool hasAncestor(int id);
protected:
/* @brief Finish construction of object given its pointer
......
......@@ -268,11 +268,11 @@ QString AbstractProjectItem::lastParentId() const
return m_lastParentId;
}
void AbstractProjectItem::changeParent(std::shared_ptr<TreeItem> newParent)
bool AbstractProjectItem::changeParent(std::shared_ptr<TreeItem> newParent)
{
m_lastParentId.clear();
if (newParent) {
m_lastParentId = std::static_pointer_cast<AbstractProjectItem>(newParent)->clipId();
}
TreeItem::changeParent(newParent);
return TreeItem::changeParent(newParent);
}
......@@ -181,7 +181,7 @@ public:
QString lastParentId() const;
/* @brief This is an overload of TreeItem::changeParent that tracks the id of the id of the parent */
void changeParent(std::shared_ptr<TreeItem> newParent) override;
bool changeParent(std::shared_ptr<TreeItem> newParent) override;
/* Returns a ptr to the enclosing dir, and nullptr if none is found.
@param strict if set to false, the enclosing dir of a dir is itself, otherwise we try to find a "true" parent
......
......@@ -491,8 +491,7 @@ Fun ProjectItemModel::requestRenameFolder_lambda(std::shared_ptr<AbstractProject
auto parent = currentFolder->parent();
parent->removeChild(currentFolder);
currentFolder->setName(newName);
parent->appendChild(currentFolder);
return true;
return parent->appendChild(currentFolder);
};
}
......@@ -532,8 +531,7 @@ Fun ProjectItemModel::addBin_lambda(std::shared_ptr<AbstractProjectItem> new_ite
return false;
}
}
new_item->changeParent(parent);
return true;
return new_item->changeParent(parent);
};
}
......
......@@ -18,6 +18,7 @@ TEST_CASE("Basic tree testing", "[TreeModel]")
{
auto model = AbstractTreeModel::construct();
REQUIRE(model->checkConsistency());
REQUIRE(model->rowCount() == 0);
SECTION("Item creation Test")
......@@ -25,18 +26,20 @@ TEST_CASE("Basic tree testing", "[TreeModel]")
auto item = TreeItem::construct(QList<QVariant>{QString("test")}, model, false);
int id = item->getId();
REQUIRE(item->depth() == 0);
REQUIRE(model->checkConsistency());
// check that a valid Id has been assigned
REQUIRE(id != -1);
// check that the item is not yet registered (not valid parent)
REQUIRE(model->m_allItems.size() == 0);
REQUIRE(model->m_allItems.size() == 1);
// Assign this to a parent
model->getRoot()->appendChild(item);
REQUIRE(model->checkConsistency());
// Now the item should be registered, we query it
REQUIRE(model->m_allItems.size() == 1);
REQUIRE(model->m_allItems.size() == 2);
REQUIRE(model->getItemById(id) == item);
REQUIRE(item->depth() == 1);
REQUIRE(model->rowCount() == 1);
......@@ -48,13 +51,143 @@ TEST_CASE("Basic tree testing", "[TreeModel]")
// Try joint creation / assignation
auto item2 = item->appendChild(QList<QVariant>{QString("test2")});
REQUIRE(item->depth() == 1);
REQUIRE(item2->depth() == 2);
REQUIRE(model->rowCount() == 1);
auto state = [&]() {
REQUIRE(model->checkConsistency());
REQUIRE(item->depth() == 1);
REQUIRE(item2->depth() == 2);
REQUIRE(model->rowCount() == 1);
REQUIRE(item->row() == 0);
REQUIRE(item2->row() == 0);
REQUIRE(model->data(model->getIndexFromItem(item2), 0) == QStringLiteral("test2"));
REQUIRE(model->rowCount(model->getIndexFromItem(item2)) == 0);
};
state();
REQUIRE(model->rowCount(model->getIndexFromItem(item)) == 1);
REQUIRE(model->rowCount(model->getIndexFromItem(item2)) == 0);
REQUIRE(model->m_allItems.size() == 2);
REQUIRE(model->data(model->getIndexFromItem(item2), 0) == QStringLiteral("test2"));
REQUIRE(model->m_allItems.size() == 3);
// Add a second child to item to check if everything collapses
auto item3 = item->appendChild(QList<QVariant>{QString("test3")});
state();
REQUIRE(model->rowCount(model->getIndexFromItem(item3)) == 0);
REQUIRE(model->rowCount(model->getIndexFromItem(item)) == 2);
REQUIRE(model->m_allItems.size() == 4);
REQUIRE(item3->depth() == 2);
REQUIRE(item3->row() == 1);
REQUIRE(model->data(model->getIndexFromItem(item3), 0) == QStringLiteral("test3"));
}
SECTION("Invalid moves")
{
auto item = model->getRoot()->appendChild(QList<QVariant>{QString("test")});
auto state = [&]() {
REQUIRE(model->checkConsistency());
REQUIRE(model->rowCount() == 1);
REQUIRE(item->depth() == 1);
REQUIRE(item->row() == 0);
REQUIRE(model->data(model->getIndexFromItem(item), 0) == QStringLiteral("test"));
};
state();
REQUIRE(model->rowCount(model->getIndexFromItem(item)) == 0);
// Try to move the root
REQUIRE_FALSE(item->appendChild(model->getRoot()));
state();
REQUIRE(model->rowCount(model->getIndexFromItem(item)) == 0);
auto item2 = item->appendChild(QList<QVariant>{QString("test2")});
auto item3 = item2->appendChild(QList<QVariant>{QString("test3")});
auto item4 = item3->appendChild(QList<QVariant>{QString("test4")});
auto state2 = [&]() {
state();
REQUIRE(item2->depth() == 2);
REQUIRE(item2->row() == 0);
REQUIRE(model->data(model->getIndexFromItem(item2), 0) == QStringLiteral("test2"));
REQUIRE(item3->depth() == 3);
REQUIRE(item3->row() == 0);
REQUIRE(model->data(model->getIndexFromItem(item3), 0) == QStringLiteral("test3"));
REQUIRE(item4->depth() == 4);
REQUIRE(item4->row() == 0);
REQUIRE(model->data(model->getIndexFromItem(item4), 0) == QStringLiteral("test4"));
};
state2();
// Try to make a loop
REQUIRE_FALSE(item->changeParent(item3));
state2();
REQUIRE_FALSE(item->changeParent(item4));
state2();
// Try to append a child that already have a parent
REQUIRE_FALSE(item->appendChild(item4));
state2();
// valid move
REQUIRE(item4->changeParent(item2));
REQUIRE(model->checkConsistency());
}
SECTION ("Deregistration tests") {
// we construct a non trivial structure
auto item = model->getRoot()->appendChild(QList<QVariant>{QString("test")});
auto item2 = item->appendChild(QList<QVariant>{QString("test2")});
auto item3 = item2->appendChild(QList<QVariant>{QString("test3")});
auto item4 = item3->appendChild(QList<QVariant>{QString("test4")});
auto item5 = item2->appendChild(QList<QVariant>{QString("test5")});
auto state = [&]() {
REQUIRE(model->checkConsistency());
REQUIRE(model->rowCount() == 1);
REQUIRE(item->depth() == 1);
REQUIRE(item->row() == 0);
REQUIRE(model->data(model->getIndexFromItem(item), 0) == QStringLiteral("test"));
REQUIRE(model->rowCount(model->getIndexFromItem(item)) == 1);
REQUIRE(item2->depth() == 2);
REQUIRE(item2->row() == 0);
REQUIRE(model->data(model->getIndexFromItem(item2), 0) == QStringLiteral("test2"));
REQUIRE(model->rowCount(model->getIndexFromItem(item2)) == 2);
REQUIRE(item3->depth() == 3);
REQUIRE(item3->row() == 0);
REQUIRE(model->data(model->getIndexFromItem(item3), 0) == QStringLiteral("test3"));
REQUIRE(model->rowCount(model->getIndexFromItem(item3)) == 1);
REQUIRE(item4->depth() == 4);
REQUIRE(item4->row() == 0);
REQUIRE(model->data(model->getIndexFromItem(item4), 0) == QStringLiteral("test4"));
REQUIRE(model->rowCount(model->getIndexFromItem(item4)) == 0);
REQUIRE(item5->depth() == 3);
REQUIRE(item5->row() == 1);
REQUIRE(model->data(model->getIndexFromItem(item5), 0) == QStringLiteral("test5"));
REQUIRE(model->rowCount(model->getIndexFromItem(item5)) == 0);
REQUIRE(model->m_allItems.size() == 6);
REQUIRE(item->isInModel());
REQUIRE(item2->isInModel());
REQUIRE(item3->isInModel());
REQUIRE(item4->isInModel());
REQUIRE(item5->isInModel());
};
state();
// deregister the topmost item, should also deregister its children
item->changeParent(std::shared_ptr<TreeItem>());
REQUIRE(model->m_allItems.size() == 1);
REQUIRE(model->rowCount() == 0);
REQUIRE(!item->isInModel());
REQUIRE(!item2->isInModel());
REQUIRE(!item3->isInModel());
REQUIRE(!item4->isInModel());
REQUIRE(!item5->isInModel());
// reinsert
REQUIRE(model->getRoot()->appendChild(item));
state();
item2->removeChild(item5);
REQUIRE(!item5->isInModel());
REQUIRE(model->rowCount(model->getIndexFromItem(item2)) == 1);
REQUIRE(model->m_allItems.size() == 5);
// reinsert
REQUIRE(item5->changeParent(item2));
state();
}
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment