Merge into trunk : headerHeightInit : Code : Ubuntu UI Toolkit

Reviewer	Review Type	Date Requested	Status
Cris Dywan		2013-12-18	Approve on 2014-01-09
PS Jenkins bot	continuous-integration		Approve on 2014-01-09
Francis Ginther (community)			Approve on 2014-01-08
Review via email: mp+199468@code.launchpad.net

Revision history for this message

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

#

FAILED: Continuous integration, rev:899
http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-ci/1479/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/1837/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty-touch/1754/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-trusty-amd64-ci/427/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-trusty-armhf-ci/427/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/1837/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/1754/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

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

#

FAILED: Continuous integration, rev:900
http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-ci/1487/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/1864/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty-touch/1781/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-trusty-amd64-ci/435/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-ui-toolkit-trusty-armhf-ci/435/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/1864/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/1781/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

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

#

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

review: Approve (continuous-integration)

Revision history for this message

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

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Tim Peeters (tpeeters) wrote on 2013-12-20:

#

CI failed because of this bug? https://bugs.launchpad.net/ubuntu/+source/qtubuntu/+bug/1262982

Revision history for this message

Tim Peeters (tpeeters) wrote on 2013-12-20:

#

no breakage on maguro with file manager, calculator, clock, calendar, gallery.

Revision history for this message

Tim Peeters (tpeeters) wrote on 2013-12-20:

#

UITK autopilot tests passed on maguro: https://pastebin.canonical.com/102317/

MR is ready to go.

Revision history for this message

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

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

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

#

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

review: Approve (continuous-integration)

Revision history for this message

Cris Dywan (kalikiana) wrote on 2013-12-23:

#

How about instead of moving the alignment test inside an Item, add a second one, to actually test that both behave the same?

And for the page test flickableY + header.height is expected now instead of just flickableY, I wonder if headerHeight can be checked also here to know the value is right? Since the existing case should not have changed but the test doesn't check.

review: Needs Information

Revision history for this message

Tim Peeters (tpeeters) wrote on 2014-01-06:

#

> How about instead of moving the alignment test inside an Item, add a second
> one, to actually test that both behave the same?

ok, done.

> And for the page test flickableY + header.height is expected now instead of
> just flickableY, I wonder if headerHeight can be checked also here to know the
> value is right? Since the existing case should not have changed but the test
> doesn't check.

Added checks for headerHeight, and clarified in the test why things are happening the way they are.

Revision history for this message

Tim Peeters (tpeeters) wrote on 2014-01-06:

#

Code in tst_page.qml mixes two conventions for having a semicolon at the end of a line. I'm not fixing that here to keep the diff small and focussed on what matters to fix the bug. Reported https://bugs.launchpad.net/ubuntu-ui-toolkit/+bug/1266453

Revision history for this message

Tim Peeters (tpeeters) wrote on 2014-01-06:

#

I didn't test the changes on device yet. Don't happrove until I do that.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-01-06:

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-01-06:

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Cris Dywan (kalikiana) wrote on 2014-01-06:

#

FAIL! : components::DatePickerAPI::test_1_changeDateToNextMonth() Uncaught exception: Cannot read property 'children' of null
Loc: [(0)]

It's failed on that before but I guess it's a race condition - unless somebody switched Qt on the Jenkins over new year's eve.

I appreciate the test clarifications, in general I'm happy for it to go in minus the weird failures.

review: Approve

Revision history for this message

Tim Peeters (tpeeters) wrote on 2014-01-06:

#

I created a bug for the jenkins failure: https://bugs.launchpad.net/ubuntu-ui-toolkit/+bug/1266515

Revision history for this message

Tim Peeters (tpeeters) wrote on 2014-01-07:

#

Tested on device with gallery, file-manager, notes. ubuntuuitoolkit autopilot tests passed.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-01-08:

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-01-08:

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-01-08:

#

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

review: Approve (continuous-integration)

Revision history for this message

Cris Dywan (kalikiana) wrote on 2014-01-08:

#

I can live with that work-around as the value we need is correct.

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-01-08:

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-01-08:

