Fix #2888 Crash on Delete View

src/Mod/TechDraw/Gui/MDIViewPage.cpp

@@ -320,6 +320,7 @@ bool MDIViewPage::attachView(App::DocumentObject *obj)
     } else if (typeId.isDerivedFrom(TechDraw::DrawHatch::getClassTypeId()) ) {
         //Hatch is not attached like other Views (since it isn't really a View)
         return true;
+    //DrawGeomHatch??
 
     } else {
         Base::Console().Log("Logic Error - Unknown view type in MDIViewPage::attachView\n");
@@ -328,11 +329,6 @@ bool MDIViewPage::attachView(App::DocumentObject *obj)
     return (qview != nullptr);
 }
 
-void MDIViewPage::removeView(QGIView *view)
-{
-    (void) m_view->removeView(view);
-}
-
 void MDIViewPage::onDeleteObject(const App::DocumentObject& obj)
 {
     if (obj.isDerivedFrom(TechDraw::DrawView::getClassTypeId())) {
@@ -341,7 +337,7 @@ void MDIViewPage::onDeleteObject(const App::DocumentObject& obj)
         TechDraw::DrawPage* dvPg = dv->findParentPage();
         if (dvPg == m_vpPage->getDrawPage()) {
             //this is a DV that is on our page
-            (void) m_view->removeView(dv);
+            (void) m_view->removeQViewByDrawView(dv);
         }
     }
 }
@@ -374,88 +370,35 @@ void MDIViewPage::updateTemplate(bool forceUpdate)
 
 void MDIViewPage::updateDrawing(bool forceUpdate)
 {
-    // We cannot guarantee if the number of graphical representations (QGIVxxxx) have changed so check the number (MLP)
+    // get all the DrawViews for this page, including the second level ones
-    // WF: this should be fixed now with onDeletedObject signal from App side?
+    // if we ever have collections of collections, we'll need to revisit this
-    // WF: the QGVP views list may still not be 100% reliable
+    std::vector<App::DocumentObject*> pChildren  = m_vpPage->getDrawPage()->Views.getValues();
-    // TODO: build list of QGIV's from scene everytime? 
+    std::vector<App::DocumentObject*> appendChildren;
-    //logging counters
+    for (auto& pc: pChildren) {
-//    int qgvpIn = 0;
+        if(pc->getTypeId().isDerivedFrom(TechDraw::DrawViewCollection::getClassTypeId())) {
-//    int qgvpValid = 0;
+            TechDraw::DrawViewCollection *collect = dynamic_cast<TechDraw::DrawViewCollection *>(pc);
-//    int qgvpClean = 0;
+            std::vector<App::DocumentObject*> cChildren = collect->Views.getValues();
-//    int dpIn = 0;
+            appendChildren.insert(std::end(appendChildren), std::begin(cChildren), std::end(cChildren));
-    const std::vector<QGIView *> &graphicsList = m_view->getViews();
+        }
-//    qgvpIn = graphicsList.size();
+    }
-    const std::vector<App::DocumentObject*> &pageChildren  = m_vpPage->getDrawPage()->Views.getValues();
+    pChildren.insert(std::end(pChildren),std::begin(appendChildren),std::end(appendChildren));
 
-    // Count total # DocumentObjects in Page
+    // if dv doesn't have a graphic, make one
-    unsigned int docObjCount = 0;
+    for (auto& dv: pChildren) {
-    for(std::vector<App::DocumentObject*>::const_iterator it = pageChildren.begin(); it != pageChildren.end(); ++it) {
+        QGIView* qv = m_view->findQViewForDocObj(dv);
-        App::DocumentObject *docObj = *it;
+        if (qv == nullptr) {
-        if(docObj->getTypeId().isDerivedFrom(TechDraw::DrawViewCollection::getClassTypeId())) {
+            attachView(dv);
-            TechDraw::DrawViewCollection *collection = dynamic_cast<TechDraw::DrawViewCollection *>(docObj);
-            docObjCount += collection->countChildren(); // Include self
         }
-        docObjCount += 1;
     }
-//    dpIn = docObjCount;
     
-    
+    // if qView doesn't have a Feature, delete it
-    //TODO: should prune QGVP.views first always, then check if view in Page missing QGIVP
+    std::vector<QGIView*> qvs = m_view->getViews();
-    //      this makes assumption that = numbers mean everythign is OK, but could be double failure - 1 extra QGIV, 1 DV missing graphics!
+    App::Document* doc = getAppDocument();
-    
+    for (auto& qv: qvs) {
-    if(graphicsList.size() < docObjCount) {
+        App::DocumentObject* obj = doc->getObject(qv->getViewName());
-        // there are more DocumentObjects than graphical representations (QGIVxxxx's)
+        if (obj == nullptr) {
-        // Find which DocumentObjects have no graphical representation (QGIVxxxx)
+            m_view->removeQView(qv);
-        // Iterate over DocumentObjects without graphical representations and create the QGIVxxxx
-        // TODO think of a better algorithm to deal with any changes to views list
-        std::vector<App::DocumentObject*> notFnd;
-        findMissingViews(pageChildren, notFnd);
-        for(std::vector<App::DocumentObject*>::const_iterator it = notFnd.begin(); it != notFnd.end(); ++it) {
-            attachView(*it);
         }
-    } else if(graphicsList.size() > docObjCount) {
-        // prune any invalid entries in QGVP.views
-        // TODO: revisit this mess.  is it still required with onDeletedItem signal implementation?
-        std::vector<QGIView *> newGraphicsList;
-        QList<QGraphicsItem *> items = m_view->scene()->items();
-        for (auto& v: graphicsList) {              //check that everything in QGVP views is valid
-            for (auto& i:items) {
-                if (v == i) {                      //this one is OK
-                    newGraphicsList.push_back(v);
-                    break;
-                }
-            }
-        }
-//        qgvpValid = newGraphicsList.size();
-        //newGraphicsList now only contains valid QGIV's
-        //now prune the ones without docObjs
-        std::vector<QGIView *> cleanItems;
-        for (auto& i: newGraphicsList) {
-            std::string viewName = (i->data(1).toString()).toStdString();
-            App::DocumentObject* dObj = getAppDocument()->getObject(viewName.c_str());
-            if (dObj == nullptr) {
-                //need to remove from group/scene
-                QGraphicsItemGroup* grp = i->group();
-                if (grp) {
-                    grp->removeFromGroup(i);
-                }
-                if (i->parentItem()) {    //not top level
-                    i->setParentItem(0);
-                }
-                if (i->scene()) {
-                    i->scene()->removeItem(i);
-                }
-                //should delete i too to prevent leak?  might be garbage pointer, though. 
-                //add to delete list and delete outside of loop
-           } else {
-               QGIView* v = static_cast<QGIView*>(i);
-               cleanItems.push_back(v);
-           }
-        }
-//    qgvpClean = cleanItems.size();
-    m_view->setViews(cleanItems);       
-//    Base::Console().Message("Log - MDIVP::updateDrawing pruning: docObjs: %d views in: %d valid views: %d views out: %d\n",
-//                            dpIn,qgvpIn,qgvpValid, qgvpClean);
     }
     
     // Update all the QGIVxxxx
@@ -1004,7 +947,7 @@ void MDIViewPage::selectFeature(App::DocumentObject *obj, const bool isSelected)
     if (hatchObj) {                                                    //Hatch does not have a QGIV of it's own. mark parent as selected.
         objCopy = hatchObj->getSourceView();                           //possible to highlight subObject?
     }
-    QGIView *view = m_view->findView(objCopy);
+    QGIView *view = m_view->findQViewForDocObj(objCopy);
 
     blockSelection(true);
     if(view) {

src/Mod/TechDraw/Gui/MDIViewPage.h

@@ -65,7 +65,6 @@ public:
     void attachTemplate(TechDraw::DrawTemplate *obj);
     void updateTemplate(bool force = false);
     void updateDrawing(bool force = false);
-    void removeView(QGIView *view);
     
     bool onMsg(const char* pMsg,const char** ppReturn);
     bool onHasMsg(const char* pMsg) const;

src/Mod/TechDraw/Gui/QGIView.cpp

@@ -287,6 +287,8 @@ const char * QGIView::getViewName() const
 
 TechDraw::DrawView * QGIView::getViewObject() const
 {
+    //DocumentObject* obj = doc->getObject(viewName.c_str());
+    //TechDraw::DrawView* dv = static_cast<TechDraw::DrawView*>(obj);
     return viewObj;
 }
 

src/Mod/TechDraw/Gui/QGVPage.cpp

@@ -164,18 +164,29 @@ void QGVPage::drawBackground(QPainter *p, const QRectF &)
     p->drawRect(poly.boundingRect());
 
     p->restore();
-
 }
 
-int QGVPage::addView(QGIView *view)
+//! retrieve the QGIView objects currently in the scene
+std::vector<QGIView *> QGVPage::getViews() const
+{
+    std::vector<QGIView*> result;
+    QList<QGraphicsItem*> items = scene()->items();
+    for (auto& v:items) {
+        QGIView* qv = dynamic_cast<QGIView*>(v);
+        if (qv != nullptr) {
+            result.push_back(qv);
+        }
+    }
+    return result;
+}
+
+int QGVPage::addQView(QGIView *view)
 {
     auto ourScene( scene() );
     assert(ourScene);
 
     ourScene->addItem(view);
 
-    views.push_back(view);
-
     // Find if it belongs to a parent
     QGIView *parent = 0;
     parent = findParent(view);
@@ -184,78 +195,51 @@ int QGVPage::addView(QGIView *view)
                     Rez::guiX(view->getViewObject()->Y.getValue() * -1));
 
     if(parent) {
-        // Transfer the child vierw to the parent
+//        // Transfer the child vierw to the parent
-        QPointF posRef(0.,0.);
+//        QPointF posRef(0.,0.);
 
-        QPointF mapPos = view->mapToItem(parent, posRef);              //setPos is called later.  this doesn't do anything?
+//        QPointF mapPos = view->mapToItem(parent, posRef);              //setPos is called later.  this doesn't do anything?
-        view->moveBy(-mapPos.x(), -mapPos.y());
+//        view->moveBy(-mapPos.x(), -mapPos.y());
 
         parent->addToGroup(view);
     }
 
     view->setPos(viewPos);
 
-    return views.size();
+    return 0;
 }
 
-int QGVPage::removeView(QGIView *view)
+int QGVPage::removeQView(QGIView *view)
 {
-
+    if (view != nullptr) {
-    std::vector<QGIView *> qviews = views;
+        removeQViewFromScene(view);
-    std::vector<QGIView *> newViews;
-    
-    std::vector<QGIView *>::iterator qvit = qviews.begin();
-    std::vector<QGIView *>::iterator qvDel = qviews.end();
-    
-    for (; qvit != qviews.end(); qvit++) {
-        if ((*qvit) == view) {
-            qvDel = qvit;
-            break;
-        }
-    }
-    
-    if (qvDel == qviews.end()) {     //didn't find view in views
-        return views.size();
-    }
-
-    removeViewFromScene(view);
-
-    qviews.erase(qvDel);
-    views = qviews;
         delete view;
-
+    }
-    return views.size();
+    return 0;
 }
 
-int QGVPage::removeView(const TechDraw::DrawView* dv)
+int QGVPage::removeQViewByDrawView(const TechDraw::DrawView* dv)
 {
-    std::vector<QGIView *> newViews;
+    std::vector<QGIView*> items = getViews();
-    QList<QGraphicsItem *> items = scene()->items();
     QString qsName = QString::fromUtf8(dv->getNameInDocument());
     bool found = false;
     QGIView* ourItem = nullptr;
     for (auto& i:items) {
         if (qsName == i->data(1).toString()) {          //is there really a QGIV for this DV in scene?
             found = true;
-            ourItem = static_cast<QGIView*>(i);
+            ourItem = i;
             break;
         }
     }
     if (found) {
-        for (auto&v :views) {
+        removeQViewFromScene(ourItem);
-            if (ourItem != v) {
-                newViews.push_back(v);
-            }
-        }
-        removeViewFromScene(ourItem);
         delete ourItem;
-        views = newViews;
     }
 
-    return views.size();
+    return 0;
 }
 
-void QGVPage::removeViewFromScene(QGIView *view)
+void QGVPage::removeQViewFromScene(QGIView *view)
 {
     QGraphicsItemGroup* grp = view->group();
     if (grp) {
@@ -278,7 +262,7 @@ QGIView * QGVPage::addViewPart(TechDraw::DrawViewPart *part)
 
     viewPart->setViewPartFeature(part);
 
-    addView(viewPart);
+    addQView(viewPart);
     return viewPart;
 }
 
@@ -288,7 +272,7 @@ QGIView * QGVPage::addViewSection(TechDraw::DrawViewPart *part)
 
     viewSection->setViewPartFeature(part);
 
-    addView(viewSection);
+    addQView(viewSection);
     return viewSection;
 }
 
@@ -296,7 +280,7 @@ QGIView * QGVPage::addProjectionGroup(TechDraw::DrawProjGroup *view) {
     auto qview( new QGIProjGroup );
 
     qview->setViewFeature(view);
-    addView(qview);
+    addQView(qview);
     return qview;
 }
 
@@ -305,7 +289,7 @@ QGIView * QGVPage::addDrawView(TechDraw::DrawView *view)
     auto qview( new QGIView );
 
     qview->setViewFeature(view);
-    addView(qview);
+    addQView(qview);
     return qview;
 }
 
@@ -314,31 +298,28 @@ QGIView * QGVPage::addDrawViewCollection(TechDraw::DrawViewCollection *view)
     auto qview( new QGIViewCollection );
 
     qview->setViewFeature(view);
-    addView(qview);
+    addQView(qview);
     return qview;
 }
 
 // TODO change to (App?) annotation object  ??
 QGIView * QGVPage::addDrawViewAnnotation(TechDraw::DrawViewAnnotation *view)
 {
-    // This essentially adds a null view feature to ensure view size is consistent
     auto qview( new QGIViewAnnotation );
 
     qview->setViewAnnoFeature(view);
 
-    addView(qview);
+    addQView(qview);
     return qview;
 }
 
 QGIView * QGVPage::addDrawViewSymbol(TechDraw::DrawViewSymbol *view)
 {
-    //QPoint qp(view->X.getValue(),view->Y.getValue());
-    // This essentially adds a null view feature to ensure view size is consistent
     auto qview( new QGIViewSymbol );
 
     qview->setViewFeature(view);
 
-    addView(qview);
+    addQView(qview);
     return qview;
 }
 
@@ -349,7 +330,7 @@ QGIView * QGVPage::addDrawViewClip(TechDraw::DrawViewClip *view)
     qview->setPosition(Rez::guiX(view->X.getValue()), Rez::guiX(view->Y.getValue()));
     qview->setViewFeature(view);
 
-    addView(qview);
+    addQView(qview);
     return qview;
 }
 
@@ -359,18 +340,17 @@ QGIView * QGVPage::addDrawViewSpreadsheet(TechDraw::DrawViewSpreadsheet *view)
 
     qview->setViewFeature(view);
 
-    addView(qview);
+    addQView(qview);
     return qview;
 }
 
 QGIView * QGVPage::addDrawViewImage(TechDraw::DrawViewImage *view)
 {
-    //QPoint qp(view->X.getValue(),view->Y.getValue());
     auto qview( new QGIViewImage );
 
     qview->setViewFeature(view);
 
-    addView(qview);
+    addQView(qview);
     return qview;
 }
 
