Merge lp:~martin-borho/ubuntu-weather-app/reboot-nav-and-snap-scrolling into lp:ubuntu-weather-app

[Martin Borho]

Implemented horizontal scrolling through loctions and a functional bottom edge.

Implemented horizontal scrolling through locations and a functional bottom edge.

But there are some caveats:

- newly added ListView for horizontal scrolling cannot be used with "snapMode: ListView.SnapOneItem", because the "currentIndex" property is getting ignored when doing so (Bug!?) I have implemented a non-snap-mode solution the, until problem with snapMode gets sorted out.

- jumping through the bottom edge to a location does't work reliable all the time. Some things are broken somehow (Qt/SDK?). Log output:

qml: WARNING! Do not put Page/Tabs/PageStack inside another Page because that causes confusion which is the active page that sets the title and actions.
file:///usr/lib/x86_64-linux-gnu/qt5/qml/Ubuntu/Components/PageWrapperUtils.js:30: TypeError: Cannot read property 'createObject' of null
qml: WARNING! Do not put Page/Tabs/PageStack inside another Page because that causes confusion which is the active page that sets the title and actions.
qml: WARNING! Do not put Page/Tabs/PageStack inside another Page because that causes confusion which is the active page that sets the title and actions.

I think it's good to go for the moment, shortcomings above shouldn't stop us from moving forward.

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

PASSED: Continuous integration, rev:13
http://91.189.93.70:8080/job/ubuntu-weather-app-reboot-ci/26/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-weather-app-reboot-vivid-amd64-ci/26

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-weather-app-reboot-ci/26/rebuild

review: Approve (continuous-integration)

[Andrew Hayzen (ahayzen)]

- newly added ListView for horizontal scrolling cannot be used with "snapMode: ListView.SnapOneItem", because the "currentIndex" property is getting ignored when doing so (Bug!?) I have implemented a non-snap-mode solution the, until problem with snapMode gets sorted out.

This sounds strange I'll investigate to see if i can figure out what is going on.

- app won't start on device, claiming "LocationPane is no type". But works on desktop!

The component is probably missing in the cmake and not being included in the .click? or an import is failing somewhere.

- jumping through the bottom edge to a location does't work reliable all the time. Some things are broken somehow (Qt/SDK?). Log output:

Most of those warnings you can safely ignore, but it should be able to work all of the time, i'll double check if there isn't something causing it to break.

1 inline comment:
- I wonder if we can change this to mainPageStack.push("ui/HomePage.qml") instead of using the component?
- I have some massive OCD over formatting, could you ensure that all of the properties have a space between the : and the value?

review: Needs Fixing

[lp:~martin-borho/ubuntu-weather-app/reboot-nav-and-snap-scrolling]

14. By Martin Borho on 2015-02-15

added current property to settings

[Andrew Hayzen (ahayzen)]

Thanks for adding the settings as discussed :)

One further thing, the Rectangle in LocationsPane L24 could probably be a Item as there is no colour set? This will help with performance.

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

PASSED: Continuous integration, rev:14
http://91.189.93.70:8080/job/ubuntu-weather-app-reboot-ci/27/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-weather-app-reboot-vivid-amd64-ci/27

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-weather-app-reboot-ci/27/rebuild

review: Approve (continuous-integration)

[Andrew Hayzen (ahayzen)]

I've also found that the currentIndex can get out of sync

1) Start the app with two locations
2) Use the bottom edge to switch to the second location
3) Restart the app, notice that it starts at the first index and not the second
4) Now try and use the bottom edge to switch to the second location, note nothing happens

It appears from the app log that as the location list is reset and loaded the currentIndex is lost.

[lp:~martin-borho/ubuntu-weather-app/reboot-nav-and-snap-scrolling]

15. By Martin Borho on 2015-02-15

removed highlightMoveDuration

16. By Martin Borho on 2015-02-15

currentIndex gets set from settings.current

17. By Martin Borho on 2015-02-15

