Merge lp:~3v1n0/unity/quicklist-keynav-fixes into lp:unity
- quicklist-keynav-fixes
- Merge into trunk
Status: | Merged | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Approved by: | Marco Trevisan (Treviño) | ||||||||||||
Approved revision: | no longer in the source branch. | ||||||||||||
Merged at revision: | 2310 | ||||||||||||
Proposed branch: | lp:~3v1n0/unity/quicklist-keynav-fixes | ||||||||||||
Merge into: | lp:unity | ||||||||||||
Diff against target: |
1067 lines (+461/-148) 14 files modified
plugins/unityshell/src/AbstractLauncherIcon.h (+1/-1) plugins/unityshell/src/LauncherIcon.cpp (+3/-3) plugins/unityshell/src/LauncherIcon.h (+1/-1) plugins/unityshell/src/MockLauncherIcon.h (+1/-1) plugins/unityshell/src/QuicklistMenuItem.cpp (+9/-23) plugins/unityshell/src/QuicklistMenuItem.h (+3/-4) plugins/unityshell/src/QuicklistMenuItemSeparator.cpp (+6/-0) plugins/unityshell/src/QuicklistMenuItemSeparator.h (+2/-0) plugins/unityshell/src/QuicklistView.cpp (+165/-95) plugins/unityshell/src/QuicklistView.h (+4/-2) tests/autopilot/autopilot/emulators/unity/quicklist.py (+46/-7) tests/autopilot/autopilot/keybindings.py (+11/-4) tests/autopilot/autopilot/tests/test_quicklist.py (+205/-7) tests/unit/TestQuicklistMenuitems.cpp (+4/-0) |
||||||||||||
To merge this branch: | bzr merge lp:~3v1n0/unity/quicklist-keynav-fixes | ||||||||||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Thomi Richards (community) | Approve | ||
Review via email: mp+101623@code.launchpad.net |
Commit message
QuicklistView: fixed the quicklist key navigation, to make it consistent with Gtk menus navigation.
The prelight of the items is now completely controlled by QuicklistView and never by QuicklistMenuItem, the children can define now when they are slectable.
Description of the change
Quicklist key navigation was broken in multiple aspect, fixed the attached bugs to make it consistent with Gtk menu navigation.
The prelight of the items is now completely controlled by QuicklistView and never by QuicklistMenuItem, the children can define now when they are slectable.
Included Autopilot tests for the QL key navigation, and updated unit-tests for QuicklistMenuItem.
Thomi Richards (thomir-deactivatedaccount) wrote : | # |
Marco Trevisan (Treviño) (3v1n0) wrote : | # |
> Hi,
>
> 82 +bool
> 83 +QuicklistMenuI
>
> Coding style says these should be on the same name.
Yes I know and I generally do that, but not when I would be inconsistent with the style currently used on the class I'm editing, so I'd prefer not to change it here not to include unneeded changes.
This is something that should be fixed globally on a different coding-style-fixes branch (for Q).
> 667 + def get_items(self, visible_only=True):
> 668 + """Returns the quicklist items"""
> 669 + if visible_only:
> 670 + return self.get_
> 671 + else:
> 672 + return self.get_
> 673 +
> 674 @property
> 675 def items(self):
> 676 """Individual items in the quicklist."""
> 677 - return self.get_
> 678 + return self.get_items()
> 679 +
> 680 + @property
> 681 + def selectable_
> 682 + """Items that can be selected in the quicklist"""
> 683 + return filter(lambda it: it.selectable == True, self.items)
>
>
> This is a bit inconsistent - you have 'get_items' and 'selectable_items' - one
> a method, one a property. Please make them both the same (either methods or
> properties, I don't mind which).
Well, I wanted to keep both for a reason: the get_items() also allow you to get the invisible items of a quicklist, while the property will give you only the "real" result.
> 704 + Mouse()
>
>
> Unless you have good reason to, please don't pass 'animate=false'
> - this is only intended to be used in exceptional circumstances.
I used it to avoid that a quicklist item would have been selected when performing that, adding a possibility to get an invalid result.
> 856 + self.assertThat
> 857 + self.assertThat
>
> Please avoid negative assertions. Instead of saying "make sure foo is not
> None", say "Make sure foo is XYZ" - it makes your tests more specific and
> robust.
I wouldn't have used, but it's just a copy and paste from your code of the same class that I used not to be wrong :D
Thanks for the review, I'll update the branch soon.
Thomi Richards (thomir-deactivatedaccount) wrote : | # |
> > Hi,
> >
> > 82 +bool
> > 83 +QuicklistMenuI
> >
> > Coding style says these should be on the same name.
>
> Yes I know and I generally do that, but not when I would be inconsistent with
> the style currently used on the class I'm editing, so I'd prefer not to change
> it here not to include unneeded changes.
> This is something that should be fixed globally on a different coding-style-
> fixes branch (for Q).
OK, fine with me.
> >
> > This is a bit inconsistent - you have 'get_items' and 'selectable_items' -
> one
> > a method, one a property. Please make them both the same (either methods or
> > properties, I don't mind which).
>
> Well, I wanted to keep both for a reason: the get_items() also allow you to
> get the invisible items of a quicklist, while the property will give you only
> the "real" result.
>
You can still have both - just either make them both properties, or both methods. Probably making them both properties makes more sense.
> > 704 + Mouse()
> >
> >
> > Unless you have good reason to, please don't pass 'animate=false'
> > - this is only intended to be used in exceptional circumstances.
>
> I used it to avoid that a quicklist item would have been selected when
> performing that, adding a possibility to get an invalid result.
>
Fair enough.
> > 856 + self.assertThat
> > 857 + self.assertThat
> >
> > Please avoid negative assertions. Instead of saying "make sure foo is not
> > None", say "Make sure foo is XYZ" - it makes your tests more specific and
> > robust.
>
> I wouldn't have used, but it's just a copy and paste from your code of the
> same class that I used not to be wrong :D
>
Yeah, my code's not perfect either. Negative assertions are something that I've come to realise are really bad very recently, so older code is likely to be wrong.
Cheers,
Marco Trevisan (Treviño) (3v1n0) wrote : | # |
> This is a bit inconsistent - you have 'get_items' and 'selectable_items' - one
> a method, one a property. Please make them both the same (either methods or
> properties, I don't mind which).
Well, I did that because I also wanted to include a method to fetch the invisible items, but I'll remove it since no one uses it yet.
> Also, please don't use 'filter' in autopilot.
Yes you're right, I used it only because at the begginning selectable was a property that I defined only on the emulator, then I moved it to the introspection too, and I forgot to update the code.
I've updated the branch as well, I hope it's fine now ;)
Thomi Richards (thomir-deactivatedaccount) wrote : | # |
Hi,
There are still some issues with this I'm afraid:
672 + """Items that can be selected in the quicklist"""
Needs a full-stop at the end of the docstring.
727 + def mouse_click(self, button=1):
728 + logger.
729 + self.mouse_
730 + sleep(.25)
731 + assert(
732 + self._mouse.
733 + sleep(.25)
Several issues here:
1) The assert should be at the start of this method, not half way through.
2) You don't need the sleep() statements, please remove them.
3) You don't need to specify the click duration. Please just use "mouse.click()".
66 + for icon in self.launcher.
867 + if (icon.tooltip_text != self.ql_app.name):
868 + self.ql_
869 + else:
870 + self.keybinding
871 + self.addCleanup
872 + break
Instead of iterating over the model icons, please use the method on the launcher model, like this:
icon = self.launcher.
If you use the desktop Id then we won't have issues in didfferent locales.
885 + self.assertThat
899 + self.assertThat
908 + self.assertThat
917 + self.assertThat
931 + self.assertThat
940 + self.assertThat
949 + self.assertThat
958 + self.assertTrue
959 + self.assertThat
979 + self.assertTrue
984 + self.assertTrue
985 + self.assertThat
995 + self.assertTrue
1000 + self.assertTrue
1001 + self.assertThat
1010 + self.assertTrue
1017 + self.assertTrue
1018 + self.assertThat
1023 + self.assertThat
1027 + self.assertThat
1032 + self.assertTrue
1033 + self.assertThat
Please use the new Eventually() matcher for all of these. To import it:
from autopilot.matchers import Eventually
to use it, take this:
self.assertThat
and turn it into this:
self.assertThat
Eventually takes any mater, so you can also do things like this:
self.assertThat
Finally, please remove the sleep() statements. Using the Eventually(...) matcher removes the n...
Marco Trevisan (Treviño) (3v1n0) wrote : | # |
> Needs a full-stop at the end of the docstring.
Ouch!
> 727 + def mouse_click(self, button=1):
> Several issues here:
> 1) The assert should be at the start of this method, not half way through.
> 2) You don't need the sleep() statements, please remove them.
> 3) You don't need to specify the click duration. Please just use
> "mouse.click()".
Done
> 66 + for icon in self.launcher.
> Instead of iterating over the model icons, please use the method on the
> launcher model, like this:
>
> icon = self.launcher.
I can't... That was a keyboard selection... I must do that.
> If you use the desktop Id then we won't have issues in didfferent locales.
Using the bamf name we don't have these issue... Tested here, with Italian locale ;)
> 885 + self.assertThat
> Equals(
> Please use the new Eventually() matcher for all of these. To import it:
> Finally, please remove the sleep() statements. Using the Eventually(...)
> matcher removes the need for random sleep statements in your tests.
Done ;)
Thomi Richards (thomir-deactivatedaccount) : | # |
Unity Merger (unity-merger) wrote : | # |
There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.
Preview Diff
1 | === modified file 'plugins/unityshell/src/AbstractLauncherIcon.h' |
2 | --- plugins/unityshell/src/AbstractLauncherIcon.h 2012-03-30 15:24:34 +0000 |
3 | +++ plugins/unityshell/src/AbstractLauncherIcon.h 2012-04-19 00:02:18 +0000 |
4 | @@ -128,7 +128,7 @@ |
5 | |
6 | virtual void SetSortPriority(int priority) = 0; |
7 | |
8 | - virtual bool OpenQuicklist(bool default_to_first_item = false, int monitor = -1) = 0; |
9 | + virtual bool OpenQuicklist(bool select_first_item = false, int monitor = -1) = 0; |
10 | |
11 | virtual void SetCenter(nux::Point3 center, int monitor, nux::Geometry parent_geo) = 0; |
12 | |
13 | |
14 | === modified file 'plugins/unityshell/src/LauncherIcon.cpp' |
15 | --- plugins/unityshell/src/LauncherIcon.cpp 2012-04-10 00:39:14 +0000 |
16 | +++ plugins/unityshell/src/LauncherIcon.cpp 2012-04-19 00:02:18 +0000 |
17 | @@ -537,7 +537,7 @@ |
18 | _tooltip->ShowWindow(false); |
19 | } |
20 | |
21 | -bool LauncherIcon::OpenQuicklist(bool default_to_first_item, int monitor) |
22 | +bool LauncherIcon::OpenQuicklist(bool select_first_item, int monitor) |
23 | { |
24 | std::list<DbusmenuMenuitem*> menus = Menus(); |
25 | |
26 | @@ -583,8 +583,8 @@ |
27 | _quicklist->AddMenuItem(ql_item); |
28 | } |
29 | |
30 | - if (default_to_first_item) |
31 | - _quicklist->DefaultToFirstItem(); |
32 | + if (select_first_item) |
33 | + _quicklist->SelectFirstItem(); |
34 | |
35 | if (monitor < 0) |
36 | { |
37 | |
38 | === modified file 'plugins/unityshell/src/LauncherIcon.h' |
39 | --- plugins/unityshell/src/LauncherIcon.h 2012-04-03 22:18:36 +0000 |
40 | +++ plugins/unityshell/src/LauncherIcon.h 2012-04-19 00:02:18 +0000 |
41 | @@ -86,7 +86,7 @@ |
42 | |
43 | void ShowTooltip(); |
44 | |
45 | - bool OpenQuicklist(bool default_to_first_item = false, int monitor = -1); |
46 | + bool OpenQuicklist(bool select_first_item = false, int monitor = -1); |
47 | |
48 | void SetCenter(nux::Point3 center, int parent_monitor, nux::Geometry parent_geo); |
49 | |
50 | |
51 | === modified file 'plugins/unityshell/src/MockLauncherIcon.h' |
52 | --- plugins/unityshell/src/MockLauncherIcon.h 2012-03-30 15:24:34 +0000 |
53 | +++ plugins/unityshell/src/MockLauncherIcon.h 2012-04-19 00:02:18 +0000 |
54 | @@ -120,7 +120,7 @@ |
55 | |
56 | void SetSortPriority(int priority) { sort_priority_ = priority; } |
57 | |
58 | - bool OpenQuicklist(bool default_to_first_item = false, int monitor = -1) |
59 | + bool OpenQuicklist(bool select_first_item = false, int monitor = -1) |
60 | { |
61 | return false; |
62 | } |
63 | |
64 | === modified file 'plugins/unityshell/src/QuicklistMenuItem.cpp' |
65 | --- plugins/unityshell/src/QuicklistMenuItem.cpp 2012-03-14 06:24:18 +0000 |
66 | +++ plugins/unityshell/src/QuicklistMenuItem.cpp 2012-04-19 00:02:18 +0000 |
67 | @@ -89,10 +89,8 @@ |
68 | this); |
69 | } |
70 | |
71 | - mouse_down.connect(sigc::mem_fun(this, &QuicklistMenuItem::RecvMouseDown)); |
72 | mouse_up.connect(sigc::mem_fun(this, &QuicklistMenuItem::RecvMouseUp)); |
73 | mouse_click.connect(sigc::mem_fun(this, &QuicklistMenuItem::RecvMouseClick)); |
74 | - mouse_move.connect(sigc::mem_fun(this, &QuicklistMenuItem::RecvMouseMove)); |
75 | mouse_drag.connect(sigc::mem_fun(this, &QuicklistMenuItem::RecvMouseDrag)); |
76 | mouse_enter.connect(sigc::mem_fun(this, &QuicklistMenuItem::RecvMouseEnter)); |
77 | mouse_leave.connect(sigc::mem_fun(this, &QuicklistMenuItem::RecvMouseLeave)); |
78 | @@ -215,6 +213,12 @@ |
79 | DBUSMENU_MENUITEM_PROP_VISIBLE); |
80 | } |
81 | |
82 | +bool |
83 | +QuicklistMenuItem::GetSelectable() |
84 | +{ |
85 | + return GetVisible() && GetEnabled(); |
86 | +} |
87 | + |
88 | void QuicklistMenuItem::ItemActivated() |
89 | { |
90 | if (_debug) |
91 | @@ -333,11 +337,6 @@ |
92 | //self->ItemActivated (); |
93 | } |
94 | |
95 | -void QuicklistMenuItem::RecvMouseDown(int x, int y, unsigned long button_flags, unsigned long key_flags) |
96 | -{ |
97 | - |
98 | -} |
99 | - |
100 | void QuicklistMenuItem::RecvMouseUp(int x, int y, unsigned long button_flags, unsigned long key_flags) |
101 | { |
102 | sigMouseReleased.emit(this, x, y); |
103 | @@ -352,11 +351,6 @@ |
104 | sigMouseClick.emit(this, x, y); |
105 | } |
106 | |
107 | -void QuicklistMenuItem::RecvMouseMove(int x, int y, int dx, int dy, unsigned long button_flags, unsigned long key_flags) |
108 | -{ |
109 | - |
110 | -} |
111 | - |
112 | void QuicklistMenuItem::RecvMouseDrag(int x, int y, int dx, int dy, unsigned long button_flags, unsigned long key_flags) |
113 | { |
114 | sigMouseDrag.emit(this, x, y); |
115 | @@ -364,13 +358,11 @@ |
116 | |
117 | void QuicklistMenuItem::RecvMouseEnter(int x, int y, unsigned long button_flags, unsigned long key_flags) |
118 | { |
119 | - _prelight = true; |
120 | sigMouseEnter.emit(this); |
121 | } |
122 | |
123 | void QuicklistMenuItem::RecvMouseLeave(int x, int y, unsigned long button_flags, unsigned long key_flags) |
124 | { |
125 | - _prelight = false; |
126 | sigMouseLeave.emit(this); |
127 | } |
128 | |
129 | @@ -484,20 +476,14 @@ |
130 | |
131 | void QuicklistMenuItem::AddProperties(GVariantBuilder* builder) |
132 | { |
133 | - nux::Geometry abs_geo = GetAbsoluteGeometry(); |
134 | - |
135 | unity::variant::BuilderWrapper(builder) |
136 | - .add("absolute_x", abs_geo.x) |
137 | - .add("absolute_y", abs_geo.y) |
138 | + .add(GetAbsoluteGeometry()) |
139 | .add("text", _text) |
140 | - .add("x", GetBaseX()) |
141 | - .add("y", GetBaseY()) |
142 | - .add("width", GetBaseWidth()) |
143 | - .add("height", GetBaseHeight()) |
144 | .add("enabled", GetEnabled()) |
145 | .add("active", GetActive()) |
146 | .add("visible", GetVisible()) |
147 | - .add("lit", _prelight); |
148 | + .add("selectable", GetSelectable()) |
149 | + .add("selected", _prelight); |
150 | } |
151 | |
152 | } //NAMESPACE |
153 | |
154 | === modified file 'plugins/unityshell/src/QuicklistMenuItem.h' |
155 | --- plugins/unityshell/src/QuicklistMenuItem.h 2012-03-14 06:24:18 +0000 |
156 | +++ plugins/unityshell/src/QuicklistMenuItem.h 2012-04-19 00:02:18 +0000 |
157 | @@ -85,11 +85,13 @@ |
158 | virtual bool GetEnabled(); |
159 | virtual bool GetActive(); |
160 | virtual bool GetVisible(); |
161 | + virtual bool GetSelectable(); |
162 | |
163 | +protected: |
164 | // Introspection |
165 | std::string GetName() const; |
166 | void AddProperties(GVariantBuilder* builder); |
167 | -protected: |
168 | + |
169 | static const int ITEM_INDENT_ABS = 16; |
170 | static const int ITEM_CORNER_RADIUS_ABS = 3; |
171 | static const int ITEM_MARGIN = 4; |
172 | @@ -116,10 +118,8 @@ |
173 | |
174 | void RecvMouseEnter(int x, int y, unsigned long button_flags, unsigned long key_flags); |
175 | void RecvMouseLeave(int x, int y, unsigned long button_flags, unsigned long key_flags); |
176 | - void RecvMouseDown(int x, int y, unsigned long button_flags, unsigned long key_flags); |
177 | void RecvMouseUp(int x, int y, unsigned long button_flags, unsigned long key_flags); |
178 | void RecvMouseClick(int x, int y, unsigned long button_flags, unsigned long key_flags); |
179 | - void RecvMouseMove(int x, int y, int dx, int dy, unsigned long button_flags, unsigned long key_flags); |
180 | void RecvMouseDrag(int x, int y, int dx, int dy, unsigned long button_flags, unsigned long key_flags); |
181 | |
182 | sigc::signal<void, QuicklistMenuItem*> sigMouseEnter; |
183 | @@ -134,7 +134,6 @@ |
184 | nux::Color _color; //!< Item rendering color factor. |
185 | bool _debug; |
186 | |
187 | - |
188 | bool _prelight; //!< True when the mouse is over the item. |
189 | |
190 | void DrawText(nux::CairoGraphics* cairo, int width, int height, nux::Color const& color); |
191 | |
192 | === modified file 'plugins/unityshell/src/QuicklistMenuItemSeparator.cpp' |
193 | --- plugins/unityshell/src/QuicklistMenuItemSeparator.cpp 2012-03-14 06:24:18 +0000 |
194 | +++ plugins/unityshell/src/QuicklistMenuItemSeparator.cpp 2012-04-19 00:02:18 +0000 |
195 | @@ -59,6 +59,12 @@ |
196 | { |
197 | } |
198 | |
199 | +bool |
200 | +QuicklistMenuItemSeparator::GetSelectable() |
201 | +{ |
202 | + return false; |
203 | +} |
204 | + |
205 | void |
206 | QuicklistMenuItemSeparator::PreLayoutManagement() |
207 | { |
208 | |
209 | === modified file 'plugins/unityshell/src/QuicklistMenuItemSeparator.h' |
210 | --- plugins/unityshell/src/QuicklistMenuItemSeparator.h 2012-03-14 06:24:18 +0000 |
211 | +++ plugins/unityshell/src/QuicklistMenuItemSeparator.h 2012-04-19 00:02:18 +0000 |
212 | @@ -43,6 +43,8 @@ |
213 | |
214 | ~QuicklistMenuItemSeparator(); |
215 | |
216 | + virtual bool GetSelectable(); |
217 | + |
218 | protected: |
219 | |
220 | void PreLayoutManagement(); |
221 | |
222 | === modified file 'plugins/unityshell/src/QuicklistView.cpp' |
223 | --- plugins/unityshell/src/QuicklistView.cpp 2012-03-14 06:24:18 +0000 |
224 | +++ plugins/unityshell/src/QuicklistView.cpp 2012-04-19 00:02:18 +0000 |
225 | @@ -69,7 +69,7 @@ |
226 | , _bottom_padding_correction_single_item(-4) |
227 | , _offset_correction(-1) |
228 | , _cairo_text_has_changed(true) |
229 | - , _current_item_index(0) |
230 | + , _current_item_index(-1) |
231 | { |
232 | SetGeometry(nux::Geometry(0, 0, 1, 1)); |
233 | |
234 | @@ -138,13 +138,35 @@ |
235 | { |
236 | } |
237 | |
238 | +void |
239 | +QuicklistView::SelectItem(int index) |
240 | +{ |
241 | + CancelItemsPrelightStatus(); |
242 | + int target_item = -1; |
243 | + |
244 | + if (IsMenuItemSelectable(index)) |
245 | + { |
246 | + QuicklistMenuItem* menu_item = GetNthItems(index); |
247 | + |
248 | + if (menu_item) |
249 | + { |
250 | + target_item = index; |
251 | + menu_item->_prelight = true; |
252 | + } |
253 | + } |
254 | + |
255 | + if (_current_item_index != target_item) |
256 | + { |
257 | + _current_item_index = target_item; |
258 | + selection_change.emit(); |
259 | + QueueDraw(); |
260 | + } |
261 | +} |
262 | + |
263 | bool |
264 | -QuicklistView::IsMenuItemSeperator(int index) |
265 | +QuicklistView::IsMenuItemSelectable(int index) |
266 | { |
267 | - QuicklistMenuItem* menu_item = NULL; |
268 | - DbusmenuMenuitem* item = NULL; |
269 | - const gchar* label = NULL; |
270 | - bool result = false; |
271 | + QuicklistMenuItem* menu_item = nullptr; |
272 | |
273 | if (index < 0) |
274 | return false; |
275 | @@ -153,17 +175,7 @@ |
276 | if (!menu_item) |
277 | return false; |
278 | |
279 | - item = menu_item->_menuItem; |
280 | - if (!item) |
281 | - return false; |
282 | - |
283 | - label = dbusmenu_menuitem_property_get(item, DBUSMENU_MENUITEM_PROP_LABEL); |
284 | - if (!label) |
285 | - result = true; |
286 | - else |
287 | - result = false; |
288 | - |
289 | - return result; |
290 | + return menu_item->GetSelectable(); |
291 | } |
292 | |
293 | void |
294 | @@ -175,56 +187,103 @@ |
295 | { |
296 | switch (key_sym) |
297 | { |
298 | + // home or page up (highlight the first menu-hitem) |
299 | + case NUX_VK_PAGE_UP: |
300 | + case NUX_VK_HOME: |
301 | + { |
302 | + int num_items = GetNumItems(); |
303 | + int target_index = -1; |
304 | + |
305 | + do |
306 | + { |
307 | + ++target_index; |
308 | + } |
309 | + while (!IsMenuItemSelectable(target_index) && target_index < num_items); |
310 | + |
311 | + if (target_index < num_items) |
312 | + SelectItem(target_index); |
313 | + |
314 | + break; |
315 | + } |
316 | + // end or page down (highlight the last menu-hitem) |
317 | + case NUX_VK_PAGE_DOWN: |
318 | + case NUX_VK_END: |
319 | + { |
320 | + int target_index = GetNumItems(); |
321 | + |
322 | + do |
323 | + { |
324 | + --target_index; |
325 | + } |
326 | + while (!IsMenuItemSelectable(target_index) && target_index >= 0); |
327 | + |
328 | + if (target_index >= 0) |
329 | + SelectItem(target_index); |
330 | + |
331 | + break; |
332 | + } |
333 | // up (highlight previous menu-item) |
334 | case NUX_VK_UP: |
335 | case NUX_KP_UP: |
336 | - // protect against edge-case of first item being a separator |
337 | - if (_current_item_index == 1 && IsMenuItemSeperator(0)) |
338 | - break; |
339 | - |
340 | - if (_current_item_index > 0) |
341 | + { |
342 | + int target_index = _current_item_index; |
343 | + bool loop_back = false; |
344 | + |
345 | + if (target_index <= 0) |
346 | + target_index = GetNumItems(); |
347 | + |
348 | + do |
349 | { |
350 | - GetNthItems(_current_item_index)->_prelight = false; |
351 | - _current_item_index--; |
352 | - |
353 | - while (IsMenuItemSeperator(_current_item_index)) |
354 | - _current_item_index--; |
355 | - |
356 | - selection_change.emit(); |
357 | - |
358 | - GetNthItems(_current_item_index)->_prelight = true; |
359 | - QueueDraw(); |
360 | + --target_index; |
361 | + |
362 | + // If the first item is not selectable, we must loop from the last one |
363 | + if (!loop_back && target_index == 0 && !IsMenuItemSelectable(target_index)) |
364 | + { |
365 | + loop_back = true; |
366 | + target_index = GetNumItems() - 1; |
367 | + } |
368 | } |
369 | + while (!IsMenuItemSelectable(target_index) && target_index >= 0); |
370 | + |
371 | + if (target_index >= 0) |
372 | + SelectItem(target_index); |
373 | + |
374 | break; |
375 | + } |
376 | |
377 | // down (highlight next menu-item) |
378 | case NUX_VK_DOWN: |
379 | case NUX_KP_DOWN: |
380 | - // protect against edge-case of last item being a separator |
381 | - if (_current_item_index == (GetNumItems() - 1) && IsMenuItemSeperator(GetNumItems())) |
382 | - break; |
383 | - |
384 | - if (_current_item_index < GetNumItems() - 1) |
385 | + { |
386 | + int target_index = _current_item_index; |
387 | + int num_items = GetNumItems(); |
388 | + bool loop_back = false; |
389 | + |
390 | + if (target_index >= num_items - 1) |
391 | + target_index = -1; |
392 | + |
393 | + do |
394 | { |
395 | - GetNthItems(_current_item_index)->_prelight = false; |
396 | - _current_item_index++; |
397 | - |
398 | - while (IsMenuItemSeperator(_current_item_index)) |
399 | - _current_item_index++; |
400 | - |
401 | - selection_change.emit(); |
402 | - |
403 | - GetNthItems(_current_item_index)->_prelight = true; |
404 | - QueueDraw(); |
405 | + ++target_index; |
406 | + |
407 | + // If the last item is not selectable, we must loop from the first one |
408 | + if (!loop_back && target_index == num_items - 1 && !IsMenuItemSelectable(target_index)) |
409 | + { |
410 | + loop_back = true; |
411 | + target_index = 0; |
412 | + } |
413 | } |
414 | + while (!IsMenuItemSelectable(target_index) && target_index < num_items); |
415 | + |
416 | + if (target_index < num_items) |
417 | + SelectItem(target_index); |
418 | + |
419 | break; |
420 | + } |
421 | |
422 | // left (close quicklist, go back to laucher key-nav) |
423 | case NUX_VK_LEFT: |
424 | case NUX_KP_LEFT: |
425 | - _current_item_index = 0; |
426 | - selection_change.emit(); |
427 | - GetNthItems(_current_item_index)->_prelight = true; |
428 | Hide(); |
429 | // inform Launcher we switch back to Launcher key-nav |
430 | ubus_server_send_message(ubus_server_get_default(), |
431 | @@ -234,32 +293,23 @@ |
432 | |
433 | // esc (close quicklist, exit key-nav) |
434 | case NUX_VK_ESCAPE: |
435 | - _current_item_index = 0; |
436 | - selection_change.emit(); |
437 | - GetNthItems(_current_item_index)->_prelight = true; |
438 | Hide(); |
439 | // inform UnityScreen we leave key-nav completely |
440 | ubus_server_send_message(ubus_server_get_default(), |
441 | UBUS_LAUNCHER_END_KEY_NAV, |
442 | NULL); |
443 | - selection_change.emit(); |
444 | break; |
445 | |
446 | // <SPACE>, <RETURN> (activate selected menu-item) |
447 | case NUX_VK_SPACE: |
448 | case NUX_VK_ENTER: |
449 | case NUX_KP_ENTER: |
450 | - if (_current_item_index >= 0 && _current_item_index < GetNumItems() && |
451 | - GetNthItems(_current_item_index)->GetEnabled()) |
452 | + if (IsMenuItemSelectable(_current_item_index)) |
453 | { |
454 | - |
455 | dbusmenu_menuitem_handle_event(GetNthItems(_current_item_index)->_menuItem, |
456 | "clicked", |
457 | NULL, |
458 | 0); |
459 | - _current_item_index = 0; |
460 | - selection_change.emit(); |
461 | - GetNthItems(_current_item_index)->_prelight = true; |
462 | Hide(); |
463 | } |
464 | break; |
465 | @@ -365,6 +415,12 @@ |
466 | UnGrabKeyboard(); |
467 | //EnableInputWindow (false); |
468 | ShowWindow(false); |
469 | + |
470 | + if (_current_item_index != -1) |
471 | + { |
472 | + selection_change.emit(); |
473 | + _current_item_index = -1; |
474 | + } |
475 | } |
476 | } |
477 | |
478 | @@ -618,54 +674,53 @@ |
479 | void QuicklistView::RecvItemMouseDrag(QuicklistMenuItem* item, int x, int y) |
480 | { |
481 | nux::Geometry geo; |
482 | - std::list<QuicklistMenuItem*>::iterator it; |
483 | - for (it = _item_list.begin(); it != _item_list.end(); it++) |
484 | + |
485 | + for (auto it : _item_list) |
486 | { |
487 | - if (!(*it)->GetVisible()) |
488 | + int item_index = GetItemIndex(it); |
489 | + |
490 | + if (!IsMenuItemSelectable(item_index)) |
491 | continue; |
492 | |
493 | - geo = (*it)->GetGeometry(); |
494 | + geo = it->GetGeometry(); |
495 | geo.width = _item_layout->GetBaseWidth(); |
496 | |
497 | if (geo.IsPointInside(x + item->GetBaseX(), y + item->GetBaseY())) |
498 | { |
499 | - (*it)->_prelight = true; |
500 | - } |
501 | - else |
502 | - { |
503 | - (*it)->_prelight = false; |
504 | + SelectItem(item_index); |
505 | } |
506 | } |
507 | |
508 | - for (it = _default_item_list.begin(); it != _default_item_list.end(); it++) |
509 | + for (auto it : _default_item_list) |
510 | { |
511 | - if (!(*it)->GetVisible()) |
512 | + int item_index = GetItemIndex(it); |
513 | + |
514 | + if (!IsMenuItemSelectable(item_index)) |
515 | continue; |
516 | |
517 | - geo = (*it)->GetGeometry(); |
518 | + geo = it->GetGeometry(); |
519 | geo.width = _default_item_layout->GetBaseWidth(); |
520 | |
521 | if (geo.IsPointInside(x + item->GetBaseX(), y + item->GetBaseY())) |
522 | { |
523 | - (*it)->_prelight = true; |
524 | - } |
525 | - else |
526 | - { |
527 | - (*it)->_prelight = false; |
528 | + SelectItem(item_index); |
529 | } |
530 | } |
531 | - |
532 | - NeedRedraw(); |
533 | } |
534 | |
535 | void QuicklistView::RecvItemMouseEnter(QuicklistMenuItem* item) |
536 | { |
537 | - NeedRedraw(); |
538 | + int item_index = GetItemIndex(item); |
539 | + |
540 | + SelectItem(item_index); |
541 | } |
542 | |
543 | void QuicklistView::RecvItemMouseLeave(QuicklistMenuItem* item) |
544 | { |
545 | - NeedRedraw(); |
546 | + int item_index = GetItemIndex(item); |
547 | + |
548 | + if (item_index < 0 || item_index == _current_item_index) |
549 | + SelectItem(-1); |
550 | } |
551 | |
552 | void QuicklistView::RecvMouseDown(int x, int y, unsigned long button_flags, unsigned long key_flags) |
553 | @@ -786,14 +841,37 @@ |
554 | if (_item_list.size() > 0) |
555 | i = _item_list.size() - 1; |
556 | std::list<QuicklistMenuItem*>::iterator it; |
557 | - for (it = _item_list.begin(); it != _item_list.end(); i++, it++) |
558 | + for (it = _default_item_list.begin(); it != _default_item_list.end(); i++, it++) |
559 | { |
560 | if (i == index) |
561 | return *it; |
562 | } |
563 | } |
564 | |
565 | - return 0; |
566 | + return nullptr; |
567 | +} |
568 | + |
569 | +int QuicklistView::GetItemIndex(QuicklistMenuItem* item) |
570 | +{ |
571 | + int index = -1; |
572 | + |
573 | + for (auto it : _item_list) |
574 | + { |
575 | + ++index; |
576 | + |
577 | + if (it == item) |
578 | + return index; |
579 | + } |
580 | + |
581 | + for (auto it : _default_item_list) |
582 | + { |
583 | + ++index; |
584 | + |
585 | + if (it == item) |
586 | + return index; |
587 | + } |
588 | + |
589 | + return index; |
590 | } |
591 | |
592 | QuicklistMenuItemType QuicklistView::GetNthType(int index) |
593 | @@ -801,6 +879,7 @@ |
594 | QuicklistMenuItem* item = GetNthItems(index); |
595 | if (item) |
596 | return item->GetItemType(); |
597 | + |
598 | return MENUITEM_TYPE_UNKNOWN; |
599 | } |
600 | |
601 | @@ -809,13 +888,9 @@ |
602 | return _item_list; |
603 | } |
604 | |
605 | -void QuicklistView::DefaultToFirstItem() |
606 | +void QuicklistView::SelectFirstItem() |
607 | { |
608 | - if (GetNumItems() >= 1) |
609 | - { |
610 | - GetNthItems(0)->_prelight = true; |
611 | - QueueDraw(); |
612 | - } |
613 | + SelectItem(0); |
614 | } |
615 | |
616 | void ql_tint_dot_hl(cairo_t* cr, |
617 | @@ -1400,15 +1475,10 @@ |
618 | |
619 | void QuicklistView::AddProperties(GVariantBuilder* builder) |
620 | { |
621 | - nux::Geometry abs_geo = GetAbsoluteGeometry(); |
622 | - |
623 | variant::BuilderWrapper(builder) |
624 | - .add("x", abs_geo.x) |
625 | - .add("y", abs_geo.y) |
626 | + .add(GetAbsoluteGeometry()) |
627 | .add("base_x", GetBaseX()) |
628 | .add("base_y", GetBaseY()) |
629 | - .add("width", GetBaseWidth()) |
630 | - .add("height", GetBaseHeight()) |
631 | .add("active", IsVisible()); |
632 | } |
633 | |
634 | |
635 | === modified file 'plugins/unityshell/src/QuicklistView.h' |
636 | --- plugins/unityshell/src/QuicklistView.h 2012-03-14 06:24:18 +0000 |
637 | +++ plugins/unityshell/src/QuicklistView.h 2012-04-19 00:02:18 +0000 |
638 | @@ -62,8 +62,9 @@ |
639 | int GetNumItems(); |
640 | QuicklistMenuItem* GetNthItems(int index); |
641 | QuicklistMenuItemType GetNthType(int index); |
642 | + int GetItemIndex(QuicklistMenuItem* item); |
643 | std::list<QuicklistMenuItem*> GetChildren(); |
644 | - void DefaultToFirstItem(); |
645 | + void SelectFirstItem(); |
646 | |
647 | void TestMenuItems(DbusmenuMenuitem* root); |
648 | |
649 | @@ -130,7 +131,8 @@ |
650 | //! Check the mouse up event sent by an item. Detect the item where the mous is and emit the appropriate signal. |
651 | void CheckAndEmitItemSignal(int x, int y); |
652 | |
653 | - bool IsMenuItemSeperator(int index); |
654 | + void SelectItem(int index); |
655 | + bool IsMenuItemSelectable(int index); |
656 | |
657 | //nux::CairoGraphics* _cairo_graphics; |
658 | int _anchorX; |
659 | |
660 | === modified file 'tests/autopilot/autopilot/emulators/unity/quicklist.py' |
661 | --- tests/autopilot/autopilot/emulators/unity/quicklist.py 2012-03-06 21:26:11 +0000 |
662 | +++ tests/autopilot/autopilot/emulators/unity/quicklist.py 2012-04-19 00:02:18 +0000 |
663 | @@ -23,7 +23,12 @@ |
664 | @property |
665 | def items(self): |
666 | """Individual items in the quicklist.""" |
667 | - return self.get_children_by_type(QuicklistMenuItem) |
668 | + return self.get_children_by_type(QuicklistMenuItem, visible=True) |
669 | + |
670 | + @property |
671 | + def selectable_items(self): |
672 | + """Items that can be selected in the quicklist.""" |
673 | + return self.get_children_by_type(QuicklistMenuItem, visible=True, selectable=True) |
674 | |
675 | def get_quicklist_item_by_text(self, text): |
676 | """Returns a QuicklistMenuItemLabel object with the given text, or None.""" |
677 | @@ -40,17 +45,51 @@ |
678 | if not isinstance(item, QuicklistMenuItem): |
679 | raise TypeError("Item must be a subclass of QuicklistMenuItem") |
680 | |
681 | - logger.debug("Clicking on quicklist item %r", item) |
682 | - mouse = Mouse() |
683 | - mouse.move(self.x + item.x + (item.width / 2), |
684 | - self.y + item.y + (item.height / 2)) |
685 | - sleep(0.25) |
686 | - mouse.click() |
687 | + item.mouse_click() |
688 | + |
689 | + def move_mouse_to_right(self): |
690 | + """Moves the mouse outside the quicklist""" |
691 | + logger.debug("Moving mouse outside the quicklist %r", self) |
692 | + target_x = self.x + self.width + 10 |
693 | + target_y = self.y + self.height / 2 |
694 | + Mouse().move(target_x, target_y, animate=False) |
695 | + |
696 | + @property |
697 | + def selected_item(self): |
698 | + items = self.get_children_by_type(QuicklistMenuItem, selected=True) |
699 | + assert(len(items) <= 1) |
700 | + return items[0] if items else None |
701 | + |
702 | + @property |
703 | + def geometry(self): |
704 | + """Returns a tuple of (x,y,w,h) for the quicklist.""" |
705 | + return (self.x, self.y, self.width, self.height) |
706 | |
707 | |
708 | class QuicklistMenuItem(UnityIntrospectionObject): |
709 | """Represents a single item in a quicklist.""" |
710 | |
711 | + def __init__(self, *args, **kwargs): |
712 | + super(QuicklistMenuItem, self).__init__(*args, **kwargs) |
713 | + self._mouse = Mouse() |
714 | + |
715 | + @property |
716 | + def geometry(self): |
717 | + """Returns a tuple of (x,y,w,h) for the quicklist item.""" |
718 | + return (self.x, self.y, self.width, self.height) |
719 | + |
720 | + def mouse_move_to(self): |
721 | + assert(self.visible) |
722 | + logger.debug("Moving mouse over quicklist item %r", self) |
723 | + target_x = self.x + self.width / 2 |
724 | + target_y = self.y + self.height / 2 |
725 | + self._mouse.move(target_x, target_y, rate=20, time_between_events=0.005) |
726 | + |
727 | + def mouse_click(self, button=1): |
728 | + logger.debug("Clicking on quicklist item %r", self) |
729 | + self.mouse_move_to() |
730 | + self._mouse.click() |
731 | + |
732 | |
733 | class QuicklistMenuItemLabel(QuicklistMenuItem): |
734 | """Represents a text label inside a quicklist.""" |
735 | |
736 | === modified file 'tests/autopilot/autopilot/keybindings.py' |
737 | --- tests/autopilot/autopilot/keybindings.py 2012-04-06 01:28:24 +0000 |
738 | +++ tests/autopilot/autopilot/keybindings.py 2012-04-19 00:02:18 +0000 |
739 | @@ -56,7 +56,14 @@ |
740 | "launcher/switcher/prev": "Shift+Tab", |
741 | "launcher/switcher/down": "Down", |
742 | "launcher/switcher/up": "Up", |
743 | - # Panel |
744 | + # Quicklist: |
745 | + "quicklist/keynav/first": "Home", |
746 | + "quicklist/keynav/last": "End", |
747 | + "quicklist/keynav/next": "Down", |
748 | + "quicklist/keynav/prev": "Up", |
749 | + "quicklist/keynav/activate": "Enter", |
750 | + "quicklist/keynav/exit": "Escape", |
751 | + # Panel: |
752 | "panel/show_menus": "Alt", |
753 | "panel/open_first_menu": ('unityshell', 'panel_first_menu'), |
754 | "panel/next_indicator": "Right", |
755 | @@ -65,12 +72,12 @@ |
756 | "dash/reveal": "Super", |
757 | "dash/lens/next": "Ctrl+Tab", |
758 | "dash/lens/prev": "Ctrl+Shift+Tab", |
759 | - # Lenses |
760 | + # Lenses: |
761 | "lens_reveal/command": ("unityshell", "execute_command"), |
762 | "lens_reveal/apps": "Super+a", |
763 | "lens_reveal/files": "Super+f", |
764 | "lens_reveal/music": "Super+m", |
765 | - # Hud |
766 | + # Hud: |
767 | "hud/reveal": ("unityshell", "show_hud"), |
768 | # Switcher: |
769 | "switcher/reveal_normal": ("unityshell", "alt_tab_forward"), |
770 | @@ -96,7 +103,7 @@ |
771 | "workspace/move_right": ("wall", "right_key"), |
772 | "workspace/move_up": ("wall", "up_key"), |
773 | "workspace/move_down": ("wall", "down_key"), |
774 | - # Window management |
775 | + # Window management: |
776 | "window/show_desktop" : ("core", "show_desktop_key"), |
777 | "window/minimize": ("core", "minimize_window_key"), |
778 | "window/maximize": ("core", "maximize_window_key"), |
779 | |
780 | === modified file 'tests/autopilot/autopilot/tests/test_quicklist.py' |
781 | --- tests/autopilot/autopilot/tests/test_quicklist.py 2012-04-02 20:18:58 +0000 |
782 | +++ tests/autopilot/autopilot/tests/test_quicklist.py 2012-04-19 00:02:18 +0000 |
783 | @@ -1,16 +1,19 @@ |
784 | # -*- Mode: Python; coding: utf-8; indent-tabs-mode: nil; tab-width: 4 -*- |
785 | # Copyright 2012 Canonical |
786 | -# Author: Thomi Richards |
787 | +# Author: Thomi Richards, |
788 | +# Marco Trevisan (Treviño) |
789 | # |
790 | # This program is free software: you can redistribute it and/or modify it |
791 | # under the terms of the GNU General Public License version 3, as published |
792 | # by the Free Software Foundation. |
793 | |
794 | import os.path |
795 | -from testtools.matchers import Not, Is, Contains |
796 | +from testtools.matchers import Contains, Equals, NotEquals |
797 | from xdg.DesktopEntry import DesktopEntry |
798 | +from time import sleep |
799 | |
800 | from autopilot.emulators.unity.quicklist import QuicklistMenuItemLabel |
801 | +from autopilot.matchers import Eventually |
802 | from autopilot.tests import AutopilotTestCase |
803 | |
804 | |
805 | @@ -26,22 +29,23 @@ |
806 | self.start_app(self.app_name) |
807 | |
808 | # load the desktop file from disk: |
809 | - desktop_file = os.path.join('/usr/share/applications', |
810 | - self.KNOWN_APPS[self.app_name]['desktop-file'] |
811 | - ) |
812 | + desktop_id = self.KNOWN_APPS[self.app_name]['desktop-file'] |
813 | + desktop_file = os.path.join('/usr/share/applications', desktop_id) |
814 | de = DesktopEntry(desktop_file) |
815 | # get the launcher icon from the launcher: |
816 | - launcher_icon = self.launcher.model.get_icon_by_tooltip_text(de.getName()) |
817 | - self.assertThat(launcher_icon, Not(Is(None))) |
818 | + launcher_icon = self.launcher.model.get_icon_by_desktop_id(desktop_id) |
819 | + self.assertThat(launcher_icon, NotEquals(None)) |
820 | |
821 | # open the icon quicklist, and get all the text labels: |
822 | launcher = self.launcher.get_launcher_for_monitor(0) |
823 | launcher.click_launcher_icon(launcher_icon, button=3) |
824 | + self.addCleanup(self.keyboard.press_and_release, "Escape") |
825 | ql = launcher_icon.get_quicklist() |
826 | ql_item_texts = [i.text for i in ql.items if type(i) is QuicklistMenuItemLabel] |
827 | |
828 | # iterate over all the actions from the desktop file, make sure they're |
829 | # present in the quicklist texts. |
830 | + # FIXME, this doesn't work using a locale other than English. |
831 | actions = de.getActions() |
832 | for action in actions: |
833 | key = 'Desktop Action ' + action |
834 | @@ -50,3 +54,197 @@ |
835 | self.assertThat(ql_item_texts, Contains(name)) |
836 | |
837 | |
838 | +class QuicklistKeyNavigationTests(AutopilotTestCase): |
839 | + """Tests for the quicklist key navigation.""" |
840 | + |
841 | + def setUp(self): |
842 | + super(QuicklistKeyNavigationTests, self).setUp() |
843 | + |
844 | + self.ql_app = self.start_app("Text Editor") |
845 | + |
846 | + self.ql_launcher_icon = self.launcher.model.get_icon_by_desktop_id(self.ql_app.desktop_file) |
847 | + self.assertThat(self.ql_launcher_icon, NotEquals(None)) |
848 | + |
849 | + self.ql_launcher = self.launcher.get_launcher_for_monitor(0) |
850 | + |
851 | + def open_quicklist_with_mouse(self): |
852 | + """Opens a quicklist with the mouse.""" |
853 | + self.ql_launcher.click_launcher_icon(self.ql_launcher_icon, button=3) |
854 | + self.addCleanup(self.keyboard.press_and_release, "Escape") |
855 | + self.quicklist = self.ql_launcher_icon.get_quicklist() |
856 | + self.assertThat(self.quicklist, NotEquals(None)) |
857 | + self.quicklist.move_mouse_to_right() |
858 | + self.assertThat(self.quicklist.selected_item, Equals(None)) |
859 | + |
860 | + def open_quicklist_with_keyboard(self): |
861 | + """Opens a quicklist using the keyboard.""" |
862 | + self.screen_geo.move_mouse_to_monitor(0) |
863 | + self.ql_launcher.key_nav_start() |
864 | + self.addCleanup(self.ql_launcher.key_nav_cancel) |
865 | + |
866 | + for icon in self.launcher.model.get_launcher_icons(): |
867 | + if icon.tooltip_text != self.ql_app.name: |
868 | + self.ql_launcher.key_nav_next() |
869 | + else: |
870 | + self.keybinding("launcher/keynav/open-quicklist") |
871 | + self.addCleanup(self.keybinding, "launcher/keynav/close-quicklist") |
872 | + break |
873 | + |
874 | + self.quicklist = self.ql_launcher_icon.get_quicklist() |
875 | + self.assertThat(self.quicklist, NotEquals(None)) |
876 | + self.assertThat(self.quicklist.selected_item, NotEquals(None)) |
877 | + |
878 | + def test_keynav_selects_first_item_when_unselected(self): |
879 | + """Home key MUST select the first selectable item in a quicklist.""" |
880 | + self.open_quicklist_with_mouse() |
881 | + |
882 | + self.keybinding("quicklist/keynav/first") |
883 | + |
884 | + expected_item = self.quicklist.selectable_items[0] |
885 | + self.assertThat(expected_item.selected, Eventually(Equals(True))) |
886 | + self.assertThat(self.quicklist.selected_item.id, Equals(expected_item.id)) |
887 | + |
888 | + def test_keynav_selects_first_item_when_selected(self): |
889 | + """Home key MUST select the first selectable item in a quicklist when |
890 | + another item is selected. |
891 | + """ |
892 | + self.open_quicklist_with_mouse() |
893 | + mouse_item = self.quicklist.selectable_items[-1] |
894 | + mouse_item.mouse_move_to() |
895 | + self.assertThat(mouse_item.selected, Eventually(Equals(True))) |
896 | + |
897 | + self.keybinding("quicklist/keynav/first") |
898 | + |
899 | + expected_item = self.quicklist.selectable_items[0] |
900 | + self.assertThat(expected_item.selected, Eventually(Equals(True))) |
901 | + self.assertThat(self.quicklist.selected_item.id, Equals(expected_item.id)) |
902 | + |
903 | + def test_keynav_next_selects_first_item_when_unselected(self): |
904 | + """Down key MUST select the first valid item when nothing is selected.""" |
905 | + self.open_quicklist_with_mouse() |
906 | + |
907 | + self.keybinding("quicklist/keynav/next") |
908 | + |
909 | + expected_item = self.quicklist.selectable_items[0] |
910 | + self.assertThat(expected_item.selected, Eventually(Equals(True))) |
911 | + self.assertThat(self.quicklist.selected_item.id, Equals(expected_item.id)) |
912 | + |
913 | + def test_keynav_selects_last_item_when_unselected(self): |
914 | + """End key MUST select the last selectable item in a quicklist.""" |
915 | + self.open_quicklist_with_mouse() |
916 | + |
917 | + self.keybinding("quicklist/keynav/last") |
918 | + |
919 | + expected_item = self.quicklist.selectable_items[-1] |
920 | + self.assertThat(expected_item.selected, Eventually(Equals(True))) |
921 | + self.assertThat(self.quicklist.selected_item.id, Equals(expected_item.id)) |
922 | + |
923 | + def test_keynav_selects_last_item_when_selected(self): |
924 | + """End key MUST select the last selectable item in a quicklist when |
925 | + another item is selected. |
926 | + """ |
927 | + self.open_quicklist_with_mouse() |
928 | + mouse_item = self.quicklist.selectable_items[0] |
929 | + mouse_item.mouse_move_to() |
930 | + self.assertThat(mouse_item.selected, Eventually(Equals(True))) |
931 | + |
932 | + self.keybinding("quicklist/keynav/last") |
933 | + |
934 | + expected_item = self.quicklist.selectable_items[-1] |
935 | + self.assertThat(expected_item.selected, Eventually(Equals(True))) |
936 | + self.assertThat(self.quicklist.selected_item.id, Equals(expected_item.id)) |
937 | + |
938 | + def test_keynav_prev_selects_last_item_when_unselected(self): |
939 | + """Up key MUST select the last valid item when nothing is selected.""" |
940 | + self.open_quicklist_with_mouse() |
941 | + |
942 | + self.keybinding("quicklist/keynav/prev") |
943 | + |
944 | + expected_item = self.quicklist.selectable_items[-1] |
945 | + self.assertThat(expected_item.selected, Eventually(Equals(True))) |
946 | + self.assertThat(self.quicklist.selected_item.id, Equals(expected_item.id)) |
947 | + |
948 | + def test_launcher_keynav_selects_first_item(self): |
949 | + """The first selectable item of the quicklist must be selected when |
950 | + opening the quicklist using the launcher key navigation. |
951 | + """ |
952 | + self.open_quicklist_with_keyboard() |
953 | + |
954 | + expected_item = self.quicklist.selectable_items[0] |
955 | + self.assertThat(expected_item.selected, Eventually(Equals(True))) |
956 | + self.assertThat(self.quicklist.selected_item.id, Equals(expected_item.id)) |
957 | + |
958 | + def test_keynav_next_selection_works(self): |
959 | + """Down key MUST select the next valid item.""" |
960 | + self.open_quicklist_with_mouse() |
961 | + |
962 | + for item in self.quicklist.selectable_items: |
963 | + self.keybinding("quicklist/keynav/next") |
964 | + self.assertThat(item.selected, Eventually(Equals(True))) |
965 | + self.assertThat(self.quicklist.selected_item.id, Equals(item.id)) |
966 | + |
967 | + def test_keynav_prev_selection_works(self): |
968 | + """Up key MUST select the previous valid item.""" |
969 | + self.open_quicklist_with_mouse() |
970 | + |
971 | + for item in reversed(self.quicklist.selectable_items): |
972 | + self.keybinding("quicklist/keynav/prev") |
973 | + self.assertThat(item.selected, Eventually(Equals(True))) |
974 | + self.assertThat(self.quicklist.selected_item.id, Equals(item.id)) |
975 | + |
976 | + def test_keynav_prev_is_cyclic(self): |
977 | + """Up key MUST select the last item, when the first one is selected.""" |
978 | + self.open_quicklist_with_mouse() |
979 | + |
980 | + mouse_item = self.quicklist.selectable_items[0] |
981 | + mouse_item.mouse_move_to() |
982 | + self.assertThat(mouse_item.selected, Eventually(Equals(True))) |
983 | + |
984 | + self.keybinding("quicklist/keynav/prev") |
985 | + expected_item = self.quicklist.selectable_items[-1] |
986 | + self.assertThat(expected_item.selected, Eventually(Equals(True))) |
987 | + self.assertThat(self.quicklist.selected_item.id, Equals(expected_item.id)) |
988 | + |
989 | + def test_keynav_next_is_cyclic(self): |
990 | + """Down key MUST select the first item, when the last one is selected.""" |
991 | + self.open_quicklist_with_mouse() |
992 | + |
993 | + mouse_item = self.quicklist.selectable_items[-1] |
994 | + mouse_item.mouse_move_to() |
995 | + self.assertThat(mouse_item.selected, Eventually(Equals(True))) |
996 | + |
997 | + self.keybinding("quicklist/keynav/next") |
998 | + expected_item = self.quicklist.selectable_items[0] |
999 | + self.assertThat(expected_item.selected, Eventually(Equals(True))) |
1000 | + self.assertThat(self.quicklist.selected_item.id, Equals(expected_item.id)) |
1001 | + |
1002 | + def test_keynav_mouse_interaction(self): |
1003 | + """Tests that the interaction between key-navigation and mouse works as |
1004 | + expected. See bug #911561. |
1005 | + """ |
1006 | + self.open_quicklist_with_mouse() |
1007 | + mouse_item = self.quicklist.selectable_items[-1] |
1008 | + mouse_item.mouse_move_to() |
1009 | + self.assertThat(mouse_item.selected, Eventually(Equals(True))) |
1010 | + |
1011 | + self.keybinding("quicklist/keynav/prev") |
1012 | + sleep(.1) |
1013 | + self.keybinding("quicklist/keynav/prev") |
1014 | + |
1015 | + key_item = self.quicklist.selectable_items[-3] |
1016 | + self.assertThat(key_item.selected, Eventually(Equals(True))) |
1017 | + self.assertThat(self.quicklist.selected_item.id, Equals(key_item.id)) |
1018 | + |
1019 | + # Moving the mouse horizontally doesn't change the selection |
1020 | + self.mouse.move(mouse_item.x + mouse_item.width - 10, mouse_item.y + mouse_item.height / 2) |
1021 | + self.assertThat(self.quicklist.selected_item.id, Equals(key_item.id)) |
1022 | + |
1023 | + # Moving the mouse outside doesn't change the selection |
1024 | + self.mouse.move(mouse_item.x + mouse_item.width + 50, mouse_item.y + mouse_item.height / 2) |
1025 | + self.assertThat(self.quicklist.selected_item.id, Equals(key_item.id)) |
1026 | + |
1027 | + # Moving the mouse to another entry, changes the selection |
1028 | + mouse_item = self.quicklist.selectable_items[-2] |
1029 | + mouse_item.mouse_move_to() |
1030 | + self.assertThat(mouse_item.selected, Eventually(Equals(True))) |
1031 | + self.assertThat(self.quicklist.selected_item.id, Equals(mouse_item.id)) |
1032 | |
1033 | === modified file 'tests/unit/TestQuicklistMenuitems.cpp' |
1034 | --- tests/unit/TestQuicklistMenuitems.cpp 2012-03-14 06:24:18 +0000 |
1035 | +++ tests/unit/TestQuicklistMenuitems.cpp 2012-04-19 00:02:18 +0000 |
1036 | @@ -94,6 +94,7 @@ |
1037 | g_assert_cmpstr(qlCheckmarkItem->GetLabel(), == , "Unchecked"); |
1038 | g_assert_cmpint(qlCheckmarkItem->GetEnabled(), == , false); |
1039 | g_assert_cmpint(qlCheckmarkItem->GetActive(), == , false); |
1040 | + g_assert_cmpint(qlCheckmarkItem->GetSelectable(), == , false); |
1041 | g_assert_cmpint(qlCheckmarkItem->IsMarkupEnabled(), == , false); |
1042 | |
1043 | //qlCheckmarkItem->sigChanged.connect (sigc::mem_fun (pointerToCallerClass, |
1044 | @@ -129,6 +130,7 @@ |
1045 | |
1046 | g_assert_cmpstr(qlLabelItem->GetLabel(), == , "A Label"); |
1047 | g_assert_cmpint(qlLabelItem->GetEnabled(), == , true); |
1048 | + g_assert_cmpint(qlLabelItem->GetSelectable(), == , true); |
1049 | g_assert_cmpint(qlLabelItem->IsMarkupEnabled(), == , true); |
1050 | |
1051 | //qlLabelItem->sigChanged.connect (sigc::mem_fun (pointerToCallerClass, |
1052 | @@ -169,6 +171,7 @@ |
1053 | g_assert_cmpstr(qlRadioItem->GetLabel(), == , "Radio Active"); |
1054 | g_assert_cmpint(qlRadioItem->GetEnabled(), == , true); |
1055 | g_assert_cmpint(qlRadioItem->GetActive(), == , true); |
1056 | + g_assert_cmpint(qlRadioItem->GetSelectable(), == , true); |
1057 | g_assert_cmpint(qlRadioItem->IsMarkupEnabled(), == , true); |
1058 | |
1059 | //qlRadioItem->sigChanged.connect (sigc::mem_fun (pointerToCallerClass, |
1060 | @@ -198,6 +201,7 @@ |
1061 | qlSeparatorItem = new QuicklistMenuItemSeparator(item, true); |
1062 | |
1063 | g_assert_cmpint(qlSeparatorItem->GetEnabled(), == , true); |
1064 | + g_assert_cmpint(qlSeparatorItem->GetSelectable(), == , false); |
1065 | |
1066 | qlSeparatorItem->Dispose(); |
1067 | g_object_unref(item); |
Hi,
82 +bool tem::GetSelecta ble()
83 +QuicklistMenuI
199 +bool temSeparator: :GetSelectable( )
200 +QuicklistMenuI
238 +void :SelectItem( int index)
239 +QuicklistView:
263 bool :IsMenuItemSepe rator(int index) :IsMenuItemSele ctable( int index)
264 -QuicklistView:
265 +QuicklistView:
Coding style says these should be on the same name.
667 + def get_items(self, visible_only=True): children_ by_type( QuicklistMenuIt em, visible=True) children_ by_type( QuicklistMenuIt em) children_ by_type( QuicklistMenuIt em) items(self) :
668 + """Returns the quicklist items"""
669 + if visible_only:
670 + return self.get_
671 + else:
672 + return self.get_
673 +
674 @property
675 def items(self):
676 """Individual items in the quicklist."""
677 - return self.get_
678 + return self.get_items()
679 +
680 + @property
681 + def selectable_
682 + """Items that can be selected in the quicklist"""
683 + return filter(lambda it: it.selectable == True, self.items)
This is a bit inconsistent - you have 'get_items' and 'selectable_items' - one a method, one a property. Please make them both the same (either methods or properties, I don't mind which). Also, please don't use 'filter' in autopilot. In most places, list comprehensions are clearer. However, in this case, you don't need either - just pass more keyword filters to get_children_ by_type (like this):
@property items(self) : children_ by_type( QuicklistMenuIt em, visible=True, selectable=True)
def selectable_
return self.get_
704 + Mouse() .move(target_ x, target_y, animate=False)
Unless you have good reason to, please don't pass 'animate=false'
- this is only intended to be used in exceptional circumstances.
856 + self.assertThat (self.quicklist , Not(Is(None))) (self.quicklist .selected_ item, Not(Is(None)))
857 + self.assertThat
Please avoid negative assertions. Instead of saying "make sure foo is not None", say "Make sure foo is XYZ" - it makes your tests more specific and robust.
A few notes on docstrings: www.python. org/dev/ peps/pep- 0257/
1. All your docstrings need a full-stop (period) at the end.
2. Please try and make your docstrings PEP257 compliant: http://
3. You can rewrite many of your docstrings to be shorter, and more forceful. For example, instead of writing this:
"""Tests that the quicklist Home key selects the first valid item when
no item is selected
"""
write this:
"""Home key MUST select the first selectable item in a quicklist."""
Note that this uses the word "must" which is better than "should", "might", or "will", and follows more closely the code of the tests themselves.
Other than that, these look good.