Ubuntu UI Toolkit

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

crossFadeImage_SourceSize_fix
Merge into trunk

Proposed by Tim Peeters on 2013-09-25

Status:

Merged

Approved by:

Tim Peeters on 2013-10-18

Approved revision:

781

Merged at revision:

798

Proposed branch:

lp:~tpeeters/ubuntu-ui-toolkit/crossFadeImage_SourceSize_fix

Merge into:

lp:ubuntu-ui-toolkit

Diff against target:

143 lines (+71/-2)

4 files modified

CHANGES (+1/-0)
components.api (+1/-1)
modules/Ubuntu/Components/CrossFadeImage.qml (+43/-1)
tests/unit/tst_components/tst_CrossFadeImage.qml (+26/-0)

To merge this branch:

bzr merge lp:~tpeeters/ubuntu-ui-toolkit/crossFadeImage_SourceSize_fix

Critical

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot	continuous-integration		Approve on 2013-10-17
Tim Peeters			Approve on 2013-10-17
Zsombor Egri			Needs Fixing on 2013-10-04
Günter Schwann (community)			Approve on 2013-09-25
Michael Zanetti (community)		2013-09-25	Needs Information on 2013-09-25
Review via email: mp+187454@code.launchpad.net

Commit message

Update CrossFadeImage API so that sourceSize can be set by the applications.

* CHANGED in CrossFadeImage: readonly property size sourceSize TO property size sourceSize

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-09-25:

FAILED: Continuous integration, rev:764
http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-ci/748/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-saucy-vm/67/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-touch/1568/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-amd64-ci/605/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-armhf-ci/605/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy-i386/4049/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy-armhf/1570/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Michael Zanetti (mzanetti) wrote on 2013-09-25:

