Add (virtual) --desktop option, similar to --screen

[frno7]

The --desktop option updates the wallpaper when the X property _NET_CURRENT_DESKTOP changes, which happens a (virtual) desktop is switched. Example:

xwallpaper --daemon					\
	--output all --desktop 0 --zoom wallpaper0.jpeg	\
	--output all --desktop 1 --zoom wallpaper1.jpeg	\
	--output all --desktop 2 --zoom wallpaper2.jpeg	\
	--output all --desktop 3 --zoom wallpaper3.jpeg

The xprop command can be used to inspect _NET_CURRENT_DESKTOP:

xprop -root _NET_CURRENT_DESKTOP

[frno7]

This patch is a proof-of-concept, still in need of some refinements. :-) For example, it would be nice to omit --output all for every --desktop option. And have some --desktop default option, which can be overridden for specific desktops.

Also, the --daemon option is now useful without RandR.

[stoeckmann]

Looks like a reasonable and in-scope addition to xwallpaper! From a very quick first view, the changes look also good. Let's see how to merge the feature in a way as you described to play along with all the others.

[stoeckmann]

Do you have an idea which earliest version supports the changes you propose?

[frno7]

I’ve tested xcb/util-wm 0.4.2 with the Gentoo package x11-libs/xcb-util-wm. It’s apparently about 3 years old. Looking at git diff 0.4.1..xcb-util-wm-0.4.2, it most likely works with 0.4.1 too, which is about 12 years old.

[frno7]

This static array of ints is a bit of a hack, by the way!

[stoeckmann]

Yeah, it would be best to compare it to root window, I think. But I didn't want to start nagging at it without experimenting with it on my own.

[frno7]

Looks like a reasonable and in-scope addition to xwallpaper! From a very quick first view, the changes look also good. Let's see how to merge the feature in a way as you described to play along with all the others.

Cool! Given that some refactoring (most likely) is needed to improve the usability of the --desktop option, I thought it would be best to publish this draft before getting carried away.

Also, I have some ideas on managing user preferences with the standard X resource database, which is much more convenient than command arguments. Something like:

Xwallpaper*mode = zoom
Xwallpaper*desktop.0.filename = wallpaper0.jpeg
Xwallpaper*desktop.1.filename = wallpaper1.jpeg
Xwallpaper*desktop.2.filename = wallpaper2.jpeg
Xwallpaper*desktop.3.filename = wallpaper3.jpeg

Maybe one could support things like:

Xwallpaper.output.HDMI-1.screen.3.desktop.7.filename = wallpaper.jpeg
...

This would be especially cool XCB_PROPERTY_NOTICE, because the wallpaper could be updated dynamically by using xprop -root -set or xrdb -merge, without needing pkill -HUP xwallpaper or similar hacks. :-)

[stoeckmann]

Also, I have some ideas on managing user preferences with the standard X resource database, which is much more convenient than command arguments.

We would have to see if it covers what xwallpaper also covers with command lines. Its CLI tries to resemble xrandr which has no such resources. But one could argue that xrandr also has no daemon mode.

First things first, let's see how we progress with --desktop.

[frno7]

It’s now possible to omit --output, and also use the default keyword, as in for example

xwallpaper --daemon					\
	--desktop default --zoom wallpaper0.jpeg	\
	--desktop 3       --zoom wallpaper3.jpeg	\
	--desktop 5       --zoom wallpaper5.jpeg

Ready for review?

[stoeckmann]

I've added remarks and hope that the comments about the hierarchy make sense. Let me know if something's not clear or if you disagree with the reasoning!

[stoeckmann]

Let's start with the least important one: Let's keep the headers sorted, so this should be one line up. :D

[stoeckmann]

I add the comment here: This breaks xwallpaper --output Virtual-1 --zoom file.jpg --output Virtual-2 --center file.jpg because only the first entry for screen 0 is used. There will be more than just one configuration per screen.

[stoeckmann]

And this comment should cover the whole screen[MAX_SCREENS] array. It would be much better to retrieve which desktop changed from the event (_NET_CURRENT_DESKTOP). This also means that only events for this parameter should be evaluated, not everything that changes some kind of property. This avoids the introduction of a static array and keeping track of desktops of screens.

