Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

daemon/config: Use regex to match info #108

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ein-shved
Copy link

Wayland may report an output description in different formats as the compositors wants to. For example, niri reports description in format <vendor name> - <monitor name> - <port name> so, we can not handle here the all formats at once. But users can use a regex to match theirs' compositors outputs.

Wayland may report an output description in different
formats as the compositors wants to. For example, niri reports
description in format `<vendor name> - <monitor name> - <port name>`
so, we can not handle here the all formats at once. But users
can use a regex to match theirs' compositors outputs.

Change-Id: I50e8de92e38dc23a124b8e7421cc9f456cc8cc41
if !k.starts_with("re:") {
continue;
}
// TODO(Shvedov): Better to compile re withing creation of

Check notice

Code scanning / devskim

A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note

Suspicious comment
@danyspin97
Copy link
Owner

Hi @ein-shved and thank you for the PR!

Wayland may report an output description in different formats as the compositors wants to

I wasn't aware of this and it is not good at all.

The PR looks good to me at a first glance. Can I ask more about the use case? I.e. how are you using it?

@ein-shved
Copy link
Author

ein-shved commented Dec 11, 2024

I wasn't aware of this and it is not good at all.

To be honest, I am not sure in this too, and I mad this conclusion based on debugging my problem.

Can I ask more about the use case? I.e. how are you using it?

There is actually long history.

Several month ago, I started switch to wayland with hyprland with its ecosystem (including hyprpaper as wallpaper). I use two setups: for my home (and mobile) office and for employer office. In employer office I have stationary PC with stationary connected displays, so I configured wallpapers for both based on ports here. But in home I have laptop and I can plug different monitors to the same port and plug same monitor to different ports. So I have default setup (for all monitors and future) and per-monitor-name setup for exacly my office at home here.

At home I have monitor named firstly XMI Mi Monitor 0000000000000. Later suddenly it changed its name to Xiaomi Corporation Mi Monitor 0000000000000 after system upgrade, so I have to update my configuration.

At last I switched to niri, and both of this names did not work, but the niri-configuration (with positioning works well with one othis names, but does not work for wpaperd, hyprpaper and hyprlock. After debugging I found out that description of my monitor is Xiaomi Corporation - Mi Monitor - DP-1.

First of workaround I came up with - is to add this patch and place re:Mi Monitor to configuration. Another - is to place Xiaomi Corporation - Mi Monitor - DP-1 to configuration. Actually, I end up with second, because this is works for all other utilities. But this patch may be useful for cases of changing the description formats after updating, or switching to different compositors.

@danyspin97
Copy link
Owner

Thanks for the explanation, it brings more detail about the description given by the compositor and the various wpaped use cases :). There are a couple of things I wanna discuss though:

Would be worth to check how niri handles the description? There might be some code that translates the data sent from wayland to a description that matches the one form Hyprland.

The second things is, would a partial match have the same result? I.e. match the wpaperd configuration for a display with XIAOMI instead of the full description string. This could result in a simplified usage and a feature out of the box for everybody. On the other hand the regex are more powerful, but require the users being aware of the feature and adding re: at the start of their string. If the partial match would be a good idea, would it cover all the use cases as the regex do?

@ein-shved
Copy link
Author

Would be worth to check how niri handles the description

Of course it would be! I could handle this, because I have plans to inject some updates to niri. But I cannot tell when though.

If the partial match would be a good idea, would it cover all the use cases as the regex do?

I believe that there is a possible setup with two monitors with names Vendor display and Vendor display v2. In such cases, partial match will break.

@danyspin97
Copy link
Owner

Of course it would be! I could handle this, because I have plans to inject some updates to niri. But I cannot tell when though.

That would be great, thank you!

I believe that there is a possible setup with two monitors with names Vendor display and Vendor display v2. In such cases, partial match will break.

Can you please elaborate more?

Anyway, after this discussion I am also planning to add partial matching, just to add a simpler mechanism alongside regex.

@ein-shved
Copy link
Author

Can you please elaborate more?

Say, user have config like next

['Vendor display"]
path = "/path/to/wp1.png"

["Vendor display v2"]
path = "/path/to/wp2.png"

If you will add partial matching, users with such configuration will find wp1.png wallpaper on both monitors after update, while they are expects wp2.png on Vendor display v2 output. And they will not have any workaround for such situation (maybe reorder configuration entries, but this will be unstable workaround).
With regexp they are able to use borders like ^Vendor display$.

@danyspin97
Copy link
Owner

That's a good point. As a workaround, we could iterate over all entries and select the one that is the longest matching, which would be a deterministic and straightforward approach from a user point of view. It takes a little bit more of code, but that shouldn't be an issue.

I still think that the regex are more powerful and a good addition. I am also thinking how should partial matching and regex work together in this case, maybe partial matching should have priority over regex when checking the config?

@ein-shved
Copy link
Author

ein-shved commented Dec 19, 2024

maybe partial matching should have priority over regex when checking the config?

I think, this is too complex logic, you have to write huge documentation on the process of matching which may not be understood correctly. Still simple partial patching is a really good option for many cases. So I think it is better to say:

When you have several entries which matched 
your output - the behaviour is undetermined. 
If you are faced with such a situation - 
you should use regexp.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants