Merge lp:~mzanetti/unity8/fix-lockscreen-wallpaper into lp:unity8

Proposed by Michael Zanetti
Status: Merged
Approved by: Michael Terry
Approved revision: 1428
Merged at revision: 1438
Proposed branch: lp:~mzanetti/unity8/fix-lockscreen-wallpaper
Merge into: lp:unity8
Diff against target: 177 lines (+47/-30)
5 files modified
qml/Components/Lockscreen.qml (+15/-0)
qml/Greeter/Greeter.qml (+1/-1)
qml/Greeter/GreeterContent.qml (+1/-18)
qml/Notifications/NotificationMenuItemFactory.qml (+1/-0)
qml/Shell.qml (+29/-11)
To merge this branch: bzr merge lp:~mzanetti/unity8/fix-lockscreen-wallpaper
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Needs Fixing
Michael Terry Approve
Olga Kemmet (community) design Approve
Review via email: mp+241067@code.launchpad.net

Commit message

unify Greeter and Lockscreen wallpaper again

This moves the actual reading of accountsservice into Shell.qml so we only have to read it once. Also this drops the GSettings for wallpaper as it hasn't been used any more (we can't change that in systemsettings any more anyways)

Description of the change

* 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?

n/a

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

Yes. did a hangout with kemmko.

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
Michael Terry (mterry) wrote :

- I might like a comment above the 0.4 alpha rectangle in Lockscreen to the effect of "This is only here to match the alpha of the main wallpaper for the greeter" -- makes a person wonder why its there otherwise

- We don't want to drop support for gsettings. That fallback is used so that image customizers can set a different default wallpaper. (They can easily change gsettings defaults, but not AS ones)

- I don't see design sign-off for this. I asked Olga about this in person in London and she preferred them to be separate backgrounds.

review: Needs Fixing
Revision history for this message
Michael Terry (mterry) wrote :

Adding nice fallback code for gsettings might be easier if we first merge the cleanup done in lp:~mterry/unity8/cache-greeter-bg

Revision history for this message
Olga Kemmet (olga-kemmet) wrote :

As seen and agreed, thanks.

review: Approve (design)
1423. By Michael Zanetti on 2014-11-11

fix review comments

Revision history for this message
Olga Kemmet (olga-kemmet) wrote :

Just as a side note: we should allow users to set at least the backgrounds for now. Further down the line, we should consider allowing to set separate backgrounds for lock and greeter screens.

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

> - I might like a comment above the 0.4 alpha rectangle in Lockscreen to the
> effect of "This is only here to match the alpha of the main wallpaper for the
> greeter" -- makes a person wonder why its there otherwise

Done

>
> - We don't want to drop support for gsettings. That fallback is used so that
> image customizers can set a different default wallpaper. (They can easily
> change gsettings defaults, but not AS ones)

Reverted back

> - I don't see design sign-off for this. I asked Olga about this in person in
> London and she preferred them to be separate backgrounds.

There has been, I asked Olga to leave a comment somewhere to make it more obvious

1424. By Michael Zanetti on 2014-11-11

make the black overlay cofigurable to not break the wizard

1425. By Michael Zanetti on 2014-11-11

merge trunk

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
1426. By Michael Zanetti on 2014-11-11

only set the gsettings image if there is no AS setting

1427. By Michael Zanetti on 2014-11-11

fix if

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Michael Terry (mterry) wrote :

I tried deb from r1425 and copying over qml changes in 1426/1427. Still just shows me default bg. But reverting to vivid debs works.

review: Needs Fixing
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
1428. By Michael Zanetti on 2014-11-12

fix it for real

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

> I tried deb from r1425 and copying over qml changes in 1426/1427. Still just
> shows me default bg. But reverting to vivid debs works.

This was a bit racy. If GSettings loaded before AS, it broke the binding. I've removed imperative code now and keep it all to nice on-the-fly updating bindings. At the cost of adding a second dummy image though, but given sourceSize is 0x0 it shouldn't hurt us in terms of memory consumption.

Revision history for this message
Michael Terry (mterry) wrote :