[stoeckmann]

We should check if randr or desktops are specified. If both do not exist, --daemon still becomes useless and the mode should be avoided.

[stoeckmann]

Centralized check in main during runtime in #59

[stoeckmann]

It should also reset last.output. Rational about my thought:

• Screens include outputs (which is a given hierarchy noticeable when working with XCB)
• Screens include desktops (also a given through XCB)

I think that it's more intuitive to use the hierarchy screen -> desktop -> output from a user experience perspective. With that in mind, let's analyse the following example:

xwallpaper --screen 0 \
    --desktop 0 --output LVMS-1 --zoom file1.png --output LVMS-2 --zoom file2.png \
    --desktop 1 --output LVMS-1 --zoom file3.png --output LVMS-2 --zoom file4.png

This would indicate that I am approaching the wallpaper setup by looking at the virtual desktop first, then at the outputs. Compared to this one, which would use the hierarchy screen -> output -> desktop:

xwallpaper --screen 0 \
    --output LVMS-1 --desktop 0 --zoom file1.png --desktop 1 --zoom file3.png \
    --output LVMS-2 --desktop 1 --zoom file2.png --desktop 1 --zoom file4.png

With that in mind, it means that whenever --desktop is encountered, any currently set output must be reset. Granted, this should have been done for screen as well, i.e. whenever --screen is encountered, output and desktop must be reset, but it slipped through with outputs until now (topic for its own PR).

[stoeckmann]

The --screen issue is fixed with #56

[stoeckmann]

We could also use all which would be pretty much treated as if no desktop was given. Another example to illustrate what I mean:

xwallpaper --screen 0 \
    --zoom file1.png \
    --desktop 2 file2.png

Specifying outputs before any --desktop is given, ultimately has to be treated as "for every desktop." So this is an implicit default/all. But I don't mind if the all keyword is added.

[stoeckmann]

What should be made clear in the manual page (and maybe example) is that specifying --desktop without --daemon does not mean that the wallpaper will be set if someone switches to the desktop afterwards. It acts more like a filter, i.e. the xwallpaper setting does only take effect if the current desktop matches the specified one. For --daemon, it's pretty much what everyone expects to see.

[stoeckmann]

And last weird comment position: We have process_screen which calls process_output. Given the idea about screen->desktop->output hierarchy, it might be best to introduce process_desktop, i.e. process_screen calls process_desktop calls process_output.

[stoeckmann]

Note: There won't be a single best option. As I've mentioned the output bug before, this could also hurt the desktop idea:
xwallpaper --desktop default --output LVMS-1 --zoom file1.png --output LVMS-2 --desktop 2 --zoom file2.png
Intuitively, I would expect that LVMS-1 always shows file1.png, regardless on which desktop. But if I am on desktop 2, the LVMS-2 output should additionaly show file2.png.

Since output LVMS-2 is not specified in default, it would be interesting to know what people expect would happen with LVMS-2 when switching back to another desktop, but --clear would make that obvious. :)

[frno7]

I've added remarks and hope that the comments about the hierarchy make sense. Let me know if something's not clear or if you disagree with the reasoning!

Many thanks! No disagreements, but it seems I don’t quite understand the proper relation between screens and outputs (I never use multi-monitor setups myself, only virtual desktops). By your comments, you seem to have a fairly concrete idea on how to proceed? Would you like to give it a go yourself? :-)

We would have to see if it covers what xwallpaper also covers with command lines. Its CLI tries to resemble xrandr which has no such resources. But one could argue that xrandr also has no daemon mode.

First things first, let's see how we progress with --desktop.

Sure! As to the hierarchy (which would be good for a section in the manual page, by the way), I suppose a fully qualified path of components would now become something like:

Xwallpaper.screen.3.desktop.7.output.hdmi-1.filename = wallpaper.jpeg

Shortened example for anyone who only cares about desktops:

