-
-
Notifications
You must be signed in to change notification settings - Fork 949
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
Fixes #1680, #1679, #839 and various other Sequence Diagram layout issues. #1777
Fixes #1680, #1679, #839 and various other Sequence Diagram layout issues. #1777
Conversation
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:
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? |
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. |
Ah. I apologize for my complete misunderstanding of that block of code in ArrowAndNoteBox.getPreferredWith(...). That block was:
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, |
Reverting the change to ArrowAndNoteBox made in pull request #1777
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. |
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. |
If you search for a new bug (in area of sequence diagram and teoz) 😏 Thank you |
The good news is that I've found where the issue come from :-) I cannot commit the correction right now, but that will be done before the end of next week. |
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, |
No, don't worry: I do not need it quickly. Good luck in your debug :-) |
Hi @arnaudroques and @jimnelson372 thank you for the fixes. Helmut |
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).
The changes I made are very isolated to blocks of code dealing with reversed self-arrows.
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:
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