Like it! Yay for declarative backgrounds! Works for me now too.

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

 * Did CI run pass? If not, please explain why.
 Yes it did! omg

 * Did you make sure that the branch does not contain spurious tags?
 Yes (can we stop doing this now? Seems like we got rid of all of them)

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)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'qml/Components/Lockscreen.qml'
2--- qml/Components/Lockscreen.qml 2014-10-30 21:42:44 +0000
3+++ qml/Components/Lockscreen.qml 2014-11-12 11:52:42 +0000
4@@ -56,6 +56,9 @@
5 property int maxPinLength: -1
6
7 property url background: ""
8+ // Use this to put a black overlay above the background
9+ // 0: normal background, 1: black background
10+ property real darkenBackground: 0
11
12 readonly property string passphrase: (pinPadLoader.item && pinPadLoader.item.passphrase) ? pinPadLoader.item.passphrase : ""
13
14@@ -100,10 +103,22 @@
15 anchors {
16 fill: parent
17 }
18+ // Limit how much memory we'll reserve for this image
19+ sourceSize.height: height
20+ sourceSize.width: width
21 source: root.required ? root.background : ""
22 fillMode: Image.PreserveAspectCrop
23 }
24
25+ // This is to
26+ // a) align it with the greeter and
27+ // b) keep the white fonts readable on bright backgrounds
28+ Rectangle {
29+ anchors.fill: parent
30+ color: "black"
31+ opacity: root.darkenBackground
32+ }
33+
34 MouseArea {
35 anchors.fill: root
36 onClicked: {
37
38=== modified file 'qml/Greeter/Greeter.qml'
39--- qml/Greeter/Greeter.qml 2014-11-04 11:19:52 +0000
40+++ qml/Greeter/Greeter.qml 2014-11-12 11:52:42 +0000
41@@ -25,7 +25,7 @@
42 enabled: shown
43 created: greeterContentLoader.status == Loader.Ready && greeterContentLoader.item.ready
44
45- property url defaultBackground
46+ property url background
47 property bool loadContent: required
48
49 // 1 when fully shown and 0 when fully hidden
50
51=== modified file 'qml/Greeter/GreeterContent.qml'
52--- qml/Greeter/GreeterContent.qml 2014-10-01 13:22:00 +0000
53+++ qml/Greeter/GreeterContent.qml 2014-11-12 11:52:42 +0000
54@@ -48,9 +48,6 @@
55 color: "black"
56 }
57
58- property url backgroundValue: AccountsService.backgroundFile != undefined && AccountsService.backgroundFile.length > 0 ? AccountsService.backgroundFile : greeter.defaultBackground
59- onBackgroundValueChanged: background.source = backgroundValue
60-
61 CrossFadeImage {
62 id: background
63 objectName: "greeterBackground"
64@@ -62,21 +59,7 @@
65 // Limit how much memory we'll reserve for this image
66 sourceSize.height: height
67 sourceSize.width: width
68- }
69-
70- // See Shell.qml's backgroundSettings treatment for why we need a separate
71- // Image, but it boils down to avoiding binding loop detection.
72- Image {
73- source: background.source
74- height: 0
75- width: 0
76- sourceSize.height: 0
77- sourceSize.width: 0
78- onStatusChanged: {
79- if (status == Image.Error && source != greeter.defaultBackground) {
80- background.source = greeter.defaultBackground
81- }
82- }
83+ source: greeter.background
84 }
85
86 Rectangle {
87
88=== modified file 'qml/Notifications/NotificationMenuItemFactory.qml'
89--- qml/Notifications/NotificationMenuItemFactory.qml 2014-11-05 15:23:43 +0000
90+++ qml/Notifications/NotificationMenuItemFactory.qml 2014-11-12 11:52:42 +0000
91@@ -145,6 +145,7 @@
92 errorText: errorAction.valid ? errorAction.state : ""
93 retryText: notification.body
94 background: shell.background
95+ darkenBackground: 0.4
96
97 onEntered: {
98 menuModel.changeState(menuIndex, passphrase);
99
100=== modified file 'qml/Shell.qml'
101--- qml/Shell.qml 2014-11-07 14:30:42 +0000
102+++ qml/Shell.qml 2014-11-12 11:52:42 +0000
103@@ -51,7 +51,8 @@
104
105 property real edgeSize: units.gu(2)
106 property url defaultBackground: Qt.resolvedUrl(shell.width >= units.gu(60) ? "graphics/tablet_background.jpg" : "graphics/phone_background.jpg")
107- property url background
108+ property url background: asImageTester.status == Image.Ready ? asImageTester.source
109+ : gsImageTester.status == Image.Ready ? gsImageTester.source : defaultBackground
110 readonly property real panelHeight: panel.panelHeight
111
112 readonly property bool locked: LightDM.Greeter.active && !LightDM.Greeter.authenticated && !forcedUnlock
113@@ -98,6 +99,31 @@
114 shell.activateApplication(app);
115 }
116
117+ // This is a dummy image to detect if the custom AS set wallpaper loads successfully.
118+ Image {
119+ id: asImageTester
120+ source: AccountsService.backgroundFile != undefined && AccountsService.backgroundFile.length > 0 ? AccountsService.backgroundFile : ""
121+ height: 0
122+ width: 0
123+ sourceSize.height: 0
124+ sourceSize.width: 0
125+ }
126+
127+ GSettings {
128+ id: backgroundSettings
129+ schema.id: "org.gnome.desktop.background"
130+ }
131+
132+ // This is a dummy image to detect if the custom GSettings set wallpaper loads successfully.
133+ Image {
134+ id: gsImageTester
135+ source: backgroundSettings.pictureUri != undefined && backgroundSettings.pictureUri.length > 0 ? backgroundSettings.pictureUri : ""
136+ height: 0
137+ width: 0
138+ sourceSize.height: 0
139+ sourceSize.width: 0
140+ }
141+
142 Binding {
143 target: LauncherModel
144 property: "applicationManager"
145@@ -114,15 +140,6 @@
146 }
147 }
148
149- GSettings {
150- id: backgroundSettings
151- schema.id: "org.gnome.desktop.background"
152- }
153- property url gSettingsPicture: backgroundSettings.pictureUri != undefined && backgroundSettings.pictureUri.length > 0 ? backgroundSettings.pictureUri : shell.defaultBackground
154- onGSettingsPictureChanged: {
155- shell.background = gSettingsPicture
156- }
157-
158 VolumeControl {
159 id: volumeControl
160 }
161@@ -328,6 +345,7 @@
162 width: parent.width
163 height: parent.height - panel.panelHeight
164 background: shell.background
165+ darkenBackground: 0.4
166 alphaNumeric: AccountsService.passwordDisplayHint === AccountsService.Keyboard
167 minPinLength: 4
168 maxPinLength: 4
169@@ -523,7 +541,7 @@
170
171 locked: shell.locked
172
173- defaultBackground: shell.background
174+ background: shell.background
175
176 width: parent.width
177 height: parent.height

Subscribers

People subscribed via source and target branches