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 fit_image_to_screen function in utils #150

Merged
merged 36 commits into from Dec 21, 2018

Conversation

Projects
None yet
2 participants
@LouisPi
Copy link
Collaborator

commented Dec 6, 2018

DO NOT ACCEPT YET. Should finish it tonight.

Edit - resizing script should be working, see comment below.

@codecov

This comment has been minimized.

Copy link

commented Dec 6, 2018

Codecov Report

Merging #150 into devel will increase coverage by 0.72%.
The diff coverage is 92.53%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
ui/printer.py 65.78% <100%> (+15.78%) ⬆️
ui/utils.py 93.8% <90.47%> (-2.03%) ⬇️
ui/tests/test_printer.py 75% <95.45%> (+20.45%) ⬆️
ui/overlays.py 97.24% <0%> (-2.76%) ⬇️
apps/media_apps/draw/main.py 37.43% <0%> (-1.07%) ⬇️
apps/wifi_country/main.py 27.02% <0%> (ø)
ui/base_list_ui.py 76.69% <0%> (+0.25%) ⬆️
apps/clock/main.py 21.31% <0%> (+0.47%) ⬆️
ui/funcs.py 67.79% <0%> (+1.12%) ⬆️
ui/tests/test_listbox.py 84.9% <0%> (+1.23%) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bf6412...82421fe. Read the comment docs.

@CRImier

CRImier approved these changes Dec 6, 2018

Copy link

left a comment

Mostly OK

Show resolved Hide resolved ui/printer.py Outdated
Show resolved Hide resolved ui/printer.py Outdated
Show resolved Hide resolved ui/printer.py Outdated
Show resolved Hide resolved ui/printer.py Outdated
@LouisPi

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 6, 2018

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.

@LouisPi

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 6, 2018

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.

@LouisPi

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 9, 2018

TODO - add tests

@CRImier

This comment has been minimized.

Copy link

commented Dec 9, 2018

Ahha. There's actually a lot of code in GraphicsPrinter by now (which makes it harder to read), and I think apps/other UI elements could make use of that as well, could you maybe make it into a separate function? I.e. fit_image_to_screen(image, o).

@LouisPi

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 9, 2018

Okay, I will try add that!

Where do you want it added? In printer.py still?

@CRImier

This comment has been minimized.

Copy link

commented Dec 9, 2018

That's a good question! I think it'd make sense if that code were in ui/utils.py.

@LouisPi

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 9, 2018

Okay thanks.

@LouisPi LouisPi changed the title first changes to GraphicsPrinter Add fit_image_to_screen function in utils Dec 9, 2018

Show resolved Hide resolved ui/printer.py Outdated
@CRImier
Copy link

left a comment

LGTM, but does need tests

Show resolved Hide resolved ui/utils.py Outdated
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")

This comment has been minimized.

Copy link
@CRImier

CRImier Dec 10, 2018

=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!

This comment has been minimized.

Copy link
@LouisPi

LouisPi Dec 10, 2018

Author Collaborator

Thanks for the feedback! Will change that tomorrow.

Show resolved Hide resolved ui/tests/test_printer.py
Show resolved Hide resolved ui/tests/test_printer.py Outdated
Show resolved Hide resolved ui/tests/test_printer.py Outdated
Show resolved Hide resolved ui/tests/test_printer.py Outdated
@CRImier
Copy link

left a comment

Mostly fine, fix the typo and I'll merge =)

Show resolved Hide resolved ui/utils.py Outdated

LouisPi added some commits Dec 21, 2018

@CRImier
Copy link

left a comment

Looks good to me!

@CRImier CRImier merged commit 519897e into ZeroPhone:devel Dec 21, 2018

2 checks passed

codecov/patch 92.53% of diff hit (target 39.83%)
Details
codecov/project 40.56% (+0.72%) compared to 5bf6412
Details
@CRImier

This comment has been minimized.

Copy link

commented Dec 21, 2018

Thank you!

@LouisPi

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 21, 2018

No problem - you pretty much told me how to do all the tests though =D so thank you as well.

@CRImier

This comment has been minimized.

Copy link

commented Dec 21, 2018

Well, there's no tutorial for ZP devs on that yet =) Also, it's not something people do a lot, unfortunately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.