Unity

Merge lp://staging/~azzar1/unity/fix-919567 into lp://staging/unity

  1. fix-919567
  2. Merge into trunk
Proposed by Andrea Azzarone
Status: Merged
Approved by: Mikkel Kamstrup Erlandsen
Approved revision: no longer in the source branch.
Merged at revision: 1884
Proposed branch: lp://staging/~azzar1/unity/fix-919567
Merge into: lp://staging/unity
Diff against target: 186 lines (+39/-7)
10 files modified
manual-tests/Dash.txt (+16/-0)
plugins/unityshell/src/DashSearchBar.cpp (+1/-1)
plugins/unityshell/src/FilterBasicButton.cpp (+11/-5)
plugins/unityshell/src/FilterBasicButton.h (+1/-0)
plugins/unityshell/src/FilterExpanderLabel.cpp (+3/-1)
plugins/unityshell/src/FilterMultiRangeButton.cpp (+2/-0)
plugins/unityshell/src/FilterRatingsButton.cpp (+1/-0)
plugins/unityshell/src/FilterWidget.cpp (+1/-0)
plugins/unityshell/src/Launcher.cpp (+1/-0)
plugins/unityshell/src/LensBarIcon.cpp (+2/-0)
To merge this branch: bzr merge lp://staging/~azzar1/unity/fix-919567
Reviewer Review Type Date Requested Status
Mikkel Kamstrup Erlandsen (community) Approve
Mirco Müller (community) Needs Fixing
Review via email: mp+90340@code.staging.launchpad.net

Description of the change

Make sure that the dash searchbar doesn't lose the focus when clicking on an empty dash area (or an area that should not get the focus on mouse down).

SetAccepetKeyNavFocus is already tested in trunk.

UNBLOCK

To post a comment you must log in.
Revision history for this message
Mirco Müller (macslow) wrote :

Even without this branch it already works with trunk. So why is this branch needed?

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

> Even without this branch it already works with trunk. So why is this branch
> needed?

To test it:
- Open the app lens
- Expand the filter bar
- Make sure the dash has the focus
- The cursor should blink
- Click (for example) between a filter expander and the All Button
  (for example between Type > and All)
- The cursor is still blinking.

Revision history for this message
Mirco Müller (macslow) wrote :

Works... add that description above as a manual-test and I'm good to approve it.

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

> Works... add that description above as a manual-test and I'm good to approve
> it.

Done.

Revision history for this message
Mikkel Kamstrup Erlandsen (kamstrup) wrote :

Why not include a test for the other bug you've linked, that is, bug #923988?

Also - the fix for this bug seems more like a band aid than a real fix. Is the bug not that the text changes on unrelated events?

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

> Why not include a test for the other bug you've linked, that is, bug #923988?
>

I've updated the manual test for the moment.

> Also - the fix for this bug seems more like a band aid than a real fix. Is the
> bug not that the text changes on unrelated events?

The text changes because the dash bar loses the key focus.

Revision history for this message
Mikkel Kamstrup Erlandsen (kamstrup) wrote :

> > Also - the fix for this bug seems more like a band aid than a real fix. Is
> the
> > bug not that the text changes on unrelated events?
>
> The text changes because the dash bar loses the key focus.

Exactly. That is what I consider to be the real bug. This patch fixes the symptoms but not the cause :-)

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

> > > Also - the fix for this bug seems more like a band aid than a real fix. Is
> > the
> > > bug not that the text changes on unrelated events?
> >
> > The text changes because the dash bar loses the key focus.
>
> Exactly. That is what I consider to be the real bug. This patch fixes the
> symptoms but not the cause :-)

Then it's a general problem. We should create two bugs:

- The searchbar should not lose the focus when clicking on a filter button
- The searchbar text should not change when the searchbar loses the focus.

Revision history for this message
Mikkel Kamstrup Erlandsen (kamstrup) wrote :

Agreed.

Code looks good to me.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.