Xwallpaper*mode = zoom
Xwallpaper*desktop.?*filename = wallpaper-default.jpeg
Xwallpaper*desktop.2*filename = wallpaper2.jpeg
Xwallpaper*desktop.5*filename = wallpaper5.jpeg

Note: X components have naming conventions for instance names and class names, in addition to wildcards * and ? (with precedence rules). I’m not sure numbers are valid on their own, so maybe it must be desktop5 instead of desktop.5.

Hopefully something like these X resource database entries could be implemented later on, without refactoring the entire hierarchy of option parsers yet again. That’s why I try to keep them in mind. And yes, it would be very sweet indeed if updating these entries with xrdb -merge worked in --deamon mode too. :-)

[stoeckmann]

Many thanks! No disagreements, but it seems I don’t quite understand the proper relation between screens and outputs (I never use multi-monitor setups myself, only virtual desktops). By your comments, you seem to have a fairly concrete idea on how to proceed? Would you like to give it a go yourself? :-)

I could prepare a base to work upon. It's up to you if I should set you as author of these commits or co-authored by or not at all.

But about screen and outputs, in a very brief summary: Nobody has more than one screen, basically. Output is what we consider a monitor/display, so multi-monitor setup is a multi-output setup. That's why I also totally forgot to reset outputs if another screen is supplied -- it just never happens.

[frno7]

I could prepare a base to work upon.

Great, thanks!

It's up to you if I should set you as author of these commits or co-authored by or not at all.

If you make a base with little or no resemblance to my contribution, I think it’s fair to have you as the author.

But about screen and outputs, in a very brief summary: Nobody has more than one screen, basically. Output is what we consider a monitor/display, so multi-monitor setup is a multi-output setup. That's why I also totally forgot to reset outputs if another screen is supplied -- it just never happens.

Oh.

I think the preexisting option parser and other parts of the code can be improved in various ways. For instance, instead of making an intermediate representation (via wp_option_t, wp_config_t, add_option(), etc.) of the given command options, it’s likely significantly simpler to make query functions iterating over the original and immutable char *argv[].

The most important query is: what is the wallpaper filename, mode, etc. for a given triplet of screen, desktop and output? Such a query function is simpler than for example add_option(), because it doesn’t need to juggle all combinations at once, perform deduplication, etc. Everything that doesn’t match the given triplet can be disregarded within the query function.

Query functions also decouple the command options from the operational parts of the program: for example, it’s easier to integrate new query functions for the X resource database. These queries are completely independent, and the operational part of the program can examine the results of both (command options, and X resource database) queries to determine precedence, and so on.

In some ways, such a query function already exists in several other parts of the code, for example as a piece here:

xwallpaper/main.c

Lines 650 to 658 in 18c8fe0

	for (opt = options; opt != NULL && opt->filename != NULL; opt++) {
	wp_output_t *output;
	/* ignore options which are not relevant for this screen */
	if (opt->screen != -1 && opt->screen != snum)
	continue;
	if (opt->output != NULL &&
	strcmp(opt->output, "all") == 0)

and here:

xwallpaper/outputs.c

Lines 52 to 53 in 18c8fe0

	for (output = outputs; output->name != NULL; output++)
	if (name != NULL && strcmp(output->name, name) == 0)

Also, verifying invalid option combinations becomes simpler when separated from the parser itself, etc.

[frno7]

If you find these ideas intriguing, I can write a new command option parser for Xwallpaper to demonstrate how it works in practice. :-)

[stoeckmann]

Cherry-picked commit in #58

[stoeckmann]

I could prepare a base to work upon.

Great, thanks!

I have prepared #60 which should implement the feature. It's very lightly tested so far, but let me know what you think and if it already works for you. Will invest more time in checking if everything's functional.

[stoeckmann]

If you find these ideas intriguing, I can write a new command option parser for Xwallpaper to demonstrate how it works in practice. :-)

In general I don't mind. I've kept the option parsing the way it is for now to focus on the desktop functionality. If it works reliably, which is not always intuitive with X specifics, we can move forward.

Keep in mind though that option handling is as powerful as in xrandr, so adjusting it might lead to unexpected regressions and loss of functionality.

