Support device selection, migrate CLI to Typer #12

tomers · 2024-04-20T18:49:28Z

No description provided.

maresb

This looks great! But now the CLI is broken for me.

$ labelle --verbose a
[DEBUG] Read config file: ~/.config/dymoprint.ini
[DEBUG] Found single device
[DEBUG] <DEVICE ID 0922:1001 on Bus 001 Address 012>
  manufacturer: Dymo
  product: DYMO LabelManager PnP
  serial: 10581529032012
  configurations:
  - <CONFIGURATION 1: 500 mA>
    interfaces:
    - <INTERFACE 0: Human Interface Device>
    - <INTERFACE 1: Mass Storage>

[DEBUG] Recognized device as LabelManager PnP (no mode switch)
[DEBUG] Active device configuration already found.
[DEBUG] Opened HID interface: <INTERFACE 0: Human Interface Device>
[DEBUG] Printing label..
[ERROR] 
Traceback (most recent call last):
  File "~/repos/dymoprint/src/labelle/lib/utils.py", line 49, in system_run
    yield
  File "~/repos/dymoprint/src/labelle/cli/cli.py", line 351, in main
    run()
  File "~/repos/dymoprint/src/labelle/cli/cli.py", line 345, in run
    dymo_labeler.print(bitmap)
  File "~/repos/dymoprint/src/labelle/lib/devices/dymo_labeler.py", line 331, in print
    self._functions.print_label(label_matrix)
  File "~/repos/dymoprint/src/labelle/lib/devices/dymo_labeler.py", line 264, in _functions
    assert self._device is not None
AssertionError

There are some problems in the surrounding code that should have been caught by mypy, so I opened #13. Sorry about that!!! Hopefully mypy will just work from here on out.

In terms of the UI layout, having the selector in the upper-left makes the window taller. I think it'd be more natural to place the selector to the left of the print icon, but no need to do that in this PR.

Not a big deal, but I'm not such a fan of declaring the private interfaces with a leading _. The reason is that the benefit of most conventions is to decrease entropy, but this does the opposite. Now when I'm trying to remember a name or come up with a new name, it's another thing I must think about, and it seems to me like a very subjective decision. Probably you have a clear idea in your head, but it's not entirely clear to me. So it might slightly increase friction for other contributors, and it'll probably be up to you to maintain the private/public distinction. Please don't change anything though, we'll just see how the code evolves.

As for how to proceed, what do you think of merging or rebasing on #13, turning the CI green, and cutting a new release? It'd be nice to ensure that you can run the release workflow yourself.

src/labelle/lib/devices/usb_device.py

maresb · 2024-04-21T10:00:54Z

I don't like how merging is blocked for maintainers when changes are requested. I'm not sure where the setting is for this:

tomers · 2024-04-21T10:46:50Z

Note: sorry for the many edits I had with this reply... I guess you got flooded by email notification.

In terms of the UI layout, having the selector in the upper-left makes the window taller. I think it'd be more natural to place the selector to the left of the print icon, but no need to do that in this PR.

I agree that UI is sub optimal. I think we should reshape the entire UI, but it is not in my focus at the moment, and I believe changing anything at the moment will be premature optimization, as things might change soon.
For example, I think label selection (label height, colors) are associated with the labeler. A user might have multiple labelers with different colors. I think the widgets associated with label type selection and labeler selection should be grouped. I am not sure how we should 'memorize' which label is associated with which printer, but I think such association is needed. I guess we should add this to the configuration file at some point.

Not a big deal, but I'm not such a fan of declaring the private interfaces with a leading _. The reason is that the benefit of most conventions is to decrease entropy, but this does the opposite. Now when I'm trying to remember a name or come up with a new name, it's another thing I must think about, and it seems to me like a very subjective decision. Probably you have a clear idea in your head, but it's not entirely clear to me. So it might slightly increase friction for other contributors, and it'll probably be up to you to maintain the private/public distinction. Please don't change anything though, we'll just see how the code evolves.

I agree. I think that at some point we had file with many functions, and it was hard to notice which was internal function and which function intended to be used from outside. While making latest changes, I also felt that I am slowly being trapped by these new convention, with no significant gain. So maybe we should indeed remove this in the near future. I'll keep it as it is now, since I want to merge my changes soon.

As for how to proceed, what do you think of merging or rebasing on #13, turning the CI green, and cutting a new release? It'd be nice to ensure that you can run the release workflow yourself.

Sure. I would like to try this. I would need the keys for https://pypi.org/project/labelle/.

maresb · 2024-04-21T18:18:57Z

I would need the keys for https://pypi.org/project/labelle/.

There are no keys. I set things up with the new OIDC mechanism.

GitHub Actions is trusted by PyPI, and it generates a certificate saying that a package upload request is coming from labelle-org/labelle under the release environment. And PyPI is configured to accept exactly such a certificate:

So if everything is set up correctly, then all you have to do is "Draft a new release":

Choose a new tag name according to semver and set to "Create new tag on publish":

Click "Generate release notes" to get automatic release notes based on the merged PRs, add any additional notes, and hit "Publish release".

That will trigger a pending Actions workflow to upload to PyPI that requires manual approval from a maintainer. Approve, then it will run, and should create a release.

maresb · 2024-04-21T19:14:37Z

Hmm, _vendor should have been ignored by mypy. The idea is that you should never need to change vendored code except through an explicit patch.

labelle/pyproject.toml

Line 194 in dd5b3b3

exclude = ["_vendor"]

maresb · 2024-04-21T21:34:03Z

That should fix the mypy errors from _vendor.

Would you be able to revert the changes to font_manager.py? It's supposed to be regeneratable via

pip install vendoring
vendoring sync

(Alternatively, there's vendoring/patches/matplotlib.patch, but currently it consists of only deleting lines.)

maresb · 2024-04-21T21:34:40Z

Any ideas about how to do device selection from the CLI? It'd be nice to get that sorted out before the next release.

tomek-szczesny · 2024-04-22T06:48:06Z

Any ideas about how to do device selection from the CLI? It'd be nice to get that sorted out before the next release.

I'd like to point out that from GUI perspective, the printer must be known before the user starts their design as label printers vary in capabilities obviously. It was a very hard thing to do with existing code, as far as I can remember.

One way to do it in CLI is to have a switch for listing all available printers, and another switch to select one of them for an actual print job.

But for the reason stated above, perhaps we should consider another approach: Persistent printer configuration. One command line switch to set a printer, which will be remembered until reset into something else. This will be more in line with GUI experience and simplify batch job scripts.

Better yet, a CLI switch for selecting a printer while the job is run, and the last used printer is to be treated as the default.

maresb · 2024-04-22T07:20:17Z

I'd like to point out that from GUI perspective, the printer must be known before the user starts their design as label printers vary in capabilities obviously. It was a very hard thing to do with existing code, as far as I can remember.

Are you sure? The way I see things is that we have an abstract representation, e.g. "barcode, then some particular text, then QR code", and this representation works for any printer. It's just the rendering process from the abstract representation to the bitmap that requires knowledge of the printer.

Perhaps the simplest way would be to use labelle.ini:

If multiple devices are available, check in labelle.ini if a preferred device (ordering?) has been set. (Obviously this setting is not yet implemented.)
If not, then print an error listing the available devices and explaining how to add them to labelle.ini.

Note that this makes it inconvenient to print alternately to multiple devices, since it requires editing a file each time. Perhaps in labele.ini we could define an alias for each device. Then we could use that alias as a CLI parameter.

I could imagine a common use case would be having two or more LabelManager PnP devices, each with a different tape color. So it would be nice if we could distinguish between different devices of the same model. (This should be straightforward using the serial number.)

@systeemkabouter, once we have both a CLI and GUI solution I think that will close computerlyrik/dymoprint#109.

tomek-szczesny · 2024-04-22T08:04:28Z

I think the user has to pick the printer first, and tape width / label size, so Labelle knows the dimensions and colors of a canvas.
The preview must be a pixel perfect representation of the printout, so the user won't be disappointed with the print results after spending time on design, and potentially wasting the medium just to know what the end result looks like.
I can't see it working in any other way. We can't just let the user slap anything on the canvas and then send it to Dymo with a 128 point header.
Sure, internally we can work on abstracts and all, but the user ought to see what they will get and nothing else.

I'm against complicating the printer selection logic. I'm considering a few user cases now:

Casual user that has just one label printer,
Semi-automated label printing station, with many printers, operated by external software calling labelle
The first case requires sane defaults. The second case requires consistency across reboots and software updates, and proper error handling.

Here's my proposed algorithm:

If labelle is called with a specified printer, then use it and save it as the default in ini file.
-- throw error instead, if the specified printer cannot be found
If labelle is called without a specified printer, try using the default from ini file:
-- if no default printer is specified, pick the first one of the available printers
-- If the default printer is specified but isn't available, throw error.

This way, the casual user will unknowingly configure their only printer, and the industrial user will not print a batch of labels on a wrong printer by accident.
Moreover, the batch script using labelle will pick the printer only once, and not on each labelle call, this is convenient.

I agree that "Printer identification string", whatever it might be, should be unique enough to differentiate between two printers of the same make and model. A serial number alone could be sufficient if only we knew for sure that all thermal printers have unique S/Ns. Some cheaper USB devices are known to not have unique S/Ns.

This is wrong, since src is NOT a module. It seems that I added it for Python 2.7 compatibility a long time ago, so it's definitely no longer needed.

This is mainly coding convention alignment to other parts of the code, where internal methods and attributes are prefixed with underscore. In this specific top level class it is a rather off to have everything prefixed, but I think it is important that we stick to some coding conventions.

This line is a leftover from earlier commits

Currently 'actions' is only print button and an error label. In the future we might have actions such as 'save to image'.

We will soon need to use it from other functions

We present image in imagemagick as output from the PrintPreviewRenderEngine. This image object is of mode=RGBA, while invert only support RGB, so it needs to be converted.

Remove some bloat from cli.py and move output handling to a dedicated file.

The logic in this module does not use this enum yet, but for good order it should be there.

src/labelle/lib/constants.py

❯ labelle --help Usage: labelle [OPTIONS] COMMAND [ARGS]... ╭─ Options ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │ --version Show application version │ │ --verbose -v Increase logging verbosity │ │ --test-pattern INTEGER Prints test pattern of a desired dot width [default: None] │ │ --output [printer|console|console_inverted|browser|imagemagick] Destination of the label render [default: printer] │ │ --install-completion Install completion for the current shell. │ │ --show-completion Show completion for the current shell, to copy it or customize the installation. │ │ --help Show this message and exit. │ ╰───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ ╭─ Device Configuration ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │ --device TEXT Select a particular device by filtering for a given substring in the device's manufacturer, product or serial number [default: None] │ │ --tape-size-mm INTEGER Tape size [mm] [default: None] │ ╰───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ ╭─ Elements ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │ --text TEXT Text, each parameter gives a new line [default: None] │ │ --qr TEXT QR code [default: None] │ │ --barcode TEXT Barcode [default: None] │ │ --barcode-type [code39|code128|ean|ean13|ean8|gs1|gtin|isbn|isbn10|isbn13|issn|jan|pzn|upc Barcode type [default: (code128)] │ │ |upca] │ │ --barcode-with-text TEXT Barcode with text [default: None] │ │ --picture PATH Picture [default: None] │ ╰───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ ╭─ Design ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │ --style [regular|bold|italic|narrow] Set fonts style [default: regular] │ │ --frame-width-px INTEGER Draw frame around the text, more arguments for thicker frame [default: None] │ │ --align [left|center|right] Align multiline text [default: left] │ │ --justify [left|center|right] Justify content of label if label content is less than the minimum or fixed length [default: left] │ │ --font TEXT User font. Overrides --style parameter [default: None] │ │ --font-scale FLOAT Scaling font factor, [0,100] [%] [default: 90] │ ╰───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ ╭─ Label Dimensions ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │ --min-length FLOAT Minimum label length [mm] [default: None] │ │ --max-length FLOAT Maximum label length [mm], error if the label won't fit [default: None] │ │ --fixed-length FLOAT Fixed label length [mm], error if the label won't fit [default: None] │ │ --margin-px FLOAT Horizontal margins [px] [default: 56] │ ╰───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯ ╭─ Commands ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮ │ list-devices │ ╰───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯

Update the list from the package's source [1]. I sorted the keys, to the sake of readability. [1] https://github.com/WhyNotHugo/python-barcode/blob/main/barcode/__init__.py#L35

maresb · 2024-04-26T11:14:08Z

Yes, it is definitely worth it. Please consider my additional commits. I've overhauled the CLI, and these changes were part of this effort.

I still don't understand. Change typer.Option to typer.Argument in text and it's no longer a breaking change.

Amazing work with Typer!!! 🤩

maresb · 2024-04-26T11:16:50Z

This PR is pretty crazy. Before it made a lot more sense when you were refactoring everything, but I don't think it makes sense to have these two completely different features (USB device selection and Typer) in the same PR.

Let's keep it for now, but not make this a habit.

tomers · 2024-04-26T11:17:31Z

I still don't understand. Change typer.Option to typer.Argument in text and it's no longer a breaking change.

Who said text label is a must? What makes text any more important than, say, a QR code? I wanted to reflect that in the API, that text is just something that we add, with no assumptions that this is what most people do with labels.

maresb · 2024-04-26T11:28:31Z

I totally agree in principle, but this is still a breaking change, and I don't see any necessity.

Think of it from the user perspective. We're asking people to switch from dymoprint to Labelle, and then they try it out and it doesn't work in the same way. The interface has been like this for the entire lifetime of the dymoprint project. People would need to adapt their habits, and also they may have built scripts that they now have to adjust.

I don't think it's that huge of a deal, and I agree that we should switch eventually, but the benefit of this seems the benefit over the disruption of introducing such a breaking change over the previous long-standing interface of many years shortly after a new release.

This is also a reason I dislike these long PRs. This discussion is holding all your previous work hostage.

How about this: let's convert it to Argument for now so that we can get this merged and released as v1.2.0. Then we create a 1-line PR to convert it back to Option that we save for when we decide to make a 2.0 release.

This is done to avoid breaking compatibility.

tomers · 2024-04-26T11:36:04Z

ok, I agree with what you said.
Actually, the mandatory text argument stood in my way many times with argparse, thus I wanted to remove it so badly.
Now with typer and the support for commands, it is no longer necessary, and I can live with keeping the API for BC.
I wanted to keep all my progress in a single PR, in order to save me from juggling between branchs, but I agree that I went too far with that :-) sorry

tomers · 2024-04-26T12:15:04Z

Note that following my change from Option to Argument, Typer cannot distinguish between command (e.g. list_devices and a text label.
I suggest we keep as is, release a new version as you suggested, and then we fix this by introducing the breaking change and moving forward to a new major release.
Please approve this PR as-is, so we can conclude with this giant change...

We cannot execute any command, as long as text label is an argument and not an option. For now, we will hide this new functionality, and relase a new minor version. Then we can introduce a --text CLI option, and expose this command (and any future commands).

maresb

Looks great, thanks @tomers!

maresb · 2024-04-26T17:48:07Z

@tomers, feel free to give the release process a try for v1.2.0. I hope it is extremely easy. If not, please let me know.

tomers · 2024-04-26T18:12:49Z

Sure, @maresb. I am going for a 2 days vacation, so I would only be able to do it afterwards. If you want, and don't want to wait, you can release yourself, and I'll do the next release.

maresb · 2024-04-26T18:17:58Z

Ok, sure, I'll take care of it then. Enjoy your vacation!!! 🏖️

This is a breaking change. It puts text on the same level as the other options like image or barcode. It is breaking because it now requires any text to be preceeded by `--text`. Ref: <labelle-org#12 (comment)> and below.

This reverts commit c767a0a. This changes --text from typer.Argument to typer.Option This is a breaking change. It puts text on the same level as the other options like image or barcode. It is breaking because it now requires any text to be preceeded by `--text`. Ref: <labelle-org#12 (comment)> and below.

tomers requested a review from maresb April 20, 2024 18:49

tomers force-pushed the tshalev-device-selector branch from 7d0202a to 087913b Compare April 20, 2024 19:02

maresb requested changes Apr 21, 2024

View reviewed changes

src/labelle/lib/devices/usb_device.py Show resolved Hide resolved

maresb mentioned this pull request Apr 21, 2024

Fix mypy #13

Closed

tomers force-pushed the tshalev-device-selector branch from 087913b to 739e757 Compare April 21, 2024 15:22

tomers force-pushed the tshalev-device-selector branch from 3b43eed to 671eeea Compare April 21, 2024 18:47

maresb and others added 15 commits April 24, 2024 05:59

Remove superfluous __init__.py in src/

3b07f13

This is wrong, since src is NOT a module. It seems that I added it for Python 2.7 compatibility a long time ago, so it's definitely no longer needed.

Configure mypy path and packages

80bb84b

Initialize class attribute

a0e6cd7

This line is a leftover from earlier commits

Extract settings controls to a separate widget

8125f3f

Organize imports

574c0af

Extract actions controls to a separate widget

b9302b7

Currently 'actions' is only print button and an error label. In the future we might have actions such as 'save to image'.

Remove unneeded layout complexity

d69d841

Remove Dymo from name of classes

b5bf6e7

Extract render control to a separate widget

cb84b6d

Keep render widget as class attribute

b9131df

We will soon need to use it from other functions

Add device selector widget

c5d875c

Separate OnlineDeviceManager manager from DeviceManager

c80ca63

Add support for future typing annotations

714312b

Fix device override

e708c2a

Fix typo

0e38a93

tomers force-pushed the tshalev-device-selector branch from 78cbd82 to 0e38a93 Compare April 26, 2024 06:47

Tomer Shalev added 4 commits April 26, 2024 10:24

Make enum keys uppercase

382955a

Fix imagemagick output

db6374e

We present image in imagemagick as output from the PrintPreviewRenderEngine. This image object is of mode=RGBA, while invert only support RGB, so it needs to be converted.

Extract output handling to a separate file

0c62568

Remove some bloat from cli.py and move output handling to a dedicated file.

Move Style to font_config as FontStyle

05e6d71

The logic in this module does not use this enum yet, but for good order it should be there.

maresb reviewed Apr 26, 2024

View reviewed changes

src/labelle/lib/constants.py Outdated Show resolved Hide resolved

Tomer Shalev added 2 commits April 26, 2024 11:03

Update barcode types

db60a61

Update the list from the package's source [1]. I sorted the keys, to the sake of readability. [1] https://github.com/WhyNotHugo/python-barcode/blob/main/barcode/__init__.py#L35

Make text option an argument instead of an option

c767a0a

This is done to avoid breaking compatibility.

Unify Align and Justify enums to Directions enum

7ecd67b

Hide list_devices command

935aeaf

We cannot execute any command, as long as text label is an argument and not an option. For now, we will hide this new functionality, and relase a new minor version. Then we can introduce a --text CLI option, and expose this command (and any future commands).

maresb changed the title ~~USB device selector widget~~ Support device selection, migrate CLI to Typer Apr 26, 2024

maresb approved these changes Apr 26, 2024

View reviewed changes

tomers merged commit d18c36e into main Apr 26, 2024
6 checks passed

tomers deleted the tshalev-device-selector branch April 26, 2024 15:50

maresb mentioned this pull request Apr 28, 2024

Change --text from argument to option #28

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support device selection, migrate CLI to Typer #12

Support device selection, migrate CLI to Typer #12

tomers commented Apr 20, 2024

maresb left a comment

maresb commented Apr 21, 2024

tomers commented Apr 21, 2024 •

edited

Loading

maresb commented Apr 21, 2024

maresb commented Apr 21, 2024 •

edited

Loading

maresb commented Apr 21, 2024

maresb commented Apr 21, 2024

tomek-szczesny commented Apr 22, 2024

maresb commented Apr 22, 2024

tomek-szczesny commented Apr 22, 2024

maresb commented Apr 26, 2024

maresb commented Apr 26, 2024

tomers commented Apr 26, 2024

maresb commented Apr 26, 2024 •

edited

Loading

tomers commented Apr 26, 2024

tomers commented Apr 26, 2024

maresb left a comment

maresb commented Apr 26, 2024

tomers commented Apr 26, 2024

maresb commented Apr 26, 2024

Support device selection, migrate CLI to Typer #12

Support device selection, migrate CLI to Typer #12

Conversation

tomers commented Apr 20, 2024

maresb left a comment

Choose a reason for hiding this comment

maresb commented Apr 21, 2024

tomers commented Apr 21, 2024 • edited Loading

maresb commented Apr 21, 2024

maresb commented Apr 21, 2024 • edited Loading

maresb commented Apr 21, 2024

maresb commented Apr 21, 2024

tomek-szczesny commented Apr 22, 2024

maresb commented Apr 22, 2024

tomek-szczesny commented Apr 22, 2024

maresb commented Apr 26, 2024

maresb commented Apr 26, 2024

tomers commented Apr 26, 2024

maresb commented Apr 26, 2024 • edited Loading

tomers commented Apr 26, 2024

tomers commented Apr 26, 2024

maresb left a comment

Choose a reason for hiding this comment

maresb commented Apr 26, 2024

tomers commented Apr 26, 2024

maresb commented Apr 26, 2024

tomers commented Apr 21, 2024 •

edited

Loading

maresb commented Apr 21, 2024 •

edited

Loading

maresb commented Apr 26, 2024 •

edited

Loading