Another batch of changes #113

tomers · 2024-02-02T18:16:21Z

Each commit does something else, so please read each one separately

maresb · 2024-02-03T10:37:57Z

I'm not quite following what is VERTICAL_PREVIEW_MARGIN_PX. Could you explain it?

Is the idea that for the preview we add a fake margin for the nonprintable area so that the preview resembles the physical tape after printing?

maresb · 2024-02-03T11:11:54Z

That's really nice with the adjustable margin in the preview. But it's a bit weird that the margin code for the preview is different from the margin code for the printer.

Looking more closely I see that the printer code prints no margin at the beginning and a double-margin at the end. I think this probably has to do with the gap between the printer head and the cutter. I'm guessing that the gap is approximately DEFAULT_MARGIN_PX pixels long. Then when we print 2 * DEFAULT_MARGIN_PX, where the first DEFAULT_MARGIN_PX is the true right-margin of the current label, and the second DEFAULT_MARGIN_PX is the left-margin of the next label. Do you agree with this assessment?

It might be nice to start distinguishing some of these concepts in the code. Like have a constant for GAP_BETWEEN_PRINT_HEAD_AND_CUTTER_PX = 13, and have separate variables in the code, something like current_right_margin and future_left_margin (or whatever makes sense), just to make the code somewhat consistent with reality.

In principle we could enable a margin-free mode where you print the first GAP_BETWEEN_PRINT_HEAD_AND_CUTTER_PX pixels, tell the user to cut, and print the remaining pixels plus future margin, and tell the user to cut again.

If the user wants a label with margin greater than GAP_BETWEEN_PRINT_HEAD_AND_CUTTER_PX, then I think the correct way to do this is:

Print a left margin of additional_left_margin = margin - GAP_BETWEEN_PRINT_HEAD_AND_CUTTER_PX, since we already have GAP_BETWEEN_PRINT_HEAD_AND_CUTTER_PX due to the gap from the previous label.
Print the main payload
Print a right margin of margin
Continue printing a margin of future_left_margin = GAP_BETWEEN_PRINT_HEAD_AND_CUTTER_PX.
(I think the current implementation is buggy since it'll produce too short of a left-margin and too long of a right-margin, but I'm not able to test at the moment.)

Am I making sense?

tomers · 2024-02-03T13:11:56Z

@marseb, thanks for the insights. I've contrinued to modify the code, and I am going to introduce some further changes soon. Thanks for the insight of explaining why we print the margins twice at the end. I was puzzled by that.
I will add some of your text as comments in the code, and I also consider taking the variable and constant names you suggest. Let's keep this PR open for a while, until I'm done with that.

maresb · 2024-02-03T13:41:21Z

Awesome, thanks so much for looking into that!!!

I've converted the PR to draft for now. When you're finished just hit "Ready for review" and I'll give it another look and merge if I approve.

tomers · 2024-02-03T16:12:29Z

Exactly. I still need to work on that part, but the idea is that preview should show both horizontal and vertical margins.

…

On Sat, Feb 3, 2024, 12:38 Ben Mares ***@***.***> wrote: Thanks @tomers <https://github.com/tomers>! I'm not quite following what is VERTICAL_PREVIEW_MARGIN_PX. Could you explain it? Is the idea that for the preview we add a fake margin for the nonprintable area so that the preview resembles the physical tape after printing? — Reply to this email directly, view it on GitHub <#113 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUL46ACLSJ36PFVNAFMEDYRYHRDAVCNFSM6AAAAABCXDDOQ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRVGI3TCNBYGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

maresb · 2024-02-03T16:31:26Z

Okay, great!

Keep in mind that there are a few different levels of abstraction coming together:

the bitmap itself that's to be printed
the way the bitmap is printed onto a piece of tape

I think there are valid use cases to have preview functionality for both. Unfortunately I don't have any clever ideas for an implementation.

Note that this is starting to connect up with @tomek-szczesny's work on a lookup table for various printer and label width combinations in #91.

It's really great how things are coming together thanks to your work @tomers! Back when #91 was opened, I don't think the code was healthy enough to carry through that implementation. But now the code is becoming much easier to understand, opening up lots of exciting possibilities!

tomek-szczesny · 2024-02-03T16:46:15Z

Certainly I'm glad someone's cleaning up my amateur code. :)
Just wanted to add, I regret that printer must be detected before rendering, in order to determine the actual printable area. I see no other way around it, barren the user-provided printer model as a command line parameter.

maresb · 2024-02-03T17:01:31Z

my amateur code

To be clear the obstacle wasn't your code, it was instead the underlying code which was very idiosyncratic with not-so-great separation of concerns and idiosyncratic variable names. Those sorts of things build up and create a tremendous amount of friction for making changes.

tomers · 2024-02-03T19:55:40Z

Thanks, @maresb and @tomek-szczesny!
I'm working on discarding margin handling in the printing driver. This adds mental load on understanding the code, and the principle of having to end the print job with an offset for the next print job, and will be a burden in the future, when we'll have multiple drivers (which would need to duplicate this task), and also in the case where we may want to implement some more complicated scenarios, such as printing batch of labels, with zero margin (i.e. print brank without ending with offset, cut the label, then iteratively printing labels without margins, or at least with SW defined margins, and instructing user to cut).

@maresb, regarding your comment about idiosyncratic code, that's indeed true, and the was my initial effort, to clean these things off the table, before approaching any other new features.

src/dymoprint/lib/utils.py

tomers · 2024-02-08T09:07:41Z

Hey @maresb and @tomek-szczesny,

Can you please review this PR?

Thanks,
Tomer

maresb

I will finish a PR soon with some more in-depth suggestions

src/dymoprint/lib/utils.py

maresb · 2024-02-10T13:41:44Z

Oh, looks like you're actively working on this. I'll hold off on further review for the moment.

maresb · 2024-02-10T13:42:18Z

Are you okay with the conflicts there? Looks like maybe you accidentally rebased on a stale branch?

tomers · 2024-02-10T14:51:59Z

It is ready for review now. Sorry, but I use git rebase a lot, and I keep forgeting to rebase on origin/master.

tomers · 2024-02-12T10:35:32Z

Any news?

maresb · 2024-02-12T11:48:29Z

Sorry, I'm in a time crunch at the moment. It might have to wait another few days.

tomers · 2024-02-28T19:52:52Z

Hey there,

Due to irresponsivenes, I'm abandoning this project, and will fork it in a new organization, managed by me. Whoever wants to join me, is most welcome to contact me via LinkedIn.

Consider `e = KeyError("x")`. Then `str(e) == "'x'"`. Instead we use `repr(e) = "KeyError('x')"`. This is achieved with `f"{e!r}"`.

I find it way faster to understand what's going on when one doesn't need to mentally parse a bunch of custom classes.

It's much better to consider the module itself as the object.

It's much easier and more inspectable by type checkers to use a NamedTuple instead of a custom class with Enum.

Using a kwargs dictionary does save a few lines of code, but it prevents the type checker from working, and it's harder to read and inspect.

Have separate class for each kind of rendered (text, barcode, QR, etc.). Combine renders are done using another kind of render (HorizontallyCombinedRenderEngine). Move logic to associate renders and dymo device in a separate labeler_device file. Note that the new architecture is design to support multiple labelers in the future. This commit is the first step in that direction.

tomers · 2024-03-24T15:29:42Z

Done with the fixes, @maresb

Add linter for markdown files [1] [1] https://github.com/igorshubovych/markdownlint-cli?tab=readme-ov-file#use-with-pre-commit

maresb · 2024-03-24T16:44:04Z

--browser now works again. There are still a few things unresolved from the last review.

It's really difficult to review such a large PR after a rebase, so could you please refrain from rebasing until after I've approved?

Following long discussion in the PR [1], I'll go with maresb's style preference of favoring literal type hint over an enum. [1] computerlyrik#113 (comment)

maresb

Whew, that was a whole lot of work, but I think we finally got it. Thanks @tomers for persevering. These are some really huge improvements.

Go ahead and rebase if you'd like, and then let's get this merged.

tomers · 2024-03-24T20:05:49Z

Thanks Ben, The branch is ready to be merged from my side. I am not going to change it anymore. You can merge according to the plan that we discussed privately.

…

On Sun, Mar 24, 2024 at 8:13 PM Ben Mares ***@***.***> wrote: ***@***.**** approved this pull request. Whew, that was a whole lot of work, but I think we finally got it. Thanks @tomers <https://github.com/tomers> for persevering. These are some really huge improvements. Go ahead and rebase if you'd like, and then let's get this merged. — Reply to this email directly, view it on GitHub <#113 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUL43WPD4PD2HOEJXUNDLYZ4JVBAVCNFSM6AAAAABCXDDOQ6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNJWGYYTMMZVGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

maresb · 2024-03-24T20:40:50Z

Thanks @tomers!

For anyone reading, to avoid giving the impression of being secretive, the plan is to not release this as DymoPrint, but instead release it as Labelle v1.1.0 (see #114). And Labelle v1.0.0 will be a rebranded DymoPrint v2.4.0.

maresb marked this pull request as draft February 3, 2024 13:39

tomers force-pushed the tomer_fixes branch 2 times, most recently from 68f41e1 to 6924b9c Compare February 4, 2024 09:15

maresb reviewed Feb 4, 2024

View reviewed changes

src/dymoprint/lib/utils.py Show resolved Hide resolved

tomers force-pushed the tomer_fixes branch from 8fec081 to 1fee230 Compare February 4, 2024 12:02

tomers marked this pull request as ready for review February 4, 2024 18:13

tomers force-pushed the tomer_fixes branch 4 times, most recently from aa8d243 to 851c0ae Compare February 8, 2024 09:05

maresb requested changes Feb 10, 2024

View reviewed changes

src/dymoprint/lib/utils.py Outdated Show resolved Hide resolved

tomers force-pushed the tomer_fixes branch from 5de5f31 to e8624e7 Compare February 10, 2024 12:45

tomers force-pushed the tomer_fixes branch 2 times, most recently from 0dc29ca to 0af729f Compare February 10, 2024 14:46

maresb and others added 14 commits March 24, 2024 15:29

Fix exception printing

40d52b3

Consider `e = KeyError("x")`. Then `str(e) == "'x'"`. Instead we use `repr(e) = "KeyError('x')"`. This is achieved with `f"{e!r}"`.

Use functional style for config file

cb24bc4

I find it way faster to understand what's going on when one doesn't need to mentally parse a bunch of custom classes.

Remove unneeded classes for font config

89ef0bd

It's much better to consider the module itself as the object.

Refactor RenderContext

6b9d144

It's much easier and more inspectable by type checkers to use a NamedTuple instead of a custom class with Enum.

Update Ruff pre-commit version

ca7e0e4

Update pyproject.toml to new ruff conventions

a16922f

Use updated ruff docstring formatting

ccacf6c

Fix typos in explanatory "print mode" comment

055fa1c

Use typing to declare possible values of align

8851f0d

Don't mutate RenderContext

b15d8d1

Install types-pillow to fix type checking

b790238

Eliminate render_kwargs

d4df4da

Using a kwargs dictionary does save a few lines of code, but it prevents the type checker from working, and it's harder to read and inspect.

Use more specific exception

ee92b33

tomers force-pushed the tomer_fixes branch from 42e2775 to ee92b33 Compare March 24, 2024 15:29

Add Markdown lint

593e4c5

Add linter for markdown files [1] [1] https://github.com/igorshubovych/markdownlint-cli?tab=readme-ov-file#use-with-pre-commit

Tomer Shalev added 7 commits March 24, 2024 16:47

Colorize margins according to system theme

d118bbb

Update type hint for justify

a41d2b2

Update margin calculation comment

6f67623

Use typing literal instead of enums

6a859d9

Following long discussion in the PR [1], I'll go with maresb's style preference of favoring literal type hint over an enum. [1] computerlyrik#113 (comment)

Fix lint issue

ad2f603

Update pre-commit repos

907c6dd

Add github.vscode-github-actions extension

e5ed453

maresb approved these changes Mar 24, 2024

View reviewed changes

maresb merged commit 52fb23a into computerlyrik:master Mar 24, 2024
6 checks passed

tomers deleted the tomer_fixes branch March 24, 2024 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Another batch of changes #113

Another batch of changes #113

tomers commented Feb 2, 2024

maresb commented Feb 3, 2024

maresb commented Feb 3, 2024

tomers commented Feb 3, 2024

maresb commented Feb 3, 2024

tomers commented Feb 3, 2024 via email

maresb commented Feb 3, 2024

tomek-szczesny commented Feb 3, 2024

maresb commented Feb 3, 2024

tomers commented Feb 3, 2024

tomers commented Feb 8, 2024

maresb left a comment

maresb commented Feb 10, 2024

maresb commented Feb 10, 2024

tomers commented Feb 10, 2024

tomers commented Feb 12, 2024

maresb commented Feb 12, 2024

tomers commented Feb 28, 2024

tomers commented Mar 24, 2024

maresb commented Mar 24, 2024

maresb left a comment

tomers commented Mar 24, 2024 via email

maresb commented Mar 24, 2024

Another batch of changes #113

Another batch of changes #113

Conversation

tomers commented Feb 2, 2024

maresb commented Feb 3, 2024

maresb commented Feb 3, 2024

tomers commented Feb 3, 2024

maresb commented Feb 3, 2024

tomers commented Feb 3, 2024 via email

maresb commented Feb 3, 2024

tomek-szczesny commented Feb 3, 2024

maresb commented Feb 3, 2024

tomers commented Feb 3, 2024

tomers commented Feb 8, 2024

maresb left a comment

Choose a reason for hiding this comment

maresb commented Feb 10, 2024

maresb commented Feb 10, 2024

tomers commented Feb 10, 2024

tomers commented Feb 12, 2024

maresb commented Feb 12, 2024

tomers commented Feb 28, 2024

tomers commented Mar 24, 2024

maresb commented Mar 24, 2024

maresb left a comment

Choose a reason for hiding this comment

tomers commented Mar 24, 2024 via email

maresb commented Mar 24, 2024