Device dependent margins/config #54

FaBjE · 2024-06-21T18:11:34Z

Closes #51

I have added a DeviceConfig object that contains a configuration for each type of printer/labeler.
I started out with the lookup table, but puzzeled by the "random" margins I figured it out how the principal works.

For every printer we now need a "print head size in pixels" and "print head active size in mm" the rest is calculated.
I think there might only be two versions (128px/18mm and 64px/9mm).
Based on my scans we determined a label-position inaccuracy of 0.7mm. I rounded it up to 1mm to use as safety margin for now.

I´d like to hear your thoughts. Bear in mind that this is my first python code. So you may find a lot of weird things...
This is not done yet, so only a draft PR.

I've tested it on my LabelManager PC II and a LabelManager PnP both can print valid labels 12mm (and 24mm on the PCII)

I have identified a couple of issues so far:

Supported tape sizes dropdown not cleared in GUI, making a mess. With the config object it should now show only the options supported by the printer
Margins preview in GUI are not correct
Boxed text, left border 1px missing
mm_to_px functions are maybe not accurate. Look into/refactor those?

Each printer/labeler now has a config class instance setting its specific properties.

tomers · 2024-06-22T04:49:05Z

src/labelle/gui/q_settings_toolbar.py

@@ -57,7 +57,7 @@ def _init_elements(self) -> None:

    def update_labeler_context(
        self,
-        supported_tape_sizes: tuple[int, ...],
+        supported_tape_sizes: list[int],


Is this really needed? In case the list is constant, it would be better as a convention to prefer tuple over list

The amount of supported tape sizes varies from printer to printer. I got an error that I tried to change the "tuple" size. A quick google search gave this as a solution. If there is a better solution please elaborate.

Ah, tuples are "immutable" so you should always generate a new one instead of modifying an old one.

Fixed (It didn´t complain this time)

Edit:
Sorry, had to revert this:

src/labelle/gui/gui.py:66: error: Argument "supported_tape_sizes" to "update_labeler_context" of "QSettingsToolbar" has incompatible type "list[int]"; expected "tuple[int, ...]" [arg-type] Found 1 error in 1 file (checked 46 source files)

src/labelle/lib/constants.py

tomers · 2024-06-22T04:51:52Z

src/labelle/lib/devices/dymo_labeler.py

+
+    # Default printer margins (for each tape size)
+    # { tapeSizeMm : (firstVisiblePixel, lastVisiblePixel) }
+    labeler_label_vertical_margins: dict[int, Any]


Maybe something like:

labeler_label_vertical_margins: dict[int, tuple[int, int]]

I am wondering what "first" and "last" mean here regarding visible pixels. I don't know offhand which pixel ordering is being used, so it would be helpful to be more explicit.

@tomers Thanks, will try again. Had some linter issues with this...

@maresb Me neither (at the time) Now I know it is "first visible pixel from the top" I will extend the comment.

This is outdated btw

tomers · 2024-06-22T04:53:33Z

src/labelle/lib/devices/dymo_labeler.py

+    # Default printer margins (for each tape size)
+    # { tapeSizeMm : (firstVisiblePixel, lastVisiblePixel) }
+    labeler_label_vertical_margins: dict[int, Any]
+
    def __init__(
        self,
        tape_size_mm: int | None = None,
        device: UsbDevice | None = None,
    ):
        if tape_size_mm is None:
            tape_size_mm = self.DEFAULT_TAPE_SIZE_MM


Is this still relevant? Is DEFAULT_TAPE_SIZE_MM supported by all printer models? I guess we should choose the first available tape size for the model instead.

I'm not really happy about this either, but the whole software kind of relies on it (also with the GUI)
Maybe I should convert it to take the smallest or largest supported tape size?

Fixed it, it now takes the highest supported size if not set.
Maybe this would make a good "user preference" in the future.

src/labelle/gui/gui.py

tomers · 2024-06-22T04:58:00Z

src/labelle/lib/constants.py

-SUPPORTED_PRODUCTS = {
-    SUPPORTED_DEVICE_ID.LABELMANAGER_PC:
+# ---- Supported USB Devices configuration ----
+SUPPORTED_PRODUCTS.append(


I think that eventually we should move these out of the code, into a JSON file. If possible, please try to do it. In not, we can try refactor it later.

I think we need a proper data model(1) for device models(2). (The first "model" in that sentence means "implementation of a schema" while the second "model" refers to a particular product that is sold by a company.)

Sorry, I was a bit confused You do already define DeviceConfig.

I'm wondering how to achieve this elegantly. I agree that it's in principle better to define these things out-of-code like in JSON. However, that unfortunately would require deserialization. Pydantic is a popular library for this, but then we're adding more project dependencies and a lot of complexity, also in the code.

The way I see it, we have two main options:

Separate the device definition from code, perform deserialization, probably add a dependency on something like Pydantic.

Keep the device definitions as code, but switch to NamedTuple or dataclass.

Another disadvantage to 1. is that editing the config would involve editing serialized data which can be a real pain. An advantage to 2. is that code tools like mypy will automatically perform validation for the data-as-code.

I'm leaning more towards 2.

What I was trying to achieve here is a what in C would be "array of structs" for the "const" configuration.
If you have any suggestions how to do this elegantly in python, let me know...

My idea was that the (existing) labeler class took this as input to know the device's exact properties.

I agree that having this out of code would be nice. But can we please keep this contained?
I started out with "just adding my printer", and it is getting a bit out of hand already imho.

Converted DeviceConfig to a dataclass

tomers · 2024-06-22T04:58:47Z

src/labelle/lib/constants.py

-    SUPPORTED_DEVICE_ID.LABELPOINT_350:
+        [int(SUPPORTED_DEVICE_ID.LABELMANAGER_PC)],
+        # ToDo: Validate config!
+        128,


It's hard to understand from reading this what each field means. Please explicitly name each field.

To be explicit, this is as simple as

DeviceConfig( name="DYMO LabelMANAGER PC", deviceIDs=[int(SUPPORTED_DEVICE_ID.LABELMANAGER_PC)], ...

Also, the use here of SUPPORTED_DEVICE_ID seems very indirect to me. Why not just do

deviceIDs=[0x0011],

If that is possible can you give me a syntax example? (It would definitely be a lot clearer)

@FaBjE that was an explicit example, and should work without other modifications to your code if you continue the obvious pattern. (Python lets you specify the names of positional arguments in the function invocation.) Does this make sense?

I didn´t see your example at first. I had this page already open I guess.
But thanks! I will try to add.

I added the enum because before the DeviceConfig class I checked with if-else in the code which properties applied to the printer (Based on the device ID), but that was ugly imho.
I agree it is kind of redundant now. Shall I remove it?

Here's an example:

def f(x, y): return 2 * x + y f(y=1, x=2) # 5

tomers · 2024-06-22T05:00:05Z

src/labelle/lib/devices/device_config.py

+    """
+    Dictonary of print margins per tape size
+    Entry: TapeSizeMM : (topMarginInPx, bottomMarginInPx)
+    Use the calibration routine to determine these for each tape size


Please add a link here to a markdown document under /doc which describes what the calibration routine is.

Unfortunately there is no document at this moment. I think we need to add a link when it is there.

src/labelle/lib/devices/device_config.py

tomers · 2024-06-22T05:02:59Z

src/labelle/lib/devices/device_config.py

+        self.name = name
+        self.deviceIDs = deviceIDs
+        self.printHeadSizePixels = printHeadSizePixels
+        self.supportedTapeSizes = supportedTapeSizes
+        self.marginsPerTape = marginsPerTape


Maybe define the class as @dataclass. https://docs.python.org/3/library/dataclasses.html

Personally, I'd probably use a NamedTuple which is simpler, but either should work.

I'm not sure of the pro's and con's of both. But I think dataclass is easier to convert to?
I'll give it a shot

Made dataclass

tomers · 2024-06-22T05:04:22Z

src/labelle/lib/devices/device_config.py

+        if idValue in self.deviceIDs:
+            return True
+        else:
+            return False


I am not sure a function is really needed for this, as it is rather short, and can be replaced with a simple expression. In any case, it should be refactored as follows:

return idValue in self.deviceIDs

Will fix. It seemed more "contained" to me than just accessing the array itself from outside the object.

tomers · 2024-06-22T05:06:32Z

src/labelle/lib/devices/device_config.py

+        else:
+            return False
+
+    def get_tape_print_margins(self, tapeSizeMM: int) -> tuple[int, int]:


Also here, tape_size_mm, not tapeSizeMM (I will refrain now from commenting of improper casing).

Please do 😉 I'll do a re-check of my work.

Outdated/fixed

tomers · 2024-06-22T05:08:24Z

src/labelle/lib/devices/device_config.py

+        if tapeSizeMM in self.supportedTapeSizes:
+            if tapeSizeMM in self.marginsPerTape:
+                # Return specified margins
+                return self.marginsPerTape[tapeSizeMM]
+            else:
+                # No specific margins set, return default
+                return (0, 0)
+        else:
+            # Tape size not supported
+            raise ValueError(
+                f"Unsupported tape size {tapeSizeMM}mm. "
+                f"Supported sizes: {self.supportedTapeSizes}mm"
+            )


As coding convention, instead of:

if term: # some very long logic else: raise ...

do:

if term: raise ... # some very long logic

This reduces nesting.

Fixed (now in dymo_labeler.py)

tomers · 2024-06-22T05:09:56Z

src/labelle/lib/devices/device_config.py

+            if tapeSizeMM in self.marginsPerTape:
+                # Return specified margins
+                return self.marginsPerTape[tapeSizeMM]
+            else:
+                # No specific margins set, return default
+                return (0, 0)


return self.marginsPerTape.get(tapeSizeMM, (0, 0))

or even better:

DEFAULT_MARGINS = (0, 0) # put this on top of file return self.marginsPerTape.get(tapeSizeMM, DEFAULT_MARGINS)

Thanks, will fix. (these are the kind of things you have no clue about when using a new language)

tomers · 2024-06-22T05:11:45Z

src/labelle/lib/devices/device_manager.py

+    """
+    #
+    for device in SUPPORTED_PRODUCTS:
+        if device.matches_device_id(idValue) is True:


Remove is True. It will work the same

ok, will fix

tomers · 2024-06-22T05:13:28Z

src/labelle/lib/devices/dymo_labeler.py

+            if foundConfig is not None:
+                self._deviceConfig = foundConfig
+            else:
+                raise ValueError(f"Unsupported device type {self._device.id_product}")


As before, style comment. Please use the following convention:

if foundConfig is None: raise ValueError(f"Unsupported device type {self._device.id_product}") self._deviceConfig = foundConfig

tomers · 2024-06-22T05:14:59Z

src/labelle/lib/devices/dymo_labeler.py

+            if foundConfig is not None:
+                self._deviceConfig = foundConfig
+            else:
+                raise ValueError(f"Unsupported device type {self._device.id_product}")


Why do you return None from the function and raise here, instead of raising in the function itself, and then you will not have to test returned value from the function (and change its type to also support None).

Good question...
Stupid answer: I learned about the raising exceptions later on.

will fix.

tomers · 2024-06-22T05:18:27Z

src/labelle/lib/devices/device_manager.py

@@ -82,8 +81,11 @@ def find_and_select_device(self, patterns: list[str] | None = None) -> UsbDevice
            LOG.debug(dev.device_info)
        dev = devices[0]
        if dev.is_supported:
-            msg = f"Recognized device as \
-                {SUPPORTED_PRODUCTS[SUPPORTED_DEVICE_ID(dev.id_product)]}"
+            foundDeviceConfig: DeviceConfig | None = get_device_config_by_id(


Do you really need to declare the type of foundDeviceConfig (I guess you meant found_device_config :-) )?
If the function declares its return type, then I think it should be fine not to declare here.

Yeah this was an ugly workaround. Will be obsolete when I fix the other function.
And yes of course I meant snake_case 😉

maresb · 2024-06-23T11:24:24Z

About that, do I click the "Resolve conversation" button, or do you do that when my changes are verified?

Ideally the person making the request (asking a question, requesting a change) resolves. If it's clear that you've decisively addressed the request and no followup is needed, then you can close yourself. It's also fine to leave them open. There are no rules, only common sense and etiquette.

Move LABELER_DISTANCE_BETWEEN_PRINT_HEAD_AND_CUTTER_MM to device config

FaBjE · 2024-06-23T14:31:43Z

About that, do I click the "Resolve conversation" button, or do you do that when my changes are verified?

Ideally the person making the request (asking a question, requesting a change) resolves. If it's clear that you've decisively addressed the request and no followup is needed, then you can close yourself. It's also fine to leave them open. There are no rules, only common sense and etiquette.

Good to know. Makes sense to me, I've left most open as I think my fixed should be check too if they are the desired way.
I think I addressed all comments now.

As the DPI or PPM (pixels per mm) is now a property of the printer. Move all calculations requiring this value to the labeler object. This will improve future support of different DPI printers

FaBjE · 2024-06-23T15:27:53Z

I did a big change for the DPI values. 7246473
I'm not sure if I should make a separate PR for that, or include it here.

maresb

Thanks so much @FaBjE for your work here.

When I run labelle-gui with my LabelManager PnP, I get the following stack trace:

qt.gui.icc: fromIccProfile: failed size sanity 2
Traceback (most recent call last):
  File "/home/mares/repos/labelle/src/labelle/gui/gui.py", line 108, in _on_settings_changed
    height_px=self._dymo_labeler.label_height_px,
  File "/home/mares/repos/labelle/src/labelle/lib/devices/dymo_labeler.py", line 277, in label_height_px
    return self.tape_print_properties.usable_tape_height_px
  File "/home/mares/repos/labelle/src/labelle/lib/devices/dymo_labeler.py", line 305, in tape_print_properties
    raise ValueError(
ValueError: Unsupported tape size 24mm. Supported sizes: [6, 9, 12]mm
Aborted (core dumped)

I believe this is indicative of a structural problem that we are introducing here. (Although the rest of the codebase is not innocent.) I strongly disapprove of the use of properties here. Accessing class attributes should be a completely safe operation. (The type checker should guarantee that they're defined.) In the stack trace we see two nested attribute accesses, which is an insane obfuscation. Unless there are exceptional circumstances, attributes should be static and functions should compute stuff. Note also how tape_print_properties is a huge block of (thankfully well-documented) code.

Instead of having a property called label_height_px I'd like to instead see an ordinary function get_label_height_as_px. The benefits to this approach are that:

we define an honest interface where attributes are attributes and functions are functions
it makes us actually think about what should be an attribute vs a function instead of lazily reencoding everything imaginable as a property, creating needless layers of indirection

But then this also brings up the issue of whether or not we should even have mm in the first case.

A tape has:

height in mm
height in px
dpi / pixels_per_mm
and any two determines the third. As far as I can tell, we don't need DPI for anything, and it doesn't seem like we even need height in mm if we treat "tape size" as a label. While we can do some clever stuff with dpi for DYMO printers, I wonder if it's actually worth all this code complexity.

maresb · 2024-06-29T16:47:36Z

src/labelle/lib/devices/dymo_labeler.py

+    TapePrintProperties = namedtuple(
+        "TapePrintProperties",
+        ["usable_tape_height_px", "top_margin_px", "bottom_margin_px"],
+    )
+    """Tape print properties tuple type.
+    Contains margins and size in pixels for printing.
+    """


Please remove this definition and recreate it outside the class Dymolabler: block as follows:

class TapePrintProperties(NamedTuple): usable_tape_height_px: int top_margin_px: int bottom_margin_px: int

I'd recommend omitting the docstring since it doesn't carry any information that's not already evident from the current names / types.

maresb · 2024-06-29T18:52:31Z

src/labelle/lib/render_engines/print_preview.py



 class PrintPreviewRenderEngine(RenderEngine):
    X_MARGIN_PX = 80
    Y_MARGIN_PX = 30
    DX = X_MARGIN_PX * 0.3
    DY = Y_MARGIN_PX * 0.3
+    dymo_labeler: DymoLabeler


It seems like we are attaching a DymoLabeler to lots of objects and passing it around in a lot of arguments. But as far as I can see, we only ever need to access the tape height in either px or mm.

Couldn't we just define a NamedTuple called TapeHeight with two attributes px and mm, and pass this around instead?

FaBjE · 2024-06-30T13:22:45Z

Thanks so much @FaBjE for your work here.

Ok, I'm going to be brutally honest here. Not that I want to, but I think it needs to happen.Up until this point I liked to contribute something to labelle and getting my label printer to work under linux.
But this is starting to change.

The current codebase is/was not made for printers with different properties. (Supported tape sizes, dpi, etc) I tried to integrate this, but I needed to touch a lot of stuff.

Trying to to change as little as possible (within reason) I did this. But there is a lot of stuff I think that can be handled different/better with the things we know now. Some may be personal preference or coding style preference.
In my opinion, either we accept it is not as pretty as we want it, but it works. Or we do a big overhaul of the code and make everything pretty.

But I think we are stuck in a no-mans-land between the two at this point.

As I said in the opening post of this PR this code is not perfect. There are issues with it, mostly caused by my changes "conflicting" the current setup of the codebase.

Let me address the points:

When I run labelle-gui with my LabelManager PnP, I get the following stack trace:
qt.gui.icc: fromIccProfile: failed size sanity 2
Traceback (most recent call last):
  File "/home/mares/repos/labelle/src/labelle/gui/gui.py", line 108, in _on_settings_changed
    height_px=self._dymo_labeler.label_height_px,
  File "/home/mares/repos/labelle/src/labelle/lib/devices/dymo_labeler.py", line 277, in label_height_px
    return self.tape_print_properties.usable_tape_height_px
  File "/home/mares/repos/labelle/src/labelle/lib/devices/dymo_labeler.py", line 305, in tape_print_properties
    raise ValueError(
ValueError: Unsupported tape size 24mm. Supported sizes: [6, 9, 12]mm
Aborted (core dumped)
I believe this is indicative of a structural problem that we are introducing here. (Although the rest of the codebase is not innocent.) I strongly disapprove of the use of properties here. Accessing class attributes should be a completely safe operation. (The type checker should guarantee that they're defined.) In the stack trace we see two nested attribute accesses, which is an insane obfuscation. Unless there are exceptional circumstances, attributes should be static and functions should compute stuff. Note also how tape_print_properties is a huge block of (thankfully well-documented) code.

Yes, this is caused by a structural problem the GUI has. For starters: I didn´t notice this as my printer supports all tapes.
But the GUI can´t (properly) handle changing printers (properties).

More low level: when the printer properties are loaded (like supported tape sizes) the change-event of the control fires every time a item like a tape size is added/removed.

It starts with the default (simulator) config. (which has all supported tape sizes) Than the USB printer is initialized and "attached" to the GUI. Than the weird stuff/problems starts to happen. The "backend" already has the USB printer. But the GUI controls still have the old data. Change events fire, and the GUI tries to set properties (left over from the default) that the printer doesn´t support. So boom, exception.

IMHO: The change events should not fire when the printer properties are being loaded.1. Disable change events, 2. Clear GUI controls, 3. Load new printer properties to controls, 4. Enable change events.

Instead of having a property called label_height_px I'd like to instead see an ordinary function get_label_height_as_px. The benefits to this approach are that:
1. we define an honest interface where attributes are attributes and functions are functions

2. it makes us actually think about what should be an attribute vs a function instead of lazily reencoding everything imaginable as a property, creating needless layers of indirection

For starters, this label_height_px was always in the codebase. Originally it was named height_px as a property of the labeler object. Which is weird imho, because it is not the height of the labeler. (as I read it) but the tape height. So i did a rename.

Second, there already were a lot of properties that do either the same or something similar to eachother.

Third, I was specifically asked to make this a property. #54 (review) If you are 100% sure you want it a function I will convert it back to a function, but that will be the last time I'm changing it.

But then this also brings up the issue of whether or not we should even have mm in the first case.

A tape has:
* height in mm

Correct

* height in px

No. The PRINTER has a height in px for a tape. There are no pixels on a tape.

* dpi / pixels_per_mm

No. The PRINTER has a DPI.

  and any two determines the third. As far as I can tell, we don't need DPI for anything, and it doesn't seem like we even need height in mm if we treat "tape size" as a label. While we can do some clever stuff with dpi for DYMO printers, I wonder if it's actually worth all this code complexity.

You need DPI for the GUI (margins display) and CLI, calculations of tape size in pixels (for a particular printer)
FYI: In my last commit the DPI from the CLI was set different than the DPI of the constants.

If I were to setup this code today, I would make the two of classes of
1: Printer Config object, containing static data of a printer
2: Printer runtime object, containing all functions of a printer and the "runtime" calculations. Of say for example the amount of pixels the printer can print on a (selected) tape size.

After that, pass the printer object to all classes that needs to to something. Like the renderers, GUI etc. They can retrieve the data they need from the printer object.

They may need some more input, like for example a text or barcode that needs to be rendered. But all printer/tape specific data can just be retrieved from the printer object instead of passing around lots of separate variables.

Well, these are just my two cents.
I´d like you to define a strategy, prospect, view or design or whatever you want to call it how you want to continue.
And I'll see if I want to be a part of that.

tomers · 2024-06-30T13:44:13Z

Third, I was specifically asked to make this a property. #54 (review) If you are 100% sure you want it a function I will convert it back to a function, but that will be the last time I'm changing it.

I suggested it, but I am not orthodox about it, and I do accept the reasoning raised earlier, as to why this was a bad suggestion. I am sorry for causing you to waste your eneregy, and lose some motivation to work on this codebase.
I hope you'll continue your excellent work, and keep improving it. The architecture indeed needs some thoughts and design in order to make it better. I love the idea of separating contant printer data (print head specification, etc.) and runtime data (current tape loaded).

maresb · 2024-06-30T15:02:08Z

Hi @FaBjE, I'm really sorry that my criticism caused frustration and discouragement. I completely agree with everything you wrote. My criticism of the overuse of properties is more about the existing codebase and the need to change course rather than your contribution in particular. It was not so tactful for me to single out your code as an example of why property overuse leads to confusion. Especially since the previous review requested use of properties.

As you have realized, there are still a lot of structural issues in the codebase. A lot of things snuck in because I didn't review code as carefully as I should have. So it's a challenging situation. I want to be encouraging of contributions from people like you. But I also want the code to remain maintainable, and avoid further entrenching our previous bad design decisions.

You ask me to lay out a vision, but I think you already lay out a very clear vision in your last comment. The one thing I'd adjust is to avoid the idea of a monolithic printer object. I'd rather design things so that we have isolated stages, each of which only requires limited information. (Like the Linux philosophy of piping together simple commands.)

To figure out what these isolated stages should look like, let's suppose we were designing a web client as a replacement for the GUI. I think the first thing we'd need is a high-level specification of the label elements (e.g. text, barcode, etc.) that are to be horizontally combined. These shouldn't depend on the tape size or device, and it should ideally be easily serializable. Next would come the rasterization step that, for a given tape size in pixels, converts the elements into a bitmap to print. Finally, with the bitmap in hand, it should be passed to the printer for printing. So there are these steps: element specification, rasterization, printing to device, which I'd like to see as isolated as possible.

As for concrete next steps for this PR, let's try and make sure things are running without any errors. Let's try within reason to reduce the amount of stuff that's being added to interfaces and being passed around. And for these structural problems, let's keep track of what's wrong, and build in a way that will facilitate eventually fixing those structural problems.

And I'll try and to better to keep things fun. I think we're all here to build cool stuff in a relaxing environment. Sorry if I was being too up-tight. I really hope that you stick around.

tomek-szczesny · 2024-06-30T20:40:49Z

Since the project has a new name and a new scope (all label printers, ideally), it makes sense to define a sustainable architecture and overhaul it, thus bump the major release number. This might cost us less work in the long run, as with a few Dymos we already struggle to keep it tidy.

@maresb's general idea has some merit. I think that each printer model should be a different "object", with its own renderer code, attributes and so on.

One thing that worries me about this proposed approach is the second stage that composes the pieces together. I am thinking about barcodes and QR codes in particular, that will not work if scaled improperly. Thus this stage has to know how many dots it's working with, something that definitely is the domain of the third stage.

I guess we'll need a graph, and a space for this discussion :)

maresb · 2024-07-01T08:33:30Z

One thing that worries me about this proposed approach is the second stage that composes the pieces together. I am thinking about barcodes and QR codes in particular, that will not work if scaled improperly. Thus this stage has to know how many dots it's working with, something that definitely is the domain of the third stage.

Right, the pipeline can't be strictly one-way. In pseudocode I'd propose something like:

def print_label(label_specification, device, tape):
    bitmap_height = device.usable_tape_height_px(tape)
    bitmap = rasterize(label_specification, bitmap_height)
    device.print(bitmap, tape)

Of course it probably will end up more complicated than this in practice due to implementation details.

So for this PR I think my specific question would be if, instead of passing around the DymoLabeler object, would it be sufficient and feasible to compute the bitmap height upfront and pass around only that instead? My hope is that this would be able to considerably simplify the code in this PR, and make it easier in the future to refactor. (It may well be that I'm wrong and that would require rewriting the whole codebase, but I hope not! 🙈)

FaBjE · 2024-07-01T14:35:18Z

I suggested it, but I am not orthodox about it, and I do accept the reasoning raised earlier, as to why this was a bad suggestion. I am sorry for causing you to waste your eneregy, and lose some motivation to work on this codebase. I hope you'll continue your excellent work, and keep improving it. The architecture indeed needs some thoughts and design in order to make it better. I love the idea of separating contant printer data (print head specification, etc.) and runtime data (current tape loaded).

I would not call it a bad suggestion. Because every other thing is put into a property in the dymo labeler class.
So in that way it made sense to me, and I changed it.

Hi @FaBjE, I'm really sorry that my criticism caused frustration and discouragement. I completely agree with everything you wrote. My criticism of the overuse of properties is more about the existing codebase and the need to change course rather than your contribution in particular. It was not so tactful for me to single out your code as an example of why property overuse leads to confusion. Especially since the previous review requested use of properties.

Well, you didn´t exactly put my code on the spot as the propery already existed. (I only renamed it).
The confusing/frustrating part for me is that this PR is instead of discussing my changes is becoming more and more discussion about the global architecture of the project.
Something I had nothing to do with, and should not overhaul in this PR.
I did the best I could with the current codebase. But as stated before, most things I encountered would not be my first choice (either). But made sense if you would set this up for 1 printer type.

As you have realized, there are still a lot of structural issues in the codebase. A lot of things snuck in because I didn't review code as carefully as I should have. So it's a challenging situation. I want to be encouraging of contributions from people like you. But I also want the code to remain maintainable, and avoid further entrenching our previous bad design decisions.

Yes, but what you are currently doing now from my perspective is criticizing stuff out of scope of this PR.

You ask me to lay out a vision, but I think you already lay out a very clear vision in your last comment. The one thing I'd adjust is to avoid the idea of a monolithic printer object. I'd rather design things so that we have isolated stages, each of which only requires limited information. (Like the Linux philosophy of piping together simple commands.)

Well, yes, but that is my vision. I have no clue in where you are standing on this or what the goal of the project is.

My advice on your view would be that it might be the most ideal design. (keeping stages separate) But be realistic and keep it simple. This is at the moment a tiny project with about no-one with time to code on it. You will introduce a abstraction and challenges with it. Which all require effort to code.

To figure out what these isolated stages should look like, let's suppose we were designing a web client as a replacement for the GUI. I think the first thing we'd need is a high-level specification of the label elements (e.g. text, barcode, etc.) that are to be horizontally combined. These shouldn't depend on the tape size or device, and it should ideally be easily serializable. Next would come the rasterization step that, for a given tape size in pixels, converts the elements into a bitmap to print. Finally, with the bitmap in hand, it should be passed to the printer for printing. So there are these steps: element specification, rasterization, printing to device, which I'd like to see as isolated as possible.

Brutally honest, I (would like to) see labelle as the simple "simple but functional"/no-nonsense portable application that came on the Dymo PnP labeler (for windows) and I think you are 80% there atm.
Thats what I am looking for at least. Just printing some labels from a GUI. nothing fancy.

An example that I read in the GUI issue is about the office setting. But not to ruin your dreams or anything.
But the likely hood of something like labelle to be used in the foreseeable future are slim imho. For (professional) office settings there are much better (and expensive) solutions available.

I think labelle's strength will be in supporting the "older" hardware. and keeping it alive. reducing e-waste.
Even on windows the official dymo software is pretty much out of support for these types of printers.
But nothing is wrong with the label printers, I can buy tape cheaply at discount stores. and it can print labels fine.

Again, it may be fine ambitions, but something (too) far in the future imho.
Especially considering the amount of active coders on the project.

On the pratical side as @tomek-szczesny pointed out in it's next post, the bitmap that needs to be rendered is direct dependent on the printer (dpi) and label (size).

As for concrete next steps for this PR, let's try and make sure things are running without any errors. Let's try within reason to reduce the amount of stuff that's being added to interfaces and being passed around. And for these structural problems, let's keep track of what's wrong, and build in a way that will facilitate eventually fixing those structural problems.

And I'll try and to better to keep things fun. I think we're all here to build cool stuff in a relaxing environment. Sorry if I was being too up-tight. I really hope that you stick around.

I have a different suggestion for this PR. I would like to see a "develop" branch.
For this branch we use this PR as starting point.

A separate overhaul of the design may be implemented on top of that.

Maybe just in tiny steps to keep it manageable. (again, amount of available coders/time)
Whenever we are happy with it, the develop branch can be merged to the main branch.

So for this PR I think my specific question would be if, instead of passing around the DymoLabeler object, would it be sufficient and feasible to compute the bitmap height upfront and pass around only that instead? My hope is that this would be able to considerably simplify the code in this PR, and make it easier in the future to refactor. (It may well be that I'm wrong and that would require rewriting the whole codebase, but I hope not! 🙈)

The dymo labeler object is current passed around for the DPI. Label height is already passed around.
there are lots more things to consider if you want to support more devices. (for example, label width, all newer types have fixed size labels instead of our "endless" tape).
But again, practically. This works.

So again my view. Merge this to develop, agree on a global design, rework everything (in tiny steps), have a proper codebase.

maresb · 2024-07-01T15:56:22Z

So again my view. Merge this to develop, agree on a global design, rework everything (in tiny steps), have a proper codebase.

I'd be happy to do this. That way we can follow up separately with the structural issues I've raised before merging to main, people can build on that if they'd like, and we avoid further blocking on this PR. Great suggestion!

I just work on this project to screw around. I have no serious ambitions. For quite a while I was the only active maintainer, and I was just trying to keep the project alive. I do have things I'd like to eventually see implemented, like a web client and server, but unfortunately I don't have the free time to implement that stuff myself.

Also, to be honest I don't understand the details of how all these classes are implemented, and whenever I look at the details I'm surprised and confused. It used to be way worse until @tomers came along and rewrote major portions of the codebase. He took us from 20% to 80% of having a stable functional app.

FaBjE added 10 commits June 20, 2024 18:36

Add "DYMO LabelMANAGER PC II" device ID

d73626d

Add 24mm tape type

06f61ea

device specific init WIP

3aecdc7

Add DeviceConfig class for each printer

1872e40

Each printer/labeler now has a config class instance setting its specific properties.

Please mypy...

1b279b3

Fix usb device recognition

2353512

Calculate margins instead of using lookup table

dcc6234

Force sending an "exact printhead width" bitmap to the printer

040f2c2

Fix config not updated when device set

f907b8b

Tested on LabelManager PnP

d64c977

FaBjE changed the title ~~Feature/device dependent margins~~ Device dependent margins/config Jun 21, 2024

FaBjE mentioned this pull request Jun 21, 2024

Adding "DYMO LabelMANAGER PC II" support (help requested) #51

Open

tomers reviewed Jun 22, 2024

View reviewed changes

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

tomers reviewed Jun 22, 2024

View reviewed changes

src/labelle/gui/gui.py Outdated Show resolved Hide resolved

tomers reviewed Jun 22, 2024

View reviewed changes

src/labelle/lib/devices/device_config.py Outdated Show resolved Hide resolved

tomers reviewed Jun 22, 2024

View reviewed changes

FaBjE added 10 commits June 23, 2024 13:41

Revert GUI supported_tape_sizes type

13fa9b9

Remove fixed default tapesize (take highest supported as default)

424ec34

Move LABELER_DISTANCE_BETWEEN_PRINT_HEAD_AND_CUTTER_MM to device config

Shorten matches_device_id function

d342a04

Fix code convention in if statement

1786928

Shorten if statement

16d82e8

Shorten if statement

e239867

Let get_device_config_by_id raise exception if not supported

b390217

Remove var / shorten line

0bdf8fd

fix comment

a4ed930

Clear up comment block

fdf3538

FaBjE added 2 commits June 23, 2024 15:18

Fix wrong var name

0fa7ad5

Remove constant DPI values and calculations

7246473

As the DPI or PPM (pixels per mm) is now a property of the printer. Move all calculations requiring this value to the labeler object. This will improve future support of different DPI printers

maresb requested changes Jun 29, 2024

View reviewed changes

maresb changed the base branch from main to develop July 1, 2024 15:57

maresb approved these changes Jul 1, 2024

View reviewed changes

FaBjE marked this pull request as ready for review July 1, 2024 16:44

maresb merged commit 11f1cdb into labelle-org:develop Jul 4, 2024
6 checks passed

FaBjE mentioned this pull request Jul 6, 2024

V2 Design discussion #58

Open

This was referenced Aug 2, 2024

Support LabelManager 420P #62

Open

Merge develop into main #65

Open

Device dependent margins/config #54

Device dependent margins/config #54

Conversation

FaBjE commented Jun 21, 2024 • edited by maresb Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FaBjE Jun 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FaBjE Jun 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maresb Jun 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomers Jun 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maresb commented Jun 23, 2024

FaBjE commented Jun 23, 2024

FaBjE commented Jun 23, 2024

maresb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FaBjE commented Jun 30, 2024

tomers commented Jun 30, 2024

maresb commented Jun 30, 2024 • edited Loading

tomek-szczesny commented Jun 30, 2024

maresb commented Jul 1, 2024

FaBjE commented Jul 1, 2024

maresb commented Jul 1, 2024

FaBjE commented Jun 21, 2024 •

edited by maresb

Loading

FaBjE Jun 23, 2024 •

edited

Loading

FaBjE Jun 23, 2024 •

edited

Loading

maresb Jun 22, 2024 •

edited

Loading

tomers Jun 22, 2024 •

edited

Loading

maresb commented Jun 30, 2024 •

edited

Loading