Unity 8

Merge lp:~lukas-kde/unity8/fixNotificationsColorWithNewUitk into lp:unity8

fixNotificationsColorWithNewUitk
Merge into trunk

Proposed by Lukáš Tinkl on 2016-03-13

Status:

Merged

Approved by:

Michael Terry on 2016-03-16

Approved revision:

2279

Merged at revision:

2281

Proposed branch:

lp:~lukas-kde/unity8/fixNotificationsColorWithNewUitk

Merge into:

lp:unity8

Diff against target:

195 lines (+27/-25)

5 files modified

qml/Notifications/Notification.qml (+10/-16)
qml/Notifications/Notifications.qml (+2/-0)
qml/Notifications/OptionToggle.qml (+1/-1)
qml/Shell.qml (+1/-0)
tests/qmltests/Notifications/tst_Notifications.qml (+13/-8)

To merge this branch:

bzr merge lp:~lukas-kde/unity8/fixNotificationsColorWithNewUitk

Related bugs:

Bug #1554616: Shell and dash visual issues with new UITK and palette	Critical	Fix Released
Bug #1556523: Notifications are the wrong color in rc-proposed	Undecided	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Michael Terry		2016-03-13	Approve on 2016-03-16
Unity8 CI Bot	continuous-integration		Needs Fixing on 2016-03-15
Review via email: mp+288869@code.launchpad.net

Commit Message

Fix notifications with the new UITK color schemes

Description of the Change

Fix notifications with the new UITK color schemes

Try to follow the new SDK palette as closely as possible

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

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

Yes

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

No (altho this patch tries to follow the SDK Palette as closely as possible)

Unity8 CI Bot (unity8-ci-bot) wrote on 2016-03-13:

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/697/rebuild

review: Approve (continuous-integration)

Daniel d'Andrada (dandrader) wrote on 2016-03-14:

"""
- readonly property color red: "#fc4949"
- readonly property color green: "#3fb24f"
+ readonly property color red: UbuntuColors.red
+ readonly property color green: UbuntuColors.green
"""

Looks like those red and green properties are now redundant. You could refer directly to UbuntuColors.red and UbuntuColors.green and the code will be clear just the same.

Michael Terry (mterry) wrote on 2016-03-14:

Agreed with Daniel.

And what's with the continued use of sdLightGrey and the manually-specified rgba value? Can we replace that with a theme equivalent?

Otherwise, works fine in my brief testing on the phone, and playing with the tryNotifications window (which, if we had more time, I would ask you to expand to include more scenarios, like a light/dark mode toggle).

review: Needs Fixing

lp:~lukas-kde/unity8/fixNotificationsColorWithNewUitk updated on 2016-03-14

2278. By Lukáš Tinkl on 2016-03-14: remove useless aliases for builtin colors

Lukáš Tinkl (lukas-kde) wrote on 2016-03-14:

> Agreed with Daniel.
>
> And what's with the continued use of sdLightGrey and the manually-specified
> rgba value? Can we replace that with a theme equivalent?

Yeah, definitely something for the future (most probably together with designers at the upcoming sprint)...

> Otherwise, works fine in my brief testing on the phone, and playing with the
> tryNotifications window (which, if we had more time, I would ask you to expand
> to include more scenarios, like a light/dark mode toggle).

.... and this too

Lukáš Tinkl (lukas-kde) wrote on 2016-03-14:

That said, I removed the hardcoded aliases

Unity8 CI Bot (unity8-ci-bot) wrote on 2016-03-14:

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/734/rebuild

review: Needs Fixing (continuous-integration)

lp:~lukas-kde/unity8/fixNotificationsColorWithNewUitk updated on 2016-03-15

2279. By Lukáš Tinkl on 2016-03-15

fix the broken binding (reaching out of context)

for inverseMode and make it propertly exported, so that it can
be tested

Unity8 CI Bot (unity8-ci-bot) wrote on 2016-03-15:

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/751/rebuild

review: Needs Fixing (continuous-integration)

Michael Terry (mterry) wrote on 2016-03-16:

* Did you perform an exploratory manual test run of the code change and any related functionality?
Yes, everything I tested seemed fine.

* Did CI run pass? If not, please explain why.
No, some weird autopilot env failure

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

It would be nice if we could do something more dynamic for sdLightGrey, but Lukas says there isn't a good replacement in the theme yet.

review: Approve

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:

Lukáš Tinkl