Regarding your X properties, keep in mind that a mode is per file, not per invocation. You can use --output Virtual-1 --zoom file1.png --output Virtual-2 --trim 10x10 --center file2.png etc.

[frno7]

I have prepared #60 which should implement the feature. It's very lightly tested so far, but let me know what you think and if it already works for you. Will invest more time in checking if everything's functional.

Nice! #60 has a few problems though:

• Wallpapers will typically be overdrawn multiple times, degrading performance and causing flickering. The reason is that process_output etc. has no perception about whether more specific wallpapers will be processed later on. For instance, xwallpaper --desktop all foo.jpeg --desktop 3 bar.jpeg will, on desktop 3, first draw foo.jpeg, and then immediately replace what it just drew with bar.jpeg. This is one of several problems mentioned with the currently tight coupling of operational logic and the option parser.
• I would argue that 3 (being more specific) should take precedence over all (being less specific), regardless of ordering, so that swapping all and 3 is invariant. For instance, xwallpaper --desktop all foo.jpeg --desktop 3 bar.jpeg and xwallpaper --desktop 3 bar.jpeg --desktop all foo.jpeg would then behave exactly the same (precedence rules would become more prominent with X resource database integration).

I’ve made a complete rewrite of the option parser, which I’m preparing for integration. It’ll fix these issues, in addition to making forthcoming X resource database integration much simpler. Regardless, I hope you’ll find this loosely coupled option parser much easier to work and reason with. :-)

[stoeckmann]

I would argue that 3 (being more specific) should take precedence over all (being less specific), regardless of ordering, so that swapping all and 3 is invariant. For instance, xwallpaper --desktop all foo.jpeg --desktop 3 bar.jpeg and xwallpaper --desktop 3 bar.jpeg --desktop all foo.jpeg would then behave exactly the same (precedence rules would become more prominent with X resource database integration).

Beware that this is not in sync either with xrandr or with common CLI tools for which the last option overrrides the former. If I specify that all desktops should have foo.jpeg, then this effectively overrides bar.jpeg.

Skipping previous attempts to avoid flickering is one possibility, which would improve current behavior. The other possibility, which I didn't implement, Is to actually draw on top of the buffer before putting onto the output. Then you can overwrite the black pixels in maximize/center with another wallpaper in the background.

[frno7]

Beware that this is not in sync either with xrandr or with common CLI tools for which the last option overrrides the former. If I specify that all desktops should have foo.jpeg, then this effectively overrides bar.jpeg.

No problem, we can discuss and adjust precedence details later on. The new option parser can easily do either variant.

I’ve made a simple (and provisional) test-suite built into xwallpaper: when supplying the --test option, it’ll print out how it would match up a (fictional) configuration consisting of 3 screens, 4 desktops, and 3 outputs (DP, HDMI and VGA).

The option query function option_match_wallpaper() makes the matching of wallpapers in the test-suite:

        const char *outputs[] = { "DP", "HDMI", "VGA", NULL };

        for (int screen = 0; screen <= 2; screen++)
        for (int desktop = 0; desktop <= 4; desktop++)
        for (int output = 0; outputs[output]; output++) {
                const struct wallpaper wp =
                        option_match_wallpaper(args, screen, desktop, outputs[output]);

                if (wp.filename)
                        printf("screen %d desktop %d output %-4s mode %-8s wallpaper %s\n",
                                screen, desktop, outputs[output], wp.modename, wp.filename);
        }

Example:

xwallpaper --test \
    --zoom foo.jpeg \
    --screen 1 \
    --center bar.jpeg \
    --desktop 2 \
    --focus fie.jpeg \
    --output HDMI \
    --maximize fuu.jpeg
