Fixes #1680, #1679, #839 and various other Sequence Diagram layout issues.

[jimnelson372]

This collection of fixes all relate to Sequence Diagrams, mostly when using left side self messages, but also in a few cases when using left to right short arrows (e.g ?->D).

• Issue #839 and #1680 were about groups not properly drawing around left side self messages.
• issue #1679 was about Notes (both left of and right of these left side self messages) not positioned correctly. I combined this fix with the group fix, since Notes within groups were equally having layout problems.
• When testing these, I found the Teoz versions were also having problems with Note positioning on Left side self messages, and even had problems drawing a far left left side self message, so found these related fixes.
• Double checking on related Teoz problems, I found a Forum post "Teoz grouping does not embrace all signals", with an example of a short arrow in a group not being drawn within the group. That issue can be found at: https://forum.plantuml.net/18832

The changes I made are very isolated to blocks of code dealing with reversed self-arrows.

1. MessageSelfArrow was fixed so that its getStartingX method, used by getMinX, properly accounted for the message being moved the preferred width to the left. I had to pass a boolean to the component creation to indicate it was a reversal, since that information wasn't obtainable as in other places.
2. ComponentRoseSelfArrow then, instead of drawing itself to the left of its area when drawing a left side self message, the positioning is already managed by the layout, so the drawing stays within the area. The left side arrow line itself, then, needed to be drawn at the far right of the area. Very straight forward.
3. Step1Abstract just needed to check if the note was on a reversed self message and instead of pushing the noteBox to the right a positive width, it now pushes a negative width. This solved the correct positioning of the Left side Note.
4. Step1Message just needed to pass the boolean of whether the self message was reversed to the MessageSelfArrow as mentioned in change 1.
5. To fix the short arrow problem in Teoz's grouping, I just had to add a condition to CommunicationExoTile for isShortArrow in the getPoint1 method, again used by its getMinX to have it positioned correctly so that the Group would know its correct location.
6. CommunicationTileSelf just needed a fix to its getMinX to account for the correct positioning of the left side self message, and the drawU method just needed to use this updated call, rather than calculating the x1 differently.
7. I created a matching pair to the CommunicationTileSelfNoteRight class, calling it CommunicationTileSelfNoteLeft. It properly positions the left side note when a self message is also left side. TileBuilder was updated to create this new class under the appropriate conditions (LEFT and self message).
8. Finally, ArrowAndNotBox just removes what appears to be a bug, where noteBox was Right Shifted by getStartingY when the NotePosition was RIGHT. First, in all my tests it doesn't seem that a right shift was needed there anymore, and second if it was, it should be based on an X coordinate, not a Y coordinate. Again, all works with that removed. I suspect that shift was either ignored, or the value of Y was small enough not to make a difference.

While I tested this in cases beyond any of the reported problems, and in cases where there weren't problems to make sure, Here are examples of what the reported issues now will look like:

For #1680, the grouping now is correctly around the message text.

For #839, the bottom group now properly just surrounds the self message:

For #1679, the bottom two notes are not positioned correctly:

For Forum 18832, the short arrow (now labled M2 [Grouped[) is correctly captured within the group. This is with Teoz turned on.

Finally, I had mentioned fixing other problems Teoz had with the left side self messages. For instance the following script:

@startuml
!pragma teoz true
skinparam {
  Maxmessagesize 200
}

group Grouping messages
    Test <- Test    : The group frame [now] does draw a border around the text (located on the left side), [lo longer] ignores its presence, and [no longer]] ignores the presence of a line.
    note right
      A note on the self message
    endnote
end
@enduml

Is currently generating:

But will now correctly generate (even with a right arrow): It is similar even without the grouping.

If the note is on the left, it currently generates: But will now generate: Which, of course, matches the right side version nicely (already working):

Let me know if you need me to change anything, or if you have any questions about my changes.

(There are a few more group sizing issues related to Teoz that I didn't include here, since they were already much closer to working that the example in the issues above, mostly involving the grouping being to large in some cases. For now, I focused just on these issues related to the left side self messages and notes.)

Regards, Jim Nelson jimnelson372

[arnaudroques]

Well, you did a great job: those issues are not easy to fix.

However, note placement can be a nightmare :-)

I think there is a regression in the following example:

@startuml
A -> B : a
note right: Note
activate B
B --> A : b
deactivate B
@enduml

The note is partially hidden. Could you have a look at it?

It would also be great if you could add your examples (and mine :-) to the regression tests https://github.com/plantuml/plantuml/tree/master/test/nonreg/simple

Does it sound good?

[jimnelson372]

Thanks @arnaudroques.

And oh, that is a regression! Ouch. That my change caused an active timeline to block a right side note does surprise me, but there it is. And, unfortunately, I believe it's due to my "fix" to the ArrowAndNoteBox that I considered a bug. I will take a second look at that later this evening or in the morning.

I'll look at how those regression tests are written and add to those tests.

Quick question, how did you find the regression example you shared? I just ran the regression tests with these latest changes in place and I'm not seeing the error showing up there.

Regards.

[jimnelson372]

Ah. I apologize for my complete misunderstanding of that block of code in ArrowAndNoteBox.getPreferredWith(...). That block was:

            if (noteBox.getNotePosition() == NotePosition.RIGHT) {
                result += noteBox.getRightShift(arrow.getStartingY());
            }

I mistakenly had thought it was calculating the amount of a right shift based on the input, and I was confused why the input should be a Y value. But when I stepped into it with a debugger and saw method names such as getRightShiftAtLevel(..Y), I finally grasped that it is looking up the appropriate amount to right shift a noteBox when at a given horizontal level in the sequence diagram. Now it makes sense!

I just created a pull request to restore this class to its original version. Fortunately, all the changes I made in the other classes do not depend on this class.

Regards, Jim Nelson

[arnaudroques]

Quick question, how did you find the regression example you shared? I just ran the regression tests with these latest changes in place and I'm not seeing the error showing up there.

Over the past 15 years, I have compiled a collection of a few thousand diagrams that I use for regression testing. As bugs and updates arise, I save example diagrams of varying complexity.

These diagrams are not published: some of them contain data from users who have sent them to me, and I cannot share them as is. Sorting them manually is quite tedious.

Another major challenge is that these regression tests are not fully automated. I generate an HTML file with the images that have changed, and it then requires visual inspection to determine if a diagram is significantly different or not.

It is probably time to industrialize this process. However, putting all these tests in https://github.com/plantuml/plantuml/tree/master/test/nonreg/simple does not seem like a good idea. First, because the tests themselves take a lot of time. Second, because every code modification always results in small differences, and I don't see how to completely automate the comparison.

We need a system that can intelligently compare two diagram images.

If you are interested, we could try a new GitHub project, "plantuml-nonreg," dedicated solely to calculating the delta between two versions of PlantUML. Perhaps with a scoring system?

I don't have much time, but it could be an interesting project, and I am willing to commit the publishable part of my diagram collection to this non-regression project.

[jimnelson372]

We need a system that can intelligently compare two diagram images.

If you are interested, we could try a new GitHub project, "plantuml-nonreg," dedicated solely to calculating the delta between two versions of PlantUML. Perhaps with a scoring system?

It is an interesting issue. Right now, I am focused on learning more about the layout processes itself, while helping to fix reported issues. Maybe after I have more experience with the code and the variety of diagrams themselves I can give this more thought.

[kirchsth]

Hi @jimnelson372

I am focused on learning more about the layout processes itself, while helping to fix reported issues

If you search for a new bug (in area of sequence diagram and teoz) :smirk: can you have a look on teoz produces OutOfMemoryError too?

Thank you Helmut

[arnaudroques]

Hi @jimnelson372

I am focused on learning more about the layout processes itself, while helping to fix reported issues

If you search for a new bug (in area of sequence diagram and teoz) 😏 can you have a look on teoz produces OutOfMemoryError too?

Thank you Helmut

The good news is that I've found where the issue come from :-) It's quite easy to fix: adding some cache in getCurrentValueInternal() method from RealMin and RealMax class.

I cannot commit the correction right now, but that will be done before the end of next week. Stay tuned!

[jimnelson372]

Hello @arnaudroques,

While working on issue #1683, I discovered that I left incomplete the positioning of the "o" arrow decorators in my fix to ComponentRoseSelfArrow. My change is causing the o's to draw to the left side of the comment, rather than along the timeline. I can include that fix when I do the other arrow drawing fixes for the left side self messages for issue #1683, but if you need this earlier, I can do a smaller pull request just to correct the decorator positioning. Just let me know if you do need it quickly. If all goes well, I'll have the #1683 fixes completed before the end of the weekend.

Regards, Jim Nelson

[arnaudroques]

Hello @arnaudroques,

While working on issue #1683, I discovered that I left incomplete the positioning of the "o" arrow decorators in my fix to ComponentRoseSelfArrow. My change is causing the o's to draw to the left side of the comment, rather than along the timeline. I can include that fix when I do the other arrow drawing fixes for the left side self messages for issue #1683, but if you need this earlier, I can do a smaller pull request just to correct the decorator positioning. Just let me know if you do need it quickly. If all goes well, I'll have the #1683 fixes completed before the end of the weekend.

Regards, Jim Nelson

No, don't worry: I do not need it quickly. Just take your time to fix it: we will release an official version only next week-end.

Good luck in your debug :-) Thanks again

[kirchsth]

Hi @arnaudroques and @jimnelson372

thank you for the fixes.

Helmut