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

Fix #411 Execution nesting changes #423

Merged
merged 4 commits into from
Nov 29, 2018

Conversation

cdamus
Copy link
Collaborator

@cdamus cdamus commented Nov 19, 2018

Fix a variety of edit scenarios involving execution nesting, including:

  • don't allow partial overlap of execution specifications. Nesting must be complete or not at all
  • ensure that executions dropped onto other executions result in proper nesting, whether the
    execution dropped is smaller than (nested within) or larger than (nesting without) the execution
    on which it is dropped
  • the previous item applies equally to executions dropped onto executions on other lifelines as
    on the same lifeline

This also includes some incidental changes unrelated to Issue #411:

  • issue Nudging an execution nudges down also nested executions #422 (fixing it was required for proper recalculation of executions in other drag/drop scenarios)
  • JUnit tests that have been skipping on assumption violations since the merge of changes to the anchoring of delete messages (I worried that it was a regression introduced by changes in this PR, but that turned out not the case)

Update JUnit tests to account for new anchoring location of
the delete message on the destruction occurrence shape.

Signed-off-by: Christian W. Damus <give.a.damus@gmail.com>
- Handle moving an execution to make it nested in another
- Handle moving an execution to make it nest another
- Ensure that the user cannot reshape execution specifications
  to effect partial nesting (incomplete overlap of extent)
- Indirectly fix eclipsesource#422 redundant nudging of nested executions

Signed-off-by: Christian W. Damus <give.a.damus@gmail.com>
@cdamus
Copy link
Collaborator Author

cdamus commented Nov 27, 2018

I had already checked that this can still be merged even after #426. 😉

Copy link
Collaborator

@rschnekenbu rschnekenbu left a comment

Choose a reason for hiding this comment

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

Hi @cdamus, indeed, this PR merges perfectly with the work on #426 ;-)

Works good, I only have a small glitch when uncovering a nested execution:
before
before

after
after - note the execution which is still located on its previous X location compared to the middle of the lifeline.

Finally, I have detected a regression on the pop up menu when creating a sync message from an execution, but it seems related to the work on #426 if I dig into the history.
menu
I can create a new bug out of this last comment, as this should probably not be handled in this PR.

@cdamus
Copy link
Collaborator Author

cdamus commented Nov 28, 2018

Thanks, @rschnekenbu ! The misaligned execution should be easy to fix. I expect the popup menu problem is caused by regeneration of the model. Oops. 😳

Address the case identified in code review of a nesting
execution being resized leaving a formerly nested execution
hanging in space.

Includes update to the auto-fixture rule to support optional
(dynamic) fixtures to simplify again the specification of tests.

Signed-off-by: Christian W. Damus <give.a.damus@gmail.com>
@cdamus
Copy link
Collaborator Author

cdamus commented Nov 28, 2018

Thanks again, @rschnekenbu. I've pushed a new commit that fixes the problem of the carpet swept out from under a nested execution.

@rschnekenbu
Copy link
Collaborator

Looks good to me ;) Thanks for update on this PR!

@rschnekenbu rschnekenbu merged commit 6be3a7e into eclipsesource:master Nov 29, 2018
@cdamus cdamus deleted the 411-exec-nesting branch November 29, 2018 12:30
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.

2 participants