#

review: Needs Fixing (continuous-integration)

Revision history for this message

Francis Ginther (fginther) wrote on 2014-01-08:

#

This can be re-approved, the autolanding failed due to a CI issue.

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-01-08:

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-01-08:

#

review: Needs Fixing (continuous-integration)

lp:~tpeeters/ubuntu-ui-toolkit/headerHeightInit updated on 2014-01-09

919. By Tim Peeters on 2014-01-09: change to test jenkins
920. By Tim Peeters on 2014-01-09: add TODO

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-01-09:

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Tim Peeters (tpeeters) wrote on 2014-01-09:

#

Tests fail because contentY gets an incorrect intermediate value. It is immediately updated to the correct value afterwards, but the test catches the wrong one. It doesn't cause problems in practise but it is not clear to me why the intermediate wrong value is there. Investigating...

lp:~tpeeters/ubuntu-ui-toolkit/headerHeightInit updated on 2014-01-09

921. By Tim Peeters on 2014-01-09: debugging
922. By Tim Peeters on 2014-01-09: remove initializeFlickablePosition. This is done automatically by the ListView
923. By Tim Peeters on 2014-01-09: put test back
924. By Tim Peeters on 2014-01-09: remove debugging code
925. By Tim Peeters on 2014-01-09: remove debugging code

Revision history for this message

Tim Peeters (tpeeters) wrote on 2014-01-09:

#

this is the change that matters and should fix the CI results: http://bazaar.launchpad.net/~tpeeters/ubuntu-ui-toolkit/headerHeightInit/revision/922

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-01-09:

#

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

review: Approve (continuous-integration)

Revision history for this message

Tim Peeters (tpeeters) wrote on 2014-01-09:

#

the diff since last happroval, to make reviewing easier:

=== modified file modules/Ubuntu/Components/Page.qml
--- modules/Ubuntu/Components/Page.qml 2013-12-19 16:22:20 +0000
+++ modules/Ubuntu/Components/Page.qml 2014-01-09 12:00:26 +0000
@@ -192,12 +192,6 @@
}
}

- Connections {
- target: page
- onFlickableChanged: internal.initializeFlickablePosition()
- }
- Component.onCompleted: internal.initializeFlickablePosition()
-
         function isVerticalFlickable(object) {
             if (object && object.hasOwnProperty("flickableDirection") && object.hasOwnProperty("contentHeight")) {
                 var direction = object.flickableDirection;
@@ -226,16 +220,5 @@
             }
             return null;
         }
-
- function initializeFlickablePosition() {
- if (page.flickable) {
- // compensating for header height and setting flickable.topMargin is done
- // automatically by the header when its flickable property is set.
- if (page.flickable.hasOwnProperty("headerItem") && page.flickable.headerItem) {
- // flickable is a ListView with a headerItem
- page.flickable.contentY -= page.flickable.headerItem.height;
- }
- }
- }
     }
}

lp:~tpeeters/ubuntu-ui-toolkit/headerHeightInit updated on 2014-01-09

926. By Tim Peeters on 2014-01-09: merge trunk

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-01-09:

#

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

review: Approve (continuous-integration)

Revision history for this message

Tim Peeters (tpeeters) wrote on 2014-01-09:

#

To clarify why the last change (removing the initializeFlickablePosition()) makes sense:

(MHeader = header of the mainview/page - this shows the current page title or the tabbar)
(LVHeader = the header inside the listview, specified by ListView.header property)

- In previous versions of the code, I used to set the contentY of the flickable to -MHeader.height,
because that ignored LVHeader, I then subtracted LVHeader.height from it.

- In the new version of the code, I don't set the contentY to -MHeader.height, but when MHeader.height is updated (or initialized), I SUBTRACT MHeader.height from contentY.

- It turns out that the listview already updates contentY when an LVHeader is used. In the previous version of our code, this update was overridden by setting the ListView.contentY to -MHeader.height, but now in the new version where we subtract MHeader.height from contentY instead of setting it, our code to accomodate for LVHeader.height in initializeFlickablePosition() is no longer needed. That's already done automatically by the ListView.

