barcode with text - Githubissues

putzio commented 9 months ago

There was no option to print the text under the generated barcode. I have just tested it on the Dymo 280 and 12mm tape, so it does not have to work in all other cases.

tomek-szczesny commented 9 months ago

For now you can print text next to a barcode, but such future feature makes a lot of sense to me.

maresb commented 9 months ago

Thanks a lot for this!

I won't be available to properly review this for a few weeks. @tomek-szczesny, would you be able to review and test this one for me?

It seems that we're duplicating the barcode choices, and the list is long enough that it's not clear to me whether the two lists are identical. To prevent drift and increase clarity, I'd prefer to define a constant list BARCODE_CHOICES which we reference in each add_argument choices parameter.

Camel case is unconventional in command-line arguments, so a more standardized choice here would be --print-code. I think we can do better with the name since print feels redundant. I'd be inclined to add --barcode as an alternative to -c for the case without text, and then --barcode-text (with no short form) for this new one. What do you think? Can we do even better?

I don't have time right now to look into this now, but a common use case for barcodes is encoding large integers like UPCs. I'm guessing that in this case those integers are not ASCII-encoded but raw, perhaps unsigned? What I'm getting at is: are there potential conflicts or options we should support when printing text below the barcode?

And finally, I don't see anything here related to the new dymoprint_gui. Do we want to insist on feature parity between dymoprint CLI and dymoprint_gui? I wonder if @Moovx would be available to help us with this?

Don't feel like you have to address all these points perfectly. I'm just trying to give as thorough a review-at-a-glance as possible.

tomek-szczesny commented 9 months ago

Barcodes usually contain only printable characters, although QR codes may also contain newlines, as far as I can tell. I would stick to whatever can be passed via command line as string. Encoders check validity of input data already. So I would anticipate no changes here, it seems to work fine, I guess?

As for parity with dymoprint_gui, it is desirable but I wouldn't reject this PR if @putzio doesn't want to implement it. As the last resort it may be listed as another issue with "enhancement" and "help wanted" tags.

I tested the code, and for any reason --printCode version have inverted colors compared to -c option. This is also visible in -n terminal preview. Most notably, text under barcode is white on black background. I tested it with code128 and ean codes. Incidentally I have "white on black" tape loaded into my PnP today and it looks okay, but I don't think this is a desired outcome. Having said that, I wouldn't pull this code unless this is fixed, because these codes are unscannable on standard black-on-white tapes.

I would also center the text under a barcode.

As for the implementation, I find it strange the image must be temporarily saved in filesystem. Can we go around that somehow?

I agree the new option should be called something else. How about -ct, like existing -c but with text? Unless short options must be a single character, then I'd vote for -b for "barcode", simply. And then change the parameter description for -c as "Barcode only, no text".

2023-10-08-114148_588x308_scrot 2023-10-08-114127_619x364_scrot

maresb commented 9 months ago

Thanks @tomek-szczesny for the review and testing! I'm thinking that my concern about encoding may be unfounded. I'm guessing that the "choices" parameter accounts for the encoding via the python-barcode package, and in the case of a numerical code it would only accept numerical characters.

I really dislike short options: not expressive, memorable, sustainable, etc. I'd vote against adding any new ones, keeping the old ones only for backwards compatibility.

Regarding the inverted colors, I agree it's a blocker. I'm guessing there could be something unconventional happening in the main code. If this is the case, then it'd be preferable to fix that rather than propagating weirdness to new code. But if it's not obvious, we may want to tackle that in a new PR.

maresb commented 9 months ago

@tomek-szczesny, does this solve the inverted colors?

This solution looks really hacky to me, probably through no fault of @putzio. Specifically, why do we have to write such hacky-looking code in order to add a new feature like this?

In any case we shouldn't be saving to a file like this. What would need to change in order to do this better?

tomek-szczesny commented 9 months ago

Yeah, I don't mean to be ungrateful for this contribution but I think the code quality must be addressed before we continue testing it, let alone pulling it.

I'm no expert in Python nor dymoprint code, but writing temporary files must be avoided. How about copying the existing barcode generating code, then just add a white rectangle, and then text in it? That should work.