Merge lp://staging/~unity-team/music-app/infographics-translations into lp://staging/music-app/remix

Proposed by Pete Woods
Status: Superseded
Proposed branch: lp://staging/~unity-team/music-app/infographics-translations
Merge into: lp://staging/music-app/remix
Diff against target: 2239 lines (+377/-366)
69 files modified
music-app.qml (+2/-2)
po/CMakeLists.txt (+1/-0)
po/am.po (+2/-2)
po/ar.po (+2/-2)
po/ast.po (+2/-2)
po/az.po (+1/-1)
po/be.po (+2/-2)
po/bg.po (+2/-2)
po/bn.po (+1/-1)
po/br.po (+2/-2)
po/ca.po (+2/-2)
po/ca@valencia.po (+2/-2)
po/ckb.po (+1/-1)
po/com.ubuntu.music.pot (+36/-36)
po/cs.po (+2/-2)
po/da.po (+2/-2)
po/de.po (+43/-41)
po/el.po (+2/-2)
po/en_AU.po (+2/-2)
po/en_GB.po (+2/-2)
po/eo.po (+1/-1)
po/es.po (+2/-2)
po/eu.po (+2/-2)
po/fa.po (+2/-2)
po/fi.po (+2/-2)
po/fo.po (+1/-1)
po/fr.po (+2/-2)
po/gd.po (+3/-3)
po/gl.po (+2/-2)
po/he.po (+2/-2)
po/hr.po (+1/-1)
po/hu.po (+53/-51)
po/id.po (+90/-88)
po/is.po (+2/-2)
po/it.po (+2/-2)
po/ja.po (+2/-2)
po/km.po (+2/-2)
po/kn.po (+1/-1)
po/ko.po (+2/-2)
po/lv.po (+1/-1)
po/mi.po (+1/-1)
po/ml.po (+1/-1)
po/mr.po (+1/-1)
po/ms.po (+2/-2)
po/my.po (+2/-2)
po/nb.po (+2/-2)
po/nl.po (+2/-2)
po/pa.po (+2/-2)
po/pl.po (+2/-2)
po/ps.po (+1/-1)
po/pt.po (+2/-2)
po/pt_BR.po (+2/-2)
po/ro.po (+3/-3)
po/ru.po (+2/-2)
po/shn.po (+1/-1)
po/si.po (+1/-1)
po/sl.po (+1/-1)
po/sq.po (+1/-1)
po/sr.po (+2/-2)
po/st.po (+1/-1)
po/sv.po (+2/-2)
po/ta.po (+1/-1)
po/te.po (+1/-1)
po/tr.po (+2/-2)
po/ug.po (+2/-2)
po/uk.po (+2/-2)
po/zh_CN.po (+45/-41)
po/zh_HK.po (+2/-2)
po/zh_TW.po (+2/-2)
To merge this branch: bzr merge lp://staging/~unity-team/music-app/infographics-translations
Reviewer Review Type Date Requested Status
David Planella Approve
Victor Thompson Needs Fixing
Review via email: mp+248004@code.staging.launchpad.net

This proposal has been superseded by a proposal from 2015-02-02.

Commit message

Strings sent to infographics are no-longer translated - just extract them to translation catalogue

Description of the change

Strings sent to infographics are no-longer translated - just extract them to translation catalogue

Music app currently does:

"<b>%1</b> " + i18n.tr("songs played today")

this has to be changed to:

translateMe("Songs played today: <b>%1</b>")

so that the string that is sent to the infographics server is untranslated, and also so that the message catalogue contains the full string "Songs played today: <b>%1</b>"

To post a comment you must log in.
Revision history for this message
Victor Thompson (vthompson) wrote :

1 inline comment: Is there a reason for changing the format here?

If someone else can get around to testing this, that'd be great. Otherwise, I can do so tonight.

review: Needs Information
819. By Victor Thompson

* Revert trackClicked() change to stop queue loader. Fixes: https://bugs.launchpad.net/bugs/1415697.

Approved by Andrew Hayzen, Ubuntu Phone Apps Jenkins Bot.

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

Also note, we had a similar MP that was rejected by David Planella [1] for changing the format as a workaround to the singular/plural issue. I don't know if that's your intent with the format change here or not... I believe the preference would be to still use the current translation/format. Additionally, changing the format would require the string be retranslated in each language.