59 + } else if (internals.loadingImage && (sourceSize != Qt.size(internals.loadingImage.sourceSize.width, internals.loadingImage.sourceSize.height))) {

Hmm... Can it be that internals.loadingImage is null at some point? In that case setting the sourceSize would get lost if setting during that period. If it can't happen we can probably drop this check either. In any case I think the "internals.loadingImage &&" should not be here as the sourceSize gets bound to currentImage and nextImage directly anyways.

No tests?

review: Needs Information

Revision history for this message

Günter Schwann (schwann) wrote on 2013-09-25:

looks good, and helps fixing the issue decscribed in the bug

review: Approve

Revision history for this message

Tim Peeters (tpeeters) wrote on 2013-09-25:

> 59 + } else if (internals.loadingImage && (sourceSize !=
> Qt.size(internals.loadingImage.sourceSize.width,
> internals.loadingImage.sourceSize.height))) {
>
> Hmm... Can it be that internals.loadingImage is null at some point? In that
> case setting the sourceSize would get lost if setting during that period. If
> it can't happen we can probably drop this check either. In any case I think
> the "internals.loadingImage &&" should not be here as the sourceSize gets
> bound to currentImage and nextImage directly anyways.

apparently it can be null/undefined when the object is being created and image pointers are not yet set.

>
> No tests?

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-09-25:

FAILED: Continuous integration, rev:769
http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-ci/754/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-saucy-vm/73/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-touch/1579/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-amd64-ci/611/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-armhf-ci/611/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy-i386/4060/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy-armhf/1581/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-09-25:

FAILED: Continuous integration, rev:770
http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-ci/755/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-saucy-vm/74/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-touch/1588/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-amd64-ci/612/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-armhf-ci/612/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy-i386/4068/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy-armhf/1590/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-09-26:

FAILED: Continuous integration, rev:771
http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-ci/760/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-saucy-vm/81/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-touch/1632/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-amd64-ci/617/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-armhf-ci/617/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy-i386/4115/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy-armhf/1634/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-09-26:

FAILED: Continuous integration, rev:772
http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-ci/764/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-saucy-vm/85/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-touch/1650/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-amd64-ci/621/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-armhf-ci/621/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy-i386/4133/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy-armhf/1652/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Michael Zanetti (mzanetti) wrote on 2013-09-27:

> > 59 + } else if (internals.loadingImage && (sourceSize !=
> > Qt.size(internals.loadingImage.sourceSize.width,
> > internals.loadingImage.sourceSize.height))) {
> >
> > Hmm... Can it be that internals.loadingImage is null at some point? In that
> > case setting the sourceSize would get lost if setting during that period. If
> > it can't happen we can probably drop this check either. In any case I think
> > the "internals.loadingImage &&" should not be here as the sourceSize gets
> > bound to currentImage and nextImage directly anyways.
>
> apparently it can be null/undefined when the object is being created and image
> pointers are not yet set.

Yeah, this is what I meant. So doesn't this fail in that case?

I mean, if you're using it like this:

CrossFadeImage {
   source: "foo.png"
   sourceSize.height: 500
   sourceSize.width: 500
}

Given that the whole thing is built up and sourceSize is set immediately, when loadingImage is still null. The sourceSize setting would just be discarded afaics. Unless the constructions happens before the property setting. In that case the check doesn't break anything, but is still useles..

Revision history for this message

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

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-autolanding/313/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-saucy-vm/182/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-touch/1995/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-amd64-autolanding/239/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-saucy-armhf-autolanding/239/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy-i386/4330/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-saucy-armhf/1997/console

review: Needs Fixing (continuous-integration)

Revision history for this message

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

52 +
53 + onSourceSizeChanged: {

qdoc failure given on this line. Also remember to merge trunk, there were issues with autopilot tests.

review: Needs Fixing

Revision history for this message

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

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/ubuntu-ui-toolkit-ci/904/rebuild

review: Approve (continuous-integration)

Revision history for this message

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

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/ubuntu-ui-toolkit-ci/908/rebuild

review: Approve (continuous-integration)

Revision history for this message

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

> > > 59 + } else if (internals.loadingImage && (sourceSize !=
> > > Qt.size(internals.loadingImage.sourceSize.width,
> > > internals.loadingImage.sourceSize.height))) {
> > >
> > > Hmm... Can it be that internals.loadingImage is null at some point? In
> that
> > > case setting the sourceSize would get lost if setting during that period.
> If
> > > it can't happen we can probably drop this check either. In any case I
> think
> > > the "internals.loadingImage &&" should not be here as the sourceSize gets
> > > bound to currentImage and nextImage directly anyways.
> >
> > apparently it can be null/undefined when the object is being created and
> image
> > pointers are not yet set.
>
> Yeah, this is what I meant. So doesn't this fail in that case?
>
> I mean, if you're using it like this:
>
> CrossFadeImage {
> source: "foo.png"
> sourceSize.height: 500
> sourceSize.width: 500
> }
>
> Given that the whole thing is built up and sourceSize is set immediately, when
> loadingImage is still null. The sourceSize setting would just be discarded
> afaics. Unless the constructions happens before the property setting. In that
> case the check doesn't break anything, but is still useles..

I added a test that checks for explicitly defined source sizes.

If no image is loaded yet, sourceSize is set to 0,0, and setting it implicitly will trigger a changed and will set forcedSourceSize, and as a result the image sourceSize will be ignored in the future. So it all seems good.

Revision history for this message

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

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/ubuntu-ui-toolkit-ci/915/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Francis Ginther (fginther) wrote on 2013-10-09:

maguro device was stuck, re-building.

Revision history for this message

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

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/ubuntu-ui-toolkit-ci/918/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

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

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/ubuntu-ui-toolkit-ci/931/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

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

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/ubuntu-ui-toolkit-ci/937/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

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

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/ubuntu-ui-toolkit-ci/944/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

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

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/ubuntu-ui-toolkit-ci/952/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

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

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/ubuntu-ui-toolkit-ci/970/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

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

let's see if stuff gets merged now.

review: Approve

Revision history for this message

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

just need positive CI results

Revision history for this message

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

review: Needs Fixing (continuous-integration)

Revision history for this message

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

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/ubuntu-ui-toolkit-ci/998/rebuild

review: Approve (continuous-integration)

Revision history for this message

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

approved by jenkins??! woohoo!!! :)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Brian Blevins

Caco V. Topia

Jaroslavas Karmazinas

Tim Peeters

Ubuntu SDK team

to status/vote changes:

william

 === modified file 'CHANGES'
 --- CHANGES	2013-10-02 06:38:07 +0000
 +++ CHANGES	2013-10-14 08:32:19 +0000
@@ -55,6 +55,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-10-03 06:27:08 +0000
 +++ components.api	2013-10-14 08:32:19 +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-14 08:32:19 +0000
@@ -81,8 +81,33 @@
      /*!
        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 +127,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 +160,12 @@
          asynchronous: true
          fillMode: parent.fillMode
          z: 1
++        Binding {
++            target: image1
++            property: "sourceSize"
++            value: internals.forcedSourceSize
++            when: internals.forcedSourceSize !== undefined
++        }
+     }
      Image {
@@ -139,6 +175,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-14 08:32:19 +0000
@@ -35,6 +35,15 @@
          target: crossFadeImage
+     }
++    CrossFadeImage {
++        id: crossFadeImagePreset
++        sourceSize {
++            width: 123
++            height: 321
++        }
++        source: Qt.resolvedUrl("../../../examples/ubuntu-ui-toolkit-gallery/demo_image.jpg")
++    }
++
      function loadImage(url) {
          console.log("Loading image...");
          source = url;
@@ -93,4 +102,21 @@
          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();
++    }
++
++    function test_sourcePreset() {
++        compare(crossFadeImagePreset.sourceSize.width, 123, "Source width incorrectly taken from preset.");
++        compare(crossFadeImagePreset.sourceSize.height, 321, "Source height incorrectly take from preset.");
++    }
+ }

Ubuntu UI Toolkit

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

Commit message

Description of the change

Preview Diff

Subscribers