Added possibility to store the images in custom paths

dpellegr commented 4 years ago

Hi and thank you for the very cool script.

I have been enjoying it for a while, but sometimes I regretted that a particularly beautiful image was discarded. I modified the script a bit and added a configuration file allowing to specify a custom working path and the possibility to store the images in it. The default behavior of discarding the images is unchanged.

Also in the PKGBUILD you can drop the perl dependency now!

mariusknaust commented 4 years ago

Hi and thank you for the contribution!

As this pull request contains multiple commits I will have a look at it soon and merge it and/or give you feedback.

dpellegr commented 4 years ago

Thank you for your interest. There are several commits as I have been testing and refining it over a few days, but please, do not go through all of them. Checking the global diff is more than enough:

https://github.com//mariusknaust/spotlight/compare/master...dpellegr:master

mariusknaust commented 4 years ago

I’ve made a few changes and merged it:

Avoid a global config file and added instructions to overwrite the service on a per-user basis
Store images in .local/share/backgrounds by default, as this seems to be the correct path for them to show up in the Gnome settings. Also adapted the naming scheme.
Removed the PKGBUILD, as it is maintained in a different repository

Please check if this still fulfills your needs and let me know if changes are needed.

Thanks again for the contribution!

dpellegr commented 4 years ago

Excellent! Your edits are nice and I approve all of them, but I disagree with you on a design concept. Here we go!

I think that the only benefit of using systemd is when you enable the timer. Other than that is just a complication that will cause you issues like https://github.com/mariusknaust/spotlight/issues/6. If you make systemd optional, people that are less familiar with it (or distros that do not ship with it like Gentoo and Alpine) can just run the script manually (or through a cron job) and enjoy the new wallpaper whenever they like.
- For this reason, I really do not like that you start the systemd service in the .desktop file. Whether I have the timer enabled or not, if I want to change the background I will just call the script. The only reason for calling the script through a service is if I want to fix the command line options...
- ...but that is an overkill! Having a tiny sourceable config file which also documents the options is so much better. With the script in my $PATH, if I dislike the background I just type spotl[TAB][ENTER] without having to remember the options, calling systemd or navigating in the desktop environment to search the desktop file.
If you do not want to force the config for all the users, then the script can look first for /etc/spotlight.conf, then for $HOME/.local/share/spotlight.conf and finally for the command line options; each, if present, overriding the previous.

What do you think?

mariusknaust commented 4 years ago

Okay, I get what you are saying. And you are right, it’s a design decision, it was designed from the beginning with the systemd timer as a main use case (spotlight.sh shouldn’t have been documented in the first place). Hence, the desktop-file triggering the one shot service, to have consistent behavior with the timer when overwriting the service. As it is currently made specific for gnome [SUPER]spot[ENTER] should do the job, when disliking a background.

But I agree and thanks for pointing this out, it’s a great pity to exclude the Gentoo and Alpine folks. The local config file might be an option, but still seems to be a bit excessive for such a small service with so little configurations. A custom script in the local bin folder calling the global script with the needed parameters might do the job?

dpellegr commented 4 years ago

There is no such a thing as "too little configuration". Several files in /etc/ are just single liner or even a single word like /etc/hostname! There is no problem with that, a conf file is just the way of setting persistent options, no matter if one or a thousand.

And no: the conf file should not go together with the executable. It might be that you see this project as a tiny little service to be petted in the home directory, but I assure you that to the rest of the world it is just another one of the zillions of services running in the background and therefore it should follow the conventions:

executable in /usr/bin/
conf file in /etc/
service and timer in /I/never/remember/systemd/paths
user conf override in $HOME/.local/

Then we let our great package managers do their work keeping track of all the files. This is what I learned in my decade-long experience with Arch as my main OS, in short: Keep It Simple! :)

dpellegr commented 4 years ago

I've just got another idea! If I dislike the background and I change it immediately, I probably do not want to store it. Therefore an option to save the past background only if they have been displayed for longer than a certain amount of time would be nice. It should be easy by looking at the file creation time.

I'll wait a bit to proceed to see if we can sort out this systemd vs conf file thing...

mariusknaust / spotlight

Added possibility to store the images in custom paths #8