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 support for printing shoppinglist with thermal printer #1273

Merged
merged 26 commits into from Jun 18, 2021
Merged

Add support for printing shoppinglist with thermal printer #1273

merged 26 commits into from Jun 18, 2021

Conversation

Forceu
Copy link
Contributor

@Forceu Forceu commented Jan 15, 2021

This PR adds the following library: https://github.com/mike42/escpos-php
This PR adds the following requirements: php-json, php-intl, php-zlib

New API call: /print/shoppinglist/thermal

With this PR it is possible to print the shoppinglist with a thermal printer straight from the UI or with the API. The configuration is through data.php and both network printers and usb printers are supported. COVID-19 was actually the reason why I created this, so that I don't have to touch my phone all the time while I am shopping.

Please note that composer updated symfony/polyfill-mbstring and symfony/polyfill-php80 while I was adding escpos-php

The API call is actually a GET call, is that okay or would you prefer a POST call for that?

image

@berrnd
Copy link
Member

berrnd commented Jan 15, 2021

Thanks a lot.

As this is very specific, and for example adds that grocy (kind of) directly communicates with hardware, maybe it would be better to provide this as an add-on (so as a separate tool/application which gets grocy data via the API).

Haven't done anything with thermal printers personally so far, interesting that they are not just normal printers protocol wise...

@berrnd
Copy link
Member

berrnd commented Jan 15, 2021

maybe it would be better to provide this as an add-on (so as a separate tool/application which gets grocy data via the API)

Via (providing a snippet for) custom JS it would still be possible to trigger this also from a button on the shopping list page.

@Forceu
Copy link
Contributor Author

Forceu commented Jan 15, 2021

Thanks for your reply! I fixed all the issues I mentioned, the PR now integrates nicely in the UI and does not output any errors anymore.
I think it would be great if it was included in grocy, as it only adds very few dependencies and I am sure a lot of people who buy barcode scanners for their home also have a thermal printer ;) From the user perspective it only adds a new button, which can be disabled with the Feature Flag or could be disabled by default.

I get your point with talking to hardware and why it would be an option to include it as an addon instead. Personally I think it is no difference to for example an LDAP configuration, as this needs to be configured as well and could be an addon.

@berrnd
Copy link
Member

berrnd commented Jan 15, 2021

Personally I think it is no difference to for example an LDAP configuration, as this needs to be configured as well and could be an addon.

Not really, as this provides authentication and not only needs to get data...

But ok, let's add it.

Looks ok at a first sight, some things I noticed:

  • Localization strings only need to be added to localization/strings.pot which is also done automatically when MODE is dev, all translations are then handled via Transifex (including the German translation)
  • It seems to currently output the amount without the unit - maybe that's what you wanted, but when using a different unit on the shopping list, this will still be the amount in QU stock, as all amounts in the database are related to that unit

@Forceu
Copy link
Contributor Author

Forceu commented Jan 15, 2021

Awesome thanks a lot! :)

Localization strings only need to be added to localization/strings.pot

Ah okay, in that case I revert the commit for the translation.

It seems to currently output the amount without the unit

Yes that is intentional to save space, as most printers have only a very limited width. I could however add a flag to include the quantities.

@berrnd berrnd self-requested a review January 15, 2021 12:14
@berrnd berrnd added this to the vNextFEATURE milestone Jan 15, 2021
@Forceu
Copy link
Contributor Author

Forceu commented Jan 15, 2021

Alright, so I included a quantity conversion and units and notes can be printed as well. Thanks a lot for including it :)

image

@mountainsandcode
Copy link

Love this idea and initiative - planning on creating something like this myself. I took a look at the library and while the protocol it uses does handle many printers, I noticed there are quite a few issue filings in the github of escpos-php with people who have some other printer, like a Brother label printer and DYMO printer, which don’t seem to be compatible because the use a different protocol.

I’m fully happy for this to be merged of course, but just two ideas that may be worthwhile considering:

A) Would it be possible to make this modular - like the Auth middleware - so that people can add their own printer libraries. Maybe someone just wants to print it on their regular printer?

