Unity 8

Merge lp:~aacid/unity8/filtergrid_bindingloop into lp:unity8

filtergrid_bindingloop
Merge into trunk

Proposed by Albert Astals Cid on 2014-04-16

Status:	Merged
Approved by:	Andrea Cimitan on 2014-04-24
Approved revision:	837
Merged at revision:	862
Proposed branch:	lp:~aacid/unity8/filtergrid_bindingloop
Merge into:	lp:unity8
Prerequisite:	lp:~aacid/unity8/card_optimizations
Diff against target:	253 lines (+53/-50) 7 files modified qml/Components/FilterGrid.qml (+38/-25) qml/Dash/CardFilterGrid.qml (+3/-8) qml/Dash/DashRenderer.qml (+3/-3) qml/Dash/GenericScopeView.qml (+4/-9) tests/autopilot/unity8/shell/tests/test_emulators.py (+1/-1) tests/qmltests/Components/tst_FilterGrid.qml (+1/-1) tests/qmltests/Dash/tst_GenericScopeView.qml (+3/-3)
To merge this branch:	bzr merge lp:~aacid/unity8/filtergrid_bindingloop
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Andrea Cimitan (community)			Approve on 2014-04-24
Michał Sawicz		2014-04-16	Needs Information on 2014-04-18
PS Jenkins bot (community)	continuous-integration		Needs Fixing on 2014-04-17
Review via email: mp+216147@code.launchpad.net

This proposal supersedes a proposal from 2014-04-16.

Commit message

Fix binding loop in FilterGrid height

Description of the change

* Are there any related MPs required for this MP to build/function as expected?
prerequisite

* Did you perform an exploratory manual test run of your code change and any related functionality?
Yes

* Did you make sure that your branch does not contain spurious tags?
Yes

* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
N/A

* If you changed the UI, has there been a design review?
N/A

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-16:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/2839/rebuild

review: Needs Fixing (continuous-integration)

lp:~aacid/unity8/filtergrid_bindingloop updated on 2014-04-17

836. By Albert Astals Cid on 2014-04-17: property rename
837. By Albert Astals Cid on 2014-04-17: Merge

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-17:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/2841/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-17:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/2845/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Michał Sawicz (saviq) wrote on 2014-04-18:

I wonder... didn't we talk that we could actually get rid of FilterGrid completely and rely on the view's delegate management by simply changing its height?

review: Needs Information

Revision history for this message

Albert Astals Cid (aacid) wrote on 2014-04-22:

Yeah i remember talking about that and yes, it should be doable, but this is a much minor change (even if the diff seems big it's just changing one thing and the small chain of changes it carries).

If you want I can give it a go at implementing the same behaviour without using the FilterGrid but I doubt it's going to give us much besides killing FilterGrid itself.

Revision history for this message

Michał Sawicz (saviq) wrote on 2014-04-22:

Right. Yeah, I never expected this to do any more than to kill FilterGrid, but that'd be a good thing in itself :).

Let's get this in, and later we'll look into getting rid of it.

Revision history for this message

Andrea Cimitan (cimi) wrote on 2014-04-24:

I wanted to get rid of the behaviour but I cannot think of anything smarter than this at the moment :)

* Did you perform an exploratory manual test run of the code change and any related functionality?
Yes
* Did CI run pass? If not, please explain why.
Slow build... passed 60 mins

Revision history for this message

Andrea Cimitan (cimi) on 2014-04-24:

review: Approve

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:

Albert Astals Cid

Mohammad Rakibul Hasan

Unity Team

 === modified file 'qml/Components/FilterGrid.qml'
 --- qml/Components/FilterGrid.qml	2014-04-08 13:32:59 +0000
 +++ qml/Components/FilterGrid.qml	2014-04-17 08:17:36 +0000
@@ -28,11 +28,6 @@
  Item {
      id: root
--    /* If true, the number of elements displayed will be limited by collapsedRowCount.
--       If false, all elements will be displayed, effectively looking the same as a regular
--       ResponsiveGridView. */
--    property bool filter: true
--
      /* Whether, when collapsed, a button should be displayed enabling the user to expand
         the grid to its full size. */
      readonly property bool expandable: model.count > rowsWhenCollapsed * iconTileGrid.columns
@@ -65,30 +60,48 @@
      readonly property alias pressDelay: iconTileGrid.pressDelay
      property alias highlightIndex: iconTileGrid.highlightIndex
      readonly property alias currentItem: iconTileGrid.currentItem
--
--    height: !filterAnimation.running ? childrenRect.height : height
++    readonly property alias filtered: d.filter
++
++    QtObject {
++        id: d
++        // We do have filter and collapsed properties because we need to decouple
++        // the real filtering with the animation since the closing animation
++        // i.e. setFilter(false. true) we still need to not be filtering until
++        // the animation finishes otherwise we hide the items when the animation
++        // is still running
++        property bool filter: true
++        property bool collapsed: true
++    }
++
++    height: d.collapsed ? root.collapsedHeight : root.uncollapsedHeight
      clip: filterAnimation.running
--    NumberAnimation {
--        property bool filterEndValue
--        id: filterAnimation
--        target: root
--        property: "height"
--        to: filterEndValue ? root.collapsedHeight : root.uncollapsedHeight
--        // Duration and easing here match the ListViewWithPageHeader::m_contentYAnimation
--        // otherwise since both animations can run at the same time you'll get
--        // some visual weirdness.
--        duration: 200
--        easing.type: Easing.InOutQuad
--        onStopped: {
--            root.filter = filterEndValue;
++    Behavior on height {
++        id: heightBehaviour
++        enabled: false
++        NumberAnimation {
++            id: filterAnimation
++            // Duration and easing here match the ListViewWithPageHeader::m_contentYAnimation
++            // otherwise since both animations can run at the same time you'll get
++            // some visual weirdness.
++            duration: 200
++            easing.type: Easing.InOutQuad
++            onRunningChanged: {
++                if (!running) {
++                    d.filter = d.collapsed;
++                }
++                heightBehaviour.enabled = false;
++            }
+         }
+     }
--    function startFilterAnimation(filter) {
--        filterAnimation.filterEndValue = filter
--        filterAnimation.start();
--    }
++    function setFilter(filter, animate) {
++        heightBehaviour.enabled = animate;
++        d.collapsed = filter;
++        if (!animate || !filter) {
++            d.filter = filter;
++        }
++   }
      ResponsiveGridView {
          id: iconTileGrid
@@ -105,7 +118,7 @@
          model: LimitProxyModel {
              model: root.model
--            limit: (filter && !filterAnimation.running) ? rowsWhenCollapsed * iconTileGrid.columns : -1
++            limit: d.filter ? rowsWhenCollapsed * iconTileGrid.columns : -1
+         }
+     }
+ }
 === modified file 'qml/Dash/CardFilterGrid.qml'
 --- qml/Dash/CardFilterGrid.qml	2014-04-16 08:29:28 +0000
 +++ qml/Dash/CardFilterGrid.qml	2014-04-17 08:17:36 +0000
@@ -28,9 +28,10 @@
      verticalSpacing: units.gu(1)
      currentItem: filterGrid.currentItem
      height: filterGrid.height
++    filtered: filterGrid.filtered
--    function startFilterAnimation(filter) {
--        filterGrid.startFilterAnimation(filter)
++    function setFilter(filter, animate) {
++        filterGrid.setFilter(filter, animate)
+     }
      FilterGrid {
@@ -41,7 +42,6 @@
          delegateHeight: cardTool.cardHeight
          verticalSpacing: genericFilterGrid.verticalSpacing
          model: genericFilterGrid.model
--        filter: genericFilterGrid.filter
          collapsedRowCount: Math.min(2, cardTool && cardTool.template && cardTool.template["collapsed-rows"] || 2)
          delegateCreationBegin: genericFilterGrid.delegateCreationBegin
          delegateCreationEnd: genericFilterGrid.delegateCreationEnd
@@ -66,10 +66,5 @@
                  onPressAndHold: genericFilterGrid.pressAndHold(index, card.y)
+             }
+         }
--
--        onFilterChanged: {
--            genericFilterGrid.filter = filter
--            filter = Qt.binding(function() { return genericFilterGrid.filter })
--        }
+     }
+ }
 === modified file 'qml/Dash/DashRenderer.qml'
 --- qml/Dash/DashRenderer.qml	2014-04-08 13:32:59 +0000
 +++ qml/Dash/DashRenderer.qml	2014-04-17 08:17:36 +0000
@@ -20,8 +20,8 @@
      // Can the item be expanded?
      property bool expandable: false
--    // In case it can be expanded, should we filter it
--    property bool filter: true
++    // In case it can be expanded, is it filtered
++    property bool filtered: true
      property int collapsedHeight: height
@@ -58,6 +58,6 @@
      /// @param itemY is y of the held delegate
      signal pressAndHold(int index, real itemY)
--    function startFilterAnimation(filter) {
++    function setFilter(filter, animate) {
+     }
+ }
 === modified file 'qml/Dash/GenericScopeView.qml'
 --- qml/Dash/GenericScopeView.qml	2014-04-16 13:44:46 +0000
 +++ qml/Dash/GenericScopeView.qml	2014-04-17 08:17:36 +0000
@@ -122,7 +122,7 @@
              showDivider: false
              readonly property bool expandable: rendererLoader.item ? rendererLoader.item.expandable : false
--            readonly property bool filtered: rendererLoader.item ? rendererLoader.item.filter : true
++            readonly property bool filtered: rendererLoader.item ? rendererLoader.item.filtered : true
              readonly property string category: categoryId
              readonly property var item: rendererLoader.item
@@ -168,9 +168,7 @@
                      item.objectName = Qt.binding(function() { return categoryId })
                      if (item.expandable) {
                          var shouldFilter = categoryId != categoryView.expandedCategoryId;
--                        if (shouldFilter != item.filter) {
--                            item.filter = shouldFilter;
--                        }
++                        item.setFilter(shouldFilter, false /*animate*/);
+                     }
                      updateDelegateCreationRange();
                      item.cardTool = cardTool;
@@ -219,11 +217,8 @@
                                  var shrinkingVisible = shouldFilter && y + item.collapsedHeight < categoryView.height;
                                  var growingVisible = !shouldFilter && y + height < categoryView.height;
                                  if (!previewListView.open || !shouldFilter) {
--                                    if (shrinkingVisible || growingVisible) {
--                                        item.startFilterAnimation(shouldFilter)
--                                    } else {
--                                        item.filter = shouldFilter;
--                                    }
++                                    var animate = shrinkingVisible || growingVisible;
++                                    item.setFilter(shouldFilter, animate)
                                      if (!shouldFilter && !previewListView.open) {
                                          categoryView.maximizeVisibleArea(index, item.uncollapsedHeight);
+                                     }
 === modified file 'tests/autopilot/unity8/shell/tests/test_emulators.py'
 --- tests/autopilot/unity8/shell/tests/test_emulators.py	2014-04-16 13:44:46 +0000
 +++ tests/autopilot/unity8/shell/tests/test_emulators.py	2014-04-17 08:17:36 +0000
@@ -196,7 +196,7 @@
              category)
          grid = category_element.select_single('CardFilterGrid')
          filtergrid = grid.select_single('FilterGrid')
--        if (grid.filter):
++        if (grid.filtered):
              return filtergrid.collapsedRowCount * filtergrid.columns
          else:
              return filtergrid.uncollapsedRowCount * filtergrid.columns
 === modified file 'tests/qmltests/Components/tst_FilterGrid.qml'
 --- tests/qmltests/Components/tst_FilterGrid.qml	2014-01-29 13:37:23 +0000
 +++ tests/qmltests/Components/tst_FilterGrid.qml	2014-04-17 08:17:36 +0000
@@ -50,6 +50,7 @@
              CheckBox {
                  id: filterCheckBox
                  checked: true
++                onCheckedChanged: filterGrid.setFilter(checked, false /*animate*/)
+             }
+         }
+     }
@@ -99,7 +100,6 @@
              maximumNumberOfColumns: 3
              collapsedRowCount:
                  collapsedRowCountSelector.values[collapsedRowCountSelector.selectedIndex]
--            filter: filterCheckBox.checked
              minimumHorizontalSpacing: units.gu(1)
              delegateWidth: units.gu(6)
              delegateHeight: units.gu(6)
 === modified file 'tests/qmltests/Dash/tst_GenericScopeView.qml'
 --- tests/qmltests/Dash/tst_GenericScopeView.qml	2014-04-14 15:49:20 +0000
 +++ tests/qmltests/Dash/tst_GenericScopeView.qml	2014-04-17 08:17:36 +0000
@@ -140,7 +140,7 @@
                  mouseClick(header, header.width / 2, header.height / 2);
                  tryCompareFunction(function() { middleHeight = category.height; return category.height > initialHeight; }, true);
                  tryCompare(category, "filtered", false);
--                verify(category.height > middleHeight);
++                tryCompareFunction(function() { return category.height > middleHeight; }, true);
                  mouseClick(header, header.width / 2, header.height / 2);
                  verify(category.expandable);
@@ -165,7 +165,7 @@
                  mouseClick(header2, header2.width / 2, header2.height / 2);
                  tryCompare(category2, "filtered", false);
--                tryCompare(category2FilterGrid, "filter", false);
++                tryCompare(category2FilterGrid, "filtered", false);
                  categoryListView.positionAtBeginning();
@@ -182,7 +182,7 @@
                  mouseClick(header0, header0.width / 2, header0.height / 2);
                  tryCompare(category0, "filtered", false);
                  tryCompare(category2, "filtered", true);
--                tryCompare(category2FilterGrid, "filter", true);
++                tryCompare(category2FilterGrid, "filtered", true);
                  mouseClick(header0, header0.width / 2, header0.height / 2);
                  tryCompare(category0, "filtered", true);
                  tryCompare(category2, "filtered", true);