From 69163cb0161c03af5c1dc2c2ebfd1247032e7465 Mon Sep 17 00:00:00 2001 From: WandererFan Date: Sun, 5 Feb 2017 17:59:20 -0500 Subject: [PATCH] Fix #2888 Crash on Delete View --- src/Mod/TechDraw/Gui/MDIViewPage.cpp | 113 ++++----------- src/Mod/TechDraw/Gui/MDIViewPage.h | 1 - src/Mod/TechDraw/Gui/QGIView.cpp | 4 +- src/Mod/TechDraw/Gui/QGVPage.cpp | 133 +++++++++--------- src/Mod/TechDraw/Gui/QGVPage.h | 20 +-- .../TechDraw/Gui/ViewProviderDrawingView.cpp | 6 +- 6 files changed, 110 insertions(+), 167 deletions(-) diff --git a/src/Mod/TechDraw/Gui/MDIViewPage.cpp b/src/Mod/TechDraw/Gui/MDIViewPage.cpp index ad07b724c..79045882a 100644 --- a/src/Mod/TechDraw/Gui/MDIViewPage.cpp +++ b/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) - // WF: this should be fixed now with onDeletedObject signal from App side? - // WF: the QGVP views list may still not be 100% reliable - // TODO: build list of QGIV's from scene everytime? - //logging counters -// int qgvpIn = 0; -// int qgvpValid = 0; -// int qgvpClean = 0; -// int dpIn = 0; - const std::vector &graphicsList = m_view->getViews(); -// qgvpIn = graphicsList.size(); - const std::vector &pageChildren = m_vpPage->getDrawPage()->Views.getValues(); - - // Count total # DocumentObjects in Page - unsigned int docObjCount = 0; - for(std::vector::const_iterator it = pageChildren.begin(); it != pageChildren.end(); ++it) { - App::DocumentObject *docObj = *it; - if(docObj->getTypeId().isDerivedFrom(TechDraw::DrawViewCollection::getClassTypeId())) { - TechDraw::DrawViewCollection *collection = dynamic_cast(docObj); - docObjCount += collection->countChildren(); // Include self + // get all the DrawViews for this page, including the second level ones + // if we ever have collections of collections, we'll need to revisit this + std::vector pChildren = m_vpPage->getDrawPage()->Views.getValues(); + std::vector appendChildren; + for (auto& pc: pChildren) { + if(pc->getTypeId().isDerivedFrom(TechDraw::DrawViewCollection::getClassTypeId())) { + TechDraw::DrawViewCollection *collect = dynamic_cast(pc); + std::vector cChildren = collect->Views.getValues(); + appendChildren.insert(std::end(appendChildren), std::begin(cChildren), std::end(cChildren)); } - docObjCount += 1; } -// dpIn = docObjCount; - - - //TODO: should prune QGVP.views first always, then check if view in Page missing QGIVP - // this makes assumption that = numbers mean everythign is OK, but could be double failure - 1 extra QGIV, 1 DV missing graphics! - - if(graphicsList.size() < docObjCount) { - // there are more DocumentObjects than graphical representations (QGIVxxxx's) - // Find which DocumentObjects have no graphical representation (QGIVxxxx) - // 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 notFnd; - findMissingViews(pageChildren, notFnd); - for(std::vector::const_iterator it = notFnd.begin(); it != notFnd.end(); ++it) { - attachView(*it); + pChildren.insert(std::end(pChildren),std::begin(appendChildren),std::end(appendChildren)); + + // if dv doesn't have a graphic, make one + for (auto& dv: pChildren) { + QGIView* qv = m_view->findQViewForDocObj(dv); + if (qv == nullptr) { + attachView(dv); } - } 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 newGraphicsList; - QList 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; - } - } + } + + // if qView doesn't have a Feature, delete it + std::vector qvs = m_view->getViews(); + App::Document* doc = getAppDocument(); + for (auto& qv: qvs) { + App::DocumentObject* obj = doc->getObject(qv->getViewName()); + if (obj == nullptr) { + m_view->removeQView(qv); } -// qgvpValid = newGraphicsList.size(); - //newGraphicsList now only contains valid QGIV's - //now prune the ones without docObjs - std::vector 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(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) { diff --git a/src/Mod/TechDraw/Gui/MDIViewPage.h b/src/Mod/TechDraw/Gui/MDIViewPage.h index 13fab0db8..0018bf61c 100644 --- a/src/Mod/TechDraw/Gui/MDIViewPage.h +++ b/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; diff --git a/src/Mod/TechDraw/Gui/QGIView.cpp b/src/Mod/TechDraw/Gui/QGIView.cpp index f22708c40..6697d6435 100644 --- a/src/Mod/TechDraw/Gui/QGIView.cpp +++ b/src/Mod/TechDraw/Gui/QGIView.cpp @@ -287,7 +287,9 @@ const char * QGIView::getViewName() const TechDraw::DrawView * QGIView::getViewObject() const { - return viewObj; + //DocumentObject* obj = doc->getObject(viewName.c_str()); + //TechDraw::DrawView* dv = static_cast(obj); + return viewObj; } void QGIView::setViewFeature(TechDraw::DrawView *obj) diff --git a/src/Mod/TechDraw/Gui/QGVPage.cpp b/src/Mod/TechDraw/Gui/QGVPage.cpp index f49f16bdc..c9ed642e5 100644 --- a/src/Mod/TechDraw/Gui/QGVPage.cpp +++ b/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 QGVPage::getViews() const +{ + std::vector result; + QList items = scene()->items(); + for (auto& v:items) { + QGIView* qv = dynamic_cast(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 - QPointF posRef(0.,0.); +// // Transfer the child vierw to the parent +// QPointF posRef(0.,0.); - QPointF mapPos = view->mapToItem(parent, posRef); //setPos is called later. this doesn't do anything? - view->moveBy(-mapPos.x(), -mapPos.y()); +// QPointF mapPos = view->mapToItem(parent, posRef); //setPos is called later. this doesn't do anything? +// 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) { - - std::vector qviews = views; - std::vector newViews; - - std::vector::iterator qvit = qviews.begin(); - std::vector::iterator qvDel = qviews.end(); - - for (; qvit != qviews.end(); qvit++) { - if ((*qvit) == view) { - qvDel = qvit; - break; - } + if (view != nullptr) { + removeQViewFromScene(view); + delete view; } - - 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 newViews; - QList items = scene()->items(); + std::vector items = getViews(); 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(i); + ourItem = i; break; } } if (found) { - for (auto&v :views) { - if (ourItem != v) { - newViews.push_back(v); - } - } - removeViewFromScene(ourItem); + removeQViewFromScene(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 qviews = views; + const std::vector qviews = getViews(); for(std::vector::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 qgItems = scene()->items(); + QList::iterator it = qgItems.begin(); + for (; it != qgItems.end(); it++) { + QGIView* qv = dynamic_cast((*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 qviews = views; + const std::vector qviews = getViews(); TechDraw::DrawView *myView = view->getViewObject(); //If type is dimension we check references first diff --git a/src/Mod/TechDraw/Gui/QGVPage.h b/src/Mod/TechDraw/Gui/QGVPage.h index 7223650a2..0591506a5 100644 --- a/src/Mod/TechDraw/Gui/QGVPage.h +++ b/src/Mod/TechDraw/Gui/QGVPage.h @@ -74,17 +74,20 @@ public: QGIView * addDrawViewImage(TechDraw::DrawViewImage *view); - QGIView * findView(App::DocumentObject *obj) const; - QGIView * findParent(QGIView *) const; + QGIView* findQViewForDocObj(App::DocumentObject *obj) const; + QGIView* getQGIVByName(std::string name); + QGIView* findParent(QGIView *) const; void addDimToParent(QGIViewDimension* dim, QGIView* parent); - const std::vector & getViews() const { return views; } +// const std::vector & getViews() const { return views; } //only used in MDIVP + std::vector getViews() const; //only used in MDIVP - int addView(QGIView * view); - int removeView(QGIView *view); - int removeView(const TechDraw::DrawView* dv); + int addQView(QGIView * view); + int removeQView(QGIView *view); + int removeQViewByDrawView(const TechDraw::DrawView* dv); + void removeQViewFromScene(QGIView *view); - void setViews(const std::vector &view) {views = view; } + //void setViews(const std::vector &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 views; +// std::vector views; //<<< why? scene already has a list of all the views. private: RendererType m_renderer; diff --git a/src/Mod/TechDraw/Gui/ViewProviderDrawingView.cpp b/src/Mod/TechDraw/Gui/ViewProviderDrawingView.cpp index 8ad70ec48..2e1eac9da 100644 --- a/src/Mod/TechDraw/Gui/ViewProviderDrawingView.cpp +++ b/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(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(dvp->getMDIViewPage()->getQGVPage()->findView(getViewObject())); + qView = dynamic_cast(dvp->getMDIViewPage()->getQGVPage()->findQViewForDocObj(getViewObject())); } } }