Merge lp:~aacid/ubuntu-ui-toolkit/save_stat_loading_already_loaded_image into lp:ubuntu-ui-toolkit/staging

Proposed by Albert Astals Cid
Status: Merged
Approved by: Christian Dywan
Approved revision: 2177
Merged at revision: 2169
Proposed branch: lp:~aacid/ubuntu-ui-toolkit/save_stat_loading_already_loaded_image
Merge into: lp:ubuntu-ui-toolkit/staging
Diff against target: 268 lines (+120/-20)
9 files modified
src/UbuntuToolkit/ucqquickimageextension.cpp (+53/-2)
src/UbuntuToolkit/ucqquickimageextension_p.h (+1/-0)
tests/unit/qquick_image_extension/borderInName.sci (+0/-7)
tests/unit/qquick_image_extension/data/borderInName.sci (+7/-0)
tests/unit/qquick_image_extension/data/hundred_faces.qml (+13/-0)
tests/unit/qquick_image_extension/data/test@18.sci (+7/-0)
tests/unit/qquick_image_extension/qquick_image_extension.pro (+1/-1)
tests/unit/qquick_image_extension/test@18.sci (+0/-7)
tests/unit/qquick_image_extension/tst_qquick_image_extension.cpp (+38/-3)
To merge this branch: bzr merge lp:~aacid/ubuntu-ui-toolkit/save_stat_loading_already_loaded_image
Reviewer Review Type Date Requested Status
ubuntu-sdk-build-bot continuous-integration Approve
Christian Dywan Approve
Zsombor Egri Needs Fixing
Review via email: mp+316235@code.launchpad.net

Commit message

Save a stat if the image we're loading is already in the cache

No need to call UCUnits::resolveResource to learn we just need to load it normally because the fact that we already loaded it normally means we need to load it normally.

Description of the change

Adding some description, i think CI may be failing because i don't have one.

To post a comment you must log in.
Revision history for this message
Christian Dywan (kalikiana) wrote :

That looks quite nice. Any chance we can unit test it? At least to see that no scaling is applied when unneeded.

review: Needs Information
Revision history for this message
Zsombor Egri (zsombi) wrote :

I have one small request to it, otherwise good.

review: Needs Fixing
2170. By Albert Astals Cid

Use a registered type

2171. By Albert Astals Cid

comment++

2172. By Albert Astals Cid

Add a test that proves this is working

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: 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: 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: Needs Fixing (continuous-integration)
2173. By Albert Astals Cid

Need core private here

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: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
2174. By Albert Astals Cid

Fix gles build

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: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
2175. By Albert Astals Cid

"Better" way of waiting for the image to have the source size set

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: Needs Fixing (continuous-integration)
Revision history for this message
ubuntu-sdk-build-bot (ubuntu-sdk-build-bot) wrote :
review: Needs Fixing (continuous-integration)
2176. By Albert Astals Cid

workaround qqmlengine_p.h using foreach

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)
2177. By Albert Astals Cid

Unfortunately can't cache the ixd, causes crashes in some tests

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
Christian Dywan (kalikiana) wrote :

Finally seems to be passing CI. And thanks for adding the test case (especially since the implementation is not straight-forward now)!

