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

Various fixes and improvements #101

Merged
merged 17 commits into from
Jan 14, 2024
Merged

Various fixes and improvements #101

merged 17 commits into from
Jan 14, 2024

Conversation

tomers
Copy link
Contributor

@tomers tomers commented Jan 13, 2024

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.

@tomers tomers force-pushed the fixes branch 2 times, most recently from b18f2ce to fdf23e1 Compare January 13, 2024 15:34
@tomers tomers changed the title Remove duplicated lines Various fixes and improvements Jan 13, 2024
@maresb
Copy link
Collaborator

maresb commented Jan 13, 2024

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

Copy link
Collaborator

@maresb maresb left a comment

Choose a reason for hiding this comment

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

I haven't tested this, but it all looks really great.

scripts/gui_dev.sh Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@tomers tomers force-pushed the fixes branch 8 times, most recently from 62d4300 to 9cc331c Compare January 14, 2024 10:52
@tomek-szczesny
Copy link
Contributor

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
Copy link
Contributor Author

tomers commented Jan 14, 2024

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 tomers force-pushed the fixes branch 3 times, most recently from 90de2f0 to 2ef587a Compare January 14, 2024 12:13
@tomers
Copy link
Contributor Author

tomers commented Jan 14, 2024

@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
Copy link
Collaborator

maresb commented Jan 14, 2024

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

QMessageBox.warning(self, "TextDymoLabelWidget render fail!", f"{err}")
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
Copy link
Collaborator

maresb commented Jan 14, 2024

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.

@maresb maresb mentioned this pull request Jan 14, 2024
tomers and others added 9 commits January 14, 2024 15:43
'list' is a reserved Python word, and should not be used for class members
This allows for easier developing in VSCode
This allows for easy execution of the GUI app, and reloading it just by closing it.
It is useful when making changes in the code and a reload is needed.
This is sort of a workaround for an issue where in devcontainer the dependant packages are not known to the IDE.
This is due to the fact that these packages were installed in postCreateCommand which installed it under /usr/local/py-utils/venvs/dymoprint/lib/python3.12.
This is an alternative to open the preview with Imagemagick
Allow to choose system font by its name.
Add '--font' long command line option.
Hide the print button in case of bad status (no device, etc.).
Show error label in case of any error.
Poll status every 2 seconds.
Allow to choose system font by its name.
Add '--font' long command line option.
Show backtrace in warning dialog
@tomers
Copy link
Contributor Author

tomers commented Jan 14, 2024

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

@maresb
Copy link
Collaborator

maresb commented Jan 14, 2024

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
Copy link
Collaborator

maresb commented Jan 14, 2024

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

@tomers
Copy link
Contributor Author

tomers commented Jan 14, 2024

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

@tomers
Copy link
Contributor Author

tomers commented Jan 14, 2024

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
Copy link
Contributor

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

@maresb
Copy link
Collaborator

maresb commented Jan 14, 2024

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

@maresb
Copy link
Collaborator

maresb commented Jan 14, 2024

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

@maresb maresb merged commit 4da8137 into computerlyrik:master Jan 14, 2024
5 checks passed
@tomers tomers deleted the fixes branch January 18, 2024 20:39
tomers added a commit to tomers/dymoprint that referenced this pull request Jan 20, 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