[PATCH] xrandr: subscribe to RRScreenChange event

[Florian Bruhin]

Hi,

* Christian Dietrich <stettberger at dokucode.de> [2014-07-13 14:41:31 +0200]:
> When Xrandr is available at compile and at run time, herbstluftwm
> subscribes to the screen change events. These events are send, when
> configuration of the monitors changes. The event is propagated to all
> herbstclients via the hook.
> 
>     randr_screen_change	4108552	1497	724
> 
> An additional script can listen to those events and can start the
> reconfiguration of the herbstluftwm monitors. This is especially useful
> for using herbstluftwm within a virtual machine, whose screen size can
> change quite often.

Do you know about herbstluft's detect_monitors command, and the
auto_detect_monitors settings? IMHO it would make more sense to make
that work with XRandR (in addition to the existing Xinerama code).

Then the other question is if you still need a hook for it, when HLWM
reconfigures the monitors automatically. If so, it might make sense to
emit it from the Xinerama code as well, if possible.

>  Hopefully my mail client didn't fuckup the patch.

Looks like it did? I have the impression randr.h should be longer than
that:

> diff --git a/src/randr.h b/src/randr.h
> new file mode 100644
> index 0000000..6e26870
> --- /dev/null
> +++ b/src/randr.h
> @@ -0,0 +1,17 @@
> +/** Copyright 2014 Christian Dietrich. All rights reserved.
> + *
> + * This software is licensed under the "Simplified BSD License".
> + * See LICENSE for details */
> +

Then some other feedback and minor nitpicks:

> @@ -765,22 +766,24 @@ static HandlerTable g_default_handler = {
>  static struct {
>      void (*init)();
>      void (*destroy)();
> +    bool (*xevent)(XEvent *);
>  } g_modules[] = {
> -    { ipc_init,         ipc_destroy         },
> -    { object_tree_init, object_tree_destroy },
> -    { key_init,         key_destroy         },
> -    { settings_init,    settings_destroy    },
> -    { floating_init,    floating_destroy    },
> -    { stacklist_init,   stacklist_destroy   },
> -    { layout_init,      layout_destroy      },
> -    { tag_init,         tag_destroy         },
> -    { clientlist_init,  clientlist_destroy  },
> -    { decorations_init, decorations_destroy },
> -    { monitor_init,     monitor_destroy     },
> -    { ewmh_init,        ewmh_destroy        },
> -    { mouse_init,       mouse_destroy       },
> -    { hook_init,        hook_destroy        },
> -    { rules_init,       rules_destroy       },
> +    { ipc_init,         ipc_destroy         , 0},
> +    { object_tree_init, object_tree_destroy , 0},
> +    { key_init,         key_destroy         , 0},
> +    { settings_init,    settings_destroy    , 0},
> +    { floating_init,    floating_destroy    , 0},
> +    { stacklist_init,   stacklist_destroy   , 0},
> +    { layout_init,      layout_destroy      , 0},
> +    { tag_init,         tag_destroy         , 0},
> +    { clientlist_init,  clientlist_destroy  , 0},
> +    { decorations_init, decorations_destroy , 0},
> +    { monitor_init,     monitor_destroy     , 0},
> +    { ewmh_init,        ewmh_destroy        , 0},
> +    { mouse_init,       mouse_destroy       , 0},
> +    { hook_init,        hook_destroy        , 0},
> +    { rules_init,       rules_destroy       , 0},
> +    { randr_init,       randr_destroy       , randr_xevent},
>  };

Wouldn't it make more sense to use NULL instead of 0 here?
Also commas are before the spaces in the first column
but after the spaces in the second ;)

> +    /* Query whether the xrandr extension is available in a version >= 1.2 */
> +    if (XRRQueryExtension (g_display, &rr_event_base, &ignore)) {
> ...

For the sake of consistency, you might want to remove the spaces
before ( with function calls. I think the current way this is handled
in hlwm currently is "foo(...)" but "while (...)" (i.e., spaces only
with control structures).

> +            HSDebug("randr: xrandr version is too old (%d.%d); no screen notify events will be availabe",
> +                    major_version, minor_version);

s/availabe/available/ ;)

> +        HSDebug("randr: xrandr extension is not available");

There's a newline missing (same with some other HSDebug calls).

> +            char timestamp[30] , width[30], height[30];

No space before that comma :P


Florian

-- 
http://www.the-compiler.org | me at the-compiler.org (Mail/XMPP)
             GPG 0xFD55A072 | http://the-compiler.org/pubkey.asc
         I love long mails! | http://email.is-not-s.ms/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://listi.jpberlin.de/pipermail/hlwm/attachments/20140716/b8c53e84/attachment.sig>