Merge lp:~ajalkane/ubuntu-filemanager-app/scroll-places-pullover into lp:ubuntu-filemanager-app

[Arto Jalkanen]

Bug #1407927: Fixed places pullover to be scrollable.

Bug #1407927: Fixed places pullover to be scrollable.

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:361
http://91.189.93.70:8080/job/ubuntu-filemanager-app-ci/447/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-vivid/561
        deb: http://91.189.93.70:8080/job/generic-mediumtests-vivid/561/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-filemanager-app-vivid-amd64-ci/69

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-filemanager-app-ci/447/rebuild

review: Approve (continuous-integration)

[Niklas Wenzel (nikwen)]

Thank you very much, Arto!

I gave your fix a go and it also seemed to work even without the changes to the Repeater. Is there any specific reason why they are needed? I'm inclined to believe that the Column already changes its height accordingly...

review: Needs Information

[Arto Jalkanen (ajalkane)]

The changes to repeater are needed - repeater does not have intrinsic height or width as it can't know how its items are laid. So we have to provide them to it.

WHy it seemed to work for you is because you didn't test all the cases. Resize the window small enough so that you can scroll it. You'll notice after scrolling the view always returns to start - because Repeater doesn't have height.

[Niklas Wenzel (nikwen)]

Well, would you mind giving it a go without the changes to the Repeater? It's working perfectly for me without them.

The reason for this is that the child items created by the Repeater are added to the Column, which means that the height of the Column changes accordingly. Therefore, you do not need to set the Repeater's height.
If you still don't believe me, add the following line to the Flickable:

onContentHeightChanged: console.log(contentHeight)

You'll see that even without the changes to the Repeater the log output looks like this:

qml: 0
qml: 48
qml: 96
qml: 144
qml: 192
qml: 240
[…]

That's the beauty of a Repeater inside a Column. ;)

review: Needs Fixing

[Niklas Wenzel (nikwen)]

Here's the respective passage from the docs:

"Items instantiated by the Repeater are inserted, in order, as children of the Repeater's parent."

(http://developer.ubuntu.com/api/qml/sdk-14.10/QtQuick.Repeater)

[Arto Jalkanen (ajalkane)]

Hi,

I don't of course mind trying again and the documents you quote would indicate it should work as you said. I did originally try without those changes in Repeater, but it's of course possible I did some other needed changes afterwards.

I'll report back or push an update.

Thanks for the detailed look into this Niklas!

[lp:~ajalkane/ubuntu-filemanager-app/scroll-places-pullover]

362. By Arto Jalkanen on 2015-01-06

Removed unnecessary code.

[Arto Jalkanen (ajalkane)]

You were totally right Niklas.

I removed the code that's not needed. Thank you!

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

PASSED: Continuous integration, rev:362
http://91.189.93.70:8080/job/ubuntu-filemanager-app-ci/448/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/generic-mediumtests-vivid/566
        deb: http://91.189.93.70:8080/job/generic-mediumtests-vivid/566/artifact/work/output/*zip*/output.zip
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-filemanager-app-vivid-amd64-ci/70

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-filemanager-app-ci/448/rebuild

review: Approve (continuous-integration)

[Niklas Wenzel (nikwen)]

Thank you for looking into it again, Arto. The MP looks fine now. Let's get it merged! :)

review: Approve

=== modified file 'src/app/qml/ui/PlacesPage.qml'
--- src/app/qml/ui/PlacesPage.qml	2015-01-01 20:43:56 +0000
+++ src/app/qml/ui/PlacesPage.qml	2015-01-06 19:56:47 +0000
@@ -32,8 +32,10 @@
     Flickable {
         objectName: "placesFlickable"
         anchors.fill: parent
+        contentHeight: content.height
 
         Column {
+            id: content
             anchors {
                 left: parent.left
                 right: parent.right