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

Allow switching output image format #116

Merged
merged 2 commits into from
Mar 15, 2024
Merged

Allow switching output image format #116

merged 2 commits into from
Mar 15, 2024

Conversation

nbarrientos
Copy link
Contributor

@nbarrientos nbarrientos commented Mar 15, 2024

Totally untested -- never ran. I've never written any NodeJS either 😁

This patchset assumes that LaTrappe's patch works.

It's interesting that LaTrappe's patch only changes the type attribute when creating the screenshot and a jpeg image is directly passed to convertImageToKindleCompatiblePngAsync which seems to happily deal with it and output jpeg, too? If that's the case then that function should be renamed as part of this patch.

The change is not backwards compatible as OUTPUT_PATH does not contain the extension anymore. Another option is to avoid using an extra configuration variable (IMAGE_FORMAT) and extract the format from the path by looking at the extension. Your call.

Closes #109.

@sibbl
Copy link
Owner

sibbl commented Mar 15, 2024

I'm really happy to see this PR so quickly after your "+1" comment this morning! 👍

Some first thoughts from a quick glance:

  1. from my point of view, the breaking change to remove the extension from the OUTPUT_PATH env var is okay. I guess most people are not using it anyway and it's rather something internal.
  2. as far as I see, you append the extension when saving the file, but not when reading it (see the fs.readFile call)
  3. please also add the new IMAGE_FORMAT variable to the run.sh and config.yaml to also let Home Assistant Add-On use it

I'd be happy to see these changes before accepting the PR and releasing a new version. Thanks a lot for your work and a happy weekend already!

@nbarrientos
Copy link
Contributor Author

Thanks for your comments, I think I've incorporated all your suggestions (history rewritten).

Please test yourself if you have time before merging, I haven't run anything myself 😆

Have a great week-end you too!

@nbarrientos
Copy link
Contributor Author

It's never late to learn some NodeJS and NPM ;). It seems to work for me. This is file on the generated image (after downloading it from the webserver):

JPEG image data, JFIF standard 1.01, resolution (DPI), density 72x72, segment length 16, baseline, precision 8, 600x800, components 1

@sibbl sibbl merged commit e616150 into sibbl:main Mar 15, 2024
@sibbl
Copy link
Owner

sibbl commented Mar 15, 2024

Thanks for the quick changes and also setting it up locally - it's always good to have a 2nd confirmation. I just updated the version and changelog: 1.0.9 is currently building and should be available soon! Great job 👍

@nbarrientos
Copy link
Contributor Author

Thanks for reviewing. I've deployed the latest Docker image IRL and it seems to work fine (JPEGs are generated).

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.

Feature request: output to jpg file format
2 participants