Ubuntu Weather App

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

  1. reboot-nav-and-snap-scrolling
  2. Merge into reboot
Proposed by Martin Borho
Status: Merged
Approved by: Victor Thompson
Approved revision: 19
Merged at revision: 11
Proposed branch: lp:~martin-borho/ubuntu-weather-app/reboot-nav-and-snap-scrolling
Merge into: lp:ubuntu-weather-app
Diff against target: 429 lines (+217/-110)
5 files modified
app/ubuntu-weather-app.qml (+10/-12)
app/ui/HomePage.qml (+53/-93)
app/ui/LocationPane.qml (+145/-0)
app/ui/LocationsPage.qml (+7/-3)
po/com.ubuntu.weather.pot (+2/-2)
To merge this branch: bzr merge lp:~martin-borho/ubuntu-weather-app/reboot-nav-and-snap-scrolling
Reviewer Review Type Date Requested Status
Victor Thompson Approve
Ubuntu Phone Apps Jenkins Bot continuous-integration Approve
Andrew Hayzen Approve
Review via email: mp+249772@code.launchpad.net

Commit message

Implemented horizontal scrolling through loctions and a functional bottom edge.

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :

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)
Revision history for this message
Andrew Hayzen (ahayzen) wrote :

- 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 updated
14. By Martin Borho

added current property to settings

Revision history for this message
Andrew Hayzen (ahayzen) wrote :

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.

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :

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)
Revision history for this message
Andrew Hayzen (ahayzen) wrote :

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 updated
15. By Martin Borho

removed highlightMoveDuration

16. By Martin Borho

currentIndex gets set from settings.current

17. By Martin Borho

formatting

18. By Martin Borho

pushing HomePage.qml directly to pagestack

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :

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)
Revision history for this message
Andrew Hayzen (ahayzen) wrote :

LGTM, thanks for the extra changes :)

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

review: Approve
Revision history for this message
Victor Thompson (vthompson) wrote :

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 updated
19. By Martin Borho

added workaround by victor for snapMode bug

Revision history for this message
Martin Borho (martin-borho) wrote :

@vthompson workaround added!

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :

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)
Revision history for this message
Victor Thompson (vthompson) wrote :

