[PATCH] Monitor objects per monitor-index

[Thorsten Wißmann]

Hi Florian,

On Sun, Oct 06, 2013 at 01:40:32AM +0200, Florian Bruhin wrote:
> * Thorsten Wißmann <edu at thorsten-wissmann.de> [2013-06-24 13:03:32 +0200]:
> > On Thu, Jun 20, 2013 at 10:40:17PM +0200, Florian Bruhin wrote:
> > > This patch adds monitor objects to be able to access them by their
> > > index if they are unnamed.
> > 
> > Did you forget to add some hunks in the patch? Because this only adds a
> > link when a new monitor is added and never renames them.
> > [...]
> 
> This should now all be fixed in the attached new patch. Tested it by
> adding and removing a few monitors and it works so far. It unlinks and
> re-links all id objects when a monitor is removed, as discussed in
> IRC.
> 
> Also, the second patch uses the new monitor_foreach function for
> all_monitors_apply_layout as well.

It doesn't work because of the following:

> +static void monitor_foreach(void (*action)(HSMonitor*)) {
> +    for (int i = 0; i < g_monitors->len; i++) {
> [...]
> +static void monitor_update_id_objects() {
> +    monitor_foreach(monitor_unlink_id_object);
> +    monitor_foreach(monitor_link_id_object);
> +}
> +
> [...]
> @@ -532,6 +558,7 @@ int remove_monitor(int index) {
>      g_string_free(monitor->display_name, true);
>      g_free(monitor);
>      g_array_remove_index(g_monitors, index);
> +    monitor_update_id_objects();

Now, the old monitor object never is removed, because the
foreach(unlink) iterates over all entries of the g_monitors array, so
unlink is not called for monitor as it already has been removed one line
earlier. So monitor_update_id_objects() does not make sense for
removing, because both monitor_foreach() calls iterate over exactly the
same g_monitors state.

The fix for remove_monitor is a little rearrangement:

    g_string_free(monitor->display_name, true);
    monitor_foreach(monitor_unlink_id_object);
    g_array_remove_index(g_monitors, index);
    g_free(monitor);
    monitor_foreach(monitor_link_id_object);

Note that the position of g_free(monitor) does not matter as long as it
called after monitor_unlink_id_object(). (And it is not needed for
add_monitor, so I dropped it).

> From 631cddfe2325e2bbed88fca66d5e746e95188a12 Mon Sep 17 00:00:00 2001
> From: Florian Bruhin <git at the-compiler.org>
> Date: Sun, 6 Oct 2013 00:45:42 +0200
> Subject: [PATCH 2/2] Use monitors_foreach for all_monitors_apply_layout

I changed it to monitor_foreach.

>  void all_monitors_apply_layout() {
> -    for (int i = 0; i < g_monitors->len; i++) {
> -        HSMonitor* m = monitor_with_index(i);
> -        monitor_apply_layout(m);
> -    }
> +    monitor_foreach(monitor_apply_layout);
>  }

Well spotted! All in all, it is much less code than I expected. Good
Work!

After the named modifications[1] I pushed it as:

b7d1d6d Use monitor_foreach for all_monitors_apply_layout
6006395 Add objects monitors.* to access monitors per id

Regards,
Thorsten

[1] If you have problems with me modifing your patch and commiting it
with your name, let me know.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 230 bytes
Desc: not available
URL: <https://listi.jpberlin.de/pipermail/hlwm/attachments/20131006/6231c85d/attachment.sig>