danyspin97 / wpaperd

Modern wallpaper daemon for Wayland
GNU General Public License v3.0
332 stars 26 forks source link

`wpaperctl next` can choose the same wallpaper #67

Closed jdek closed 5 months ago

jdek commented 5 months ago

Thanks your project, I like it a lot. For some reason with default settings it's possible that wpaperd will select the same wallpaper as the currently applied one. (There's also a race condition with wpaper next -> wpaperctl get-all where the old paper is printed, but this could be regarded as by design, i.e. the wallpaper is only changed when it finishes changing not when it starts).

My config:

[default]
path = "/home/user/Pictures/Wallpapers"

Example:

~/Pictures/Wallpapers$ ls
nasa-hubble-space-telescope-SGIy0Vp3qEo-unsplash.jpg  o3746154.jpg

~/Pictures/Wallpapers$ wpaperctl next && sleep 1 && wpaperctl get-all
DP-2: /home/user/Pictures/Wallpapers/nasa-hubble-space-telescope-SGIy0Vp3qEo-unsplash.jpg

~/Pictures/Wallpapers$ wpaperctl next && sleep 1 && wpaperctl get-all
DP-2: /home/user/Pictures/Wallpapers/o3746154.jpg

~/Pictures/Wallpapers$ wpaperctl next && sleep 1 && wpaperctl get-all
DP-2: /home/user/Pictures/Wallpapers/nasa-hubble-space-telescope-SGIy0Vp3qEo-unsplash.jpg

~/Pictures/Wallpapers$ wpaperctl next && sleep 1 && wpaperctl get-all
DP-2: /home/user/Pictures/Wallpapers/o3746154.jpg

~/Pictures/Wallpapers$ wpaperctl next && sleep 1 && wpaperctl get-all
DP-2: /home/user/Pictures/Wallpapers/nasa-hubble-space-telescope-SGIy0Vp3qEo-unsplash.jpg

~/Pictures/Wallpapers$ wpaperctl next && sleep 1 && wpaperctl get-all
DP-2: /home/user/Pictures/Wallpapers/o3746154.jpg

~/Pictures/Wallpapers$ wpaperctl next && sleep 1 && wpaperctl get-all
DP-2: /home/user/Pictures/Wallpapers/nasa-hubble-space-telescope-SGIy0Vp3qEo-unsplash.jpg

~/Pictures/Wallpapers$ wpaperctl next && sleep 1 && wpaperctl get-all
DP-2: /home/user/Pictures/Wallpapers/o3746154.jpg

~/Pictures/Wallpapers$ wpaperctl next && sleep 1 && wpaperctl get-all
DP-2: /home/user/Pictures/Wallpapers/o3746154.jpg

I'm using wpaperctl 1.0.1 from NixOS Unstable.

danyspin97 commented 5 months ago

Hello @jdek ! Thank you for your kind words, I really appreciate it :)

Regarding the bug, I have added a quick fix so that wpaperd always chooses a new wallpaper over the current one. Previously it was always based on a random number, so it might happen (in your case 50% chance) that it might draw the same wallpaper.

danyspin97 commented 5 months ago

(There's also a race condition with wpaper next -> wpaperctl get-all where the old paper is printed, but this could be regarded as by design, i.e. the wallpaper is only changed when it finishes changing not when it starts)

I have looked more into this, it doesn't depend on the transition, and I don't think it might be considered a race condition. Rather, it depends on loading the image from the disk; this operation might fail for many reasons, so I wait until it has been successfully decoded to update the current image. If the image loader fails, we try another one, in the meanwhile the current image is still the previous one.

This leads to the question, should wpaperd show something about images that are being loaded? Should it behave differently than now? Let me know what you think.

jdek commented 5 months ago

Honestly, I'm not really sure. It's nice to have the commands dispatched asynchronously, so any blocking action which would prevent the cli from returning whilst the daemon swaps wallpaper doesn't really make sense.

You could decide to preload the next image such that the switch will occur instantly but this will only move the problem around (if you do next quickly in succession the next command will eventually catch up to the preload and a get-all will return the same wallpaper twice).

The current behaviour is probably OK as is.