Merge lp:~3v1n0/unity/super-tab-improvements into lp:unity

Proposed by Marco Trevisan (Treviño) on 2012-01-20
Status: Merged
Approved by: Marco Trevisan (Treviño) on 2012-01-27
Approved revision: 1844
Merged at revision: 1875
Proposed branch: lp:~3v1n0/unity/super-tab-improvements
Merge into: lp:unity
Prerequisite: lp:~3v1n0/unity/super-tab-switcher-shortcut-interaction
Diff against target: 284 lines (+126/-10)
5 files modified
manual-tests/SuperTab.txt (+33/-0)
plugins/unityshell/src/Launcher.cpp (+36/-7)
plugins/unityshell/src/Launcher.h (+1/-0)
plugins/unityshell/src/unityshell.cpp (+54/-3)
plugins/unityshell/src/unityshell.h (+2/-0)
To merge this branch: bzr merge lp:~3v1n0/unity/super-tab-improvements
Reviewer Review Type Date Requested Status
Sam Spilsbury (community) 2012-01-20 Approve on 2012-01-20
Review via email: mp+89371@code.launchpad.net

Description of the change

Improved the Super+Tab switcher making possible to escape from it using the Esc key and making the selection circular.

This branch mostly fixes bug #919018 and bug #919019 as requested by design on recent updates of the main bug #891620

To post a comment you must log in.
Sam Spilsbury (smspillaz) wrote :

49 - if ((*it)->GetCenter().y + - _icon_size/ 2 < GetGeometry().y)
50 + if ((*it)->GetCenter().y - _icon_size / 2 < GetGeometry().y)
51 + {

That should test for > 0. In the diff it initially looked like you were changing a + to a - :)

52 _launcher_drag_delta += (_icon_size + _space_between_icons);
53 + }
54 + else if ((*it)->GetCenter().y + _icon_size / 2 > GetGeometry().height)
55 + {
56 + _launcher_drag_delta -= (*it)->GetCenter().y + _icon_size/2 +
57 + _space_between_icons - GetGeometry().height;
58 + }
59 }

Other than that. Approve

review: Approve
Marco Trevisan (Treviño) (3v1n0) wrote :

> 49 - if ((*it)->GetCenter().y + - _icon_size/ 2 < GetGeometry().y)
> 50 + if ((*it)->GetCenter().y - _icon_size / 2 < GetGeometry().y)
> 51 + {
>
> That should test for > 0. In the diff it initially looked like you were
> changing a + to a - :)

What do you mean? You'd prefer to move GetGeometry().y to the left side?
The old code contained a copy&paste typo because there was a "+ -" operator that is just a "-" in fact.

Sam Spilsbury (smspillaz) wrote :

> > 49 - if ((*it)->GetCenter().y + - _icon_size/ 2 < GetGeometry().y)
> > 50 + if ((*it)->GetCenter().y - _icon_size / 2 < GetGeometry().y)
> > 51 + {
> >
> > That should test for > 0. In the diff it initially looked like you were
> > changing a + to a - :)
>
> What do you mean? You'd prefer to move GetGeometry().y to the left side?
> The old code contained a copy&paste typo because there was a "+ -" operator
> that is just a "-" in fact.

Oh whoops, looks like I didn't see the <.

The typo threw me off :)

Marco Trevisan (Treviño) (3v1n0) wrote :

