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

Add Part and Shared Association to Tool Palette #352

Merged
merged 19 commits into from Jun 30, 2020

Conversation

danyeaw
Copy link
Member

@danyeaw danyeaw commented Jun 14, 2020

Signed-off-by: Dan Yeaw dan@yeaw.me

This PR adds buttons to the tool palette for creating a Part (Direct Composition) and Shared Associations. This save a few step when create a Block Definition Diagram, so that you don't have to create an association and change the navigability and aggregation each time.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Documentation content changes

What is the current behavior?

Issue Number: Partial #138

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

TODO

  • Change the navigability and aggregation of the association as it is created
  • Prevent removal of navigability and aggregation upon disconnect

Signed-off-by: Dan Yeaw <dan@yeaw.me>
@danyeaw danyeaw added this to the 2.0 milestone Jun 14, 2020
@github-actions github-actions bot added the feature A new feature label Jun 14, 2020
@amolenaar
Copy link
Member

I think in the icon, the diamond on the association end may be a little smaller.

Also I'd like this in the UML palette too.

Signed-off-by: Dan Yeaw <dan@yeaw.me>
Signed-off-by: Dan Yeaw <dan@yeaw.me>
@danyeaw
Copy link
Member Author

danyeaw commented Jun 17, 2020

@amolenaar Thanks again for the feedback.:clap: I made those two updates. What do you think of the updated icons?

@amolenaar
Copy link
Member

What do you think of the updated icons?

Those look really nice 👍

Signed-off-by: Dan Yeaw <dan@yeaw.me>
Signed-off-by: Dan Yeaw <dan@yeaw.me>
Signed-off-by: Dan Yeaw <dan@yeaw.me>
Signed-off-by: Dan Yeaw <dan@yeaw.me>
Signed-off-by: Dan Yeaw <dan@yeaw.me>
Signed-off-by: Dan Yeaw <dan@yeaw.me>
Signed-off-by: Dan Yeaw <dan@yeaw.me>
Copy link
Member

@amolenaar amolenaar left a comment

Choose a reason for hiding this comment

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

Nice improvement! 💪

