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

[Sam Spilsbury]

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

[Jason Smith (jassmith)]

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

review: Approve

[lp:~dx-unity/unity/unity.fix_702431]

1047. By Sam Spilsbury on 2011-03-31

Don't mix display connections

[Didier Roche (didrocks)]

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]

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)]

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)]

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)]

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)]

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

[David Barth (dbarth)]

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

=== modified file 'src/Launcher.cpp'
--- src/Launcher.cpp	2011-03-31 21:54:24 +0000
+++ src/Launcher.cpp	2011-04-02 02:13:25 +0000
@@ -614,7 +614,7 @@
   _hide_machine->SetQuirk (LauncherHideMachine::KEY_NAV_ACTIVE, true);
   _hover_machine->SetQuirk (LauncherHoverMachine::KEY_NAV_ACTIVE, true);
   _hide_machine->SetQuirk (LauncherHideMachine::LAST_ACTION_ACTIVATE, false);
-  
+
   GrabKeyboard ();
   GrabPointer ();
   
@@ -1475,14 +1475,14 @@
     bool tap_on_super;
     bool shortcuts_shown = false;
     clock_gettime (CLOCK_MONOTONIC, &current);
-        
+
     tap_on_super = (TimeDelta (&current, &_times[TIME_TAP_SUPER]) < SUPER_TAP_DURATION);
 
     if (_hide_machine->GetQuirk (LauncherHideMachine::TRIGGER_BUTTON_DOWN))
       shortcuts_shown = !tap_on_super;
 
-    _hover_machine->SetQuirk (LauncherHoverMachine::SHOTCUT_KEYS_VISIBLE, shortcuts_shown);
-    
+     _hover_machine->SetQuirk (LauncherHoverMachine::SHOTCUT_KEYS_VISIBLE, shortcuts_shown);
+
     return tap_on_super;
     
 }
@@ -1502,7 +1502,6 @@
 
 void Launcher::EndKeyShowLauncher ()
 {
-    
     _hide_machine->SetQuirk (LauncherHideMachine::TRIGGER_BUTTON_DOWN, false);
     _hover_machine->SetQuirk (LauncherHoverMachine::SHOTCUT_KEYS_VISIBLE, false);
     QueueDraw ();

=== modified file 'src/LauncherIcon.cpp'
--- src/LauncherIcon.cpp	2011-03-31 20:33:43 +0000
+++ src/LauncherIcon.cpp	2011-04-02 02:13:25 +0000
@@ -373,10 +373,59 @@
 void
 LauncherIcon::SetShortcut (guint64 shortcut)
 {
+  unsigned int old_shortcut = _shortcut;
+  // also remove old shortcut
+  if (_shortcut > 32)
+  {
+    unsigned int keycode = XKeysymToKeycode (screen->dpy (), _shortcut);
+    unsigned int modifiers = modHandler->virtualToRealModMask (PluginAdapter::Default ()->GetShowLauncherKeyMods ());
+
+    for (unsigned int ignore = 0; ignore <= modHandler->ignoredModMask (); ignore++)
+    {
+      if (ignore & ~modHandler->ignoredModMask ())
+	continue;
+
+      /* Keycode is always > 0, so we can always grab directly rather than having
+       * to grab modifier trees */
+
+      XUngrabKey (screen->dpy (),
+		  keycode,
+		  modifiers | ignore,
+		  screen->root ());
+
+      _shortcut = 0;
+    }
+  }
+
   // only relocate a digit with a digit (don't overwrite other shortcuts)
-  if ((!_shortcut || (g_ascii_isdigit ((gchar)_shortcut)))
-        || !(g_ascii_isdigit ((gchar) shortcut)))
+  if ((!old_shortcut || (g_ascii_isdigit ((gchar)old_shortcut)))
+	|| !(g_ascii_isdigit ((gchar) old_shortcut)))
+  {
     _shortcut = shortcut;
+
+    if (_shortcut > 32)
+    {
+      unsigned int keycode = XKeysymToKeycode (screen->dpy (), _shortcut);
+      unsigned int modifiers = modHandler->virtualToRealModMask (PluginAdapter::Default ()->GetShowLauncherKeyMods ());
+
+      for (unsigned int ignore = 0; ignore <= modHandler->ignoredModMask (); ignore++)
+      {
+	if (ignore & ~modHandler->ignoredModMask ())
+	  continue;
+
+	/* Keycode is always > 0, so we can always grab directly rather than having
+	 * to grab modifier trees */
+
+	XGrabKey (screen->dpy (),
+		  keycode,
+		  modifiers | ignore,
+		  screen->root (),
+		  true,
+		  GrabModeAsync,
+		  GrabModeAsync);
+      }
+    }
+  }
 }
 
 guint64

=== modified file 'src/PluginAdapter.cpp'
--- src/PluginAdapter.cpp	2011-04-01 20:04:14 +0000
+++ src/PluginAdapter.cpp	2011-04-02 02:13:25 +0000
@@ -52,6 +52,8 @@
   _grab_show_action = 0;
   _grab_hide_action = 0;
   _grab_toggle_action = 0;
+
+  _show_launcher_action = 0;
 }
 
 PluginAdapter::~PluginAdapter()
@@ -320,6 +322,18 @@
     m_ExpoActionList.InitiateAll (argument, 0);
 }
 
