Merge lp:~elementary-pantheon/granite/lp-1066136 into lp:~elementary-pantheon/granite/granite

[Victor Martinez]

Fix bug #1066136: Removing an item from the ModeButton widget renders consecutive item indexes invalid.

This branch fixes Bug #1066136: Removing an item from the ModeButton widget renders consecutive item indexes invalid.

The approach followed here is using an internal hash map to keep a strong key-item association, and smarter code for creating a new index in ModeButton.append().

You can find more information (and even a test) in the bug report. Thanks in advance for the review!

[David Gomes (davidgomes)]

The code really looks fine, but I believe that before a merge this should have some more testing, we really can't break ModeButton.

[Victor Martinez (victored)]

I don't think this breaks anything, unless some application is using lp:1066136 as a feature rather than a bug xD

We'll never know unless we merge it. It's better to figure out we broke something before it's already too-late to fix it in this development cycle.

[Sergey "Shnatsel" Davidoff (shnatsel)]

I wonder why this is still not merged...

[David Gomes (davidgomes)]

review: Approve

=== modified file 'lib/Widgets/ModeButton.vala'
--- lib/Widgets/ModeButton.vala	2012-09-26 20:49:21 +0000
+++ lib/Widgets/ModeButton.vala	2012-10-13 02:12:20 +0000
@@ -21,6 +21,15 @@
 
     public class ModeButton : Gtk.Box {
 
+        private class Item : Gtk.ToggleButton {
+            public int index { get; construct; }
+            public Item (int index) {
+                Object (index: index);
+                can_focus = false;
+                add_events (Gdk.EventMask.SCROLL_MASK);
+            }
+        }
+
         public signal void mode_added (int index, Gtk.Widget widget);
         public signal void mode_removed (int index, Gtk.Widget widget);
         public signal void mode_changed (Gtk.Widget widget);
@@ -31,16 +40,19 @@
         }
 
         public uint n_items {
-            get { return get_children ().length (); }
+            get { return item_map.size; }
         }
 
         private int _selected = -1;
+        private Gee.HashMap<int, Item> item_map;
 
         public ModeButton () {
             homogeneous = true;
             spacing = 0;
             can_focus = false;
 
+            item_map = new Gee.HashMap<int, Item> ();
+
             var style = get_style_context ();
             style.add_class (Gtk.STYLE_CLASS_LINKED);
             style.add_class ("raised"); // needed for toolbars
@@ -59,41 +71,42 @@
         }
 
         public int append (Gtk.Widget w) {
-            var button = new Gtk.ToggleButton ();
-            button.can_focus = false;
-            button.add_events (Gdk.EventMask.SCROLL_MASK);
-            button.scroll_event.connect (on_scroll_event);
-
-            button.add (w);
-
-            button.button_press_event.connect ( () => {
-                set_active (get_children ().index (button));
+            int index;
+            for (index = item_map.size; item_map.has_key (index); index++);
+            assert (item_map[index] == null);
+
+            var item = new Item (index);
+            item.scroll_event.connect (on_scroll_event);
+            item.add (w);
+
+            item.button_press_event.connect (() => {
+                set_active (item.index);
                 return true;
             });
 
-            add (button);
-            button.show_all ();
-
-            var children = get_children ();
-            int item_index = (int)children.length () - 1;
-            mode_added (item_index, w);
-            return item_index;
+            item_map[index] = item;
+
+            add (item);
+            item.show_all ();
+
+            mode_added (index, w);
+
+            return index;
         }
 
         public void set_active (int new_active_index) {
-            var children = get_children ();
-            return_if_fail (new_active_index >= 0 && new_active_index < children.length ());
-
-            var new_item = children.nth_data (new_active_index) as Gtk.ToggleButton;
+            return_if_fail (item_map.has_key (new_active_index));
+            var new_item = item_map[new_active_index] as Item;
 
             if (new_item != null) {
+                assert (new_item.index == new_active_index);
                 new_item.set_active (true);
 
                 if (_selected == new_active_index)
                     return;
 
                 // Unselect the previous item
-                var old_item = children.nth_data (_selected) as Gtk.ToggleButton;
+                var old_item = item_map[_selected] as Item;
                 if (old_item != null)
                     old_item.set_active (false);
 
@@ -104,24 +117,24 @@
         }
 
         public void set_item_visible (int index, bool val) {
-            var children = get_children ();
-            return_if_fail (index >= 0 && index < children.length ());
-
-            var item = children.nth_data (index);
+            return_if_fail (item_map.has_key (index));
+            var item = item_map[index] as Item;
 
             if (item != null) {
+                assert (item.index == index);
                 item.no_show_all = !val;
                 item.visible = val;
             }
         }
 
         public new void remove (int index) {
-            var children = get_children ();
-            return_if_fail (index >= 0 && index < children.length ());
+            return_if_fail (item_map.has_key (index));
+            var item = item_map[index] as Item;
 
-            var item = children.nth_data (index) as Gtk.Bin;
             if (item != null) {
+                assert (item.index == index);
                 mode_removed (index, item.get_child ());
+                item_map.unset (index);
                 item.destroy ();
             }
         }
@@ -133,6 +146,8 @@
                     base.remove (button);
             }
 
+            item_map.clear ();
+
             _selected = -1;
         }
 
@@ -152,18 +167,26 @@
                     return false;
             }
 
-            int new_item = selected;
-
-            // Try to find a valid item, since there could be invisible items in the middle
-            // and those shouldn't be selected
+            // Try to find a valid item, since there could be invisible items in
+            // the middle and those shouldn't be selected. We use the children list
+            // instead of item_map because order matters here.
             var children = get_children ();
             uint n_children = children.length ();
 
+            var selected_item = item_map[selected];
+            if (selected_item == null)
+                return false;
+
+            int new_item = children.index (selected_item);
+            if (new_item < 0)
+                return false;
+
             do {
                 new_item += offset;
-                var item = children.nth_data (new_item);
-                if (item != null && item.visible) {
-                    selected = new_item;
+                var item = children.nth_data (new_item) as Item;
+
+                if (item != null && item.visible && item.sensitive) {
+                    selected = item.index;
                     break;
                 }
             } while (new_item >= 0 && new_item < n_children);