Merge lp:~macslow/unity8/unbreak-notification-positioning-fix-1422711 into lp:unity8

[Mirco Müller]

Reset the fullscreen-flag of the notification-listview, so notifications following any fullscreen notification don't get placed on top the panel.

Reset the fullscreen-flag of the notification-listview, so notifications following any fullscreen notification don't get placed on top the panel.

* Are there any related MPs required for this MP to build/function as expected?
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?
Not applicable

[lp:~macslow/unity8/unbreak-notification-positioning-fix-1422711]

1619. By Mirco Müller on 2015-02-17

Make reset of fullscreen-flag a bit more robust.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1618
http://jenkins.qa.ubuntu.com/job/unity8-ci/5331/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/1393
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/495
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/496
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/496
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/1230
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1391
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1391/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18115

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5331/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1619
http://jenkins.qa.ubuntu.com/job/unity8-ci/5336/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/1403
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/500
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/501
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/501
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/1240
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1401
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1401/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18134

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5336/rebuild

review: Needs Fixing (continuous-integration)

[Albert Astals Cid (aacid)]

I don't understand the change, some questions:

 * Is a given Notification reused? Is that why you're resetting the fullscreen of the notification on NotificationMenuItemFactory destruction?

 * With your new Binding, each and every Notification delegate is affecting the topmostIsFullscreen property of notificationList, shouldn't only the first one affect it?

review: Needs Information

[Mirco Müller (macslow)]

> * Is a given Notification reused? Is that why you're resetting the fullscreen
> of the notification on NotificationMenuItemFactory destruction?

No. I've intentionally placed the reset of notificationList.topmostIsFullscreen in NotificationMenuItemFactory's onDestruction, to limit changes to topmostIsFullscreen to notifications, which have a UnityMenuModel-hint set (only those can have the custom pinlock-UI, which implies a fullscreen notification-layout).

> * With your new Binding, each and every Notification delegate is affecting
> the topmostIsFullscreen property of notificationList, shouldn't only the first
> one affect it?

Sure. I'll revert that part.

[lp:~macslow/unity8/unbreak-notification-positioning-fix-1422711]

1620. By Mirco Müller on 2015-02-24

Only change the topmostIsFullscreen flag based on the first visible notification in the visual queue.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1620
http://jenkins.qa.ubuntu.com/job/unity8-ci/5358/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/1506
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-phablet-qmluitests-vivid/522
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-amd64-ci/523
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-vivid-i386-ci/523
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/1338
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1504
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/1504/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/18310

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5358/rebuild

review: Needs Fixing (continuous-integration)

[Albert Astals Cid (aacid)]

If Notification is not reused i don't understand why changing the fullscreen value of the notification on destruction is the proper fix, i mean that notification is going to go away just after setting the fullscreen to false, why should we change it's fullscreen property?

review: Needs Information

[Mirco Müller (macslow)]

Because the topmostIsFullscreen-flag is based on the notification's/delegate's fullscreen flag. If it is not changed (back to false, if a fullscreen notification closes) the topmostIsFullscreen-flag will reflect a wrong state.

[Albert Astals Cid (aacid)]

Well, but since the delegate will be deleted, the flag should be updated to reflect the value of the next topmost delegate fullscreen value now?

review: Needs Information

[Mirco Müller (macslow)]

> Well, but since the delegate will be deleted, the flag should be updated to
> reflect the value of the next topmost delegate fullscreen value now?

That is happening with...

Component.onCompleted: {
            if (index == 1) {
                notificationList.topmostIsFullscreen = fullscreen
            }
        }

... in Notifications.qml

[Mirco Müller (macslow)]

*sigh* the branch is broken again... setting it back to wip

[Albert Astals Cid (aacid)]

This branch makes no sense

review: Disapprove

Unmerged revisions

1620. By Mirco Müller on 2015-02-24

Only change the topmostIsFullscreen flag based on the first visible notification in the visual queue.

1619. By Mirco Müller on 2015-02-17

Make reset of fullscreen-flag a bit more robust.

1618. By Mirco Müller on 2015-02-17

Reset the fullscreen-flag of the notification-listview, so notifications following any fullscreen notification don't get placed on top the panel.

=== modified file 'qml/Notifications/Notification.qml'
--- qml/Notifications/Notification.qml	2015-01-21 17:19:13 +0000
+++ qml/Notifications/Notification.qml	2015-02-24 17:03:55 +0000
@@ -396,6 +396,11 @@
                         onAccepted: {
                             notification.notification.invokeAction(actionRepeater.itemAt(0).actionId)
                         }
+                        Component.onDestruction: {
+                            if (notification.fullscreen) {
+                                notification.fullscreen = false;
+                            }
+                        }
                     }
                 }
             }