+unsigned int
+PluginAdapter::GetShowLauncherKeyMods ()
+{
+  /* Find the action used to trigger showing the launcher
+   * and then return its key mods */
+
+  if (_show_launcher_action)
+    return _show_launcher_action->key ().modifiers ();
+  else
+    return CompSuperMask;
+}
+
 // WindowManager implementation
 bool
 PluginAdapter::IsWindowMaximized (guint xid)

=== modified file 'src/PluginAdapter.h'
--- src/PluginAdapter.h	2011-03-30 17:29:51 +0000
+++ src/PluginAdapter.h	2011-04-02 02:13:25 +0000
@@ -65,6 +65,8 @@
     void SetHideHandlesAction (CompAction *action) { _grab_hide_action = action; }
     void SetToggleHandlesAction (CompAction *action) { _grab_toggle_action = action; }
 
+    void SetShowLauncherAction (CompAction *action) { _show_launcher_action = action; }
+
     void OnScreenGrabbed ();
     void OnScreenUngrabbed ();
 
@@ -74,6 +76,8 @@
     
     void InitiateExpo ();
     bool IsExpoActive ();
+
+    unsigned int GetShowLauncherKeyMods ();
     
     void ShowGrabHandles (CompWindow *window, bool use_timer);
     void HideGrabHandles (CompWindow *window);
@@ -117,6 +121,7 @@
     CompAction *_grab_show_action;
     CompAction *_grab_hide_action;
     CompAction *_grab_toggle_action;
+    CompAction *_show_launcher_action;
     
     static PluginAdapter *_default;
 };

=== modified file 'src/unityshell.cpp'
--- src/unityshell.cpp	2011-03-31 21:45:39 +0000
+++ src/unityshell.cpp	2011-04-02 02:13:25 +0000
@@ -443,6 +443,12 @@
 {
   CompPlugin *p;
 
+  /* Internal show launcher action */
+
+  PluginAdapter::Default ()->SetShowLauncherAction (&UnityScreen::get (screen)->optionGetShowLauncher ());
+
+  /* Expo */
+
   p = CompPlugin::find ("expo");
 
   if (p)
@@ -464,6 +470,8 @@
     PluginAdapter::Default ()->SetExpoAction (expoActions);
   }
 
+  /* Scale */
+
   p = CompPlugin::find ("scale");
 
   if (p)
@@ -497,6 +505,8 @@
     PluginAdapter::Default ()->SetScaleAction (scaleActions);
   }
 
+  /* Unity MT Grab Handles (AKA Love Handles) */
+
   p = CompPlugin::find ("unitymtgrabhandles");
 
   if (p)
@@ -822,7 +832,7 @@
 
 UnityScreen::UnityScreen (CompScreen *screen) :
     PluginClassHandler <UnityScreen, CompScreen> (screen),
-    screen (screen),
+    compScreen (screen),
     cScreen (CompositeScreen::get (screen)),
     gScreen (GLScreen::get (screen)),
     relayoutSourceId (0),
@@ -852,6 +862,8 @@
 
   StartupNotifyService::Default ()->SetSnDisplay (screen->snDisplay (), screen->screenNum ());
 
+  initPluginActions ((void *) this);
+
   nux::NuxInitialize (0);
   wt = nux::CreateFromForeignWindow (cScreen->output (),
                                      glXGetCurrentContext (),
@@ -905,8 +917,6 @@
                                  UBUS_QUICKLIST_END_KEY_NAV,
                                  (UBusCallback)&UnityScreen::OnQuicklistEndKeyNav,
                                  this);
-
-  g_timeout_add (0, &UnityScreen::initPluginActions, this);
   g_timeout_add (5000, (GSourceFunc) write_logger_data_to_disk, NULL);
 
   g_signal_connect (gdk_screen_get_default (),
@@ -949,7 +959,7 @@
   LOGGER_START_PROCESS ("initLauncher-Launcher");
   self->launcherWindow = new nux::BaseWindow(TEXT("LauncherWindow"));
   self->launcherWindow->SinkReference ();
-  self->launcher = new Launcher(self->launcherWindow, self->screen);
+  self->launcher = new Launcher(self->launcherWindow, self->compScreen);
   self->AddChild (self->launcher);
 
   nux::HLayout* layout = new nux::HLayout();
@@ -959,7 +969,7 @@
   layout->SetVerticalExternalMargin(0);
   layout->SetHorizontalExternalMargin(0);
 
-  self->controller = new LauncherController (self->launcher, self->screen, self->launcherWindow);
+  self->controller = new LauncherController (self->launcher, self->compScreen, self->launcherWindow);
 
   self->launcherWindow->SetConfigureNotifyCallback(&UnityScreen::launcherWindowConfigureCallback, self);
   self->launcherWindow->SetLayout(layout);

=== modified file 'src/unityshell.h'
--- src/unityshell.h	2011-03-31 06:41:06 +0000
+++ src/unityshell.h	2011-04-02 02:13:25 +0000
@@ -61,7 +61,7 @@
 	~UnityScreen ();
 
 	/* We store these  to avoid unecessary calls to ::get */
-	CompScreen      *screen;
+	CompScreen      *compScreen;
 	CompositeScreen *cScreen;
         GLScreen        *gScreen;