Merge lp:~3v1n0/unity/super-tab-switcher-shortcut-interaction into lp:unity

[Marco Trevisan (Treviño)]

Fixed the logical interaction between lp:~3v1n0/unity/super-tab-switcher and lp:~andyrock/unity/shortcut-hint as defined by design.

Plus, some code improvements and fixed an issue that caused the shortcut hint to show when pressing Super and clicking over the BFB to show the dash.

I'm wondering if we should avoid the same also when pressing over the workspace switcher.

[Andrea Azzarone (azzar1)]

>Plus, some code improvements and fixed an issue that caused the shortcut hint to show when pressing Super and clicking over the BFB to show the dash.

Which issue?

review: Needs Information

[Andrea Azzarone (azzar1)]

Never mind ;)

[Andrea Azzarone (azzar1)]

173 +bool Controller::IsEnabled()
174 +{
175 + return enabled_;
176 +}
177 +
178 +void Controller::SetEnabled(bool enabled)
179 +{
180 + enabled_ = enabled;
181 +}
182 +

Why not nux::Property enabled? What do you think?

+ optionSetShowMinimizedWindowsNotify (boost::bind(&UnityScreen::optionChanged, this, _1, _2));

Remove the space, please :)

271 +
272 + ubus_manager_.RegisterInterest(UBUS_LAUNCHER_START_KEY_NAV,
273 + sigc::mem_fun(this, &UnityScreen::OnLauncherStartKeyNav));
274 +
275 + ubus_manager_.RegisterInterest(UBUS_LAUNCHER_END_KEY_NAV,
276 + sigc::mem_fun(this, &UnityScreen::OnLauncherEndKeyNav));
277 +
278 + ubus_manager_.RegisterInterest(UBUS_QUICKLIST_END_KEY_NAV,
279 + sigc::mem_fun(this, &UnityScreen::OnQuicklistEndKeyNav));

Fix the indentation...

Btw it works well.

review: Needs Fixing

[Marco Trevisan (Treviño) (3v1n0)]

> 173 +bool Controller::IsEnabled()
> 174 +{
> 175 + return enabled_;
> 176 +}
> 177 +
> 178 +void Controller::SetEnabled(bool enabled)
> 179 +{
> 180 + enabled_ = enabled;
> 181 +}
> 182 +
>
> Why not nux::Property enabled? What do you think?

Mh... I'd prefer using this way, but let me know what you prefer.

> + optionSetShowMinimizedWindowsNotify
> (boost::bind(&UnityScreen::optionChanged, this, _1, _2));
>
> Remove the space, please :)
>
> 271 +
> 272 + ubus_manager_.RegisterInterest(UBUS_LAUNCHER_START_KEY_NAV,
> 273 +
> sigc::mem_fun(this, &UnityScreen::OnLauncherStartKeyNav));
> 274 +
> 275 + ubus_manager_.RegisterInterest(UBUS_LAUNCHER_END_KEY_NAV,
> 276 +
> sigc::mem_fun(this, &UnityScreen::OnLauncherEndKeyNav));
> 277 +
> 278 + ubus_manager_.RegisterInterest(UBUS_QUICKLIST_END_KEY_NAV,
> 279 +
> sigc::mem_fun(this, &UnityScreen::OnQuicklistEndKeyNav));
>
> Fix the indentation...

Damned you! :D
This was an Easter egg for you! ^_^

> Btw it works well.

Nice.
Do you have any design input about how the interaction with the expo? (Should the overlay be hidden when using mouse to activate it?)

[Andrea Azzarone (azzar1)]

> > 173 +bool Controller::IsEnabled()
> > 174 +{
> > 175 + return enabled_;
> > 176 +}
> > 177 +
> > 178 +void Controller::SetEnabled(bool enabled)
> > 179 +{
> > 180 + enabled_ = enabled;
> > 181 +}
> > 182 +
> >
> > Why not nux::Property enabled? What do you think?
>
> Mh... I'd prefer using this way, but let me know what you prefer.
>

I prefer nux::Property, but let decide Tim ;)

