From c32bd844573c3fed2c83225456dfcf0b61eb71c1 Mon Sep 17 00:00:00 2001
From: Tobias Bodmer <Tobias.Bodmer@gmx.de>
Date: Mon, 7 Nov 2022 17:04:49 +0100
Subject: [PATCH] Unique Dict keys, separate edit properties

AronTreeWidgetItem now stores two flags, which column should be
editable. This is necessary, because the parent entry could enable
editing column 0 whilst the current elment can be edited in its value.
Only using the flag of QTreeElement is then too messy.

Also added a parent pointer to the CreatorVisitor. This provides enough
context for a centralized instantiation.

As pointed out: Newly added keys are now unique.
---
 .../SkillManagerMonitorWidgetController.cpp   |  15 +--
 .../aronTreeWidget/AronTreeWidgetItem.cpp     |   5 +
 .../aronTreeWidget/AronTreeWidgetItem.h       |   6 +-
 .../visitors/AronTreeWidgetContextMenu.cpp    |   6 +-
 .../visitors/AronTreeWidgetCreator.cpp        | 120 +++++++++++++-----
 .../visitors/AronTreeWidgetCreator.h          |  18 ++-
 .../visitors/AronTreeWidgetSetter.cpp         |   5 +-
 .../visitors/AronTreeWidgetSetter.h           |   2 +-
 8 files changed, 121 insertions(+), 56 deletions(-)

diff --git a/source/RobotAPI/gui-plugins/SkillManagerPlugin/SkillManagerMonitorWidgetController.cpp b/source/RobotAPI/gui-plugins/SkillManagerPlugin/SkillManagerMonitorWidgetController.cpp
index c19c01fc5..ef6b252f1 100644
--- a/source/RobotAPI/gui-plugins/SkillManagerPlugin/SkillManagerMonitorWidgetController.cpp
+++ b/source/RobotAPI/gui-plugins/SkillManagerPlugin/SkillManagerMonitorWidgetController.cpp
@@ -411,13 +411,13 @@ namespace armarx
             return;
         }
         auto * aronItem = AronTreeWidgetItem::DynamicCast(item);
-
         if (column == 1 && aronItem)
         {
-            if (item->flags() & Qt::ItemIsEditable) // we use the flag to indicate whether the item is editable or not
+            if (aronItem->col1Editable)
             {
 
-                std::string name = aronItem->text(aron_tree_widget::constantes::TREE_WIDGET_ITEM_NAME).toStdString();
+                std::string name = aronItem->text(
+                            aron_tree_widget::constantes::TREE_WIDGET_ITEM_NAME).toStdString();
                 // depending on aron type, create extra gui element.
                 AronTreeWidgetModalCreatorVisitor v(name, aronItem, widget.treeWidgetSkillDetails);
                 aron::type::visit(v, aronItem->aronType);
@@ -433,13 +433,8 @@ namespace armarx
                 }
             }
         }
-        else if (column == 0 && aronItem){
-            // only allow to edit dict keys
-            auto* aronParent = AronTreeWidgetItem::DynamicCast(aronItem->QTreeWidgetItem::parent());
-
-            if (aronParent && aronParent->aronType->getDescriptor() == aron::type::Descriptor::DICT){
-                aronItem->treeWidget()->editItem(item, column);
-            }
+        else if (column == 0 && aronItem && aronItem->col0Editable){
+                item->treeWidget()->editItem(item, column);
         }
     }
 }
diff --git a/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/AronTreeWidgetItem.cpp b/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/AronTreeWidgetItem.cpp
index 19be10dc6..4e591ef6f 100644
--- a/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/AronTreeWidgetItem.cpp
+++ b/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/AronTreeWidgetItem.cpp
@@ -34,5 +34,10 @@ namespace armarx
         ARMARX_CHECK_NOT_NULL(c);
         return c;
     }
+    AronTreeWidgetItem::AronTreeWidgetItem(bool editKey, bool editVal)
+    {
+        col0Editable = editKey;
+        col1Editable = editVal;
+    }
 
 }
diff --git a/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/AronTreeWidgetItem.h b/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/AronTreeWidgetItem.h
index eb1d6c140..01be1b858 100644
--- a/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/AronTreeWidgetItem.h
+++ b/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/AronTreeWidgetItem.h
@@ -19,6 +19,7 @@ namespace armarx
         Q_OBJECT
     public:
         AronTreeWidgetItem(const AronTreeWidgetItem& other);
+        AronTreeWidgetItem(bool editKey, bool editVal);
 
         using QTreeWidgetItem::QTreeWidgetItem;
 
@@ -29,6 +30,9 @@ namespace armarx
         static AronTreeWidgetItem* DynamicCastAndCheck(QTreeWidgetItem*);
 
         aron::type::VariantPtr aronType;
-
+        // if editing the first column should be allowed
+        bool col0Editable = false;
+        // if editing the second column should be allowed
+        bool col1Editable = false;
     };
 }
diff --git a/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/visitors/AronTreeWidgetContextMenu.cpp b/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/visitors/AronTreeWidgetContextMenu.cpp
index a894b9bbe..38a696103 100644
--- a/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/visitors/AronTreeWidgetContextMenu.cpp
+++ b/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/visitors/AronTreeWidgetContextMenu.cpp
@@ -63,16 +63,14 @@ void armarx::AronTreeWidgetContextMenuVisitor::addAddAction()
 
 void AronTreeWidgetContextMenuVisitor::executeAddElement()
 {
-    AronTreeWidgetCreatorVisitor creator;
+    AronTreeWidgetCreatorVisitor creator(parentItem);
     aron::type::visit(creator, parentItem->aronType->getChildren()[0]);
 
     // TODO: this creator should check if it is contained in a list / dict -> set key val properly
     if(!creator.createdQWidgetItem){
         throw std::runtime_error("Creation of TreeElementChild failed unexpectedly");
     }
-    // add column 0 name to newly created item (this gets called from list and dict here)
-    std::string childNr = std::to_string(parentItem->childCount());
-    creator.createdQWidgetItem->setText(0, childNr.c_str());
+
     parentItem->addChild(creator.createdQWidgetItem);
 }
 
diff --git a/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/visitors/AronTreeWidgetCreator.cpp b/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/visitors/AronTreeWidgetCreator.cpp
index 8e18b39d5..50074b873 100644
--- a/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/visitors/AronTreeWidgetCreator.cpp
+++ b/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/visitors/AronTreeWidgetCreator.cpp
@@ -31,20 +31,58 @@
 
 namespace armarx
 {
-    void
-    AronTreeWidgetCreatorVisitor::createSimpleTreeViewWidget(Input& i, const std::string& defaul, bool isEditable)
+
+AronTreeWidgetCreatorVisitor::AronTreeWidgetCreatorVisitor(QTreeWidgetItem *parentInstance) : parentOfCreatedObj(parentInstance)
+{
+}
+
+std::string AronTreeWidgetCreatorVisitor::generateUniqueKeyFromSet(std::set<std::string>&& usedKeys){
+    size_t num = 0;
+    while(true){
+        std::string proposedName = this->defaultMapKeyName + std::to_string(num);
+        auto it = usedKeys.find(proposedName);
+        if(it == usedKeys.end()) {
+            break;
+        }
+        ++num;
+    }
+    return this->defaultMapKeyName + std::to_string(num);
+}
+
+void
+    AronTreeWidgetCreatorVisitor::createSimpleTreeViewWidget(Input& i, const std::string& defaul)
     {
         ARMARX_CHECK_NOT_NULL(i);
 
         auto key = i->getPath().getLastElement();
-        createdQWidgetItem = new AronTreeWidgetItem();
+        bool isDictChild = false;
+        // edit key, to be a unique string, if the parent is a dict
+        auto* aronParent = AronTreeWidgetItem::DynamicCast(parentOfCreatedObj);
+        if(aronParent && aronParent->aronType->getDescriptor() == aron::type::Descriptor::DICT){
+            isDictChild = true;
+            std::set<std::string> usedKeys;
+            for(int i = 0; i < parentOfCreatedObj->childCount(); ++i){
+                auto* sibling = AronTreeWidgetItem::DynamicCast(parentOfCreatedObj->child(i));
+                if(sibling) {
+                    usedKeys.insert(sibling->text(0).toStdString());
+                }
+            }
+            key = generateUniqueKeyFromSet(std::move(usedKeys));
+        }
+        // if it's a list -> choose the right number
+        else if(aronParent && aronParent->aronType->getDescriptor() == aron::type::Descriptor::LIST)
+        {
+            key = std::to_string(parentOfCreatedObj->childCount());
+        }
+
+        createdQWidgetItem = new AronTreeWidgetItem(isDictChild, editableValue);
         createdQWidgetItem->aronType = i;
         createdQWidgetItem->setText(0, QString::fromStdString(key));
         createdQWidgetItem->setText(1, QString::fromStdString(defaul));
         createdQWidgetItem->setText(2, QString::fromStdString(i->getShortName()));
         createdQWidgetItem->setText(3, QString::fromStdString(aron_tree_widget::constantes::ITEM_EMPTY_MESSAGE) /*QString::fromStdString(i->getDefaultFromString())*/);
-        if (isEditable)
-        {
+
+        if(editableValue || isDictChild){
             createdQWidgetItem->setFlags(createdQWidgetItem->flags() | Qt::ItemIsEditable);
         }
     }
@@ -66,7 +104,7 @@ namespace armarx
 
         for (const auto& [key, value] : i->getMemberTypes())
         {
-            AronTreeWidgetCreatorVisitor v;
+            AronTreeWidgetCreatorVisitor v(createdQWidgetItem);
             aron::type::visit(v, value);
 
             if (v.createdQWidgetItem)
@@ -81,34 +119,36 @@ namespace armarx
     {
         ARMARX_CHECK_NOT_NULL(i);
 
-        auto key = i->getShortName();
-        if (i->getPath().withDetachedLastElement().hasElement())
-        {
-            key = i->getPath().withDetachedLastElement().getLastElement();
-        }
-
-        createdQWidgetItem = new AronTreeWidgetItem();
-        createdQWidgetItem->aronType = i;
-        createdQWidgetItem->setText(2, key.c_str());
-
+        createSimpleTreeViewWidget(i, "");
         // The DictType has only one member, its key-type. This also must be present
         ARMARX_CHECK_EQUAL(i->getChildren().size(),1);
     }
     void
     AronTreeWidgetCreatorVisitor::visitAronVariant(const aron::type::PairPtr& i)
-    { createSimpleTreeViewWidget(i, ""); }
+    {
+        editableValue = true;
+        createSimpleTreeViewWidget(i, "");
+    }
     void
     AronTreeWidgetCreatorVisitor::visitAronVariant(const aron::type::TuplePtr& i)
-    { createSimpleTreeViewWidget(i, ""); }
+    {
+        editableValue = true;
+        createSimpleTreeViewWidget(i, "");
+    }
     void
     AronTreeWidgetCreatorVisitor::visitAronVariant(const aron::type::ListPtr& i)
-    { createSimpleTreeViewWidget(i, "", false); }
+    {
+        createSimpleTreeViewWidget(i, "");
+    }
     void
     AronTreeWidgetCreatorVisitor::visitAronVariant(const aron::type::NDArrayPtr& i)
     { createSimpleTreeViewWidget(i, ""); }
     void
     AronTreeWidgetCreatorVisitor::visitAronVariant(const aron::type::MatrixPtr& i)
-    { createSimpleTreeViewWidget(i, ""); }
+    {
+        editableValue = true;
+        createSimpleTreeViewWidget(i, "");
+    }
     void
     AronTreeWidgetCreatorVisitor::visitAronVariant(const aron::type::QuaternionPtr& i)
     { createSimpleTreeViewWidget(i, ""); }
@@ -121,38 +161,48 @@ namespace armarx
     void
     AronTreeWidgetCreatorVisitor::visitAronVariant(const aron::type::IntEnumPtr& i)
     {
+        editableValue = true;
         ARMARX_CHECK_NOT_NULL(i);
         ARMARX_CHECK_GREATER(i->getAcceptedValueNames().size(), 0);
 
-        auto key = i->getPath().getLastElement();
-        createdQWidgetItem = new AronTreeWidgetItem();
-        createdQWidgetItem->aronType = i;
-        createdQWidgetItem->setText(0, "");
-        createdQWidgetItem->setText(1, QString::fromStdString(i->getAcceptedValueNames()[0]));
-        createdQWidgetItem->setText(2, QString::fromStdString(i->getShortName()));
-        createdQWidgetItem->setText(3, QString::fromStdString(aron_tree_widget::constantes::ITEM_EMPTY_MESSAGE));
-
-        createdQWidgetItem->setFlags(createdQWidgetItem->flags() | Qt::ItemIsEditable);
+        createSimpleTreeViewWidget(i, i->getAcceptedValueNames()[0]);
 
     }
     void
     AronTreeWidgetCreatorVisitor::visitAronVariant(const aron::type::IntPtr& i)
-    { createSimpleTreeViewWidget(i, "0"); }
+    {
+        editableValue = true;
+        createSimpleTreeViewWidget(i, "0");
+    }
     void
     AronTreeWidgetCreatorVisitor::visitAronVariant(const aron::type::LongPtr& i)
-    { createSimpleTreeViewWidget(i, "0"); }
+    {
+        editableValue = true;
+        createSimpleTreeViewWidget(i, "0");
+    }
     void
     AronTreeWidgetCreatorVisitor::visitAronVariant(const aron::type::FloatPtr& i)
-    { createSimpleTreeViewWidget(i, "0.0"); }
+    {
+        editableValue = true;
+        createSimpleTreeViewWidget(i, "0.0");
+    }
     void
     AronTreeWidgetCreatorVisitor::visitAronVariant(const aron::type::DoublePtr& i)
-    { createSimpleTreeViewWidget(i, "0.0"); }
+    {
+        editableValue = true;
+        createSimpleTreeViewWidget(i, "0.0");
+    }
     void
     AronTreeWidgetCreatorVisitor::visitAronVariant(const aron::type::BoolPtr& i)
-    { createSimpleTreeViewWidget(i, "false"); }
+    {
+        editableValue = true;
+        createSimpleTreeViewWidget(i, "false");
+    }
     void
     AronTreeWidgetCreatorVisitor::visitAronVariant(const aron::type::StringPtr& i)
-    { createSimpleTreeViewWidget(i, ""); }
+    {
+        createSimpleTreeViewWidget(i, "");
+    }
 
     void AronTreeWidgetCreatorVisitor::visitUnknown(Input&)
     {
diff --git a/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/visitors/AronTreeWidgetCreator.h b/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/visitors/AronTreeWidgetCreator.h
index 86c207549..b8d9c48a5 100644
--- a/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/visitors/AronTreeWidgetCreator.h
+++ b/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/visitors/AronTreeWidgetCreator.h
@@ -24,6 +24,7 @@
 #include <RobotAPI/libraries/aron/core/type/variant/All.h>
 #include <RobotAPI/libraries/aron/core/data/variant/All.h>
 #include <RobotAPI/libraries/aron/core/type/visitor/variant/VariantVisitor.h>
+#include <QTreeWidgetItem>
 
 namespace armarx
 {
@@ -34,11 +35,16 @@ namespace armarx
             public armarx::aron::type::ConstVariantVisitor
     {
     public:
-        AronTreeWidgetItem* createdQWidgetItem;
+        AronTreeWidgetItem* createdQWidgetItem = nullptr;
 
         AronTreeWidgetCreatorVisitor() = default;
+        // constructor wich also takes the dedicated parent
+        // this will NOT be uesd to attach the child, rather to initialize the new instance better.
+        // usages: find out if dict / list child, assign unique key in dict
+        AronTreeWidgetCreatorVisitor(QTreeWidgetItem* parentInstance);
+
+        void createSimpleTreeViewWidget(Input& i, const std::string&);
 
-        void createSimpleTreeViewWidget(Input& i, const std::string&, bool isEditable = true);
 
         void visitAronVariant(const aron::type::ObjectPtr&) final;
         void visitAronVariant(const aron::type::DictPtr& i) final;
@@ -58,6 +64,14 @@ namespace armarx
         void visitAronVariant(const aron::type::BoolPtr& i) final;
         void visitAronVariant(const aron::type::StringPtr& i) final;
         void visitUnknown(Input&) final;
+
+    private:
+        void handleEditable();
+        std::string generateUniqueKeyFromSet(std::set<std::string>&& usedKeys);
+        QTreeWidgetItem* parentOfCreatedObj = nullptr;
+        const std::string defaultMapKeyName = "key_";
+        // controls, if values (column 1) can be edited directly
+        bool editableValue = false;
     };
 }
 
diff --git a/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/visitors/AronTreeWidgetSetter.cpp b/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/visitors/AronTreeWidgetSetter.cpp
index a4a730171..e3e2cc0a1 100644
--- a/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/visitors/AronTreeWidgetSetter.cpp
+++ b/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/visitors/AronTreeWidgetSetter.cpp
@@ -54,12 +54,11 @@ namespace armarx
         if (((size_t) parent->childCount()) < numChildren){
             // The type to create must be the only child of the current aron type
             ARMARX_CHECK_EQUAL(parent->aronType->childrenSize(),1);
-            size_t childrenToAdd= numChildren - parent->childCount();
+            size_t childrenToAdd = numChildren - parent->childCount();
             for(size_t j = 0; j < childrenToAdd; ++j){
-                AronTreeWidgetCreatorVisitor childCreator;
+                AronTreeWidgetCreatorVisitor childCreator(parent);
                 aron::type::visit(childCreator,parent->aronType->getChildren()[0]);
                 ARMARX_CHECK_NOT_NULL(childCreator.createdQWidgetItem);
-
                 parent->addChild(childCreator.createdQWidgetItem);
             }
         }
diff --git a/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/visitors/AronTreeWidgetSetter.h b/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/visitors/AronTreeWidgetSetter.h
index d2a917f79..46bafd878 100644
--- a/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/visitors/AronTreeWidgetSetter.h
+++ b/source/RobotAPI/gui-plugins/SkillManagerPlugin/aronTreeWidget/visitors/AronTreeWidgetSetter.h
@@ -31,7 +31,7 @@
 
 namespace armarx
 {
-    // Conversion from TreeView to aron data
+    // Conversion from aron data to Tree View
     class AronTreeWidgetSetterVisitor :
             public armarx::aron::data::ConstVariantVisitor
     {
-- 
GitLab