Unity 8

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

unbreak-notification-positioning-fix-1422711
Merge into trunk

Proposed by Mirco Müller on 2015-02-17

Status:	Rejected
Rejected by:	Albert Astals Cid on 2016-03-15
Proposed branch:	lp:~macslow/unity8/unbreak-notification-positioning-fix-1422711
Merge into:	lp:unity8
Diff against target:	15 lines (+5/-0) 1 file modified qml/Notifications/Notification.qml (+5/-0)
To merge this branch:	bzr merge lp:~macslow/unity8/unbreak-notification-positioning-fix-1422711
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Albert Astals Cid (community)		2015-02-17	Disapprove on 2016-03-15
PS Jenkins bot	continuous-integration		Needs Fixing on 2015-02-24
Review via email: mp+249998@code.launchpad.net

Commit Message

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

Description of the Change

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 updated on 2015-02-17

1619. By Mirco Müller on 2015-02-17: Make reset of fullscreen-flag a bit more robust.

PS Jenkins bot (ps-jenkins) wrote on 2015-02-17:

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) wrote on 2015-02-17:

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) wrote on 2015-02-18:

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) wrote on 2015-02-18:

> * 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 updated on 2015-02-24

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) wrote on 2015-02-24:

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) wrote on 2015-02-25:

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) wrote on 2015-02-25:

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) wrote on 2015-02-25:

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) wrote on 2015-02-25:

> 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) wrote on 2015-02-25:

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

Albert Astals Cid (aacid) wrote on 2016-03-15:

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Mirco Müller

Mohammad Rakibul Hasan

Unity Team

1	=== modified file 'qml/Notifications/Notification.qml'
2	--- qml/Notifications/Notification.qml 2015-01-21 17:19:13 +0000
3	+++ qml/Notifications/Notification.qml 2015-02-24 17:03:55 +0000
4	@@ -396,6 +396,11 @@
5	onAccepted: {
6	notification.notification.invokeAction(actionRepeater.itemAt(0).actionId)
7	}
8	+ Component.onDestruction: {
9	+ if (notification.fullscreen) {
10	+ notification.fullscreen = false;
11	+ }
12	+ }
13	}
14	}
15	}