screen 0 desktop 0 output DP   mode zoom     wallpaper foo.jpeg
screen 0 desktop 0 output HDMI mode zoom     wallpaper foo.jpeg
screen 0 desktop 0 output VGA  mode zoom     wallpaper foo.jpeg
screen 0 desktop 1 output DP   mode zoom     wallpaper foo.jpeg
screen 0 desktop 1 output HDMI mode zoom     wallpaper foo.jpeg
screen 0 desktop 1 output VGA  mode zoom     wallpaper foo.jpeg
screen 0 desktop 2 output DP   mode zoom     wallpaper foo.jpeg
screen 0 desktop 2 output HDMI mode zoom     wallpaper foo.jpeg
screen 0 desktop 2 output VGA  mode zoom     wallpaper foo.jpeg
screen 0 desktop 3 output DP   mode zoom     wallpaper foo.jpeg
screen 0 desktop 3 output HDMI mode zoom     wallpaper foo.jpeg
screen 0 desktop 3 output VGA  mode zoom     wallpaper foo.jpeg
screen 0 desktop 4 output DP   mode zoom     wallpaper foo.jpeg
screen 0 desktop 4 output HDMI mode zoom     wallpaper foo.jpeg
screen 0 desktop 4 output VGA  mode zoom     wallpaper foo.jpeg
screen 1 desktop 0 output DP   mode center   wallpaper bar.jpeg
screen 1 desktop 0 output HDMI mode center   wallpaper bar.jpeg
screen 1 desktop 0 output VGA  mode center   wallpaper bar.jpeg
screen 1 desktop 1 output DP   mode center   wallpaper bar.jpeg
screen 1 desktop 1 output HDMI mode center   wallpaper bar.jpeg
screen 1 desktop 1 output VGA  mode center   wallpaper bar.jpeg
screen 1 desktop 2 output DP   mode focus    wallpaper fie.jpeg
screen 1 desktop 2 output HDMI mode maximize wallpaper fuu.jpeg
screen 1 desktop 2 output VGA  mode focus    wallpaper fie.jpeg
screen 1 desktop 3 output DP   mode center   wallpaper bar.jpeg
screen 1 desktop 3 output HDMI mode center   wallpaper bar.jpeg
screen 1 desktop 3 output VGA  mode center   wallpaper bar.jpeg
screen 1 desktop 4 output DP   mode center   wallpaper bar.jpeg
screen 1 desktop 4 output HDMI mode center   wallpaper bar.jpeg
screen 1 desktop 4 output VGA  mode center   wallpaper bar.jpeg
screen 2 desktop 0 output DP   mode zoom     wallpaper foo.jpeg
screen 2 desktop 0 output HDMI mode zoom     wallpaper foo.jpeg
screen 2 desktop 0 output VGA  mode zoom     wallpaper foo.jpeg
screen 2 desktop 1 output DP   mode zoom     wallpaper foo.jpeg
screen 2 desktop 1 output HDMI mode zoom     wallpaper foo.jpeg
screen 2 desktop 1 output VGA  mode zoom     wallpaper foo.jpeg
screen 2 desktop 2 output DP   mode zoom     wallpaper foo.jpeg
screen 2 desktop 2 output HDMI mode zoom     wallpaper foo.jpeg
screen 2 desktop 2 output VGA  mode zoom     wallpaper foo.jpeg
screen 2 desktop 3 output DP   mode zoom     wallpaper foo.jpeg
screen 2 desktop 3 output HDMI mode zoom     wallpaper foo.jpeg
screen 2 desktop 3 output VGA  mode zoom     wallpaper foo.jpeg
screen 2 desktop 4 output DP   mode zoom     wallpaper foo.jpeg
screen 2 desktop 4 output HDMI mode zoom     wallpaper foo.jpeg
screen 2 desktop 4 output VGA  mode zoom     wallpaper foo.jpeg

[stoeckmann]

Looking forward to these changes! Since nobody complained so far that it's not possible to overdraw wallpapers, let's keep it at that.

[stoeckmann]

Wallpapers will typically be overdrawn multiple times, degrading performance and causing flickering. The reason is that process_output etc. has no perception about whether more specific wallpapers will be processed later on. For instance, xwallpaper --desktop all foo.jpeg --desktop 3 bar.jpeg will, on desktop 3, first draw foo.jpeg, and then immediately replace what it just drew with bar.jpeg. This is one of several problems mentioned with the currently tight coupling of operational logic and the option parser.

