search

emersion / xdg-desktop-portal-wlr

xdg-desktop-portal backend for wlroots
MIT License
595 stars 59 forks source link

Add support for config file #75

Closed columbarius closed 3 years ago

columbarius commented 3 years ago

Resolves #60

Note: while this is not merged and man pages are not ready, the locations for a config file are in order:

The system wide config can be in another place, if SYSCONFDIR differs from /etc

danshick commented 3 years ago

Nice. What should the syntax be for a command to determine the output instead of a fixed output string? If the config had both, which would take precedence?

I don't think it is important to implement arbitrary output command handling now, but i think we should have a plan for the syntax and behavior.

columbarius commented 3 years ago

Nice. What should the syntax be for a command to determine the output instead of a fixed output string? If the config had both, which would take precedence?

I think that depends, if we want to have a set of hardcoded default choosers or not. If we have one, we need a way to hard override the chooser with a given monitor. If you could disable all choosers (like adding an empty chooser entry) it would be irrelevant, if you first skip the choosers and then take the set monitor or choose the set monitor in the first place. So i would let the hardcoded output be priority.

I don't think it is important to implement arbitrary output command handling now, but i think we should have a plan for the syntax and behavior.

For an arbitrary output chooser, we would just need two options in the file. chooser_cmd and chooser_type. Those would override the hardcoded ones. Question is, if we want to fail screencast, if no monitor is selected, or if we still pick the "first" one?

I could rebase #59 onto this MR.

danshick commented 3 years ago

I could rebase #59 onto this MR.

I was thinking of landing this one first, then going back to #59 and just adding the needed config entries.

My initial thoughts for config would be

chooser_type = <first | fixed | command>

chooser_cmd = <string representation of command to run, should accept list of outputs on stdin and output the choice on stdout, ignored for all types but command>

chooser_value = <string representation of output string, ignores for all types other than fixed>

I wonder if we should define an ini section for these settings. I think it makes sense to fall back to first if nothing is specified or a chooser fails.

columbarius commented 3 years ago

I added my thought on how to config the chooser in #59. We could adapt it in a way that type=none would use output_name or chooser_value to select a monitor directly.

columbarius commented 3 years ago

I would also look at /etc/xdg/xdg-desktop-portal-wlr/{DESKTOP,config} before /etc/xdg-desktop-portal-wlr/... for a config to be compliant with xdg spec. @danshick @emersion is there still something to work on, or can I squash and clean everything up for review?

danshick commented 3 years ago

I would also look at /etc/xdg/xdg-desktop-portal-wlr/{DESKTOP,config} before /etc/xdg-desktop-portal-wlr/... for a config to be compliant with xdg spec.

Makes sense.

@danshick @emersion is there still something to work on, or can I squash and clean everything up for review?

Nope, looks good to me.

columbarius commented 3 years ago

@emersion I think I addressed all open points except the nested structs of the config struct. I like it more that way, but if you want, I can flatten them. Ready for a new review, if you have time.

columbarius commented 3 years ago

rebased and squashed the commits for cleanup

oranenj commented 3 years ago

As a user testing this out, my primary gripe with this PR is that it needs documentation. I had to run xdpw with tracelogging on to figure out what files it actually tries to load. :/ A short manual page would be ideal.

Also, could reasonable defaults be hardcoded into the system? Like, if there's something to choose, use slurp automatically unless configured otherwise, and if it fails, default to picking the first display? I generally prefer software to do something sane without having to configure it.

columbarius commented 3 years ago

@oranenj Added a note about the locations at the description of this PR. Documentation will either follow in the wiki or with #67 as soon as this is merged. Thanks for pointing that out. I didn't realize, that those MR's are widely used.

I'm also preferring sane defaults and if you don't have a config file the output chooser should try 3 different choosers (I admit, we could differentiate if the screencast was declined, or if the chooser was not installed), but please feel free to suggest your idea at #59.

danshick commented 3 years ago

@oranenj

Re: documentation, man pages are planned. I've been working on them for what feels like a decade, but I keep getting side tracked. They'll come out soon and we will update them with this PR if they land first, and they will include this PR if they land second. Also, this PR will likely get merged with the output selection PR, so both will need to be ready before either is stabilized. In the future, updating docs should be a criteria for accepted PRs, but I doubt docs will ever get written before something is ready to be merged.

I like sane defaults as well, but I'm not a big fan of hardcoding the names of other programs into a codebase. We've already got a hard dependency on grim (for screenshots), so slurp doesn't feel like a big stretch. I'm very wary of adding any others, but I might be in the minority on this one. I think we will always gracefully fall back to the first display and have a CLI flag that takes precedence over any chooser, just in case.

oranenj commented 3 years ago

@danshick I totally agree with you on dependencies. In this case though, slurp wouldn't be as much a dependency as an extremely convenient integration, and since it's sort of part of the "wlroots ecosystem" it feels justified. If you don't feel like hardcoding it in C, one option might be to ship a "default configuration" in /usr/share somewhere that's not meant to be edited (and will be overwritten by package managers) which will be used as a last resort.

danshick commented 3 years ago

one option might be to ship a "default configuration" in /usr/share somewhere that's not meant to be edited (and will be overwritten by package managers) which will be used as a last resort.

I like this idea a lot. Also lets different distros ship a custom default config if they so choose.

What does everyone else think? @emersion @columbarius?

columbarius commented 3 years ago

Depends... if we make slurp (with the specific commit) a hard dependency, we could ship a default config in /user/share with an slurp entry. If not, we would ship an almost empty config file such that xdpw falls back to the default path. That way, I would not ship a config, but try to create the most sane default possible and rely on the distribution to ship something in etc reflecting their choice.

oranenj commented 3 years ago

I might be a good idea to nudge distributions towards using the defaults from upstream. Otherwise, you might end up having to support users using very different configurations just because distros happen to pick whatever they think is best at the moment of packaging if upstream has no opinion on the matter.

columbarius commented 3 years ago

Since we are writing stuff for a library which could be used to write window managers and desktop environments i don't think there is something like a good upstream default, since we can assume very little as preinstalled. With a config file, we can only support one output chooser, while currently we have three assumptions as default, if the config has no configured chooser. Afaics we have the options:

columbarius commented 3 years ago

Thanks for the squash and merge!