Merge lp:~zsombi/ubuntu-ui-toolkit/imageExtensionFix into lp:ubuntu-ui-toolkit/staging

[Zsombor Egri]

Use original Image.source in image extension when no scaling is needed in order to keep the formatting which would be otherwise removed from a resolved file URL.

[Cris Dywan (kalikiana)]

Can you add a comment on this behavior next to the fixed line? I think the wording in your commits would be good to have fore future reference. Like "// Use original source instead of resolved to keep invalid characters like #"

review: Needs Fixing

[Olivier Tilloy (osomon)]

I can confirm this fixes the issue originally reported.

I’m not sure about the unit test though:

    "Non-existing file" is misleading, the file does exist. The description should probably be "URL with fragment"

    [file: file + "#" + Date.now()] should probably be [file: "file://" + file + "#" + Date.now()] for correctness

[Olivier Tilloy (osomon)]

And again: "#" in a URL is not an invalid character. If the URL happens to point to a local file, then the fragment will be ignored, but that doesn’t make it invalid.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1784
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/2668/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-amd64-ci/1393
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/1394
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/1394/artifact/work/output/*zip*/output.zip
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-i386-ci/1392

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/2668/rebuild

review: Needs Fixing (continuous-integration)

[Zsombor Egri (zsombi)]

> Can you add a comment on this behavior next to the fixed line? I think the
> wording in your commits would be good to have fore future reference. Like "//
> Use original source instead of resolved to keep invalid characters like #"

I've added a more precise comment there.

[Zsombor Egri (zsombi)]

> I can confirm this fixes the issue originally reported.
>
> I’m not sure about the unit test though:
>
> "Non-existing file" is misleading, the file does exist. The description
> should probably be "URL with fragment"
>
> [file: file + "#" + Date.now()] should probably be [file: "file://" + file
> + "#" + Date.now()] for correctness

Right, it is a valid url, tag fixed as well as proper protocol added. However "file://" protocol is not mandatory in case of files :) even if you add fragments to the url.

[lp:~zsombi/ubuntu-ui-toolkit/imageExtensionFix]

1785. By Zsombor Egri on 2015-12-18

review comments addressed

[Olivier Tilloy (osomon)]

Looks good to me, thanks!

review: Approve

[Cris Dywan (kalikiana)]

Good to go.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1785
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/2669/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-amd64-ci/1394
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/1395
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-armhf-ci/1395/artifact/work/output/*zip*/output.zip
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-vivid-i386-ci/1393

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/2669/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'src/Ubuntu/Components/plugin/ucqquickimageextension.cpp'
--- src/Ubuntu/Components/plugin/ucqquickimageextension.cpp	2015-10-15 08:43:17 +0000
+++ src/Ubuntu/Components/plugin/ucqquickimageextension.cpp	2015-12-18 12:33:45 +0000
@@ -81,8 +81,9 @@
     if (scaleFactor == "1") {
         if (qFuzzyCompare(qGuiApp->devicePixelRatio(), (qreal)1.0)
                 || selectedFilePath.endsWith(".svg") || selectedFilePath.endsWith(".svgz")) {
-            // No scaling necessary. Just pass the file as is.
-            m_image->setSource(QUrl::fromLocalFile(selectedFilePath));
+            // No scaling necessary. Just pass the original file as is
+            // to keep the full url structure
+            m_image->setSource(m_source);
         } else {
             // Need to scale the pixel-based image to suit the devicePixelRatio setting ourselves.
             // If we let Qt do it, Qt will not choose the UITK-supported "@gu" scaled images.

=== modified file 'tests/unit_x11/tst_components/tst_imageprovider.qml'
--- tests/unit_x11/tst_components/tst_imageprovider.qml	2015-03-03 13:20:06 +0000
+++ tests/unit_x11/tst_components/tst_imageprovider.qml	2015-12-18 12:33:45 +0000
@@ -53,10 +53,24 @@
             signalName: "visibleChanged"
         }
 
-        function test_sourceChanged() {
+        function cleanup() {
+            sourceChangeSpy.clear();
+            visibleChangedSpy.clear();
+        }
+
+        // tests adjusted to reproduce bug #1401920
+        function test_sourceChanged_bug1401920_data() {
+            var file = "file:///usr/share/icons/ubuntu-mobile/actions/scalable/delete.svg";
+            return [
+                {rag: "Existing file", file: file},
+                {rag: "Url with fragment", file: file + "#" + Date.now()},
+            ];
+        }
+
+        function test_sourceChanged_bug1401920(data) {
             sourceChangeSpy.target = test;
-            test.source = "/usr/share/icons/ubuntu-mobile/actions/scalable/delete.svg";
-            sourceChangeSpy.wait();
+            test.source = data.file;
+            sourceChangeSpy.wait(400);
         }
 
         function test_sourceNOTIFYable() {