charmbracelet / freeze

Generate images of code and terminal output 📸
MIT License
3.45k stars 60 forks source link

png: rsvg-convert implementation #51

Open ccoVeille opened 8 months ago

ccoVeille commented 8 months ago

There is something strange to me with current code

https://github.com/charmbracelet/freeze/blob/4fcacffcfe36ffa64637675987496fe86e5ca12c/png.go#L14-L31

You are not using w and h variables.

So either, you should simplify the signature, or use them as parameter for rsvg-convert

as it supports it

USAGE:
    rsvg-convert [FLAGS] [OPTIONS] [FILE]...

FLAGS:
    -?, --help                  Prints help information
    -a, --keep-aspect-ratio     Preserve the aspect ratio
        --keep-image-data       Keep image data
        --no-keep-image-data    Do not keep image data
    -u, --unlimited             Allow huge SVG files
    -v, --version               Prints version information

OPTIONS:
    -l, --accept-language <languages>    Languages to accept, for example "es-MX,de,en" [default uses language from the
                                         environment]
    -b, --background-color <color>       Set the background color using a CSS color spec
    -i, --export-id <object id>          SVG id of object to export [default is to export all objects]
    -f, --format <format>                Output format [default: png]  [possible values: Png, Pdf, Ps, Eps, Svg]
        --left <length>                  Distance between left edge of page and the image [defaults to 0]
    -o, --output <output>                Output filename [defaults to stdout]
        --page-height <length>           Height of output media [defaults to the height of the SVG]
        --page-width <length>            Width of output media [defaults to the width of the SVG]
    -d, --dpi-x <number>                 Pixels per inch [default: 96]
    -p, --dpi-y <number>                 Pixels per inch [default: 96]
    -w, --width <length>                 Width [defaults to the width of the SVG]
    -h, --height <length>                Height [defaults to the height of the SVG]
    -s, --stylesheet <filename.css>      Filename of CSS stylesheet to apply
        --top <length>                   Distance between top edge of page and the image [defaults to 0]
    -z, --zoom <number>                  Zoom factor
    -x, --x-zoom <number>                Horizontal zoom factor
    -y, --y-zoom <number>                Vertical zoom factor

So the code would be something like this

    rsvgConvert := exec.Command("rsvg-convert", "-o", output, "-w", fmt.Sprintf("%.0f", w), "-h", fmt.Sprintf("%.0f", w))
ccoVeille commented 8 months ago

BTW, what is the current status with rsvg-convert support.

I see nothing in the readme about it, the code does not suggest installing it to speed up PNG generation.

maaslalani commented 8 months ago

Hey @ccoVeille, are you noticing issues with the --width and --height?

We do all the calculations in the SVG such that we don't need to specify the width and height during the conversion but let me know if you are seeing incorrect behaviour.

ccoVeille commented 8 months ago

Hey @ccoVeille, are you noticing issues with the --width and --height?

We do all the calculations in the SVG such that we don't need to specify the width and height during the conversion but let me know if you are seeing incorrect behaviour.

Nope, I just noticed the signature was defining unused variables.

I was suggesting to either pass the variables to binary or remove them from the function signature.

I think it confirms they can be removed. If you think they should stay for signature consistency. You could use , float64 but I don't think it's a good idea.

At first, I thought your code was relation on interfaces, so you had to get them in the signature.

It's up to you.