Thanks! lgtm, thanks Martin!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
45 /*45 /*
46 List of locations and their data, accessible through index46 List of locations and their data, accessible through index
47 */47 */
48 property var locationsList: []48 property var locationsList: []
4949
50 /*50 /*
51 Index of Location before a refresh, to go back after51 Index of Location before a refresh, to go back after
@@ -81,9 +81,8 @@
8181
82 /* Fill the location pages with their data. */82 /* Fill the location pages with their data. */
83 function fillPages(locations) {83 function fillPages(locations) {
84 locationsList = []
84 locationsList = locations;85 locationsList = locations;
85 // refactor this when Location are in a ListView!
86 homePage.renderData();
87 }86 }
8887
89 /*88 /*
@@ -98,9 +97,9 @@
98 WeatherApi.sendRequest({97 WeatherApi.sendRequest({
99 action: "updateData",98 action: "updateData",
100 params: {99 params: {
101 locations:locations,100 locations: locations,
102 force:force_refresh,101 force: force_refresh,
103 service:settings.service,102 service: settings.service,
104 api_key: Key.twcKey,103 api_key: Key.twcKey,
105 interval: settings.refreshInterval104 interval: settings.refreshInterval
106 }105 }
@@ -112,6 +111,10 @@
112 Settings {111 Settings {
113 id: settings112 id: settings
114 category: "weatherSettings"113 category: "weatherSettings"
114 /*
115 Index of the current locationList of locations and their data, accessible through index
116 */
117 property int current: 0
115118
116 property int refreshInterval: 1800119 property int refreshInterval: 1800
117 property string precipUnits120 property string precipUnits
@@ -142,11 +145,6 @@
142145
143 PageStack {146 PageStack {
144 id: mainPageStack147 id: mainPageStack
145148 Component.onCompleted: mainPageStack.push(Qt.resolvedUrl("ui/HomePage.qml"))
146 HomePage {
147 id: homePage
148 }
149
150 Component.onCompleted: mainPageStack.push(homePage)
151 }149 }
152}150}
153151
=== 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 @@
1818
19import QtQuick 2.319import QtQuick 2.3
20import Ubuntu.Components 1.120import Ubuntu.Components 1.1
21import Ubuntu.Components.ListItems 0.1 as ListItem
22import "../components"21import "../components"
2322
2423
@@ -32,16 +31,6 @@
32 tipColor: UbuntuColors.orange31 tipColor: UbuntuColors.orange
33 tipLabelColor: "#FFF"32 tipLabelColor: "#FFF"
3433
35 /*
36 Data properties
37 */
38 property string name
39 property string conditionText
40 property string currentTemp
41 property string todayMaxTemp
42 property string todayMinTemp
43 property string iconName
44
45 // TODO map iconnames to source image names34 // TODO map iconnames to source image names
46 property var iconMap: {35 property var iconMap: {
47 "moon": "moon.svg"36 "moon": "moon.svg"
@@ -57,89 +46,60 @@
57 }46 }
5847
59 /*48 /*
60 Extracts values from the location weather data and puts them into the appropriate components49 Flickable to scroll the location vertically.
61 to display them.50 The respective contentHeight gets calculated after the Location is filled with data.
62
63 Attention: Data access happens through "weatherApp.locationList[]" by index, since complex
64 data in models will lead to type problems.
65 */51 */
66 function renderData() {52 Flickable {
67 var index = Math.floor(Math.random()*weatherApp.locationsList.length), // TODO get this value from ListView.currentIndex later!53 id: locationFlickable
68 data = weatherApp.locationsList[index],54 width: parent.width
69 current = data.data[0].current,55 height: parent.height
70 forecasts = data.data,56 contentWidth: parent.width
71 forecastsLength = forecasts.length,57
72 today = forecasts[0];58 /*
7359 ListView for locations with snap-scrolling horizontally.
74 var tempUnits = settings.tempScale === "°C" ? "metric" : "imperial"60 */
7561 ListView {
76 // set general location data62 id: locationPages
77 name = data.location.name;63 anchors.fill: parent
7864 width: parent.width
79 // set current temps and condition65 height: childrenRect.height
80 iconName = (current.icon) ? current.icon : ""66 contentWidth: parent.width
81 conditionText = (current.condition.main) ? current.condition.main : current.condition; // difference TWC/OWM67 contentHeight: childrenRect.height
82 todayMaxTemp = (today[tempUnits].tempMax !== undefined) ? Math.round(today[tempUnits].tempMax).toString() + settings.tempScale: "";68 model: weatherApp.locationsList.length
83 todayMinTemp = Math.round(today[tempUnits].tempMin).toString() + settings.tempScale;69 // TODO with snapMode, currentIndex is not properly set and setting currentIndex fails
84 currentTemp = Math.round(current[tempUnits].temp).toString() + String("°");70 //snapMode: ListView.SnapOneItem
8571 orientation: ListView.Horizontal
86 // reset days list72 currentIndex: settings.current
87 mainPageWeekdayListView.model.clear()73 highlightRangeMode: ListView.StrictlyEnforceRange
8874 onCurrentIndexChanged: {
89 // set daily forecasts75 print("CI: "+currentIndex)
90 if(forecastsLength > 0) {76 if (loaded) {
91 for(var x=1;x<forecastsLength;x++) {77 settings.current = currentIndex
92 // print(JSON.stringify(forecasts[x][units]))78 }
93 var dayData = {79 }
94 day: formatTimestamp(forecasts[x].date, 'dddd'),80 onModelChanged: {
95 low: Math.round(forecasts[x][tempUnits].tempMin).toString() + settings.tempScale,81 currentIndex = settings.current
96 high: (forecasts[x][tempUnits].tempMax !== undefined) ? Math.round(forecasts[x][tempUnits].tempMax).toString() + settings.tempScale : "",82
97 image: (forecasts[x].icon !== undefined && iconMap[forecasts[x].icon] !== undefined) ? iconMap[forecasts[x].icon] + settings.tempScale : ""83 if (model > 0) {
98 }84 loaded = true
99 mainPageWeekdayListView.model.append(dayData);85 }
100 }86 }
101 }87 delegate: LocationPane {}
102 }88
10389 property bool loaded: false
104 ListView {90 // TODO: workaround for not being able to use snapMode property
105 id: mainPageWeekdayListView91 Component.onCompleted: {
106 anchors {92 var scaleFactor = units.gridUnit * 10;
107 fill: parent93 maximumFlickVelocity = maximumFlickVelocity * scaleFactor;
108 margins: units.gu(2)94 flickDeceleration = flickDeceleration * scaleFactor;
109 }95 }
11096
111 header: Column {97 Connections {
112 anchors {98 target: settings
113 left: parent.left99 onCurrentChanged: {
114 right: parent.right100 locationPages.currentIndex = settings.current
115 }101 }
116 spacing: units.gu(1)102 }
117
118 HeaderRow {
119 locationName: locationPage.name
120 }
121
122 HomeGraphic {
123 icon: locationPage.iconName
124 }
125
126 HomeTempInfo {
127 description: locationPage.conditionText
128 high: locationPage.todayMaxTemp
129 low: locationPage.todayMinTemp
130 now: locationPage.currentTemp
131 }
132
133 ListItem.ThinDivider {
134
135 }
136 }
137 model: ListModel {}
138
139 delegate: DayDelegate {
140 day: model.day
141 high: model.high
142 low: model.low
143 }103 }
144 }104 }
145}105}
146106
=== 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 @@
1/*
2 * Copyright (C) 2015 Canonical Ltd
3 *
4 * This file is part of Ubuntu Weather App
5 *
6 * Ubuntu Weather App is free software: you can redistribute it and/or modify
7 * it under the terms of the GNU General Public License version 3 as
8 * published by the Free Software Foundation.
9 *
10 * Ubuntu Weather App is distributed in the hope that it will be useful,
11 * but WITHOUT ANY WARRANTY; without even the implied warranty of
12 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
13 * GNU General Public License for more details.
14 *
15 * You should have received a copy of the GNU General Public License
16 * along with this program. If not, see <http://www.gnu.org/licenses/>.
17 */
18
19import QtQuick 2.3
20import Ubuntu.Components 1.1
21import Ubuntu.Components.ListItems 0.1 as ListItem
22import "../components"
23
24Rectangle {
25 id: locationItem
26 /*
27 Data properties
28 */
29 property string name
30 property string conditionText
31 property string currentTemp
32 property string todayMaxTemp
33 property string todayMinTemp
34 property string iconName
35
36 Component.onCompleted: renderData(index)
37
38 width: locationPage.width
39 height: childrenRect.height
40 anchors.fill: parent.fill
41
42 /*
43 Calculates the height of all location data components, to set the Flickable.contentHeight right.
44 */
45 function setFlickableContentHeight() {
46 var contentHeightGu = (homeTempInfo.height+homeGraphic.height
47 +(weekdayColumn.height*mainPageWeekdayListView.model.count))/units.gridUnit;
48 locationFlickable.contentHeight = units.gu(contentHeightGu+25);
49 }
50
51 /*
52 Extracts values from the location weather data and puts them into the appropriate components
53 to display them.
54
55 Attention: Data access happens through "weatherApp.locationList[]" by index, since complex
56 data in models will lead to type problems.
57 */
58 function renderData(index) {
59 var data = weatherApp.locationsList[index],
60 current = data.data[0].current,
61 forecasts = data.data,
62 forecastsLength = forecasts.length,
63 today = forecasts[0];
64
65 var tempUnits = settings.tempScale === "°C" ? "metric" : "imperial"
66
67 // set general location data
68 name = data.location.name;
69
70 // set current temps and condition
71 iconName = (current.icon) ? current.icon : ""
72 conditionText = (current.condition.main) ? current.condition.main : current.condition; // difference TWC/OWM
73 todayMaxTemp = (today[tempUnits].tempMax !== undefined) ? Math.round(today[tempUnits].tempMax).toString() + settings.tempScale: "";
74 todayMinTemp = Math.round(today[tempUnits].tempMin).toString() + settings.tempScale;
75 currentTemp = Math.round(current[tempUnits].temp).toString() + String("°");
76
77 // reset days list
78 mainPageWeekdayListView.model.clear()
79
80 // set daily forecasts
81 if(forecastsLength > 0) {
82 for(var x=1;x<forecastsLength;x++) {
83 // print(JSON.stringify(forecasts[x][units]))
84 var dayData = {
85 day: formatTimestamp(forecasts[x].date, 'dddd'),
86 low: Math.round(forecasts[x][tempUnits].tempMin).toString() + settings.tempScale,
87 high: (forecasts[x][tempUnits].tempMax !== undefined) ? Math.round(forecasts[x][tempUnits].tempMax).toString() + settings.tempScale : "",
88 image: (forecasts[x].icon !== undefined && iconMap[forecasts[x].icon] !== undefined) ? iconMap[forecasts[x].icon] + settings.tempScale : ""
89 }
90 mainPageWeekdayListView.model.append(dayData);
91 }
92 }
93 setFlickableContentHeight();
94 }
95
96 Column {
97 id: locationTop
98 anchors {
99 left: parent.left
100 right: parent.right
101 margins: units.gu(2)
102 }
103 spacing: units.gu(1)
104
105 HeaderRow {
106 id: headerRow
107 locationName: locationItem.name
108 }
109
110 HomeGraphic {
111 id: homeGraphic
112 icon: locationItem.iconName
113 }
114
115 HomeTempInfo {
116 id: homeTempInfo
117 description: conditionText
118 high: locationItem.todayMaxTemp
119 low: locationItem.todayMinTemp
120 now: locationItem.currentTemp
121 }
122
123 ListItem.ThinDivider {}
124 }
125 Column {
126 id: weekdayColumn
127 width: parent.width
128 height: childrenRect.height
129 anchors {
130 top: locationTop.bottom
131 left: parent.left
132 right: parent.right
133 margins: units.gu(2)
134 }
135 Repeater {
136 id: mainPageWeekdayListView
137 model: ListModel{}
138 DayDelegate {
139 day: model.day
140 high: model.high
141 low: model.low
142 }
143 }
144 }
145}
0146
=== 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 @@
36 }36 }
37 delegate: ListItem.Standard {37 delegate: ListItem.Standard {
38 text: model.location.name38 text: model.location.name
39 onClicked: {
40 settings.current = index;
41 pageStack.pop()
42 }
39 }43 }
40 }44 }
4145
42 function populateLocationsModel(locations) {46 function populateLocationsModel(locations) {
43 for (var i=0; i < locations.length; i++) {47 for (var i=0; i < weatherApp.locationsList.length; i++) {
44 locationsModel.append(locations[i])48 locationsModel.append(weatherApp.locationsList[i])
45 }49 }
46 }50 }
4751
48 Component.onCompleted: storage.getLocations(populateLocationsModel)52 Component.onCompleted: populateLocationsModel()
49}53}
5054
=== 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 @@
8msgstr ""8msgstr ""
9"Project-Id-Version: ubuntu-weather-app\n"9"Project-Id-Version: ubuntu-weather-app\n"
10"Report-Msgid-Bugs-To: \n"10"Report-Msgid-Bugs-To: \n"
11"POT-Creation-Date: 2015-02-11 04:15+0000\n"11"POT-Creation-Date: 2015-02-15 14:57+0100\n"
12"PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"12"PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
13"Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"13"Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
14"Language-Team: LANGUAGE <LL@li.org>\n"14"Language-Team: LANGUAGE <LL@li.org>\n"
@@ -21,7 +21,7 @@
21msgid "Today"21msgid "Today"
22msgstr ""22msgstr ""
2323
24#: ../app/ui/HomePage.qml:31 ../app/ui/LocationsPage.qml:2824#: ../app/ui/HomePage.qml:30 ../app/ui/LocationsPage.qml:28
25msgid "Locations"25msgid "Locations"
26msgstr ""26msgstr ""
2727

Subscribers

People subscribed via source and target branches

to all changes: