Merge lp:~mc-return/unity/unity.merge-fix-launcher-should-never-autohide-when-pointer-hovers-it into lp:unity

[MC Return]

Simplified the check if the launcher should hide again once it is revealed. It should never hide if

1. VISIBLE_REQUIRED is true or
2. MOUSE_OVER_LAUNCHER is true

There is no need to check for movement of the mouse (which is set to true once decaymulator_.value > reveal_pressure)

Also removed remains of the hide_modes DODGE_WINDOWS and DODGE_ACTIVE_WINDOW forgotten in rev1933, which removed the dodge window modes.

[MC Return (mc-return)]

Simplifies the check if the launcher should hide again once it is revealed. It should never hide if

1. _should_show_quirk is true or
2. VISIBLE_REQUIRED is true or
3. MOUSE_OVER_LAUNCHER is true

There is no need to check for movement of the mouse (which is set to true once decaymulator_.value > reveal_pressure)

[Didier Roche (didrocks)]

This is breaking this use case:
- put the mouse in the launcher area (launcher hidden)
- press super
- release it
-> the launcher will still be shown.
what it should do: it should hide as you triggered a keyboard-only event.

[Sam Spilsbury (smspillaz)]

Hey, looks good.

Do you know if this code is covered by unit tests? Or will tests need to be written?

[MC Return (mc-return)]

I did not check if it is already covered by unit tests, but I could write a manual test.

[Sam Spilsbury (smspillaz)]

Nah, unit tests are the way to go.

I had a look and its not covered by any so far. The good news is that it should be fairly easy to get this class under test.

Lets have a look at an existing testcase:

test_launcher.cpp

using namespace testing; // pulls in the ::testing namespace. I usually don't do this and prefer using ::testing::_; for the various bits I need but I guess its the unity style

#include <Nux/Nux.h>
#include <Nux/BaseWindow.h>

#include "launcher/Launcher.h" // include the stuff we need.
#include "test_utils.h"
using namespace unity;

namespace unity
{
namespace launcher
{

class TestLauncher : public Test // This is the test fixture class, so in your case you'd have TestLauncherHideMachine
{
public:
  ...

  TestLauncher()
    : ...
    , launcher_(new MockLauncher(parent_window_, dnd_collection_window_)) // initialize "MockLauncher" in our case it would be LauncherHideMachine
  {
    launcher_->options = options_;
    launcher_->SetModel(model_);
  }

...
  nux::ObjectPtr<MockLauncher> launcher_; // Storage for MockLaucnher
};

TEST_F(TestLauncher, TestQuirksDuringDnd) // Declares a new test for the TestLauncher test fixture
{
  MockMockLauncherIcon::Ptr first(new MockMockLauncherIcon); // Mock classes can have their function calls traced.
  model_->AddIcon(first);

  MockMockLauncherIcon::Ptr second(new MockMockLauncherIcon);
  model_->AddIcon(second);

  MockMockLauncherIcon::Ptr third(new MockMockLauncherIcon);
  model_->AddIcon(third);

  EXPECT_CALL(*first, ShouldHighlightOnDrag(_))
      .WillRepeatedly(Return(true));

  EXPECT_CALL(*second, ShouldHighlightOnDrag(_))
      .WillRepeatedly(Return(true));

  EXPECT_CALL(*third, ShouldHighlightOnDrag(_)) // Expecting calls on the mock class
      .WillRepeatedly(Return(false));

  std::list<char*> uris;
  dnd_collection_window_->collected.emit(uris);

  Utils::WaitForTimeout(1);

  EXPECT_FALSE(first->GetQuirk(launcher::AbstractLauncherIcon::Quirk::DESAT)); // Expecting the value of GetQuirk (launcher::AbstractLauncherIcon::Quirk::DEST) to be false
  EXPECT_FALSE(second->GetQuirk(launcher::AbstractLauncherIcon::Quirk::DESAT));
  EXPECT_TRUE(third->GetQuirk(launcher::AbstractLauncherIcon::Quirk::DESAT));
}

}
}

So in your case, you want to instantiate LauncherHideMachine and then check LauncherHideMachine::ShouldHide () is false after you call LauncherHideMachine::SetMode (AUTOHIDE) and LauncherHideMachine::SetQuirk (VISIBLE_REQUIRED | MOUSE_OVER_LAUNCHER)

As an aside, _should_show_quirk is unintialized when you hit this point in the code:
12 + _should_show_quirk = (HideQuirk)(_should_show_quirk | (VISIBLE_REQUIRED) | MOUSE_OVER_LAUNCHER);

So this can be expressed as:

12 + _should_show_quirk = (HideQuirk)((VISIBLE_REQUIRED) | MOUSE_OVER_LAUNCHER);

Bonus points for initializing _should_show_quirk when it is declared too. Eg

HideQuirk _should_show_quirk = DEFAULT;

[MC Return (mc-return)]

> Nah, unit tests are the way to go.
>
> I had a look and its not covered by any so far. The good news is that it
> should be fairly easy to get this class under test.
>

Okay, I'll try it.

> Lets have a look at an existing testcase:
>
> test_launcher.cpp
>
> using namespace testing; // pulls in the ::testing namespace. I usually don't
> do this and prefer using ::testing::_; for the various bits I need but I guess
> its the unity style
>
> #include <Nux/Nux.h>
> #include <Nux/BaseWindow.h>
>
> #include "launcher/Launcher.h" // include the stuff we need.
> #include "test_utils.h"
> using namespace unity;
>
> namespace unity
> {
> namespace launcher
> {
>
> class TestLauncher : public Test // This is the test fixture class, so in your
> case you'd have TestLauncherHideMachine
> {
> public:
> ...
>
> TestLauncher()
> : ...
> , launcher_(new MockLauncher(parent_window_, dnd_collection_window_)) //
> initialize "MockLauncher" in our case it would be LauncherHideMachine
> {
> launcher_->options = options_;
> launcher_->SetModel(model_);
> }
>
> ...
> nux::ObjectPtr<MockLauncher> launcher_; // Storage for MockLaucnher
> };
>
> TEST_F(TestLauncher, TestQuirksDuringDnd) // Declares a new test for the
> TestLauncher test fixture
> {
> MockMockLauncherIcon::Ptr first(new MockMockLauncherIcon); // Mock classes
> can have their function calls traced.
> model_->AddIcon(first);
>
> MockMockLauncherIcon::Ptr second(new MockMockLauncherIcon);
> model_->AddIcon(second);
>
> MockMockLauncherIcon::Ptr third(new MockMockLauncherIcon);
> model_->AddIcon(third);
>
> EXPECT_CALL(*first, ShouldHighlightOnDrag(_))
> .WillRepeatedly(Return(true));
>
> EXPECT_CALL(*second, ShouldHighlightOnDrag(_))
> .WillRepeatedly(Return(true));
>
> EXPECT_CALL(*third, ShouldHighlightOnDrag(_)) // Expecting calls on the mock
> class
> .WillRepeatedly(Return(false));
>
> std::list<char*> uris;
> dnd_collection_window_->collected.emit(uris);
>
> Utils::WaitForTimeout(1);
>
> EXPECT_FALSE(first->GetQuirk(launcher::AbstractLauncherIcon::Quirk::DESAT));
> // Expecting the value of GetQuirk
> (launcher::AbstractLauncherIcon::Quirk::DEST) to be false
> EXPECT_FALSE(second->GetQuirk(launcher::AbstractLauncherIcon::Quirk::DESAT));
> EXPECT_TRUE(third->GetQuirk(launcher::AbstractLauncherIcon::Quirk::DESAT));
> }
>
> }
> }
>
>
> So in your case, you want to instantiate LauncherHideMachine and then check
> LauncherHideMachine::ShouldHide () is false after you call
> LauncherHideMachine::SetMode (AUTOHIDE) and LauncherHideMachine::SetQuirk
> (VISIBLE_REQUIRED | MOUSE_OVER_LAUNCHER)
>

Thanks for the detailed explanation. This is really appreciated :)

> As an aside, _should_show_quirk is unintialized when you hit this point in the
> code:
> 12 + _should_show_quirk = (HideQuirk)(_should_show_quirk |
> (VISIBLE_REQUIRED) | MOUSE_OVER_LAUNCHER);
>
> So this can be expressed as:
>
> 12 + _should_show_quirk = (HideQuirk)((VISIBLE_REQUIRED) |
> MOUSE_OVER_LAUNCHER);
>
> Bonus points for initializing _should_show_quirk when it is declared too. Eg
>
> HideQuirk _should_show_quirk = DEFAU...

[lp:~mc-return/unity/unity.merge-fix-launcher-should-never-autohide-when-pointer-hovers-it]

2660. By MC Return on 2012-09-07

Fixed usage of the uninitialized variable _should_show_quirk

2661. By MC Return on 2012-09-07

More simplifying for better readability

2662. By MC Return on 2012-09-07

Removed DODGE_WINDOWS and DODGE_ACTIVE_WINDOW because these modes are not available anymore

Unmerged revisions

2662. By MC Return on 2012-09-07

Removed DODGE_WINDOWS and DODGE_ACTIVE_WINDOW because these modes are not available anymore

2661. By MC Return on 2012-09-07

More simplifying for better readability

2660. By MC Return on 2012-09-07

Fixed usage of the uninitialized variable _should_show_quirk

2659. By MC Return on 2012-09-07

Rebased on latest lp:unity

2658. By MC Return on 2012-09-07

Simplify the check if the launcher should hide again once it is revealed. It should never hide if
1. _should_show_quirk is true or
2. VISIBLE_REQUIRED is true or
3. MOUSE_OVER_LAUNCHER is true

There is no need to check for movement of the mouse (which is set to true once decaymulator_.value > reveal_pressure)

=== modified file 'launcher/LauncherHideMachine.cpp'
--- launcher/LauncherHideMachine.cpp	2012-08-16 23:57:49 +0000
+++ launcher/LauncherHideMachine.cpp	2012-09-07 15:39:20 +0000
@@ -141,14 +141,11 @@
     HideQuirk _should_show_quirk;
     if (GetQuirk(LAUNCHER_HIDDEN))
     {
-      _should_show_quirk = (HideQuirk) ((VISIBLE_REQUIRED) | REVEAL_PRESSURE_PASS);
+      _should_show_quirk = (HideQuirk)(VISIBLE_REQUIRED | REVEAL_PRESSURE_PASS);
     }
     else
     {
-      _should_show_quirk = (HideQuirk)(VISIBLE_REQUIRED);
-      // mouse position over launcher is only taken into account if we move it after the revealing state
-      if (GetQuirk(MOUSE_MOVE_POST_REVEAL))
-        _should_show_quirk = (HideQuirk)(_should_show_quirk | MOUSE_OVER_LAUNCHER);
+      _should_show_quirk = (HideQuirk)(VISIBLE_REQUIRED | MOUSE_OVER_LAUNCHER);
     }
 
     if (GetQuirk(_should_show_quirk))

=== modified file 'launcher/LauncherHideMachine.h'
--- launcher/LauncherHideMachine.h	2012-08-10 17:57:50 +0000
+++ launcher/LauncherHideMachine.h	2012-09-07 15:39:20 +0000
@@ -39,8 +39,6 @@
   {
     HIDE_NEVER,
     AUTOHIDE,
-    DODGE_WINDOWS,
-    DODGE_ACTIVE_WINDOW,
   } HideMode;
 
   typedef enum