Merge into trunk : crossFadeImage_SourceSize_fix : Code : Ubuntu UI Toolkit

Reviewer	Review Type	Date Requested	Status
Tim Peeters			Disapprove on 2013-10-11
PS Jenkins bot	continuous-integration		Needs Fixing on 2013-10-02
Zoltan Balogh		2013-10-02	Approve on 2013-10-02
Review via email: mp+188778@code.launchpad.net

Revision history for this message

Michael Zanetti (mzanetti) wrote on 2013-10-02:

#

108 +
109 + function test_sourceSize() {
110 + loadImage("../../../examples/ubuntu-ui-toolkit-gallery/demo_image.jpg");
111 + compare(crossFadeImage.sourceSize.width, 640, "Source width incorrectly initialized.");
112 + compare(crossFadeImage.sourceSize.height, 427, "Source height incorrectly initialized.");
113 + crossFadeImage.sourceSize.width = 100;
114 + crossFadeImage.sourceSize.height = 101;
115 + compare(crossFadeImage.sourceSize.width, 100, "Source width incorrectly updated.");
116 + compare(crossFadeImage.sourceSize.height, 101, "Source height incorrectly updated.");
117 + waitForAnimation();
118 + cleanupTest();
119 + }

I've seen many of those tests in the SDK and I personally think this is quite useless. It only tests if the component actually compiles (ok, better than nothing) and if there actually is a property size sourceSize which is not readonly. That's it. But if the sourceSize is actually used by image? This is not tested. Here's an example how to check that:

// Put this into some helper.js, or TestCaseBase class to inherit from as you're gonna need this in every test as of now. In unity we have a UnityTestCase which inherits a TestCase and adds lots of useful helpers. Feel free to copy that too.

    // Find an object with the given name in the children tree of "obj"
    function findChild(obj,objectName) {
        var childs = new Array(0);
        childs.push(obj)
        while (childs.length > 0) {
            if (childs[0].objectName == objectName) {
                return childs[0]
            }
            for (var i in childs[0].children) {
                childs.push(childs[0].children[i])
            }
            childs.splice(0, 1);
        }
        return undefined;
    }

// Now we have a way to look inside the components. Let's do the test using this:

Edit CrossFadeImage.qml and add "objectName" properties to image1 and image2, like this:

Image {
    id: image1
    objectName: "image1"
    ...
}

And now the test

function test_sourceSize() {
  loadImage("../../../examples/ubuntu-ui-toolkit-gallery/demo_image.jpg");
  compare(crossFadeImage.sourceSize.width, 640, "Source width incorrectly initialized.");
  compare(crossFadeImage.sourceSize.height, 427, "Source height incorrectly initialized.");
  crossFadeImage.sourceSize.width = 100;
  crossFadeImage.sourceSize.height = 101;

var image1 = findChild(crossFadeImage, "image1");
var image2 = findChild(crossFadeImage, "image2");

compare(image1.sourceSize.width, 100, "Source width incorrectly updated.");
compare(image1.sourceSize.height, 101, "Source height incorrectly updated.");

compare(image2.sourceSize.width, 100, "Source width incorrectly updated.");
compare(image2.sourceSize.height, 101, "Source height incorrectly updated.");

waitForAnimation();
cleanupTest();
}

108	+
109	+ function test_sourceSize() {
110	+ loadImage("../../../examples/ubuntu-ui-toolkit-gallery/demo_image.jpg");
111	+ compare(crossFadeImage.sourceSize.width, 640, "Source width incorrectly initialized.");
112	+ compare(crossFadeImage.sourceSize.height, 427, "Source height incorrectly initialized.");
113	+ crossFadeImage.sourceSize.width = 100;
114	+ crossFadeImage.sourceSize.height = 101;
115	+ compare(crossFadeImage.sourceSize.width, 100, "Source width incorrectly updated.");
116	+ compare(crossFadeImage.sourceSize.height, 101, "Source height incorrectly updated.");
117	+ waitForAnimation();
118	+ cleanupTest();
119	+ }

I've seen many of those tests in the SDK and I personally think this is quite useless. It only tests if the component actually compiles (ok, better than nothing) and if there actually is a property size sourceSize which is not readonly. That's it. But if the sourceSize is actually used by image? This is not tested. Here's an example how to check that:

// Put this into some helper.js, or TestCaseBase class to inherit from as you're gonna need this in every test as of now. In unity we have a UnityTestCase which inherits a TestCase and adds lots of useful helpers. Feel free to copy that too.