Fortunately this is no regression, since it already happens with --output all ... --output HDMI-1 etc. It just happens more often because the amount can be doubled now, i.e. up to 4 times.

[frno7]

Looking forward to these changes!

The new option parser, as well as the new query function option_match_wallpaper(), have no mutable state. In the world of functional languages, these are pure functions.

The heavy lifting is done with a new option parser macro for_each_option(), shown in use below, and its associate macros option_label, option_string and option_integer (which certainly are not global variables). The macro for_each_option() iterates over the original argv array, so there’s no intermediate data representation at all.

This is (currently) the complete implementation of option_match_wallpaper() for matching wallpapers to the given triple screen, desktop and output. Notice how it’s completely decoupled from the operational logic for the actual hardware configuration of the machine, and how simple it is:

struct wallpaper {
        int mode;
        const char *trim;
        const char *modename;
        const char *filename;

        int screen;
        int desktop;
        const char *output;
};

static int screen_desktop_output_match(struct wallpaper wp,
        int screen, int desktop, const char *output)
{
        /* -1 acts as a match "all" wildcard for screen and desktop */

        if (wp.screen >= 0 && screen >= 0 && wp.screen != screen)
                return 0;

        if (wp.desktop >= 0 && desktop >= 0 && wp.desktop != desktop)
                return 0;

        if (strcmp(wp.output, "all") == 0 || strcmp(output, "all") == 0)
                return 1;

        return strcmp(wp.output, output) == 0;
}

struct wallpaper option_match_wallpaper(struct argcv args,
        int screen, int desktop, const char *output)
{
        struct wallpaper wp = {
                .screen = -1,
                .desktop = -1,
                .output = "all",
        };
        struct wallpaper best = wp;

        for_each_option (args) {
                switch (option_label) {
                case OPTION_center:
                case OPTION_focus:
                case OPTION_maximize:
                case OPTION_stretch:
                case OPTION_tile:
                case OPTION_zoom:
                        wp.mode = option_label;
                        wp.modename = &option_name[2];
                        wp.filename = option_string;

                        if (screen_desktop_output_match(wp, screen, desktop, output))
                                best = wp;
                        break;

                case OPTION_trim:
                        wp.trim = option_string;
                        break;

                case OPTION_screen:
                        wp.trim = NULL;
                        wp.screen = option_integer;
                        wp.desktop = -1;
                        wp.output = "all";
                        break;

                case OPTION_desktop:
                        wp.trim = NULL;
                        wp.desktop = option_integer;
                        wp.output = "all";
                        break;

                case OPTION_output:
                        wp.trim = NULL;
                        wp.output = option_string;
                        break;
                }
        }

        return best;
}

[frno7]

The new functionally pure option parser and its associated option_match_wallpaper() query function are now integrated in commit efba122:

• Lightly tested, but it shows how everything fits together.
• I had to configure it with --without-seccomp, due to not preopening files for reading, which will be a problem with a forthcoming X resource database integration as well.
• The function process_screen() has a few FIXMEs.
• We can very easily make an xrdb_match_wallpaper() function to query the X resource database now, and have command options and X resources play nicely together, with appropriate precedence. :-)

Commit a06ed1f adds an automatically generated --help option:

$ xwallpaper --help
usage: xwallpaper [options]...

options:

  --help                 prints help and exits
  --center   <file>      centers the input file on the output
  --clear                initializes screen with a black background
  --daemon               keeps xwallpaper running in background
  --debug                displays debug messages on standard error
  --desktop  <number>    desktop by its _NET_CURRENT_DESKTOP number
  --focus    <file>      trim box guaranteed to be visible on output
  --maximize <file>      maximizes to fit without cropping
  --no-atoms             atoms for pseudo transparency are not updated
  --no-randr             uses the whole output inputs
  --no-root              background of the root window is not updated
  --output   <output>    output for subsequent wallpaper files
  --screen   <number>    screen by its screen number
  --stretch  <file>      stretches file to fully cover output
  --tile     <file>      tiles file repeatedly
  --trim     <geometry>  area of interest in source file
  --version              prints version and exits
  --zoom     <file>      zooms file to fit output with cropping

