Merge lp://staging/~azzar1/unity/inverse-mouse-area-previews into lp://staging/unity/phablet

Proposed by Andrea Azzarone
Status: Work in progress
Proposed branch: lp://staging/~azzar1/unity/inverse-mouse-area-previews
Merge into: lp://staging/unity/phablet
Diff against target: 159 lines (+50/-30)
5 files modified
Dash/DashContent.qml (+0/-1)
Dash/DashPeople.qml (+1/-12)
Dash/DashPreview.qml (+9/-0)
Dash/DashVideos.qml (+3/-15)
tests/qmluitests/tst_DashPreview.qml (+37/-2)
To merge this branch: bzr merge lp://staging/~azzar1/unity/inverse-mouse-area-previews
Reviewer Review Type Date Requested Status
Michael Zanetti (community) Needs Fixing
Michał Sawicz Needs Fixing
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+157557@code.staging.launchpad.net

Commit message

Use InverseMouseArea for DashPreview (remove two TODOs).

Description of the change

== Test ==
Unit test added.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Michał Sawicz (saviq) wrote :

Hey, please move the calls to .clear() to an init() function in the TestCase

547. By Andrea Azzarone

Minor change.

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

> Hey, please move the calls to .clear() to an init() function in the TestCase

Done.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Michał Sawicz (saviq) wrote :

What is the problem with the InverseMouseArea?

Also, shouldn't this be a separate tst_DashContent test?

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

InverseMouseArea fails to detect click outside its area in the test, maybe
a problem with event filtering?

Why? we are testing dash previews here.
Il giorno 08/apr/2013 01:44, "Michał Sawicz" <email address hidden>
ha scritto:

> Review: Needs Information
>
> What is the problem with the InverseMouseArea?
>
> Also, shouldn't this be a separate tst_DashContent test?
> --
>
> https://code.launchpad.net/~andyrock/unity/inverse-mouse-area-previews/+merge/157557
> You are the owner of lp:~andyrock/unity/inverse-mouse-area-previews.
>

Revision history for this message
Michał Sawicz (saviq) wrote :

I think your InverseMouseArea issue is with the fact that the DashContent is 0x0 size, so the sensing area is thus 0x0.

The DashPreview itself should not know / care about currentIndex - it's the Dash* components that should. The DashPreview could in theory be used outside of any ListView (not to mention outside of DashContent).

I don't think we need a DashContent here at all.

For future reference - you're accessing dashContent in DashPreview, which means accessing objects from out of DashPreview's scope directly. You should instead use properties / signals on the DashPreview to maintain encapsulation.

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

> I think your InverseMouseArea issue is with the fact that the DashContent is
> 0x0 size, so the sensing area is thus 0x0.

Nope, it does not work even removing sensingArea: dashContent.

>
> The DashPreview itself should not know / care about currentIndex - it's the
> Dash* components that should. The DashPreview could in theory be used outside
> of any ListView (not to mention outside of DashContent).
>
> I don't think we need a DashContent here at all.
>
> For future reference - you're accessing dashContent in DashPreview, which
> means accessing objects from out of DashPreview's scope directly. You should
> instead use properties / signals on the DashPreview to maintain encapsulation.

Revision history for this message
Michał Sawicz (saviq) wrote :

There seems to be a bug in the SDK, (see bug #1166127) - you can add an explicit sensingArea that spans the whole component for now.

548. By Andrea Azzarone

Merge trunk.

549. By Andrea Azzarone

Improve FIXME comment.

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

> > The DashPreview itself should not know / care about currentIndex - it's the
> > Dash* components that should. The DashPreview could in theory be used
> outside
> > of any ListView (not to mention outside of DashContent).
> >
> > I don't think we need a DashContent here at all.
> >
> > For future reference - you're accessing dashContent in DashPreview, which
> > means accessing objects from out of DashPreview's scope directly. You should
> > instead use properties / signals on the DashPreview to maintain
> encapsulation.

For the moment I removed that change. This is actually another bug, will propose a new branch soon.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Michał Sawicz (saviq) wrote :

What's the MouseArea for? And the yellow Recatangle?

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

> What's the MouseArea for? And the yellow Recatangle?

I can remove the yellow Rectangle but without MouseArea how can I test that clicking outside the preview emits the close signal?

Revision history for this message
Michał Sawicz (saviq) wrote :

Hey, there's a conflict in tst_DashPreview.qml, can you please fix?

review: Needs Fixing
Revision history for this message
Michał Sawicz (saviq) wrote :

> I can remove the yellow Rectangle but without MouseArea how can I test
> that clicking outside the preview emits the close signal?

The InverseMouseArea will take care of that, you don't need the MouseArea, you just need to try and deliver a click to the preview's dismissArea.

mouseClick(preview.dismissArea, 1, 1)

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

Sweet. I'll fix it ASAP
Il giorno 15/apr/2013 15:25, "Michał Sawicz" <email address hidden>
ha scritto:

> Review: Needs Fixing
>
> > I can remove the yellow Rectangle but without MouseArea how can I test
> > that clicking outside the preview emits the close signal?
>
> The InverseMouseArea will take care of that, you don't need the MouseArea,
> you just need to try and deliver a click to the preview's dismissArea.
>
> mouseClick(preview.dismissArea, 1, 1)
> --
>
> https://code.launchpad.net/~andyrock/unity/inverse-mouse-area-previews/+merge/157557
> You are the owner of lp:~andyrock/unity/inverse-mouse-area-previews.
>

Revision history for this message
Michael Zanetti (mzanetti) wrote :

Its a good start, however, I don't think this should be merged as long as it just replaces the TODO's with FIXME's

49 + // FIXME: Workaround for bug: https://bugs.launchpad.net/ubuntu-ui-toolkit/+bug/1166127

Better wait until the InverseMouseArea is really fixed and fix this properly afterwards...

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

Ok.

Unmerged revisions

549. By Andrea Azzarone

Improve FIXME comment.

548. By Andrea Azzarone

Merge trunk.

547. By Andrea Azzarone

Minor change.

546. By Andrea Azzarone

Allow to use notification bar when a preview is open.

545. By Andrea Azzarone

Use InverseMouseArea.

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