This branch has been merged on lp:~unity-team/unity/unity.multi-launcher so let's close this review.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'manual-tests/SuperTab.txt'
2--- manual-tests/SuperTab.txt 2012-01-20 02:25:29 +0000
3+++ manual-tests/SuperTab.txt 2012-01-20 02:25:29 +0000
4@@ -42,3 +42,36 @@
5 Outcome:
6 Super+Tab switcher is initialized and the shortcut hint overlay is not shown
7 even keeping only super pressed until releasing it and pressing it again.
8+
9+Super Tab switcher cycling over launcher applications
10+-----------------------------------------------------
11+This test shows how the Super+Tab switcher should use a circular selection.
12+
13+#. Start with a clean screen
14+#. Press Super+Tab multiple times to make the selection to reach the bottom
15+ launcher icon
16+#. Press Tab again
17+
18+Outcome:
19+ The selection should go from the last launcher icon, to the first one.
20+ The launcher view should be expanded/moved if needed to show the highlighted item.
21+
22+#. Start with a clean screen
23+#. Press Super+Tab once to make the first launcher icon to highlight
24+#. Press Shift+Tab while keeping Super pressed
25+
26+Outcome:
27+ The selection should go from the first launcher icon to the last one.
28+ The launcher view should be expanded/moved if needed to show the highlighted item.
29+
30+Escaping from Super Tab switcher
31+--------------------------------
32+This test shows how to escape from the Super+Tab switcher.
33+
34+#. Start with a clean screen
35+#. Press Super+Tab one or more times to make the launcher switcher to initialize
36+#. Press the Escape key when still pressing Super
37+
38+Outcome:
39+ The Super Tab launcher switcher should terminate without performing any action
40+ The previously selected launcher item should be deselected.
41
42=== modified file 'plugins/unityshell/src/Launcher.cpp'
43--- plugins/unityshell/src/Launcher.cpp 2012-01-18 11:16:38 +0000
44+++ plugins/unityshell/src/Launcher.cpp 2012-01-20 02:25:29 +0000
45@@ -2606,9 +2606,17 @@
46 {
47 _current_icon_index = temp_current_icon_index;
48
49- if ((*it)->GetCenter().y + - _icon_size/ 2 < GetGeometry().y)
50+ if ((*it)->GetCenter().y - _icon_size / 2 < GetGeometry().y)
51+ {
52 _launcher_drag_delta += (_icon_size + _space_between_icons);
53+ }
54+ else if ((*it)->GetCenter().y + _icon_size / 2 > GetGeometry().height)
55+ {
56+ _launcher_drag_delta -= (*it)->GetCenter().y + _icon_size/2 +
57+ _space_between_icons - GetGeometry().height;
58+ }
59 }
60+
61 EnsureAnimation();
62 selection_change.emit();
63 }
64@@ -2634,7 +2642,13 @@
65 _current_icon_index = temp_current_icon_index;
66
67 if ((*it)->GetCenter().y + _icon_size / 2 > GetGeometry().height)
68+ {
69 _launcher_drag_delta -= (_icon_size + _space_between_icons);
70+ }
71+ else if ((*it)->GetCenter().y - _icon_size / 2 < GetGeometry().y)
72+ {
73+ _launcher_drag_delta += GetGeometry().y - ((*it)->GetCenter().y - _icon_size);
74+ }
75 }
76
77 EnsureAnimation();
78@@ -2684,6 +2698,15 @@
79 selection_change.emit();
80 }
81
82+void Launcher::KeySwitcherCancel()
83+{
84+ if (!_key_switcher_activated)
85+ return;
86+
87+ _current_icon_index = -1;
88+ KeySwitcherTerminate();
89+}
90+
91 bool Launcher::KeySwitcherIsActive()
92 {
93 return _key_switcher_activated;
94@@ -2694,6 +2717,11 @@
95 if (!_key_switcher_activated)
96 return;
97
98+ if (_current_icon_index == _model->Size() - 1)
99+ {
100+ _current_icon_index = -1;
101+ }
102+
103 SelectNextIcon();
104 }
105
106@@ -2702,6 +2730,11 @@
107 if (!_key_switcher_activated)
108 return;
109
110+ if (_current_icon_index == 0)
111+ {
112+ _current_icon_index = _model->Size();
113+ }
114+
115 SelectPreviousIcon();
116 }
117
118@@ -2861,13 +2894,9 @@
119 _start_dragicon_handle = g_timeout_add(START_DRAGICON_DURATION, &Launcher::StartIconDragTimeout, this);
120
121 launcher_icon->mouse_down.emit(nux::GetEventButton(button_flags));
122-
123- if (_key_switcher_activated)
124- {
125- _current_icon_index = -1;
126- KeySwitcherTerminate();
127- }
128 }
129+
130+ KeySwitcherCancel();
131 }
132
133 void Launcher::MouseUpLogic(int x, int y, unsigned long button_flags, unsigned long key_flags)
134
135=== modified file 'plugins/unityshell/src/Launcher.h'
136--- plugins/unityshell/src/Launcher.h 2012-01-17 15:41:30 +0000
137+++ plugins/unityshell/src/Launcher.h 2012-01-20 02:25:29 +0000
138@@ -210,6 +210,7 @@
139
140 void KeySwitcherActivate();
141 void KeySwitcherTerminate();
142+ void KeySwitcherCancel();
143 bool KeySwitcherIsActive();
144 void KeySwitcherNext();
145 void KeySwitcherPrevious();
146
147=== modified file 'plugins/unityshell/src/unityshell.cpp'
148--- plugins/unityshell/src/unityshell.cpp 2012-01-20 02:25:29 +0000
149+++ plugins/unityshell/src/unityshell.cpp 2012-01-20 02:25:29 +0000
150@@ -107,6 +107,7 @@
151 , _edge_trigger_handle(0)
152 , _redraw_handle(0)
153 , _edge_pointerY(0)
154+ , _escape_action(nullptr)
155 , newFocusedWindow(nullptr)
156 , doShellRepaint(false)
157 , allowWindowPaint(false)
158@@ -423,6 +424,25 @@
159 _shortcut_actions.push_back(action);
160 }
161
162+void UnityScreen::EnableCancelAction(bool enabled, int modifiers)
163+{
164+ if (enabled)
165+ {
166+ /* Create a new keybinding for the Escape key and the current modifiers */
167+ CompAction::KeyBinding binding(9, modifiers);
168+
169+ _escape_action = CompActionPtr(new CompAction());
170+ _escape_action->setKey(binding);
171+
172+ screen->addAction(_escape_action.get());
173+ }
174+ else if (!enabled && _escape_action.get())
175+ {
176+ screen->removeAction(_escape_action.get());
177+ _escape_action = nullptr;
178+ }
179+}
180+
181 void UnityScreen::nuxPrologue()
182 {
183 /* Vertex lighting isn't used in Unity, we disable that state as it could have
184@@ -1098,6 +1118,13 @@
185 launcher.startKeyNavMode();
186 _key_nav_mode_requested = false;
187 break;
188+ case ButtonPress:
189+ if (super_keypressed_)
190+ {
191+ launcher.KeySwitcherCancel();
192+ EnableCancelAction(false);
193+ }
194+ break;
195 case KeyPress:
196 {
197 KeySym key_sym;
198@@ -1109,7 +1136,7 @@
199 // we should just say "key_string[1] = 0" because that is the only
200 // thing that could possibly make sense here.
201 key_string[result] = 0;
202- if (super_keypressed_)
203+ if (super_keypressed_ && key_sym != XK_Escape)
204 {
205 g_idle_add([] (gpointer data) -> gboolean {
206 auto self = static_cast<UnityScreen*>(data);
207@@ -1270,9 +1297,11 @@
208 CompAction::State state,
209 CompOption::Vector& options)
210 {
211+ if (state & CompAction::StateCancel)
212+ return false;
213+
214 super_keypressed_ = false;
215 launcher_controller_->launcher().EndKeyShowLauncher();
216- launcher_controller_->launcher().KeySwitcherTerminate();
217
218 shortcut_controller_->SetEnabled(enable_shortcut_overlay_);
219 shortcut_controller_->Hide();
220@@ -1537,21 +1566,43 @@
221 Launcher& launcher = launcher_controller_->launcher();
222
223 if (!launcher.KeySwitcherIsActive())
224+ {
225+ EnableCancelAction(true, action->key().modifiers());
226 launcher.KeySwitcherActivate();
227+ }
228 else
229+ {
230 launcher.KeySwitcherNext();
231+ }
232
233+ action->setState(action->state() | CompAction::StateTermKey);
234 return false;
235 }
236+
237 bool UnityScreen::launcherSwitcherPrevInitiate(CompAction* action, CompAction::State state, CompOption::Vector& options)
238 {
239 launcher_controller_->launcher().KeySwitcherPrevious();
240
241 return false;
242 }
243+
244 bool UnityScreen::launcherSwitcherTerminate(CompAction* action, CompAction::State state, CompOption::Vector& options)
245 {
246- launcher_controller_->launcher().KeySwitcherTerminate();
247+ Launcher& launcher = launcher_controller_->launcher();
248+
249+ if (launcher.KeySwitcherIsActive())
250+ {
251+ if (state & CompAction::StateCancel)
252+ {
253+ launcher.KeySwitcherCancel();
254+ }
255+ else
256+ {
257+ launcher.KeySwitcherTerminate();
258+ }
259+
260+ EnableCancelAction(false);
261+ }
262
263 return false;
264 }
265
266=== modified file 'plugins/unityshell/src/unityshell.h'
267--- plugins/unityshell/src/unityshell.h 2012-01-20 02:25:29 +0000
268+++ plugins/unityshell/src/unityshell.h 2012-01-20 02:25:29 +0000
269@@ -225,6 +225,7 @@
270
271 void EnsureSuperKeybindings ();
272 void CreateSuperNewAction(char shortcut, bool use_shift=false, bool use_numpad=false);
273+ void EnableCancelAction(bool enabled, int modifiers = 0);
274
275 static gboolean initPluginActions(gpointer data);
276 void initLauncher();
277@@ -281,6 +282,7 @@
278 typedef std::shared_ptr<CompAction> CompActionPtr;
279 typedef std::vector<CompActionPtr> ShortcutActions;
280 ShortcutActions _shortcut_actions;
281+ CompActionPtr _escape_action;
282 bool super_keypressed_;
283
284 /* keyboard-nav mode */