wader / ansisvg

Convert ANSI to SVG
MIT License
89 stars 9 forks source link

Add margin/border support #43

Closed patrislav1 closed 8 months ago

patrislav1 commented 8 months ago

This is a quick hack to add some "breathing room" around the image contents, to avoid that the characters touch the edge of the image. (I believe it's more readable and more pleasant to look at). It turns out that, once again, the easy way to achieve this with border=... does not work across all viewers :clown_face: so we have to do some gymnastics offsetting the contents with margin and raising the image size accordingly so that no contents get clipped off.

So this is a quick hack with hard coded 10px margin; it only works when using charbox mode (b/c we cannot mix px and em for the image size obviously).

If you like it we can add a margin argument or similar; not sure yet how to handle it in non-charbox mode; it may be a bit of a pain but I believe it will be worth it.

wader commented 8 months ago

Hey! sounds like a good idea. Would this off by default? wonder if a user would expect a filled character or one with background color to fill all the way?

No need for bottom/right margin?

patrislav1 commented 8 months ago

so I thought we could have an option like --margin <X>x<Y> which would set the margin in the appropriate units (px if charboxsize is used, ch/em otherwise).

already added the required logic, what bothers me a bit is how we need to define custom types multiple times in different places (see e.g. cli.boxSize vs. svgscreen.BoxSize), I remember that there were even three places where a variant of boxSize was defined and I already reduced it to two. Before I add the type for margin (x,y like boxSize but as floats) I'd like to refactor it further so we can maybe just have one instance of each custom type? :pray:

I considered defining them in svgscreen or to add an extra module for custom types without further dependencies (svgscreen.types or similar). A little drawback would be that the Set and String functions had to be implemented outside the CLI module, but maybe it's not that dramatic? Is there a common pattern or best practice how this kind of issue is best dealt with in Go?

wader commented 8 months ago

I think it's perfectly fine and quite common to have a go package (a directory basically) for just one type, even if very simple and small. That is also sometimes the only way to not cause cyclic package dependencies which is not allowed in go. Not sure i have any great idea for how a "dimension" type could be done, could get some inspiration from how CSS does it? what you want is a type that stores with/height and what unit it is in? no conversions needed etc? maybe width/height as ints and a enum for unit, possibly some NewFrom* functions?

Having Set (for flag?) and String interface implementations for the type i think is quite idiomatic. Usually interfaces are suppose to be seen as behaviours or things the type can do or be etc, so think it make sense.

patrislav1 commented 8 months ago

Ditched the margin CSS style again, in favor of a simple coordinate offset - it seems to be more robust / give better consistency across different viewers. Added test cases as well.

patrislav1 commented 8 months ago

Renamed WIP commit; updated README.