PiSupply / PaPiRus

Resources for PaPiRus ePaper eInk displays
https://www.pi-supply.com/product/papirus-epaper-eink-screen-hat-for-raspberry-pi/
Other
346 stars 88 forks source link

Initial unicode support for Text and TextPos #112

Closed cyberscribe closed 7 years ago

cyberscribe commented 7 years ago

See line 38 of papirus/textpos.py - I am not sure why we are using decode('string_escape') but it is not utf-8 friendly. All tests seem to work fine without it. Please advise before merging.

shawaj commented 7 years ago

@cyberscribe - thanks very much. Not got a PaPiRus with me at home this weekend but will test and merge next week with @francesco-vannini

cyberscribe commented 7 years ago

Excellent, I tested what I could but would be great to have another pair of eyes. Also do be sure to check out the function I dropped--didn't affect all the sample cases I tried, but I'm not sure if it was put in there for an edge case I might have missed. Which reminds me--are there any unit tests for this package?

shawaj commented 7 years ago

When you say unit tests, what do you mean? Sorry we're not really software experts so bear with us on the terminologies :-)

And which function did you drop?

cyberscribe commented 7 years ago

Hi @shawaj -- I dropped the .decode('string_escape') from line 38.

Here's a decent introduction to unit testing: https://python-guide-pt-br.readthedocs.io/en/latest/writing/tests/

It would basically save time to have a battery of all edge cases to test, because future contributors could validate that their changes have not broken anything by running these automated tests before submitting a pull request.

shawaj commented 7 years ago

Awesome. That sounds great. Could you submit an issue for that unit testing? Sounds like a very good idea! Would certainly save us a lot of time!

cyberscribe commented 7 years ago

Have done!

On 23 April 2017 at 00:45, Aaron Shaw notifications@github.com wrote:

Awesome. That sounds great. Could you submit an issue for that unit testing? Sounds like a very good idea! Would certainly save us a lot of time!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/PiSupply/PaPiRus/pull/112#issuecomment-296408936, or mute the thread https://github.com/notifications/unsubscribe-auth/AAqV5Xv6XLRNOdno6_Z4dAdU78Rgt-dUks5rypERgaJpZM4NFK2r .

francesco-vannini commented 7 years ago

This solves #10. Thanks @cyberscribe for your help! Tested on both text.py and textpos.py

cyberscribe commented 7 years ago

Great! Happy to help, and happy to be able to draw unicode symbols on my PaPiRus!

shawaj commented 7 years ago

@francesco-vannini did you check that the removal of decode('string_escape') hasn't caused any knock on errors?

Was thinking it might stop line breaks from being recognised...

francesco-vannini commented 7 years ago

I tested that and it still works. I don't quite see why the decode method was used really http://www.tutorialspoint.com/python/string_decode.htm @BenScarb could you maybe clarify this?

shawaj commented 7 years ago

Perhaps it was to stop attack code being introduced - although that would be a bit weird for you to attack your own PaPiRus

cyberscribe commented 7 years ago

Seems something like this will strip non-printable Unicode characters from the output if there is still a concern about data sanitisation: http://stackoverflow.com/a/93557

BenScarb commented 7 years ago

Can we pretend I did it for data security? ;) I added it in as it was a way to make sure the /n and /r were recognised, without it (on my set up) throwing /n line breaks weren't being seen and were getting shown on the display. I'll do a pull and see what happens, it'll probably work a treat.

If it's working without, great stuff, the less code in there, the faster it'll work!

shawaj commented 7 years ago

@francesco-vannini did you test with both /n and /r line breaks?

If so then happy days!

francesco-vannini commented 7 years ago

I have checked again with and without the changes in this PR. Using \n or \r still works (not to be confused though with /n and /r which are printed on the screen)