@@ -384,12 +364,6 @@ QGIView * QGVPage::addViewDimension(TechDraw::DrawViewDimension *dim)
 
     dimGroup->setViewPartFeature(dim);
 
-    // TODO consider changing dimension feature to use another property for label position
-    // Instead of calling addView - the view must for now be added manually
-
-    //Note dimension X,Y is different from other views -> can't use addView
-    views.push_back(dimGroup);
-
     // Find if it belongs to a parent
     QGIView *parent = 0;
     parent = findParent(dimGroup);
@@ -412,10 +386,11 @@ void QGVPage::addDimToParent(QGIViewDimension* dim, QGIView* parent)
     dim->setZValue(ZVALUE::DIMENSION);
 }
 
-QGIView * QGVPage::findView(App::DocumentObject *obj) const
+//! find the graphic for a DocumentObject
+QGIView * QGVPage::findQViewForDocObj(App::DocumentObject *obj) const
 {
   if(obj) {
-    const std::vector<QGIView *> qviews = views;
+    const std::vector<QGIView *> qviews = getViews();
     for(std::vector<QGIView *>::const_iterator it = qviews.begin(); it != qviews.end(); ++it) {
           if(strcmp(obj->getNameInDocument(), (*it)->getViewName()) == 0)
               return *it;
@@ -424,9 +399,27 @@ QGIView * QGVPage::findView(App::DocumentObject *obj) const
     return 0;
 }
 
+//! find the graphic for DocumentObject with name
+QGIView* QGVPage::getQGIVByName(std::string name)
+{
+    QList<QGraphicsItem*> qgItems = scene()->items();
+    QList<QGraphicsItem*>::iterator it = qgItems.begin();
+    for (; it != qgItems.end(); it++) {
+        QGIView* qv = dynamic_cast<QGIView*>((*it));
+        if (qv) {
+            const char* qvName = qv->getViewName();
+            if(name.compare(qvName) == 0) {
+                return (qv);
+            }
+        }
+    }
+    return nullptr;
+}
+
+
 QGIView * QGVPage::findParent(QGIView *view) const
 {
-    const std::vector<QGIView *> qviews = views;
+    const std::vector<QGIView *> qviews = getViews();
     TechDraw::DrawView *myView = view->getViewObject();
 
     //If type is dimension we check references first

src/Mod/TechDraw/Gui/QGVPage.h

@@ -74,17 +74,20 @@ public:
     QGIView * addDrawViewImage(TechDraw::DrawViewImage *view);
 
 
-    QGIView * findView(App::DocumentObject *obj) const;
+    QGIView* findQViewForDocObj(App::DocumentObject *obj) const;
-    QGIView * findParent(QGIView *) const;
+    QGIView* getQGIVByName(std::string name);
+    QGIView* findParent(QGIView *) const;
 
     void addDimToParent(QGIViewDimension* dim, QGIView* parent);
-    const std::vector<QGIView *> & getViews() const { return views; }
+//    const std::vector<QGIView *> & getViews() const { return views; }    //only used in MDIVP
+    std::vector<QGIView *> getViews() const;   //only used in MDIVP
 
-    int addView(QGIView * view);
+    int addQView(QGIView * view);
-    int removeView(QGIView *view);
+    int removeQView(QGIView *view);
-    int removeView(const TechDraw::DrawView* dv);
+    int removeQViewByDrawView(const TechDraw::DrawView* dv);
+    void removeQViewFromScene(QGIView *view);
 
-    void setViews(const std::vector<QGIView *> &view) {views = view; }
+    //void setViews(const std::vector<QGIView *> &view) {views = view; }
     void setPageTemplate(TechDraw::DrawTemplate *pageTemplate);
 
     QGITemplate * getTemplate() const;
@@ -112,10 +115,9 @@ protected:
     static QColor PreselectColor;
     QColor getBackgroundColor();
     
-    void removeViewFromScene(QGIView *view);
 
     QGITemplate *pageTemplate;
-    std::vector<QGIView *> views;
+//    std::vector<QGIView *> views;                          //<<< why?  scene already has a list of all the views.
 
 private:
     RendererType m_renderer;

src/Mod/TechDraw/Gui/ViewProviderDrawingView.cpp

@@ -137,6 +137,10 @@ void ViewProviderDrawingView::hide(void)
 
 QGIView* ViewProviderDrawingView::getQView(void)
 {
+    //TODO: vp can get its MDIView with 1 call getActiveView()?
+    //      instead of going back to App side an up tree and back to Gui?
+    //MDIVPage* mdivp = static_cast<MDIVPage*>(getActiveView());
+    //qView = mdivp->getQGVPage()->findQViewForDocObj(getViewObject());
     QGIView *qView = nullptr;
     if (m_docReady){
         TechDraw::DrawView* dv = getViewObject();
@@ -147,7 +151,7 @@ QGIView* ViewProviderDrawingView::getQView(void)
             if (dvp) {
                 if (dvp->getMDIViewPage()) {
                     if (dvp->getMDIViewPage()->getQGVPage()) {
-                        qView = dynamic_cast<QGIView *>(dvp->getMDIViewPage()->getQGVPage()->findView(getViewObject()));
+                        qView = dynamic_cast<QGIView *>(dvp->getMDIViewPage()->getQGVPage()->findQViewForDocObj(getViewObject()));
                     }
                 }
             }