Merge lp://qastaging/~unity-2d-team/unity-2d/filter-option-compact-shell into lp://qastaging/unity-2d

Proposed by Lohith D Shivamurthy
Status: Merged
Approved by: Gerry Boland
Approved revision: 938
Merged at revision: 938
Proposed branch: lp://qastaging/~unity-2d-team/unity-2d/filter-option-compact-shell
Merge into: lp://qastaging/unity-2d
Diff against target: 252 lines (+178/-2)
7 files modified
shell/dash/FilterCheckoption.qml (+4/-2)
shell/dash/FilterCheckoptionCompact.qml (+22/-0)
shell/dash/FilterLoader.qml (+2/-0)
shell/dash/FilterPane.qml (+1/-0)
shell/dash/FilterRadiooption.qml (+13/-0)
shell/dash/LensBar.qml (+1/-0)
tests/dash/test-renderer-filter-check-option-compact.rb (+135/-0)
To merge this branch: bzr merge lp://qastaging/~unity-2d-team/unity-2d/filter-option-compact-shell
Reviewer Review Type Date Requested Status
Gerry Boland (community) Approve
Michał Sawicz Pending
Review via email: mp+93346@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2012-01-24.

Description of the change

[shell][dash] Now we have a separate renderer filter-checkoption-compact. Hence the same change is incorparated in unity-2d-shell.

To post a comment you must log in.
Revision history for this message
Michał Sawicz (saviq) wrote : Posted in a previous version of this proposal

The tests rely on the fact that Application and Music lens as we know them have 2 and 3 columns, respectively.
That doesn't look like a reliable way to test, can you think of a better way? The easiest would probably be to go through the lenses / filters to find one that's filter-check-option and the other filter-check-option-compact.

Obviously that won't work when no lens uses those, but at some point we might have a fake testing lens that will have all of the different filter types for testing and the tests will still work, whereas we can't be sure people actually have the Applications or the Music lens.

Gerry, what's your take on it?

review: Needs Fixing
Revision history for this message
Gerry Boland (gerboland) wrote : Posted in a previous version of this proposal

The lenses are something we don't have control over. However in the absense of test lenses, we've got to assume *some* lenses are there. I'm happy assuming Applications, Documents & Music are there, as they're the defaults.

And to do a reliable test, we've got to know a lens with filter-check-option, and one with filter-check-option-compact. Applications & Music (resp.) are suitable. Cycling through them doesn't test something we know, only that we found a lens which claims to have 2 or 3 columns.

So I think these tests are sufficient.

Long term goal is to have a test lens, so we don't need to worry about lens changes.

Revision history for this message
Gerry Boland (gerboland) wrote : Posted in a previous version of this proposal

Comments on test script:

lines 28, 66, 111: Pet peeve: please use "lens" instead of "lense"
 - http://oxforddictionaries.com/spellcheck/?region=us&q=lense

line 29: 'pwd' not needed

lines 78, 123: Expand the comments like "Could not find AppLensButton" to ask if the relevant lens package is installed. Should help people who have removed it.

lines 82+, and others
+ XDo::Mouse.move(button['x_absolute'].to_i + 1, button['y_absolute'].to_i + 1, 0, true)
+ XDo::Mouse.click(nil, nil, :left)
Use button.tap, it's easier & safer.

line 86,130: no need to reset "button"

line 99:
+ verify( TIMEOUT, 'FilterCheckOption don\'t have two columns' ) {
+ loader.GridViewWithSpacing()['columns'] == '2'
+ }
Please use:
verify_equal( 2, TIMEOUT, 'FilterCheckOption don\'t have two columns' ) {
  loader.GridViewWithSpacing()['columns'].to_i
}
instead, as it returns better error output if it fails.

line 123: wrong comment

And some small English suggestions:
61: Check Filter results list view having renderer as filter-checkoption is displayed with two columns
   -> "Check Filter results list view rendered with filter-checkoption is displayed with two columns"
68: Check that filter-checkoption renderer is having two columns
   -> "Check that the filter-checkoption renderer uses two columns"
and similar.

review: Needs Fixing
Revision history for this message
Gerry Boland (gerboland) wrote : Posted in a previous version of this proposal

Ok now I've verified that Unity-Core has been updated to support this change, I've tested it and it's working.

Issue found: the Documents lens, the "Last Modified" checkboxes are not compact - but they should be according to the mockups.

This requires a fix in the documents lens. I've logged this bug here:
https://bugs.launchpad.net/unity-lens-files/+bug/928208
I'll hold off merging this until that bug is fixed.

I suggest in the mean time you keep this branch up to date.

Revision history for this message
Florian Boucault (fboucault) wrote : Posted in a previous version of this proposal

Please resubmit the MR against lp:unity-2d

Revision history for this message
Gerry Boland (gerboland) wrote :

Question: can your small hack be a little more specific to the Files lens? Right now, any lens filterModel with id "modified" will be changed with this hack.

Small comment change needed:
+ unity-core fixes the issue
It is the files lens which needs fixing, not unity-core.

review: Needs Fixing
Revision history for this message
Gerry Boland (gerboland) wrote :

Well I can answer my question: No easy way to make this more specific to Files lens, without changing filters API. This hack will do.

Also, from comments on the bug attached, it appears fixes to unity-core will be needed.
As a result I withdraw my objections and approve. Thanks!

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.

Subscribers

People subscribed via source and target branches