Bug 425156 - Template typenames in attribute and operation parameter types don't work
Summary: Template typenames in attribute and operation parameter types don't work
Status: ASSIGNED
Alias: None
Product: umbrello
Classification: Applications
Component: general (show other bugs)
Version: Git
Platform: Other Linux
: NOR major
Target Milestone: ---
Assignee: Umbrello Development Group
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-08-09 11:32 UTC by Robert Hairgrove
Modified: 2020-08-16 07:58 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
testcase (10.27 KB, application/x-uml)
2020-08-13 11:31 UTC, Ralf Habacker
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hairgrove 2020-08-09 11:32:00 UTC
SUMMARY
Parameters of an operation in a class with templates cannot define
template typenames as their types without Umbrello creating spurious
additional classes at global scope.

STEPS TO REPRODUCE
1. Start a new project.
2. Under the folder "Logical View", create three nested folders named
   PKG1, PKG2, and PKG3. Under PKG3, create a new class "ClassBase" (for example).
3. Add a template to ClassBase "CharType" (for example).
4. Add an operation to ClassBase with a parameter called "param". Set the parameter
   type to "CharType".

OBSERVED RESULT
In the operation properties dialog's parameter list, the following is
displayed: "param :" (empty type). When the same parameter "param" is selected and "Properties"
is clicked, the type has changed to "PKG1::PKG2::PKG3::ClassBase::CharType".
If "OK" is clicked, there is a newly-created class called "PKG1::PKG2::PKG3::ClassBase::CharType"
at global scope in the tree view.

EXPECTED RESULT
The parameter type should simply show the template typename; no extra class
or datatype should be created on the fly.

SOFTWARE/OS VERSIONS
Linux Ubuntu 18.04.4
Qt Version: 5.x

ADDITIONAL INFORMATION
The reason for this is that in the function UMLAttribute::toString, the
call to "UMLClassifier *type = UMLClassifierListItem::getType();" returns
NULL instead of a pointer to the template. However, UMLTemplate is not a
descendent of UMLClassifier, but UMLClassifierListItem, so the dynamic_cast 
in the body of the getType() function fails.

FIX:
1 - attribute.cpp needs to #include "template.h".
2 - If UMLClassifier *type == NULL after the above call, adding a check 
for m_pSecondary != NULL && m_pSecondary->asUMLTemplate() != NULL, then calling name() will
fetch the correct type.

Additional testing should be done to ensure that the XMI document is
correctly updated.

RELATED TO: Bug 425009
Comment 1 Bug Janitor Service 2020-08-09 12:13:40 UTC
A possibly relevant merge request was started @ https://invent.kde.org/sdk/umbrello/-/merge_requests/9
Comment 2 Robert Hairgrove 2020-08-09 12:55:27 UTC
XMI doesn't seem to understand this. Loading and saving to/from XMI does not set the proper data type as template typename.

At the moment, I cannot tell where the parsing fails, so maybe this merge request was premature?
Comment 3 Robert Hairgrove 2020-08-10 08:15:35 UTC
The same problem exists in UMLOperation::saveToXMI1() -- namely, the assumption being that all attributes have a type which is a descendent of UMLClassifier. If it is a template, then this will not work because templates descend from UMLClassifierListItem.

