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

Proposed by Victor Martinez on 2012-10-13
Status: Merged
Merged at revision: 422
Proposed branch: lp:~elementary-pantheon/granite/lp-1066136
Merge into: lp:~elementary-pantheon/granite/granite
Diff against target: 179 lines (+59/-36)
1 file modified
lib/Widgets/ModeButton.vala (+59/-36)
To merge this branch: bzr merge lp:~elementary-pantheon/granite/lp-1066136
Reviewer Review Type Date Requested Status
David Gomes 2012-10-13 Approve on 2012-10-23
Review via email: mp+129546@code.launchpad.net

Commit message

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

Description of the change

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!

To post a comment you must log in.
David Gomes (davidgomes) wrote :

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) wrote :

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.

I wonder why this is still not merged...

David Gomes (davidgomes) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/Widgets/ModeButton.vala'
2--- lib/Widgets/ModeButton.vala 2012-09-26 20:49:21 +0000
3+++ lib/Widgets/ModeButton.vala 2012-10-13 02:12:20 +0000
4@@ -21,6 +21,15 @@
5
6 public class ModeButton : Gtk.Box {
7
8+ private class Item : Gtk.ToggleButton {
9+ public int index { get; construct; }
10+ public Item (int index) {
11+ Object (index: index);
12+ can_focus = false;
13+ add_events (Gdk.EventMask.SCROLL_MASK);
14+ }
15+ }
16+
17 public signal void mode_added (int index, Gtk.Widget widget);
18 public signal void mode_removed (int index, Gtk.Widget widget);
19 public signal void mode_changed (Gtk.Widget widget);
20@@ -31,16 +40,19 @@
21 }
22
23 public uint n_items {
24- get { return get_children ().length (); }
25+ get { return item_map.size; }
26 }
27
28 private int _selected = -1;
29+ private Gee.HashMap<int, Item> item_map;
30
31 public ModeButton () {
32 homogeneous = true;
33 spacing = 0;
34 can_focus = false;
35
36+ item_map = new Gee.HashMap<int, Item> ();
37+
38 var style = get_style_context ();
39 style.add_class (Gtk.STYLE_CLASS_LINKED);
40 style.add_class ("raised"); // needed for toolbars
41@@ -59,41 +71,42 @@
42 }
43
44 public int append (Gtk.Widget w) {
45- var button = new Gtk.ToggleButton ();
46- button.can_focus = false;
47- button.add_events (Gdk.EventMask.SCROLL_MASK);
48- button.scroll_event.connect (on_scroll_event);
49-
50- button.add (w);
51-
52- button.button_press_event.connect ( () => {
53- set_active (get_children ().index (button));
54+ int index;
55+ for (index = item_map.size; item_map.has_key (index); index++);
56+ assert (item_map[index] == null);
57+
58+ var item = new Item (index);
59+ item.scroll_event.connect (on_scroll_event);
60+ item.add (w);
61+
62+ item.button_press_event.connect (() => {
63+ set_active (item.index);
64 return true;
65 });
66
67- add (button);
68- button.show_all ();
69-
70- var children = get_children ();
71- int item_index = (int)children.length () - 1;
72- mode_added (item_index, w);
73- return item_index;
74+ item_map[index] = item;
75+
76+ add (item);
77+ item.show_all ();
78+
79+ mode_added (index, w);
80+
81+ return index;
82 }
83
84 public void set_active (int new_active_index) {
85- var children = get_children ();
86- return_if_fail (new_active_index >= 0 && new_active_index < children.length ());
87-
88- var new_item = children.nth_data (new_active_index) as Gtk.ToggleButton;
89+ return_if_fail (item_map.has_key (new_active_index));
90+ var new_item = item_map[new_active_index] as Item;
91
92 if (new_item != null) {
93+ assert (new_item.index == new_active_index);
94 new_item.set_active (true);
95
96 if (_selected == new_active_index)
97 return;
98
99 // Unselect the previous item
100- var old_item = children.nth_data (_selected) as Gtk.ToggleButton;
101+ var old_item = item_map[_selected] as Item;
102 if (old_item != null)
103 old_item.set_active (false);
104
105@@ -104,24 +117,24 @@
106 }
107
108 public void set_item_visible (int index, bool val) {
109- var children = get_children ();
110- return_if_fail (index >= 0 && index < children.length ());
111-
112- var item = children.nth_data (index);
113+ return_if_fail (item_map.has_key (index));
114+ var item = item_map[index] as Item;
115
116 if (item != null) {
117+ assert (item.index == index);
118 item.no_show_all = !val;
119 item.visible = val;
120 }
121 }
122
123 public new void remove (int index) {
124- var children = get_children ();
125- return_if_fail (index >= 0 && index < children.length ());
126+ return_if_fail (item_map.has_key (index));
127+ var item = item_map[index] as Item;
128
129- var item = children.nth_data (index) as Gtk.Bin;
130 if (item != null) {
131+ assert (item.index == index);
132 mode_removed (index, item.get_child ());
133+ item_map.unset (index);
134 item.destroy ();
135 }
136 }
137@@ -133,6 +146,8 @@
138 base.remove (button);
139 }
140
141+ item_map.clear ();
142+
143 _selected = -1;
144 }
145
146@@ -152,18 +167,26 @@
147 return false;
148 }
149
150- int new_item = selected;
151-
152- // Try to find a valid item, since there could be invisible items in the middle
153- // and those shouldn't be selected
154+ // Try to find a valid item, since there could be invisible items in
155+ // the middle and those shouldn't be selected. We use the children list
156+ // instead of item_map because order matters here.
157 var children = get_children ();
158 uint n_children = children.length ();
159
160+ var selected_item = item_map[selected];
161+ if (selected_item == null)
162+ return false;
163+
164+ int new_item = children.index (selected_item);
165+ if (new_item < 0)
166+ return false;
167+
168 do {
169 new_item += offset;
170- var item = children.nth_data (new_item);
171- if (item != null && item.visible) {
172- selected = new_item;
173+ var item = children.nth_data (new_item) as Item;
174+
175+ if (item != null && item.visible && item.sensitive) {
176+ selected = item.index;
177 break;
178 }
179 } while (new_item >= 0 && new_item < n_children);

Subscribers

People subscribed via source and target branches