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

[Albert Astals Cid]

Make more CardTool properties readonly

This removes "hacks" from GenericScopeView and moves them to either CardTool or CardGrid in a less "this is a hack" way

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

 * Did you perform an exploratory manual test run of your code change and any related functionality?
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

[Unity8 CI Bot (unity8-ci-bot)]

FAILED: Continuous integration, rev:2598
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/1992/
Executed test runs:
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build/2611/console
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/2639
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/2518
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial+overlay/2518
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-1-sourcepkg/release=yakkety/2518
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/2512
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/2512/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/2512
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/2512/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=yakkety/2512
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=yakkety/2512/artifact/output/*zip*/output.zip
    FAILURE: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/2512/console
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/2512
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/2512/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=yakkety/2512
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=yakkety/2512/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/2512
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/2512/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/2512
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/2512/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=yakkety/2512
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=yakkety/2512/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/1992/rebuild

review: Needs Fixing (continuous-integration)

[lp:~aacid/unity8/readonly_properties_to_cardtool]

2599. By Albert Astals Cid on 2016-09-27

Merge

[Unity8 CI Bot (unity8-ci-bot)]

PASSED: Continuous integration, rev:2599
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/2257/
Executed test runs:
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build/2971
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=amd64,release=vivid+overlay,testname=qmluitests.sh/1637
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=amd64,release=xenial+overlay,testname=qmluitests.sh/1637
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/test-0-autopkgtest/label=amd64,release=yakkety,testname=qmluitests.sh/1637
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-0-fetch/2999
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/2857
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=vivid+overlay/2857/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/2857
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=xenial+overlay/2857/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=yakkety/2857
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=amd64,release=yakkety/2857/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/2857
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=vivid+overlay/2857/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/2857
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=xenial+overlay/2857/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=yakkety/2857
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=armhf,release=yakkety/2857/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/2857
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=vivid+overlay/2857/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/2857
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=xenial+overlay/2857/artifact/output/*zip*/output.zip
    SUCCESS: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=yakkety/2857
        deb: https://unity8-jenkins.ubuntu.com/job/build-2-binpkg/arch=i386,release=yakkety/2857/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://unity8-jenkins.ubuntu.com/job/lp-unity8-ci/2257/rebuild

review: Approve (continuous-integration)

[Andrea Cimitan (cimi)]

 * Did you perform an exploratory manual test run of the code change and any related functionality?
yes in a silo
 * Did CI run pass? If not, please explain why.
y

review: Approve

=== modified file 'qml/Dash/CardGrid.qml'
--- qml/Dash/CardGrid.qml	2016-05-09 08:34:38 +0000
+++ qml/Dash/CardGrid.qml	2016-09-27 13:36:54 +0000
@@ -20,13 +20,12 @@
 DashRenderer {
     id: root
 
-    readonly property real defaultMinimumHorizontalSpacing: units.gu(1)
     readonly property int collapsedRows: {
         if (!cardTool || !cardTool.template || typeof cardTool.template["collapsed-rows"] != "number") return 2;
         return cardTool.template["collapsed-rows"];
     }
-    property string backgroundShapeStyle: "inset";
-    property alias minimumHorizontalSpacing: grid.minimumHorizontalSpacing
+                                                   // ↓ This is the ubuntu store icon
+    readonly property string backgroundShapeStyle: cardTool.isAppLikeScope && !cardTool.isAppLikeScopeAppCategory ? "shadow" : "inset"
 
     expandedHeight: grid.totalContentHeight
     collapsedHeight: Math.min(grid.contentHeightForRows(collapsedRows, grid.cellHeight), expandedHeight)
@@ -46,7 +45,7 @@
     ResponsiveGridView {
         id: grid
         anchors.fill: parent
-        minimumHorizontalSpacing: root.defaultMinimumHorizontalSpacing
+        minimumHorizontalSpacing: (cardTool.isAppLikeScopeAppCategory && cardTool.isWideView) ? units.gu(5) : units.gu(1)
         delegateWidth: cardTool.cardWidth
         delegateHeight: cardTool.cardHeight
         verticalSpacing: units.gu(1)

=== modified file 'qml/Dash/CardTool.qml'
--- qml/Dash/CardTool.qml	2016-08-23 12:48:23 +0000
+++ qml/Dash/CardTool.qml	2016-09-27 13:36:54 +0000
@@ -42,11 +42,6 @@
     property real viewWidth
 
     /*!
-     \brief Scaling factor of selected Carousel item.
-     */
-    readonly property real carouselSelectedItemScaleFactor: 1.38  // XXX assuming 1.38 carousel scaling factor for cards
-
-    /*!
      \brief Template supplied for the category.
      */
     property var template
@@ -57,9 +52,24 @@
     property var components
 
     /*!
+     \brief The Scope this cardTool is representing
+     */
+    property string scopeId
+
+    /*!
+     \brief The Scope category this cardTool is representing
+     */
+    property string categoryId
+
+    /*!
+     \brief Scaling factor of selected Carousel item.
+     */
+    readonly property real carouselSelectedItemScaleFactor: 1.38  // XXX assuming 1.38 carousel scaling factor for cards
+
+    /*!
      \brief The category layout for this card tool.
      */
-    property string categoryLayout: {
+    readonly property string categoryLayout: {
         var layout = template["category-layout"];
 
         // carousel fallback mode to grid
@@ -67,9 +77,17 @@
         return layout;
     }
 
+    readonly property bool isAppLikeScope: scopeId === "clickscope" || scopeId === "libertine-scope.ubuntu_libertine-scope"
+    readonly property bool isAppLikeScopeAppCategory: ((scopeId === "clickscope" && (categoryId === "predefined" || categoryId === "local"))
+                                                      || (scopeId === "libertine-scope.ubuntu_libertine-scope" && categoryId !== "hint"))
 
-    // Not readonly because gets overwritten from GenericScopeView in some cases
-    property string artShapeStyle: categoryLayout === "carousel" ? "shadow" : "inset"
+    readonly property string artShapeStyle: {
+        if (isAppLikeScope) {
+            return isAppLikeScopeAppCategory ? "icon" : "flat";
+        } else {
+            return categoryLayout === "carousel" ? "shadow" : "inset"
+        }
+    }
 
     // FIXME ? This seems like it should not be needed, but on Qt 5.4 + phone
     // we are doing unneeded calls to getCardComponent with artShapeStyle and categoryLayout being empty
@@ -90,12 +108,31 @@
     height: 0
     clip: true
 
+    // We have 3 view "widths"
+    //   narrow <= 45gu
+    //   normal
+    //   wide >= 70gu
+    readonly property bool isWideView: viewWidth >= units.gu(70)
+    readonly property bool isNarrowView: viewWidth <= units.gu(45)
+
     /*!
      type:real \brief Width to be enforced on the card in this configuration.
 
      If -1, should use implicit width of the actual card.
      */
-    property real cardWidth: {
+    readonly property real cardWidth: {
+        if (isAppLikeScopeAppCategory) {
+            if (!isNarrowView) {
+                if (isWideView) {
+                    return units.gu(11);
+                } else {
+                    return units.gu(10);
+                }
+            } else {
+                return units.gu(12);
+            }
+        }
+
         switch (categoryLayout) {
             case "grid":
             case "vertical-journal":
@@ -103,16 +140,16 @@
                 if (template["card-layout"] === "horizontal") size = "large";
                 switch (size) {
                     case "small": {
-                        if (viewWidth <= units.gu(45)) return units.gu(12);
+                        if (isNarrowView) return units.gu(12);
                         else return units.gu(14);
                     }
                     case "large": {
-                        if (viewWidth >= units.gu(70)) return units.gu(42);
+                        if (isWideView) return units.gu(42);
                         else return viewWidth - units.gu(2);
                     }
                 }
-                if (viewWidth <= units.gu(45)) return units.gu(18);
-                else if (viewWidth >= units.gu(70)) return units.gu(20);
+                if (isNarrowView) return units.gu(18);
+                else if (isWideView) return units.gu(20);
                 else return units.gu(23);
             case "carousel":
             case "horizontal-list":
@@ -152,12 +189,19 @@
      type:real \brief Height of the card's header.
     */
     readonly property int headerHeight: cardLoader.item ? cardLoader.item.headerHeight : 0
-    property size artShapeSize: cardLoader.item ? cardLoader.item.artShapeSize : Qt.size(0, 0)
+
+    readonly property size artShapeSize: {
+        if (isAppLikeScopeAppCategory) {
+            return Qt.size(units.gu(8), units.gu(7.5));
+        } else {
+            return cardLoader.item ? cardLoader.item.artShapeSize : Qt.size(0, 0)
+        }
+    }
 
     QtObject {
         id: carouselTool
 
-        property real minimumTileWidth: {
+        readonly property real minimumTileWidth: {
             if (cardTool.viewWidth === undefined) return undefined;
             if (cardTool.viewWidth <= units.gu(40)) return units.gu(18);
             if (cardTool.viewWidth >= units.gu(128)) return units.gu(26);
@@ -166,7 +210,7 @@
 
         readonly property real pathItemCount: 4.8457 /// (848 / 175) reference values
 
-        property real realPathItemCount: {
+        readonly property real realPathItemCount: {
             var scaledMinimumTileWidth = minimumTileWidth / cardTool.carouselSelectedItemScaleFactor;
             var tileWidth = Math.max(cardTool.viewWidth / pathItemCount, scaledMinimumTileWidth);
             return Math.min(cardTool.viewWidth / tileWidth, pathItemCount);

=== modified file 'qml/Dash/GenericScopeView.qml'
--- qml/Dash/GenericScopeView.qml	2016-09-22 07:37:21 +0000
+++ qml/Dash/GenericScopeView.qml	2016-09-27 13:36:54 +0000
@@ -267,6 +267,8 @@
                 template: model.renderer
                 components: model.components
                 viewWidth: parent.width
+                scopeId: scope ? scope.id : ""
+                categoryId: baseItem.category
             }
 
             onExpandableChanged: {
@@ -342,17 +344,6 @@
                         baseItem.expand(shouldExpand, false /*animate*/);
                     }
                     updateRanges();
-                    clickScopeSizingHacks();
-                    if (scope && (scope.id === "clickscope" || scope.id === "libertine-scope.ubuntu_libertine-scope")) {
-                        if (isLibertineContainerCategory() || categoryId === "predefined" || categoryId === "local") {
-                            cardTool.artShapeSize = Qt.binding(function() { return Qt.size(units.gu(8), units.gu(7.5)) });
-                            cardTool.artShapeStyle = "icon";
-                        } else {
-                            // Should be ubuntu store icon
-                            cardTool.artShapeStyle = "flat";
-                            item.backgroundShapeStyle = "shadow";
-                        }
-                    }
                     item.cardTool = cardTool;
                 }
 
@@ -361,36 +352,6 @@
                         scopeView.enableHeightBehaviorOnNextCreation = item.enableHeightBehaviorOnNextCreation;
                     }
                 }
-                // FIXME: directly connecting to onUnitsChanged cause a compile error:
-                // Cannot assign to non-existent property "onUnitsChanged"
-                // Until the units object is reworked to properly do all we need, let's go through a intermediate property
-                property int pxpgu: units.gu(1);
-                onPxpguChanged: clickScopeSizingHacks();
-
-                // Returns true if the current category pertains to a Libertine container
-                function isLibertineContainerCategory() {
-                  return scope && scope.id === "libertine-scope.ubuntu_libertine-scope" && categoryId !== "hint";
-                }
-
-                function clickScopeSizingHacks() {
-                    if (scope &&
-                        ((scope.id === "clickscope" && (categoryId === "predefined" || categoryId === "local")) ||
-                         isLibertineContainerCategory())) {
-                        // Yeah, hackish :/
-                        if (scopeView.width > units.gu(45)) {
-                            if (scopeView.width >= units.gu(70)) {
-                                cardTool.cardWidth = units.gu(11);
-                                item.minimumHorizontalSpacing = units.gu(5);
-                                return;
-                            } else {
-                                cardTool.cardWidth = units.gu(10);
-                            }
-                        } else {
-                            cardTool.cardWidth = units.gu(12);
-                        }
-                        item.minimumHorizontalSpacing = item.defaultMinimumHorizontalSpacing;
-                    }
-                }
 
                 Connections {
                     target: rendererLoader.item
@@ -425,7 +386,6 @@
                     target: scopeView
                     onIsCurrentChanged: rendererLoader.updateRanges();
                     onVisibleToParentChanged: rendererLoader.updateRanges();
-                    onWidthChanged: rendererLoader.clickScopeSizingHacks();
                 }
                 Connections {
                     target: holdingList