1 - https://code.launchpad.net/~andrew-hayzen/music-app/fix-infographics-1351421/+merge/229506/comments/558727

review: Needs Fixing
820. By Victor Thompson

* Make stroked buttons elide text. Fixes: https://bugs.launchpad.net/bugs/1416115.

Approved by Andrew Hayzen, Ubuntu Phone Apps Jenkins Bot.

821. By Launchpad Translations on behalf of music-app-dev

Launchpad automatic translations update.

Revision history for this message
Pete Woods (pete-woods) wrote :

The reason for changing the format string is because it's not possible to do natural translations with correct plurals, as the infographics work with floating point numbers, and gettext only supports integers.

I have done my best attempt at updating the translations here. If you want to keep the format string the same, one of the music app developers will have to make a change that is acceptable - this is just me trying to help out.

Revision history for this message
Pete Woods (pete-woods) wrote :

Thinking about it more, I don't think we have a choice about changing the format string.

The app is doing translate(prefix + "translatable_string") -> infographics server.

The "translatable_string" gets extracted into the catalogue.

The infographics server then tries to translate translate("prefix+translatable_string"). This won't work for two reasons:
1) The catalogue contains "translatable_string" instead of "prefix+translatable_string"
2) The combined string has already been translated by the music app.

We need to simplify all that and just put a straightforward translatable format string into the catalogue, as I have done in this MR.

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

I've added dpm to this review. It sounds like the following would still be acceptable to your solution:

format: translateMe("<b>%1</b> songs played today")

Clearly that would still require the translations to be updated, but it'd also avoid the natural language for plurals issue. But if the direction for all apps using the infographics will be to use the same workaround, then the Music app should follow.

822. By Launchpad Translations on behalf of music-app-dev

Launchpad automatic translations update.

823. By Victor Thompson

* Provide visual feedback on walkthrough buttons
* Increase size of walkthrough continue button
* Increase size of walkthrough skip button. Fixes: https://bugs.launchpad.net/bugs/1416373.

Approved by Andrew Hayzen, Ubuntu Phone Apps Jenkins Bot.

824. By Launchpad Translations on behalf of music-app-dev

Launchpad automatic translations update.

825. By Launchpad Translations on behalf of music-app-dev

Launchpad automatic translations update.

Revision history for this message
Pete Woods (pete-woods) wrote :

Well it won't truly avoid the plural problem, due to the issue of the different plural forms for different languages.

However I don't feel strongly about exactly what string will be used in the music app, so if you definitely want to use the one you suggest, that's fine by me.

Revision history for this message
David Planella (dpm) wrote :

After discussing it on IRC, we've agreed that rather than introducing a new custom translation function, it would make more sense to extend the i18n.tr() API.

See bug 1417031

review: Disapprove
Revision history for this message
Pete Woods (pete-woods) wrote :

We still need to broadly make this change, even if we use the proposed i18n.noop() function. I will update the MR accordingly.

Revision history for this message
David Planella (dpm) wrote :

This now looks more inline with the SDK, good work!

Just two comments:
- See inline comment for RTL languages
- I know it's a separate issue that we'll handle separately, but just for the record we should see if we can find a solution or a workaround for supporting plural forms. From my understanding from talking on IRC, the limitation lies in the fact that in theory libusermetrics could be showing floating-point strings, but gettext supports only integers for plural forms.

Approve, but pending the i18n.noop() merge from the SDK upstream

review: Approve
Revision history for this message
Pete Woods (pete-woods) wrote :

I believe I have updated the RTL languages now (I used the list of RTL languages on Wikipedia as a reference).

826. By Victor Thompson

* Allow walkthrough titles to word wrap up to 2 lines and elide if over 2 lines. Fixes: https://bugs.launchpad.net/bugs/1417043.

Approved by Andrew Hayzen, Sebastien Bacher, Ubuntu Phone Apps Jenkins Bot.

827. By Pete Woods

Use i18n.noop function

828. By Pete Woods

Update translations with new infographics format string

829. By Pete Woods

Merge trunk

830. By Pete Woods

Switch to tag function as actually released in the SDK

831. By Pete Woods

Merge trunk

Unmerged revisions

831. By Pete Woods

Merge trunk

830. By Pete Woods

Switch to tag function as actually released in the SDK

829. By Pete Woods

Merge trunk

828. By Pete Woods

Update translations with new infographics format string

827. By Pete Woods

Use i18n.noop function

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches