From e3d4bcef3ddfd73038b9573c0e80da20c99f6ea4 Mon Sep 17 00:00:00 2001
From: Pieter Hijma <pieterhijma@users.noreply.github.com>
Date: Thu, 25 Apr 2024 21:09:46 +0200
Subject: [PATCH] Gui: Rearrange the property view context menu

---
 src/Gui/propertyeditor/PropertyEditor.cpp | 111 +++++++++++++---------
 1 file changed, 65 insertions(+), 46 deletions(-)

diff --git a/src/Gui/propertyeditor/PropertyEditor.cpp b/src/Gui/propertyeditor/PropertyEditor.cpp
index f9dc427a47..a4fb1105ac 100644
--- a/src/Gui/propertyeditor/PropertyEditor.cpp
+++ b/src/Gui/propertyeditor/PropertyEditor.cpp
@@ -651,18 +651,11 @@ enum MenuAction {
 
 void PropertyEditor::contextMenuEvent(QContextMenuEvent *) {
     QMenu menu;
-    QAction *autoExpand = menu.addAction(tr("Auto expand"));
-    autoExpand->setCheckable(true);
-    autoExpand->setChecked(autoexpand);
-    autoExpand->setData(QVariant(MA_AutoExpand));
-
-    QAction *showAll = menu.addAction(tr("Show all"));
-    showAll->setCheckable(true);
-    showAll->setChecked(PropertyView::showAll());
-    showAll->setData(QVariant(MA_ShowAll));
+    QAction *autoExpand = nullptr;
 
     auto contextIndex = currentIndex();
 
+    // acquiring the selected properties
     std::unordered_set<App::Property*> props;
     const auto indexes = selectedIndexes();
     for(const auto& index : indexes) {
@@ -678,49 +671,70 @@ void PropertyEditor::contextMenuEvent(QContextMenuEvent *) {
         }
     }
 
-    if(props.size() == 1) {
-        auto item = static_cast<PropertyItem*>(contextIndex.internalPointer());
-        auto prop = *props.begin();
-        if(item->isBound() 
-            && !prop->isDerivedFrom(App::PropertyExpressionEngine::getClassTypeId())
-            && !prop->isReadOnly() 
-            && !prop->testStatus(App::Property::Immutable)
-            && !(prop->getType() & App::Prop_ReadOnly))
+    // add property
+    if(!props.empty()) {
+        menu.addAction(tr("Add property"))->setData(QVariant(MA_AddProp));
+        if (std::all_of(props.begin(), props.end(), [](auto prop) {
+            return prop->testStatus(App::Property::PropDynamic)
+                && !boost::starts_with(prop->getName(),prop->getGroup());
+        }))
         {
-            contextIndex = propertyModel->buddy(contextIndex);
-            setCurrentIndex(contextIndex);
-            menu.addSeparator();
-            menu.addAction(tr("Expression..."))->setData(QVariant(MA_Expression));
+            menu.addAction(tr("Rename property group"))->setData(QVariant(MA_EditPropGroup));
         }
     }
 
+    // remove property
+    bool canRemove = !props.empty();
+    unsigned long propType = 0;
+    unsigned long propStatus = 0xffffffff;
+    for(auto prop : props) {
+        propType |= prop->getType();
+        propStatus &= prop->getStatus();
+        if(!prop->testStatus(App::Property::PropDynamic)
+           || prop->testStatus(App::Property::LockDynamic))
+        {
+            canRemove = false;
+        }
+    }
+    if(canRemove)
+        menu.addAction(tr("Remove property"))->setData(QVariant(MA_RemoveProp));
+
+    // add a separator between adding/removing properties and the rest
+    if (!props.empty() || canRemove) {
+        menu.addSeparator();
+    }
+
+    // show all
+    QAction *showAll = menu.addAction(tr("Show all"));
+    showAll->setCheckable(true);
+    showAll->setChecked(PropertyView::showAll());
+    showAll->setData(QVariant(MA_ShowAll));
+
     if(PropertyView::showAll()) {
-        if(!props.empty()) {
-            menu.addAction(tr("Add property"))->setData(QVariant(MA_AddProp));
-            if (std::all_of(props.begin(), props.end(), [](auto prop) {
-                    return prop->testStatus(App::Property::PropDynamic)
-                        && !boost::starts_with(prop->getName(),prop->getGroup());
-               }))
+        // auto expand
+        autoExpand = menu.addAction(tr("Auto expand"));
+        autoExpand->setCheckable(true);
+        autoExpand->setChecked(autoexpand);
+        autoExpand->setData(QVariant(MA_AutoExpand));
+
+        // expression
+        if(props.size() == 1) {
+            auto item = static_cast<PropertyItem*>(contextIndex.internalPointer());
+            auto prop = *props.begin();
+            if(item->isBound() 
+               && !prop->isDerivedFrom(App::PropertyExpressionEngine::getClassTypeId())
+               && !prop->isReadOnly() 
+               && !prop->testStatus(App::Property::Immutable)
+               && !(prop->getType() & App::Prop_ReadOnly))
             {
-                menu.addAction(tr("Rename property group"))->setData(QVariant(MA_EditPropGroup));
+                contextIndex = propertyModel->buddy(contextIndex);
+                setCurrentIndex(contextIndex);
+                // menu.addSeparator();
+                menu.addAction(tr("Expression..."))->setData(QVariant(MA_Expression));
             }
         }
 
-        bool canRemove = !props.empty();
-        unsigned long propType = 0;
-        unsigned long propStatus = 0xffffffff;
-        for(auto prop : props) {
-            propType |= prop->getType();
-            propStatus &= prop->getStatus();
-            if(!prop->testStatus(App::Property::PropDynamic)
-                || prop->testStatus(App::Property::LockDynamic))
-            {
-                canRemove = false;
-            }
-        }
-        if(canRemove)
-            menu.addAction(tr("Remove property"))->setData(QVariant(MA_RemoveProp));
-
+        // the various flags
         if(!props.empty()) {
             menu.addSeparator();
 
@@ -759,9 +773,14 @@ void PropertyEditor::contextMenuEvent(QContextMenuEvent *) {
 
     switch(action->data().toInt()) {
     case MA_AutoExpand:
-        autoexpand = autoExpand->isChecked();
-        if (autoexpand)
-            expandAll();
+        if (autoExpand) {
+            // Variable autoExpand should not be null when we arrive here, but
+            // since we explicitly initialize the variable to nullptr, a check
+            // nonetheless.
+            autoexpand = autoExpand->isChecked();
+            if (autoexpand)
+                expandAll();
+        }
         return;
     case MA_ShowAll:
         PropertyView::setShowAll(action->isChecked());