Merge lp:~dx-unity/unity/unity.fix_702431 into lp:unity

Proposed by Sam Spilsbury on 2011-03-31
Status: Rejected
Rejected by: David Barth on 2011-04-07
Proposed branch: lp:~dx-unity/unity/unity.fix_702431
Merge into: lp:unity
Diff against target: 258 lines (+90/-13)
6 files modified
src/Launcher.cpp (+4/-5)
src/LauncherIcon.cpp (+51/-2)
src/PluginAdapter.cpp (+14/-0)
src/PluginAdapter.h (+5/-0)
src/unityshell.cpp (+15/-5)
src/unityshell.h (+1/-1)
To merge this branch: bzr merge lp:~dx-unity/unity/unity.fix_702431
Reviewer Review Type Date Requested Status
David Barth (community) Disapprove on 2011-04-06
Alejandro Piñeiro (community) Approve on 2011-04-05
Didier Roche Needs Fixing on 2011-04-01
Jason Smith (community) 2011-03-31 Approve on 2011-03-31
Rodrigo Moya 2011-03-31 Pending
Review via email: mp+55795@code.launchpad.net

Description of the change

Don't rely on the compiz grab to trigger keybindings but instead grab the keys ourselves.

The compiz grab behaviour is going to change such that the grab is automatically disabled again once the key has been handled, rather than once the key is released since this means that if we have a grab on <super> then gnome-do won't be able to use <super>space

To post a comment you must log in.
Jason Smith (jassmith) wrote :

Looks good from a technical standpoint, requesting a review for a11y changes.

review: Approve
lp:~dx-unity/unity/unity.fix_702431 updated on 2011-03-31
1047. By Sam Spilsbury on 2011-03-31

Don't mix display connections

Didier Roche (didrocks) wrote :

as discussed on IRC, !shortcuts_shown doesn't mean shortcuts are not active, so no need to difference the grab/ungrab case there ;)

review: Needs Fixing
lp:~dx-unity/unity/unity.fix_702431 updated on 2011-04-02
1048. By Sam Spilsbury on 2011-04-02

Move key grabbing into LauncherIcon::SetShortcut since we want to be
able to trigger shortcuts even when the launcher is hidden

Alejandro Piñeiro (apinheiro) wrote :

Adding myself on the review list. Jason Smith asked for a a11y review, and I implemented most of the a11y support on the launcher.

Alejandro Piñeiro (apinheiro) wrote :

Review done:

  * On my first test of this branch, the only difference with the current branch behaviour is that the Launcher doesn't announce always that the Launcher received the focus (so again bug 727133), just that the launcher icon receive the focus (that it mostly ok most of the cases)

  * Probably this change made a change on the event order.

  * Anyway, I realized that this branch is "old" compared with the current trunk (rev1047 vs rev1074)

  * After doing a merge with the trunk this seems to work fine.

*But*, it seems that right now the trunk includes this branch:
https://code.launchpad.net/~apinheiro/unity/a11y-review-places/+merge/55139

that Jay merged in my behalf but David told me to not merge (because it was a really big change, and the deadline was there).

So, as this needs some clarification about what a11y code is or not is here, I will set this as "Needs Information"

review: Needs Information
Alejandro Piñeiro (apinheiro) wrote :

I have just tested old revisions of unity (revno 1029 and revno 1050) and the "Launcher is not announced" regression is also there. Although it is also true that I'm using the last compiz (as was required for this proposal), and the last nux, what it is true is that this specific change doesn't seems to be the one causing the regression (as the one causing it seems to be already merged).

So, as the code seems ok, and the regression is not caused directly by this change, I think that it would be ok to approve it.

review: Approve
Didier Roche (didrocks) wrote :

75 + || !(g_ascii_isdigit ((gchar) old_shortcut)))
should be:
75 + || !(g_ascii_isdigit ((gchar) shortcut)))

David Barth (dbarth) wrote :

Update on the change: the grab change stays out. It's too late and the benefits of the change are too limited to risk destabilizing the rest of the stack.

I've reverted the compiz change already. The unity change should not be merged, but kept aside for later (an SRU, something else, we'll see).

review: Disapprove

Unmerged revisions

1048. By Sam Spilsbury on 2011-04-02

Move key grabbing into LauncherIcon::SetShortcut since we want to be
able to trigger shortcuts even when the launcher is hidden

1047. By Sam Spilsbury on 2011-03-31

Don't mix display connections

1046. By Sam Spilsbury on 2011-03-31

Fix typos

1045. By Sam Spilsbury on 2011-03-31

