Bug 117875

Created attachment 13808 [details]
C++ Header file mentioned in the bug report

Created attachment 13809 [details]
XMI file generated by Umbrello, as mentioned in the bug report

Created attachment 13810 [details]
First screenshot mentioned in the bug report

Created attachment 13811 [details]
Second screenshot mentioned in the bug report

I have found two bugs in Umbrello that were causing the behavior that I described in this report. I have fixed these bugs and tested the program (patches attached).

The bugs were surfacing because of the sequence of class declarations in zCDataTypes.H. Several classes are declared as part of namespace zMol, first by forward declarations, and then by full class declarations (e.g. class zCAnySpaceData). As a result of 2 bugs that I've found, these classes were not correctly included in programs's data structures, and resultantly were omitted from the XMI file during the save operation. Below are the details.

The direct reason why some classes were not saved was that they had the package attribute (UMLObject::m_pUMLPackage) set correctly to the name of the namespace in which they were declared, but did not belong to the list of children of that package (UMLPackage::m_objects). Because of that they were skipped in both UMLDoc::saveToXMI (umldoc.cpp line 1636), and in UMLPackage::saveToXMI (package.cpp lines 172-177, called from UMLDoc::saveToXMI, line 1656). Code in both saveToXMI methods is correct, and the problem is the m_pUMLPackage/m_objects anomaly.

That anomaly was caused by the bug in ClassImport::createUMLObject (classimport.cpp lines 167-169). Class zCAnySpaceData (and other like it) are processed through createUMLObject twice, first as a result of a forward declaration, and then as a result of a full declaration. As a result of a second bug, that I describe later, namespace was ignored for the forward declaration. During the second pass through createUMLObject, namespace was provided correctly, and this resulted in UMLObject being reassigned to a different UMLPackage. However, only UMLObject::m_pUMLPackage was assigned, and the object was not added to the UMLPackage::m_object list. That created the anomaly that was preventing object from being saved.

The patch (also attached as classimport.cpp.diff):
168c168,171
< 		o->setUMLPackage(parentPkg);
---
>             if (o->getUMLPackage())
>                 o->getUMLPackage()->removeObject(o);
>             o->setUMLPackage(parentPkg);
>             parentPkg->addObject(o);

As a side note, I am not sure that reassigning classes from one namespace to another is a correct behavior. If there are 2 identically named classes declared in 2 namespaces, they should be present in the tree as 2 distinct objects, rather than one object in the namespace that was seen last, as the code appears to be doing now.

The second bug is in CppTree2Uml::parseElaboratedTypeSpecifier (classparser/cpptree2uml.cpp, line 404). The code ignores the current namespace (m_currentNamespace[m_nsCtl], which results in class being assigned to the empty namespace (NULL). 

Patch (also attached as cpptree2uml.cpp.diff):

404c404
<     UMLObject *o = m_importer->createUMLObject( Uml::ot_Class, text );
---
>     UMLObject *o = m_importer->createUMLObject( Uml::ot_Class, text, m_currentNamespace[m_nsCnt] );

Please forgive me any deficiencies in this report; I am a newbie to Umbrello, KDE and UML. I am not sure whether I should leave this report as UNCONFIRMED (it is quite clear that the bugs exist, and where), or change it to RESOLVED. While the issue has been resolved for the copy of Umbrello that I have, the bugs persist in the publicly available release. (?)

Created attachment 13905 [details]
classimport.cpp.diff

Created attachment 13906 [details]
cpptree2uml.cpp.diff

Ah, those are the bugs I like - self fixing ;-)
Thanks for the excellent work tracking this down.

SVN commit 488516 by okellogg:

CppTree2Uml::parseElaboratedTypeSpecifier(): Supply the m_currentNamespace
 at the call to Import_Utils::createUMLObject().
Import_Utils::createUMLObject(): Add missing call to parentPkg->addObject(o).
Thanks to Maciej Puzio for providing a very well written PR and solution.
BUG:117875


 M  +1 -1      ChangeLog  
 M  +1 -0      THANKS  
 M  +1 -1      umbrello/classparser/cpptree2uml.cpp  
 M  +3 -0      umbrello/import_utils.cpp  


--- branches/KDE/3.5/kdesdk/umbrello/ChangeLog #488515:488516
@@ -1,7 +1,7 @@
 Version 1.5.1
 
 * Bugs fixed / wishes implemented (see http://bugs.kde.org)
-109963 117791
+109963 117875 117791
 
 Version 1.5
 
--- branches/KDE/3.5/kdesdk/umbrello/THANKS #488515:488516
@@ -52,6 +52,7 @@
 Dimitri Ognibene <ognibened @yahoo.it>
 Carsten Pfeiffer <pfeiffer @kde.org>
 Ivan Porres <iporres @abo.fi>
+Maciej Puzio <maciek @work.swmed.edu>
 Ruediger Ranft <kdebugs @rranft1.mail.htwm.de>
 John Ratke <jratke @comcast.net>
 Daniel Richard G. <skunk @iskunk.org>
--- branches/KDE/3.5/kdesdk/umbrello/umbrello/classparser/cpptree2uml.cpp #488515:488516
@@ -406,7 +406,7 @@
     QString text = typeSpec->text();
     kdDebug() << "CppTree2Uml::parseElaboratedTypeSpecifier: text is " << text << endl;
     text.remove(QRegExp("^class\\s+"));
-    UMLObject *o = Import_Utils::createUMLObject( Uml::ot_Class, text );
+    UMLObject *o = Import_Utils::createUMLObject(Uml::ot_Class, text, m_currentNamespace[m_nsCnt]);
     flushTemplateParams( static_cast<UMLClassifier*>(o) );
 }
 
--- branches/KDE/3.5/kdesdk/umbrello/umbrello/import_utils.cpp #488515:488516
@@ -200,7 +200,10 @@
             o = origType;
         }
     } else if (parentPkg && !bPutAtGlobalScope) {
+        if (o->getUMLPackage())
+            o->getUMLPackage()->removeObject(o);
         o->setUMLPackage(parentPkg);
+        parentPkg->addObject(o);
     }
     QString strippedComment = formatComment(comment);
     if (! strippedComment.isEmpty()) {