chillerlan / php-qrcode

A PHP QR Code generator and reader with a user-friendly API.
https://smiley.codes/qrcode/
Apache License 2.0
2k stars 297 forks source link

[BUG] changing the viewbox size doesn't seem to expand the code to fit. #222

Closed fr3nch13 closed 1 year ago

fr3nch13 commented 1 year ago

Tested on 4.3, 5.0-beta, and main.

Describe the bug or unexpected behaviour

setting the svgViewBoxSize to an int, doesn't seem to fill the viewBox with the QrCode. Without setting the svgViewBoxSize, the output makes it <svg xmlns="http://www.w3.org/2000/svg" class="qr-svg qrcode" viewBox="0 0 41 41" preserveAspectRatio="xMidYMid">. Setting it, and the only thing that changes is the viewBox="0 0 500 500"

Unfortunately I don't know enough about SVG to know if this is the intended output change, or if there should be more.

Hopefully I'm just overlooking some setting that I should be using.

Steps to reproduce the behavior

Current code, simplified for testing.

    $this->options = new QROptions([
            'svgViewBoxSize' => $this->getConfig('svgViewBoxSize', 500),
            'eccLevel' => $this->getConfig('eccLevel', EccLevel::H),
            'outputType' => $this->getConfig('outputType', QROutputInterface::MARKUP_SVG),
            'svgUseFillAttributes' => $this->getConfig('svgUseFillAttributes', true),
            'drawLightModules' => $this->getConfig('drawLightModules', false),
    ]);
$this->data = "https://localhost";
$optionsLight = clone $this->options;
//$optionsLight->setColor($color);
$qrLight = new ChillerlanQRCode($optionsLight);
$qrLight->render($this->data, $qrImagePathLight);

Expected behavior

That if I set the viewBox size, that the QR code, (and svg logo inside) would expand to fill the view box.

Screenshots SVG Gist

1-dark

Environment (please complete the following information):

Additional context

codemasher commented 1 year ago

Hi. This is not a bug. The expectation that changing the viewbox attribute also changes the fill size of the content is a misconception. The SVG documentation says:

The viewBox attribute defines the position and dimension, in user space, of an SVG viewport.

It's useful to change this attribute if you want to add additional content, e.g. text under the QR symbol. The setting $svgViewBoxSize is deprecated because of this and the preferred method is to override the method QRMarkupSVG::header().

https://github.com/chillerlan/php-qrcode/blob/8c0da25c72e2b2f2303ce460a7a7193555cc7c52/src/Output/QRMarkupSVG.php#L90-L110

In order to change the size of the SVG to a fixed, which is what i assume you expect, you need to either add width and height attributes to the svg root element or change its size via CSS.

codemasher commented 1 year ago

In fact, i should remove this option entirely before releasing v5. I added it a long time ago when i knew nothing about SVG and fell for the same misconception. It is misleading and most likely not widely used anyway.

fr3nch13 commented 1 year ago

Well, while you were writing this out, I actually did go a similar path, only I overwrote the getViewBox() method so I wasn't overwriting other header info, and possibly no longer be compatible with your original header.


    protected function getViewBox(): string
    {
        $size = parent::getViewBox();
        [$width, $height] = $this->getOutputDimensions();
        $size = sprintf('0 0 %s %s', $width, $height);

        return sprintf('%1$s" width="%2$s" height="%3$s',
            $size,
            ($this->options->svgViewBoxSize ?? $width),
            ($this->options->svgViewBoxSize ?? $height)
        );
    }

Would, instead of svgViewBoxSize, could scale be used here like it is in the other Types like png? Something like:


    /**
     * returns the <svg> header with the given options parsed
     *
     * @see https://developer.mozilla.org/en-US/docs/Web/SVG/Element/svg
     */
    protected function header():string{

        $width = null;
        $height = null;
        if ($this->options->scale) {
            $width = sprintf(' width="%1$s"', $this->options->scale);
            $height = sprintf(' height="%1$s"', $this->options->scale);
        }
        $header = sprintf(
            '<svg xmlns="http://www.w3.org/2000/svg" class="qr-svg %1$s" viewBox="%2$s"%3$s%4$s preserveAspectRatio="%5$s">%6$s',
            $this->options->cssClass,
            $this->getViewBox(),
            $width,
            $height,
            $this->options->svgPreserveAspectRatio,
            $this->eol
        );

        if($this->options->svgAddXmlHeader){
            $header = sprintf('<?xml version="1.0" encoding="UTF-8"?>%s%s', $this->eol, $header);
        }

        return $header;
    }
codemasher commented 1 year ago

I was experimenting in the past but there's no "best" way to set the size for the SVG, so i decided to leave this to the user entirely. the $scale option in combination with the width and height attributes isn't the best as it would fix the size which is often not preferred (ImagMagick requires them for properly sized output, though). SVG output is almost exclusively used in a web browser where CSS is available, and if it isn't, the size can be changed via CSS in the <defs> element or the aforementioned attributes.

fr3nch13 commented 1 year ago

Sigh, yeah. The whole point of the "S" in SVG. The issue I'm coming across, specific to me. I'm trying to import the QR codes, and the viewport is 0 0 41 41. which gimp makes a 41x41 pixel import. I can scale it up when importing, but it then becomes really fuzzy, like trying to upscale a jpg/png/etc. I was just trying to solve this from an import perspective.

codemasher commented 1 year ago

That sounds like the same issue i had when converting via ImageMagick - try the output from that example and see what happens.

fr3nch13 commented 1 year ago

Ok, I just pulled/composer update and I'm running on dev-main.

I copy-pasta'd your svg example here: https://github.com/chillerlan/php-qrcode/blob/main/examples/svg.php

and getting a viewport of 53 and it's trying to import as a 53x53 image:

<svg xmlns="http://www.w3.org/2000/svg" class="qr-svg qrcode" viewBox="0 0 53 53" preserveAspectRatio="xMidYMid">
fr3nch13 commented 1 year ago

I could be wrong, but it'd be nice to have the svg be large by default, for importing into things like gimp/photoshop/cad/non-html, browser. Then make them scale it down with css/html attributes.

codemasher commented 1 year ago

That is correct. I meant try the output from the imagick conversion example with the changed <svg> element.

codemasher commented 1 year ago

I'm going to close this issue here. Feel free to open a discussion for further questions.

codemasher commented 1 year ago

I could be wrong, but it'd be nice to have the svg be large by default, for importing into things like gimp/photoshop/cad/non-html, browser. Then make them scale it down with css/html attributes.

Like i said, that is not the main focus or use case for SVG output and the reason why i decided not to add these attributes.

fr3nch13 commented 1 year ago

Oh, derp.

I got: <svg xmlns="http://www.w3.org/2000/svg" class="qr-svg qrcode" viewBox="0 0 41 41" preserveAspectRatio="xMidYMid" width="205" height="205">

fr3nch13 commented 1 year ago

I could be wrong, but it'd be nice to have the svg be large by default, for importing into things like gimp/photoshop/cad/non-html, browser. Then make them scale it down with css/html attributes.

Like i said, that is not the main focus or use case for SVG and the reason why i decided not to add these attributes.

Gotcha, well at least you made it extendable, and you did a great job on the documentation, I personally appreciate that part! :-p

codemasher commented 1 year ago

Glad to hear because that's exactly why i'm doing this.

fr3nch13 commented 1 year ago

Well I don't mind being your userland guinea pig.

I'm using it over here, in case you're curious: https://qr.fr3nch.com

fr3nch13 commented 1 year ago

I chose yours, because it's much more configurable, and it runs locally, verse google's graph api.

I also wasn't going to pay bitly and other QR code hosters $35/month when I can make my own site.

codemasher commented 1 year ago

Ah yes the QR code scammers. Still a wild concept to me so generate a QR code and charge for it on a monthly basis - for tracking statistics you can get for free elsewhere. If i had the time and capacity i'd run such a website for free haha. Oh wait, i did (cheaply)