Commit 0f0e42c adds a --test option for the test-suite mentioned.

[stoeckmann]

I had to configure it with --without-seccomp, due to not preopening files for reading, which will be a problem with a forthcoming X resource database integration as well.

Is this just temporary? I wouldn't want to loosen sandbox mechanisms for option parsing.

We can very easily make an xrdb_match_wallpaper() function to query the X resource database now, and have command options and X resources play nicely together, with appropriate precedence. :-)

If it requires lowering stage1 or stage2 sandbox (or completely disabling it), then this would put it out of scope of xwallpaper. It is one of its main goals.

[frno7]

Is this just temporary?

Yes, seccomp is trivial to add back in, with its previous semantics by preopening files.

I wouldn't want to loosen sandbox mechanisms for option parsing.

No problem.

If it requires lowering stage1 or stage2 sandbox (or completely disabling it), then this would put it out of scope of xwallpaper. It is one of its main goals.

If X resources are only read during start-up, they could, like their corresponding command options, have preopened files. However, if a change is made to the X resource database, such as changing wallpaper files, the xwallpaper process will have to be restarted, via the command sequence pkill xwallpaper ; xwallpaper --daemon, for instance.

Personally, I’d like to have xwallpaper monitor the X resource database for changes, without me having to pkill it to perform its thing. I can maintain such a patch for myself, so there’s no need to have that feature in the mainline repository:

A solution could be to let xwallpaper fork itself during start-up. One process only monitors changes in the X resource database, and runs (or restarts) the other file-reading wallpaper display process as needed. A two-process daemon, would that be a desirable alternative?

Proper security is a serious matter, and on that topic I think the new option parser is a significant step forward. Notice, for instance, how simple option_match_wallpaper() becomes because it doesn’t need to do any error checking on its own. The reason is that all necessary error checking has already been done in option_verify().

[stoeckmann]

Is this just temporary?

Yes, seccomp is trivial to add back in, with its previous semantics by preopening files.

That's good.

So, to get these topics forward, I would like to do the following:

• Fix a current option parser bug which I just noticed (xwallpaper --output all --zoom file.jpg --output LVMS-1 --center file.jpg --output all --zoom file.jpg leads to a centered file on LVMS-1 because the replacement happens in-place in code, which is wrong)
• Fix the screen selection if no RandR and tiling is enabled
• Avoid repeated output on the same monitor

With these in place and maybe other bug fixes, I would release 0.7.7 to have a better idea what's a breaking change for users, if there is any, which I hope is not, since all of these issues were bugs.

Then we can add --desktop with 0.8.0 and see about the option parsing. Especially comparing the fixed code base with the new one to better see advantages and disadvantages.

About the new option parsing: It contains a lot of macros, which might be difficult to read for people who are not very familiar with C. If we can keep the amount down, it would make the code more accessible. If there are still people around interested in C and X ... ;)

[frno7]

Then we can add --desktop with 0.8.0 and see about the option parsing. Especially comparing the fixed code base with the new one to better see advantages and disadvantages.

Sure.

About the new option parsing: It contains a lot of macros, which might be difficult to read for people who are not very familiar with C. If we can keep the amount down, it would make the code more accessible. If there are still people around interested in C and X ... ;)

argparse.h is a bit on the advanced side, to obtain the for_each_option() macro. Removing these macros would increase code repetition, and the risk of trivial errors, on the other hand.

The current code-base on the master branch as a whole isn’t particularly easy to understand, even for experienced programmers: functions are often very large, with many mutable variables, entangled states, and so on. I think it would require some refactoring effort to make the code easier to understand, debug, and extend with new features. Untangling the option parser is, in my view, a simplification, though.

[stoeckmann]

The current code-base on the master branch as a whole isn’t particularly easy to understand, even for experienced programmers: functions are often very large, with many mutable variables, entangled states, and so on. I think it would require some refactoring effort to make the code easier to understand, debug, and extend with new features. Untangling the option parser is, in my view, a simplification, though.

