Merge lp:~azzar1/unity/fix-1028810 into lp:unity

[Andrea Azzarone]

Fix bottom-up key navigation in dash filterbar.

== Problem ==
Bottom - Up key navigation is broken in dash filterbar

== Test ==
AP test added.

Requires lp:~andyrock/nux/fix-gridhlayout-bu-keynav.

[jenkins (martin-mrazik+qa)]

FAILED: Continuous integration, rev:2518
http://jenkins.qa.ubuntu.com/job/unity-ci/123/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-ci/./distribution=quantal,flavor=amd64/123/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-ci/./distribution=quantal,flavor=i386/123/console

review: Needs Fixing (continuous-integration)

[Martin Mrazik (mrazik)]

Sorry for the jenkins failure. I'm looking into this.

[Francis Ginther (fginther)]

Review was claimed by accident, please ignore.

review: Abstain

[Tim Penhey (thumper)]

  FilterExpanderLabel* filter = dynamic_cast<FilterExpanderLabel*>(child);
  if (child)
    tabs.push_back(filter->expander_view());

Did you mean "if (filter)" ?

review: Needs Information

[Andrea Azzarone (azzar1)]

> FilterExpanderLabel* filter = dynamic_cast<FilterExpanderLabel*>(child);
> if (child)
> tabs.push_back(filter->expander_view());
>
> Did you mean "if (filter)" ?

Yes. Fixing.

[Andrea Azzarone (azzar1)]

Fixed.

[Brandon Schaefer (brandontschaefer)]

Could you split up that AP tests into smaller ones? Or split some of the logic into smaller ones so it will be easy to figure out what is causing a potential problem later on.

Such as making a test that proves that it is getting to Filter Results, then a test that shows it can tab to the first filter result ... etc.

review: Needs Fixing

[Andrea Azzarone (azzar1)]

> Could you split up that AP tests into smaller ones? Or split some of the logic
> into smaller ones so it will be easy to figure out what is causing a potential
> problem later on.

Hey I missed this comment, sorry!

>
> Such as making a test that proves that it is getting to Filter Results, then a
> test that shows it can tab to the first filter result ... etc.

We already have these tests.

[Brandon Schaefer (brandontschaefer)]

Awesome then! Looks good!

review: Approve

[Unity Merger (unity-merger)]

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-unity/1114/console reported an error when processing this lp:~andyrock/unity/fix-1028810 branch.
Not merging it.

[Unity Merger (unity-merger)]

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-unity/1115/console reported an error when processing this lp:~andyrock/unity/fix-1028810 branch.
Not merging it.

=== modified file 'dash/DashView.cpp'
--- dash/DashView.cpp	2012-08-22 10:04:18 +0000
+++ dash/DashView.cpp	2012-08-22 14:09:41 +0000
@@ -29,6 +29,7 @@
 #include <UnityCore/GLibWrapper.h>
 #include <UnityCore/RadioOptionFilter.h>
 
+#include "FilterExpanderLabel.h"
 #include "unity-shared/DashStyle.h"
 #include "unity-shared/KeyboardUtil.h"
 #include "unity-shared/UnitySettings.h"
@@ -969,9 +970,11 @@
     if (active_lens_view_->filter_bar() && active_lens_view_->fscroll_view() &&
         active_lens_view_->fscroll_view()->IsVisible())
     {
-      for (auto filter : active_lens_view_->filter_bar()->GetLayout()->GetChildren())
+      for (auto child : active_lens_view_->filter_bar()->GetLayout()->GetChildren())
       {
-        tabs.push_back(filter);
+        FilterExpanderLabel* filter = dynamic_cast<FilterExpanderLabel*>(child);
+        if (filter)
+          tabs.push_back(filter->expander_view());
       }
     }
 
 
=== modified file 'dash/FilterExpanderLabel.cpp'
--- dash/FilterExpanderLabel.cpp	2012-05-07 22:28:17 +0000
+++ dash/FilterExpanderLabel.cpp	2012-08-22 14:09:41 +0000
@@ -96,7 +96,6 @@
 {
   expanded.changed.connect(sigc::mem_fun(this, &FilterExpanderLabel::DoExpandChange));
   BuildLayout();
-  SetAcceptKeyNavFocusOnMouseDown(false);
 
   separator_ = new HSeparator;
   separator_->SinkReference();
@@ -227,11 +226,6 @@
   expander_view_->key_nav_focus_activate.connect(key_expand);
   cairo_label_->mouse_click.connect(mouse_expand);
   expand_icon_->mouse_click.connect(mouse_expand);
-  key_nav_focus_change.connect([&](nux::Area* area, bool has_focus, nux::KeyNavDirection direction)
-  {
-    if(has_focus)
-      nux::GetWindowCompositor().SetKeyFocusArea(expander_view_);
-  });
 
   QueueRelayout();
   NeedRedraw();
@@ -304,7 +298,7 @@
 //
 bool FilterExpanderLabel::AcceptKeyNavFocus()
 {
-  return true;
+  return false;
 }
 
 //
@@ -317,9 +311,18 @@
 
 void FilterExpanderLabel::AddProperties(GVariantBuilder* builder)
 {
+  bool content_has_focus = false;
+  auto focus_area = nux::GetWindowCompositor().GetKeyFocusArea();
+
+  if (focus_area && contents_)
+    content_has_focus = focus_area->IsChildOf(contents_.GetPointer());
+
   unity::variant::BuilderWrapper wrapper(builder);
 
-  wrapper.add("expander-has-focus", expander_view_->HasKeyFocus());
+  wrapper.add("expander-has-focus", (expander_view_ && expander_view_->HasKeyFocus()))
+         .add("expanded", expanded())
+         .add(GetAbsoluteGeometry())
+         .add("content-has-focus", content_has_focus);
 }
 
 } // namespace dash
 