// Find an object with the given name in the children tree of "obj"
    function findChild(obj,objectName) {
        var childs = new Array(0);
        childs.push(obj)
        while (childs.length > 0) {
            if (childs[0].objectName == objectName) {
                return childs[0]
            }
            for (var i in childs[0].children) {
                childs.push(childs[0].children[i])
            }
            childs.splice(0, 1);
        }
        return undefined;
    }

// Now we have a way to look inside the components. Let's do the test using this:

Edit CrossFadeImage.qml and add "objectName" properties to image1 and image2, like this:

Image {
    id: image1
    objectName: "image1"
    ...
}

And now the test

function test_sourceSize() {
  loadImage("../../../examples/ubuntu-ui-toolkit-gallery/demo_image.jpg");
  compare(crossFadeImage.sourceSize.width, 640, "Source width incorrectly initialized.");
  compare(crossFadeImage.sourceSize.height, 427, "Source height incorrectly initialized.");
  crossFadeImage.sourceSize.width = 100;
  crossFadeImage.sourceSize.height = 101;

var image1 = findChild(crossFadeImage, "image1");  
  var image2 = findChild(crossFadeImage, "image2");

compare(image1.sourceSize.width, 100, "Source width incorrectly updated.");
  compare(image1.sourceSize.height, 101, "Source height incorrectly updated.");

compare(image2.sourceSize.width, 100, "Source width incorrectly updated.");
  compare(image2.sourceSize.height, 101, "Source height incorrectly updated.");

waitForAnimation();
  cleanupTest();
}

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-10-02:

#

Click here to trigger a rebuild:
http://s-jenkins:8080/job/ubuntu-ui-toolkit-ci/841/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Zsombor Egri (zsombi) wrote on 2013-10-02:

#

