Merge lp:~nik90/ubuntu-weather-app/improved-settings-page into lp:ubuntu-weather-app
| Status: | Merged |
|---|---|
| Approved by: | Victor Thompson on 2015-04-06 |
| Approved revision: | 26 |
| Merged at revision: | 30 |
| Proposed branch: | lp:~nik90/ubuntu-weather-app/improved-settings-page |
| Merge into: | lp:ubuntu-weather-app |
| Diff against target: |
529 lines (+336/-88) 5 files modified
app/components/ExpandableListItem.qml (+90/-0) app/ui/settings/DataProviderPage.qml (+28/-22) app/ui/settings/RefreshIntervalPage.qml (+32/-24) app/ui/settings/UnitsPage.qml (+126/-32) po/com.ubuntu.weather.pot (+60/-10) |
| To merge this branch: | bzr merge lp:~nik90/ubuntu-weather-app/improved-settings-page |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Victor Thompson | Approve on 2015-04-06 | ||
| Andrew Hayzen | 2015-03-20 | Approve on 2015-04-05 | |
| Ubuntu Phone Apps Jenkins Bot | continuous-integration | Approve on 2015-04-05 | |
|
Review via email:
|
|||
Commit Message
Converted the settings page list items to the design followed by the clock app. In the process, the settings options are now properly translatable.
Description of the Change
This MP converts the settings page list items to the design followed by the clock app. In the process, the settings options are now properly translatable.
| Andrew Hayzen (ahayzen) wrote : | # |
I think you need to ensure all the comparisons to some of the settings are made translatable as well eg [0], also ensure that none of the settings are directly used in api calls otherwise these could break if the stored value itself is translated.
| Nekhelesh Ramananthan (nik90) wrote : | # |
> I think you need to ensure all the comparisons to some of the settings are
> made translatable as well eg [0], also ensure that none of the settings are
> directly used in api calls otherwise these could break if the stored value
> itself is translated.
>
> 0 - http://
> page/view/
Nice catch! This issue would only affect the unit settings page as the other settings page like the dataprovider page requires no translation and the refresh interval page uses value instead of text to do the comparison.
Let me think about how I can fix the units settings page.
- 22. By Nekhelesh Ramananthan on 2015-03-20
-
Fixed issue pointed out by ahayzen
| Nekhelesh Ramananthan (nik90) wrote : | # |
I fixed the issue. The units setting page models now have a text and value property. The text property which is displayed is translated and shown to the user while the corresponding value property is the raw value. The value property will be assigned to the settings keys used throughout the app and should work regardless of the language used.
PASSED: Continuous integration, rev:22
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Victor Thompson (vthompson) wrote : | # |
I have a few comments.
1) I do not think "°C" and "°F" are translatable, as they both actually have dedicated unicode characters. I think the representation of each is universal, but I'm not 100% sure.
2) I really think we need notes to translators for each unit abbreviation. It would be very difficult to ascertain what the intent is without a note.
| Victor Thompson (vthompson) wrote : | # |
It might even be the case that standard unit abbreviations should not be translated. KPH seems to be an SI standard, perhaps the others are standardized as well?
| Nekhelesh Ramananthan (nik90) wrote : | # |
I do not know much translations in general, but I have noticed that every time I presume that a certain abbreviation or word is the same in all languages, I am almost always wrong ;-). Looking at http://
That said, I could ask in the ubuntu translation mailing list about this to get their opinion.
| Nekhelesh Ramananthan (nik90) wrote : | # |
I have sent a mail to mailing list at https:/
| Nekhelesh Ramananthan (nik90) wrote : | # |
I got a reply at https:/
| Victor Thompson (vthompson) wrote : | # |
Awesome! Thanks, Nekhelesh! Could you double check for "°C" and "°F"? It seems pretty consistent from what I've seen on the referenced website [1].
- 23. By Nekhelesh Ramananthan on 2015-03-20
-
Added translation notes
PASSED: Continuous integration, rev:23
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Nekhelesh Ramananthan (nik90) wrote : | # |
I checked that website for my language "Tamil" and found that "°C" and "°F" were also translated to {0}°செ. and {0}°ஃபா. respectively ;)
| Victor Thompson (vthompson) wrote : | # |
Thanks, Nekhelesh! Could you make sure that the translation notes get listed for each option (both "in" and "mm")? You'll need to list the note for each line, rather than once for the pair. Also, since each note is related to a specific abbreviation, it might be nice to give the unabbreviated form of the unit:
// TRANSLATORS: The strings are standard measurement units
// of precipitation in inches and are shown in the settings page. Only
// the abbreviated form of inches should be used.
- 24. By Nekhelesh Ramananthan on 2015-04-04
-
Added more translator messages
- 25. By Nekhelesh Ramananthan on 2015-04-04
-
merged lp:ubuntu-weather-app/reboot
PASSED: Continuous integration, rev:25
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Victor Thompson (vthompson) wrote : | # |
Actually, I just noticed a typo: "windSpeeModel" should be windSpeedModel
Additionally, and maybe this should be filed as a separate bug/issue to be tackled later because we would probably want Design input, but I think the Units ListModels should list "Celsius (°C)", "Inches (in)", etc. Also, I really think we need to have the Data Provider options be "Open Weather Map" and "The Weather Channel".
- 26. By Nekhelesh Ramananthan on 2015-04-05
-
Corrected typo windspeemodel
PASSED: Continuous integration, rev:26
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
| Andrew Hayzen (ahayzen) wrote : | # |
Looks good from a functional point of view, however I agree with Victor that "Celsius (°C)" etc may read nicer that "°C" but this can be done in a future mp :)
| Victor Thompson (vthompson) wrote : | # |
Agreed, let's try to get some consensus via a bug report.


PASSED: Continuous integration, rev:21 91.189. 93.70:8080/ job/ubuntu- weather- app-reboot- ci/54/ 91.189. 93.70:8080/ job/ubuntu- weather- app-reboot- utopic- amd64-ci/ 24 91.189. 93.70:8080/ job/ubuntu- weather- app-reboot- vivid-amd64- ci/54
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/54/rebuild
http://