Yeah, code simplification would be a plus. If I would have known that XCB would turn out as a proof of concept without widespread usage, it would have been probably better to just use the X libraries. But looking at their security track record and what I've found in them, it's probably good the way it is.

Just kidding. I think the whole X communication and these "pseudo transparency" ideas make the tasks more complicated than they would have to be.

The best design choice would have been to turn all of that into its own library so others could re-use it, e.g. for X property handling with their own tools. ;)

[stoeckmann]

Nice! #60 has a few problems though:

Wallpapers will typically be overdrawn multiple times, degrading performance and causing flickering. The reason is that process_output etc. has no perception about whether more specific wallpapers will be processed later on. For instance, xwallpaper --desktop all foo.jpeg --desktop 3 bar.jpeg will, on desktop 3, first draw foo.jpeg, and then immediately replace what it just drew with bar.jpeg. This is one of several problems mentioned with the currently tight coupling of operational logic and the option parser.

Should be fixed now. An idea how to fix these issues in 0.7.6 with small adjustments exists in parallel, rebased on top of it.

Note: Rebased on the fix branch since I know that you don't have multiple outputs to test with.

[stoeckmann]

Personally, I’d like to have xwallpaper monitor the X resource database for changes, without me having to pkill it to perform its thing. I can maintain such a patch for myself, so there’s no need to have that feature in the mainline repository:

A solution could be to let xwallpaper fork itself during start-up. One process only monitors changes in the X resource database, and runs (or restarts) the other file-reading wallpaper display process as needed. A two-process daemon, would that be a desirable alternative?

Perhaps it makes sense to discuss this in an own issue. From my point of view, it wouldn't take much to split this entirely off from xwallpaper by having an own daemon which handles x properties, manages xwallpaper as a child, and restarts it with proper options when needed.

This would indicate that the functionality can exist outside of xwallpaper, even for other wallpaper setting utilities.

[frno7]

Perhaps it makes sense to discuss this in an own issue. From my point of view, it wouldn't take much to split this entirely off from xwallpaper by having an own daemon which handles x properties, manages xwallpaper as a child, and restarts it with proper options when needed.

If the purpose of seccomp is to protect against malicious JPEG/PNG/XPM files, it seems forking, followed by seccomp in the child process, is most straightforward: the child process would read the source image on its standard input, and write the decoded image on its standard output; the child would not be able to communicate with the X server at all. That would be a stronger protection than what is available now.

Additional security measures can be taken. In daemon mode, for instance, xwallpaper could perhaps restrict itself to reading files in certain directories. This would deny access to arbitrary files, but still allow some flexibility in the X resource database to change wallpapers during runtime.

[stoeckmann]

That would be a stronger protection than what is available now.

And one which slows xwallpaper significantly down, which is why I've tossed the proof of concept back then. The protection is also against a rogue X server. No matter what goes wrong in all this XCB handling, no files can be opened anymore and no processes spawned. At least with OpenBSD's pledge, since there are quite some exceptions with seccomp if an attacker gets control over these still available system calls.

Additional security measures can be taken. In daemon mode, for instance, xwallpaper could perhaps restrict itself to reading files in certain directories. This would deny access to arbitrary files, but still allow some flexibility in the X resource database to change wallpapers during runtime.

Yes, a separation of concern would eventually lead to 3 running instances (management, X connectivity, image parsing), with one instance only useful in daemon mode. It would probably take a survey to figure out a) how many people still use X and then also xwallpaper and b) how many people actually use daemon mode. And while at it, c) how many people have a multi monitor setup and still perform a) and b).

By now, it is probably easier to use nitrogen or other wallpaper setting utilities which use gdk-pixbuf for image loading. The glycin project, a collection of sandboxed Rust-based image parsers, is used in gdk-pixbuf by now and takes a lot of burden away from handling hostile image files. This leaves the X connectivity vulnerable, but still a better base for adjustments.

[frno7]

And one which slows xwallpaper significantly down, which is why I've tossed the proof of concept back then. The protection is also against a rogue X server.

For maximum speed, I would write decoded images to for instance ~/.cache. But hey, I realise I should make my own wallpaper program from scratch by now. :-)