formatting

18. By Martin Borho on 2015-02-15

pushing HomePage.qml directly to pagestack

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

PASSED: Continuous integration, rev:18
http://91.189.93.70:8080/job/ubuntu-weather-app-reboot-ci/28/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-weather-app-reboot-vivid-amd64-ci/28

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-weather-app-reboot-ci/28/rebuild

review: Approve (continuous-integration)

[Andrew Hayzen (ahayzen)]

LGTM, thanks for the extra changes :)

@Victor can you double check there isn't anything else :)

review: Approve

[Victor Thompson (vthompson)]

The code looks good, maybe we can do something like the following to workaround the snapMode issue?

=== modified file 'app/ui/HomePage.qml'
--- app/ui/HomePage.qml 2015-02-15 15:44:33 +0000
+++ app/ui/HomePage.qml 2015-02-15 18:22:31 +0000
@@ -88,6 +88,13 @@

             property bool loaded: false

+ // TODO: workaround for not being able to use snapMode property
+ Component.onCompleted: {
+ var scaleFactor = units.gridUnit * 10;
+ maximumFlickVelocity = maximumFlickVelocity * scaleFactor;
+ flickDeceleration = flickDeceleration * scaleFactor;
+ }
+

[lp:~martin-borho/ubuntu-weather-app/reboot-nav-and-snap-scrolling]

19. By Martin Borho on 2015-02-15

added workaround by victor for snapMode bug

[Martin Borho (martin-borho)]

@vthompson workaround added!

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

PASSED: Continuous integration, rev:19
http://91.189.93.70:8080/job/ubuntu-weather-app-reboot-ci/29/
Executed test runs:
    SUCCESS: http://91.189.93.70:8080/job/ubuntu-weather-app-reboot-vivid-amd64-ci/29

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-weather-app-reboot-ci/29/rebuild

review: Approve (continuous-integration)

[Victor Thompson (vthompson)]

Thanks! lgtm, thanks Martin!

review: Approve

=== modified file 'app/ubuntu-weather-app.qml'
--- app/ubuntu-weather-app.qml	2015-02-10 14:09:17 +0000
+++ app/ubuntu-weather-app.qml	2015-02-15 19:22:09 +0000
@@ -45,7 +45,7 @@
     /*
       List of locations and their data, accessible through index
     */
-    property var locationsList: []
+    property var locationsList: []    
 
     /*
       Index of Location before a refresh, to go back after
@@ -81,9 +81,8 @@
 
     /* Fill the location pages with their data. */
     function fillPages(locations) {
+        locationsList = []
         locationsList = locations;
-        // refactor this when Location are in a ListView!
-        homePage.renderData();
     }
 
     /*
@@ -98,9 +97,9 @@
                 WeatherApi.sendRequest({
                     action: "updateData",
                     params: {
-                        locations:locations,
-                        force:force_refresh,
-                        service:settings.service,
+                        locations: locations,
+                        force: force_refresh,
+                        service: settings.service,
                         api_key: Key.twcKey,
                         interval: settings.refreshInterval
                     }
@@ -112,6 +111,10 @@
     Settings {
         id: settings
         category: "weatherSettings"
+        /*
+          Index of the current locationList of locations and their data, accessible through index
+        */
+        property int current: 0
 
         property int refreshInterval: 1800
         property string precipUnits
@@ -142,11 +145,6 @@
 
     PageStack {
         id: mainPageStack
-
-        HomePage {
-            id: homePage
-        }
-
-        Component.onCompleted: mainPageStack.push(homePage)
+        Component.onCompleted: mainPageStack.push(Qt.resolvedUrl("ui/HomePage.qml"))
     }
 }

=== modified file 'app/ui/HomePage.qml'
--- app/ui/HomePage.qml	2015-02-11 04:14:16 +0000
+++ app/ui/HomePage.qml	2015-02-15 19:22:09 +0000
@@ -18,7 +18,6 @@
 
 import QtQuick 2.3
 import Ubuntu.Components 1.1
-import Ubuntu.Components.ListItems 0.1 as ListItem
 import "../components"
 
 
@@ -32,16 +31,6 @@
     tipColor: UbuntuColors.orange
     tipLabelColor: "#FFF"
 
-    /*
-      Data properties
-    */
-    property string name
-    property string conditionText
-    property string currentTemp
-    property string todayMaxTemp
-    property string todayMinTemp
-    property string iconName
-
     // TODO map iconnames to source image names
     property var iconMap: {
         "moon": "moon.svg"
@@ -57,89 +46,60 @@
     }
 
     /*
-      Extracts values from the location weather data and puts them into the appropriate components
-      to display them.
-
-      Attention: Data access happens through "weatherApp.locationList[]" by index, since complex
-      data in models will lead to type problems.
+      Flickable to scroll the location vertically.
+      The respective contentHeight gets calculated after the Location is filled with data.
     */
-    function renderData() {
-        var index = Math.floor(Math.random()*weatherApp.locationsList.length), // TODO get this value from ListView.currentIndex later!
-            data = weatherApp.locationsList[index],
-            current = data.data[0].current,
-            forecasts = data.data,
-            forecastsLength = forecasts.length,
-            today = forecasts[0];
-
-        var tempUnits = settings.tempScale === "°C" ? "metric" : "imperial"
-
-        // set general location data
-        name = data.location.name;
-
-        // set current temps and condition
-        iconName = (current.icon) ? current.icon : ""
-        conditionText = (current.condition.main) ? current.condition.main : current.condition; // difference TWC/OWM
-        todayMaxTemp = (today[tempUnits].tempMax !== undefined) ? Math.round(today[tempUnits].tempMax).toString() + settings.tempScale: "";
-        todayMinTemp = Math.round(today[tempUnits].tempMin).toString() + settings.tempScale;
-        currentTemp = Math.round(current[tempUnits].temp).toString() + String("°");
-
-        // reset days list
-        mainPageWeekdayListView.model.clear()
-
-        // set daily forecasts
-        if(forecastsLength > 0) {
-            for(var x=1;x<forecastsLength;x++) {
-                // print(JSON.stringify(forecasts[x][units]))
-                var dayData = {
-                    day: formatTimestamp(forecasts[x].date, 'dddd'),
-                    low: Math.round(forecasts[x][tempUnits].tempMin).toString() + settings.tempScale,
-                    high: (forecasts[x][tempUnits].tempMax !== undefined) ? Math.round(forecasts[x][tempUnits].tempMax).toString() + settings.tempScale : "",
-                    image: (forecasts[x].icon !== undefined && iconMap[forecasts[x].icon] !== undefined) ? iconMap[forecasts[x].icon] + settings.tempScale : ""
-                }
-                mainPageWeekdayListView.model.append(dayData);
-            }
-        }
-    }
-
-    ListView {
-        id: mainPageWeekdayListView
-        anchors {
-            fill: parent
-            margins: units.gu(2)
-        }
-
-        header: Column {
-            anchors {
-                left: parent.left
-                right: parent.right
-            }
-            spacing: units.gu(1)
-
-            HeaderRow {
-                locationName: locationPage.name
-            }
-
-            HomeGraphic {
-                icon: locationPage.iconName
-            }
-
-            HomeTempInfo {
-                description: locationPage.conditionText
-                high: locationPage.todayMaxTemp
-                low: locationPage.todayMinTemp
-                now: locationPage.currentTemp
-            }
-
-            ListItem.ThinDivider {
-
-            }
-        }
-        model: ListModel {}
-
-        delegate: DayDelegate {
-            day: model.day
-            high: model.high
-            low: model.low
+    Flickable {
+        id: locationFlickable
+        width: parent.width
+        height: parent.height
+        contentWidth: parent.width
+
+        /*
+          ListView for locations with snap-scrolling horizontally.
+        */
+        ListView {
+            id: locationPages
+            anchors.fill: parent
+            width: parent.width
+            height: childrenRect.height
+            contentWidth: parent.width
+            contentHeight: childrenRect.height
+            model: weatherApp.locationsList.length
+            // TODO with snapMode, currentIndex is not properly set and setting currentIndex fails
+            //snapMode: ListView.SnapOneItem
+            orientation: ListView.Horizontal
+            currentIndex: settings.current
+            highlightRangeMode: ListView.StrictlyEnforceRange
+            onCurrentIndexChanged: {
+                print("CI: "+currentIndex)
+                if (loaded) {
+                    settings.current = currentIndex
+                }
+            }
+            onModelChanged: {
+                currentIndex = settings.current
+
+                if (model > 0) {
+                    loaded = true
+                }
+            }
+            delegate: LocationPane {}
+
+            property bool loaded: false
+            // TODO: workaround for not being able to use snapMode property
+            Component.onCompleted: {
+                var scaleFactor = units.gridUnit * 10;
+                maximumFlickVelocity = maximumFlickVelocity * scaleFactor;
+                flickDeceleration = flickDeceleration * scaleFactor;
+            }
+
+            Connections {
+                target: settings
+                onCurrentChanged: {
+                    locationPages.currentIndex = settings.current
+                }
+            }
         }
     }
 }

=== added file 'app/ui/LocationPane.qml'
--- app/ui/LocationPane.qml	1970-01-01 00:00:00 +0000
+++ app/ui/LocationPane.qml	2015-02-15 19:22:09 +0000
@@ -0,0 +1,145 @@
+/*
+ * Copyright (C) 2015 Canonical Ltd
+ *
+ * This file is part of Ubuntu Weather App
+ *
+ * Ubuntu Weather App is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * Ubuntu Weather App is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+import QtQuick 2.3
+import Ubuntu.Components 1.1
+import Ubuntu.Components.ListItems 0.1 as ListItem
+import "../components"
+
+Rectangle {
+    id: locationItem
+    /*
+      Data properties
+    */
+    property string name
+    property string conditionText
+    property string currentTemp
+    property string todayMaxTemp
+    property string todayMinTemp
+    property string iconName
+
+    Component.onCompleted: renderData(index)
+
+    width: locationPage.width
+    height: childrenRect.height
+    anchors.fill: parent.fill
+
+    /*
+      Calculates the height of all location data components, to set the Flickable.contentHeight right.
+    */
+    function setFlickableContentHeight() {
+        var contentHeightGu = (homeTempInfo.height+homeGraphic.height
+                               +(weekdayColumn.height*mainPageWeekdayListView.model.count))/units.gridUnit;
+        locationFlickable.contentHeight = units.gu(contentHeightGu+25);
+    }
+
+    /*
+      Extracts values from the location weather data and puts them into the appropriate components
+      to display them.
+
+      Attention: Data access happens through "weatherApp.locationList[]" by index, since complex
+      data in models will lead to type problems.
+    */
+    function renderData(index) {
+        var data = weatherApp.locationsList[index],
+                current = data.data[0].current,
+                forecasts = data.data,
+                forecastsLength = forecasts.length,
+                today = forecasts[0];
+
+        var tempUnits = settings.tempScale === "°C" ? "metric" : "imperial"
+
+        // set general location data
+        name = data.location.name;
+
+        // set current temps and condition
+        iconName = (current.icon) ? current.icon : ""
+        conditionText = (current.condition.main) ? current.condition.main : current.condition; // difference TWC/OWM
+        todayMaxTemp = (today[tempUnits].tempMax !== undefined) ? Math.round(today[tempUnits].tempMax).toString() + settings.tempScale: "";
+        todayMinTemp = Math.round(today[tempUnits].tempMin).toString() + settings.tempScale;
+        currentTemp = Math.round(current[tempUnits].temp).toString() + String("°");
+
+        // reset days list
+        mainPageWeekdayListView.model.clear()
+
+        // set daily forecasts
+        if(forecastsLength > 0) {
+            for(var x=1;x<forecastsLength;x++) {
+                // print(JSON.stringify(forecasts[x][units]))
+                var dayData = {
+                    day: formatTimestamp(forecasts[x].date, 'dddd'),
+                    low: Math.round(forecasts[x][tempUnits].tempMin).toString() + settings.tempScale,
+                    high: (forecasts[x][tempUnits].tempMax !== undefined) ? Math.round(forecasts[x][tempUnits].tempMax).toString() + settings.tempScale : "",
+                                                                            image: (forecasts[x].icon !== undefined && iconMap[forecasts[x].icon] !== undefined) ? iconMap[forecasts[x].icon] + settings.tempScale : ""
+                }
+                mainPageWeekdayListView.model.append(dayData);
+            }
+        }        
+        setFlickableContentHeight();
+    }
+
+    Column {
+        id: locationTop
+        anchors {
+            left: parent.left
+            right: parent.right
+            margins: units.gu(2)
+        }
+        spacing: units.gu(1)
+
+        HeaderRow {
+            id: headerRow
+            locationName: locationItem.name
+        }
+
+        HomeGraphic {
+            id: homeGraphic
+            icon: locationItem.iconName
+        }
+
+        HomeTempInfo {
+            id: homeTempInfo
+            description: conditionText
+            high: locationItem.todayMaxTemp
+            low: locationItem.todayMinTemp
+            now: locationItem.currentTemp
+        }
+
+        ListItem.ThinDivider {}
+    }
+    Column {
+        id: weekdayColumn
+        width: parent.width
+        height: childrenRect.height
+        anchors {
+            top: locationTop.bottom
+            left: parent.left
+            right: parent.right
+            margins: units.gu(2)
+        }
+        Repeater {
+            id: mainPageWeekdayListView
+            model: ListModel{}
+            DayDelegate {
+                day: model.day
+                high: model.high
+                low: model.low
+            }
+        }
+    }
+}

=== modified file 'app/ui/LocationsPage.qml'
--- app/ui/LocationsPage.qml	2015-02-08 19:09:07 +0000
+++ app/ui/LocationsPage.qml	2015-02-15 19:22:09 +0000
@@ -36,14 +36,18 @@
         }
         delegate: ListItem.Standard {
             text: model.location.name
+            onClicked: {
+                settings.current = index;
+                pageStack.pop()
+            }
         }
     }
 
     function populateLocationsModel(locations) {
-        for (var i=0; i < locations.length; i++) {
-            locationsModel.append(locations[i])
+        for (var i=0; i < weatherApp.locationsList.length; i++) {
+            locationsModel.append(weatherApp.locationsList[i])
         }
     }
 
-    Component.onCompleted: storage.getLocations(populateLocationsModel)
+    Component.onCompleted: populateLocationsModel()
 }

=== modified file 'po/com.ubuntu.weather.pot'
--- po/com.ubuntu.weather.pot	2015-02-11 04:29:28 +0000
+++ po/com.ubuntu.weather.pot	2015-02-15 19:22:09 +0000
@@ -8,7 +8,7 @@
 msgstr ""
 "Project-Id-Version: ubuntu-weather-app\n"
 "Report-Msgid-Bugs-To: \n"
-"POT-Creation-Date: 2015-02-11 04:15+0000\n"
+"POT-Creation-Date: 2015-02-15 14:57+0100\n"
 "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
 "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
 "Language-Team: LANGUAGE <[e-mail]>\n"
@@ -21,7 +21,7 @@
 msgid "Today"
 msgstr ""
 
-#: ../app/ui/HomePage.qml:31 ../app/ui/LocationsPage.qml:28
+#: ../app/ui/HomePage.qml:30 ../app/ui/LocationsPage.qml:28
 msgid "Locations"
 msgstr ""