anufrievroman / waypaper

GUI wallpaper manager for Wayland and Xorg Linux systems
https://anufrievroman.gitbook.io/waypaper
GNU General Public License v3.0
396 stars 27 forks source link

Overwriting of configured monitor options with "all" in state files #89

Closed darth62969 closed 2 weeks ago

darth62969 commented 1 month ago

So I've been debugging something and I have figured out that any time I try to use a state file, and configure it to load an image to a specific monitor, Waypaper overwrites the monitor i have selected in the state file.

So the state file for my main monitor is as follows:

[State]
folder = ~/Pictures/(wallpaperfolder)
monitors = DP-1
wallpaper = ~/Pictures/(wallpaperfolder)/(image) 

It Becomes:

[State]
folder = ~/Pictures/(wallpaper_folder)
monitors = All
wallpaper = ~/Pictures/(wallpaper_folder)/(image) 

or for my vertical monitor

[State]
folder = ~/Pictures/(vertical_wallpaper_folder)
monitors = DP-2
wallpaper = ~/Pictures/(vertical_wallpaper_folder)/(image) 

Which becomes

[State]
folder = ~/Pictures/(vertical_wallpaper_folder)
monitors = All
wallpaper = ~/Pictures/(vertical_wallpaper_folder)/(image) 

this defeats my script and what i'm trying to do with my monitors... having a different image on each monitor is important to me, and having vertical images for the vertical monitor is very much appreciated.

my script is as follows:

#!/bin/bash

while true; do
    waypaper --random --state-file dp1.ini
    waypaper --random --state-file dp2.ini
    waypaper --random --state-file hdmi1.ini
    sleep 300
done

I've tracked down the issue to: https://github.com/anufrievroman/waypaper/blob/020cd50f7b97f86737b59ff8e49a203baf6cbd8a/waypaper/config.py#L214

which calls the function: https://github.com/anufrievroman/waypaper/blob/020cd50f7b97f86737b59ff8e49a203baf6cbd8a/waypaper/config.py#L156

the issue is that since the code never changes this variable in the config object when running from the terminal, it just overwrites what is in the config file, instead of saving that monitors configuration to that file. https://github.com/anufrievroman/waypaper/blob/020cd50f7b97f86737b59ff8e49a203baf6cbd8a/waypaper/config.py#L24

When using the gui, it works fine, just when you are using state files it does not.

I'm using hyprpaper, and 3 monitors i want to individually control.

What's the strategy for dealing with this issue? Is this an intended behavior, and if it is can we work to change it?

anufrievroman commented 1 month ago

I think the problem might be that you are trying to set wallpaper three times with three state files, but I think you need to put all three into one state file, comma separated. I can't test it right now, but try: monitors = dp1,dp2,hdmi1

darth62969 commented 1 month ago

So I tried your suggestion, it changed all the wallpapers, but still overwrote the monitor section of the state file with all. It also removed the specific wallpapers in the file and only kept the last one it saved.

Also I've been looking at the code, it doesn't seem like I can load a specific file folder for a specific monitor in a single state file. It does seem like this is unintended behavior based on how you've addressed the issue, but I don't want to try to make a PR with a fix if it is intended to behave in this way.

I want to be able to run a script to change the wallpaper per screen and use a different folder for my vertical monitor, so instead of standard landscape wallpapers, it would have only portrait wallpapers in it. that's the main reason why I split up the state files. so i can change each monitor's wallpaper individually.

anufrievroman commented 1 month ago

Okay, maybe I misunderstood the problem. I think you're right that attribute_selected_wallpaper() writes All as the monitor because it bases its choice on selected_monitor which is All by default and does not get a value unless a GUI is run. So, I think multimonitor support with cli user arguments is quite broken. I can work on it right now, but I'll think about this behavior...

darth62969 commented 1 month ago

I think the end solution is going to be to index the monitors in the state files and then keep them as is. I think that #90 is a first step and possibly a temp fix. Without changing too much code as I'm a bit shy of trying to do anything drastic.

anufrievroman commented 1 month ago

Thank you for PR and first fix! Okay, so is it correct that this fix allows you to use your multi state file script as intended, but the problem of randomization on multimonitors remains?

darth62969 commented 1 month ago

it allows me to have the randomization on multi monitors with the the script. Fixes my problem specifically. Should allow others to do similar things with their systems through the use of state files but, i do feel there is more to discuss on how to properly fix this, right now the fix will not allow someone to run 2 monitors off one state file or use one file in general to have different wallpapers on different monitors. so that's to be considered.

anufrievroman commented 1 month ago

Yes, as I look into the code, I see how the things are getting messy as we run the cli options, especially --random, but I don't currently see a fast way to fix that, I'll try to find to work on it.

anufrievroman commented 3 weeks ago

Hi, with the recent commit, I tried to fix the whole situation with random and saving. Now, --random randomizes the wallpaper for monitors specified in the config. I didn't test with the state file, but with regular config it seems to work. I think with this you can also use the same state file for all monitors. Could you please try the current version in the main branch?

darth62969 commented 3 weeks ago

Grabbed the latest commit, i noticed 2 things. With state files, It does seem to want to set all the wallpapers at once regardless of if there is a specific monitor desired. And when you do set all the wallpapers to random images, what ends up happening when you run waypaper --random it first changes all the monitors to 1 wallpaper then goes through and changes each individually. I'll see if I can look into both behaviors. I'll update my theme with the config i was using for my wallpaper setup so you can look at it. My theme Repo: https://github.com/darth62969/BeyondTheWired_Theme

anufrievroman commented 3 weeks ago

Interesting. A couple of questions:

With state files, It does seem to want to set all the wallpapers at once regardless of if there is a specific monitor desired.

As I see it, there should be no difference between using regular config or state files. Do you see a difference?

And when you do set all the wallpapers to random images, what ends up happening when you run waypaper --random it first changes all the monitors to 1 wallpaper then goes through and changes each individually.

I think that can only happen if in config/state file it's written monitors = All,DP-1..., is it not the case?

I'll try to take a look at you setup as well, but at a glance, the problem might be that you have only one wallpaper, but three monitors. I think the program expects lists of equal length, even if you want the same wallpaper, it should be repeated three times then.

darth62969 commented 3 weeks ago

I think that can only happen if in config/state file it's written monitors = All,DP-1..., is it not the case?

According to my config, it seems that i do not have "all" listed in the general config. Ah the default state file has "ALL" in it. So that's something to note.

As I see it, there should be no difference between using regular config or state files. Do you see a difference?

Seems to be working now. I don't know what was happening prior but, it does seem to be working as intended after I retested some things.

So instead of 3 state files I can use 2, which is good.

The only other issue I've run into overall is sometimes I just have to reload the cache to get things to cycle again, but... that might be because I have thousands of wallpapers in my rotation. plus that seems like an impossible to debug issue as... it's quite random and not really reproducible outside of "sit and wait."

anufrievroman commented 2 weeks ago

Great! Glad that it works. The default "All" is there to prevent the situation when someone sets only some of the monitors and others just have no wallpaper.

About the rotation in random, in your waypaper cache directory, there is text file that's keeping track of the wallpapers that were already used, so as to not repeated wallpapers. After all wallpapers have been used once, it's supposed to reset that file. Likewise, when you "Refresh" it deletes all the cache. But now I realized that I didn't actually test if that automatic reset is properly happening. So, if it feels like there's something wrong with the rotation during random, it might be a new issue. I'll try to test on occasion.

anufrievroman commented 2 weeks ago

Okay, I could confirm the issue with cycling through wallpapers, so I opened a separate issue for this #96 So I'll close this issue as complete. Thanks for reporting!