Hi Florian, On Sun, Oct 06, 2013 at 01:40:32AM +0200, Florian Bruhin wrote: > * Thorsten Wißmann <edu _at_ thorsten _minus_ wissmann _dot_ 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 _minus_ compiler _dot_ 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.
Attachment:
pgpx8xAc5XxnK.pgp
Description: PGP signature