Merge lp:~mterry/unity/683735 into lp:unity

[Michael Terry]

[Alex Launi (alexlauni)]

Patch works nicely. Maybe it'd be good to check the scale action's state before calling terminate? It doesn't crash the way it is, but it would save a little bit of overhead.

[Sam Spilsbury (smspillaz)]

Yeah alex is right. Basically just do an if (m_ScaleAction->state () & (CompAction::StateTermKey | CompAction::StateTermButton)) terminate ();

[lp:~mterry/unity/683735]

650. By Michael Terry on 2010-12-04

don't terminate scale if we're not scaling; terminate scale if we click on any launcher icon, not just running bamf ones

[Michael Terry (mterry)]

I've modified the code to:
 * Terminate the scale if any launcher icon is clicked, not just running bamf icons
 * Does not scale again if the user clicks on the same bamf launcher while scaled
 * Does not terminate scale if we aren't scaled

In order to determine if we're scaled, I could not use your state() code, Same. The only thing that worked for me was actually getting the plugin class and asking if it had the grab. The action object itself just didn't have the information. The state() function always returned the same state. Which I could have fixed by passing StateInitButton when initializing the scale, but even so, it would only work when checking for scaling done by that same action. If the user pressed Super+A, they would get a scale that couldn't be terminated by launcher clicks, though I think it should be.

So, I linked against the scale plugin and checked its status directly.

[Sam Spilsbury (smspillaz)]

That's strange the state () should reflect what is there of the action (since the scale plugin does action->setState ().

Maybe the initial state isn't set to StateInitKey, in which case you'll need to force that (eg m_ScaleAction->setState (CompAction::StateInitKey); before actually initiating it

[Sam Spilsbury (smspillaz)]

Oh, pass StateInitKey | StateInitButton. That should do it. core doesn't really care what state the action is in, it only sets those things so that the plugins can respond appropriately.

[Michael Terry (mterry)]

Right... As I said, "I could have fixed by passing StateInitButton when initializing the scale, but even so, it would only work when checking for scaling done by that same action. If the user pressed Super+A, they would get a scale that couldn't be terminated by launcher clicks, though I think it should be. So, I linked against the scale plugin and checked its status directly."

There's a difference between checking the action's state and the plugin's state.

[Alex Launi (alexlauni)]

You can pass StateInitKey | StateInitButton. That should handle the Super + A case.

[Michael Terry (mterry)]

Actually, in my testing the plugin only handles state like we want if we pass just StateInitButton. Passing in StateInitKey (either additionally or by itself) doens't change the results of state() calls.

Even if we did pass them both in, my point was that the action/plugin relationship was multiple-to-one (as I understand it, I may be totally wrong). So just checking the state of one action is not sufficient as you don't know what other actions are doing. So you have to go to the source, the plugin. That's why we can't handle Super+A without checking the plugin.

[Sam Spilsbury (smspillaz)]

Huh. My impression was that it worked differently. I'll check the code, sec

[Sam Spilsbury (smspillaz)]

Ah yes, the problem is that there is one CompAction * per CompOption *, which means that there is one CompAction * for initiate_key, initiate_button and initiate_edge, as well as initiate_all_* initiate_group_* etc

I guess this means you'll have to store all the actions and check all
of their states and fire the actions appropriately (eg don't initiate if one of them is already on StateTerminateKey | StateTerminateButton | StateTerminateEdge)

Since the fix is *fairly* non-trivial I will merge it for now and finish it myself tonight. Thanks for all your help Michael :)

review: Approve

[lp:~mterry/unity/683735]

651. By Michael Terry on 2010-12-13

merge from trunk

[Neil J. Patel (njpatel)]

Thanks for updating the branch, it's merged now!

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2010-12-09 18:02:17 +0000
+++ CMakeLists.txt	2010-12-13 23:35:23 +0000
@@ -80,7 +80,7 @@
 include (CompizPlugin)
 compiz_plugin (unityshell
                PKGDEPS ${UNITY_PLUGIN_DEPS}
-               PLUGINDEPS composite opengl
+               PLUGINDEPS composite opengl scale
                CFLAGSADD "-DINSTALLPREFIX='\"${CMAKE_INSTALL_PREFIX}\"' -DPKGDATADIR='\"${CMAKE_INSTALL_PREFIX}/share/unity/3\"' -I${CMAKE_BINARY_DIR}"
                LIBRARIES "unity"
                )

=== modified file 'src/BamfLauncherIcon.cpp'
--- src/BamfLauncherIcon.cpp	2010-12-13 22:50:48 +0000
+++ src/BamfLauncherIcon.cpp	2010-12-13 23:35:23 +0000
@@ -306,6 +306,10 @@
 void
 BamfLauncherIcon::OnMouseClick (int button)
 {
+  bool scaleWasActive = PluginAdapter::Default ()->IsScaleActive();
+
+  SimpleLauncherIcon::OnMouseClick (button);
+
   if (button != 1)
     return;
 
@@ -322,10 +326,10 @@
     OpenInstance ();
     return;
   }
-  else if (active)
+  else if (!active)
+    Focus ();
+  else if (!scaleWasActive)
     Spread ();
-  else
-    Focus ();
 }
 
 void

=== modified file 'src/PluginAdapter.cpp'
--- src/PluginAdapter.cpp	2010-11-10 15:45:18 +0000
+++ src/PluginAdapter.cpp	2010-12-13 23:35:23 +0000
@@ -17,6 +17,7 @@
  */
  
 #include <glib.h>
+#include <scale/scale.h>
 #include "PluginAdapter.h"
 
 PluginAdapter * PluginAdapter::_default = 0;
@@ -111,6 +112,35 @@
     m_ScaleAction->initiate () (m_ScaleAction, 0, argument);
 }
     
+bool 
+PluginAdapter::IsScaleActive ()
+{
+    SCALE_SCREEN(m_Screen);
+    return (m_ScaleAction && ss && ss->hasGrab ());
+}
+
+void 
+PluginAdapter::TerminateScale ()
+{
+    if (!IsScaleActive ())
+        return;
+
+    CompOption::Value value;
+    CompOption::Type  type;
+    CompOption::Vector argument;
+    char             *name;
+
+    name = (char *) "root";
+    type = CompOption::TypeInt;
+    value.set ((int) m_Screen->root ());
+    
+    CompOption arg = CompOption (name, type);
+    arg.set (value);
+    argument.push_back (arg);
+    
+    m_ScaleAction->terminate () (m_ScaleAction, 0, argument);
+}
+
 void 
 PluginAdapter::InitiateExpo ()
 {

=== modified file 'src/PluginAdapter.h'
--- src/PluginAdapter.h	2010-11-10 14:42:46 +0000
+++ src/PluginAdapter.h	2010-12-13 23:35:23 +0000
@@ -40,6 +40,10 @@
     void SetExpoAction (CompAction *expo);
     
     void InitiateScale (std::string *match);
+
+    bool IsScaleActive ();
+
+    void TerminateScale ();
     
     void InitiateExpo ();
     

=== modified file 'src/SimpleLauncherIcon.cpp'
--- src/SimpleLauncherIcon.cpp	2010-12-06 20:12:06 +0000
+++ src/SimpleLauncherIcon.cpp	2010-12-13 23:35:23 +0000
@@ -21,6 +21,7 @@
 
 #include "SimpleLauncherIcon.h"
 #include "Launcher.h"
+#include "PluginAdapter.h"
 
 SimpleLauncherIcon::SimpleLauncherIcon (Launcher* IconManager)
 :   LauncherIcon(IconManager)
@@ -54,6 +55,8 @@
 void
 SimpleLauncherIcon::OnMouseClick (int button)
 {
+  if (button == 1)
+    PluginAdapter::Default ()->TerminateScale ();
 }
 
 void

=== modified file 'src/TrashLauncherIcon.cpp'
--- src/TrashLauncherIcon.cpp	2010-12-07 18:14:21 +0000
+++ src/TrashLauncherIcon.cpp	2010-12-13 23:35:23 +0000
@@ -50,6 +50,8 @@
 void
 TrashLauncherIcon::OnMouseClick (int button)
 {
+  SimpleLauncherIcon::OnMouseClick (button);
+
   if (button == 1)
   {
     GError *error = NULL;