search

markubiak / wallpaper-reddit

Downloads and sets wallpapers pulled from reddit.com
GNU General Public License v3.0
113 stars 42 forks source link

Improved support for non-jpg source images #40

Closed cdglitch closed 6 years ago

cdglitch commented 6 years ago

Fixes an issue with certain images. Based on solution found here: https://github.com/python-pillow/Pillow/issues/1380 which can cause a mysterious "Error saving image!" message

cdglitch commented 6 years ago

0b73dea (extra feature) adds a lottery mode I found myself wanting. I run a 4920x1920 desktop resolution so I wasnt getting frequent changes. --lottery allows selects a random image in the last n that wpreddit fetches and uses a random one provided it isnt the same as the current one.

Its a dirty hack (I only dabble in python and thats it. Think <500 lines a year) so it might need cleaned up but it seems to work just fine for me.

Also, for any people running i3 on multiple monitors and who want a single bg image accross all screens; set the change command to run [feh --bg-max --no-xinerama --bg-scale /home/bengreggsmith/.wallpaper/wallpaper.png]

markubiak commented 6 years ago

Hey there, sorry for the late response. School and whatnot.

Based on your code and thorough documentation, I'm guessing you have significant experience in another language. I don't pretend to be a python expert, but this language shoots for maximum code cleanliness at the expense of performance. In my opinion, your changes to config.py are fine, but your changes in reddit.py are overly complicated for python. I don't see what the random.seed() does, as we really don't need to update the seed for picking random list elements in my opinion and I have not seen that function used. It would also be significantly cleaner to keep my original code and simply do an in-place shuffle before the for loop in choose_valid() using the function random.shuffle(links). Let me know your thoughts.

Thanks for the PR!

cdglitch commented 6 years ago

That's all pretty fair for the main implementation of --lottery mode. I'm currently also in exams but I should a bit of time afterwards to clean that up some.

I added the random.seed() since for whatever reason the first few times I did some testing I always got back the same image on my system and did not find a call to initialize the RNG anywhere else which in the languages I usually work with would cause that behaviour to happen.

Have you got a preferred method of doing an in-place shuffle that uses a built in python feature or would I be okay going ahead and just doing that in a loop straight up?

markubiak commented 6 years ago

Again, the random.shuffle(list) function does an in-place shuffle on a list. That's what I'd recommend for simplicity.

markubiak commented 6 years ago

I actually just encountered this; your first commit with the RGB conversion worked perfectly. If you're close to finishing the lottery just update the PR; if not, I'd appreciate if you'd file this fix separately and then do the lottery later.

cdglitch commented 6 years ago

Sorry for the late reply on my end, exams only just ended yesterday for me. I'll see to fixing up lottery mode just now.

cdglitch commented 6 years ago

Did some quick testing of the cleanup in reddit.py and it didnt put up a fight. Everything seems to work and its a bit cleaner now. See what you think.

markubiak commented 6 years ago

I tested out your changes with the lottery; the url changes worked very well but unfortunately the titles and permalinks weren't getting shuffled or really tracked at all. I ended up just fixing it up; my data structures in this project were always a little bit messy; I had three separate lists for the links, titles, and permalinks. I ended up hanging it to a single list of tuples [link, title, permalink] and changed up the code to suit. It wasn't terribly difficult and is a much better way of going about it. You're free to review the commit; I'm unsure why Github doesn't put anything about my commit in this PR. I checked out your PR and made my commits there; that may be why. Anyhow, thanks for the help. I'd appreciate if you'd check out master and confirm that everything's working fine. My preliminary tests are going swimmingly.

cdglitch commented 6 years ago

Fair. Really just wanted to get this thrown out there as a usability thing in case someone else was wanting something similar. I'm actually on holiday at the moment enjoying the near-dialup speeds so I'll take a look when I get back home in the new year. I have to confess I did very little reading of the rest of the codebase and python feels pretty alien to me. Glad it didn't take too long by the sounds of it.