> 108 +
> 109 + function test_sourceSize() {
> 110 + loadImage("../../../examples/ubuntu-ui-toolkit-
> gallery/demo_image.jpg");
> 111 + compare(crossFadeImage.sourceSize.width, 640, "Source width
> incorrectly initialized.");
> 112 + compare(crossFadeImage.sourceSize.height, 427, "Source height
> incorrectly initialized.");
> 113 + crossFadeImage.sourceSize.width = 100;
> 114 + crossFadeImage.sourceSize.height = 101;
> 115 + compare(crossFadeImage.sourceSize.width, 100, "Source width
> incorrectly updated.");
> 116 + compare(crossFadeImage.sourceSize.height, 101, "Source height
> incorrectly updated.");
> 117 + waitForAnimation();
> 118 + cleanupTest();
> 119 + }
>
> I've seen many of those tests in the SDK and I personally think this is quite
> useless. It only tests if the component actually compiles (ok, better than
> nothing) and if there actually is a property size sourceSize which is not
> readonly. That's it. But if the sourceSize is actually used by image? This is
> not tested. Here's an example how to check that:
>
> // Put this into some helper.js, or TestCaseBase class to inherit from as
> you're gonna need this in every test as of now. In unity we have a
> UnityTestCase which inherits a TestCase and adds lots of useful helpers. Feel
> free to copy that too.
>
> // Find an object with the given name in the children tree of "obj"
> function findChild(obj,objectName) {
> var childs = new Array(0);
> childs.push(obj)
> while (childs.length > 0) {
> if (childs[0].objectName == objectName) {
> return childs[0]
> }
> for (var i in childs[0].children) {
> childs.push(childs[0].children[i])
> }
> childs.splice(0, 1);
> }
> return undefined;
> }
>
>
> // Now we have a way to look inside the components. Let's do the test using
> this:
>
> Edit CrossFadeImage.qml and add "objectName" properties to image1 and image2,
> like this:
>
> Image {
> id: image1
> objectName: "image1"
> ...
> }
>
> And now the test
>
> function test_sourceSize() {
> loadImage("../../../examples/ubuntu-ui-toolkit-gallery/demo_image.jpg");
> compare(crossFadeImage.sourceSize.width, 640, "Source width incorrectly
> initialized.");
> compare(crossFadeImage.sourceSize.height, 427, "Source height incorrectly
> initialized.");
> crossFadeImage.sourceSize.width = 100;
> crossFadeImage.sourceSize.height = 101;
>
> var image1 = findChild(crossFadeImage, "image1");
> var image2 = findChild(crossFadeImage, "image2");
>
> compare(image1.sourceSize.width, 100, "Source width incorrectly updated.");
> compare(image1.sourceSize.height, 101, "Source height incorrectly
> updated.");
>
> compare(image2.sourceSize.width, 100, "Source width incorrectly updated.");
> compare(image2.sourceSize.height, 101, "Source height incorrectly
> updated.");
>
> waitForAnimation();
> cleanupTest();
> }

Nice. But let's do this in a separate MR as we also need to agree on how to use the objectName in our components.

> 108     +
> 109     + function test_sourceSize() {
> 110     + loadImage("../../../examples/ubuntu-ui-toolkit-
> gallery/demo_image.jpg");
> 111     + compare(crossFadeImage.sourceSize.width, 640, "Source width
> incorrectly initialized.");
> 112     + compare(crossFadeImage.sourceSize.height, 427, "Source height
> incorrectly initialized.");
> 113     + crossFadeImage.sourceSize.width = 100;
> 114     + crossFadeImage.sourceSize.height = 101;
> 115     + compare(crossFadeImage.sourceSize.width, 100, "Source width
> incorrectly updated.");
> 116     + compare(crossFadeImage.sourceSize.height, 101, "Source height
> incorrectly updated.");
> 117     + waitForAnimation();
> 118     + cleanupTest();
> 119     + }
> 
> I've seen many of those tests in the SDK and I personally think this is quite
> useless. It only tests if the component actually compiles (ok, better than
> nothing) and if there actually is a property size sourceSize which is not
> readonly. That's it. But if the sourceSize is actually used by image? This is
> not tested. Here's an example how to check that:
> 
> // Put this into some helper.js, or TestCaseBase class to inherit from as
> you're gonna need this in every test as of now. In unity we have a
> UnityTestCase which inherits a TestCase and adds lots of useful helpers. Feel
> free to copy that too.
> 
>     // Find an object with the given name in the children tree of "obj"
>     function findChild(obj,objectName) {
>         var childs = new Array(0);
>         childs.push(obj)
>         while (childs.length > 0) {
>             if (childs[0].objectName == objectName) {
>                 return childs[0]
>             }
>             for (var i in childs[0].children) {
>                 childs.push(childs[0].children[i])
>             }
>             childs.splice(0, 1);
>         }
>         return undefined;
>     }
> 
> 
> // Now we have a way to look inside the components. Let's do the test using
> this:
> 
> Edit CrossFadeImage.qml and add "objectName" properties to image1 and image2,
> like this:
> 
> Image {
>     id: image1
>     objectName: "image1"
>     ...
> }
> 
> And now the test
> 
> function test_sourceSize() {
>   loadImage("../../../examples/ubuntu-ui-toolkit-gallery/demo_image.jpg");
>   compare(crossFadeImage.sourceSize.width, 640, "Source width incorrectly
> initialized.");
>   compare(crossFadeImage.sourceSize.height, 427, "Source height incorrectly
> initialized.");
>   crossFadeImage.sourceSize.width = 100;
>   crossFadeImage.sourceSize.height = 101;
> 
>   var image1 = findChild(crossFadeImage, "image1");
>   var image2 = findChild(crossFadeImage, "image2");
> 
>   compare(image1.sourceSize.width, 100, "Source width incorrectly updated.");
>   compare(image1.sourceSize.height, 101, "Source height incorrectly
> updated.");
> 
>   compare(image2.sourceSize.width, 100, "Source width incorrectly updated.");
>   compare(image2.sourceSize.height, 101, "Source height incorrectly
> updated.");
> 
>   waitForAnimation();
>   cleanupTest();
> }

Nice. But let's do this in a separate MR as we also need to agree on how to use the objectName in our components.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-10-02:

#

Click here to trigger a rebuild:
http://s-jenkins:8080/job/ubuntu-ui-toolkit-ci/843/rebuild

review: Approve (continuous-integration)

Revision history for this message

Zoltan Balogh (bzoltan) on 2013-10-02:

#

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-10-02:

#

review: Needs Fixing (continuous-integration)

Revision history for this message

Tim Peeters (tpeeters) wrote on 2013-10-11:

#

is this MR still valid?

Looks the same as this one: https://code.launchpad.net/~tpeeters/ubuntu-ui-toolkit/crossFadeImage_SourceSize_fix/+merge/187454 but that one has additional changes.

review: Needs Information

Revision history for this message

Tim Peeters (tpeeters) wrote on 2013-10-11:

#

original MR fixed the qdoc issue, so let's go back to that one.

review: Disapprove

 === modified file 'CHANGES'
 --- CHANGES	2013-09-24 19:47:54 +0000
 +++ CHANGES	2013-10-02 08:07:31 +0000
@@ -54,6 +54,7 @@
  * ADDED IN Panel: function open()
  * ADDED IN Panel: function close()
  * DEPRECATED IN Panel: writable property opened. Will be made read-only.
++* CHANGED in CrossFadeImage: readonly property size sourceSize TO property size sourceSize
  Compatibility Breaks
  ********************
 === modified file 'components.api'
 --- components.api	2013-09-30 12:00:41 +0000
 +++ components.api	2013-10-02 08:07:31 +0000
@@ -42,7 +42,7 @@
      property int fillMode
      property int fadeDuration
      readonly property bool running
--    readonly property size sourceSize
++    property size sourceSize
      readonly property int status
  modules/Ubuntu/Components/DraggingArea.qml
  MouseArea
 === modified file 'modules/Ubuntu/Components/CrossFadeImage.qml'
 --- modules/Ubuntu/Components/CrossFadeImage.qml	2013-08-28 12:56:17 +0000
 +++ modules/Ubuntu/Components/CrossFadeImage.qml	2013-10-02 08:07:31 +0000
@@ -81,8 +81,31 @@
      /*!
        The actual width and height of the loaded image
++      This property holds the actual width and height of the loaded image.
++
++      Unlike the width and height properties, which scale the painting of the image,
++      this property sets the actual number of pixels stored for the loaded image so that large
++      images do not use more memory than necessary.
++
++      Note: Changing this property dynamically causes the image source to be reloaded, potentially
++      even from the network, if it is not in the disk cache.
      */
--    readonly property size sourceSize: Qt.size(internals.loadingImage.sourceSize.width, internals.loadingImage.sourceSize.height)
++    // FIXME: Support resetting sourceSize
++    property size sourceSize: internals.loadingImage ? Qt.size(internals.loadingImage.sourceSize.width, internals.loadingImage.sourceSize.height) : Qt.size(0, 0)
++
++    Binding {
++        target: crossFadeImage
++        property: "sourceSize"
++        value: internals.loadingImage ? Qt.size(internals.loadingImage.sourceSize.width, internals.loadingImage.sourceSize.height) : Qt.size(0, 0)
++        when: internals.forcedSourceSize === undefined
++    }
++
++    /*! \internal */
++    onSourceSizeChanged: {
++        if (internals.loadingImage && (sourceSize != Qt.size(internals.loadingImage.sourceSize.width, internals.loadingImage.sourceSize.height))) {
++            internals.forcedSourceSize = sourceSize;
++        }
++    }
      /*!
        \qmlproperty enumeration status
@@ -102,6 +125,11 @@
          id: internals
          /*! \internal
++          Source size specified by the setting crossFadeImage.sourceSize.
++         */
++        property size forcedSourceSize
++
++        /*! \internal
            Defines the image currently being shown
          */
          property Image currentImage: image1
@@ -130,6 +158,12 @@
          asynchronous: true
          fillMode: parent.fillMode
          z: 1
++        Binding {
++            target: image1
++            property: "sourceSize"
++            value: internals.forcedSourceSize
++            when: internals.forcedSourceSize !== undefined
++        }
+     }
      Image {
@@ -139,6 +173,12 @@
          asynchronous: true
          fillMode: parent.fillMode
          z: 0
++        Binding {
++            target: image2
++            property: "sourceSize"
++            value: internals.forcedSourceSize
++            when: internals.forcedSourceSize !== undefined
++        }
+     }
      /*!
 === modified file 'tests/unit/tst_components/tst_CrossFadeImage.qml'
 --- tests/unit/tst_components/tst_CrossFadeImage.qml	2013-07-08 13:44:33 +0000
 +++ tests/unit/tst_components/tst_CrossFadeImage.qml	2013-10-02 08:07:31 +0000
@@ -93,4 +93,16 @@
          waitForAnimation();
          cleanupTest();
+     }
++
++    function test_sourceSize() {
++        loadImage("../../../examples/ubuntu-ui-toolkit-gallery/demo_image.jpg");
++        compare(crossFadeImage.sourceSize.width, 640, "Source width incorrectly initialized.");
++        compare(crossFadeImage.sourceSize.height, 427, "Source height incorrectly initialized.");
++        crossFadeImage.sourceSize.width = 100;
++        crossFadeImage.sourceSize.height = 101;
++        compare(crossFadeImage.sourceSize.width, 100, "Source width incorrectly updated.");
++        compare(crossFadeImage.sourceSize.height, 101, "Source height incorrectly updated.");
++        waitForAnimation();
++        cleanupTest();
++    }
+ }

Ubuntu UI Toolkit

Merge lp:~zsombi/ubuntu-ui-toolkit/crossFadeImage_SourceSize_fix into lp:ubuntu-ui-toolkit

Commit message

Description of the change

Unmerged revisions

Preview Diff

Subscribers