Merge lp://staging/~macslow/unity8/swipe-dismiss-snap-decisions into lp://staging/unity8

Proposed by Mirco Müller
Status: Merged
Approved by: Michał Sawicz
Approved revision: 1285
Merged at revision: 1599
Proposed branch: lp://staging/~macslow/unity8/swipe-dismiss-snap-decisions
Merge into: lp://staging/unity8
Diff against target: 1554 lines (+836/-252)
15 files modified
qml/Notifications/Notification.qml (+36/-6)
qml/Notifications/Notifications.qml (+2/-6)
tests/mocks/Unity/Notifications/CMakeLists.txt (+2/-1)
tests/mocks/Unity/Notifications/MockActionModel.cpp (+6/-4)
tests/mocks/Unity/Notifications/MockActionModel.h (+4/-2)
tests/mocks/Unity/Notifications/MockNotification.cpp (+156/-4)
tests/mocks/Unity/Notifications/MockNotification.h (+63/-5)
tests/mocks/Unity/Notifications/MockNotificationModel.cpp (+172/-0)
tests/mocks/Unity/Notifications/MockNotificationModel.h (+72/-0)
tests/mocks/Unity/Notifications/plugin.cpp (+5/-3)
tests/mocks/Unity/Notifications/plugin.h (+1/-2)
tests/qmltests/Notifications/Notification.qml (+31/-0)
tests/qmltests/Notifications/tst_Notifications.qml (+271/-213)
tests/qmltests/Notifications/tst_OptionToggle.qml (+11/-6)
tests/qmltests/Notifications/tst_SwipeToAct.qml (+4/-0)
To merge this branch: bzr merge lp://staging/~macslow/unity8/swipe-dismiss-snap-decisions
Reviewer Review Type Date Requested Status
Michał Sawicz tests Approve
PS Jenkins bot (community) continuous-integration Approve
Albert Astals Cid (community) Approve
Michael Zanetti (community) Abstain
Antti Kaijanmäki (community) Needs Information
Review via email: mp+233347@code.staging.launchpad.net

Commit message

Allow swipe-to-dismiss for contracted snap-decision notifications, interactive notifications and ephemeral notifications.

Description of the change

Allow swipe-to-dismiss for contracted snap-decision notifications, interactive notifications and ephemeral notifications.

For the reviewers convenience, here's a video of the branch in action: https://www.youtube.com/watch?v=3kHuR-3Mns0

* Are there any related MPs required for this MP to build/function as expected? Please list.
No.

* Did you perform an exploratory manual test run of your code change and any related functionality?
Yes.

* Did you make sure that your branch does not contain spurious tags?
Yes.

* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
Not applicable.

* If you changed the UI, has there been a design review?
Yes.

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
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Albert Astals Cid (aacid) wrote :

Text conflict in qml/Notifications/Notification.qml
1 conflicts encountered.

Revision history for this message
Albert Astals Cid (aacid) wrote :

Text conflict in qml/Notifications/Notification.qml
Text conflict in tests/qmltests/Notifications/tst_Notifications.qml
2 conflicts encountered.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Antti Kaijanmäki (kaijanmaki) wrote :

+ readonly property bool draggable: state == "contracted" || notification.type !== Notification.Confirmation

Does this cover pin-unlock dialog as well? I don't have the code at hand right now to check if it's type is set to Notification.Confirmation as otherwise it becomes draggable.

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

> + readonly property bool draggable: state == "contracted" ||
> notification.type !== Notification.Confirmation
>
>
> Does this cover pin-unlock dialog as well? I don't have the code at hand right
> now to check if it's type is set to Notification.Confirmation as otherwise it
> becomes draggable.

No, that won't happen. A SIM-unlock "dialog" is an expanded (or non-contractable) snap-decision, thus it cannot be dragged.

Revision history for this message
Albert Astals Cid (aacid) wrote :

file:///tmp/buildd/unity8-8.01+15.04.20141104bzr1236pkg0vivid36/tests/qmltests/Notifications/tst_Notifications.qml:672:13: Unexpected token `if'
                 if (data.type !== Notification.SnapDecision && notification.state !== "expanded") {
                 ^
file:///tmp/buildd/unity8-8.01+15.04.20141104bzr1236pkg0vivid36/tests/qmltests/Notifications/tst_Notifications.qml:672:16: Expected a qualified name id
                 if (data.type !== Notification.SnapDecision && notification.state !== "expanded") {

Revision history for this message
Albert Astals Cid (aacid) wrote :

make testNotifications fails here

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

> make testNotifications fails here

Like discussed on IRC, I can't reproduce your reported failures. Everything works and passes as expected.

Revision history for this message
Albert Astals Cid (aacid) wrote :

> > make testNotifications fails here
>
> Like discussed on IRC, I can't reproduce your reported failures. Everything
> works and passes as expected.

Barring that for some reasons test don't pass for me the rest looks good, if everyone else wants to take over these two days i'm out, feel free :)

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Michael Zanetti (mzanetti) wrote :

I can reproduce the test failures Albert reported. Sometimes it seems that a swipe to remove removes 2 notifications instead of just one. I can also reproduce that manually with tryNotifications.

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

Seems fixing the FIXME in Notifications.qml:44 fixes this. Can't reproduce the issue any more neither using the test, nor manually.

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

hmm.. sorry, after reverting the debug prints in the test it fails again here...

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

Test passing fine now.

review: Abstain
Revision history for this message
Albert Astals Cid (aacid) wrote :

I think relying on the JS->C++ binding ignoring the extra parameter to call an invokable function with less parameters is a bit too much

Revision history for this message
Albert Astals Cid (aacid) wrote :

 * Did you perform an exploratory manual test run of the code change and any related functionality?
Yes

 * Did CI run pass?
Waiting for it to finish

 * Did you make sure that the branch does not contain spurious tags?
Yes

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Albert Astals Cid (aacid) wrote :

qmluitests failed on CI for some reason byt they all passed fine so i'm going to top approve anyway

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 :

There's a failing test I'm afraid:

FAIL! : qmltestrunner::NotificationRendererTest::test_NotificationRenderer(Ephemeral notification) property count
   Actual (): 3
   Expected (): 2
   Loc: [/home/michal/Pobrane/unity8-8.02+15.04.20150204.1/tests/qmltests/Notifications/tst_Notifications.qml(666)]

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

Looking into it.

1284. By Mirco Müller

Merge with trunk.

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

I've updated my system, re-merged this branch with lp:unity8 trunk and recompiled everything, but still can't reproduce this failure. All notification-related qml-tests pass.

1285. By Mirco Müller

Make sure all rendering operations are completed before doing the swipe-dismiss-checks, thus the tests also pass on very slow (jenkins) systems.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (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) :
review: Approve (tests)

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