Unity

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

  1. fix-1028810
  2. Merge into trunk
Proposed by Andrea Azzarone
Status: Merged
Approved by: Omer Akram
Approved revision: no longer in the source branch.
Merged at revision: 2619
Proposed branch: lp:~azzar1/unity/fix-1028810
Merge into: lp:unity
Diff against target: 181 lines (+68/-11)
6 files modified
dash/DashView.cpp (+5/-2)
dash/FilterExpanderLabel.cpp (+11/-8)
dash/FilterExpanderLabel.h (+2/-0)
dash/FilterRatingsButton.cpp (+2/-1)
tests/autopilot/unity/emulators/dash.py (+20/-0)
tests/autopilot/unity/tests/test_dash.py (+28/-0)
To merge this branch: bzr merge lp:~azzar1/unity/fix-1028810
Reviewer Review Type Date Requested Status
Brandon Schaefer (community) Approve
Tim Penhey (community) Needs Information
Francis Ginther Abstain
jenkins (community) continuous-integration Needs Fixing
Review via email: mp+116744@code.launchpad.net

Commit message

Fix bottom-up key navigation in dash filterbar.

Description of the change

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

== Test ==
AP test added.

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

To post a comment you must log in.
Revision history for this message
jenkins (martin-mrazik+qa) wrote :

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)
Revision history for this message
Martin Mrazik (mrazik) wrote :

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

Revision history for this message
Francis Ginther (fginther) wrote :

Review was claimed by accident, please ignore.

review: Abstain
Revision history for this message
Tim Penhey (thumper) wrote :

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

Did you mean "if (filter)" ?

review: Needs Information
Revision history for this message
Andrea Azzarone (azzar1) wrote :

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

Yes. Fixing.

Revision history for this message
Andrea Azzarone (azzar1) wrote :

Fixed.

Revision history for this message
Brandon Schaefer (brandontschaefer) wrote :

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
Revision history for this message
Andrea Azzarone (azzar1) wrote :

> 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.

Revision history for this message
Brandon Schaefer (brandontschaefer) wrote :

Awesome then! Looks good!

review: Approve
Revision history for this message
Unity Merger (unity-merger) wrote :

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.