review: Approve
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)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/UbuntuToolkit/ucqquickimageextension.cpp'
2--- src/UbuntuToolkit/ucqquickimageextension.cpp 2016-09-12 09:03:50 +0000
3+++ src/UbuntuToolkit/ucqquickimageextension.cpp 2017-02-06 13:21:12 +0000
4@@ -22,7 +22,13 @@
5 #include <QtCore/QFile>
6 #include <QtCore/QFileInfo>
7 #include <QtGui/QGuiApplication>
8+#include <QtQuick/private/qquickitem_p.h>
9 #include <QtQuick/private/qquickimagebase_p.h>
10+#include <QtQuick/private/qquickpixmapcache_p.h>
11+
12+#define foreach Q_FOREACH
13+#include <QtQml/private/qqmlengine_p.h>
14+#undef foreach
15
16 #include "ucunits_p.h"
17
18@@ -61,11 +67,35 @@
19 return m_source;
20 }
21
22+
23 void UCQQuickImageExtension::setSource(const QUrl& url)
24 {
25 if (url != m_source) {
26 m_source = url;
27- reloadSource();
28+ // We need to wait until the component is complete
29+ // so that m_image->sourceSize() is actually valid
30+ if (QQuickItemPrivate::get(m_image)->componentComplete) {
31+ reloadSource();
32+ } else {
33+ // This is a bit convoluted but i couldn't find a better way to get notified of when
34+ // the image actually finishes constructing.
35+ // Since what we're interested in reloadSource() is the image having the sourceSize set,
36+ // what we do is connect to the sourceSizeChanged signal.
37+ // The problem is that this signal isn't fired if the Image {} doesn't have sourceSize set
38+ // so we tell the engine to fire the sourceSizeChanged signal when the image finishes constructing
39+ // This way if the Image {} has a sourceSize set the lambda gets called because of it
40+ // and if there's no sourceSize set the lambda gets called because we registered the finalize callback
41+
42+ connect(m_image, &QQuickImageBase::sourceSizeChanged,
43+ this,
44+ [&] {
45+ QObject::disconnect(m_image, &QQuickImageBase::sourceSizeChanged, this, nullptr);
46+ reloadSource();
47+ });
48+
49+ QQmlEnginePrivate *engPriv = QQmlEnginePrivate::get(qmlEngine(m_image));
50+ engPriv->registerFinalizeCallback(m_image, m_image->metaObject()->indexOfSignal("sourceSizeChanged()"));
51+ }
52 }
53 }
54
55@@ -80,6 +110,25 @@
56 return;
57 }
58
59+ // If the url we're trying to load is already in the cache and
60+ // the devicePixelRatio is 1, we save calling UCUnits::resolveResource
61+ // and just set that image directly.
62+ // UCUnits::resolveResource is not cheap (does a stat on disk)
63+ if (qFuzzyCompare(qGuiApp->devicePixelRatio(), (qreal)1.0)) {
64+ QSize ss = m_image->sourceSize();
65+ if (ss.isNull() && m_image->image().isNull()) {
66+ // For some reason QQuickImage returns 0x0 as sourceSize
67+ // when the sourceSize is not set (and the image has not yet been loaded)
68+ // so set it back to -1x-1
69+ ss = QSize(-1, -1);
70+ }
71+
72+ if (QQuickPixmap::isCached(m_source, ss)) {
73+ m_image->setSource(m_source);
74+ return;
75+ }
76+ }
77+
78 QString resolved = UCUnits::instance()->resolveResource(m_source);
79
80 if (resolved.isEmpty()) {
81@@ -98,7 +147,9 @@
82 || selectedFilePath.endsWith(QStringLiteral(".svgz"))) {
83 // Take care to pass the original fragment
84 QUrl selectedFileUrl(QUrl::fromLocalFile(selectedFilePath));
85- selectedFileUrl.setFragment(fragment);
86+ if (m_source.hasFragment()) {
87+ selectedFileUrl.setFragment(fragment);
88+ }
89 m_image->setSource(selectedFileUrl);
90 } else {
91 // Need to scale the pixel-based image to suit the devicePixelRatio setting ourselves.
92
93=== modified file 'src/UbuntuToolkit/ucqquickimageextension_p.h'
94--- src/UbuntuToolkit/ucqquickimageextension_p.h 2016-09-09 17:49:07 +0000
95+++ src/UbuntuToolkit/ucqquickimageextension_p.h 2017-02-06 13:21:12 +0000
96@@ -19,6 +19,7 @@
97 #ifndef UCQQUICKIMAGEEXTENSION_P_H
98 #define UCQQUICKIMAGEEXTENSION_P_H
99
100+#include <QtCore/QEvent>
101 #include <QtCore/QByteArray>
102 #include <QtCore/QObject>
103 #include <QtCore/QSharedPointer>
104
105=== removed file 'tests/unit/qquick_image_extension/borderInName.sci'
106--- tests/unit/qquick_image_extension/borderInName.sci 2013-05-06 16:33:54 +0000
107+++ tests/unit/qquick_image_extension/borderInName.sci 1970-01-01 00:00:00 +0000
108@@ -1,7 +0,0 @@
109-source: "borderInName.png"
110-border.left: 9
111-border.right: 2
112-border.top: 9
113-border.bottom: 0
114-horizontalTileMode: Stretch
115-verticalTileMode: Stretch
116
117=== added directory 'tests/unit/qquick_image_extension/data'
118=== added file 'tests/unit/qquick_image_extension/data/borderInName.sci'
119--- tests/unit/qquick_image_extension/data/borderInName.sci 1970-01-01 00:00:00 +0000
120+++ tests/unit/qquick_image_extension/data/borderInName.sci 2017-02-06 13:21:12 +0000
121@@ -0,0 +1,7 @@
122+source: "borderInName.png"
123+border.left: 9
124+border.right: 2
125+border.top: 9
126+border.bottom: 0
127+horizontalTileMode: Stretch
128+verticalTileMode: Stretch
129
130=== added file 'tests/unit/qquick_image_extension/data/face.png'
131Binary files tests/unit/qquick_image_extension/data/face.png 1970-01-01 00:00:00 +0000 and tests/unit/qquick_image_extension/data/face.png 2017-02-06 13:21:12 +0000 differ
132=== added file 'tests/unit/qquick_image_extension/data/hundred_faces.qml'
133--- tests/unit/qquick_image_extension/data/hundred_faces.qml 1970-01-01 00:00:00 +0000
134+++ tests/unit/qquick_image_extension/data/hundred_faces.qml 2017-02-06 13:21:12 +0000
135@@ -0,0 +1,13 @@
136+
137+import QtQuick 2.4
138+import Ubuntu.Components 1.3
139+
140+Repeater {
141+ width: 400
142+ height: 600
143+
144+ model: 100
145+ delegate: Image {
146+ source: "face.png"
147+ }
148+}
149
150=== added file 'tests/unit/qquick_image_extension/data/test@18.sci'
151--- tests/unit/qquick_image_extension/data/test@18.sci 1970-01-01 00:00:00 +0000
152+++ tests/unit/qquick_image_extension/data/test@18.sci 2017-02-06 13:21:12 +0000
153@@ -0,0 +1,7 @@
154+source: "test@18.png"
155+border.left: 9
156+border.right: 2
157+border.top: 9
158+border.bottom: 0
159+horizontalTileMode: Stretch
160+verticalTileMode: Stretch
161
162=== modified file 'tests/unit/qquick_image_extension/qquick_image_extension.pro'
163--- tests/unit/qquick_image_extension/qquick_image_extension.pro 2016-05-31 09:02:35 +0000
164+++ tests/unit/qquick_image_extension/qquick_image_extension.pro 2017-02-06 13:21:12 +0000
165@@ -1,3 +1,3 @@
166 include(../test-include.pri)
167 SOURCES += tst_qquick_image_extension.cpp
168-QT += quick-private
169+QT += core-private quick-private
170
171=== removed file 'tests/unit/qquick_image_extension/test@18.sci'
172--- tests/unit/qquick_image_extension/test@18.sci 2013-05-06 16:33:54 +0000
173+++ tests/unit/qquick_image_extension/test@18.sci 1970-01-01 00:00:00 +0000
174@@ -1,7 +0,0 @@
175-source: "test@18.png"
176-border.left: 9
177-border.right: 2
178-border.top: 9
179-border.bottom: 0
180-horizontalTileMode: Stretch
181-verticalTileMode: Stretch
182
183=== modified file 'tests/unit/qquick_image_extension/tst_qquick_image_extension.cpp'
184--- tests/unit/qquick_image_extension/tst_qquick_image_extension.cpp 2016-09-09 17:49:07 +0000
185+++ tests/unit/qquick_image_extension/tst_qquick_image_extension.cpp 2017-02-06 13:21:12 +0000
186@@ -23,6 +23,8 @@
187 #define protected public
188 #include <UbuntuToolkit/private/ucqquickimageextension_p.h>
189 #undef protected
190+#include <QtCore/private/qabstractfileengine_p.h>
191+#include <QQuickView>
192
193 UT_USE_NAMESPACE
194
195@@ -32,6 +34,20 @@
196 return QDir::temp().entryList(nameFilters, QDir::Files).count();
197 }
198
199+int nFaces = 0;
200+
201+class DummyFileEngineHandler : public QAbstractFileEngineHandler
202+{
203+public:
204+ QAbstractFileEngine *create(const QString &fileName) const
205+ {
206+ if (fileName.endsWith("/face.png"))
207+ nFaces++;
208+
209+ return nullptr;
210+ }
211+};
212+
213 class tst_UCQQuickImageExtension : public QObject
214 {
215 Q_OBJECT
216@@ -78,7 +94,7 @@
217
218 void rewriteContainsBorderInName() {
219 UCQQuickImageExtension image;
220- QString sciFilePath = "borderInName.sci";
221+ QString sciFilePath = "data/borderInName.sci";
222 QString result;
223
224 QTextStream resultStream(&result);
225@@ -86,7 +102,7 @@
226
227 QString expected;
228 QTextStream expectedStream(&expected);
229- expectedStream << "source: \"image://scaling/1/./borderInName.png\"" << endl;
230+ expectedStream << "source: \"image://scaling/1/data/borderInName.png\"" << endl;
231 expectedStream << "border.left: 9" << endl;
232 expectedStream << "border.right: 2" << endl;
233 expectedStream << "border.top: 9" << endl;
234@@ -105,7 +121,7 @@
235 QQuickImageBase baseImage;
236 UCQQuickImageExtension* image1 = new UCQQuickImageExtension(&baseImage);
237 UCQQuickImageExtension* image2 = new UCQQuickImageExtension(&baseImage);
238- QUrl sciFileUrl = QUrl::fromLocalFile("./test.sci");
239+ QUrl sciFileUrl = QUrl::fromLocalFile("./data/test.sci");
240
241 unsigned int initialNumberOfSciFiles = numberOfTemporarySciFiles();
242
243@@ -124,6 +140,25 @@
244 delete image2;
245 QCOMPARE(numberOfTemporarySciFiles(), initialNumberOfSciFiles + 1);
246 }
247+
248+ void onlyOneStatRepeatedImage() {
249+ DummyFileEngineHandler handler;
250+
251+ QQuickView view(QUrl::fromLocalFile("./data/hundred_faces.qml"));
252+
253+ // Wait until the app is idle
254+ bool idle = false;
255+ while (!idle) {
256+ QEventLoop l;
257+ idle = !l.processEvents();
258+ }
259+ // We have actually loaded the image
260+ QVERIFY(nFaces > 0);
261+ // The number at the moment of writing this test is 4, but we
262+ // can't know what Qt does internally, so i'm going for a relatively safe <10
263+ // the important part is that is not 100 times (i.e. one for every Image item)
264+ QVERIFY(nFaces < 10);
265+ }
266 };
267
268 QTEST_MAIN(tst_UCQQuickImageExtension)

Subscribers

People subscribed via source and target branches