Merge lp:~aacid/ubuntu-ui-toolkit/nonsquareicons into lp:ubuntu-ui-toolkit/staging
- nonsquareicons
- Merge into staging
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Zsombor Egri | ||||
Approved revision: | 1418 | ||||
Merged at revision: | 1414 | ||||
Proposed branch: | lp:~aacid/ubuntu-ui-toolkit/nonsquareicons | ||||
Merge into: | lp:ubuntu-ui-toolkit/staging | ||||
Diff against target: |
271 lines (+142/-15) 7 files modified
modules/Ubuntu/Components/plugin/unitythemeiconprovider.cpp (+17/-14) modules/Ubuntu/Components/plugin/unitythemeiconprovider.h (+1/-1) tests/unit_x11/tst_iconprovider/icons/mockTheme/actions/scalable/battery-100-charging.svg (+25/-0) tests/unit_x11/tst_iconprovider/icons/mockTheme/index.theme (+17/-0) tests/unit_x11/tst_iconprovider/tst_iconprovider.cpp (+76/-0) tests/unit_x11/tst_iconprovider/tst_iconprovider.pro (+5/-0) tests/unit_x11/unit_x11.pro (+1/-0) |
||||
To merge this branch: | bzr merge lp:~aacid/ubuntu-ui-toolkit/nonsquareicons | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
PS Jenkins bot | continuous-integration | Approve | |
Lars Karlitski (community) | Approve | ||
Zsombor Egri | Approve | ||
Sebastien Bacher (community) | runtime testing | Approve | |
Review via email: mp+250110@code.launchpad.net |
Commit message
Fix infinite icon size grow when asked for a non square icon
Without this patch we could end up in loops like
requested icon "battery-
returned QSize(37, 24)
requested icon "battery-
returned QSize(57, 37)
requested icon "battery-
returned QSize(88, 57)
requested icon "battery-
returned QSize(88, 57)
Description of the change
Albert Astals Cid (aacid) wrote : | # |
- 1411. By Albert Astals Cid
-
Add tests for the icon provider
They are bit dependent on the suru icon theme so may need a bit of adjusting as time goes by
Albert Astals Cid (aacid) wrote : | # |
The diff seems very big but that's just because i moved the declaration of the icon theme class to the header in r1411 :/
- 1412. By Albert Astals Cid
-
Update
- 1413. By Albert Astals Cid
-
Get my own mockTheme for more stable testin
- 1414. By Albert Astals Cid
-
Revert unneeded changes
- 1415. By Albert Astals Cid
-
We don't need this either
- 1416. By Albert Astals Cid
-
smaller diff against original code
- 1417. By Albert Astals Cid
-
my mock inherits noone
Albert Astals Cid (aacid) wrote : | # |
Without the changes in the cpp code these are the tests that fail
tst_iconprovider: FAIL! : tst_IconProvide
tst_
tst_
tst_
i.e. asking for a 16x-1 icon returns a 24x16 icon
tst_iconprovider: FAIL! : tst_IconProvide
tst_
tst_
tst_
i.e. asking for a 16x0 icon returns a 24x16 icon
tst_iconprovider: FAIL! : tst_IconProvide
tst_
tst_
tst_
i.e. asking for a 24x16 icon returns a 37x24 icon
tst_iconprovider: FAIL! : tst_IconProvide
tst_
tst_
tst_
i.e. asking for a 37x24 icon returns a 57x27 icon
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1410
http://
Executed test runs:
UNSTABLE: http://
FAILURE: http://
SUCCESS: http://
deb: http://
FAILURE: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Sebastien Bacher (seb128) wrote : | # |
The changes work for me, my unity8 desktop session starts again with that update
Zsombor Egri (zsombi) wrote : | # |
Looks good, lets wait till we get the staging fixed with 5.4 (i.e. binding loops in Picker)
Lars Karlitski (larsu) wrote : | # |
I can't reproduce the infinite loop. That isn't an issue in with the provider, though, as it sets its outgoing size correctly.
However, I agree that returning e.g. a pixmap sized (57, 37) when requesting (37, 24) is obviously bogus. Thanks for the patch!
Note that you still return values greater than requested in some cases. Not sure if that lead to the orginal problem.
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1417
http://
Executed test runs:
UNSTABLE: http://
FAILURE: http://
SUCCESS: http://
deb: http://
FAILURE: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Albert Astals Cid (aacid) wrote : | # |
Agreed, the Qt::KeepAspectR
Zsombor, do you want me to explore fixing that too?
- 1418. By Albert Astals Cid
-
Merge staging
Zsombor Egri (zsombi) wrote : | # |
> Agreed, the Qt::KeepAspectR
> QSize(37, 24) when asking one of QSize(24, 24) and i'm with you that's a
> questionable choice.
>
> Zsombor, do you want me to explore fixing that too?
If you have time to check it, I wouldn't mind, more, I would be very thankful!
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:1418
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) : | # |
Preview Diff
1 | === modified file 'modules/Ubuntu/Components/plugin/unitythemeiconprovider.cpp' |
2 | --- modules/Ubuntu/Components/plugin/unitythemeiconprovider.cpp 2014-09-02 09:39:09 +0000 |
3 | +++ modules/Ubuntu/Components/plugin/unitythemeiconprovider.cpp 2015-02-19 08:47:10 +0000 |
4 | @@ -48,7 +48,7 @@ |
5 | // Does a breadth-first search for an icon with any name in @names. Parent |
6 | // themes are only looked at if the current theme doesn't contain any icon |
7 | // in @names. |
8 | - QPixmap findBestIcon(const QStringList &names, int size) |
9 | + QPixmap findBestIcon(const QStringList &names, const QSize &size) |
10 | { |
11 | Q_FOREACH(const QString &name, names) { |
12 | QPixmap pixmap = lookupIcon(name, size); |
13 | @@ -121,18 +121,20 @@ |
14 | return Fixed; |
15 | } |
16 | |
17 | - static QPixmap loadIcon(const QString &filename, int size) |
18 | + static QPixmap loadIcon(const QString &filename, const QSize &size) |
19 | { |
20 | QPixmap pixmap; |
21 | |
22 | if (filename.endsWith(".png")) { |
23 | pixmap = QPixmap(filename); |
24 | - if (!pixmap.isNull() && size > 0 && (pixmap.width() != size || pixmap.height() != size)) |
25 | - pixmap = pixmap.scaled(size, size, Qt::KeepAspectRatioByExpanding); |
26 | + if (!pixmap.isNull() && !size.isNull() && (pixmap.width() != size.width() || pixmap.height() != size.height())) { |
27 | + const QSize newSize = pixmap.size().scaled(size.width(), size.height(), Qt::KeepAspectRatioByExpanding); |
28 | + pixmap = pixmap.scaled(newSize); |
29 | + } |
30 | } |
31 | else if (filename.endsWith(".svg")) { |
32 | QSvgRenderer renderer(filename); |
33 | - pixmap = QPixmap(renderer.defaultSize().scaled(size, size, Qt::KeepAspectRatioByExpanding)); |
34 | + pixmap = QPixmap(renderer.defaultSize().scaled(size.width(), size.height(), Qt::KeepAspectRatioByExpanding)); |
35 | pixmap.fill(Qt::transparent); |
36 | QPainter painter(&pixmap); |
37 | renderer.render(&painter); |
38 | @@ -160,15 +162,16 @@ |
39 | return QString(); |
40 | } |
41 | |
42 | - QPixmap lookupIcon(const QString &iconName, int size) |
43 | + QPixmap lookupIcon(const QString &iconName, const QSize &size) |
44 | { |
45 | - if (size > 0) |
46 | + const int iconSize = qMax(size.width(), size.height()); |
47 | + if (iconSize > 0) |
48 | return lookupBestMatchingIcon(iconName, size); |
49 | else |
50 | return lookupLargestIcon(iconName); |
51 | } |
52 | |
53 | - QPixmap lookupBestMatchingIcon(const QString &iconName, int size) |
54 | + QPixmap lookupBestMatchingIcon(const QString &iconName, const QSize &size) |
55 | { |
56 | int minDistance = 10000; |
57 | QString bestFilename; |
58 | @@ -213,13 +216,14 @@ |
59 | } |
60 | |
61 | if (!bestFilename.isNull()) |
62 | - return loadIcon(bestFilename, maxSize); |
63 | + return loadIcon(bestFilename, QSize(maxSize, maxSize)); |
64 | |
65 | return QPixmap(); |
66 | } |
67 | |
68 | - int directorySizeDistance(const Directory &dir, int size) |
69 | + int directorySizeDistance(const Directory &dir, const QSize &iconSize) |
70 | { |
71 | + const int size = qMax(iconSize.width(), iconSize.height()); |
72 | switch (dir.sizeType) { |
73 | case Fixed: |
74 | return qAbs(size - dir.size); |
75 | @@ -241,16 +245,15 @@ |
76 | QList<IconThemePointer> parents; |
77 | }; |
78 | |
79 | -UnityThemeIconProvider::UnityThemeIconProvider(): |
80 | +UnityThemeIconProvider::UnityThemeIconProvider(const QString &themeName): |
81 | QQuickImageProvider(QQuickImageProvider::Pixmap) |
82 | { |
83 | - theme = IconTheme::get("suru"); |
84 | + theme = IconTheme::get(themeName); |
85 | } |
86 | |
87 | QPixmap UnityThemeIconProvider::requestPixmap(const QString &id, QSize *size, const QSize &requestedSize) |
88 | { |
89 | - int iconSize = qMax(requestedSize.width(), requestedSize.height()); |
90 | - QPixmap pixmap = theme->findBestIcon(id.split(",", QString::SkipEmptyParts), iconSize); |
91 | + QPixmap pixmap = theme->findBestIcon(id.split(",", QString::SkipEmptyParts), requestedSize); |
92 | *size = pixmap.size(); |
93 | return pixmap; |
94 | } |
95 | |
96 | === modified file 'modules/Ubuntu/Components/plugin/unitythemeiconprovider.h' |
97 | --- modules/Ubuntu/Components/plugin/unitythemeiconprovider.h 2014-09-02 09:39:09 +0000 |
98 | +++ modules/Ubuntu/Components/plugin/unitythemeiconprovider.h 2015-02-19 08:47:10 +0000 |
99 | @@ -24,7 +24,7 @@ |
100 | class UnityThemeIconProvider: public QQuickImageProvider |
101 | { |
102 | public: |
103 | - UnityThemeIconProvider(); |
104 | + UnityThemeIconProvider(const QString &themeName = "suru"); |
105 | QPixmap requestPixmap(const QString &id, QSize *size, const QSize &requestedSize); |
106 | |
107 | private: |
108 | |
109 | === added directory 'tests/unit_x11/tst_iconprovider' |
110 | === added directory 'tests/unit_x11/tst_iconprovider/icons' |
111 | === added directory 'tests/unit_x11/tst_iconprovider/icons/mockTheme' |
112 | === added directory 'tests/unit_x11/tst_iconprovider/icons/mockTheme/actions' |
113 | === added directory 'tests/unit_x11/tst_iconprovider/icons/mockTheme/actions/scalable' |
114 | === added file 'tests/unit_x11/tst_iconprovider/icons/mockTheme/actions/scalable/battery-100-charging.svg' |
115 | --- tests/unit_x11/tst_iconprovider/icons/mockTheme/actions/scalable/battery-100-charging.svg 1970-01-01 00:00:00 +0000 |
116 | +++ tests/unit_x11/tst_iconprovider/icons/mockTheme/actions/scalable/battery-100-charging.svg 2015-02-19 08:47:10 +0000 |
117 | @@ -0,0 +1,25 @@ |
118 | +<?xml version="1.0" encoding="UTF-8" standalone="no"?> |
119 | +<!-- Created with Inkscape (http://www.inkscape.org/) --> |
120 | +<svg id="svg4966" xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#" xmlns="http://www.w3.org/2000/svg" height="90" viewBox="0 0 139.00489 90.000001" width="139" version="1.1" xmlns:cc="http://creativecommons.org/ns#" xmlns:dc="http://purl.org/dc/elements/1.1/"> |
121 | + <metadata id="metadata4971"> |
122 | + <rdf:RDF> |
123 | + <cc:Work rdf:about=""> |
124 | + <dc:format>image/svg+xml</dc:format> |
125 | + <dc:type rdf:resource="http://purl.org/dc/dcmitype/StillImage"/> |
126 | + <dc:title/> |
127 | + </cc:Work> |
128 | + </rdf:RDF> |
129 | + </metadata> |
130 | + <g id="layer1" transform="translate(-149.78 -484.51)"> |
131 | + <g id="g5120" transform="translate(-433.79 125.71)"> |
132 | + <g id="g5124" transform="translate(409.57 223.43)"> |
133 | + <g id="g5126" style="fill:none" transform="matrix(2.1875 0 0 1.875 -456 -1657.8)"> |
134 | + <rect id="rect5128" style="opacity:.21171;fill:none" transform="translate(0 804.36)" height="48" width="48" y="152" x="288"/> |
135 | + </g> |
136 | + <path id="path5130" style="fill:#808080" transform="translate(174 135.36)" d="m86 15-75.156 0.012c-7.844-0.012-10.844 1.988-10.844 11.988v18 18c0 10 3 12 10.844 11.988l75.156 0.012c7.8438 0.01172 11-2.3633 11-12v-4.0117h8v-13.988-13.988h-8v-4.012c0-9.637-3.156-12.012-11-12zm-75 6c21.619 0.0096 48.956 0.0081 75 0 4 0 5 1 5 6v18 18c0 5-1 6-5 6-26.044-0.008-53.381-0.01-75 0-4 0-5-1-5-6v-18-18c0-5 1-6 5-6z"/> |
137 | + <path id="path5132" style="fill:#38b44a" d="m1051 697c-3 0-4 0-4 5v26c0 5 1 5 4 5h65c3 0 4 0 4-5v-26c0-5-1-5-4-5z" transform="translate(-861 -534.64)"/> |
138 | + <path id="path3847-0" style="fill:#808080" d="m299 150.36-13.996 35h14v25l14.005-35h-14z"/> |
139 | + </g> |
140 | + </g> |
141 | + </g> |
142 | +</svg> |
143 | |
144 | === added directory 'tests/unit_x11/tst_iconprovider/icons/mockTheme/apps' |
145 | === added directory 'tests/unit_x11/tst_iconprovider/icons/mockTheme/apps/512' |
146 | === added file 'tests/unit_x11/tst_iconprovider/icons/mockTheme/apps/512/gallery-app.png' |
147 | Binary files tests/unit_x11/tst_iconprovider/icons/mockTheme/apps/512/gallery-app.png 1970-01-01 00:00:00 +0000 and tests/unit_x11/tst_iconprovider/icons/mockTheme/apps/512/gallery-app.png 2015-02-19 08:47:10 +0000 differ |
148 | === added file 'tests/unit_x11/tst_iconprovider/icons/mockTheme/index.theme' |
149 | --- tests/unit_x11/tst_iconprovider/icons/mockTheme/index.theme 1970-01-01 00:00:00 +0000 |
150 | +++ tests/unit_x11/tst_iconprovider/icons/mockTheme/index.theme 2015-02-19 08:47:10 +0000 |
151 | @@ -0,0 +1,17 @@ |
152 | +[Icon Theme] |
153 | +Name=MockTheme |
154 | + |
155 | +Directories=actions/scalable,apps/512 |
156 | + |
157 | +[actions/scalable] |
158 | +MinSize=9 |
159 | +Size=90 |
160 | +MaxSize=256 |
161 | +Context=Actions |
162 | +Type=Scalable |
163 | + |
164 | +[apps/512] |
165 | +Size=512 |
166 | +Context=Applications |
167 | +Type=Fixed |
168 | + |
169 | |
170 | === added file 'tests/unit_x11/tst_iconprovider/tst_iconprovider.cpp' |
171 | --- tests/unit_x11/tst_iconprovider/tst_iconprovider.cpp 1970-01-01 00:00:00 +0000 |
172 | +++ tests/unit_x11/tst_iconprovider/tst_iconprovider.cpp 2015-02-19 08:47:10 +0000 |
173 | @@ -0,0 +1,76 @@ |
174 | +/* |
175 | + * Copyright 2015 Canonical Ltd. |
176 | + * |
177 | + * This program is free software; you can redistribute it and/or modify |
178 | + * it under the terms of the GNU Lesser General Public License as published by |
179 | + * the Free Software Foundation; version 3. |
180 | + * |
181 | + * This program is distributed in the hope that it will be useful, |
182 | + * but WITHOUT ANY WARRANTY; without even the implied warranty of |
183 | + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
184 | + * GNU Lesser General Public License for more details. |
185 | + * |
186 | + * You should have received a copy of the GNU Lesser General Public License |
187 | + * along with this program. If not, see <http://www.gnu.org/licenses/>. |
188 | + */ |
189 | + |
190 | +#include <QtTest/QtTest> |
191 | + |
192 | +#define private public |
193 | +#include "unitythemeiconprovider.h" |
194 | +#undef private |
195 | + |
196 | +class tst_IconProvider : public QObject |
197 | +{ |
198 | + Q_OBJECT |
199 | +public: |
200 | + tst_IconProvider() {} |
201 | + |
202 | +private Q_SLOTS: |
203 | + |
204 | + void initTestCase() |
205 | + { |
206 | + qputenv("XDG_DATA_DIRS", SRCDIR); |
207 | + } |
208 | + |
209 | + void test_loadIcon_data() |
210 | + { |
211 | + QTest::addColumn<QString>("icon"); |
212 | + QTest::addColumn<QSize>("requestSize"); |
213 | + QTest::addColumn<QSize>("resultSize"); |
214 | + |
215 | + QTest::newRow("battery0") << "battery-100-charging" << QSize(-1, -1) << QSize(395, 256); |
216 | + QTest::newRow("battery1") << "battery-100-charging" << QSize(-1, 16) << QSize(24, 16); |
217 | + QTest::newRow("battery2") << "battery-100-charging" << QSize(16, -1) << QSize(16, 10); |
218 | + QTest::newRow("battery3") << "battery-100-charging" << QSize(0, 16) << QSize(24, 16); |
219 | + QTest::newRow("battery4") << "battery-100-charging" << QSize(16, 0) << QSize(16, 10); |
220 | + QTest::newRow("battery5") << "battery-100-charging" << QSize(24, 16) << QSize(24, 16); |
221 | + QTest::newRow("battery6") << "battery-100-charging" << QSize(24, 24) << QSize(37, 24); |
222 | + QTest::newRow("battery7") << "battery-100-charging" << QSize(37, 24) << QSize(37, 24); |
223 | + |
224 | + QTest::newRow("gallery0") << "gallery-app" << QSize(-1, -1) << QSize(512, 512); |
225 | + QTest::newRow("gallery1") << "gallery-app" << QSize(-1, 16) << QSize(16, 16); |
226 | + QTest::newRow("gallery2") << "gallery-app" << QSize(16, -1) << QSize(16, 16); |
227 | + QTest::newRow("gallery3") << "gallery-app" << QSize(0, 16) << QSize(16, 16); |
228 | + QTest::newRow("gallery4") << "gallery-app" << QSize(16, 0) << QSize(16, 16); |
229 | + QTest::newRow("gallery5") << "gallery-app" << QSize(24, 16) << QSize(24, 24); |
230 | + QTest::newRow("gallery6") << "gallery-app" << QSize(24, 24) << QSize(24, 24); |
231 | + } |
232 | + |
233 | + void test_loadIcon() |
234 | + { |
235 | + QFETCH(QString, icon); |
236 | + QFETCH(QSize, requestSize); |
237 | + QFETCH(QSize, resultSize); |
238 | + |
239 | + UnityThemeIconProvider provider("mockTheme"); |
240 | + QSize returnedSize; |
241 | + const QPixmap p = provider.requestPixmap(icon, &returnedSize, requestSize); |
242 | + QCOMPARE(p.size(), resultSize); |
243 | + QCOMPARE(returnedSize, resultSize); |
244 | + } |
245 | +}; |
246 | + |
247 | +QTEST_MAIN(tst_IconProvider) |
248 | + |
249 | +#include "tst_iconprovider.moc" |
250 | |
251 | === added file 'tests/unit_x11/tst_iconprovider/tst_iconprovider.pro' |
252 | --- tests/unit_x11/tst_iconprovider/tst_iconprovider.pro 1970-01-01 00:00:00 +0000 |
253 | +++ tests/unit_x11/tst_iconprovider/tst_iconprovider.pro 2015-02-19 08:47:10 +0000 |
254 | @@ -0,0 +1,5 @@ |
255 | +include(../test-include.pri) |
256 | +DEFINES += SRCDIR=\\\"$$PWD/\\\" |
257 | + |
258 | +SOURCES += \ |
259 | + tst_iconprovider.cpp |
260 | |
261 | === modified file 'tests/unit_x11/unit_x11.pro' |
262 | --- tests/unit_x11/unit_x11.pro 2015-02-10 16:47:59 +0000 |
263 | +++ tests/unit_x11/unit_x11.pro 2015-02-19 08:47:10 +0000 |
264 | @@ -4,6 +4,7 @@ |
265 | tst_ubuntu_shape \ |
266 | tst_page \ |
267 | tst_test \ |
268 | + tst_iconprovider \ |
269 | tst_inversemousearea \ |
270 | tst_recreateview \ |
271 | tst_statesaver \ |
I'm also working on a unit test, since i couldn't find any