Revision history for this message
Unity Merger (unity-merger) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'dash/DashView.cpp'
2--- dash/DashView.cpp 2012-08-22 10:04:18 +0000
3+++ dash/DashView.cpp 2012-08-22 14:09:41 +0000
4@@ -29,6 +29,7 @@
5 #include <UnityCore/GLibWrapper.h>
6 #include <UnityCore/RadioOptionFilter.h>
7
8+#include "FilterExpanderLabel.h"
9 #include "unity-shared/DashStyle.h"
10 #include "unity-shared/KeyboardUtil.h"
11 #include "unity-shared/UnitySettings.h"
12@@ -969,9 +970,11 @@
13 if (active_lens_view_->filter_bar() && active_lens_view_->fscroll_view() &&
14 active_lens_view_->fscroll_view()->IsVisible())
15 {
16- for (auto filter : active_lens_view_->filter_bar()->GetLayout()->GetChildren())
17+ for (auto child : active_lens_view_->filter_bar()->GetLayout()->GetChildren())
18 {
19- tabs.push_back(filter);
20+ FilterExpanderLabel* filter = dynamic_cast<FilterExpanderLabel*>(child);
21+ if (filter)
22+ tabs.push_back(filter->expander_view());
23 }
24 }
25
26
27=== modified file 'dash/FilterExpanderLabel.cpp'
28--- dash/FilterExpanderLabel.cpp 2012-05-07 22:28:17 +0000
29+++ dash/FilterExpanderLabel.cpp 2012-08-22 14:09:41 +0000
30@@ -96,7 +96,6 @@
31 {
32 expanded.changed.connect(sigc::mem_fun(this, &FilterExpanderLabel::DoExpandChange));
33 BuildLayout();
34- SetAcceptKeyNavFocusOnMouseDown(false);
35
36 separator_ = new HSeparator;
37 separator_->SinkReference();
38@@ -227,11 +226,6 @@
39 expander_view_->key_nav_focus_activate.connect(key_expand);
40 cairo_label_->mouse_click.connect(mouse_expand);
41 expand_icon_->mouse_click.connect(mouse_expand);
42- key_nav_focus_change.connect([&](nux::Area* area, bool has_focus, nux::KeyNavDirection direction)
43- {
44- if(has_focus)
45- nux::GetWindowCompositor().SetKeyFocusArea(expander_view_);
46- });
47
48 QueueRelayout();
49 NeedRedraw();
50@@ -304,7 +298,7 @@
51 //
52 bool FilterExpanderLabel::AcceptKeyNavFocus()
53 {
54- return true;
55+ return false;
56 }
57
58 //
59@@ -317,9 +311,18 @@
60
61 void FilterExpanderLabel::AddProperties(GVariantBuilder* builder)
62 {
63+ bool content_has_focus = false;
64+ auto focus_area = nux::GetWindowCompositor().GetKeyFocusArea();
65+
66+ if (focus_area && contents_)
67+ content_has_focus = focus_area->IsChildOf(contents_.GetPointer());
68+
69 unity::variant::BuilderWrapper wrapper(builder);
70
71- wrapper.add("expander-has-focus", expander_view_->HasKeyFocus());
72+ wrapper.add("expander-has-focus", (expander_view_ && expander_view_->HasKeyFocus()))
73+ .add("expanded", expanded())
74+ .add(GetAbsoluteGeometry())
75+ .add("content-has-focus", content_has_focus);
76 }
77
78 } // namespace dash
79
80=== modified file 'dash/FilterExpanderLabel.h'
81--- dash/FilterExpanderLabel.h 2012-05-06 23:48:38 +0000
82+++ dash/FilterExpanderLabel.h 2012-08-22 14:09:41 +0000
83@@ -63,6 +63,8 @@
84 virtual void SetFilter(Filter::Ptr const& filter) = 0;
85 virtual std::string GetFilterType() = 0;
86
87+ nux::View* expander_view() const { return expander_view_; }
88+
89 nux::Property<bool> expanded;
90 nux::Property<bool> draw_separator;
91
92
93=== modified file 'dash/FilterRatingsButton.cpp'
94--- dash/FilterRatingsButton.cpp 2012-05-06 23:48:38 +0000
95+++ dash/FilterRatingsButton.cpp 2012-08-22 14:09:41 +0000
96@@ -51,13 +51,14 @@
97
98 key_nav_focus_change.connect([&](nux::Area* area, bool has_focus, nux::KeyNavDirection direction)
99 {
100- if (has_focus && direction != nux::KEY_NAV_NONE)
101+ if (has_focus)
102 focused_star_ = 0;
103 else if (!has_focus)
104 focused_star_ = -1;
105
106 QueueDraw();
107 });
108+
109 key_nav_focus_activate.connect([&](nux::Area*) { filter_->rating = static_cast<float>(focused_star_+1)/num_stars; });
110 key_down.connect(sigc::mem_fun(this, &FilterRatingsButton::OnKeyDown));
111 }
112
113=== modified file 'tests/autopilot/unity/emulators/dash.py'
114--- tests/autopilot/unity/emulators/dash.py 2012-08-20 15:12:27 +0000
115+++ tests/autopilot/unity/emulators/dash.py 2012-08-22 14:09:41 +0000
116@@ -332,6 +332,26 @@
117 class FilterExpanderLabel(UnityIntrospectionObject):
118 """A label that expands into a filter within a filter bar."""
119
120+ def ensure_expanded(self):
121+ """Expand the filter expander label, if it's not already"""
122+ if not self.expanded:
123+ tx = x + width / 2
124+ ty = y + height / 2
125+ m = Mouse()
126+ m.move(tx, ty)
127+ m.click()
128+ self.expanded.wait_for(True)
129+
130+ def ensure_collapsed(self):
131+ """Collapse the filter expander label, if it's not already"""
132+ if self.expanded:
133+ tx = x + width / 2
134+ ty = y + height / 2
135+ m = Mouse()
136+ m.move(tx, ty)
137+ m.click()
138+ self.expanded.wait_for(False)
139+
140
141 class CoverArt(UnityIntrospectionObject):
142 """A view which can be used to show a texture, or generate one using a thumbnailer."""
143
144=== modified file 'tests/autopilot/unity/tests/test_dash.py'
145--- tests/autopilot/unity/tests/test_dash.py 2012-08-22 08:16:02 +0000
146+++ tests/autopilot/unity/tests/test_dash.py 2012-08-22 14:09:41 +0000
147@@ -284,6 +284,34 @@
148 category = lens.get_focused_category()
149 self.assertIsNot(category, None)
150
151+ def test_bottom_up_keynav_with_filter_bar(self):
152+ """This test makes sure that bottom-up key navigation works well
153+ in the dash filter bar.
154+ """
155+ self.dash.reveal_application_lens()
156+ lens = self.dash.get_current_lens()
157+
158+ filter_bar = lens.get_filterbar()
159+ filter_bar.ensure_expanded()
160+
161+ # Tab to fist filter expander
162+ self.keyboard.press_and_release('Tab')
163+ self.assertThat(lambda: filter_bar.get_focused_filter(), Eventually(NotEquals(None)))
164+ old_focused_filter = filter_bar.get_focused_filter()
165+ old_focused_filter.ensure_expanded()
166+
167+ # Tab to the next filter expander
168+ self.keyboard.press_and_release('Tab')
169+ self.assertThat(lambda: filter_bar.get_focused_filter(), Eventually(NotEquals(None)))
170+ new_focused_filter = filter_bar.get_focused_filter()
171+ self.assertNotEqual(old_focused_filter, new_focused_filter)
172+ new_focused_filter.ensure_expanded()
173+
174+ # Move the focus up.
175+ self.keyboard.press_and_release("Up")
176+ self.assertThat(lambda: filter_bar.get_focused_filter(), Eventually(Equals(None)))
177+ self.assertThat(old_focused_filter.content_has_focus, Eventually(Equals(True)))
178+
179
180 class DashClipboardTests(DashTestCase):
181 """Test the Unity clipboard"""