Allow all glob patterns in output configs

[pedrocr]

Instead of special casing output * ... just make output take all glob patterns. This is useful if you want to change a config in all your HDMI or DP outputs, or want to match any monitor of a given model.

This is implemented by just storing all output commands unaltered and then matching the glob patterns to the outputs whenever needed. Match order is the same as command introduction order so that any newer commands are always respected.

Code becomes significantly simpler by doing it this way, with a bunch of complex matching logic across several functions being replaced with a straightforward merge sequence in find_output_config(sway_output).

Fixes #5632

[pedrocr]

At this point this PR is really a refactor of the code that then allows glob matching to be done easily. The old behavior could be brought back just by changing the line that does the two fnmatch() calls into three strcmp() calls. I can put the introduction of glob matching into a separate commit at the end if it makes more sense in the commit log.

[pedrocr]

I've split out the patch series as I've found that the initial refactor fixes at least one bug with the existing code, unrelated to the matching itself.

[pedrocr]

I've been running this locally with no issues. Any comments on it?

[RedSoxFan]

I'm not sure how I feel about this as a whole, but here are some comments.

Something to consider is swaybg (as well as anything that is compatible with swaybg_command like oguri). We build a command based on the output configs. swaybg currently does not support globbing and would fail to correctly set the wallpaper. Additionally, the output command is unique in that it allows for multiple subcommands to be provided on the same line (for backwards compatibility). This may result in multiple output configs having a wallpaper for the same output glob pattern, which would result in the output being specified multiple times in the swaybg spawn command.

[pedrocr]

I hadn't looked at swaybg_command but if I'm understanding things correctly it's easy to solve with full backwards compatibility. Currently spawn_swaybg() is iterating over the output_configs and building the command from that. Instead it should iterate over the outputs, call find_output_config() with each of them to get the background options in effect, and add those to the command line with the output name. No globbing support is needed at all. How does that sound?

[pedrocr]

Thanks for the review @RedSoxFan. Let me know if you think these two remaining things are ok and I'll do the swaybg_command changes as well if so.

[RedSoxFan]

I hadn't looked at swaybg_command but if I'm understanding things correctly it's easy to solve with full backwards compatibility. Currently spawn_swaybg() is iterating over the output_configs and building the command from that. Instead it should iterate over the outputs, call find_output_config() with each of them to get the background options in effect, and add those to the command line with the output name. No globbing support is needed at all. How does that sound?

That would require also restarting swaybg everytime an output is connected or disconnected. Currently, we only restart it when there would be a change. I think this would just make #3693 more visible.

[pedrocr]

I've implemented the swaybg change with the restart on any output config change. Is the restart problem that swaybg doesn't even need to be restarted if the resolution changes because it can adapt by itself? It should only be restarted if we're altering the command line commands it receives?

[emersion]

It should only be restarted if we're altering the command line commands it receives?

Correct.

[pedrocr]

The simplest way I can think of to do that is to keep the command line between calls and just do a no-op if it's the same. It would mirror what happens on mode change with wlroots turning a mode set into a no-op. Seems simple to implement too.

[pedrocr]

I've implemented the check that the command is not the same, and confirmed that it eliminates the restarts when the background config hasn't changed. I think that takes care of the last issue discussed, but maybe there are other doubts about this.

[RedSoxFan]

I'm not a huge fan of this PR as a whole.

1. I'm not convinced that full glob matching is actually needed.
2. I realize it will have no noticeable impact for most, but this does result in higher memory usage due to each (non shadowed) delta being stored long term as well as the swaybg command.
3. Yes, wlroots will prevent output commits without any effect from touching the outputs, but it is still unnecessary to process the output configs of every single output any time that any output config changes or any output is hotplugged.
4. Similar to the previous point, regenerating and comparing the swaybg command every single output config change or hotplug is also additional computation that will be unnecessary in most cases.
5. As mentioned above, having a swaybg command that has to be regenerated for hotplugging will make When setting a wallpaper image, the background switches to gray and then to the image #3693 more noticeable. If I had to guess, the most common use case is probably output * bg </path/to/wallpaper> <mode>. This does not require any knowledge of the active outputs to pass this information off to swaybg. Depending on the output, it's possible that the two cycles occur before the output is even presenting anything to the user so they don't even see the default background.

