From f12ae8a13c91d1e1597c7fc52f13d8eb103ba3ff Mon Sep 17 00:00:00 2001
From: Zheng Lei <realthunder.dev@gmail.com>
Date: Mon, 21 Feb 2022 19:26:21 +0800
Subject: [PATCH] Gui: improve PropertyEditor refresh (#3535)

* Gui: fix PropertyView 'Add property' action

* Gui: fix property view font color for linked property

* Gui: improve PropertyEditor refresh

* Gui: show real property name in property view tool tip

* Gui: improve property view tool tip

* Gui: fix auto recompute in property view

* Gui: remove duplicated PropertyModel signal of dataChanged()

* Gui: fix property view update on property change
Including changes in document properties

* Gui: fix transaction closing on property editor change
On editing row removal and on model reset.

* Gui: fix property view auto expansion of previous selected item

* Gui: improve property editor navigation using tab/shift+tab
---
 src/Gui/DlgAddProperty.cpp                    |   5 +-
 src/Gui/PropertyView.cpp                      |  83 ++--
 src/Gui/PropertyView.h                        |   3 +-
 src/Gui/ViewParams.h                          |   1 +
 src/Gui/propertyeditor/PropertyEditor.cpp     | 279 +++++++----
 src/Gui/propertyeditor/PropertyEditor.h       |  17 +-
 src/Gui/propertyeditor/PropertyItem.cpp       |  49 +-
 src/Gui/propertyeditor/PropertyItem.h         |  19 +-
 .../propertyeditor/PropertyItemDelegate.cpp   |  29 +-
 src/Gui/propertyeditor/PropertyItemDelegate.h |   1 -
 src/Gui/propertyeditor/PropertyModel.cpp      | 438 +++++++++++-------
 src/Gui/propertyeditor/PropertyModel.h        |  13 +-
 12 files changed, 603 insertions(+), 334 deletions(-)

diff --git a/src/Gui/DlgAddProperty.cpp b/src/Gui/DlgAddProperty.cpp
index 9b6ccd21d6..a8e5c97730 100644
--- a/src/Gui/DlgAddProperty.cpp
+++ b/src/Gui/DlgAddProperty.cpp
@@ -107,7 +107,8 @@ void DlgAddProperty::accept()
         name = group + "_" + name;
 
     for(auto c : containers) {
-        if(c->getPropertyByName(name.c_str())) {
+        auto prop = c->getPropertyByName(name.c_str());
+        if(prop && prop->getContainer() == c) {
             QMessageBox::critical(getMainWindow(),
                 QObject::tr("Invalid name"),
                 QObject::tr("The property '%1' already exists in '%2'").arg(
@@ -127,7 +128,7 @@ void DlgAddProperty::accept()
             e.ReportException();
             for(auto it2=containers.begin();it2!=it;++it2) {
                 try {
-                    (*it)->removeDynamicProperty(name.c_str());
+                    (*it2)->removeDynamicProperty(name.c_str());
                 } catch(Base::Exception &e) {
                     e.ReportException();
                 }
diff --git a/src/Gui/PropertyView.cpp b/src/Gui/PropertyView.cpp
index 3e8ce61fb2..e12f2bdbc7 100644
--- a/src/Gui/PropertyView.cpp
+++ b/src/Gui/PropertyView.cpp
@@ -111,7 +111,7 @@ PropertyView::PropertyView(QWidget *parent)
 
     this->connectPropData =
     App::GetApplication().signalChangedObject.connect(boost::bind
-        (&PropertyView::slotChangePropertyData, this, bp::_1, bp::_2));
+        (&PropertyView::slotChangePropertyData, this, bp::_2));
     this->connectPropView =
     Gui::Application::Instance->signalChangedObject.connect(boost::bind
         (&PropertyView::slotChangePropertyView, this, bp::_1, bp::_2));
@@ -142,6 +142,8 @@ PropertyView::PropertyView(QWidget *parent)
     this->connectDelObject =
         App::GetApplication().signalDeletedObject.connect(
                 boost::bind(&PropertyView::slotDeletedObject, this, bp::_1));
+    this->connectChangedDocument = App::GetApplication().signalChangedDocument.connect(
+            boost::bind(&PropertyView::slotChangePropertyData, this, bp::_2));
 }
 
 PropertyView::~PropertyView()
@@ -157,6 +159,7 @@ PropertyView::~PropertyView()
     this->connectDelDocument.disconnect();
     this->connectDelObject.disconnect();
     this->connectDelViewObject.disconnect();
+    this->connectChangedDocument.disconnect();
 }
 
 static bool _ShowAll;
@@ -169,8 +172,11 @@ void PropertyView::setShowAll(bool enable) {
     if(_ShowAll != enable) {
         _ShowAll = enable;
         for(auto view : getMainWindow()->findChildren<PropertyView*>()) {
-            if(view->isVisible())
+            if(view->isVisible()) {
+                view->propertyEditorData->buildUp();
+                view->propertyEditorView->buildUp();
                 view->onTimer();
+            }
         }
     }
 }
@@ -187,7 +193,7 @@ void PropertyView::hideEvent(QHideEvent *ev) {
 
 void PropertyView::showEvent(QShowEvent *ev) {
     this->attachSelection();
-    this->timer->start(100);
+    this->timer->start(ViewParams::instance()->getPropertyViewTimer());
     QWidget::showEvent(ev);
 }
 
@@ -209,14 +215,20 @@ void PropertyView::slotRollback() {
     clearPropertyItemSelection();
 }
 
-void PropertyView::slotChangePropertyData(const App::DocumentObject&, const App::Property& prop)
+void PropertyView::slotChangePropertyData(const App::Property& prop)
 {
-    propertyEditorData->updateProperty(prop);
+    if (propertyEditorData->propOwners.count(prop.getContainer())) {
+        propertyEditorData->updateProperty(prop);
+        timer->start(ViewParams::instance()->getPropertyViewTimer());
+    }
 }
 
 void PropertyView::slotChangePropertyView(const Gui::ViewProvider&, const App::Property& prop)
 {
-    propertyEditorView->updateProperty(prop);
+    if (propertyEditorView->propOwners.count(prop.getContainer())) {
+        propertyEditorView->updateProperty(prop);
+        timer->start(ViewParams::instance()->getPropertyViewTimer());
+    }
 }
 
 bool PropertyView::isPropertyHidden(const App::Property *prop) {
@@ -229,10 +241,11 @@ void PropertyView::slotAppendDynamicProperty(const App::Property& prop)
     if (isPropertyHidden(&prop))
         return;
 
-    if (propertyEditorData->appendProperty(prop)
-            || propertyEditorView->appendProperty(prop))
+    App::PropertyContainer* parent = prop.getContainer();
+    if (propertyEditorData->propOwners.count(parent)
+            || propertyEditorView->propOwners.count(parent))
     {
-        timer->start(100);
+        timer->start(ViewParams::instance()->getPropertyViewTimer());
     }
 }
 
@@ -243,34 +256,17 @@ void PropertyView::slotRemoveDynamicProperty(const App::Property& prop)
         propertyEditorData->removeProperty(prop);
     else if(propertyEditorView->propOwners.count(parent))
         propertyEditorView->removeProperty(prop);
+    else
+        return;
+    timer->start(ViewParams::instance()->getPropertyViewTimer());
 }
 
 void PropertyView::slotChangePropertyEditor(const App::Document &, const App::Property& prop)
 {
     App::PropertyContainer* parent = prop.getContainer();
-    Gui::PropertyEditor::PropertyEditor* editor = nullptr;
-
-    if (parent && propertyEditorData->propOwners.count(parent))
-        editor = propertyEditorData;
-    else if (parent && propertyEditorView->propOwners.count(parent))
-        editor = propertyEditorView;
-    else
-        return;
-
-    if(showAll() || isPropertyHidden(&prop)) {
-        editor->updateEditorMode(prop);
-        return;
-    }
-    for(auto &v : editor->propList) {
-        for(auto p : v.second)
-            if(p == &prop) {
-                editor->updateEditorMode(prop);
-                return;
-            }
-    }
-    // The property is not in the list, probably because it is hidden before.
-    // So perform a full update.
-    timer->start(50);
+    if (propertyEditorData->propOwners.count(parent)
+            || propertyEditorView->propOwners.count(parent))
+        timer->start(ViewParams::instance()->getPropertyViewTimer());
 }
 
 void PropertyView::slotDeleteDocument(const Gui::Document &doc) {
@@ -278,7 +274,7 @@ void PropertyView::slotDeleteDocument(const Gui::Document &doc) {
         propertyEditorView->buildUp();
         propertyEditorData->buildUp();
         clearPropertyItemSelection();
-        timer->start(50);
+        timer->start(ViewParams::instance()->getPropertyViewTimer());
     }
 }
 
@@ -287,7 +283,7 @@ void PropertyView::slotDeletedViewObject(const Gui::ViewProvider &vp) {
         propertyEditorView->buildUp();
         propertyEditorData->buildUp();
         clearPropertyItemSelection();
-        timer->start(50);
+        timer->start(ViewParams::instance()->getPropertyViewTimer());
     }
 }
 
@@ -296,7 +292,7 @@ void PropertyView::slotDeletedObject(const App::DocumentObject &obj) {
         propertyEditorView->buildUp();
         propertyEditorData->buildUp();
         clearPropertyItemSelection();
-        timer->start(50);
+        timer->start(ViewParams::instance()->getPropertyViewTimer());
     }
 }
 
@@ -341,23 +337,28 @@ void PropertyView::onSelectionChanged(const SelectionChanges& msg)
         return;
 
     // clear the properties.
-    timer->start(50);
+    timer->start(ViewParams::instance()->getPropertyViewTimer());
 }
 
 void PropertyView::onTimer() {
 
-    propertyEditorData->buildUp();
-    propertyEditorView->buildUp();
-    clearPropertyItemSelection();
     timer->stop();
 
-    if(!this->isSelectionAttached())
+    if(!this->isSelectionAttached()) {
+        propertyEditorData->buildUp();
+        propertyEditorView->buildUp();
+        clearPropertyItemSelection();
         return;
+    }
 
     if(!Gui::Selection().hasSelection()) {
         auto gdoc = TreeWidget::selectedDocument();
-        if(!gdoc || !gdoc->getDocument())
+        if(!gdoc || !gdoc->getDocument()) {
+            propertyEditorData->buildUp();
+            propertyEditorView->buildUp();
+            clearPropertyItemSelection();
             return;
+        }
 
         PropertyModel::PropertyList docProps;
 
diff --git a/src/Gui/PropertyView.h b/src/Gui/PropertyView.h
index 9d052ff76a..c020655ade 100644
--- a/src/Gui/PropertyView.h
+++ b/src/Gui/PropertyView.h
@@ -81,7 +81,7 @@ protected:
 
 private:
     void onSelectionChanged(const SelectionChanges& msg) override;
-    void slotChangePropertyData(const App::DocumentObject&, const App::Property&);
+    void slotChangePropertyData(const App::Property&);
     void slotChangePropertyView(const Gui::ViewProvider&, const App::Property&);
     void slotAppendDynamicProperty(const App::Property&);
     void slotRemoveDynamicProperty(const App::Property&);
@@ -109,6 +109,7 @@ private:
     Connection connectDelDocument;
     Connection connectDelObject;
     Connection connectDelViewObject;
+    Connection connectChangedDocument;
     QTabWidget* tabs;
     QTimer* timer;
 };
diff --git a/src/Gui/ViewParams.h b/src/Gui/ViewParams.h
index 6dc897cff0..ff55628400 100644
--- a/src/Gui/ViewParams.h
+++ b/src/Gui/ViewParams.h
@@ -61,6 +61,7 @@ public:
     FC_VIEW_PARAM(CoinCycleCheck,bool,Bool,true) \
     FC_VIEW_PARAM(EnablePropertyViewForInactiveDocument,bool,Bool,true) \
     FC_VIEW_PARAM(ShowSelectionBoundingBox,bool,Bool,false) \
+    FC_VIEW_PARAM(PropertyViewTimer, unsigned long, Unsigned, 100) \
 
 #undef FC_VIEW_PARAM
 #define FC_VIEW_PARAM(_name,_ctype,_type,_def) \
diff --git a/src/Gui/propertyeditor/PropertyEditor.cpp b/src/Gui/propertyeditor/PropertyEditor.cpp
index 0dc694d29f..017575459f 100644
--- a/src/Gui/propertyeditor/PropertyEditor.cpp
+++ b/src/Gui/propertyeditor/PropertyEditor.cpp
@@ -61,6 +61,7 @@ PropertyEditor::PropertyEditor(QWidget *parent)
     , delaybuild(false)
     , binding(false)
     , checkDocument(false)
+    , closingEditor(false)
 {
     propertyModel = new PropertyModel(this);
     setModel(propertyModel);
@@ -73,7 +74,8 @@ PropertyEditor::PropertyEditor(QWidget *parent)
     setItemDelegate(delegate);
 
     setAlternatingRowColors(true);
-    setRootIsDecorated(true);
+    setRootIsDecorated(false);
+    setExpandsOnDoubleClick(true);
 
     QStyleOptionViewItem opt = viewOptions();
     this->background = opt.palette.dark();
@@ -83,6 +85,12 @@ PropertyEditor::PropertyEditor(QWidget *parent)
 
     connect(this, SIGNAL(activated(const QModelIndex &)), this, SLOT(onItemActivated(const QModelIndex &)));
     connect(this, SIGNAL(clicked(const QModelIndex &)), this, SLOT(onItemActivated(const QModelIndex &)));
+    connect(this, SIGNAL(expanded(const QModelIndex &)), this, SLOT(onItemExpanded(const QModelIndex &)));
+    connect(this, SIGNAL(collapsed(const QModelIndex &)), this, SLOT(onItemCollapsed(const QModelIndex &)));
+    connect(propertyModel, SIGNAL(rowsMoved(const QModelIndex &, int, int, const QModelIndex &, int)),
+            this, SLOT(onRowsMoved(const QModelIndex &, int, int, const QModelIndex &, int)));
+    connect(propertyModel, SIGNAL(rowsRemoved(const QModelIndex &, int, int)),
+            this, SLOT(onRowsRemoved(const QModelIndex &, int, int)));
 }
 
 PropertyEditor::~PropertyEditor()
@@ -102,6 +110,20 @@ bool PropertyEditor::isAutomaticExpand(bool) const
     return autoexpand;
 }
 
+void PropertyEditor::onItemExpanded(const QModelIndex &index)
+{
+    PropertyItem* item = static_cast<PropertyItem*>(index.internalPointer());
+    item->setExpanded(true);
+    for(int i=0, c=item->childCount(); i<c; ++i)
+        setExpanded(propertyModel->index(i, 0, index), item->child(i)->isExpanded());
+}
+
+void PropertyEditor::onItemCollapsed(const QModelIndex &index)
+{
+    PropertyItem* item = static_cast<PropertyItem*>(index.internalPointer());
+    item->setExpanded(false);
+}
+
 void PropertyEditor::setAutomaticDocumentUpdate(bool v)
 {
     autoupdate = v;
@@ -202,19 +224,37 @@ void PropertyEditor::currentChanged ( const QModelIndex & current, const QModelI
     //     openPersistentEditor(model()->buddy(current));
 }
 
-void PropertyEditor::setupTransaction(const QModelIndex &index) {
-    if(!autoupdate)
-        return;
-    if(this->state()!=EditingState) {
-        FC_LOG("editor not editing");
-        return;
+void PropertyEditor::closeEditor()
+{
+    if (editingIndex.isValid()) {
+        Base::StateLocker guard(closingEditor);
+        bool hasFocus = activeEditor && activeEditor->hasFocus();
+        closePersistentEditor(editingIndex);
+        editingIndex = QPersistentModelIndex();
+        activeEditor = nullptr;
+        if(hasFocus)
+            setFocus();
     }
+}
+
+void PropertyEditor::openEditor(const QModelIndex &index)
+{
+    if(editingIndex == index && activeEditor)
+        return;
+
+    closeEditor();
+
+    openPersistentEditor(model()->buddy(index));
+
+    if(!editingIndex.isValid() || !autoupdate)
+        return;
+
     auto &app = App::GetApplication();
     if(app.getActiveTransaction()) {
         FC_LOG("editor already transacting " << app.getActiveTransaction());
         return;
     }
-    PropertyItem* item = static_cast<PropertyItem*>(index.internalPointer());
+    PropertyItem* item = static_cast<PropertyItem*>(editingIndex.internalPointer());
     auto items = item->getPropertyData();
     for(auto propItem=item->parent();items.empty() && propItem;propItem=propItem->parent())
         items = propItem->getPropertyData();
@@ -261,14 +301,13 @@ void PropertyEditor::onItemActivated ( const QModelIndex & index )
 {
     if(index.column() != 1)
         return;
-    edit(model()->buddy(index),AllEditTriggers,0);
-    setupTransaction(index);
+    openEditor(index);
 }
 
 void PropertyEditor::recomputeDocument(App::Document* doc)
 {
     try {
-        if (doc) {
+        if (App::Document* doc = App::GetApplication().getActiveDocument()) {
             if (!doc->isTransactionEmpty()) {
                 // Between opening and committing a transaction a recompute
                 // could already have been done
@@ -283,7 +322,7 @@ void PropertyEditor::recomputeDocument(App::Document* doc)
     }
     catch (const std::exception& e) {
         Base::Console().Error("Unhandled std::exception caught in PropertyEditor::recomputeDocument.\n"
-                              "The error message is: %s\n", e.what());
+                            "The error message is: %s\n", e.what());
     }
     catch (...) {
         Base::Console().Error("Unhandled unknown exception caught in PropertyEditor::recomputeDocument.\n");
@@ -304,15 +343,37 @@ void PropertyEditor::closeTransaction()
 
 void PropertyEditor::closeEditor (QWidget * editor, QAbstractItemDelegate::EndEditHint hint)
 {
-    QTreeView::closeEditor(editor, hint);
+    if (closingEditor)
+        return;
+
+    if (removingRows) {
+        // When removing rows, QTreeView will temporary hide the editor which
+        // will trigger Event::FocusOut and subsequently trigger call of
+        // closeEditor() here. Since we are using persistent editor, QTreeView
+        // will not destroy the editor. But we still needs to call
+        // QTreeView::closeEditor() here, in case the editor belongs to the
+        // removed rows.
+        QTreeView::closeEditor(editor, hint);
+        return;
+    }
 
     closeTransaction();
 
-    QModelIndex indexSaved = currentIndex();
-    FC_LOG("index saved " << indexSaved.row() << ", " << indexSaved.column());
+    // If we are not removing rows, then QTreeView::closeEditor() does nothing
+    // because we are using persistent editor, so we have to call our own
+    // version of closeEditor()
+    this->closeEditor();
 
-    QModelIndex lastIndex;
-    while(this->state()!=EditingState) {
+    QModelIndex indexSaved = currentIndex();
+
+    if (indexSaved.column() == 0) {
+        // Calling setCurrentIndex() to make sure we focus on column 1 instead of 0.
+        setCurrentIndex(propertyModel->buddy(indexSaved));
+    }
+
+    QModelIndex lastIndex = indexSaved;
+    bool wrapped = false;
+    do {
         QModelIndex index;
         if (hint == QAbstractItemDelegate::EditNextItem) {
             index = moveCursor(MoveDown,Qt::NoModifier);
@@ -320,31 +381,122 @@ void PropertyEditor::closeEditor (QWidget * editor, QAbstractItemDelegate::EndEd
             index = moveCursor(MoveUp,Qt::NoModifier);
         } else
             break;
-        if(!index.isValid() || index==lastIndex) {
-            setCurrentIndex(indexSaved);
-            break;
+        if (!index.isValid() || index == lastIndex) {
+            if (wrapped) {
+                setCurrentIndex(propertyModel->buddy(indexSaved));
+                break;
+            }
+            wrapped = true;
+            if (hint == QAbstractItemDelegate::EditNextItem)
+                index = moveCursor(MoveHome, Qt::NoModifier);
+            else
+                index = moveCursor(MoveEnd, Qt::NoModifier);
+            if (!index.isValid() || index == indexSaved)
+                break;
         }
         lastIndex = index;
-        setCurrentIndex(index);
-        edit(index,AllEditTriggers,0);
-    }
-    setupTransaction(currentIndex());
+        setCurrentIndex(propertyModel->buddy(index));
+
+        PropertyItem *item = static_cast<PropertyItem*>(index.internalPointer());
+        // Skip readonly item, because the editor will be disabled and hence
+        // does not accept focus, and in turn break Tab/Backtab navigation.
+        if (item && item->isReadOnly())
+            continue;
+
+        openEditor(index);
+
+    } while (!editingIndex.isValid());
 }
 
 void PropertyEditor::reset()
 {
     QTreeView::reset();
 
-    QModelIndex index;
-    int numRows = propertyModel->rowCount(index);
-    if (numRows > 0)
-        setEditorMode(index, 0, numRows-1);
+    closeTransaction();
+
+    QModelIndex parent;
+    int numRows = propertyModel->rowCount(parent);
+    for (int i=0; i<numRows; ++i) {
+        QModelIndex index = propertyModel->index(i, 0, parent);
+        PropertyItem *item = static_cast<PropertyItem*>(index.internalPointer());
+        if (item->childCount() == 0) {
+            if(item->isSeparator())
+                setRowHidden(i, parent, true);
+        } else
+            setEditorMode(index, 0, item->childCount()-1);
+        if(item->isExpanded())
+            setExpanded(index, true);
+    }
+}
+
+void PropertyEditor::onRowsMoved(const QModelIndex &parent, int start, int end, const QModelIndex &dst, int)
+{
+    if(parent != dst) {
+        PropertyItem *item = static_cast<PropertyItem*>(parent.internalPointer());
+        if(item && item->isSeparator() && item->childCount()==0)
+            setRowHidden(parent.row(), propertyModel->parent(parent), true);
+        item = static_cast<PropertyItem*>(dst.internalPointer());
+        if(item && item->isSeparator() && item->childCount()==end-start+1) {
+            setRowHidden(dst.row(), propertyModel->parent(dst), false);
+            setExpanded(dst, true);
+        }
+    }
 }
 
 void PropertyEditor::rowsInserted (const QModelIndex & parent, int start, int end)
 {
     QTreeView::rowsInserted(parent, start, end);
-    setEditorMode(parent, start, end);
+
+    PropertyItem *item = static_cast<PropertyItem*>(parent.internalPointer());
+    if (item && item->isSeparator() && item->childCount() == end-start+1) {
+        setRowHidden(parent.row(), propertyModel->parent(parent), false);
+        if(item->isExpanded())
+            setExpanded(parent, true);
+    }
+
+    for (int i=start; i<end; ++i) {
+        QModelIndex index = propertyModel->index(i, 0, parent);
+        PropertyItem *child = static_cast<PropertyItem*>(index.internalPointer());
+        if(child->isSeparator()) {
+            // Set group header rows to span all columns
+            setFirstColumnSpanned(i, parent, true);
+        }
+        if(child->isExpanded())
+            setExpanded(index, true);
+    }
+
+    if(parent.isValid())
+        setEditorMode(parent, start, end);
+}
+
+void PropertyEditor::rowsAboutToBeRemoved (const QModelIndex & parent, int start, int end)
+{
+    QTreeView::rowsAboutToBeRemoved(parent, start, end);
+
+    PropertyItem *item = static_cast<PropertyItem*>(parent.internalPointer());
+    if (item && item->isSeparator() && item->childCount() == end-start+1)
+        setRowHidden(parent.row(), propertyModel->parent(parent), true);
+
+    if (editingIndex.isValid()) {
+        if (editingIndex.row() >= start && editingIndex.row() <= end)
+            closeTransaction();
+        else {
+            removingRows = 1;
+            for (QWidget *w = qApp->focusWidget(); w; w = w->parentWidget()) {
+                if(w == activeEditor) {
+                    removingRows = -1;
+                    break;
+                }
+            }
+        }
+    }
+}
+
+void PropertyEditor::onRowsRemoved(const QModelIndex &, int, int)
+{
+    if (removingRows < 0 && activeEditor)
+        activeEditor->setFocus();
+    removingRows = 0;
 }
 
 void PropertyEditor::drawBranches(QPainter *painter, const QRect &rect, const QModelIndex &index) const
@@ -376,7 +528,10 @@ void PropertyEditor::buildUp(PropertyModel::PropertyList &&props, bool _checkDoc
         return;
     }
 
-    closeTransaction();
+    // Do not close transaction here, because we are now doing incremental
+    // update in PropertyModel::buildUp()
+    //
+    // closeTransaction();
 
     QModelIndex index = this->currentIndex();
     QStringList propertyPath = propertyModel->propertyPathFromIndex(index);
@@ -422,60 +577,9 @@ void PropertyEditor::setEditorMode(const QModelIndex & parent, int start, int en
         if (!PropertyView::showAll() && propItem && propItem->testStatus(App::Property::Hidden)) {
             setRowHidden (i, parent, true);
         }
-        if (propItem && propItem->isSeparator()) {
-            // Set group header rows to span all columns
-            setFirstColumnSpanned(i, parent, true);
-        }
     }
 }
 
-void PropertyEditor::updateEditorMode(const App::Property& prop)
-{
-    // check if the parent object is selected
-    std::string editor = prop.getEditorName();
-    if (!PropertyView::showAll() && editor.empty())
-        return;
-
-    bool hidden = PropertyView::isPropertyHidden(&prop);
-    bool readOnly = prop.testStatus(App::Property::ReadOnly);
-
-    int column = 1;
-    int numRows = propertyModel->rowCount();
-    for (int i=0; i<numRows; i++) {
-        QModelIndex item = propertyModel->index(i, column);
-        PropertyItem* propItem = static_cast<PropertyItem*>(item.internalPointer());
-        if (propItem && propItem->hasProperty(&prop)) {
-            setRowHidden (i, QModelIndex(), hidden);
-
-            propItem->updateData();
-            if (item.isValid()) {
-                updateItemEditor(!readOnly, column, item);
-                dataChanged(item, item);
-            }
-            break;
-        }
-    }
-}
-
-void PropertyEditor::updateItemEditor(bool enable, int column, const QModelIndex& parent)
-{
-    QWidget* editor = indexWidget(parent);
-    if (editor)
-        editor->setEnabled(enable);
-
-    int numRows = propertyModel->rowCount(parent);
-    for (int i=0; i<numRows; i++) {
-        QModelIndex item = propertyModel->index(i, column, parent);
-        if (item.isValid()) {
-            updateItemEditor(enable, column, item);
-        }
-    }
-}
-
-bool PropertyEditor::appendProperty(const App::Property& prop) {
-    return !!propOwners.count(prop.getContainer());
-}
-
 void PropertyEditor::removeProperty(const App::Property& prop)
 {
     for (PropertyModel::PropertyList::iterator it = propList.begin(); it != propList.end(); ++it) {
@@ -647,17 +751,20 @@ void PropertyEditor::contextMenuEvent(QContextMenuEvent *) {
         break;
     case MA_Expression:
         if(contextIndex == currentIndex()) {
-            closePersistentEditor(contextIndex);
             Base::FlagToggler<> flag(binding);
-            edit(contextIndex,AllEditTriggers,0);
-            setupTransaction(contextIndex);
+            openEditor(contextIndex);
         }
         break;
     case MA_AddProp: {
         App::AutoTransaction committer("Add property");
         std::unordered_set<App::PropertyContainer*> containers;
-        for(auto prop : props)
-            containers.insert(prop->getContainer());
+        auto sels = Gui::Selection().getSelection("*");
+        if(sels.size() == 1)
+            containers.insert(sels[0].pObject);
+        else {
+            for(auto prop : props)
+                containers.insert(prop->getContainer());
+        }
         Gui::Dialog::DlgAddProperty dlg(
                 Gui::getMainWindow(),std::move(containers));
         dlg.exec();
diff --git a/src/Gui/propertyeditor/PropertyEditor.h b/src/Gui/propertyeditor/PropertyEditor.h
index f9b55798af..bf4fc46990 100644
--- a/src/Gui/propertyeditor/PropertyEditor.h
+++ b/src/Gui/propertyeditor/PropertyEditor.h
@@ -75,8 +75,6 @@ public:
     /** Builds up the list view with the properties. */
     void buildUp(PropertyModel::PropertyList &&props = PropertyModel::PropertyList(), bool checkDocument=false);
     void updateProperty(const App::Property&);
-    void updateEditorMode(const App::Property&);
-    bool appendProperty(const App::Property&);
     void removeProperty(const App::Property&);
     void setAutomaticExpand(bool);
     bool isAutomaticExpand(bool) const;
@@ -91,9 +89,15 @@ public:
     void setGroupTextColor(const QColor& c);
 
     bool isBinding() const { return binding; }
+    void openEditor(const QModelIndex &index);
+    void closeEditor();
 
 protected Q_SLOTS:
     void onItemActivated(const QModelIndex &index);
+    void onItemExpanded(const QModelIndex &index);
+    void onItemCollapsed(const QModelIndex &index);
+    void onRowsMoved(const QModelIndex &parent, int start, int end, const QModelIndex &dst, int row);
+    void onRowsRemoved(const QModelIndex &parent, int start, int end);
 
 protected:
     virtual void closeEditor (QWidget * editor, QAbstractItemDelegate::EndEditHint hint);
@@ -101,6 +105,7 @@ protected:
     virtual void editorDestroyed (QObject * editor);
     virtual void currentChanged (const QModelIndex & current, const QModelIndex & previous);
     virtual void rowsInserted (const QModelIndex & parent, int start, int end);
+    virtual void rowsAboutToBeRemoved (const QModelIndex & parent, int start, int end);
     virtual void drawBranches(QPainter *painter, const QRect &rect, const QModelIndex &index) const;
     virtual QStyleOptionViewItem viewOptions() const;
     virtual void contextMenuEvent(QContextMenuEvent *event);
@@ -108,8 +113,6 @@ protected:
 
 private:
     void setEditorMode(const QModelIndex & parent, int start, int end);
-    void updateItemEditor(bool enable, int column, const QModelIndex& parent);
-    void setupTransaction(const QModelIndex &);
     void closeTransaction();
     void recomputeDocument(App::Document*);
 
@@ -125,13 +128,19 @@ private:
     bool delaybuild;
     bool binding;
     bool checkDocument;
+    bool closingEditor;
 
     int transactionID = 0;
 
     QColor groupColor;
     QBrush background;
 
+    QPointer<QWidget> activeEditor;
+    QPersistentModelIndex editingIndex;
+    int removingRows = 0;
+
     friend class Gui::PropertyView;
+    friend class PropertyItemDelegate;
 };
 
 } //namespace PropertyEditor
diff --git a/src/Gui/propertyeditor/PropertyItem.cpp b/src/Gui/propertyeditor/PropertyItem.cpp
index 0942dad4b3..5c2c217728 100644
--- a/src/Gui/propertyeditor/PropertyItem.cpp
+++ b/src/Gui/propertyeditor/PropertyItem.cpp
@@ -95,7 +95,7 @@ Q_DECLARE_METATYPE(Py::Object)
 
 PROPERTYITEM_SOURCE(Gui::PropertyEditor::PropertyItem)
 
-PropertyItem::PropertyItem() : parentItem(0), readonly(false), cleared(false), linked(false)
+PropertyItem::PropertyItem() : parentItem(0), readonly(false), linked(false), expanded(false)
 {
     precision = Base::UnitsApi::getDecimals();
     setAutoApply(true);
@@ -255,6 +255,11 @@ void PropertyItem::removeChildren(int from, int to)
     }
 }
 
+void PropertyItem::moveChild(int from, int to)
+{
+    childItems.move(from, to);
+}
+
 /*!
  * \brief PropertyItem::takeChild
  * Removes the child at index row but doesn't delete it
@@ -305,6 +310,16 @@ bool PropertyItem::isLinked() const
     return linked;
 }
 
+void PropertyItem::setExpanded(bool enable)
+{
+    expanded = enable;
+}
+
+bool PropertyItem::isExpanded() const
+{
+    return expanded;
+}
+
 bool PropertyItem::testStatus(App::Property::Status pos) const
 {
     std::vector<App::Property*>::const_iterator it;
@@ -468,9 +483,15 @@ QString PropertyItem::propertyName() const
     return propName;
 }
 
-void PropertyItem::setPropertyName(const QString& name)
+void PropertyItem::setPropertyName(QString name, QString realName)
 {
-    setObjectName(name);
+    if(realName.size())
+        propName = realName;
+    else
+        propName = name;
+
+    setObjectName(propName);
+
     QString display;
     bool upper = false;
     for (int i=0; i<name.length(); i++) {
@@ -552,10 +573,10 @@ QVariant PropertyItem::data(int column, int role) const
 {
     // property name
     if (column == 0) {
-        if (role == Qt::ForegroundRole && linked)
+        if (role == Qt::TextColorRole && linked)
             return QVariant::fromValue(QColor(0x20,0xaa,0x20));
 
-        if (role == Qt::BackgroundRole || role == Qt::ForegroundRole) {
+        if (role == Qt::BackgroundRole || role == Qt::TextColorRole) {
             if(PropertyView::showAll()
                 && propertyItems.size() == 1
                 && propertyItems.front()->testStatus(App::Property::PropDynamic)
@@ -575,11 +596,12 @@ QVariant PropertyItem::data(int column, int role) const
             return QVariant();
         }
         else if (role == Qt::ToolTipRole) {
-            if(!PropertyView::showAll())
-                return toolTip(propertyItems[0]);
-            QString type = QString::fromLatin1("Type: %1").arg(
-                    QString::fromLatin1(propertyItems[0]->getTypeId().getName()));
-            QString doc = toolTip(propertyItems[0]).toString();
+            QString type = QString::fromLatin1("Type: %1\nName: %2").arg(
+                    QString::fromLatin1(propertyItems[0]->getTypeId().getName()), objectName());
+
+            QString doc = PropertyItem::toolTip(propertyItems[0]).toString();
+            if(doc.isEmpty())
+                doc = toolTip(propertyItems[0]).toString();
             if(doc.size())
                 return type + QLatin1String("\n\n") + doc;
             return type;
@@ -637,8 +659,6 @@ QVariant PropertyItem::data(int column, int role) const
 
 bool PropertyItem::setData (const QVariant& value)
 {
-    cleared = false;
-
     // This is the basic mechanism to set the value to
     // a property and if no property is set for this item
     // it delegates it to its parent which sets then the
@@ -4136,6 +4156,9 @@ LinkLabel::LinkLabel (QWidget * parent, const App::Property *prop)
 #endif
     editButton->setToolTip(tr("Change the linked object"));
     layout->addWidget(editButton);
+
+    this->setFocusPolicy(Qt::StrongFocus);
+    this->setFocusProxy(label);
     
     // setLayout(layout);
     
@@ -4201,7 +4224,7 @@ void LinkLabel::onEditClicked ()
     if(!dlg) {
         dlg = new DlgPropertyLink(this);
         dlg->init(objProp,true);
-        connect(dlg, SIGNAL(accepted()), this, SLOT(onLinkChanged()));
+        connect(dlg, SIGNAL(finished(int)), this, SLOT(onLinkChanged()));
     } else
         dlg->init(objProp,false);
     dlg->show();
diff --git a/src/Gui/propertyeditor/PropertyItem.h b/src/Gui/propertyeditor/PropertyItem.h
index c409a0be90..a5871ca694 100644
--- a/src/Gui/propertyeditor/PropertyItem.h
+++ b/src/Gui/propertyeditor/PropertyItem.h
@@ -76,6 +76,7 @@ class DlgPropertyLink;
 namespace PropertyEditor {
 
 class PropertyItem;
+class PropertyModel;
 
 /**
  * The PropertyItemFactory provides methods for the dynamic creation of property items.
@@ -149,6 +150,7 @@ public:
     PropertyItem *parent() const;
     void appendChild(PropertyItem *child);
     void insertChild(int, PropertyItem *child);
+    void moveChild(int from, int to);
     void removeChildren(int from, int to);
     PropertyItem *takeChild(int);
 
@@ -161,16 +163,19 @@ public:
     void setLinked(bool);
     bool isLinked() const;
 
+    bool isExpanded() const;
+    void setExpanded(bool e);
+
     PropertyItem *child(int row);
     int childCount() const;
     int columnCount() const;
     QString propertyName() const;
-    void setPropertyName(const QString&);
+    void setPropertyName(QString name, QString realName=QString());
     void setPropertyValue(const QString&);
     virtual QVariant data(int column, int role) const;
     bool setData (const QVariant& value);
     Qt::ItemFlags flags(int column) const;
-    int row() const;
+    virtual int row() const;
     void reset();
 
     bool hasAnyExpression() const;
@@ -197,8 +202,8 @@ protected:
     QList<PropertyItem*> childItems;
     bool readonly;
     int precision;
-    bool cleared;
     bool linked;
+    bool expanded;
 };
 
 /**
@@ -254,6 +259,14 @@ class GuiExport PropertySeparatorItem : public PropertyItem
 
     bool isSeparator() const { return true; }
     QWidget* createEditor(QWidget* parent, const QObject* receiver, const char* method) const;
+
+    virtual int row() const {
+        return _row<0?PropertyItem::row():_row;
+    }
+
+private:
+    friend PropertyModel;
+    int _row = -1;
 };
 
 /**
diff --git a/src/Gui/propertyeditor/PropertyItemDelegate.cpp b/src/Gui/propertyeditor/PropertyItemDelegate.cpp
index cbff438b4b..fa5bb7db9e 100644
--- a/src/Gui/propertyeditor/PropertyItemDelegate.cpp
+++ b/src/Gui/propertyeditor/PropertyItemDelegate.cpp
@@ -76,10 +76,13 @@ void PropertyItemDelegate::paint(QPainter *painter, const QStyleOptionViewItem &
         }
         option.palette.setColor(QPalette::Text, color);
         option.font.setBold(true);
-        option.state &= ~QStyle::State_Selected;
-    }
 
-    if (index.column() == 1) {
+        // Since the group item now parents all the property items and can be
+        // collapsed, it makes sense to have some selection visual clue for it.
+        //
+        // option.state &= ~QStyle::State_Selected;
+    }
+    else if (index.column() == 1) {
         option.state &= ~QStyle::State_Selected;
     }
 
@@ -131,9 +134,15 @@ QWidget * PropertyItemDelegate::createEditor (QWidget * parent, const QStyleOpti
     if (!childItem)
         return 0;
 
+    PropertyEditor *parentEditor = qobject_cast<PropertyEditor*>(this->parent());
+    if(parentEditor)
+        parentEditor->closeEditor();
+
+    if (childItem->isSeparator())
+        return 0;
+
     FC_LOG("create editor " << index.row() << "," << index.column());
 
-    PropertyEditor *parentEditor = qobject_cast<PropertyEditor*>(this->parent());
     QWidget* editor;
     expressionEditor = 0;
     if(parentEditor && parentEditor->isBinding())
@@ -153,6 +162,18 @@ QWidget * PropertyItemDelegate::createEditor (QWidget * parent, const QStyleOpti
     }
     this->pressed = false;
 
+    if (editor) {
+        for (auto w : editor->findChildren<QWidget*>()) {
+            if (qobject_cast<QAbstractButton*>(w)
+                    || qobject_cast<QLabel*>(w))
+            {
+                w->installEventFilter(const_cast<PropertyItemDelegate*>(this));
+            }
+        }
+        parentEditor->activeEditor = editor;
+        parentEditor->editingIndex = index;
+    }
+
     return editor;
 }
 
diff --git a/src/Gui/propertyeditor/PropertyItemDelegate.h b/src/Gui/propertyeditor/PropertyItemDelegate.h
index f2ae422954..f7f8a659e7 100644
--- a/src/Gui/propertyeditor/PropertyItemDelegate.h
+++ b/src/Gui/propertyeditor/PropertyItemDelegate.h
@@ -44,7 +44,6 @@ public:
     virtual QSize sizeHint(const QStyleOptionViewItem & option, const QModelIndex & index) const;
     virtual bool editorEvent (QEvent *event, QAbstractItemModel *model,
                               const QStyleOptionViewItem& option, const QModelIndex& index);
-
 public Q_SLOTS:
     void valueChanged();
 
diff --git a/src/Gui/propertyeditor/PropertyModel.cpp b/src/Gui/propertyeditor/PropertyModel.cpp
index 41254c5801..029ca31641 100644
--- a/src/Gui/propertyeditor/PropertyModel.cpp
+++ b/src/Gui/propertyeditor/PropertyModel.cpp
@@ -33,6 +33,7 @@
 #include "PropertyItem.h"
 #include "PropertyView.h"
 
+using namespace Gui;
 using namespace Gui::PropertyEditor;
 
 
@@ -178,12 +179,9 @@ QStringList PropertyModel::propertyPathFromIndex(const QModelIndex& index) const
     QStringList path;
     if (index.isValid()) {
         PropertyItem* item = static_cast<PropertyItem*>(index.internalPointer());
-        if (!item->isSeparator()) {
-            do {
-                path.push_front(item->propertyName());
-                item = item->parent();
-            }
-            while (item != this->rootItem && item != 0);
+        while (item && item != this->rootItem) {
+            path.push_front(item->propertyName());
+            item = item->parent();
         }
     }
 
@@ -192,215 +190,299 @@ QStringList PropertyModel::propertyPathFromIndex(const QModelIndex& index) const
 
 QModelIndex PropertyModel::propertyIndexFromPath(const QStringList& path) const
 {
-    QModelIndex parent;
-    for (QStringList::const_iterator it = path.begin(); it != path.end(); ++it) {
-        int rows = this->rowCount(parent);
-        for (int i=0; i<rows; i++) {
-            QModelIndex index = this->index(i, 0, parent);
-            if (index.isValid()) {
-                PropertyItem* item = static_cast<PropertyItem*>(index.internalPointer());
-                if (item->propertyName() == *it) {
-                    parent = index;
-                    break;
-                }
+    if (path.size() < 2)
+        return QModelIndex();
+
+    auto it = groupItems.find(path.front());
+    if (it == groupItems.end())
+        return QModelIndex();
+
+    PropertyItem *item = it->second.groupItem;
+    QModelIndex index = this->index(item->row(), 0, QModelIndex());
+
+    for (int j=1; j<path.size(); ++j) {
+        bool found = false;
+        for (int i=0, c = item->childCount(); i<c; ++i) {
+            auto child = item->child(i);
+            if (child->propertyName() == path[j]) {
+                index = this->index(i, 1, index);
+                item = child;
+                found = true;
+                break;
             }
         }
+        if (!found)
+            return j==1?QModelIndex():index;
     }
-    return parent;
+    return index;
 }
 
-struct PropItemInfo {
-    const std::string &name;
-    const std::vector<App::Property*> &props;
-
-    PropItemInfo(const std::string &n, const std::vector<App::Property*> &p)
-        :name(n),props(p)
-    {}
-};
-
 static void setPropertyItemName(PropertyItem *item, const char *propName, QString groupName) {
     QString name = QString::fromLatin1(propName);
+    QString realName = name;
     if(name.size()>groupName.size()+1 
             && name.startsWith(groupName + QLatin1Char('_')))
         name = name.right(name.size()-groupName.size()-1);
 
-    item->setPropertyName(name);
+    item->setPropertyName(name, realName);
+}
+
+static PropertyItem *createPropertyItem(App::Property *prop)
+{
+    const char *editor = prop->getEditorName();
+    if (!editor || !editor[0]) {
+        if (PropertyView::showAll())
+            editor = "Gui::PropertyEditor::PropertyItem";
+        else
+            return nullptr;
+    }
+    auto item = static_cast<PropertyItem*>(
+            PropertyItemFactory::instance().createPropertyItem(editor));
+    if (!item) {
+        qWarning("No property item for type %s found\n", editor);
+    }
+    return item;
+}
+
+PropertyModel::GroupInfo &PropertyModel::getGroupInfo(App::Property *prop)
+{
+    const char* group = prop->getGroup();
+    bool isEmpty = (group == 0 || group[0] == '\0');
+    QString groupName = QString::fromLatin1(
+            isEmpty ? QT_TRANSLATE_NOOP("App::Property", "Base") : group);
+
+    auto res = groupItems.insert(std::make_pair(groupName, GroupInfo()));
+    if (res.second) {
+        auto &groupInfo = res.first->second;
+        groupInfo.groupItem = static_cast<PropertySeparatorItem*>(PropertySeparatorItem::create());
+        groupInfo.groupItem->setReadOnly(true);
+        groupInfo.groupItem->setExpanded(true);
+        groupInfo.groupItem->setParent(rootItem);
+        groupInfo.groupItem->setPropertyName(groupName);
+
+        auto it = res.first;
+        int row = 0;
+        if (it != groupItems.begin()) {
+            --it;
+            row = it->second.groupItem->_row + 1;
+            ++it;
+        }
+        groupInfo.groupItem->_row = row;
+        beginInsertRows(QModelIndex(), row, row);
+        rootItem->insertChild(row, groupInfo.groupItem);
+        // update row index for all group items behind
+        for (++it; it!=groupItems.end(); ++it)
+            ++it->second.groupItem->_row;
+        endInsertRows();
+    }
+
+    return res.first->second;
 }
 
 void PropertyModel::buildUp(const PropertyModel::PropertyList& props)
 {
-    beginResetModel();
-
-    // fill up the listview with the properties
-    rootItem->reset();
-
-    // sort the properties into their groups
-    std::map<std::string, std::vector<PropItemInfo> > propGroup;
-    PropertyModel::PropertyList::const_iterator jt;
-    for (jt = props.begin(); jt != props.end(); ++jt) {
-        App::Property* prop = jt->second.front();
-        const char* group = prop->getGroup();
-        bool isEmpty = (group == 0 || group[0] == '\0');
-        std::string grp = isEmpty ? QT_TRANSLATE_NOOP("App::Property", "Base") : group;
-        propGroup[grp].emplace_back(jt->first,jt->second);
+    // If props empty, then simply reset all property items, but keep the group
+    // items.
+    if (props.empty()) {
+        beginResetModel();
+        for(auto &v : groupItems) {
+            auto &groupInfo = v.second;
+            groupInfo.groupItem->reset();
+            groupInfo.children.clear();
+        }
+        itemMap.clear();
+        endResetModel();
+        return;
     }
 
-    for (auto kt = propGroup.begin(); kt != propGroup.end(); ++kt) {
-        // set group item
-        PropertyItem* group = static_cast<PropertyItem*>(PropertySeparatorItem::create());
-        group->setParent(rootItem);
-        rootItem->appendChild(group);
-        QString groupName = QString::fromLatin1(kt->first.c_str());
-        group->setPropertyName(groupName);
+    // First step, init group info
+    for (auto &v : groupItems) {
+        auto &groupInfo = v.second;
+        groupInfo.children.clear();
+    }
 
-        // setup the items for the properties
-        for (auto it = kt->second.begin(); it != kt->second.end(); ++it) {
-            const auto &info = *it;
-            App::Property* prop = info.props.front();
-            std::string editor(prop->getEditorName());
-            if(editor.empty() && PropertyView::showAll())
-                editor = "Gui::PropertyEditor::PropertyItem";
-            if (!editor.empty()) {
-                PropertyItem* item = PropertyItemFactory::instance().createPropertyItem(editor.c_str());
-                if (!item) {
-                    qWarning("No property item for type %s found\n", editor.c_str());
-                    continue;
-                }
-                else {
-                    if(boost::ends_with(info.name,"*"))
-                        item->setLinked(true);
-                    PropertyItem* child = (PropertyItem*)item;
-                    child->setParent(rootItem);
-                    rootItem->appendChild(child);
+    // Second step, either find existing items or create new items for the given
+    // properties. There is no signaling of model change at this stage. The
+    // change information is kept pending in GroupInfo::children
+    for (auto jt = props.begin(); jt != props.end(); ++jt) {
+        App::Property* prop = jt->second.front();
 
-                    setPropertyItemName(child,prop->getName(),groupName);
+        PropertyItem *item = nullptr;
+        for (auto prop : jt->second) {
+            auto it = itemMap.find(prop);
+            if (it == itemMap.end() || !it->second)
+                continue;
+            item = it->second;
+            break;
+        }
 
-                    child->setPropertyData(info.props);
-                }
-            }
+        if (!item) {
+            item = createPropertyItem(prop);
+            if (!item)
+                continue;
+        }
+
+        GroupInfo &groupInfo = getGroupInfo(prop);
+        groupInfo.children.push_back(item);
+
+        item->setLinked(boost::ends_with(jt->first,"*"));
+        setPropertyItemName(item, prop->getName(), groupInfo.groupItem->propertyName());
+
+        if (jt->second != item->getPropertyData()) {
+            for (auto prop : item->getPropertyData())
+                itemMap.erase(prop);
+            for (auto prop : jt->second)
+                itemMap[prop] = item;
+            // TODO: is it necessary to make sure the item has no pending commit?
+            item->setPropertyData(jt->second);
         }
     }
 
-    endResetModel();
+    // Third step, signal item insertion and movement.
+    for (auto &v : groupItems) {
+        auto &groupInfo = v.second;
+        int beginChange = -1;
+        int endChange = 0;
+        int beginInsert = -1;
+        int endInsert = 0;
+        int row = -1;
+        QModelIndex midx = this->index(groupInfo.groupItem->_row, 0, QModelIndex());
+
+        auto flushInserts = [&]() {
+            if (beginInsert < 0)
+                return;
+            beginInsertRows(midx, beginInsert, endInsert);
+            for (int i=beginInsert; i<=endInsert; ++i)
+                groupInfo.groupItem->insertChild(i, groupInfo.children[i]);
+            endInsertRows();
+            beginInsert = -1;
+        };
+
+        auto flushChanges = [&]() {
+            if (beginChange < 0)
+                return;
+            (void)endChange;
+            // There is no need to signal dataChange(), because PropertyEditor
+            // will call PropertyModel::updateProperty() on any property
+            // changes.
+            //
+            // dataChanged(this->index(beginChange,0,midx), this->index(endChange,1,midx));
+            beginChange = -1;
+        };
+
+        for (auto item : groupInfo.children) {
+            ++row;
+            if (!item->parent()) {
+                flushChanges();
+                item->setParent(groupInfo.groupItem);
+                if (beginInsert < 0)
+                    beginInsert = row;
+                endInsert = row;
+            } else {
+                flushInserts();
+                int oldRow = item->row();
+                // Dynamic property can rename group, so must check
+                auto groupItem = item->parent();
+                assert(groupItem);
+                if (oldRow == row && groupItem == groupInfo.groupItem) {
+                    if (beginChange < 0)
+                        beginChange = row;
+                    endChange = row;
+                } else {
+                    flushChanges();
+                    beginMoveRows(createIndex(groupItem->row(),0,groupItem),
+                            oldRow, oldRow, midx, row);
+                    if (groupItem == groupInfo.groupItem)
+                        groupInfo.groupItem->moveChild(oldRow, row);
+                    else {
+                        groupItem->takeChild(oldRow);
+                        item->setParent(groupInfo.groupItem);
+                        groupInfo.groupItem->insertChild(row, item);
+                    }
+                    endMoveRows();
+                }
+            }
+        }
+        flushChanges();
+        flushInserts();
+    }
+
+    // Final step, signal item removal. This is separated from the above because
+    // of the possibility of moving items between groups.
+    for (auto &v : groupItems) {
+        auto &groupInfo = v.second;
+        int last = groupInfo.groupItem->childCount();
+        int first = static_cast<int>(groupInfo.children.size());
+        if (last > first) {
+            QModelIndex midx = this->index(groupInfo.groupItem->_row,0,QModelIndex());
+            beginRemoveRows(midx, first, last-1);
+            groupInfo.groupItem->removeChildren(first, last-1);
+            endRemoveRows();
+        } else {
+            assert(last == first);
+        }
+    }
 }
 
 void PropertyModel::updateProperty(const App::Property& prop)
 {
+    auto it = itemMap.find(const_cast<App::Property*>(&prop));
+    if (it == itemMap.end() || !it->second || !it->second->parent())
+        return;
+
     int column = 1;
-    int numChild = rootItem->childCount();
-    for (int row=0; row<numChild; row++) {
-        PropertyItem* child = rootItem->child(row);
-        if (child->hasProperty(&prop)) {
-            child->updateData();
-            QModelIndex data = this->index(row, column, QModelIndex());
-            if (data.isValid()) {
-                child->assignProperty(&prop);
-                dataChanged(data, data);
-                updateChildren(child, column, data);
-            }
-            break;
-        }
-    }
+    PropertyItem *item = it->second;
+    item->updateData();
+    QModelIndex parent = this->index(item->parent()->row(), 0, QModelIndex());
+    item->assignProperty(&prop);
+    QModelIndex data = this->index(item->row(), column, parent);
+    dataChanged(data, data);
+    updateChildren(item, column, data);
 }
 
-void PropertyModel::appendProperty(const App::Property& prop)
+void PropertyModel::appendProperty(const App::Property& _prop)
 {
-    std::string editor(prop.getEditorName());
-    if(editor.empty() && PropertyView::showAll())
-        editor = "Gui::PropertyEditor::PropertyItem";
-    if (!editor.empty()) {
-        PropertyItem* item = PropertyItemFactory::instance().createPropertyItem(editor.c_str());
-        if (!item) {
-            qWarning("No property item for type %s found\n", editor.c_str());
-            return;
-        }
+    auto prop = const_cast<App::Property*>(&_prop);
+    if (!prop->getName())
+        return;
+    auto it = itemMap.find(prop);
+    if (it == itemMap.end() || !it->second)
+        return;
 
-        const char* group = prop.getGroup();
-        bool isEmpty = (group == 0 || group[0] == '\0');
-        std::string grp = isEmpty ? QT_TRANSLATE_NOOP("App::Property", "Base") : group;
-        QString groupName = QString::fromStdString(grp);
+    PropertyItem *item = createPropertyItem(prop);
+    GroupInfo &groupInfo = getGroupInfo(prop);
 
-        // go through all group names and check if one matches
-        int index = -1;
-        for (int i=0; i<rootItem->childCount(); i++) {
-            PropertyItem* child = rootItem->child(i);
-            if (child->isSeparator()) {
-                if (groupName == child->propertyName()) {
-                    index = i+1;
-                    break;
-                }
-            }
-        }
-
-        int numChilds = rootItem->childCount();
-        int first = 0;
-        int last = 0;
-        if (index < 0) {
-            // create a new group
-            first = numChilds;
-            last = first + 1;
-        }
-        else {
-            // the group exists, determine the position before the next group
-            // or at the end if there is no further group
-            for (int i=index; i<rootItem->childCount(); i++) {
-                index++;
-                PropertyItem* child = rootItem->child(i);
-                if (child->isSeparator()) {
-                    index = i;
-                    break;
-                }
-            }
-
-            first = index;
-            last = index;
-        }
-
-        // notify system to add new row
-        beginInsertRows(QModelIndex(), first, last);
-
-        // create a new group at the end with the property
-        if (index < 0) {
-            PropertyItem* group = static_cast<PropertyItem*>(PropertySeparatorItem::create());
-            group->setParent(rootItem);
-            rootItem->appendChild(group);
-            group->setPropertyName(groupName);
-
-            item->setParent(rootItem);
-            rootItem->appendChild(item);
-        }
-        // add the property at the end of its group
-        else if (index < numChilds) {
-            item->setParent(rootItem);
-            rootItem->insertChild(index, item);
-        }
-        // add the property at end
-        else {
-            item->setParent(rootItem);
-            rootItem->appendChild(item);
-        }
-
-        std::vector<App::Property*> data;
-        data.push_back(const_cast<App::Property*>(&prop));
-
-        setPropertyItemName(item,prop.getName(),groupName);
-        item->setPropertyData(data);
-
-        endInsertRows();
+    int row = 0;
+    for (int c=groupInfo.groupItem->childCount(); row<c; ++row) {
+        PropertyItem *child = groupInfo.groupItem->child(row);
+        App::Property *firstProp = item->getFirstProperty();
+        if (firstProp && firstProp->testStatus(App::Property::PropDynamic)
+                      && item->propertyName() >= child->propertyName())
+            break;
     }
+
+    QModelIndex midx = this->index(groupInfo.groupItem->_row,0,QModelIndex());
+    beginInsertRows(midx, row, row);
+    groupInfo.groupItem->insertChild(row, item);
+    setPropertyItemName(item, prop->getName(), groupInfo.groupItem->propertyName());
+    item->setPropertyData({prop});
+    endInsertRows();
 }
 
-void PropertyModel::removeProperty(const App::Property& prop)
+void PropertyModel::removeProperty(const App::Property& _prop)
 {
-    int numChild = rootItem->childCount();
-    for (int row=0; row<numChild; row++) {
-        PropertyItem* child = rootItem->child(row);
-        if (child->hasProperty(&prop)) {
-            if (child->removeProperty(&prop)) {
-                removeRow(row, QModelIndex());
-            }
-            break;
-        }
+    auto prop = const_cast<App::Property*>(&_prop);
+    auto it = itemMap.find(prop);
+    if (it == itemMap.end() || !it->second)
+        return;
+
+    PropertyItem *item = it->second;
+    if (item->removeProperty(prop)) {
+        PropertyItem *parent = item->parent();
+        int row = item->row();
+        beginRemoveRows(this->index(parent->row(), 0, QModelIndex()), row, row);
+        parent->removeChildren(row,row);
+        endRemoveRows();
     }
 }
 
diff --git a/src/Gui/propertyeditor/PropertyModel.h b/src/Gui/propertyeditor/PropertyModel.h
index 97b3c75c9d..a28779dc32 100644
--- a/src/Gui/propertyeditor/PropertyModel.h
+++ b/src/Gui/propertyeditor/PropertyModel.h
@@ -28,6 +28,8 @@
 #include <QStringList>
 #include <vector>
 #include <map>
+#include <unordered_map>
+#include "PropertyItem.h"
 
 namespace App {
 class Property;
@@ -35,7 +37,6 @@ class Property;
 namespace Gui {
 namespace PropertyEditor {
 
-class PropertyItem;
 class PropertyModel : public QAbstractItemModel
 {
     Q_OBJECT
@@ -70,8 +71,18 @@ public:
 private:
     void updateChildren(PropertyItem* item, int column, const QModelIndex& parent);
 
+    struct GroupInfo {
+        PropertySeparatorItem *groupItem = nullptr;
+        std::vector<PropertyItem *> children;
+    };
+    GroupInfo &getGroupInfo(App::Property *);
+
 private:
     PropertyItem *rootItem;
+
+    std::unordered_map<App::Property*, QPointer<PropertyItem> > itemMap;
+
+    std::map<QString, GroupInfo> groupItems;
 };
 
 } //namespace PropertyEditor