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 fit_image_to_screen function in utils #150
Conversation
Codecov Report
@@ Coverage Diff @@
## devel #150 +/- ##
==========================================
+ Coverage 39.83% 40.56% +0.72%
==========================================
Files 247 248 +1
Lines 19703 19947 +244
==========================================
+ Hits 7849 8091 +242
- Misses 11854 11856 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly OK
Definitely not ready yet just thought I would start a pull request. Renaming variables was on my TODO as I realise the names are confusing. The typo is because I originally tried out the new Graphics Printer in an old ZPUI directory I made and had to copy everything over. Deleted the old directory now so that shouldn't happen again. Am thinking of scrapping my current technique for resizing to make an image bigger and utilising some of this (https://gist.github.com/tomvon/ae288482869b495201a0) code. |
No tests added yet. Variable names might need clearing up to avoid confusion, comments need to be added. I have tested this as well as I can on my screen trying out multiple image sizes. Will try sort any issues listed ASAP. Help on making tests for it would be appreciated =D. |
…other smaller than the correct size
…e as the screen size
TODO - add tests |
Ahha. There's actually a lot of code in |
Okay, I will try add that! Where do you want it added? In printer.py still? |
That's a good question! I think it'd make sense if that code were in |
Okay thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but does need tests
ui/utils.py
Outdated
@@ -251,4 +254,6 @@ def fit_image_to_screen(image, o): | |||
top = delta // 2 | |||
bottom = delta - top | |||
image = ImageOps.expand(image, border=(left, top, right, bottom), fill="black") | |||
if (o.width, o.height) != image.size: | |||
logger.debug("Something strange has happened! Please contact LouisPi on the Zerophone IRC for help") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
=D Better throw an exception? People won't read the logs (and with logger.debug
being the lowest logging level typically not enabled by default, this will not appear in logs anyway). Also, no need to tell people to contact anyone - with each extra work they need to do, there's less and less possibility they will do it. We do have bugreporting and will be adding custom exception hooks to make reporting automatic, don't worry =) In general, very mindful of you to add this - one small thing, for runtime checks like this, it's best if you use assert
, it will throw an exception by itself too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback! Will change that tomorrow.
This reverts commit 53d2a3d.
…et image dimensions and do stuff with them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly fine, fix the typo and I'll merge =)
…d equal dimensions compared to the image
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
Thank you! |
No problem - you pretty much told me how to do all the tests though =D so thank you as well. |
Well, there's no tutorial for ZP devs on that yet =) Also, it's not something people do a lot, unfortunately. |
DO NOT ACCEPT YET. Should finish it tonight.
Edit - resizing script should be working, see comment below.