Mohammad Rakibul Hasan

Unity Team

1	=== modified file 'qml/Notifications/Notification.qml'
2	--- qml/Notifications/Notification.qml 2016-03-10 22:41:22 +0000
3	+++ qml/Notifications/Notification.qml 2016-03-15 20:18:12 +0000
4	@@ -41,15 +41,12 @@
5	property int maxHeight
6	property int margins
7	readonly property bool draggable: (type === Notification.SnapDecision && state === "contracted") \|\| type === Notification.Interactive \|\| type === Notification.Ephemeral
8	- readonly property bool darkOnBright: panel.indicators.shown \|\| type === Notification.SnapDecision
9	- readonly property color red: "#fc4949"
10	- readonly property color green: "#3fb24f"
11	+ readonly property bool darkOnBright: inverseMode \|\| type === Notification.SnapDecision
12	readonly property color sdLightGrey: "#eaeaea"
13	- readonly property color sdDarkGrey: "#dddddd"
14	- readonly property color sdFontColor: "#5d5d5d"
15	readonly property real contentSpacing: units.gu(2)
16	readonly property bool canBeClosed: type === Notification.Ephemeral
17	property bool hasMouse
18	+ property bool inverseMode
19	property url background: ""
20
21	objectName: "background"
22	@@ -59,7 +56,7 @@
23	opacity: 1 - (x / notification.width) // FIXME: non-zero initially because of LP: #1354406 workaround, we want this to start at 0 upon creation eventually
24
25	theme: ThemeSettings {
26	- name: "Ubuntu.Components.Themes.Ambiance"
27	+ name: darkOnBright ? "Ubuntu.Components.Themes.Ambiance" : "Ubuntu.Components.Themes.SuruDark"
28	}
29
30	state: {
31	@@ -285,7 +282,6 @@
32	}
33	visible: type !== Notification.Confirmation
34	fontSize: "medium"
35	- color: darkOnBright ? sdFontColor : theme.palette.normal.backgroundText
36	elide: Text.ElideRight
37	textFormat: Text.PlainText
38	}
39	@@ -300,7 +296,6 @@
40	}
41	visible: body != "" && type !== Notification.Confirmation
42	fontSize: "small"
43	- color: darkOnBright ? sdFontColor : theme.palette.normal.backgroundText
44	wrapMode: Text.Wrap
45	maximumLineCount: type == Notification.SnapDecision ? 12 : 2
46	elide: Text.ElideRight
47	@@ -341,7 +336,6 @@
48	anchors.horizontalCenter: parent.horizontalCenter
49	visible: type === Notification.Confirmation && body !== ""
50	fontSize: "medium"
51	- color: darkOnBright ? sdFontColor : theme.palette.normal.backgroundText
52	wrapMode: Text.WordWrap
53	maximumLineCount: 1
54	elide: Text.ElideRight
55	@@ -361,7 +355,7 @@
56	}
57
58	height: units.gu(1)
59	- backgroundColor: darkOnBright ? UbuntuColors.darkGrey : UbuntuColors.lightGrey
60	+ backgroundColor: theme.palette.normal.background
61	aspect: UbuntuShape.Flat
62	radius: "small"
63
64	@@ -370,7 +364,7 @@
65	objectName: "innerBar"
66	width: valueIndicator.width * valueIndicator.value / 100
67	height: units.gu(1)
68	- backgroundColor: notification.hints["x-canonical-value-bar-tint"] === "true" ? UbuntuColors.orange : darkOnBright ? UbuntuColors.lightGrey : "white"
69	+ backgroundColor: notification.hints["x-canonical-value-bar-tint"] === "true" ? theme.palette.normal.activity : theme.palette.highlighted.foreground
70	aspect: UbuntuShape.Flat
71	radius: "small"
72	}
73	@@ -447,7 +441,7 @@
74	objectName: "notify_oot_button" + index
75	width: oneOverTwoCase.width
76	text: oneOverTwoLoaderTop.actionLabel
77	- color: notification.hints["x-canonical-private-affirmative-tint"] == "true" ? green : sdDarkGrey
78	+ color: notification.hints["x-canonical-private-affirmative-tint"] == "true" ? UbuntuColors.green : theme.palette.normal.baseText
79	onClicked: notification.notification.invokeAction(oneOverTwoLoaderTop.actionId)
80	}
81	}
82	@@ -475,7 +469,7 @@
83	objectName: "notify_oot_button" + index
84	width: oneOverTwoCase.width / 2 - spacing * 2
85	text: oneOverTwoLoaderBottom.actionLabel
86	- color: index == 1 && notification.hints["x-canonical-private-rejection-tint"] == "true" ? red : sdDarkGrey
87	+ color: index == 1 && notification.hints["x-canonical-private-rejection-tint"] == "true" ? UbuntuColors.red : theme.palette.normal.baseText
88	onClicked: notification.notification.invokeAction(oneOverTwoLoaderBottom.actionId)
89	}
90	}
91	@@ -536,12 +530,12 @@
92	width: buttonRow.width / 2 - spacing * 2
93	text: loader.actionLabel
94	color: {
95	- var result = sdDarkGrey;
96	+ var result = theme.palette.normal.baseText;
97	if (index == 0 && notification.hints["x-canonical-private-affirmative-tint"] == "true") {
98	- result = green;
99	+ result = UbuntuColors.green;
100	}
101	if (index == 1 && notification.hints["x-canonical-private-rejection-tint"] == "true") {
102	- result = red;
103	+ result = UbuntuColors.red;
104	}
105	return result;
106	}
107
108	=== modified file 'qml/Notifications/Notifications.qml'
109	--- qml/Notifications/Notifications.qml 2015-10-26 09:59:50 +0000
110	+++ qml/Notifications/Notifications.qml 2016-03-15 20:18:12 +0000
111	@@ -29,6 +29,7 @@
112	property real margin
113	property bool useModal: snapDecisionProxyModel.count > 0
114	property bool hasMouse
115	+ property bool inverseMode
116	property url background: ""
117
118	UnitySortFilterProxyModel {
119	@@ -61,6 +62,7 @@
120	maxHeight: notificationList.height
121	margins: notificationList.margin
122	hasMouse: notificationList.hasMouse
123	+ inverseMode: notificationList.inverseMode
124	background: notificationList.background
125
126	// make sure there's no opacity-difference between the several
127
128	=== modified file 'qml/Notifications/OptionToggle.qml'
129	--- qml/Notifications/OptionToggle.qml 2015-11-04 14:57:13 +0000
130	+++ qml/Notifications/OptionToggle.qml 2016-03-15 20:18:12 +0000
131	@@ -96,7 +96,7 @@
132
133	width: parent.width
134	text: splitLabel[3]
135	- color:"#5d5d5d"
136	+ color: theme.palette.normal.backgroundText
137	fontSize: "small"
138	maximumLineCount: 1
139	elide: Text.ElideRight
140
141	=== modified file 'qml/Shell.qml'
142	--- qml/Shell.qml 2016-03-10 22:37:44 +0000
143	+++ qml/Shell.qml 2016-03-15 20:18:12 +0000
144	@@ -663,6 +663,7 @@
145	model: NotificationBackend.Model
146	margin: units.gu(1)
147	hasMouse: shell.hasMouse
148	+ inverseMode: panel.indicators.shown
149	background: wallpaperResolver.background
150
151	y: topmostIsFullscreen ? 0 : panel.panelHeight
152
153	=== modified file 'tests/qmltests/Notifications/tst_Notifications.qml'
154	--- tests/qmltests/Notifications/tst_Notifications.qml 2015-11-23 15:41:34 +0000
155	+++ tests/qmltests/Notifications/tst_Notifications.qml 2016-03-15 20:18:12 +0000
156	@@ -187,7 +187,8 @@
157
158	anchors.fill: parent
159	model: mockModel
160	- hasMouse: false
161	+ hasMouse: fakeMouseCB.checked
162	+ inverseMode: inverseModeCB.checked
163	}
164	}
165
166	@@ -261,19 +262,23 @@
167	Layout.fillWidth: true
168	CheckBox {
169	id: fakeMouseCB
170	- onClicked: {
171	- if (checked) {
172	- notifications.hasMouse = true;
173	- } else {
174	- notifications.hasMouse = false;
175	- }
176	- }
177	}
178	Label {
179	text: "With fake mouse"
180	anchors.verticalCenter: parent.verticalCenter
181	}
182	}
183	+
184	+ RowLayout {
185	+ Layout.fillWidth: true
186	+ CheckBox {
187	+ id: inverseModeCB
188	+ }
189	+ Label {
190	+ text: "Inverse mode"
191	+ anchors.verticalCenter: parent.verticalCenter
192	+ }
193	+ }
194	}
195	}
196