boramalper / himawaripy

Set near-realtime picture of Earth as your desktop background
http://labs.boramalper.org/himawaripy
MIT License
1.62k stars 239 forks source link

[macOS][bug] changing the wallpaper restarts the Dock, which is annoying #70

Closed sg-s closed 8 years ago

sg-s commented 8 years ago

problem is as described: when the wallpaper is reset, the Dock reloads, causing it to close and open, which is very distracting.

Surely there is a better way of changing the wallpaper than reloading the Dock/Finder?

boramalper commented 8 years ago

Found the cause! We explicitly restart the Dock to (I think?) make OS X refresh the image.

Since I have no idea about OS X, maybe the author of OS X support (@jcmiller11) might help us here. :)

sg-s commented 8 years ago

this may be a bit of a overkill, but maybe use wallpaper?

it takes just one line to change the wallpaper, and does not need you to kill the Dock:

wallpaper path/to/file
sg-s commented 8 years ago

is there more than one killall?

I tried replacing that section with:

    elif de == "mac":
        subprocess.call(["wallpaper " + file_path])
        # subprocess.call(["osascript", "-e", 'tell application "System Events"\n'
        #                  'set theDesktops to a reference to every desktop\n'
        #                  'repeat with aDesktop in theDesktops\n'
        #                  'set the picture of aDesktop to \"' + file_path + '"\nend repeat\nend tell'])
        # subprocess.call(["killall", "Dock"])

but the Dock still restarts. mysterious.

boramalper commented 8 years ago

is there more than one killall?

No. It's even more mysterious for me as someone who never used a mac. :)

I'll look for a Python module to set the wallpaper, or going to create one as a separate project. The current status seems like a bit of mess and that really bothers me. I thought about using appscript but it's abandoned. Making people install a node.js app for a 300 lines of Python script seems like an overkill.

Thank you so much! I'm keeping this open, in case someone finds a solution.

sg-s commented 8 years ago

nevermind, fixed it:

subprocess.call(["wallpaper", file_path])

works. you need to have wallpaper installed.

boramalper commented 8 years ago

Glad to hear! I'll keep the issue open though, for the problem is still not resolved.

hche608 commented 8 years ago

@sg-s @boramalper wallpaper It works, but the code only runs once, I have to use code like this to keep it to run every 10 mins. This does not make sense.

        subprocess.call(["osascript", "-e", 'tell application "System Events"\n'
                         'set theDesktops to a reference to every desktop\n'
                         'repeat with aDesktop in theDesktops\n'
                         'set the picture of aDesktop to \"' + file_path + '"\nend repeat\nend tell'])
        #subprocess.call(["killall", "Dock"])
        subprocess.call(["wallpaper", file_path])

this can execute can every 10 mins.

boramalper commented 8 years ago

Thanks, but I'm strongly against using wallpaper, maybe I can dig into their source and find how they do it. :)

sg-s commented 8 years ago

@hche608

not sure why you're using the AppleScript in addition to wallpaper. To be clear, just this works:

subprocess.call(["wallpaper", file_path])
sg-s commented 8 years ago

@boramalper agreed; the simpler the better!

jcmiller11 commented 8 years ago

Hey guys, was on vacation when this was happening! So as I commented when I added that line:

https://github.com/jcmiller11/himawaripy/commit/8fc30136f5d7cce418cb1eb9ea1a4f678381dd51

The killing of the dock is to stop a problem where OS X would cache the wallpaper in RAM and not check to see if the picture file had changed given that it was using the same filename each time. It was kind of a hacky solution but it works. A more elegant solution might be to make the filename of the latest downloaded image be unique each time, I believe that would work, but I didn't pursue that method at the time because I didn't want to mess with too much of the non-platform specific stuff just to get OS X support up and going.

@boramalper would you be opposed to using unique filenames in other platforms rather than just the filename-latest.png ?

Also note, if you take a quick glance at how wallpaper does it, it seems to depend on a compiled binary written in objective-c: https://github.com/sindresorhus/macos-wallpaper which itself depends on Apple's Appkit. No clue whether this method also has the issue with multiple calls to the same filename not always updating.

Edit: a quick search seems to suggest that the method that wallpaper uses also suffers from the same issue: http://stackoverflow.com/questions/34607809/redraw-desktop-background-without-restarting-dock-in-os-x That stack overflow question seems to suggest that using unique filenames will work... unless the user is currently in a fullscreen application when the update happens... which seems to just be a bug with OS X that should ideally be filed with Apple.

jcmiller11 commented 8 years ago

It's been a long time since I touched all of this code, and it's all coming back to me solving this problem when I first added that killall line. We can add code to the main system to do something like timestamps for a unique filename to partially solve the problem of the picture update only working occasionally, but then we need to deal with a more complex cleanup system to remove them later... which only solves the problem if the user is not in full screen... or we can just kill the dock, which is also inconvenient... particularly if the user makes use of some window management systems in OS X like minimizing....

Kinda sucks but I couldn't find a perfect solution :/ so I just went with the scorched earth approach by just killing the process

sg-s commented 8 years ago

how bad would it be to use unique filenames on macOS? you could even use random file names -- i agree it is clunky but that seems to be the only reasonable way

jcmiller11 commented 8 years ago

would have to add in some whole way to store the name of the last file to delete, destroying the simplicity of relying on each native filesystem to overwrite the file, would most easily be implemented outside of the environment specific code, still wouldn't totally solve the problem. I'll wait for @boramalper to weigh in on this

boramalper commented 8 years ago

I think we can append the date in the file name. Then we'll delete the last image, and save the new one.

Since himawaripy is using appdirs to save images in its own dedicated directory, we can check if there is only one .png file with the prefix himawaripy- in that directory, we can delete that image and save the latest one; if not, we will exit the program to prevent any accidental data loss.

I'm really busy this week, but I'll try to implement it as soon as possible afterwards. :) Thank you all for your contribution!

sg-s commented 8 years ago

how about a cruder approach?

  1. delete all .png files with the prefix "himawaripy" in the target folder
  2. download the new image, save it with a random name (or timestamp)
  3. set wallpaper (since filenames are now unique, it should work)

repeat

boramalper commented 8 years ago

@sg-s Why cruder? The only difference (checking whether there are more than one .png file to be deleted) is very very simple to implement (just checking the length of the results) yet can save us from a possible catastrophe. Given that how many things went wrong with this intended-to-be-small-script, I'm really hesitant of crude approaches. :)

jcmiller11 commented 8 years ago

Agreed, any script that deletes a file should do so very judiciously as a small bug can be catastrophic, for example: https://github.com/MrMEEE/bumblebee-Old-and-abbandoned/issues/123

Using a random name might necessitate an extra check for the very small chance of a collision lest one in a million updates fail to display for mac users, lol. I think @boramalper 's proposed solution is the best choice.

boramalper commented 8 years ago

This commit should resolve the issue. Can you confirm @sg-s?

sg-s commented 8 years ago

HI @boramalper, thanks! I think you forgot to remove this line:

subprocess.call(["killall", "Dock"])

in utils.py

I removed it, and it looks like it's working! Awesome!

boramalper commented 8 years ago

@sg-s Fixed. Thank you all so much!