Don't rely on the implcit screengrab made by compiz to execute keybindings
while super is pressed, instead directly grab and ungrab those keys

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/Launcher.cpp'
2--- src/Launcher.cpp 2011-03-31 21:54:24 +0000
3+++ src/Launcher.cpp 2011-04-02 02:13:25 +0000
4@@ -614,7 +614,7 @@
5 _hide_machine->SetQuirk (LauncherHideMachine::KEY_NAV_ACTIVE, true);
6 _hover_machine->SetQuirk (LauncherHoverMachine::KEY_NAV_ACTIVE, true);
7 _hide_machine->SetQuirk (LauncherHideMachine::LAST_ACTION_ACTIVATE, false);
8-
9+
10 GrabKeyboard ();
11 GrabPointer ();
12
13@@ -1475,14 +1475,14 @@
14 bool tap_on_super;
15 bool shortcuts_shown = false;
16 clock_gettime (CLOCK_MONOTONIC, &current);
17-
18+
19 tap_on_super = (TimeDelta (&current, &_times[TIME_TAP_SUPER]) < SUPER_TAP_DURATION);
20
21 if (_hide_machine->GetQuirk (LauncherHideMachine::TRIGGER_BUTTON_DOWN))
22 shortcuts_shown = !tap_on_super;
23
24- _hover_machine->SetQuirk (LauncherHoverMachine::SHOTCUT_KEYS_VISIBLE, shortcuts_shown);
25-
26+ _hover_machine->SetQuirk (LauncherHoverMachine::SHOTCUT_KEYS_VISIBLE, shortcuts_shown);
27+
28 return tap_on_super;
29
30 }
31@@ -1502,7 +1502,6 @@
32
33 void Launcher::EndKeyShowLauncher ()
34 {
35-
36 _hide_machine->SetQuirk (LauncherHideMachine::TRIGGER_BUTTON_DOWN, false);
37 _hover_machine->SetQuirk (LauncherHoverMachine::SHOTCUT_KEYS_VISIBLE, false);
38 QueueDraw ();
39
40=== modified file 'src/LauncherIcon.cpp'
41--- src/LauncherIcon.cpp 2011-03-31 20:33:43 +0000
42+++ src/LauncherIcon.cpp 2011-04-02 02:13:25 +0000
43@@ -373,10 +373,59 @@
44 void
45 LauncherIcon::SetShortcut (guint64 shortcut)
46 {
47+ unsigned int old_shortcut = _shortcut;
48+ // also remove old shortcut
49+ if (_shortcut > 32)
50+ {
51+ unsigned int keycode = XKeysymToKeycode (screen->dpy (), _shortcut);
52+ unsigned int modifiers = modHandler->virtualToRealModMask (PluginAdapter::Default ()->GetShowLauncherKeyMods ());
53+
54+ for (unsigned int ignore = 0; ignore <= modHandler->ignoredModMask (); ignore++)
55+ {
56+ if (ignore & ~modHandler->ignoredModMask ())
57+ continue;
58+
59+ /* Keycode is always > 0, so we can always grab directly rather than having
60+ * to grab modifier trees */
61+
62+ XUngrabKey (screen->dpy (),
63+ keycode,
64+ modifiers | ignore,
65+ screen->root ());
66+
67+ _shortcut = 0;
68+ }
69+ }
70+
71 // only relocate a digit with a digit (don't overwrite other shortcuts)
72- if ((!_shortcut || (g_ascii_isdigit ((gchar)_shortcut)))
73- || !(g_ascii_isdigit ((gchar) shortcut)))
74+ if ((!old_shortcut || (g_ascii_isdigit ((gchar)old_shortcut)))
75+ || !(g_ascii_isdigit ((gchar) old_shortcut)))
76+ {
77 _shortcut = shortcut;
78+
79+ if (_shortcut > 32)
80+ {
81+ unsigned int keycode = XKeysymToKeycode (screen->dpy (), _shortcut);
82+ unsigned int modifiers = modHandler->virtualToRealModMask (PluginAdapter::Default ()->GetShowLauncherKeyMods ());
83+
84+ for (unsigned int ignore = 0; ignore <= modHandler->ignoredModMask (); ignore++)
85+ {
86+ if (ignore & ~modHandler->ignoredModMask ())
87+ continue;
88+
89+ /* Keycode is always > 0, so we can always grab directly rather than having
90+ * to grab modifier trees */
91+
92+ XGrabKey (screen->dpy (),
93+ keycode,
94+ modifiers | ignore,
95+ screen->root (),
96+ true,
97+ GrabModeAsync,
98+ GrabModeAsync);
99+ }
100+ }
101+ }
102 }
103
104 guint64
105
106=== modified file 'src/PluginAdapter.cpp'
107--- src/PluginAdapter.cpp 2011-04-01 20:04:14 +0000
108+++ src/PluginAdapter.cpp 2011-04-02 02:13:25 +0000
109@@ -52,6 +52,8 @@
110 _grab_show_action = 0;
111 _grab_hide_action = 0;
112 _grab_toggle_action = 0;
113+
114+ _show_launcher_action = 0;
115 }
116
117 PluginAdapter::~PluginAdapter()
118@@ -320,6 +322,18 @@
119 m_ExpoActionList.InitiateAll (argument, 0);
120 }
121
122+unsigned int
123+PluginAdapter::GetShowLauncherKeyMods ()
124+{
125+ /* Find the action used to trigger showing the launcher
126+ * and then return its key mods */
127+
128+ if (_show_launcher_action)
129+ return _show_launcher_action->key ().modifiers ();
130+ else
131+ return CompSuperMask;
132+}
133+
134 // WindowManager implementation
135 bool
136 PluginAdapter::IsWindowMaximized (guint xid)
137
138=== modified file 'src/PluginAdapter.h'
139--- src/PluginAdapter.h 2011-03-30 17:29:51 +0000
140+++ src/PluginAdapter.h 2011-04-02 02:13:25 +0000
141@@ -65,6 +65,8 @@
142 void SetHideHandlesAction (CompAction *action) { _grab_hide_action = action; }
143 void SetToggleHandlesAction (CompAction *action) { _grab_toggle_action = action; }
144
145+ void SetShowLauncherAction (CompAction *action) { _show_launcher_action = action; }
146+
147 void OnScreenGrabbed ();
148 void OnScreenUngrabbed ();
149
150@@ -74,6 +76,8 @@
151
152 void InitiateExpo ();
153 bool IsExpoActive ();
154+
155+ unsigned int GetShowLauncherKeyMods ();
156
157 void ShowGrabHandles (CompWindow *window, bool use_timer);
158 void HideGrabHandles (CompWindow *window);
159@@ -117,6 +121,7 @@
160 CompAction *_grab_show_action;
161 CompAction *_grab_hide_action;
162 CompAction *_grab_toggle_action;
163+ CompAction *_show_launcher_action;
164
165 static PluginAdapter *_default;
166 };
167
168=== modified file 'src/unityshell.cpp'
169--- src/unityshell.cpp 2011-03-31 21:45:39 +0000
170+++ src/unityshell.cpp 2011-04-02 02:13:25 +0000
171@@ -443,6 +443,12 @@
172 {
173 CompPlugin *p;
174
175+ /* Internal show launcher action */
176+
177+ PluginAdapter::Default ()->SetShowLauncherAction (&UnityScreen::get (screen)->optionGetShowLauncher ());
178+
179+ /* Expo */
180+
181 p = CompPlugin::find ("expo");
182
183 if (p)
184@@ -464,6 +470,8 @@
185 PluginAdapter::Default ()->SetExpoAction (expoActions);
186 }
187
188+ /* Scale */
189+
190 p = CompPlugin::find ("scale");
191
192 if (p)
193@@ -497,6 +505,8 @@
194 PluginAdapter::Default ()->SetScaleAction (scaleActions);
195 }
196
197+ /* Unity MT Grab Handles (AKA Love Handles) */
198+
199 p = CompPlugin::find ("unitymtgrabhandles");
200
201 if (p)
202@@ -822,7 +832,7 @@
203
204 UnityScreen::UnityScreen (CompScreen *screen) :
205 PluginClassHandler <UnityScreen, CompScreen> (screen),
206- screen (screen),
207+ compScreen (screen),
208 cScreen (CompositeScreen::get (screen)),
209 gScreen (GLScreen::get (screen)),
210 relayoutSourceId (0),
211@@ -852,6 +862,8 @@
212
213 StartupNotifyService::Default ()->SetSnDisplay (screen->snDisplay (), screen->screenNum ());
214
215+ initPluginActions ((void *) this);
216+
217 nux::NuxInitialize (0);
218 wt = nux::CreateFromForeignWindow (cScreen->output (),
219 glXGetCurrentContext (),
220@@ -905,8 +917,6 @@
221 UBUS_QUICKLIST_END_KEY_NAV,
222 (UBusCallback)&UnityScreen::OnQuicklistEndKeyNav,
223 this);
224-
225- g_timeout_add (0, &UnityScreen::initPluginActions, this);
226 g_timeout_add (5000, (GSourceFunc) write_logger_data_to_disk, NULL);
227
228 g_signal_connect (gdk_screen_get_default (),
229@@ -949,7 +959,7 @@
230 LOGGER_START_PROCESS ("initLauncher-Launcher");
231 self->launcherWindow = new nux::BaseWindow(TEXT("LauncherWindow"));
232 self->launcherWindow->SinkReference ();
233- self->launcher = new Launcher(self->launcherWindow, self->screen);
234+ self->launcher = new Launcher(self->launcherWindow, self->compScreen);
235 self->AddChild (self->launcher);
236
237 nux::HLayout* layout = new nux::HLayout();
238@@ -959,7 +969,7 @@
239 layout->SetVerticalExternalMargin(0);
240 layout->SetHorizontalExternalMargin(0);
241
242- self->controller = new LauncherController (self->launcher, self->screen, self->launcherWindow);
243+ self->controller = new LauncherController (self->launcher, self->compScreen, self->launcherWindow);
244
245 self->launcherWindow->SetConfigureNotifyCallback(&UnityScreen::launcherWindowConfigureCallback, self);
246 self->launcherWindow->SetLayout(layout);
247
248=== modified file 'src/unityshell.h'
249--- src/unityshell.h 2011-03-31 06:41:06 +0000
250+++ src/unityshell.h 2011-04-02 02:13:25 +0000
251@@ -61,7 +61,7 @@
252 ~UnityScreen ();
253
254 /* We store these to avoid unecessary calls to ::get */
255- CompScreen *screen;
256+ CompScreen *compScreen;
257 CompositeScreen *cScreen;
258 GLScreen *gScreen;
259