Revision history for this message

Cris Dywan (kalikiana) wrote on 2014-01-09:

#

This surprisingly makes sense :-D It also explains the earlier issue with the tests but these can stay, they're nicer now anyway.

review: Approve

Ubuntu UI Toolkit

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

Commit message

Description of the change

Preview Diff

Subscribers

 === modified file 'modules/Ubuntu/Components/Header.qml'
 --- modules/Ubuntu/Components/Header.qml	2013-12-02 18:54:08 +0000
 +++ modules/Ubuntu/Components/Header.qml	2014-01-09 13:31:04 +0000
@@ -49,9 +49,15 @@
+     }
      /*! \internal */
--    onHeightChanged: internal.movementEnded()
++    onHeightChanged: {
++        internal.checkFlickableMargins();
++        internal.movementEnded();
++    }
      visible: title || contents
++    onVisibleChanged: {
++        internal.checkFlickableMargins();
++    }
      /*!
        Show the header
@@ -86,6 +92,7 @@
       */
      property Flickable flickable: null
      onFlickableChanged: {
++        internal.checkFlickableMargins();
          internal.connectFlickable();
          header.show();
+     }
@@ -158,6 +165,23 @@
          function interactiveChanged() {
              if (flickable && !flickable.interactive) header.show();
+         }
++
++        /*
++          Check the topMargin of the flickable and set it if needed to avoid
++          contents becoming unavailable behind the header.
++         */
++        function checkFlickableMargins() {
++            if (header.flickable) {
++                var headerHeight = header.visible ? header.height : 0
++                if (flickable.topMargin !== headerHeight) {
++                    var previousHeaderHeight = flickable.topMargin;
++                    flickable.topMargin = headerHeight;
++                    // push down contents when header grows,
++                    // pull up contents when header shrinks.
++                    flickable.contentY -= headerHeight - previousHeaderHeight;
++                }
++            }
++        }
+     }
      style: Theme.createStyleComponent("HeaderStyle.qml", header)
 === modified file 'modules/Ubuntu/Components/Page.qml'
 --- modules/Ubuntu/Components/Page.qml	2013-11-26 14:19:38 +0000
 +++ modules/Ubuntu/Components/Page.qml	2014-01-09 13:31:04 +0000
@@ -170,6 +170,10 @@
          property Header header: page.__propagated && page.__propagated.header ? page.__propagated.header : null
          property Toolbar toolbar: page.__propagated && page.__propagated.toolbar ? page.__propagated.toolbar : null
++        // Used to position the Page when there is no flickable.
++        // When there is a flickable, the header will automatically position it.
++        property real headerHeight: internal.header && internal.header.visible ? internal.header.height : 0
++
          onHeaderChanged: internal.updateHeaderAndToolbar()
          onToolbarChanged: internal.updateHeaderAndToolbar()
@@ -188,14 +192,6 @@
+             }
+         }
--        Connections {
--            target: page
--            onFlickableChanged: internal.updateFlickablePosition()
--        }
--        Component.onCompleted: internal.updateFlickablePosition()
--
--        property real headerHeight: internal.header && internal.header.visible ? internal.header.height : 0
--
          function isVerticalFlickable(object) {
              if (object && object.hasOwnProperty("flickableDirection") && object.hasOwnProperty("contentHeight")) {
                  var direction = object.flickableDirection;
@@ -224,25 +220,5 @@
+             }
              return null;
+         }
--
--        Binding {
--            target: page.flickable
--            property: "topMargin"
--            value: internal.headerHeight
--            when: page.flickable
--        }
--
--        function updateFlickablePosition() {
--            if (page.flickable) {
--                // Set-up the top-margin of the contents of the Flickable so that
--                //  the contents is never hidden by the header
--                var displacement = headerHeight;
--                if (page.flickable.hasOwnProperty("headerItem") && page.flickable.headerItem) {
--                    // flickable is a ListView with a headerItem
--                    displacement += page.flickable.headerItem.height;
--                }
--                page.flickable.contentY = -displacement;
--            }
--        }
+     }
+ }
 === modified file 'tests/unit/tst_components/tst_page.qml'
 --- tests/unit/tst_components/tst_page.qml	2013-09-03 10:51:40 +0000
 +++ tests/unit/tst_components/tst_page.qml	2014-01-09 13:31:04 +0000
