danyspin97 / wpaperd

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

Previous wallpaper functionality #36

Closed Rolv-Apneseth closed 1 year ago

Rolv-Apneseth commented 1 year ago

In relation to #34

Add previous-wallpaper subcommand for wpaperctl which now goes backwards through a newly implemented vec of cached, previously used wallpaper paths.

I'm sure you'll have some suggestions to make so just let me know what you think and about any changes I should make etc.

Also I have no way of testing how/if it's working with multiple monitors, but I think it should work fine.

Rolv-Apneseth commented 1 year ago

I see this will have direct conflicts with #35, but maybe with this PR, the whole files vec could be cached, in the desired sorting order, and next-wallpaper would go through the wallpapers accordingly.

anoldguy commented 1 year ago

Happy to revisit my stab at ordering in light of this. I agree, a cached list of previously displayed wallpapers would be cool.

Rolv-Apneseth commented 1 year ago

Sure yeah, might be easier to wait for changes to this first, see if it will be merged etc., we just have to wait for @danyspin97 to suggest how best to proceed since it's his project

danyspin97 commented 1 year ago

Thank you @Rolv-Apneseth for you contribution!

I see this will have direct conflicts with https://github.com/danyspin97/wpaperd/pull/35, but maybe with this PR, the whole files vec could be cached, in the desired sorting order, and next-wallpaper would go through the wallpapers accordingly.

Caching would definitely makes things simpler. Although I haven't done that because I personally don't like daemons that needs to be restarted due to some small changes. In this case wpaperd should be either restarted after a wallpaper has been added and/or removed in the directory or there should be some reload command. Maybe it's just my pet-peeve and we should do it anyway. Let me know what you think

Sure yeah, might be easier to wait for changes to this first, see if it will be merged etc., we just have to wait for @danyspin97 to suggest how best to proceed since it's his project

I would go with #35 first since it's smaller, and then add this PR on top, if you are both okay with this.

Also, many thanks @Rolv-Apneseth for updating the README, I totally forgot about updating it after adding wpaperctl. In addition I forgot to move the build.rs 😓 .

Rolv-Apneseth commented 1 year ago

I'll go through the changes in the review shortly, just to reply:

Caching would definitely makes things simpler. Although I haven't done that because I personally don't like daemons that needs to be restarted due to some small changes. In this case wpaperd should be either restarted after a wallpaper has been added and/or removed in the directory or there should be some reload command. Maybe it's just my pet-peeve and we should do it anyway. Let me know what you think

I'll be honest and say this is the first time I'm even looking at code for a daemon, so I'd say you definitely have a better idea of what should be done haha. I'll have a look at a reload command, you mean something like wpaperctl reload or how should it be called?

I would go with #35 first since it's smaller, and then add this PR on top, if you are both okay with this.

Completely fine by me yeah, I'll rebase once it's merged.

Also, many thanks @Rolv-Apneseth for updating the README, I totally forgot about updating it after adding wpaperctl. In addition I forgot to move the build.rs

No worries, it confused me for a minute there so figured I may as well update it. Was that serde dependency fine too, hypaperctl wasn't building for me before adding that

Rolv-Apneseth commented 1 year ago

Would something like this for the reload work, since it would be like running it for the first time again:

/// Update wallpapers by clearing cached image paths to force a refetch of the directory
/// Use after a new file is added / a file is removed from the directory
pub fn update_wallpapers(&mut self) {
    self.timer_expired = true;
    self.current_index = 0;
    self.image_paths = Vec::new();
}
danyspin97 commented 1 year ago

I'll be honest and say this is the first time I'm even looking at code for a daemon, so I'd say you definitely have a better idea of what should be done haha. I'll have a look at a reload command, you mean something like wpaperctl reload or how should it be called?

Everyone has its own way of making daemons, more or less dynamic and user friendly. Personally, I try to make wpaperd as seamless and dynamic as possible; this include reloading the config on the fly and picking up changes in the wallpaper directory. I feel like adding a cache vector and a reload command would stray a bit from this goal without adding much value. Let me explain in detail.

Walkdir is designed to be fast, so iterating over 1k-10k shouldn't take more than a couple of milliseconds (everything under 100ms is still acceptable imho). Even if it done in row for multiple monitors, Linux will already have cached all the file descriptors for the same directory. The new wallpaper is shown only after this procedure has ended, without causing any interruption to the user.

I made these considerations after our discussion yesterday about the reload command :)

Rolv-Apneseth commented 1 year ago

I see, so then how would you suggest proceeding? Should I check every cached file still exists before using it or do you have something else in mind?