> > + optionSetShowMinimizedWindowsNotify
> > (boost::bind(&UnityScreen::optionChanged, this, _1, _2));
> >
> > Remove the space, please :)
> >
> > 271 +
> > 272 + ubus_manager_.RegisterInterest(UBUS_LAUNCHER_START_KEY_NAV,
> > 273 +
> > sigc::mem_fun(this, &UnityScreen::OnLauncherStartKeyNav));
> > 274 +
> > 275 + ubus_manager_.RegisterInterest(UBUS_LAUNCHER_END_KEY_NAV,
> > 276 +
> > sigc::mem_fun(this, &UnityScreen::OnLauncherEndKeyNav));
> > 277 +
> > 278 + ubus_manager_.RegisterInterest(UBUS_QUICKLIST_END_KEY_NAV,
> > 279 +
> > sigc::mem_fun(this, &UnityScreen::OnQuicklistEndKeyNav));
> >
> > Fix the indentation...
>
> Damned you! :D
> This was an Easter egg for you! ^_^
>
> > Btw it works well.
>
> Nice.
> Do you have any design input about how the interaction with the expo? (Should
> the overlay be hidden when using mouse to activate it?)

No.

[Tim Penhey (thumper)]

I wouldn't get too hung up on it. Your indentation is still off in a few places, but not blocking.

Perhaps this is worth writing an autopilot test for?

You would need to expose the shortcut controller :)

review: Approve

[Marco Trevisan (Treviño) (3v1n0)]

Well, I guess that the autopilot test could be included into the generic launcher switcher test...

[Andrea Azzarone (azzar1)]

review: Approve

=== modified file 'manual-tests/SuperTab.txt'
--- manual-tests/SuperTab.txt	2012-01-13 15:51:12 +0000
+++ manual-tests/SuperTab.txt	2012-01-19 23:43:23 +0000
@@ -20,3 +20,25 @@
   If in the launcher there are many icons and they are shown as collapsed,
   when the Super+Tab is activated and the icons should be expanded when
   neeeded.
+
+Super Tab switcher interaction with Shortcut Hint
+-------------------------------------------------
+This test shows the interaction between the shortcut hint overlay and the
+Super+Tab switcher.
+
+#. Start with a clean screen
+#. Press Super to make the shortcuts-overlay to show
+#. Then press Tab to initiate the Super+Tab launcher switcher
+
+Outcome:
+  The Super+Tab switcher is initialized, and the shortcut hint overlay is still
+  showing. Pressing also Shift to reverse the switcher direction doesn't hide
+  the overlay, that will be hidden once Super is released.
+
+#. Start with a clean screen
+#. Press Super+Tab quickly to make the launcher switcher to initialize without
+   making the shortcut overlay to show.
+
+Outcome:
+  Super+Tab switcher is initialized and the shortcut hint overlay is not shown
+  even keeping only super pressed until releasing it and pressing it again.

=== modified file 'plugins/unityshell/src/ShortcutController.cpp'
--- plugins/unityshell/src/ShortcutController.cpp	2011-12-19 22:42:19 +0000
+++ plugins/unityshell/src/ShortcutController.cpp	2012-01-19 23:43:23 +0000
@@ -19,7 +19,6 @@
 #include "ShortcutController.h"
 
 #include "UBusMessages.h"
-#include "ubus-server.h"
 #include "WindowManager.h"
 
 namespace unity
@@ -34,39 +33,43 @@
 Controller::Controller(std::list<AbstractHint*>& hints)
   : view_window_(0)
   , visible_(false)
+  , enabled_(true)
   , show_timer_(0)
-  , fade_in_animator_(new Animator(100))
-  , fade_out_animator_(new Animator(100))
-
+  , fade_in_animator_(100)
+  , fade_out_animator_(100)
 {
   bg_color_ = nux::Color(0.0, 0.0, 0.0, 0.5);
 
-  UBusServer *ubus = ubus_server_get_default();
-  bg_update_handle_ = ubus_server_register_interest(ubus, UBUS_BACKGROUND_COLOR_CHANGED,
-                                                    (UBusCallback)&Controller::OnBackgroundUpdate,
-                                                    this);
+  ubus_manager_.RegisterInterest(UBUS_BACKGROUND_COLOR_CHANGED,
+                                 sigc::mem_fun(this, &Controller::OnBackgroundUpdate));
+
+  ubus_manager_.RegisterInterest(UBUS_LAUNCHER_START_KEY_SWTICHER, [&] (GVariant*) {
+                                   enabled_ = false;
+                                 });
+
+  ubus_manager_.RegisterInterest(UBUS_LAUNCHER_END_KEY_SWTICHER, [&] (GVariant*) {
+                                   enabled_ = true;
+                                 });
+
+  ubus_manager_.RegisterInterest(UBUS_PLACE_VIEW_SHOWN, [&] (GVariant*) {
+                                   Hide();
+                                 });
+
+  ubus_manager_.SendMessage(UBUS_BACKGROUND_REQUEST_COLOUR_EMIT);
 
   model_.reset(new Model(hints));
-  
+
   model_->Fill();
   ConstructView();
 
-  fade_in_animator_->animation_updated.connect(sigc::mem_fun(this, &Controller::OnFadeInUpdated));
-  fade_in_animator_->animation_ended.connect(sigc::mem_fun(this, &Controller::OnFadeInEnded));
-  fade_out_animator_->animation_updated.connect(sigc::mem_fun(this, &Controller::OnFadeOutUpdated));
-  fade_out_animator_->animation_ended.connect(sigc::mem_fun(this, &Controller::OnFadeOutEnded));
+  fade_in_animator_.animation_updated.connect(sigc::mem_fun(this, &Controller::OnFadeInUpdated));
+  fade_in_animator_.animation_ended.connect(sigc::mem_fun(this, &Controller::OnFadeInEnded));
+  fade_out_animator_.animation_updated.connect(sigc::mem_fun(this, &Controller::OnFadeOutUpdated));
+  fade_out_animator_.animation_ended.connect(sigc::mem_fun(this, &Controller::OnFadeOutEnded));
 }
 
 Controller::~Controller()
 {
-  ubus_server_unregister_interest(ubus_server_get_default(), bg_update_handle_);
-  
-  if (fade_in_animator_)
-    delete fade_in_animator_;
-
-  if (fade_out_animator_)
-    delete fade_out_animator_;
-  
   if (view_window_)
     view_window_->UnReference();
     
@@ -96,21 +99,23 @@
 }
 
 
-void Controller::OnBackgroundUpdate(GVariant* data, Controller* self)
+void Controller::OnBackgroundUpdate(GVariant* data)
 {
   gdouble red, green, blue, alpha;
   g_variant_get(data, "(dddd)", &red, &green, &blue, &alpha);
-  self->bg_color_ = nux::Color(red, green, blue, alpha);
+  bg_color_ = nux::Color(red, green, blue, alpha);
 
-  if (self->view_)
-    self->view_->background_color = self->bg_color_;
+  if (view_)
+    view_->background_color = bg_color_;
 }
 
 void Controller::Show()
 {
   if (show_timer_)
     g_source_remove (show_timer_);
-  show_timer_ = g_timeout_add(SUPER_TAP_DURATION, &Controller::OnShowTimer, this);
+
+  if (enabled_)
+    show_timer_ = g_timeout_add(SUPER_TAP_DURATION, &Controller::OnShowTimer, this);
 
   model_->Fill();
   visible_ = true;
@@ -119,16 +124,19 @@
 gboolean Controller::OnShowTimer(gpointer data)
 {
   Controller* self = static_cast<Controller*>(data);
-  
-  ubus_server_send_message(ubus_server_get_default(),
-                           UBUS_PLACE_VIEW_CLOSE_REQUEST,
-                           NULL);
+
+  if (!self->enabled_)
+  {
+    return FALSE;
+  }
+
+  self->ubus_manager_.SendMessage(UBUS_PLACE_VIEW_CLOSE_REQUEST);
 
   if (self->visible_)
   {
     self->view_->SetupBackground(true);
-    self->fade_out_animator_->Stop();
-    self->fade_in_animator_->Start(self->view_window_->GetOpacity());
+    self->fade_out_animator_.Stop();
+    self->fade_in_animator_.Start(self->view_window_->GetOpacity());
   }
 
   self->show_timer_ = 0;
@@ -176,8 +184,8 @@
   if (view_window_)
   {
     view_->SetupBackground(false);
-    fade_in_animator_->Stop();
-    fade_out_animator_->Start(1.0 - view_window_->GetOpacity());
+    fade_in_animator_.Stop();
+    fade_out_animator_.Start(1.0 - view_window_->GetOpacity());
   }
 
   if (show_timer_)
@@ -190,5 +198,15 @@
   return visible_;
 }
 
+bool Controller::IsEnabled()
+{
+  return enabled_;
+}
+
+void Controller::SetEnabled(bool enabled)
+{
+  enabled_ = enabled;
+}
+
 } // namespace shortcut
 } // namespace unity

=== modified file 'plugins/unityshell/src/ShortcutController.h'
--- plugins/unityshell/src/ShortcutController.h	2011-12-13 23:31:20 +0000
+++ plugins/unityshell/src/ShortcutController.h	2012-01-19 23:43:23 +0000
@@ -29,6 +29,7 @@
 #include "Animator.h"
 #include "ShortcutModel.h"
 #include "ShortcutView.h"
+#include "UBusWrapper.h"
 
 namespace unity
 {
@@ -49,13 +50,15 @@
   void Hide();
   
   bool Visible();
+  bool IsEnabled();
 
   void SetWorkspace(nux::Geometry const& geo);
+  void SetEnabled(bool enabled);
 
 private:
   // Private Methods
   void ConstructView();
-  static void OnBackgroundUpdate(GVariant* data, Controller* self);
+  void OnBackgroundUpdate(GVariant* data);
   void OnFadeInUpdated(double opacity);
   void OnFadeInEnded();
   void OnFadeOutUpdated(double opacity);
@@ -72,13 +75,15 @@
   nux::HLayout* main_layout_;
   
   bool visible_;
+  bool enabled_;
   nux::Color bg_color_;
   guint show_timer_;
-  guint bg_update_handle_;
   
-  Animator* fade_in_animator_;
-  Animator* fade_out_animator_;
-}; 
+  Animator fade_in_animator_;
+  Animator fade_out_animator_;
+
+  UBusManager ubus_manager_;
+};
 
 } // namespace shortcut
 } // namespace unity

=== modified file 'plugins/unityshell/src/unityshell.cpp'
--- plugins/unityshell/src/unityshell.cpp	2012-01-19 15:13:35 +0000
+++ plugins/unityshell/src/unityshell.cpp	2012-01-19 23:43:23 +0000
@@ -52,7 +52,6 @@
 
 #include "unitya11y.h"
 
-#include "ubus-server.h"
 #include "UBusMessages.h"
 #include "UScreen.h"
 
@@ -294,26 +293,16 @@
      optionSetLauncherSwitcherPrevInitiate(boost::bind(&UnityScreen::launcherSwitcherPrevInitiate, this, _1, _2, _3));
      optionSetLauncherSwitcherForwardTerminate(boost::bind(&UnityScreen::launcherSwitcherTerminate, this, _1, _2, _3));
 
-     optionSetShowMinimizedWindowsNotify (boost::bind (&UnityScreen::optionChanged, this, _1, _2));
-
-     for (unsigned int i = 0; i < G_N_ELEMENTS(_ubus_handles); i++)
-       _ubus_handles[i] = 0;
-
-     UBusServer* ubus = ubus_server_get_default();
-     _ubus_handles[0] = ubus_server_register_interest(ubus,
-						      UBUS_LAUNCHER_START_KEY_NAV,
-						      (UBusCallback)&UnityScreen::OnLauncherStartKeyNav,
-						      this);
-
-     _ubus_handles[1] = ubus_server_register_interest(ubus,
-						      UBUS_LAUNCHER_END_KEY_NAV,
-						      (UBusCallback)&UnityScreen::OnLauncherEndKeyNav,
-						      this);
-
-     _ubus_handles[2] = ubus_server_register_interest(ubus,
-						      UBUS_QUICKLIST_END_KEY_NAV,
-						      (UBusCallback)&UnityScreen::OnQuicklistEndKeyNav,
-						      this);
+     optionSetShowMinimizedWindowsNotify(boost::bind(&UnityScreen::optionChanged, this, _1, _2));
+
+     ubus_manager_.RegisterInterest(UBUS_LAUNCHER_START_KEY_NAV,
+                   sigc::mem_fun(this, &UnityScreen::OnLauncherStartKeyNav));
+
+     ubus_manager_.RegisterInterest(UBUS_LAUNCHER_END_KEY_NAV,
+                   sigc::mem_fun(this, &UnityScreen::OnLauncherEndKeyNav));
+
+     ubus_manager_.RegisterInterest(UBUS_QUICKLIST_END_KEY_NAV,
+                   sigc::mem_fun(this, &UnityScreen::OnQuicklistEndKeyNav));
 
      g_idle_add_full (G_PRIORITY_DEFAULT, &UnityScreen::initPluginActions, this, NULL);
      super_keypressed_ = false;
@@ -343,13 +332,6 @@
 
   unity_a11y_finalize();
 
-  UBusServer* ubus = ubus_server_get_default();
-  for (unsigned int i = 0; i < G_N_ELEMENTS(_ubus_handles); i++)
-  {
-    if (_ubus_handles[i] != 0)
-      ubus_server_unregister_interest(ubus, _ubus_handles[i]);
-  }
-
   if (relayoutSourceId != 0)
     g_source_remove(relayoutSourceId);
 
@@ -1129,7 +1111,16 @@
         key_string[result] = 0;
         if (super_keypressed_)
         {
-          shortcut_controller_->Hide();
+          g_idle_add([] (gpointer data) -> gboolean {
+            auto self = static_cast<UnityScreen*>(data);
+            if (!self->launcher_controller_->launcher().KeySwitcherIsActive())
+            {
+              self->shortcut_controller_->SetEnabled(false);
+              self->shortcut_controller_->Hide();
+            }
+            return FALSE;
+          }, this);
+
           skip_other_plugins = launcher.CheckSuperShortcutPressed(screen->dpy(), key_sym, event->xkey.keycode, event->xkey.state, key_string);
           if (!skip_other_plugins)
           {
@@ -1225,10 +1216,9 @@
   PluginAdapter::Default()->NotifyCompizEvent(plugin, event, option);
   compiz::CompizMinimizedWindowHandler<UnityScreen, UnityWindow>::handleCompizEvent (plugin, event, option);
 
-  if (dash_is_open_ && 
-      strcmp(event, "start_viewport_switch") == 0)
+  if (dash_is_open_ && g_strcmp0(event, "start_viewport_switch") == 0)
   {
-    ubus_server_send_message(ubus_server_get_default(), UBUS_PLACE_VIEW_CLOSE_REQUEST, NULL);
+    ubus_manager_.SendMessage(UBUS_PLACE_VIEW_CLOSE_REQUEST);
   }
 
   screen->handleCompizEvent(plugin, event, option);
@@ -1245,33 +1235,33 @@
   super_keypressed_ = true;
   launcher_controller_->launcher().StartKeyShowLauncher();
   EnsureSuperKeybindings ();
-  
-  if (enable_shortcut_overlay_ and !shortcut_controller_->Visible())
+
+  if (!shortcut_controller_->Visible() && shortcut_controller_->IsEnabled())
   {
     static nux::Geometry last_geo;
     UScreen* uscreen = UScreen::GetDefault();
     int primary_monitor = uscreen->GetPrimaryMonitor();
     auto monitor_geo = uscreen->GetMonitorGeometry(primary_monitor);
-        
+
     int width = 970;
     int height =  680;
     int launcher_width = optionGetIconSize() + 18;
     int panel_height = 24;
     int x = monitor_geo.x + launcher_width + (monitor_geo.width - launcher_width- width) / 2;
     int y = monitor_geo.y + panel_height + (monitor_geo.height - panel_height - height) / 2;
-    
+
     nux::Geometry geo (x, y, width, height);
-    
+
     if (last_geo != geo)
     {
       shortcut_controller_->SetWorkspace(geo);
       last_geo = geo;
     }
-    
+
     if (last_geo.x > monitor_geo.x and last_geo.y > monitor_geo.y)
       shortcut_controller_->Show();
    }
-  
+
   return false;
 }
 
@@ -1282,6 +1272,8 @@
   super_keypressed_ = false;
   launcher_controller_->launcher().EndKeyShowLauncher();
   launcher_controller_->launcher().KeySwitcherTerminate();
+
+  shortcut_controller_->SetEnabled(enable_shortcut_overlay_);
   shortcut_controller_->Hide();
   return false;
 }
@@ -1370,12 +1362,8 @@
 
 void UnityScreen::SendExecuteCommand()
 {
-  ubus_server_send_message(ubus_server_get_default(),
-                           UBUS_PLACE_ENTRY_ACTIVATE_REQUEST,
-                           g_variant_new("(sus)",
-                                         "commands.lens",
-                                         0,
-                                         ""));
+  ubus_manager_.SendMessage(UBUS_PLACE_ENTRY_ACTIVATE_REQUEST,
+                            g_variant_new("(sus)", "commands.lens", 0, ""));
 }
 
 bool UnityScreen::executeCommand(CompAction* action,
@@ -1563,13 +1551,12 @@
   return false;
 }
 
-void UnityScreen::OnLauncherStartKeyNav(GVariant* data, void* value)
+void UnityScreen::OnLauncherStartKeyNav(GVariant* data)
 {
-  UnityScreen* self = reinterpret_cast<UnityScreen*>(value);
-  self->startLauncherKeyNav();
+  startLauncherKeyNav();
 }
 
-void UnityScreen::OnLauncherEndKeyNav(GVariant* data, void* value)
+void UnityScreen::OnLauncherEndKeyNav(GVariant* data)
 {
   bool preserve_focus = false;
 
@@ -1584,10 +1571,9 @@
     PluginAdapter::Default ()->restoreInputFocus ();
 }
 
-void UnityScreen::OnQuicklistEndKeyNav(GVariant* data, void* value)
+void UnityScreen::OnQuicklistEndKeyNav(GVariant* data)
 {
-  UnityScreen* self = reinterpret_cast<UnityScreen*>(value);
-  self->restartLauncherKeyNav();
+  restartLauncherKeyNav();
 }
 
 gboolean UnityScreen::initPluginActions(gpointer data)
@@ -2242,6 +2228,7 @@
       break;
     case UnityshellOptions::ShortcutOverlay:
       enable_shortcut_overlay_ = optionGetShortcutOverlay();
+      shortcut_controller_->SetEnabled(enable_shortcut_overlay_);
       break;
     case UnityshellOptions::ShowDesktopIcon:
       launcher_controller_->SetShowDesktopIcon(optionGetShowDesktopIcon());

=== modified file 'plugins/unityshell/src/unityshell.h'
--- plugins/unityshell/src/unityshell.h	2012-01-14 13:05:16 +0000
+++ plugins/unityshell/src/unityshell.h	2012-01-19 23:43:23 +0000
@@ -245,10 +245,10 @@
 
   void OnDashRealized ();
 
-  static void OnQuicklistEndKeyNav(GVariant* data, void* value);
-  static void OnLauncherStartKeyNav(GVariant* data, void* value);
-  static void OnLauncherEndKeyNav(GVariant* data, void* value);
-  
+  void OnQuicklistEndKeyNav(GVariant* data);
+  void OnLauncherStartKeyNav(GVariant* data);
+  void OnLauncherEndKeyNav(GVariant* data);
+
   void InitHints();
 
   dash::Settings dash_settings_;
@@ -277,8 +277,7 @@
   guint                                 _edge_trigger_handle;
   guint32                               _redraw_handle;
   gint                                  _edge_pointerY;
-  guint                                 _ubus_handles[3];
-  
+
   typedef std::shared_ptr<CompAction> CompActionPtr;
   typedef std::vector<CompActionPtr> ShortcutActions;
   ShortcutActions _shortcut_actions;