Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement message labels #251 #337

Merged
merged 7 commits into from
Oct 31, 2018
Merged

Implement message labels #251 #337

merged 7 commits into from
Oct 31, 2018

Conversation

tortmayr
Copy link
Collaborator

@tortmayr tortmayr commented Aug 8, 2018

Note: This PR reuses components which have been contributed with PR #18 for issue #16


This PR provides labels for messages which are displayed as stated in UML spec 2.5.1 (17.4.4.1). In particular it contains the following contributions:

@tortmayr tortmayr changed the title Implemen message labels #251 Implement message labels #251 Aug 8, 2018
Copy link
Collaborator

@planger planger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks! I briefly tested it and it already looks very well.

A few minor changes:

  • Update of operation name isn't reflected in label. To reproduce, add an operation in the signature --> Label is updated --> rename the operation --> label remains the same. Label is only updated when the name is changed or the signature object is replaced/removed itself. The same seems to be true when renaming arguments.
  • Please set the font size to the size of the lifeline header label; currently it is bigger than that.
  • It would be nice to avoid overflow of the label (over lifelines). I suggest to put a ... if it doesn't fit at the end of the label. Note that this will have to be automatically updated, if the lifelines are moved. Do you think that'll be easy to do?

Thanks!

* Editpart for Message labels
* Adapt insert Message commands accordingly
* LabelParser which conforms to UML spec
*  Add test cases
* Adapt MessageUtil & add OperationUtil
With the addition of the MessageLabelPart  the moveAsyncMessage() test
cases did move the label instead of the message.
->
Changed grab point for  from the middle of the line to the first quarter
of the line
@tortmayr
Copy link
Collaborator Author

I rebased this PR on the current master and added a fix for the label font size.
Additionally, I investigated the label update issue.

In the properties-view only changes made to attributes will trigger a label refresh. Changes made to references will not trigger the refresh (only if the reference is deleted or replaced)

Using the model explorer to edit the references directly will also not trigger a label refresh.
We have to register the MessageLabelEdit part as change listener for this references but I'm not sure about best way and place to achieve this.

@cdamus
Copy link
Collaborator

cdamus commented Sep 5, 2018

Indeed, the MessageLabelEditPart will have to behave something like a combination of EContentAdapter and ECrossReferenceAdapter: listening not only to the Message and all contained objects such as argument ValueSpecifications but also referenced objects such as the signature and, where that is a Signal or an Operation, the objects contained by that (attributes and parameters, respectively).

I'd suggest a notification-helper object alongside the MessageParser that is attached by the edit-part to the Message as an Adapter. This helper keeps track of the message's contents and its signature and the signature's contents (à la EContentAdapter) and delegates to some interface provided the edit-part to tell it about semantic elements that the edit-part needs to listen to. The parser shouldn't know about the specific MessageLabelEditPart, of course, hence the suggestion for some delegate interface that the helper can use to notify which objects a client should start or stop listening to (in the label edit-part's case via the GMF notification broker as usual).

@planger
Copy link
Collaborator

planger commented Sep 24, 2018

@cdamus Please feel free to continue with this task, if you have room for it currently. @tortmayr is currently busy with a Papyrus Compare task. He would be able to continue at the end of this week or beginning of next week.

Conflicts:
	plugins/org.eclipse.papyrus.uml.diagram.sequence.runtime/src/org/eclipse/papyrus/uml/diagram/sequence/runtime/internal/providers/SequenceElementTypes.java
	plugins/org.eclipse.papyrus.uml.diagram.sequence.runtime/src/org/eclipse/papyrus/uml/diagram/sequence/runtime/internal/providers/SequenceParserProvider.java
	tests/org.eclipse.papyrus.uml.diagram.sequence.runtime.tests/META-INF/MANIFEST.MF
- extend add/remove semantic listeners to get notifications on model
changes required for label updates
- small NPE coming from reorient message update

Change-Id: I31b346b998ca52014cd34966bda1a13a4a67b6a2
Signed-off-by: Remi Schnekenburger <rschnekenburger@eclipsesource.com>
@rschnekenbu
Copy link
Collaborator

I updated the PR with 2 commit: solve conflicts (trivial) and update the listener mechanism to watch model modifications and refresh semantic listeners

- fix test issues: removing labels that prohibits some moves on self
messages.

Change-Id: Ie170814ce57b8b42a3b8026e0bd301454bbf6c4a
Signed-off-by: Remi Schnekenburger <rschnekenburger@eclipsesource.com>
@rschnekenbu
Copy link
Collaborator

I updated message label edit parts and listeners to be notified on relevant model changes
Also, I updated the test models with some css to avoid labels being on the top of the self messages. I have hidden with css the labels to avoid such issues
There are still some evolutions required for the message labels (message wrapping for example, or placing them nicely and automatically in the diagram), but this could be handled later on a separate branch.
Could this additional commits be reviewed, @planger ?

@planger planger merged commit 86b25f2 into master Oct 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants