Merge lp:~alales/unity/unity-4.0-fix-for-791810 into lp:unity/4.0

[Alain Lessard]

Reactivate the current quicklist when Expo exit on right-clicking, otherwise another window will be active and the quicklist will freeze (bug #791810).

Has been tested in oneiric with sudo make install + logout + login.

Exit expo set on Button3 in CompizConfig Settings Manager.

[Andrea Azzarone (azzar1)]

17 +Launcher::OnExpoTerminated()
18 +{
19 + _hide_machine->SetQuirk (LauncherHideMachine::EXPO_ACTIVE, WindowManager::Default ()->IsExpoActive ());

Plese don't put a space between the function name and the open parenthesis.

20 + _hide_machine->SetQuirk (LauncherHideMachine::SCALE_ACTIVE, WindowManager::Default ()->IsScaleActive ());

Is the above line really needed? We're in the "OnExpoTerminated" function...

26 + if (_hidemode == LAUNCHER_HIDE_NEVER)
27 + return;
28 +}

This if is not really needed. Please remove it.

54 +void QuicklistManager::Activate()
55 +{
56 + if (_current_quicklist)
57 + _current_quicklist->Activate();
58 +}

Wrong indentation. Please fix it :)

After the adjustments i'm going to continue the review. Thanks in advance :)

review: Needs Fixing

[lp:~alales/unity/unity-4.0-fix-for-791810]

1719. By Alain Lessard on 2011-11-30

Modifications required by code review 1.

[Alain Lessard (alales)]

On Sat, 2011-11-26 at 01:41 +0000, Andrea Azzarone wrote:
> Review: Needs Fixing
>
> 17 +Launcher::OnExpoTerminated()
> 18 +{
> 19 + _hide_machine->SetQuirk (LauncherHideMachine::EXPO_ACTIVE, WindowManager::Default ()->IsExpoActive ());
>
> Plese don't put a space between the function name and the open parenthesis.

OK, I've done this.

>
> 20 + _hide_machine->SetQuirk (LauncherHideMachine::SCALE_ACTIVE, WindowManager::Default ()->IsScaleActive ());
>
> Is the above line really needed? We're in the "OnExpoTerminated" function...

Prior to my modification, this line was used in the method
Launcher::OnPluginStateChanged()

That method was connected to the terminate_expo event

My intention was to use the same code plus the call to
QuicklistManager::Default()->Activate() that I've added, just to make sure
I don't break any existing features...

>
> 26 + if (_hidemode == LAUNCHER_HIDE_NEVER)
> 27 + return;
> 28 +}
>
> This if is not really needed. Please remove it.

OK, I've done this.

>
> 54 +void QuicklistManager::Activate()
> 55 +{
> 56 + if (_current_quicklist)
> 57 + _current_quicklist->Activate();
> 58 +}
>
> Wrong indentation. Please fix it :)

OK, I've done this.

>
> After the adjustments i'm going to continue the review. Thanks in advance :)

OK. Thanks.

[Andrea Azzarone (azzar1)]

> On Sat, 2011-11-26 at 01:41 +0000, Andrea Azzarone wrote:
> > Review: Needs Fixing
> >
> > 17 +Launcher::OnExpoTerminated()
> > 18 +{
> > 19 + _hide_machine->SetQuirk (LauncherHideMachine::EXPO_ACTIVE,
> WindowManager::Default ()->IsExpoActive ());
> >
> > Plese don't put a space between the function name and the open parenthesis.
>
> OK, I've done this.
>
> >
> > 20 + _hide_machine->SetQuirk (LauncherHideMachine::SCALE_ACTIVE,
> WindowManager::Default ()->IsScaleActive ());
> >
> > Is the above line really needed? We're in the "OnExpoTerminated" function...
>
> Prior to my modification, this line was used in the method
> Launcher::OnPluginStateChanged()
>
> That method was connected to the terminate_expo event
>
> My intention was to use the same code plus the call to
> QuicklistManager::Default()->Activate() that I've added, just to make sure
> I don't break any existing features...

I know but you still have the function OnPluginStateChanged that is called when the scale/spread plugin state change.

>
> >
> > 26 + if (_hidemode == LAUNCHER_HIDE_NEVER)
> > 27 + return;
> > 28 +}
> >
> > This if is not really needed. Please remove it.
>
> OK, I've done this.
>
> >
> > 54 +void QuicklistManager::Activate()
> > 55 +{
> > 56 + if (_current_quicklist)
> > 57 + _current_quicklist->Activate();
> > 58 +}
> >
> > Wrong indentation. Please fix it :)
>
> OK, I've done this.
>
> >
> > After the adjustments i'm going to continue the review. Thanks in advance :)
>
> OK. Thanks.

review: Needs Fixing

[Alain Lessard (alales)]

On Wed, 2011-11-30 at 08:16 +0000, Andrea Azzarone wrote:
> Review: Needs Fixing
>
> > On Sat, 2011-11-26 at 01:41 +0000, Andrea Azzarone wrote:
> > > Review: Needs Fixing
> > >
> > > 17 +Launcher::OnExpoTerminated()
> > > 18 +{
> > > 19 + _hide_machine->SetQuirk (LauncherHideMachine::EXPO_ACTIVE,
> > WindowManager::Default ()->IsExpoActive ());
> > >
> > > Plese don't put a space between the function name and the open parenthesis.
> >
> > OK, I've done this.
> >
> > >
> > > 20 + _hide_machine->SetQuirk (LauncherHideMachine::SCALE_ACTIVE,
> > WindowManager::Default ()->IsScaleActive ());
> > >
> > > Is the above line really needed? We're in the "OnExpoTerminated" function...
> >
> > Prior to my modification, this line was used in the method
> > Launcher::OnPluginStateChanged()
> >
> > That method was connected to the terminate_expo event
> >
> > My intention was to use the same code plus the call to
> > QuicklistManager::Default()->Activate() that I've added, just to make sure
> > I don't break any existing features...
>
> I know but you still have the function OnPluginStateChanged that is called when the scale/spread plugin state change.
>
OK, this is done.
> >
> > >
> > > 26 + if (_hidemode == LAUNCHER_HIDE_NEVER)
> > > 27 + return;
> > > 28 +}
> > >
> > > This if is not really needed. Please remove it.
> >
> > OK, I've done this.
> >
> > >
> > > 54 +void QuicklistManager::Activate()
> > > 55 +{
> > > 56 + if (_current_quicklist)
> > > 57 + _current_quicklist->Activate();
> > > 58 +}
> > >
> > > Wrong indentation. Please fix it :)
> >
> > OK, I've done this.
> >
> > >
> > > After the adjustments i'm going to continue the review. Thanks in advance :)
> >
> > OK. Thanks.

[lp:~alales/unity/unity-4.0-fix-for-791810]

1720. By Alain Lessard on 2011-11-30

Modifications required by code review 2.

[Andrea Azzarone (azzar1)]

Ok the code looks good to me and it works well. I don't now if we need a test for it.

review: Approve

[Didier Roche-Tolomelli (didrocks)]

@Andrea: you'll need a test for this. At least a manual test and ideally an automated test. Can you handle that before approving the branch? Thanks :)

[Andrea Azzarone (azzar1)]

review: Needs Fixing

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

Works well, but we need a test.

Also, it would be cool if you could ask for merge against lp:unity too, since it suffers of the same bug as well.

Going back to tests, I'm not sure if also autopilot is enough here (speaking for the trunk branch), because even if it's easy to to reproduce it, then I'm not sure that we can easily and automatically check that the QL is actually freezed (maybe, using a fake quicklist and making the autopilot to click over one of its entries without getting any signal, but this needs some work).

So, at least for the unity/4.0 branch I guess that a manual test should be fine, so please compile one and add it to this branch following the guidelines that you can find at: http://bazaar.launchpad.net/~unity-team/unity/trunk/view/head:/manual-tests/ReadMe.txt

Thank you.

review: Needs Fixing

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

Hi Alain, thank you again for your code.

However, looking better at it, I've seen that we could have done this also without using workarounds to grab pointer or keyboard focus, but just making the quicklist to show only after that the expo plugin has been terminated.

The fixed code is now on my branch lp:~3v1n0/unity/quicklist-on-expo that is on review at [1] (and [2] for oneiric's unity), I encourage you to look at it.

I'm sorry to say this, but I've to reject your merge request, even if thanks to it I got the proper way to fix the bug you've reported.

[1] https://code.launchpad.net/~3v1n0/unity/quicklist-on-expo/+merge/84421
[2] https://code.launchpad.net/~3v1n0/unity/quicklist-on-expo.stable/+merge/84422

review: Disapprove

Unmerged revisions

1720. By Alain Lessard on 2011-11-30

Modifications required by code review 2.

1719. By Alain Lessard on 2011-11-30

Modifications required by code review 1.

1718. By Alain Lessard on 2011-11-09

reactivate the current quicklist when Expo exit on right-clicking,
otherwise another window will be active and the quicklist will
freeze (bug #791810).

=== modified file 'plugins/unityshell/src/Launcher.cpp'
--- plugins/unityshell/src/Launcher.cpp	2011-09-29 03:45:59 +0000
+++ plugins/unityshell/src/Launcher.cpp	2011-11-30 15:59:26 +0000
@@ -183,7 +183,7 @@
   plugin_adapter.initiate_spread.connect(sigc::mem_fun(this, &Launcher::OnPluginStateChanged));
   plugin_adapter.initiate_expo.connect(sigc::mem_fun(this, &Launcher::OnPluginStateChanged));
   plugin_adapter.terminate_spread.connect(sigc::mem_fun(this, &Launcher::OnPluginStateChanged));
-  plugin_adapter.terminate_expo.connect(sigc::mem_fun(this, &Launcher::OnPluginStateChanged));
+  plugin_adapter.terminate_expo.connect(sigc::mem_fun(this, &Launcher::OnExpoTerminated));
   plugin_adapter.compiz_screen_viewport_switch_started.connect(sigc::mem_fun(this, &Launcher::OnViewPortSwitchStarted));
   plugin_adapter.compiz_screen_viewport_switch_ended.connect(sigc::mem_fun(this, &Launcher::OnViewPortSwitchEnded));
 
@@ -1702,6 +1702,16 @@
 }
 
 void
+Launcher::OnExpoTerminated()
+{
+  _hide_machine->SetQuirk(LauncherHideMachine::EXPO_ACTIVE, WindowManager::Default()->IsExpoActive());
+
+  // reactivate the current quicklist when Expo exit on right-clicking.
+  // otherwise another window will be active and the quicklist will freeze (bug #791810).
+  QuicklistManager::Default()->Activate();
+}
+
+void
 Launcher::OnViewPortSwitchStarted()
 {
   /*

=== modified file 'plugins/unityshell/src/Launcher.h'
--- plugins/unityshell/src/Launcher.h	2011-09-28 16:39:36 +0000
+++ plugins/unityshell/src/Launcher.h	2011-11-30 15:59:26 +0000
@@ -252,6 +252,7 @@
   void OnDragFinish(GeisAdapter::GeisDragData* data);
 
   void OnPluginStateChanged();
+  void OnExpoTerminated();
 
   void OnViewPortSwitchStarted();
   void OnViewPortSwitchEnded();

=== modified file 'plugins/unityshell/src/QuicklistManager.cpp'
--- plugins/unityshell/src/QuicklistManager.cpp	2011-09-06 18:30:26 +0000
+++ plugins/unityshell/src/QuicklistManager.cpp	2011-11-30 15:59:26 +0000
@@ -58,6 +58,12 @@
   return _current_quicklist;
 }
 
+void QuicklistManager::Activate()
+{
+  if (_current_quicklist)
+    _current_quicklist->Activate();
+}
+
 void QuicklistManager::RegisterQuicklist(QuicklistView* quicklist)
 {
   std::list<QuicklistView*>::iterator it;

=== modified file 'plugins/unityshell/src/QuicklistManager.h'
--- plugins/unityshell/src/QuicklistManager.h	2011-08-18 04:22:08 +0000
+++ plugins/unityshell/src/QuicklistManager.h	2011-11-30 15:59:26 +0000
@@ -32,6 +32,8 @@
 
   QuicklistView* Current();
 
+  void Activate();
+
   void RegisterQuicklist(QuicklistView* quicklist);
   void ShowQuicklist(QuicklistView* quicklist, int tip_x, int tip_y, bool hide_existing_if_open = true);
   void HideQuicklist(QuicklistView* quicklist);

=== modified file 'plugins/unityshell/src/QuicklistView.cpp'
--- plugins/unityshell/src/QuicklistView.cpp	2011-09-06 18:30:26 +0000
+++ plugins/unityshell/src/QuicklistView.cpp	2011-11-30 15:59:26 +0000
@@ -331,6 +331,19 @@
   BaseWindow::ShowWindow(b, start_modal);
 }
 
+void QuicklistView::Activate()
+{
+  // FIXME: ShowWindow shouldn't need to be called first
+  ShowWindow(true);
+  PushToFront();
+  //EnableInputWindow (true, "quicklist", false, true);
+  //SetInputFocus ();
+  GrabPointer();
+  GrabKeyboard();
+  QueueDraw();
+  _compute_blur_bkg = true;
+}
+
 void QuicklistView::Show()
 {
   if (!IsVisible())

=== modified file 'plugins/unityshell/src/QuicklistView.h'
--- plugins/unityshell/src/QuicklistView.h	2011-09-06 18:30:26 +0000
+++ plugins/unityshell/src/QuicklistView.h	2011-11-30 15:59:26 +0000
@@ -79,6 +79,7 @@
   void ShowQuicklistWithTipAt(int anchor_tip_x, int anchor_tip_y);
   virtual void ShowWindow(bool b, bool StartModal = false);
 
+  void Activate();
   void Show();
   void Hide();