computerlyrik / dymoprint

Linux Software to print with LabelManager PnP from Dymo
Apache License 2.0
150 stars 51 forks source link

Various fixes and improvements #101

Closed tomers closed 5 months ago

tomers commented 5 months ago

Give some love to this nice tool. I would like to do some changes in it, so I go over the code first, and fix what I think should be fixed. I would be grateful in the maintainer of this tool would accept these changes.

maresb commented 5 months ago

Hi @tomers, thanks so much for the excellent cleanup!!!

I have a single request regarding your changes. I don't like adding the entire matplotlib as a dependency just so that we can discover fonts. I created a branch called vendor-matplotlib. If you merge that branch into here, then you should be able to remove the matplotlib dependency and access findSystemFonts with

from dymoprint._vendor.matplotlib.font_manager import findSystemFonts
tomek-szczesny commented 5 months ago

Just wanted to extend the gratitude for this work you're putting into this code. Certainly I'm not qualified to do any of this.

I wouldn't mind a limited selection of system fonts, it may become overwhelming if it's too many, and I assume that instant preview of fonts like in LibreOffice may be too much to ask for.

tomers commented 5 months ago

Just wanted to extend the gratitude for this work you're putting into this code. Certainly I'm not qualified to do any of this.

I wouldn't mind a limited selection of system fonts, it may become overwhelming if it's too many, and I assume that instant preview of fonts like in LibreOffice may be too much to ask for.

Let's add it as-is, with all the fonts. I find the overwhelming amount to be preferrable over having missing a desired font. I definitely would like to add a preview to all fonts, but not in this iteration.

tomers commented 5 months ago

@maresb, @tomek-szczesny , can you please merge this now? I would like to keep working on this, but there's too many changes in this PR now...

maresb commented 5 months ago

I'm really excited about the live updating of the button when the device is ready or not.

Feel free to limit scope on this PR, this is already quite a lot!

I tried running it, but I'm getting the following error message, apparently due to inconsistency between Path and str types.

Traceback (most recent call last):
  File "~/dymoprint/src/dymoprint/q_dymo_label_widgets.py", line 145, in render_label
    render = self.render_engine.render_text(
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "~/dymoprint/src/dymoprint/dymo_print_engines.py", line 205, in render_text
    font = ImageFont.truetype(font_file_name, font_size_px)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "~/dymoprint-dev/lib/python3.12/site-packages/PIL/ImageFont.py", line 797, in truetype
    return freetype(font)
           ^^^^^^^^^^^^^^
  File "~/dymoprint-dev/lib/python3.12/site-packages/PIL/ImageFont.py", line 794, in freetype
    return FreeTypeFont(font, size, index, encoding, layout_engine)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "~/dymoprint-dev/lib/python3.12/site-packages/PIL/ImageFont.py", line 226, in __init__
    self.font = core.getfont(
                ^^^^^^^^^^^^^
TypeError: argument 1 must be str, bytes or bytearray, not PosixPath

In order to get this traceback I had to modify https://github.com/computerlyrik/dymoprint/blob/caf47b19732e7bdf8118941f1eebd083f3b3fd62/src/dymoprint/q_dymo_label_widgets.py#L155 and replace errtraceback.format_exc(). (Also import traceback goes up top.) There are three other instances in this file which should probably be updated accordingly.

maresb commented 5 months ago

We could already start merging this in pieces. If I merge the vendoring stuff, then if you rebase it'll trim down quite a bit on the massive number of files modified.

tomers commented 5 months ago

Thanks @maresb for this quick and useful feedback. I've fixed all the issues you've reported.

maresb commented 5 months ago

Looks like you got it @tomers, it's running for me! And I see loads of fonts.

I'm on the train at the moment, so I can't test printing myself. @tomek-szczesny, is this something you could try out? If it works for you too, then I'll cut a release.

maresb commented 5 months ago

@tomers, would you be willing to write the release notes for this?

tomers commented 5 months ago

@maresb where can I write the release notes? I didn't notice any RELEASES file

tomers commented 5 months ago

Release notes:

What's Changed

- Use installed system fonts
- Development improvements:
  - It is now possible to develop with Dev Container in VS Code
  - `gui_dev.sh` script to aid developers iterating on the GUI application
- Print button is now available only when the printer is ready. Otherwise the cause of error is showing.
- Some internal code refactoring and cleanups
tomek-szczesny commented 5 months ago

I'm sorry I can't do any testing in next 48h either :(

maresb commented 5 months ago

Perfect @tomers , there is no file in the repo, but I will paste that into the GitHub release notes

maresb commented 5 months ago

Let's merge for now but delay the release until someone else tests with a device.