Is there any reason why the type must be a UMLClassifier?
Comment 4 Ralf Habacker 2020-08-10 09:06:05 UTC
(In reply to Robert Hairgrove from comment #3)
> Is there any reason why the type must be a UMLClassifier?

Types may be classes and therefore need to be a UMLClassifier
Comment 5 Ralf Habacker 2020-08-10 09:07:36 UTC
(In reply to Robert Hairgrove from comment #2)
> XMI doesn't seem to understand this. Loading and saving to/from XMI does not
> set the proper data type as template typename.
> 
> At the moment, I cannot tell where the parsing fails, so maybe this merge
> request was premature?
If you edit the merge request you will see a hint that you can prefix the title with WIP: to indicate that this merge request is a "work in progress".
Comment 6 Ralf Habacker 2020-08-10 09:31:21 UTC
(In reply to Bug Janitor Service from comment #1)
> A possibly relevant merge request was started @
> https://invent.kde.org/sdk/umbrello/-/merge_requests/9

The mr shows 6 commits where it should have only one. There seems to be a rebasing problem with release/20.08. 

If your git remote setup is as described at https://community.kde.org/Infrastructure/GitLab#Submitting_a_merge_request you can clean up your branch by running.

git checkout template_typenames_fix

This should show the following commits:

commit 41a731ee3f8cc93bede427cf6a4ee68611f87cef (HEAD -> template_typenames_fix, bhg/template_typenames_fix)
Author: Robert Hairgrove <code@roberthairgrove.com>
Date:   Sun Aug 9 14:07:23 2020 +0200

    Fix for template typenames as attribute types and operation parameter types

commit 839314a652fb70da65e42491a50d1da8e03c3e12 (bhg/release/20.08)
Merge: d53a0e78d 47cbb2052
Author: Robert Hairgrove <code@roberthairgrove.com>
Date:   Fri Aug 7 10:43:26 2020 +0200

    Edited CMakeLists.txt

commit d53a0e78d8fc5eb54d9a9c269a92e6c9869b3822
Merge: ea001bcd1 6cb3d4974
Author: Robert Hairgrove <code@roberthairgrove.com>
Date:   Fri Aug 7 10:24:41 2020 +0200

    Merge branch 'release/20.08' of https://invent.kde.org/sdk/umbrello into release/20.08

commit 6cb3d49749c4e460cf34d777472cf00f5946ce43 (origin/release/20.08, release/20.08)
Author: Robert Hairgrove <code@roberthairgrove.com>
Date:   Mon Aug 3 22:47:39 2020 +0200

    New feature allowing word wrapping in use case texts when resizing and entering line breaks
    
    BUG: 424863
    FIXED-IN:2.32.01 (KDE releases 20.08.1)


Now cleanup your branch with

git rebase -i HEAD~3

In the editor, which is then opened, remove all lines except the line containing "Fix for template typenames as" and save

Now run

git rebase origin/release/20.08

Then git log should show 

commit 9d426ca2d29289f8d11b3aeef62fba50ed415c40 (HEAD -> template_typenames_fix)
Author: Robert Hairgrove <code@roberthairgrove.com>
Date:   Sun Aug 9 14:07:23 2020 +0200

    Fix for template typenames as attribute types and operation parameter types

commit 6cb3d49749c4e460cf34d777472cf00f5946ce43 (origin/release/20.08, release/20.08)
Author: Robert Hairgrove <code@roberthairgrove.com>
Date:   Mon Aug 3 22:47:39 2020 +0200

    New feature allowing word wrapping in use case texts when resizing and entering line breaks
    
    BUG: 424863
    FIXED-IN:2.32.01 (KDE releases 20.08.1)

commit b0705c5cdf305d9ff4da36ea6cb899e465fa3ff3
Author: Christoph Feck <cfeck@kde.org>
Date:   Wed Aug 5 09:24:57 2020 +0200

    GIT_SILENT Upgrade release service version to 20.08.0.
...

now update the branch used for the merge request by:

git push -f fork
Comment 7 Robert Hairgrove 2020-08-10 09:58:33 UTC
Thank you, Ralf -- sorry for all the trouble I have caused.

I hope it is OK now. I added "WIP:" to the title.
Comment 8 Robert Hairgrove 2020-08-10 11:16:07 UTC
(In reply to Ralf Habacker from comment #4)
> (In reply to Robert Hairgrove from comment #3)
> > Is there any reason why the type must be a UMLClassifier?
> 
> Types may be classes and therefore need to be a UMLClassifier

The problem is, we can select a template from the "type" list for the types of lots of different things. Everything else in the list is either a datatype or a class name. Indeed, the type of a template parameter is "class", unless some integral expression or type with external linkage is specified.

So template parameters SHOULD be considered classifiers, but they aren't -- at least not in Umbrello.

What I am looking at now is how to properly load and save these in the XMI file. I've looked at XMI files produced by other applications, for example Modelio, and I have the XMI reference as a PDF file. But every other UML tool uses a different format.

How interchangeable does this have to be? Code generation and reverse engineering needs to be looked at here as well. Since templates have been part of UML for some time, I don't see why we cannot make this work somehow.
Comment 9 Robert Hairgrove 2020-08-10 14:05:32 UTC
Thank you again for all your help, Ralf!

I have a fix now for this -- XMI loads and saves OK, code generation and import seems to work. The only thing I noticed is that the generated code, instead of making nested namespaces, makes three real folders (PKG1/PKG2/PKG3) and constructs a new namespace "PKG1_PKG2_PKG3". I shall try this with packages instead of folders.

However, this has nothing to do with templates, so I won't delve into this yet.

A question about the fix:
I needed to make UMLOperation a friend class of UMLAttribute in order to access the m_pSecondary member on which the id() function is called. Since it is a protected member, and UMLOperation does all the reading and writing from/to XMI, I changed the header "attribute.h" to make the friend. Otherwise, there is no change to any of the header files. Only other changes were a few lines in "operation.cpp" and "classifierlistitem.cpp".

Can I make the commits to the same branch which is used for the merge request? I'll leave the "WIP:" for now. Or do I need to close the current merge request and make another one?
Comment 10 Robert Hairgrove 2020-08-11 08:54:57 UTC
Added another commit to existing merge request yesterday.
Comment 11 Robert Hairgrove 2020-08-11 13:46:48 UTC
Removed the WIP. Added two commits today.
Comment 12 Robert Hairgrove 2020-08-13 06:16:02 UTC
If no one has any requests for changes, could somebody with write access please merge this for me?

There should be an additional "FIXED-IN" line in the comments, but I didn't know exactly what version to write. I saw a reference to "20.08.1" somewhere, but I don't know if that would be correct.
Comment 13 Ralf Habacker 2020-08-13 06:41:30 UTC
(In reply to Robert Hairgrove from comment #12)
You wrote in the merge request: 
> Needs testing in other modules, e.g. code generation and XMI load/save operations.

Then this merge request is not ready for including in the stable release.

New features that require more testing, or for which it is not yet known what side effects exist, have been added to the master branch as "unstable features" that are disabled in stable releases. See https://invent.kde.org/sdk/umbrello/-/blob/master/CMakeLists.txt#L205 for details. 

For example, there is a feature that displays the documentation of the class in the class widget, but still has drawing problems and therefore is wrapped with the CPP macro ENABLE_WIDGET_SHOW_DOC
and is only enabled in unstable builds ( UMBRELLO_VERSION_PATCH >= 70).
Comment 14 Robert Hairgrove 2020-08-13 07:07:41 UTC
Well, I did test these things, and it seems to work OK:

1. Saving and loading from XMI works, AFAICT.

2. Code generation works, as far as the templates go in C++; I don't know enough about other languages to know what side effects there might be -- in Java, for instance. At least now I can assign a template as the type of an attribute or an operation parameter or return value (for operations, this was already in place, but it did not work correctly).

If anyone sees any problems I may have overlooked, then I will be happy to change it. It is not really a new feature, but a bug fix for what was failing before (and was in the stable release).
Comment 15 Ralf Habacker 2020-08-13 11:10:01 UTC
(In reply to Robert Hairgrove from comment #12)

> There should be an additional "FIXED-IN" line in the comments, but I didn't
> know exactly what version to write. I saw a reference to "20.08.1"
> somewhere, but I don't know if that would be correct.

Umbrello use a different versioning, which would be for example
 
FIXED-IN:2.32.01 (KDE release 20.08.1)

I'm going to try that with a different merge request.
Comment 16 Ralf Habacker 2020-08-13 11:15:42 UTC
(In reply to Ralf Habacker from comment #15)
> I'm going to try that with a different merge request.
It works as expected: see latest commits in release/20.08 branch how it should be used.
Comment 17 Ralf Habacker 2020-08-13 11:31:05 UTC
Created attachment 130847 [details]
testcase
Comment 18 Ralf Habacker 2020-08-13 11:39:34 UTC
(In reply to Ralf Habacker from comment #17)
> Created attachment 130847 [details]
> testcase

With your patches applied and after loading the testcase in the tree view now I see

operation(param:undef)

I need to reassign the type "CharType" to the parameter 'param' to see 

operation(param:CharType)

which looks like what you expects to see.

> The parameter type should simply show the template typename; no extra class
> or datatype should be created on the fly.
Comment 19 Robert Hairgrove 2020-08-13 11:53:44 UTC
Yes, this is what I see as well. Of course, the XMI file was malformed because there is no datatype "pkg1::pkg2::pkg3::ClassBase::CharType" listed in the datatypes. It was probably created without the fix in place.

If you save the file after selecting "CharType" in the operation parameter properties, then reopen it in Umbrello, it should have the correct type.
Comment 20 Robert Hairgrove 2020-08-13 11:56:43 UTC
In the fixed version, the "type" attribute of the parameter now contains the xmi.id value of the template (as it should).
Comment 21 Ralf Habacker 2020-08-13 12:21:55 UTC
(In reply to Robert Hairgrove from comment #20)
> In the fixed version, the "type" attribute of the parameter now contains the
> xmi.id value of the template (as it should).

I can see that: 

<UML:TemplateParameter xmi.id="ur8dhzHRrZNNa" ... />
...
<UML:Parameter xmi.id="u7pOztaTUrnyB" type="ur8dhzHRrZNNa" .../>
Comment 22 Ralf Habacker 2020-08-13 12:54:25 UTC
(In reply to Robert Hairgrove from comment #8)
> (In reply to Ralf Habacker from comment #4)
> > (In reply to Robert Hairgrove from comment #3)
> > > Is there any reason why the type must be a UMLClassifier?
> > 
> > Types may be classes and therefore need to be a UMLClassifier
> 
> The problem is, we can select a template from the "type" list for the types
> of lots of different things. Everything else in the list is either a
> datatype or a class name. Indeed, the type of a template parameter is
> "class", unless some integral expression or type with external linkage is
> specified.
> 
> So template parameters SHOULD be considered classifiers, but they aren't --
> at least not in Umbrello.

This probably comes from the fact, that classifiers do not have types, instead they have super classes. 

> What I am looking at now is how to properly load and save these in the XMI
> file. I've looked at XMI files produced by other applications, for example
> Modelio, and I have the XMI reference as a PDF file. But every other UML
> tool uses a different format.

Looking at the xmi 1.4 dtd at https://www.omg.org/spec/UML/20010967/01-02-16.dtd there is 

<!ELEMENT UML:ModelElement.templateParameter (UML:TemplateParameter)*>

<!ELEMENT UML:TemplateParameter (%UML:TemplateParameterFeatures;)*>
<!ENTITY % UML:TemplateParameterFeatures 'XMI.extension |
   UML:TemplateParameter.template |
   UML:TemplateParameter.parameter |
   UML:TemplateParameter.defaultElement'>

<!ELEMENT UML:TemplateParameter.defaultElement (UML:ModelElement)*>

<!-- ========= UML:TemplateParameter ========= -->
<!ELEMENT UML:TemplateParameter.template (UML:ModelElement)*>

<!ELEMENT UML:TemplateParameter.parameter (UML:ModelElement|
   UML:GeneralizableElement|UML:Classifier|UML:Class|UML:AssociationClass|
   UML:DataType|UML:Primitive|UML:Enumeration|UML:ProgrammingLanguageDataType|
   UML:Interface|UML:Component|UML:Node|UML:Artifact|UML:Signal|UML:Exception|
   UML:UseCase|UML:Actor|UML:ClassifierRole|UML:ClassifierInState|
   UML:Subsystem|UML:Association|UML:AssociationRole|UML:Stereotype|
   UML:Collaboration|UML:Package|UML:Model|UML:Namespace|UML:Feature|
   UML:StructuralFeature|UML:Attribute|UML:BehavioralFeature|UML:Operation|
   UML:Method|UML:Reception|UML:AssociationEnd|UML:AssociationEndRole|
   UML:Constraint|UML:Relationship|UML:Generalization|UML:Dependency|
   UML:Abstraction|UML:Usage|UML:Binding|UML:Permission|UML:Flow|UML:Extend|
   UML:Include|UML:Parameter|UML:Comment|UML:EnumerationLiteral|
   UML:TagDefinition|UML:TaggedValue|UML:Instance|UML:Object|UML:LinkObject|
   UML:DataValue|UML:ComponentInstance|UML:NodeInstance|UML:SubsystemInstance|
   UML:UseCaseInstance|UML:Action|UML:CreateAction|UML:DestroyAction|
   UML:UninterpretedAction|UML:CallAction|UML:SendAction|UML:ActionSequence|
   UML:ReturnAction|UML:TerminateAction|UML:AttributeLink|UML:Link|
   UML:Argument|UML:LinkEnd|UML:Stimulus|UML:ExtensionPoint|UML:StateMachine|
   UML:ActivityGraph|UML:Event|UML:TimeEvent|UML:CallEvent|UML:SignalEvent|
   UML:ChangeEvent|UML:StateVertex|UML:State|UML:CompositeState|
   UML:SubmachineState|UML:SubactivityState|UML:SimpleState|UML:ActionState|
   UML:CallState|UML:ObjectFlowState|UML:FinalState|UML:Pseudostate|
   UML:SynchState|UML:StubState|UML:Transition|UML:Guard|UML:Message|
   UML:Interaction|UML:InteractionInstanceSet|UML:CollaborationInstanceSet|
   UML:Partition)*>

and this is implemented into umbrello

 UMLTemplate::saveToXMI1(QDomDocument& qDoc, QDomElement& qElement)
{
    QDomElement attributeElement = UMLObject::save1(QLatin1String("UML:TemplateParameter"), qDoc);
    if (m_pSecondary)
        attributeElement.setAttribute(QLatin1String("type"), Uml::ID::toString(m_pSecondary->id()));
    qElement.appendChild(attributeElement);
}

There is room for additional work.
Comment 23 Bug Janitor Service 2020-08-16 07:58:54 UTC
A possibly relevant merge request was started @ https://invent.kde.org/sdk/umbrello/-/merge_requests/12