B) Alternatively, I guess the real key here is having a convenient way of triggering the printing. Does it perhaps make sense to provide a way to register actions/ buttons somewhere - like registering a button on the shopping list page that calls a URL? That way, additional tools of any kind could be easily integrated without having to have all of the logic in grocy itself

As said, would be excited to have this merged, but just wanted to share my reflections.

@Forceu
Copy link
Contributor Author

Forceu commented Jan 26, 2021

Thanks a lot for the feedback :)

B) Alternatively, I guess the real key here is having a convenient way of triggering the printing. Does it perhaps make sense to provide a way to register actions/ buttons somewhere - like registering a button on the shopping list page that calls a URL? That way, additional tools of any kind could be easily integrated without having to have all of the logic in grocy itself

Yes, with that PR there is a new API call /print/shoppinglist/thermal. With that URL it can be triggered remotely.

@mountainsandcode
Copy link

My idea for B was slightly different - it would allow a button to be registered that calls an external URL - that way, the functionality could be maintained independently (e.g., a separate docker container which contains the script querying the grocy API and handling the printing), thereby keeping the flexibility of not having to integrate the functionality in the core but maintaining the usability of having the button integrated right there

But again, totally fine with me if this moves ahead as currently structured - looking forward to using it myself :)

@berrnd
Copy link
Member

berrnd commented Jan 26, 2021

Does it perhaps make sense to provide a way to register actions/ buttons somewhere - like registering a button on the shopping list page that calls a URL?

Consider simply using custom JS to add any button you want on any page, here is a snippet (for data/custom_js.html) which adds a button to the shopping list page which calls an external URL while also providing the current shopping list id as a query parameter.

<script>

if (Grocy.ActiveNav == "shoppinglist")
{
	$("#print-shopping-list-button").after('<a class="btn btn-outline-dark" target="_blank" href="https://example.com/whatever?shopping-list=' + $("#selected-shopping-list").val() + '">Custom button</a>');
}

</script>

@hackathi
Copy link
Contributor

hackathi commented Jun 7, 2021

Having thought about it a bit, while this is great to have implemented, I think this should be implemented as a webhook. Right now, this is only working if the printer is accessible from whatever machine is hosting grocy.

In #1500, I work on adding a webhook runner that could be used to dispatch this printing as well. This would also take care of own printing libraries – just implement a different webhook receiver, in whatever language you please, working with whatever printer language you need.

Unfortunately, much of this PR's code is so tightly integrated with grocy right now, that refactoring it into using a webhook is bascially a re-write. Still I think it'd be worth to consider.

@berrnd
Copy link
Member

berrnd commented Jun 12, 2021

Hi @Forceu, I think the WebHook approach introduced in #1500 by @mistressofjellyfish is a really good idea.

While I would say a lot grocy users run grocy at home and therefore could have a direct attached or via local network reachable printer, the use case where grocy runs elsewhere, where you even cannot directly connect the local/remote network via VPN or something (shared webhosting seems to be not so unpopular when referring to install problem request) is IMHO worth to consider...

This is specifically about printing shopping lists on thermal printers, so not really related to what was introduced in #1500, I'm still open to merge this as it is.

What do you think? Leave it as it is for now or (partly) re-write it to also use a WebHook approach?
(A default/example WebHook receiver could also be integrated into grocy somehow, providing at the end the same functionality as of now "out of the box"...)

@berrnd berrnd modified the milestones: vNextFEATURE, vNEXT Jun 12, 2021
@Forceu
Copy link
Contributor Author

Forceu commented Jun 14, 2021

Yes, I guess a webhook would make more sense and makes maintaining the feature a lot easier. You may close the PR in that case.

- Default FEATURE_FLAG_THERMAL_PRINTER to disabled
- Added missing localization strings (and slightly adjusted one)
Copy link
Member

@berrnd berrnd left a comment

Choose a reason for hiding this comment

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

Looks really good, works (from what I can test, don't have a thermal printer) - thanks again a lot! :)

For me the most important thing for grocy is still the practical outcome, this adds something which is probably useful for a couple of users - so let's add this for now as it is and maybe improve it later (WebHook), if that's really needed by someone...

@berrnd berrnd merged commit eb135ae into grocy:master Jun 18, 2021
berrnd added a commit that referenced this pull request Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Feature request: Shopping list thermal printer support
4 participants