If others disagree with me, I won't block the PR from being merged (and will clear my requested changes review), but I doubt this will be getting an approval from me.

@emersion I'm curious what your thoughts are on this PR

[pedrocr]

I'll try to answer the objections if it helps with the discussion, but I can also close the PR if this is not really wanted.

I'm not convinced that full glob matching is actually needed.

My use case is for applying the same config to several screens of the same make and model. It's a common situation in hot-seat offices that all have the same monitors. That specific use case could potentially be implemented by itself by adding make/model match but I can see people wanting to do things like set variable refresh on all DisplayPort outputs and other conveniences. I agree this isn't a must have feature though, so if it has many other downsides it can just be scrapped.

I realize it will have no noticeable impact for most, but this does result in higher memory usage due to each (non shadowed) delta being stored long term as well as the swaybg command.

From a quick calculation it seems to be less than 1000 bytes per output in the absolute worst case scenario. It can also result in less memory when you can use a glob whereas before several configs are needed. I don't think there's any platform sway is interested where it makes a difference either way.

Yes, wlroots will prevent output commits without any effect from touching the outputs, but it is still unnecessary to process the output configs of every single output any time that any output config changes or any output is hotplugged.
Similar to the previous point, regenerating and comparing the swaybg command every single output config change or hotplug is also additional computation that will be unnecessary in most cases.

There's a bit of extra calculation. If the processing cost was too high caching could be done relatively easily but there's no calculation that's particularly expensive here, so the simplicity seems like a win.

As mentioned above, having a swaybg command that has to be regenerated for hotplugging will make #3693 more noticeable. If I had to guess, the most common use case is probably output * bg </path/to/wallpaper> . This does not require any knowledge of the active outputs to pass this information off to swaybg. Depending on the output, it's possible that the two cycles occur before the output is even presenting anything to the user so they don't even see the default background.

This should allow less cases of showing #3693 actually. It only ever restarts swaybg if it needs to be restarted because the command line has changed. With the existing code if you issue the exact same background command over swaymsg the background flickers even though nothing has changed. The output * bg case could be special cased if that's wanted.

As I said I'm happy to just close this if there's no interest. The globbing itself I've found helpful but the PR has ended up reworking the code. Maybe too much? It's less code that to me seems simpler with at least one bug fixed.

[pedrocr]

Just noticed that the same feature request and equivalent PR also exists for kanshi:

emersion/kanshi#72
emersion/kanshi#73

[pedrocr]

I've been running this for nearly 6 months with no issues now. Is there anything else I can help with in the discussion?

[emersion]

As I stated elsewhere, I'm still not convinced globs are a good feature to have for output configs.

[pedrocr]

@emersion hadn't seen your opinion on that then. I was asking because both this and the kanshi PRs have been left open but without any further discussion. Feel free to close this if you don't want the feature then. I'll have to make do without it.

[emersion]

Ref emersion/kanshi#72

[pedrocr]

To be able to implement the glob patterns I had to refactor the code and fixed at least one bug in the process. The glob support is a single line change after that. If it would help I can submit a PR that doesn't change functionality at all and then make the glob pattern support a trivial PR on top of that.

[emersion]

Sure, feel free to split the refactoring part up if it makes sense to merge it standalone. Disclaimer, I haven't looked again at the refactoring yet.

[pedrocr]

The fnmatch() change is a one liner either way (plus documentation) so the PR is already representative of what that would be.

[WhyNotHugo]

I'd also really appreciate this feature. A bit of background:

My monitor changes id depending on which ports I use on my laptop and the monitor, so without globbing, I need to keep all permutations of the ids around. Any time I make a change, I usually have to change all of them too.

There are around 10 desks at my office all which all have monitors of the same model, so glob matching would allow one line to adapt all 10 of them. So far, I've to reconfigure sway each time I use a new one, and keep around a pile of identifiers in my config.

I understand the desire for optimisation, but 1kb memory and a few string comparisons when there's an output change seems a bit extreme; I'm sure there's much juicier targets for optimisations.

[h-2]

I really need this feature!

In my shared office, the screens have individual IDs, so I need to configure each one individually, which is super annoying, e.g.:

output 'LG Electronics LG ULTRAFINE 308MALFA5952' {
...
}
output 'LG Electronics LG ULTRAFINE 211MAFCQ5123' {
...
}
...

I would like to be able to do just:

output 'LG Electronics LG ULTRAFINE *' {
}

[josecastillolema]

I'd also really appreciate this feature. Shared office, lots of individual IDs in the screens.
Requested in #8258

[hw0lff]

I just stumbled upon this thread.
I too have shared offices spaces and had the need for a more advanced output matching.
I used kanshi for a while but it didn't offer the functionality I needed (to manage >30 profiles deterministically).

I've pondered about a complete solution over the last 2 years.
Globbing cannot and will never be able to describe all monitor configurations due to being to unspecific in certain cases.
A single regex alone will not be enough either since it could match to much.
A complete solution must be able to distinguish between several (read: all) attributes of a monitor.

I came up with a matching procedure that allows full text matching, substring comparison and regex matching for every monitor attribute individually.
It needed a separate syntax to be able to describe an output configuration completely.
With that procedure it is possible to express every output configuration.

However, I don't think this procedure belongs into sways codebase for several reasons.

1. It would probably break i3 compatibility.
2. The code that implements this will be very complex¹.
   It needs an extended version of a full cardinality graph matching algorithm.
   It gets more complex when specifying modes add constraints to the procedure.
3. I would prefer to keep sway simple rather to add this complexity.
   The current way of configuring outputs with sway is simple.
   simple to use, simple to understand. This is good.
   For most people the simple way is enough to get around.
4. This advanced matching feature can be achieved with an external program using the wlroots output-management protocol.
   It won't add a maintenance burden to the sway maintainers and it can be used with every other wlroots compositor.

P.S.:
This complete matching procedure is already implemented in shikane.
For anyone interested, the custom syntax can be found in the manual under the search field.

Reading through this myself, it may seem like a self-promotion at first glance.
However, it is meant as a kind warning that the globbing support of this PR will not cover all configuration needs.
I've racked my brain way too much about this topic and abandonded the idea of relying solely on extended globbing a long time ago.
It will only push the threshold of perceptibility of the underlying problem further back.

Footnotes

1. my implementation has ~2kloc for the procedure alone ↩

[hw0lff]

The issue #6410 is a perfect example of what I meant with the shift of perceptibility of the underlying problem.
This globbing PR and #6410 talk about different ways to find a solution to their specific use cases.

When we look at them more broadly then both are about advanced output matching.
However, both see only their specific subset of the problem.
Implementing one (e.g. the glob match) would be just reducing the set of affected people.
It'd shift the focus on the "better & advanced output matching" feature further away.
When I said "affected people", I mean affected by the lack of advanced output matching.

What about matching a serial number?
What if any output should be chosen if the supplied mode can be applied?
What if an output should be chosen only if the supplied mode fits && the vendor matches to a given regex?
What about this weird mode which is @ 50,863Hz? Will the decimals be truncated or rounded when matching?

A complete solution can handle all of the above.

[pedrocr]

The part of this PR that actually implements glob patterns is a single line. The more extensive changes were needed to replace the complex and error prone config merge code with a much simpler method of just applying all the rules all the time in the order the user specified them. That's how at least one bug is fixed by this. I've already offered to do a PR without the actual glob pattern single line change if that makes it easier. It would fix at least one bug and not have any functionality changes. The additional memory overhead is minimal.

Since the glob pattern change is then a one-liner match difference I'd still have that over more complex stuff. It doesn't cover all cases but covers quite a few that are useful, most commonly the "office full of identical monitors with different serial numbers" setup.

[WhyNotHugo]

In principle, I agree with approaches like kanshi, swww or shikane, where an external client takes care of output-specific configuration. There's no need for the compositor to handle all this itself.

In practice, the main issue is that these clients start after the compositor, so the compositor renders first with the default scale/resolution/rotation/etc and then the external client applies the correct settings.

I can't think of a sensible way to work around this. The only thing that comes to mind is telling sway to delay rendering until a client has explicitly configured outputs... but honestly, this sounds like it could have a million edge cases where sway becomes unusable.