Variable row height

[lorrden]

The row-height can be set globally, but resetting it before e.g. the next row means that we get overlapping rows (if we reduce the row height).

We can insert a row manually, but then the automatic labels with offsets are messed up, like this:

(def row-height 60) 
(draw-column-headers)
(draw-box "version" {:span 3})
(draw-box (text "type" {:writing-mode "vertical-rl"}) {:span 1})
(draw-box "dfh" {:span 1})
(draw-box "apid" {:span 11})

(next-row)
(def row-height 30) 
(draw-box "seq. flags" {:span 2})
(draw-box "sequence count" {:span 14})

(draw-box "packet length" {:span 16})

(draw-box "shf" {:span 1})
(draw-box "pusvers" {:span 3})
(draw-box "ack" {:span 4})
(draw-box "service type" {:span 8})
(draw-box "service subtype" {:span 8})

(draw-gap)

(draw-box "packet error control" {:span 16})

We can insert draw-row-header "00" before, the (next-row), but that doesn't increment the automatic row headers.

I suppose this is also a question of documentation in the end, how do we manually draw the row header address and increment it for the next line?

[brunchboy]

I’m sorry, I’m afraid I don’t understand exactly what you are saying or trying to accomplish here. When I have some time I will try to render the diagram that results from this code and see if that helps, but it will probably have to wait until after work.

[brunchboy]

I am certainly willing to believe there is some interaction that I haven’t properly accounted for yet, though! I extended several things in different directions recently without spending enough time in a hammock thinking about how they might affect each other. I am sure we can find a good solution though. 😄

[lorrden]

Basically, I am trying to change the row height after drawing a row.

This needs to happen after advancing to the next row, but before drawing the first box on the next row.

I can do that with an explicit next-row, and then redefine the height, but that looses the automatic row heading on the first row.

[brunchboy]

Yeah, I guessed it was the auto-headers that were probably involved. Those were a cool feature I snuck in later on. Does manually adding the first row header in that circumstance solve it for now, or does that lead to other problems in later rows? One option might be to add a :next-row-height attribute you could pass to draw-box, and it would apply it after it called next-row internally, but would have no effect if the box wasn’t the start of a new row. Any way you slice it, I can’t shield diagram authors from all of the complexity here.

[lorrden]

Manually adding the first doesn’t update the next header. I suppose this is not a big issue since I am auto generating my diagrams.

What I was trying to do was figuring out if I could call the auto header drawing/update function manually, but I am a bit lost on this and my clojure experience is pretty much 2 days old 😉

[brunchboy]

This is the part where I didn’t understand what you meant, but after work when I have time to experiment with your sample code I’ll get there. Thanks for raising this.

[brunchboy]

…actually, snuck a little time at lunch to try it and see the problem. Ok, now I can look at the code and try to remember how all that stuff works. 😄

[brunchboy]

Ok, the problem is that next-row is low level and doesn’t take part in the box counting and address updating. One quick and dirty alternative is to add this line after you call next-row, where you are setting row-height back down:

(swap! diagram-state update :address + 16)

But that is not a great answer. If you could call auto-advance-row instead of next-row before drawing the first box on the second row, it would take care of drawing both headers for you and updating the address of the current row. But it would draw both headers using the current row height, so the new row’s header would be in the wrong place. And that function is private because of such concerns, so you can’t call it without hackery anyway. I think the only convenient answer will be adding support for setting a new row height by passing an attribute to draw-box like I was thinking above.

[brunchboy]

Except, that shortcut ends up not drawing either of the first row two labels, ugh. Yes, this is going to need internal support to be usable. Either passing :next-row-height as an attribute to draw-box, or having another global setting, like :standard-row-height which the row height gets reset to whenever an auto-advance happens, in case the current row was temporarily set to something different. I think that would be more surprising and confusing than the draw-box attribute, so I am leaning towards that approach.

[brunchboy]

Ok, I have a solution, @lorrden although I forgot to tag this issue in the commit, https://github.com/Deep-Symmetry/bytefield-svg/commit/c22daf5af92d7b21986ab3331c21f29baf1dff46

With this change in place, if you tweak your source like this:

(def row-height 60)
(draw-column-headers)
(draw-box "version" {:span 3})
(draw-box (text "type" {:writing-mode "vertical-rl"}) {:span 1})
(draw-box "dfh" {:span 1})
(draw-box "apid" {:span 11})

(draw-box "seq. flags" {:span 2 :next-row-height 30})
(draw-box "sequence count" {:span 14})

(draw-box "packet length" {:span 16})

(draw-box "shf" {:span 1})
(draw-box "pusvers" {:span 3})
(draw-box "ack" {:span 4})
(draw-box "service type" {:span 8})
(draw-box "service subtype" {:span 8})

(draw-gap)

(draw-box "packet error control" {:span 16})

It produces a diagram like https://deepsymmetry.org/images/test-next-row-height.svg

The documentation is updated in the draw-box attributes and at the bottom of the vertical text section.

Is there any way you can run from source to see if it is working for you? Or are you going to have to wait until I do another release, along with whatever other tool chain you are using?

[lorrden]

I tried to clone and build myself, but I ran into classpath issues when shadow-cljs is launched :(

Clojure is installed via apt-get on my Debian machine.

I think I will wait for the next release.

[brunchboy]

Thanks for trying, and sorry to hear that. I’ll push a release to npm today, you should at least be able to swap that in to your tool chain by messing with your node modules folder until whatever parent project is updated. I can always do more as you identify other areas for improvement.

[brunchboy]

All right, release 1.4.2 has been published, @lorrden

[brunchboy]

Whoops! I had to quickly replace it with release 1.4.3 because I had forgotten to build a release version before publishing, and discovered the problem when trying to build a new release of asciidoctor-bytefield which used it. But asciidoctor-bytefield 1.2.0 has been published now as well.

[brunchboy]

@lorrden could you let me know if this has been addressed in a way that works for you? If I don’t hear anything i will close the issue for inactivity, but confirmation would be appreciated.

[lorrden]

Yes, it is working great. Sorry for not closing the ticket.

[brunchboy]

Glad to hear it, thanks!