search

TeamScheire / familiekiosk

Bouwplannen en uitleg om zelf een fotokanaal te maken, net zoals Maria in het RVT
https://www.canvas.be/team-scheire/maria-wil-meer-contact-met-haar-familie
Apache License 2.0
10 stars 4 forks source link

Print output to system journal, auto-rotate images and allow randomization of photo's #7

Closed ErwinP closed 4 years ago

ErwinP commented 5 years ago

You should know that I have no background in programming, let alone writing code in Python. Therefore, the suggested implementations might be written in a "dirty" way. It does work (it's running on 10" screen at my grandmothers house), but feel free to re-write this in a better way.

Secondly: my grandmother was very pleased with this gift - therefore: THANK YOU!

bmcage commented 4 years ago

@ErwinP As people run into errors with current python, I would merge this. Then see if I would do some things differently when I upgrade my box and move to python3 (don't fix what's not broken). Is this PR finalized? The code looks good to me, for somebody with no prior knowledge, congrats!

The main thing I'm worried about is changes to the documentation, so the .md files. I believe only RANDOMIZE_PHOTOS in config.in could be stated also in the documentation, correct?

ErwinP commented 4 years ago

@ErwinP As people run into errors with current python, I would merge this. Then see if I would do some things differently when I upgrade my box and move to python3 (don't fix what's not broken). Is this PR finalized? The code looks good to me, for somebody with no prior knowledge, congrats!

Thank you :) The code until commit f81bc6f was being used as such on the familiekiosk of my grandmother, without problems. However, at that time, no buttons were present, we didn't send video's or audio, and there was no reply button. Meaning I don't know if it affects that. In the mean time, I added a next and previous button, and that still worked (it determines the random order once, and than you're able to navigate through it).

Code untill 845443a has been tested by janvangent. He ended up cloning my fork and using that, rather than applying all changes manually. @janvangent perhaps you can comment furhter?

The main thing I'm worried about is changes to the documentation, so the .md files. I believe only RANDOMIZE_PHOTOS in config.in could be stated also in the documentation, correct?

That is correct.

PS: excuse my previous comment abouth the python transition not being present in this PR; I didn't know these got updated dynamically.

bmcage commented 4 years ago

Merged. Thanks. Keep the PR coming if you do further improvements!

janvangent commented 4 years ago

@ErwinP As people run into errors with current python, I would merge this. Then see if I would do some things differently when I upgrade my box and move to python3 (don't fix what's not broken). Is this PR finalized? The code looks good to me, for somebody with no prior knowledge, congrats!

Thank you :) The code until commit f81bc6f was being used as such on the familiekiosk of my grandmother, without problems. However, at that time, no buttons were present, we didn't send video's or audio, and there was no reply button. Meaning I don't know if it affects that. In the mean time, I added a next and previous button, and that still worked (it determines the random order once, and than you're able to navigate through it).

Code untill 845443a has been tested by janvangent. He ended up cloning my fork and using that, rather than applying all changes manually. @janvangent perhaps you can comment furhter?

The main thing I'm worried about is changes to the documentation, so the .md files. I believe only RANDOMIZE_PHOTOS in config.in could be stated also in the documentation, correct?

That is correct.

PS: excuse my previous comment abouth the python transition not being present in this PR; I didn't know these got updated dynamically.

Correct, with your changes everything works 100%! Great work