=== modified file 'dash/FilterExpanderLabel.h'
--- dash/FilterExpanderLabel.h	2012-05-06 23:48:38 +0000
+++ dash/FilterExpanderLabel.h	2012-08-22 14:09:41 +0000
@@ -63,6 +63,8 @@
   virtual void SetFilter(Filter::Ptr const& filter) = 0;
   virtual std::string GetFilterType() = 0;
 
+  nux::View* expander_view() const { return expander_view_; }
+
   nux::Property<bool> expanded;
   nux::Property<bool> draw_separator;
 
 
=== modified file 'dash/FilterRatingsButton.cpp'
--- dash/FilterRatingsButton.cpp	2012-05-06 23:48:38 +0000
+++ dash/FilterRatingsButton.cpp	2012-08-22 14:09:41 +0000
@@ -51,13 +51,14 @@
 
   key_nav_focus_change.connect([&](nux::Area* area, bool has_focus, nux::KeyNavDirection direction)
   {
-    if (has_focus && direction != nux::KEY_NAV_NONE)
+    if (has_focus)
       focused_star_ = 0;
     else if (!has_focus)
       focused_star_ = -1;
 
     QueueDraw();
   });
+
   key_nav_focus_activate.connect([&](nux::Area*) { filter_->rating = static_cast<float>(focused_star_+1)/num_stars; });
   key_down.connect(sigc::mem_fun(this, &FilterRatingsButton::OnKeyDown));
 }
 
=== modified file 'tests/autopilot/unity/emulators/dash.py'
--- tests/autopilot/unity/emulators/dash.py	2012-08-20 15:12:27 +0000
+++ tests/autopilot/unity/emulators/dash.py	2012-08-22 14:09:41 +0000
@@ -332,6 +332,26 @@
 class FilterExpanderLabel(UnityIntrospectionObject):
     """A label that expands into a filter within a filter bar."""
 
+    def ensure_expanded(self):
+        """Expand the filter expander label, if it's not already"""
+        if not self.expanded:
+            tx = x + width / 2
+            ty = y + height / 2
+            m = Mouse()
+            m.move(tx, ty)
+            m.click()
+            self.expanded.wait_for(True)
+
+    def ensure_collapsed(self):
+        """Collapse the filter expander label, if it's not already"""
+        if self.expanded:
+            tx = x + width / 2
+            ty = y + height / 2
+            m = Mouse()
+            m.move(tx, ty)
+            m.click()
+            self.expanded.wait_for(False)
+
 
 class CoverArt(UnityIntrospectionObject):
     """A view which can be used to show a texture, or generate one using a thumbnailer."""
 
=== modified file 'tests/autopilot/unity/tests/test_dash.py'
--- tests/autopilot/unity/tests/test_dash.py	2012-08-22 08:16:02 +0000
+++ tests/autopilot/unity/tests/test_dash.py	2012-08-22 14:09:41 +0000
@@ -284,6 +284,34 @@
         category = lens.get_focused_category()
         self.assertIsNot(category, None)
 
+    def test_bottom_up_keynav_with_filter_bar(self):
+        """This test makes sure that bottom-up key navigation works well
+        in the dash filter bar.
+        """
+        self.dash.reveal_application_lens()
+        lens = self.dash.get_current_lens()
+
+        filter_bar = lens.get_filterbar()
+        filter_bar.ensure_expanded()
+
+        # Tab to fist filter expander
+        self.keyboard.press_and_release('Tab')
+        self.assertThat(lambda: filter_bar.get_focused_filter(), Eventually(NotEquals(None)))
+        old_focused_filter = filter_bar.get_focused_filter()
+        old_focused_filter.ensure_expanded()
+
+        # Tab to the next filter expander
+        self.keyboard.press_and_release('Tab')
+        self.assertThat(lambda: filter_bar.get_focused_filter(), Eventually(NotEquals(None)))
+        new_focused_filter = filter_bar.get_focused_filter()
+        self.assertNotEqual(old_focused_filter, new_focused_filter)
+        new_focused_filter.ensure_expanded()
+
+        # Move the focus up.
+        self.keyboard.press_and_release("Up")
+        self.assertThat(lambda: filter_bar.get_focused_filter(), Eventually(Equals(None)))
+        self.assertThat(old_focused_filter.content_has_focus, Eventually(Equals(True)))
+
 
 class DashClipboardTests(DashTestCase):
     """Test the Unity clipboard"""