Skip to content
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

Add margin/border support #43

Merged
merged 6 commits into from
Mar 11, 2024
Merged

Add margin/border support #43

merged 6 commits into from
Mar 11, 2024

Conversation

patrislav1
Copy link
Collaborator

@patrislav1 patrislav1 commented Feb 21, 2024

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 🤡 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
Copy link
Owner

wader commented Feb 21, 2024

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?

svgscreen/template.svg Outdated Show resolved Hide resolved
@patrislav1
Copy link
Collaborator Author

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? 🙏

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
Copy link
Owner

wader commented Feb 23, 2024

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
Copy link
Collaborator Author

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.

@wader wader changed the title wip: margin/border around image Add margin/border support Mar 11, 2024
Copy link
Owner

@wader wader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! maybe fix up the wip commit

Copy link
Owner

@wader wader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update README with new option?

@patrislav1
Copy link
Collaborator Author

Renamed WIP commit; updated README.

Copy link
Owner

@wader wader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! i'm ok to merge if you ready

@patrislav1 patrislav1 merged commit 2a87c3c into wader:master Mar 11, 2024
1 check passed
@patrislav1 patrislav1 deleted the wip-border branch March 11, 2024 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants