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 possibility to show scissors instead of text if printable=false #258

Closed
wants to merge 13 commits into from

Conversation

tafel
Copy link
Contributor

@tafel tafel commented Aug 28, 2024

This PR adds the opportunity to replace the text "Separate before paying in" with scissors icons.
It addresses issue #180 and #212

// example with TCPDF
$tcPdf = 'an instance of TCPDF';
$output = new QrBill\PaymentPart\Output\TcPdfOutput\TcPdfOutput($qrBill, 'en', $tcPdf);
$options = new QrBill\PaymentPart\Output\PrintOptions();
$options
    ->setPrintable(false)
    ->setVerticalSeparator(true); // TRUE to show scissors instead of text
$output
    ->setPrintOptions($options)
    ->getPaymentPart();

Tests
All tests are ok. New PDF and HTML files are created so we can see the final result. These are not commited in this PR.

About TCPDF
This implementation should be fully compatible with TCPDF, withtout any external libraries.

About HTML
The CSS code works on chrome browsers. Not sure if it's 100% compatible with all browsers (use :before and :after pseudo-element, and transform property)

About FPDF
Dashed line and text rotation are not supported by default. It seems the only way to add these functionalities is to extends FPDF and add methods (to access protected props and methods, like $this->k or $this->_out()).

Scissors still appear, but lines are not dashed, and the vertical scissors are not rotated.

@kohlerdominik
Copy link
Contributor

This looks solid. Good work.

Are the fonts you used for the _pdfOutputs common across production systems (ubuntu/debian, alpine, probably windows?)

@tafel
Copy link
Contributor Author

tafel commented Aug 29, 2024

Thanks @kohlerdominik

All fonts (freeserif for TCPDF, zapfdingbats for FPDF, unicode char for HTML) are all bundled into their respective package/browser.

There are three unknown factors, which are:

  1. how does it react on Windows and Mac? I use Linux, and I made my tests within a docker container (php:8.2-cli)
  2. I'm not a CSS expert. So the HTML part may not be compatible with some browsers (tested only in Chrome and Firefox under ubuntu 22)
  3. does the HTML unicode char display correctly with any <meta charset="" /> (yes it does, now with CSS unicode code)

About FPDF (again)
To address the partial support with FPDF, I had the idea of creating a Trait, bundled into the php swiss qr bill code, which could be used to add the missing methods into the FPDF object.

I added a commit with this proposition

@tafel
Copy link
Contributor Author

tafel commented Aug 29, 2024

Regarding #212 , the way layout options are managed seems not optimal (I added a scissors property directly into AbstractOutput class).

Maybe we could identify all possible options, and create a new class (\Sprain\SwissQrBill\PaymentPart\Output\Layout ?) to manage them?

From what I saw, here are some (or all?) options:

option name type values default comment
printable bool true , false false
line string none , solid , dashed solid May not be setted by consumer. The scissors and printable states would manage this
scissors bool true , false false
verticalScissorsPosition string top , bottom top
font string helvetica , frutiger helvetica Maybe the font could be totally custom? In that case, does it violate the QR standard?
textDownArrow bool true , false false

Anyway, all these params could be set on the layout class. And then, this layout can be used to interact with outputs.

Any thoughts about this?

@sprain
Copy link
Owner

sprain commented Aug 30, 2024

Thanks guys, you are awesome!
Will look into this asap. Busy times …

@kohlerdominik
Copy link
Contributor

kohlerdominik commented Aug 30, 2024

@tafel you might want to look into the tests. They should be all green.

Another input from my side, maybe @sprain has some opinion on this as well:
I don't think the property should be called scissors. What about separaterSymbol? Or maybe there is a specific title for it in the (english) specs?

Edit: about your suggestion from the layout - this is really something that sprain has to decide. In my opinion, the best modern approach nowadays is a dedicated options-object (probably readonly, but as long as cloneWith is not an option in php, I dislike readonly). However, less forward facing devs might have issues understanding the pattern.

enum LineType {/* ... */}
enum SeparatorSymbol {/* ... */}
enum SeparatorSymbolPosition {/* ... */}
enum Font {/* ... */}


class LayoutOptions
{
  public bool $printable = false;
  public LineType $line = LineType::solid;
  public SeparatorSymbol $separatorSymbol = SeparatorSymbol::scissors;
  public SeparatorSymbolPosition $separatorSymbolPosition = SeparatorSymbolPosition::top
  public string|Font $font = Font::helvetica;
  public bool $textDownArrow = false;
}

@tafel
Copy link
Contributor Author

tafel commented Aug 30, 2024

Here is what I came with. It's quite the same idea as you (it's only pseudo-code). I agree that @sprain must be the referee ;)

<?php declare(strict_types=1);

namespace Sprain\SwissQrBill\PaymentPart\Output;

enum LineStyles
{
    case SOLID;
    case DASHED;
}

enum VerticalSeparatorSymbolPositions
{
    case TOP;
    case BOTTOM;
}

enum Fonts
{
    case DEFAULT;
    case UTF8;
}

final class PrintOptions
{
    private bool $printable;
    private bool $separatorSymbol;
    private VerticalSeparatorSymbolPositions $verticalSeparatorSymbolPosition;
    private bool $textDownArrows;
    private LineStyles $lineStyle;
    private bool $text;
    private string|Fonts $font;

    public function __construct()
    {
        // set default values. Should not generate breaking changes with existing codes
        $this->setSeparatorSymbol(false);
        $this->setPrintable(false);
        $this->setVerticalSeparatorSymbolPosition(VerticalSeparatorSymbolPositions::TOP);
        $this->setFont(Fonts::DEFAULT);
        $this->setTextDownArrows(false);
        $this->setText(true);
    }

    /**
     * Define printable state. If true, nothing is displayed (no line, no text)
     */
    public function isPrintable(): bool;
    public function setPrintable(bool $value): static;

    /**
     * Define scissors state. If true, lines will be dashed, and will hwve scissors
     */
    public function hasSeparatorSymbol(): bool;
    public function setSeparatorSymbol(bool $value): static;

    /**
     * Define down arrows next to "detach" text. If true, will print 3 arrows on right and left of the text
     */
    public function hasTextDownArrows(): bool;
    public function setTextDownArrows(bool $value): static;

    /**
     * Define "detach" text state. If false, no text is displayed
     */
    public function hasText(): bool;
    public function setText(bool $value): static;

    /**
     * Vertical scissors can be on top of the payment part, on at bottom
     */
    public function getVerticalSeparatorSymbolPosition(): VerticalSeparatorSymbolPositions;
    public function setVerticalSeparatorSymbolPosition(VerticalSeparatorSymbolPositions $value): static;

    /**
     * Font usage. Either "default", or "utf8" if one needs to print special chars.
     * Problem with FPDF: true utf8 font is not bundled. We need to authorize custom string, so one can
     * load his own font
     */
    public function getFont(): string|Fonts;
    public function setFont(string|Fonts $value): static;

    /**
     * Line style. Either dashed or solid
     */
    public function getLineStyle(): LineStyles;
}

And how we can use it:

        $output = (new TcPdfOutput($qrBill, 'en', $tcPdf));
        $output
            ->setPrintOptions((new PrintOptions())->setPrintable(false)->setFont(Fonts::UTF8))
            ->setQrCodeImageFormat(QrCode::FILE_FORMAT_SVG)
            ->getPaymentPart();

About tests, I was not sure if I could commit all the new generated PDF documents. I can commit them all, for sure, if it's better.

EDIT: changed suggestion of class definition, based on feedbacks

@tafel
Copy link
Contributor Author

tafel commented Aug 30, 2024

@kohlerdominik I see the point of using options objects to manage all these states. But we need to keep in mind that not all combination should be possible.

Is it spec-compatible to have a separator-symbol on solid lines? Can we have the detach text, while printable is true (no line visible)?

If the spec doesn't say anything about that, we could let users configure all the props individually. But if we need to make checks, I'm more for setters which reequilibrate values between props, either by changing them, or by throwing exceptions.

About the separator symbol, I agree to use this name. But I think it's better to only have a bool, since it's not useful to let user choose a custom separator symbol. Moreover, this symbol is not the same across outputs.

PS: just as a reminder about fonts. Directly from SIX specification

Only the sans-serif fonts Arial, Frutiger, Helvetica and Liberation Sans are permitted in black

Which is problematic because none of them has special chars like ł or ą. For TCPDF, I use FreeSans , and for FPDF, I didn't find a way to load gracefully a suitable font until now...

@tafel
Copy link
Contributor Author

tafel commented Sep 26, 2024

I read a bit of documentation, to see what are the official options available. It seems there're only three:

  1. no line at all
  2. solid lines and text (only text without down arrows)
  3. dotted lines, with scissors symbol (no other symbol authorized)

See chapter 3.7 (https://www.six-group.com/dam/download/banking-services/standardization/qr-bill/ig-qr-bill-v2.3-en.pdf)

If the QR-bill with payment part and receipt or the separate payment part with receipt are generated
as a PDF document and sent electronically, the A6 format of the payment part and the receipt on the
left must be indicated by lines. Each of these lines must bear the scissors symbol or alternatively the
instruction "Separate before paying in" above the line (outside the payment part)
. This indicates to the
debtor that he or she must neatly separate the payment part and receipt if they want to forward the
QR-bill to their financial institution by post for payment, or settle it at the post office counter (post
office branches or branches of partner organizations).

So the available options should be:

  1. printable or not
  2. with text (and solid lines) or with scissors (and dotted lines)

Your thoughts?

@tafel
Copy link
Contributor Author

tafel commented Oct 22, 2024

I pushed a prototype of PrintOptions. All tests are good on my local machine.

I dropped the font managment, since full UTF-8/Unicode fonts are not in the spec.

Copy link
Owner

@sprain sprain left a comment

Choose a reason for hiding this comment

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

Thanks for your work!
I finally managed to have a look at this and added some comments.

In addition:

  • According to the style guide, it should be either the text or the scissors, not both. This PR currently adds both.

src/PaymentPart/Output/FpdfOutput/FpdfTrait.php Outdated Show resolved Hide resolved
src/PaymentPart/Output/AbstractOutput.php Show resolved Hide resolved
src/PaymentPart/Output/FpdfOutput/FpdfTrait.php Outdated Show resolved Hide resolved
src/PaymentPart/Output/FpdfOutput/FpdfTrait.php Outdated Show resolved Hide resolved
src/PaymentPart/Output/HtmlOutput/HtmlOutput.php Outdated Show resolved Hide resolved
@tafel
Copy link
Contributor Author

tafel commented Nov 5, 2024

@sprain I added all the necessary stuff so all your comments should be resolved. Thanks for your reviews.

I found that I used enums for print options, and enums are not compatible with PHP 8.0. I wrote an alternative. That's less clean than enums, but it should minimize breaking changes when/if you drop PHP 8.0 support.

Last thing, I regenerated all test files (in normal an hardened environment). I hope this time, tests will pass

Comment on lines 16 to 18
public static $SOLID = 'SOLID';
public static $DASHED = 'DASHED';
public static $NONE = 'NONE';
Copy link
Owner

Choose a reason for hiding this comment

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

You could use const, then no $ is needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sprain PHP 8.0 went EOL (no security fixes) 1 ago year. Do we really need to implement backward facing implementations like this to maintain compatibility with it?

Copy link
Contributor Author

@tafel tafel Nov 5, 2024

Choose a reason for hiding this comment

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

lol, I just forgot this, thanks :D I replaced static by const. But that's true that PHP 8.0 is old. We may drop the support. But it means a breaking change...

Copy link
Owner

Choose a reason for hiding this comment

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

@kohlerdominik
PHP 8.0 is only 5% of current installs. We can consider dropping it. In a future release, since this is already PHP 8.0 compatible now ;)

@tafel
Thanks!
FYI: dropping a PHP version is not a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sprain ok, so composer manage matches between PHP version and packages version itself. The question is, in the current implementation, we can use the ::TOP const property, but we can also just write 'TOP' as a string. When/If we migrate the code to enums, it will be a breaking change, because the method will only accept enum type.

Should we go with the current implementation, and document this breaking change later? Or is it better to just drop PHP 8.0 right now, and not worry about breaking change at all?

@sprain
Copy link
Owner

sprain commented Nov 7, 2024

Thanks for all your work here!

I continued working on your code over in #264.
Especially, I worked on naming to make it more user-friendly and also simplified your new class.

I just need to handle the tests that I messed up, then we are close to a release 💪

@sprain sprain closed this Nov 7, 2024
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.

3 participants