sverweij / mscgen_js

text => sequence charts
https://mscgen.js.org
GNU General Public License v3.0
206 stars 25 forks source link

Using label and arcskip on a link breaks horizontal alignment #250

Closed 2pl closed 6 years ago

2pl commented 6 years ago

Hi,

I'm having issue with horizontal alignment when using links that have a label attribute and a non zero arcskpip value. The below Xu code is showing two examples of this issue.

I would expect the "Update" link to be always horizontally aligned with the end of the first link, but only the first alternative is showing correct alignment, adding the "Send" label to the first link breaks horizontal alignment.

Tried this with the msc online and various browser with same result

msc {

kfk2 [label="Kafka"], 
k2db [label="KFK 2 DB"],
mysql [label="MySQL"];

kfk2 alt mysql [label="OK"]{
    kfk2 => k2db [arcskip=1];
    k2db => mysql [label="Update"];
};

kfk2 alt mysql [label="NOT OK"]{
    kfk2 => k2db [label="Send", arcskip=1];
    k2db => mysql [label="Update"];
};

kfk2 alt mysql [label="NOT OK"]{
    kfk2 => k2db [label="Send", arcskip=2];
    k2db => mysql [label="Update"];
};

}
sverweij commented 6 years ago

Thanks! There's certainly room for improvement there. I'll have to wrap my head around the problem a bit before there'll be a solution

For rows with an arcskip mscgen.js currently takes the base (~ the minimum) height of a row (source). Instead ...

I also see that even in the 'OK' part of your chart the arc is pointing a pixel too high. I'll fix that too.

screen shot 2017-11-19 at 10 10 14

sverweij commented 6 years ago

I'm working on a fix (☝️). It already solves part of the problem, and addresses some other inconsistencies with the arcsckips and arc gradients (see the PR for details).

I'll have to see if I can tackle (part of) the remaining stuff now or release this first and tackle the rest it later.

sverweij commented 6 years ago

@2pl I've published the improvements the previous comment talks about (to npm, to mscgen.js.org and to atom.io for atom-mscgen-preview).

Deep link with the chart in the interpreter here; your chart now looks like this:

screen shot 2017-11-19 at 17 56 06

Still not perfect, but better then it was.

(B.t.w. For comparison I've taken a look at how the mscgen reference implementation would've rendered this (minus the xù specific stuff):

2pl commented 6 years ago

@sverweij thank you for the fix, it definitely improves rendering for arcskip=1 with label use case !

As for the issue with arcskip=2, it is indeed not related to label, I found other examples of weird alignement when using arcskip=2, it looks like using arcksip=2 is shifting the source line down, see below

msc {

kfk2 [label="Kafka"], 
k2db [label="KFK 2 DB"],
mysql [label="MySQL"];

kfk2 alt mysql [label="This is now OK !"]{
    kfk2 >> mysql, 
    kfk2 => k2db [label="Send", arcskip=1];
    k2db => mysql [label="Update"];
};

kfk2 alt mysql [label="not OK"]{
    kfk2 >> mysql, 
    kfk2 => k2db [arcskip=1];
    k2db => mysql [label="Update"];
    k2db => mysql [label="Update"],
    kfk2 >> k2db [label="skip 1 is now ok", arcskip=1];
    k2db => mysql [label="Update"];
    k2db => mysql [label="I feel a bit low"],
    kfk2 >> k2db [label="skip 2 is not ok", arcskip=2];
    k2db => mysql [label="Update"];
};

kfk2 alt mysql [label="out of the box !"]{
    k2db => mysql [label="Update"];
    k2db => mysql [label="I should be higher"],
    kfk2 >> k2db [label="oops skipping out of the box", arcskip=2];
};

kfk2 alt mysql [label="not OK even with no label"]{
    kfk2 >> mysql [label="I am a bit low"], 
    kfk2 >> k2db [arcskip=2], 
    kfk2 => k2db [arcskip=1];
    k2db => mysql [label="I look a bit high but i'm OK !"];
};
}
sverweij commented 6 years ago

One step at a time :-)

To be transparent: I work on a thing and if it's an improvement release it (☝️ to witness) instead of making a complete solution covering all cases. This allows me to work on it during the small slots of time I have available and still progress. Otherwise weeks would go by before there's movement on an issue.

E.g. the PR above makes that arcskip for n > 1 behaves more as expected. I released it yesterday (mscgenjs 1.12.6) It does not solve cases where the height of an arc is larger than the default and it does not take into account inline expressions - which I'll work on next (in that order).

sverweij commented 6 years ago

@2pl TL;DR - the remaining issues are now merged and published (to all npm libraries, mscgen.js.org and the atom plugin); give it a spin and let me know what you think.

Thanks again for raising this; it has made the arcskip functionality a lot better than it was; more predictable and (I think) better looking!

Details are in the PR's (so if you feel like it peruse them at will), but I made two decisions that might need some 'splaining:

  1. pointing out of the box
    For inline expressions (technical name for those alt, opt & loop things) I had to take a decision:
    • should we point to the edge of the inline expression (which is meaningless) ...
    • or should we point it to the next 'real' arc row The road of least astonishment is the second one; it's weird to count edges of inline expressions as arcs in the first place.
arcskip-out-of-the-box
  1. to infinity... and beyond
    What to do with arcskips that skip beyond the chart?
    I've followed the original MscGen implementation: max out at the last arc row. In this example chart, arcskip=42 doesn't get us further than the next arc, because that's the last in the chart: and-beyond

(B.t.w.; the correct way to point lower than that would be to add an empty row (|||, or even ...) on the bottom).

2pl commented 6 years ago

@sverweij I updated Atom with the latest and greatest mscgen-preview and it works great, you mastered the arcskip !