@@ -126,16 +126,21 @@
          function test_flickableY_bug1201452() {
              var pageTitle = "Hello bug!";
              page.title = pageTitle;
--            compare(page.__propagated.header.visible, true, "header is visible when title is set")
--            compare(page.__propagated.header.height > 0, true, "header has a height when title is set")
++            var header = page.__propagated.header;
++            compare(header.visible, true, "header is visible when title is set")
++            compare(header.height > 0, true, "header has a height when title is set")
++            var headerHeight = header.height
              var flickableY = 150;
              page.flickable.contentY = flickableY;
              compare(page.flickable.contentY, flickableY, "flickable.contentY can be set");
++            compare(page.flickable.topMargin, headerHeight, "topMargin of the flickable equals header height");
              page.title = "";
--            compare(page.__propagated.header.visible, false, "header is hidden when title is unset")
--            compare(page.flickable.contentY, flickableY, "Making header invisible does not reset flickable.contentY");
++            compare(header.visible, false, "header is hidden when title is unset")
++            compare(page.flickable.topMargin, 0, "topMargin becomes 0 because header is hidden"); // used to be headerHeight
++            compare(page.flickable.contentY, flickableY + headerHeight, "contentY is updated when header is made invisible to compensate for the change in topMargin");
              page.title = pageTitle;
              compare(page.flickable.contentY, flickableY, "Making header visible again does not reset flickable.contentY");
++            compare(page.flickable.topMargin, headerHeight, "topMargin is updated when header becomes visible.")
+         }
+     }
+ }
 === modified file 'tests/unit_x11/tst_components/tst_header.qml'
 --- tests/unit_x11/tst_components/tst_header.qml	2013-11-11 11:15:53 +0000
 +++ tests/unit_x11/tst_components/tst_header.qml	2014-01-09 13:31:04 +0000
@@ -22,11 +22,15 @@
      width: units.gu(50)
      height: units.gu(80)
++    id: root
++    property real listViewHeaderHeight: units.gu(5)
++
      MainView {
          id: mainViewHeader
          anchors.fill: parent
          Page {
++            id: page
              title: "listview"
              ListView {
@@ -36,7 +40,7 @@
                  header: Rectangle {
                      color: "red"
                      width: parent.width
--                    height: units.gu(5)
++                    height: root.listViewHeaderHeight
+                 }
                  model: 500
                  delegate: Label {
@@ -44,6 +48,33 @@
+                 }
+             }
+         }
++
++        Item {
++            // Wrapping the Page inside this Item should not
++            // affect the header alignment, see bug #1261907.
++            anchors.fill: parent
++            id: wrappingItem
++
++            Page {
++                id: wrappedPage
++                title: "listview"
++
++                ListView {
++                    anchors.fill: parent
++                    id: wrappedListView
++
++                    header: Rectangle {
++                        color: "red"
++                        width: parent.width
++                        height: root.listViewHeaderHeight
++                    }
++                    model: 500
++                    delegate: Label {
++                        text: "number " +index
++                    }
++                }
++            }
++        }
+     }
      TestCase {
@@ -51,8 +82,13 @@
          when: windowShown
          function test_ListViewHeaderAlignment_bug1202277() {
--            compare(listView.contentY, -listView.headerItem.height - mainViewHeader.__propagated.header.height,
++            compare(listView.contentY, -root.listViewHeaderHeight - mainViewHeader.__propagated.header.height,
                      "ListView header is aligned with the MainView header");
+         }
++
++        function test_WrappedListViewHeaderAlignment_bug1261907() {
++            compare(wrappedListView.contentY, -root.listViewHeaderHeight - mainViewHeader.__propagated.header.height,
++                    "ListView header inside wrapped Page is aligned with the MainView header");
++        }
+     }
+ }