Support device selection, migrate CLI to Typer

[maresb]

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]

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 https://github.com/labelle-org/labelle/pull/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]

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]

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.

https://github.com/labelle-org/labelle/blob/dd5b3b370f728124f2d6178dabdb90babc544149/pyproject.toml#L194

[maresb]

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]

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]

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]

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:

1. If multiple devices are available, check in labelle.ini if a preferred device (ordering?) has been set. (Obviously this setting is not yet implemented.)
2. 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 https://github.com/computerlyrik/dymoprint/issues/109.

[tomek-szczesny]

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.

[maresb]

@tomers, please see #19 for yet another fix to the mypy settings :see_no_evil:

Those commits were associated with https://github.com/labelle-org/labelle/pull/12#issuecomment-2068207856

[tomers]

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 just added a --device flag which should allow exactly this.

[maresb]

Another review round in #20.

[maresb]

For simplicity of code require explicit --text argument for the lab

There are advantages and disadvantages, but this is a pretty major breaking change. Do you think this is really worth it?

[tomers]

For simplicity of code require explicit --text argument for the lab

There are advantages and disadvantages, but this is a pretty major breaking change. Do you think this is really worth it?

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

[maresb]

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!!! :star_struck:

[maresb]

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]

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]

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.

[tomers]

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]

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...

[maresb]

@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]

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]

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