Merge lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/trunk-fix-icon-svg-image-loading into lp:ubuntu-ui-toolkit/staging

Proposed by Michał Sawicz
Status: Rejected
Rejected by: Michał Sawicz
Proposed branch: lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/trunk-fix-icon-svg-image-loading
Merge into: lp:ubuntu-ui-toolkit/staging
Diff against target: 280 lines (+77/-66)
3 files modified
src/Ubuntu/Components/plugin/unitythemeiconprovider.cpp (+61/-51)
src/Ubuntu/Components/plugin/unitythemeiconprovider.h (+1/-1)
tests/unit_x11/tst_iconprovider/tst_iconprovider.cpp (+15/-14)
To merge this branch: bzr merge lp:~ubuntu-sdk-team/ubuntu-ui-toolkit/trunk-fix-icon-svg-image-loading
Reviewer Review Type Date Requested Status
Michał Sawicz Disapprove
ubuntu-sdk-build-bot continuous-integration Approve
Review via email: mp+292509@code.launchpad.net

Commit message

Adjust the UnityThemeIconProvider to duplicate Qt's own image loading behaviour, fixes some image scaling issues

Description of the change

The Pixmap->Image thing might be unnecessary, but
1) I wanted to duplicate exactly Qt's behaviour and
2) Qt converts the QPixmap to a QImage internally before uploading to texture

To post a comment you must log in.
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
1283. By Michał Sawicz

Make tst_IconProvider talk in QImage

Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
1284. By Lukáš Tinkl

fix loading & scaling SVGs

Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
1285. By Lukáš Tinkl

add a test with wrong aspect ratio that returns a correct one

1286. By Lukáš Tinkl

add a test that passes here and fails with trunk

Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Michał Sawicz (saviq) wrote :
review: Disapprove

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/Ubuntu/Components/plugin/unitythemeiconprovider.cpp'
--- src/Ubuntu/Components/plugin/unitythemeiconprovider.cpp 2016-03-16 14:24:15 +0000
+++ src/Ubuntu/Components/plugin/unitythemeiconprovider.cpp 2016-04-27 16:15:41 +0000
@@ -21,8 +21,7 @@
21#include <QDir>21#include <QDir>
22#include <QFileInfo>22#include <QFileInfo>
23#include <QSettings>23#include <QSettings>
24#include <QSvgRenderer>24#include <QImageReader>
25#include <QPainter>
26#include <QStandardPaths>25#include <QStandardPaths>
27#include <QtDebug>26#include <QtDebug>
2827
@@ -48,27 +47,27 @@
48 // Does a breadth-first search for an icon with any name in @names. Parent47 // Does a breadth-first search for an icon with any name in @names. Parent
49 // themes are only looked at if the current theme doesn't contain any icon48 // themes are only looked at if the current theme doesn't contain any icon
50 // in @names.49 // in @names.
51 QPixmap findBestIcon(const QStringList &names, const QSize &size, QSet<QString> *alreadySearchedThemes)50 QImage findBestIcon(const QStringList &names, QSize *impsize, const QSize &size, QSet<QString> *alreadySearchedThemes)
52 {51 {
53 if (alreadySearchedThemes) {52 if (alreadySearchedThemes) {
54 if (alreadySearchedThemes->contains(name))53 if (alreadySearchedThemes->contains(name))
55 return QPixmap();54 return QImage();
56 alreadySearchedThemes->insert(name);55 alreadySearchedThemes->insert(name);
57 }56 }
5857
59 Q_FOREACH(const QString &name, names) {58 Q_FOREACH(const QString &name, names) {
60 QPixmap pixmap = lookupIcon(name, size);59 QImage image = lookupIcon(name, impsize, size);
61 if (!pixmap.isNull())60 if (!image.isNull())
62 return pixmap;61 return image;
63 }62 }
6463
65 Q_FOREACH(IconThemePointer theme, parents) {64 Q_FOREACH(IconThemePointer theme, parents) {
66 QPixmap pixmap = theme->findBestIcon(names, size, alreadySearchedThemes);65 QImage image = theme->findBestIcon(names, impsize, size, alreadySearchedThemes);
67 if (!pixmap.isNull())66 if (!image.isNull())
68 return pixmap;67 return image;
69 }68 }
7069
71 return QPixmap();70 return QImage();
72 }71 }
7372
74private:73private:
@@ -82,7 +81,7 @@
8281
83 IconTheme(const QString &name): name(name)82 IconTheme(const QString &name): name(name)
84 {83 {
85 QStringList paths = QStandardPaths::standardLocations(QStandardPaths::GenericDataLocation);84 const QStringList paths = QStandardPaths::standardLocations(QStandardPaths::GenericDataLocation);
8685
87 Q_FOREACH(const QString &path, paths) {86 Q_FOREACH(const QString &path, paths) {
88 QDir dir(path + "/icons/" + name);87 QDir dir(path + "/icons/" + name);
@@ -130,30 +129,42 @@
130 return Fixed;129 return Fixed;
131 }130 }
132131
133 static QPixmap loadIcon(const QString &filename, const QSize &size)132 static QImage loadIcon(const QString &filename, QSize *impsize, const QSize &requestSize)
134 {133 {
135 QPixmap pixmap;134 QImage image;
136135 QImageReader imgio(filename);
137 const bool anyZero = size.width() <= 0 || size.height() <= 0;136
138 const Qt::AspectRatioMode scaleMode = anyZero ? Qt::KeepAspectRatioByExpanding : Qt::KeepAspectRatio;137 const bool force_scale = imgio.format() == QStringLiteral("svg")
139138 || imgio.format() == QStringLiteral("svgz");
140 if (filename.endsWith(QLatin1String(".png"))) {139
141 pixmap = QPixmap(filename);140 if (requestSize.width() > 0 || requestSize.height() > 0) {
142 if (!pixmap.isNull() && !size.isNull() && (pixmap.width() != size.width() || pixmap.height() != size.height())) {141 QSize s = imgio.size();
143 const QSize newSize = pixmap.size().scaled(size.width(), size.height(), scaleMode);142 qreal ratio = 0.0;
144 pixmap = pixmap.scaled(newSize);143 if (requestSize.width() > 0 && (force_scale || requestSize.width() < s.width())) {
145 }144 ratio = qreal(requestSize.width())/s.width();
146 }145 }
147 else if (filename.endsWith(QLatin1String(".svg"))) {146 if (requestSize.height() > 0 && (force_scale || requestSize.height() < s.height())) {
148 QSvgRenderer renderer(filename);147 qreal hr = qreal(requestSize.height())/s.height();
149 pixmap = QPixmap(renderer.defaultSize().scaled(size.width(), size.height(), scaleMode));148 if (ratio == 0.0 || hr < ratio)
150 pixmap.fill(Qt::transparent);149 ratio = hr;
151 QPainter painter(&pixmap);150 }
152 renderer.render(&painter);151 if (ratio > 0.0) {
153 painter.end();152 s.setHeight(qRound(s.height() * ratio));
154 }153 s.setWidth(qRound(s.width() * ratio));
155154 imgio.setScaledSize(s);
156 return pixmap;155 }
156 }
157
158 if (impsize)
159 *impsize = imgio.scaledSize();
160
161 if (imgio.read(&image)) {
162 if (impsize)
163 *impsize = image.size();
164 return image;
165 } else {
166 return QImage();
167 }
157 }168 }
158169
159 QString lookupIconFile(const QString &dir, const QString &name)170 QString lookupIconFile(const QString &dir, const QString &name)
@@ -174,16 +185,16 @@
174 return QString();185 return QString();
175 }186 }
176187
177 QPixmap lookupIcon(const QString &iconName, const QSize &size)188 QImage lookupIcon(const QString &iconName, QSize *impsize, const QSize &size)
178 {189 {
179 const int iconSize = qMax(size.width(), size.height());190 const int iconSize = qMax(size.width(), size.height());
180 if (iconSize > 0)191 if (iconSize > 0)
181 return lookupBestMatchingIcon(iconName, size);192 return lookupBestMatchingIcon(iconName, impsize, size);
182 else193 else
183 return lookupLargestIcon(iconName);194 return lookupLargestIcon(iconName, impsize);
184 }195 }
185196
186 QPixmap lookupBestMatchingIcon(const QString &iconName, const QSize &size)197 QImage lookupBestMatchingIcon(const QString &iconName, QSize *impsize, const QSize &size)
187 {198 {
188 int minDistance = 10000;199 int minDistance = 10000;
189 QString bestFilename;200 QString bestFilename;
@@ -205,12 +216,12 @@
205 }216 }
206217
207 if (!bestFilename.isNull())218 if (!bestFilename.isNull())
208 return loadIcon(bestFilename, size);219 return loadIcon(bestFilename, impsize, size);
209220
210 return QPixmap();221 return QImage();
211 }222 }
212223
213 QPixmap lookupLargestIcon(const QString &iconName)224 QImage lookupLargestIcon(const QString &iconName, QSize *impsize)
214 {225 {
215 int maxSize = 0;226 int maxSize = 0;
216 QString bestFilename;227 QString bestFilename;
@@ -228,9 +239,9 @@
228 }239 }
229240
230 if (!bestFilename.isNull())241 if (!bestFilename.isNull())
231 return loadIcon(bestFilename, QSize(maxSize, maxSize));242 return loadIcon(bestFilename, impsize, QSize(maxSize, maxSize));
232243
233 return QPixmap();244 return QImage();
234 }245 }
235246
236 int directorySizeDistance(const Directory &dir, const QSize &iconSize)247 int directorySizeDistance(const Directory &dir, const QSize &iconSize)
@@ -258,24 +269,23 @@
258};269};
259270
260UnityThemeIconProvider::UnityThemeIconProvider(const QString &themeName):271UnityThemeIconProvider::UnityThemeIconProvider(const QString &themeName):
261 QQuickImageProvider(QQuickImageProvider::Pixmap)272 QQuickImageProvider(QQuickImageProvider::Image)
262{273{
263 theme = IconTheme::get(themeName);274 theme = IconTheme::get(themeName);
264}275}
265276
266QPixmap UnityThemeIconProvider::requestPixmap(const QString &id, QSize *size, const QSize &requestedSize)277QImage UnityThemeIconProvider::requestImage(const QString &id, QSize *size, const QSize &requestedSize)
267{278{
268 // The hicolor theme will be searched last as per279 // The hicolor theme will be searched last as per
269 // https://specifications.freedesktop.org/icon-theme-spec/icon-theme-spec-latest.html280 // https://specifications.freedesktop.org/icon-theme-spec/icon-theme-spec-latest.html
270 QSet<QString> alreadySearchedThemes;281 QSet<QString> alreadySearchedThemes;
271 const QStringList names = id.split(QLatin1Char(','), QString::SkipEmptyParts);282 const QStringList names = id.split(QLatin1Char(','), QString::SkipEmptyParts);
272 QPixmap pixmap = theme->findBestIcon(names, requestedSize, &alreadySearchedThemes);283 QImage image = theme->findBestIcon(names, size, requestedSize, &alreadySearchedThemes);
273284
274 if (pixmap.isNull()) {285 if (image.isNull()) {
275 IconTheme::IconThemePointer theme = IconTheme::get(QStringLiteral("hicolor"));286 IconTheme::IconThemePointer theme = IconTheme::get(QStringLiteral("hicolor"));
276 return theme->findBestIcon(names, requestedSize, nullptr);287 return theme->findBestIcon(names, size, requestedSize, nullptr);
277 }288 }
278289
279 *size = pixmap.size();290 return image;
280 return pixmap;
281}291}
282292
=== modified file 'src/Ubuntu/Components/plugin/unitythemeiconprovider.h'
--- src/Ubuntu/Components/plugin/unitythemeiconprovider.h 2016-03-16 14:24:15 +0000
+++ src/Ubuntu/Components/plugin/unitythemeiconprovider.h 2016-04-27 16:15:41 +0000
@@ -25,7 +25,7 @@
25{25{
26public:26public:
27 UnityThemeIconProvider(const QString &themeName = QStringLiteral("suru"));27 UnityThemeIconProvider(const QString &themeName = QStringLiteral("suru"));
28 QPixmap requestPixmap(const QString &id, QSize *size, const QSize &requestedSize);28 QImage requestImage(const QString &id, QSize *size, const QSize &requestedSize);
2929
30private:30private:
31 QSharedPointer<class IconTheme> theme;31 QSharedPointer<class IconTheme> theme;
3232
=== modified file 'tests/unit_x11/tst_iconprovider/tst_iconprovider.cpp'
--- tests/unit_x11/tst_iconprovider/tst_iconprovider.cpp 2016-03-16 15:02:35 +0000
+++ tests/unit_x11/tst_iconprovider/tst_iconprovider.cpp 2016-04-27 16:15:41 +0000
@@ -39,14 +39,15 @@
39 QTest::addColumn<QSize>("requestSize");39 QTest::addColumn<QSize>("requestSize");
40 QTest::addColumn<QSize>("resultSize");40 QTest::addColumn<QSize>("resultSize");
4141
42 QTest::newRow("battery0") << "battery-100-charging" << QSize(-1, -1) << QSize(256, 165);42 QTest::newRow("battery0") << "battery-100-charging" << QSize(-1, -1) << QSize(256, 166);
43 QTest::newRow("battery1") << "battery-100-charging" << QSize(-1, 16) << QSize(24, 16);43 QTest::newRow("battery1") << "battery-100-charging" << QSize(-1, 16) << QSize(25, 16);
44 QTest::newRow("battery2") << "battery-100-charging" << QSize(16, -1) << QSize(16, 10);44 QTest::newRow("battery2") << "battery-100-charging" << QSize(16, -1) << QSize(16, 10);
45 QTest::newRow("battery3") << "battery-100-charging" << QSize(0, 16) << QSize(24, 16);45 QTest::newRow("battery3") << "battery-100-charging" << QSize(0, 16) << QSize(25, 16);
46 QTest::newRow("battery4") << "battery-100-charging" << QSize(16, 0) << QSize(16, 10);46 QTest::newRow("battery4") << "battery-100-charging" << QSize(16, 0) << QSize(16, 10);
47 QTest::newRow("battery5") << "battery-100-charging" << QSize(24, 16) << QSize(24, 16);47 QTest::newRow("battery5") << "battery-100-charging" << QSize(24, 16) << QSize(24, 16);
48 QTest::newRow("battery6") << "battery-100-charging" << QSize(24, 24) << QSize(24, 15);48 QTest::newRow("battery6") << "battery-100-charging" << QSize(24, 24) << QSize(24, 16);
49 QTest::newRow("battery7") << "battery-100-charging" << QSize(37, 24) << QSize(37, 24);49 QTest::newRow("battery7") << "battery-100-charging" << QSize(37, 24) << QSize(37, 24);
50 QTest::newRow("battery8") << "battery-100-charging" << QSize(15, 512) << QSize(15, 10);
5051
51 QTest::newRow("gallery0") << "gallery-app" << QSize(-1, -1) << QSize(512, 512);52 QTest::newRow("gallery0") << "gallery-app" << QSize(-1, -1) << QSize(512, 512);
52 QTest::newRow("gallery1") << "gallery-app" << QSize(-1, 16) << QSize(16, 16);53 QTest::newRow("gallery1") << "gallery-app" << QSize(-1, 16) << QSize(16, 16);
@@ -65,13 +66,13 @@
6566
66 UnityThemeIconProvider provider("mockTheme");67 UnityThemeIconProvider provider("mockTheme");
67 QSize returnedSize;68 QSize returnedSize;
68 QPixmap p = provider.requestPixmap(icon, &returnedSize, requestSize);69 QImage i = provider.requestImage(icon, &returnedSize, requestSize);
69 QCOMPARE(p.size(), resultSize);70 QCOMPARE(i.size(), resultSize);
70 QCOMPARE(returnedSize, resultSize);71 QCOMPARE(returnedSize, resultSize);
7172
72 // Search again to make sure subsequent searches still work73 // Search again to make sure subsequent searches still work
73 p = provider.requestPixmap(icon, &returnedSize, requestSize);74 i = provider.requestImage(icon, &returnedSize, requestSize);
74 QCOMPARE(p.size(), resultSize);75 QCOMPARE(i.size(), resultSize);
75 QCOMPARE(returnedSize, resultSize);76 QCOMPARE(returnedSize, resultSize);
76 }77 }
7778
@@ -82,14 +83,14 @@
8283
83 // myapp is in MockTheme3 (white) and hicolor (black)84 // myapp is in MockTheme3 (white) and hicolor (black)
84 // MockTheme3 one is returned since hicolor is last per spec85 // MockTheme3 one is returned since hicolor is last per spec
85 QPixmap p = provider.requestPixmap("myapp", &returnedSize, QSize(-1, -1));86 QImage i = provider.requestImage("myapp", &returnedSize, QSize(-1, -1));
86 QVERIFY(!p.isNull());87 QVERIFY(!i.isNull());
87 QCOMPARE(QColor(p.toImage().pixel(0,0)), QColor(Qt::white));88 QCOMPARE(QColor(i.pixel(0,0)), QColor(Qt::white));
8889
89 // myapp2 is only in hicolor (black) so that's the one returned90 // myapp2 is only in hicolor (black) so that's the one returned
90 p = provider.requestPixmap("myapp2", &returnedSize, QSize(-1, -1));91 i = provider.requestImage("myapp2", &returnedSize, QSize(-1, -1));
91 QVERIFY(!p.isNull());92 QVERIFY(!i.isNull());
92 QCOMPARE(QColor(p.toImage().pixel(0,0)), QColor(Qt::black));93 QCOMPARE(QColor(i.pixel(0,0)), QColor(Qt::black));
93 }94 }
94};95};
9596

Subscribers

People subscribed via source and target branches