gaphor/SysML/toolbox.py Outdated Show resolved Hide resolved
gaphor/SysML/toolbox.py Show resolved Hide resolved
gaphor/ui/icons/Makefile Outdated Show resolved Hide resolved
Signed-off-by: Dan Yeaw <dan@yeaw.me>
Signed-off-by: Dan Yeaw <dan@yeaw.me>
Signed-off-by: Dan Yeaw <dan@yeaw.me>
Signed-off-by: Dan Yeaw <dan@yeaw.me>
@danyeaw danyeaw marked this pull request as ready for review June 24, 2020 02:12
"""
opposite = self.line.opposite(handle)
c1 = self.get_connected(handle)
c2 = self.get_connected(opposite)
if c1 and c2:
old: UML.Association = self.line.subject
del self.line.subject
Copy link
Member Author

Choose a reason for hiding this comment

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

@amolenaar In order to keep the UML.Association including the navigability and aggregation, does this seem reasonable to you to not delete these on disconnect?

Copy link
Member

Choose a reason for hiding this comment

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

This will, however, disconnect the member end from everything. Including the model. So these memberEnd properties can not be reused... Same goes for self.line.subject.unlink(): the Association instance will be completely removed from the model. This causes errors when saving and loading the model.

Since the Association and its memberEnds are created when the item is placed on the diagram, they should remain part of the model after disconnecting. Alternatively, we can just create the Association and add the memberEnds only when an association is connected. It will violate the multiplicity constraint, though ([2..*]).

I think this approach makes sense. An Association is also a Classifier, so it has a different role than, say, Generalization. Also, this will allow us to create Association Classes (or 3/4/5-way associations for that matter).
The trade-off is we give up the possibility to present an association in different diagrams. We can always work around this by offering such an option though the element editor, which may be fine, since I generally to not tend to use associations in multiple diagrams anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

@amolenaar Thanks for the clarification on the unlink!

Ideally I think the associations should be reusable in many diagrams. A user should be able to grab a few elements in a model and then ask the model to display the relationships that already exist.

I thought we had to create the Association and memberEnds to display the navigability and aggregation. This is how it was a few commits ago, the association goes back to being a plain one as soon as you disconnect it.

Copy link
Member

Choose a reason for hiding this comment

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

Ideally I think the associations should be reusable in many diagrams. A user should be able to grab a few elements in a model and then ask the model to display the relationships that already exist.

That would be a nice feature. But it's different from:

  1. create a diagram with 2 classes and draw an association between them (name it "foo")
  2. create a new diagram, add the same 2 classes and draw another association
  3. Will the association be:
    a. a new association
    b. be the already existing association named "foo"?

Currently we do option 3a. Which is fine IMHO.

I thought we had to create the Association and memberEnds to display the navigability and aggregation. This is how it was a few commits ago, the association goes back to being a plain one as soon as you disconnect it.

Indeed you'll need both Association and the Properties at the memberEnds. Since the logic on navigability lies at the model. Not in the diagram items. It should, however, still disconnect the "type"s at the end. In the connect() method, the association ends are paired with the classes they connect to. On disconnect, this relation should be removed I think. Otherwise the model and the diagrams are no longer in sync.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks good catch. I don't think I was properly connecting the the type to the element either. How does this look?

@danyeaw
Copy link
Member Author

danyeaw commented Jun 25, 2020

@amolenaar I think this is ready, would you mind giving me your feedback on the two items above?

  1. Use of append on the memberEnd to avoid mypy errors
  2. Skipping deletion of the line.subject and head/tail subject on disconnect

Thanks!

Signed-off-by: Dan Yeaw <dan@yeaw.me>
Signed-off-by: Dan Yeaw <dan@yeaw.me>
Signed-off-by: Dan Yeaw <dan@yeaw.me>
@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Jun 29, 2020

Sourcery Code Quality Report (beta)

✅ Merging this PR will increase code quality in the affected files by 0.01 out of 10.

Before After Change
Quality 8.67 8.69 0.01
Complexity 1.32 1.25 -0.07
Method Length 61.67 62.01 0.34
Lines 2249 2296 47
Changed files Quality Before Quality After Quality Change
gaphor/SysML/toolbox.py 8.84 8.91 0.07 ⬆️
gaphor/UML/modelfactory.py 8.83 8.83 0.0
gaphor/UML/toolbox.py 8.86 8.86 0.0
gaphor/UML/classes/classconnect.py 8.41 8.47 0.06 ⬆️
gaphor/UML/classes/classespropertypages.py 8.89 8.88 -0.0 ⬇️
gaphor/UML/classes/tests/test_classconnect.py 7.98 7.97 -0.01 ⬇️

Here are some functions in these files that still need a tune-up:

File Function Quality Recommendation
gaphor/UML/classes/classconnect.py AssociationConnect.connect_subject 5.08 Split out functionality
gaphor/UML/classes/classespropertypages.py OperationsPage.construct 5.96 Split out functionality
gaphor/UML/classes/classespropertypages.py AttributesPage.construct 6.16 Split out functionality
gaphor/UML/classes/tests/test_classconnect.py DependencyTestCase.test_multi_dependency 6.28 Split out functionality
gaphor/UML/classes/classespropertypages.py AssociationPropertyPage.construct 6.45 Split out functionality

Please see our documentation here for details on how these metrics are calculated.

We are actively working on this report - lots more documentation and extra metrics to come!

Let us know what you think of it via email or our Gitter!

@danyeaw
Copy link
Member Author

danyeaw commented Jun 30, 2020

@amolenaar Anything left to do before I merge this?

@danyeaw danyeaw merged commit ea8ca89 into master Jun 30, 2020
@danyeaw danyeaw deleted the feature/bdd-association-shortcuts branch June 30, 2020 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants