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

Add option to print multiple labels to a single page #4805

Closed

Conversation

martonmiklos
Copy link
Contributor

@martonmiklos martonmiklos commented May 13, 2023

Closes #4626

InvenTree/label/api.py Outdated Show resolved Hide resolved
InvenTree/label/api.py Outdated Show resolved Hide resolved
InvenTree/label/api.py Outdated Show resolved Hide resolved
InvenTree/label/models.py Outdated Show resolved Hide resolved
Copy link
Member

@matmair matmair left a comment

Choose a reason for hiding this comment

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

The code looks clean; there are missing migrations though; you can generate them by running 'inv migrate' on your instance.

@martonmiklos
Copy link
Contributor Author

The code looks clean; there are missing migrations though; you can generate them by running 'inv migrate' on your instance.

Yeah, I still need to separate the print method as Oliver requested.

@wolflu05 wolflu05 mentioned this pull request May 19, 2023
@martonmiklos martonmiklos changed the title [WIP]Add multipage label printing Add multipage label printing May 23, 2023
@wolflu05
Copy link
Contributor

Please don't force push, this will make review much harder, as references break. All prs will be squash merged anyway, so history doesn't matter.

@martonmiklos martonmiklos changed the title Add multipage label printing Add option to print multiple labels to a single page May 24, 2023
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@matmair
Copy link
Member

matmair commented May 24, 2023

@martonmiklos why is absolute positioning not possible? Couldn't we wrap the labels into 0-width containers that provide absolute position within?

@wolflu05
Copy link
Contributor

wolflu05 commented May 24, 2023

I think the key cas setting is position: relative for the wrappers, then the inner labels which are positioned absolut use the origin of the relative container.

@martonmiklos
Copy link
Contributor Author

I think the key cas setting is position: relative for the wrappers, then the inner labels which are positioned absolut use the origin of the relative container.

Well that is true, I fixed it by adding a wrapper div:
kép

@wolflu05
Copy link
Contributor

Do we really need this extra div? Doesn't this also work with the class applied to the td's?

@martonmiklos
Copy link
Contributor Author

Do we really need this extra div? Doesn't this also work with the class applied to the td's?

That's true, it looks to be working fine, code updated.

Copy link
Member

@matmair matmair left a comment

Choose a reason for hiding this comment

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

LGTM, great work!

InvenTree/label/admin.py Show resolved Hide resolved
Copy link
Contributor

@wolflu05 wolflu05 left a comment

Choose a reason for hiding this comment

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

Hello @martonmiklos ,
thank you for your work here. This is a very good addition and I also like that this is implemented directly in inventree core. This opens much doors also with machine integration (#4824). But I found a few bugs/had a few ideas while testing. Sorry that this list is so long. I really appreciate your work.

  1. I think ci test failure is related
  2. The .qr css class in all preshipped templates contains a position: fixed setting which does not work as fixed ignores the relative positioning.
    • InvenTree/label/templates/label/part/part_label_code128.html
    • InvenTree/label/templates/label/part/part_label.html
    • InvenTree/label/templates/label/stockitem/qr.html
    • InvenTree/label/templates/label/stocklocation/qr_and_text.html
    • InvenTree/label/templates/label/stocklocation/qr.html
    • InvenTree/report/templates/report/inventree_report_base.html
  3. I actually discovered, that label_base.html already adds a content wrapper with relative positioning to the label which causes some errors. You need to at least at the following to the .content class (I think you also have this problem in your screenshot shown here):
             top: 0;
             left: 0;
             height: 100%;
    otherwise labels look like this:
    image
  4. I'm not sure if each label should have its own same style rendered, this is very weird for debugging in browser devtools. What do you think @matmair ? See:
    image
  5. It would be good to have some way of controlling the border spacing. It seems like this is actually not possible with borders, but doable with linear gradient backgrounds. See this post
  6. What about an edgecuts marker feature. This would only add small lines on the paper edges where to cut, instead of the whole paper. That way label would look cleaner on the edges as there is no border. See:
    image
  7. It would be cool to have some way of defining some kind of min spacing, so that labels aren't printed to close to the edges of the paper
  8. I discovered an issue if label printing is not set to debug mode, so weasyprint is used to generate a PDF. The @page selector in the label_base.html template is interfering with weasyprint and the complete page is only that big. But even if I comment that part out temporarily, it still does not work properly with absolute positioning, as the div.content from the label_base.html is somehow misplaced even the changes added from 3). See (I decreased the size of the QR codes in this picture):
    image

InvenTree/label/migrations/0010_auto_20230502_1646.py Outdated Show resolved Hide resolved
InvenTree/label/models.py Outdated Show resolved Hide resolved
Comment on lines 350 to 353
if self.get_object().multipage and plugin is None:
return self.print_multiple_per_page(request, items_to_print, debug_mode)
else:
return self.print_single_label_per_page(request, items_to_print, debug_mode, plugin)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this plugin is None check useful for? A plugin printer can actually also be a paper (A4) printer.

I suggest refactoring that so that machine integration (#4824) could be easily added. My suggestion would be to let the self.print_multiple_per_page and self.print_single_label_per_page function return either html or a pdf and then do the offload task / download file thing based on the query param in this function. That way those two other functions are not aware of any plugin or download thing and this would enable easy machine integration as I would just need to add an condition if the machine query param is set and then fetch machines instead of printing plugins and offload their printing task.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well my idea/motivation between with this was the following: the plugins could have better understanding how the pagination can be done on the printer they are operating. For e.g. we have a Brady label printer which is capable to print to per-perforated cable marking vinyls. But maybe I am wrong and passing paginated html/PDF is a better approach in most cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

@SchrodingersGat @matmair what is your opinion on this thread? And what is the current state of this PR, I would need it for #4824 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And what is the current state of this PR

My right hand is now in rail (not in cast) so I have limited typing ability. I rebased this PR onto master, added support for the Buildline labels (introduced in the meantime). I also improved the docs about avoiding the position: fixed in multipage labels. I think if we get agreement on the way we should handle the plugins then we could move forward with this.

Copy link
Member

Choose a reason for hiding this comment

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

@martonmiklos good to hear you are on your way to recovery! There has been some discussion about cleaning up the current implementation to make this (and other plugin / machine features) more streamlined. I am currently working on a label printing architecture update which will impact this PR. Please hold off on any work on this until I've got this up and running (should be under a week). Keep resting :)

InvenTree/label/templates/label/multipage_label_base.html Outdated Show resolved Hide resolved
InvenTree/label/models.py Outdated Show resolved Hide resolved
@wolflu05 wolflu05 added enhancement This is an suggested enhancement or new feature report Report/Label generation labels May 25, 2023
@matmair
Copy link
Member

matmair commented May 25, 2023

good work @martonmiklos and good review @wolflu05; my 2 cents:
4) moving it to a class makes more sense IMO. But there might be a reason this way was chosen
5) feels like a hack. This would need to be explained in docs or comment as it is really not self-explaining
6/7) good idea but possibly a future PR?
8) there is a border margin option in weasyprint (like requested in 7) - might that be it?

@SchrodingersGat
Copy link
Member

@martonmiklos does the current version work for you? I just pulled latest commit and it only prints a single lable instance, even if multiple are selected. It also does not print to full-page (i.e. A4)

@SchrodingersGat
Copy link
Member

Additionally I think we may need some more parameters to be able to support sheets of label stickers e.g. https://www.averyproducts.com.au/labels/shape/rectangle

Note the many and varied border sizes available:
image

@martonmiklos
Copy link
Contributor Author

@martonmiklos does the current version work for you? I just pulled latest commit and it only prints a single lable instance, even if multiple are selected. It also does not print to full-page (i.e. A4)

Hmm. With stock location labels it works at me now. (I have to admit have not tried part/stock labels.)
Could you share the template did you used, what label/page size did you specified?

@SchrodingersGat
Copy link
Member

As per suggestion by @wolflu05 I removed the @page spec from base label template and now it seems to be working:

image

@SchrodingersGat
Copy link
Member

With regard to the "avery" labels above (and there are many others):

Should we expose even more configurable parameters to the label templates, to allow for such complexity? Or, should we consider how we could expose multi-label-printing to the plugin interface? That way, someone could write an "avery label plugin" for InvenTree which handles all their templates, and a different plugin for a different label manufacturer.

This way, we only need to consider the w/h dimensions of the individual labels themselves, and not the interactions between labels and pages.

We could implement what you have done above as a "built-in" plugin which supports multiple labels. Then, in the "print labels" dialog, you select "multi page printing" as the printer plugin target. Instead of throwing the labels at an external printer it returns a compiled .pdf object.

Thoughts?

@martonmiklos
Copy link
Contributor Author

@wolflu05 @matmair @SchrodingersGat @roman-dvorak
I collected the not-yet-addressed ideas/features under the original issue:
#4626

May I ask you to take a look on that list and double check if I missed something?

I think the different behavior of the fixed positioning might need to be noted in the docs, but other than that I think the PR could be moved forward.

@matmair
Copy link
Member

matmair commented May 31, 2023

I appreciate the effort but in the future, I think it would be better to create a new issue instead of changing the scope of the original one that is already being developed. This way it becomes unclear to new readers what the original intent was.

Content-wise I think everything is there.
If other ideas pop up they should be filed as a separate issue. The more we pack into this FR the longer it takes to get it merged - which means merging upstream and fixing migrations and API docs another few rounds.

@wolflu05
Copy link
Contributor

What is actually the current plan with moving this into a plugin? Was this just an idea or some planned refactor in the future?

@martonmiklos
Copy link
Contributor Author

I appreciate the effort but in the future, I think it would be better to create a new issue instead of changing the scope of the original one that is already being developed. This way it becomes unclear to new readers what the original intent was.

Content-wise I think everything is there. If other ideas pop up they should be filed as a separate issue. The more we pack into this FR the longer it takes to get it merged - which means merging upstream and fixing migrations and API docs another few rounds.

I do not want to change the scope of this PR I just wanted to capture all the ideas which came up as comments/review comments in one place first etc. We can separate them easily to separate issues and remove that section.

@martonmiklos
Copy link
Contributor Author

What is actually the current plan with moving this into a plugin? Was this just an idea or some planned refactor in the future?

Well it was one of the first questions/concerns:
#4626 (comment)
and I decided it to go directly into InvenTree.

I generally like the plugins approach, however I think it could be a quite common use case to print labels with standard printers on standard paper sizes, so supporting it out of the box would add much to the traits of InvenTree.

@wolflu05
Copy link
Contributor

wolflu05 commented May 31, 2023

Actually I was more referring to: #4805 (comment)

@SchrodingersGat
Copy link
Member

@martonmiklos please check out the refactor in #5251

The idea with the new plugin system is to radically simplify the API, and make everything handled by the plugin. The new system defines a "builtin" plugin for label printing, which provides the out-of-the-box PDF generation.

If you want to print multiple plugins to a single page, it will be now very easy. Implement as a plugin, and override the print_labels method. The plugin mixing has a number of helper methods which would make this really simple now..

If / when the refactor gets merged in, let's circle back and have a chat about how this will make your life much easier here. We could implement your process as an official plugin quite easily.

@SchrodingersGat
Copy link
Member

@martonmiklos #5251 is merged in now, so your goals here should be significantly easier to implement. Sorry to throw a spanner in the works, but LMK if you want any pointers regarding working within the new framework.

@martonmiklos
Copy link
Contributor Author

@martonmiklos #5251 is merged in now, so your goals here should be significantly easier to implement. Sorry to throw a spanner in the works, but LMK if you want any pointers regarding working within the new framework.

Thanks for the heads up, I will start rebasing and reworking the PR.

@martonmiklos
Copy link
Contributor Author

LMK if you want any pointers regarding working within the new framework.

I started to work on it, butt got stuck a bit: the InvenTreeLabel plugin looks to be loaded fine:
kép

However when I try to print a label the plugin parameter got undefined:

URL: /api/label/location/1/print/?location=87&plugin=undefined

How can I make the new builtin plugin default?

@SchrodingersGat
Copy link
Member

You should get the option when selecting the labels to print:

image

And you should also be able to set the default printer in the user settings page:

image

@martonmiklos
Copy link
Contributor Author

Dah, I had the plugins_enabled set to False in the setup.cfg.

Maybe it would be useful to change the builtin plugins active mode this case?
kép

@SchrodingersGat
Copy link
Member

Maybe it would be useful to change the builtin plugins active mode this case?

Builtin plugins are always enabled:

image

@martonmiklos
Copy link
Contributor Author

Builtin plugins are always enabled:

Well then it is not an intended behavior, I have submitted a PR to fix it: #5304

I have also updated this PR to fit the new plugin based approach.

@wolflu05
Copy link
Contributor

Any updates on this? What was the last state?

@SchrodingersGat
Copy link
Member

@wolflu05 @martonmiklos sorry I have been very swamped with work / life - hoping to get back to this soon.

@wolflu05 wolflu05 mentioned this pull request Oct 25, 2023
@SchrodingersGat
Copy link
Member

@martonmiklos apologies for leaving this one hanging. There has been a lot of activity which has distracted me from this, but I do want to get this implemented for our next major release (0.13.0).

After discussion offline with @matmair and @wolflu05 I would like to change the direction of this a bit. With some of the recent refactoring of the label printing functionality, we have now an opportunity to leverage the great basework you have done here, but basically implement it as a plugin. I'll explain in some further detail:

The "nuts and bolts" of what you hav implemented here is fantastic. However rather than placing all of the extra code right in the core "label printing" ecosystem I would like to see if become a plugin. The workflow would be as follows:

  1. The user selects a number of items to print labels for
  2. The user selects a label template, which defines how each individual label is printed
  3. The user selects a label "printer" - in this case, the "multi label page plugin"
  4. The new plugin takes all the labels, renders them against the template, and then combines them onto a single page

The recent feature by @wolflu05 to allow custom "printing options" to be specified at run-time by plugins helps us a lot here too - the user can select page size, number of labels per sheet, which ones to skip, etc.

The advantage here is that the base label printing mechanism does not need to know anything about how to combine labels into a single page. It remains simply a mechanism for rendering a simple template for printing a single label. The logic for "combining" is purely handled by the plugin.

We can make this a "builtin" plugin which provides some generic array functionality as you have here - most of your code would be able to be migrated to this approach I think. Further down the line, similar plugins could even be written which provide the correct array geometry for rendering against third-party sticker sheets, etc.

Again, sorry for the huge delay and then change of direction!!

@SchrodingersGat SchrodingersGat added this to the 0.13.0 milestone Nov 8, 2023
@SchrodingersGat
Copy link
Member

Replaced by #5883

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an suggested enhancement or new feature report Report/Label generation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FR]Print multiple labels on a sheet
5 participants