Merge lp:~nik90/ubuntu-weather-app/finish-listitem-migration into lp:ubuntu-weather-app
Status: | Rejected |
---|---|
Rejected by: | Victor Thompson on 2015-08-10 |
Proposed branch: | lp:~nik90/ubuntu-weather-app/finish-listitem-migration |
Merge into: | lp:ubuntu-weather-app |
Diff against target: |
1514 lines (+224/-973) 15 files modified
app/components/CMakeLists.txt (+0/-2) app/components/FakeHeader.qml (+39/-0) app/components/ListItemActions/CMakeLists.txt (+0/-5) app/components/ListItemActions/CheckBox.qml (+0/-25) app/components/ListItemActions/Remove.qml (+0/-27) app/components/ListItemReorderComponent.qml (+0/-106) app/components/ListItemWithActions.qml (+0/-496) app/components/MultiSelectHeadState.qml (+0/-72) app/components/MultiSelectListView.qml (+27/-8) app/components/PageWithBottomEdge.qml (+68/-18) app/components/WeatherListItem.qml (+0/-137) app/components/WeatherListView.qml (+1/-2) app/ui/LocationsPage.qml (+82/-53) po/com.ubuntu.weather.pot (+6/-22) ubuntu-weather-app.desktop.in.in (+1/-0) |
To merge this branch: | bzr merge lp:~nik90/ubuntu-weather-app/finish-listitem-migration |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Nekhelesh Ramananthan (community) | Disapprove on 2015-07-03 | ||
Ubuntu Phone Apps Jenkins Bot | continuous-integration | Approve on 2015-06-29 | |
Victor Thompson | 2015-06-26 | Needs Fixing on 2015-06-28 | |
Review via email:
|
Commit message
* Finished the migration to the new SDK list items
* Locked weather app in the Portrait orientation until we get some landscape designs
* Made the page with bottom edge animation less jarring when it reaches the top by adding a fake header (similar to what the clock app does)
Description of the change
This MP implements the following,
* Finish the migration to the new SDK list items
* Lock weather app in the Portrait orientation until we get some landscape designs
* Make the page with bottom edge animation less jarring when it reaches the top by adding a fake header (similar to what the clock app does)
- 64. By Nekhelesh Ramananthan on 2015-06-26
-
Fixed copyright header formatting in fakeheader.qml
- 65. By Nekhelesh Ramananthan on 2015-06-26
-
removed multiselect head state as it was no longer used
- 66. By Nekhelesh Ramananthan on 2015-06-27
-
Fixed locations names not eliding in locations page
PASSED: Continuous integration, rev:66
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
- 67. By Nekhelesh Ramananthan on 2015-06-27
-
Changed multi-delete to use the removeMultiLoca
tions() function
PASSED: Continuous integration, rev:67
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Nekhelesh Ramananthan (nik90) wrote : | # |
I consider this MP to be complete now. Please note the following,
- In trunk, there is a selectAll header button which is shown. Also pressing on the list item selects/deselects it.
This behaviour has not been implemented intentionally due to a bug [1] which I observed in a couple of apps now. I discussed this with Andrew Hayzen, and it turns out this bug is also present in his music-app MP [2]. So this issue is a SDK one and does not matter on the implementation which is a bit different between this MP and andrew's.
[1] https:/
[2] https:/
Victor Thompson (vthompson) wrote : | # |
Currently, there is no trailing divider in the LocationsPage.
The margins of the list items in the LocationsPage has been reduced in this MP--is that intentional?
Can we add a Select all/none toggle action to the multiselection on the LocationsPage?
The settings page always seems to have the list items offset/partially down the page. This was introduced in the previous mp. Can we fix this behaviour?
I see a lot of instances of the following error message, can we fix this?
file://
Nekhelesh Ramananthan (nik90) wrote : | # |
> Currently, there is no trailing divider in the LocationsPage.
>
Btw were you referring to the divider of the trailing swipe actions? Or for the listitem as a whole?
I believe the default behaviour of the new listitems and UbuntuListView is to *not* show a divider for the very last item of the listview. If you add more locations, you will notice the divider being shown.
> The margins of the list items in the LocationsPage has been reduced in this MP
> --is that intentional?
>
Nope, I will fix this.
> Can we add a Select all/none toggle action to the multiselection on the
> LocationsPage?
I'm afraid I can't. I have explained the reasoning at https:/
>
> The settings page always seems to have the list items offset/partially down
> the page. This was introduced in the previous mp. Can we fix this behaviour?
>
This is something I have noticed while testing on rc-proposed channel. And not with the my BQ E4.5 runnign the stable channel. I can check with t1mp tomorrow as to why this is the case. And yes same behavior observed with Podbird while testing on Nexus 4 and BQ.
> I see a lot of instances of the following error message, can we fix this?
>
> file://
> gnu/qt5/
> TypeError: Cannot read property of null
I believe this is a SDK error message that is seen in other apps as well. I haven't changed the PullToRefresh component.
Victor Thompson (vthompson) wrote : | # |
For that first comment, I was talking about the ListItem as a whole. While that may be the default, it is inconsistent with the listitems in the rest of the app. Also, and this is subjective, I think it looks kinda odd without a divider below the final item.
Victor Thompson (vthompson) wrote : | # |
Nik, I have a fix for the settings page ListItems being "offset" [1]. So you probably don't need to ping t1mp.
- 68. By Nekhelesh Ramananthan on 2015-06-29
-
merged lp:ubuntu-weather-app/reboot
- 69. By Nekhelesh Ramananthan on 2015-06-29
-
Added thindivider to the last listitem
PASSED: Continuous integration, rev:69
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Nekhelesh Ramananthan (nik90) wrote : | # |
@Victor, I added the thindivider to the last list item. Going over the stuff you wanted fixed,
- On looking closer, the margins of the listitems in the locations Page look the same to me. I also cross-checked with design and it looks the same. If you were referring to the margins during the multi-select mode, then yes that's changed and that's because the SDK draws the checkbox and the drag handler. I cannot change their margins I am afraid.
- After merging trunk, I am noticing the following error while deleting a location. I am unable to delete a location. On further testing, it seems commit 57 in trunk seems to be causing it. Can we get that reverted pls?
file://
All other issues have been addressed except for the newly cropped delete issue.
Victor Thompson (vthompson) wrote : | # |
I don't think that error is being caused by commit 57. Do you not have the API key and are using TWC perhaps?
Nekhelesh Ramananthan (nik90) wrote : | # |
> I don't think that error is being caused by commit 57. Do you not have the API
> key and are using TWC perhaps?
I cannot remember where I placed the API key, but I do all my testing with the OpenWeatherMap data provider. Also I can add locations and also see the weather data correctly. I am having trouble with only deleting locations. How can that be related to the API key or data provider?
Victor Thompson (vthompson) wrote : | # |
Andrew had seen the same error when he wasn't able to talk to the API (wifi and data were off) and had GPS/location detection turned off.
Maybe the issue is that you've tested with the migration done in commit 57, went back, and then tested with it in place again? Perhaps change addedCurrentLoc
Nekhelesh Ramananthan (nik90) wrote : | # |
What is your workflow? Where do you copy the key.js file to ensure that TheWeatherChannel API works? Either way don't you think it is an issue that with wifi/data/GPS off, deleting a location deletes some other random location? First of all I cannot wrap my head around how deleting a location is remotely connected to having wifi/data/GPS off. Its not like we report back to TheWeatherChannel when a user deletes a location. Somehow I am not happy with the way the storage is implemented.
Feel free to continue this branch. As far I can tell I simply migrated to the new listitems and used the same storage functions calls as before. I cannot resolve this bug.
Nekhelesh Ramananthan (nik90) wrote : | # |
And yes I tried starting with a clean setup by removing all existing databases and config files.
Unmerged revisions
- 69. By Nekhelesh Ramananthan on 2015-06-29
-
Added thindivider to the last listitem
- 68. By Nekhelesh Ramananthan on 2015-06-29
-
merged lp:ubuntu-weather-app/reboot
- 67. By Nekhelesh Ramananthan on 2015-06-27
-
Changed multi-delete to use the removeMultiLoca
tions() function - 66. By Nekhelesh Ramananthan on 2015-06-27
-
Fixed locations names not eliding in locations page
- 65. By Nekhelesh Ramananthan on 2015-06-26
-
removed multiselect head state as it was no longer used
- 64. By Nekhelesh Ramananthan on 2015-06-26
-
Fixed copyright header formatting in fakeheader.qml
- 63. By Nekhelesh Ramananthan on 2015-06-26
-
Lock orientation to portrait
- 62. By Nekhelesh Ramananthan on 2015-06-26
-
Removed more unnecessary components now that we have migrated to the new sdk listitems
- 61. By Nekhelesh Ramananthan on 2015-06-26
-
Implemented reorder support
- 60. By Nekhelesh Ramananthan on 2015-06-26
-
Removed unnecessary ListItemWithAct
ions.qml and WeatherListItem.qml
PASSED: Continuous integration, rev:65 91.189. 93.70:8080/ job/ubuntu- weather- app-reboot- ci/121/ 91.189. 93.70:8080/ job/ubuntu- weather- app-reboot- utopic- amd64-ci/ 91 91.189. 93.70:8080/ job/ubuntu- weather- app-reboot- vivid-amd64- ci/121
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild: 91.189. 93.70:8080/ job/ubuntu- weather- app-reboot- ci/121/ rebuild
http://