changelog + i18n: Document changes since v27.165; fix language tags #4901

chrisbobbe · 2021-07-16T20:14:11Z

EDIT: And add a few more languages to the UI, and do a Transifex sync.

gnprice

Thanks! Reading the changelog now. Meanwhile, comments on the translations part of the branch.

gnprice · 2021-07-19T19:52:57Z

src/settings/languages.js

-  // TODO: add `en_GB` -- it's "100% translated", though that just means
-  //   the one string mentioning "organization" has s/z/s/ in that word
+  { locale: 'en_GB', name: 'English (U.K.)', nativeName: 'English (U.K.)' },


This needs a corresponding entry in src/i18n/messages.js -- otherwise I think it won't work if you try to use it.

(Hmm, perhaps we could get Flow to check for that for us, with something like $Keys<typeof messages>.)

gnprice · 2021-07-19T20:02:16Z

static/translations/messages_ar.json

+  "Message": "Message",
+  "Send": "إرسال",
+  "Please choose recipients to share with": "Please choose recipients to share with",


Note: The "Portuguese (Brazil)" message, added in messages_en.json in this series, is added to the other languages in this commit, instead of in a separate "Sync recently-added message strings" commit. That's because I ran `tools/tx-sync` twice after that string was added, and I kept the result from the second time. This was a small mistake that I don't expect to cause real problems, but it does explain what happened with that string.

Yeah, and it looks like the same thing happened with a number of these other strings.

Nothing all that bad happens -- nothing at all goes wrong at runtime. The only effect is that it makes it hard to spot the actual new translations in the history. (You can get some idea by looking at the diffstat and seeing that some languages have more edits than others; but then there's also cases like this one where a new string already has a translation.)

Definitely not worth trying to go back and pick apart the changes into two commits. But that's the reason why I developed a habit of doing first a pull for new translations on existing strings, committing that, then a push + pull to sync translations across. And then codifying that habit into the tools/tx-sync script 🙂

Basically, the Transifex push that happens in the last step of tools/tx-sync has side effects -- it pushes strings to Transifex, and so can immediately change what the next pull from Transifex will get -- so you always want to push (in Git) the result of the first step of tools/tx-sync, where it pulled translations down from Transifex.

gnprice

And comments on the changelog. Thanks again!

gnprice · 2021-07-19T20:05:37Z

docs/changelog.md

+  (PR #4590), #4657, #4115

 ## 27.165 (2021-06-24)


nit: two blank lines between sections (here and above) -- helpful for people reading the Markdown source to visually parse it

gnprice · 2021-07-19T20:09:52Z

docs/changelog.md

+* Resolved issues (latest to earliest): #117, #4165, #4858, #4850, #4849,
+  #3205, part of #4309 (PR #4817), #3244, #3452, parts of #4540 and #2366
+  (PR #4590), #4657, #4115


It looks like I started using this particular format for this list last fall. I think the reason I made it latest-to-earliest was to make it agree with the order in the GitHub search results:
https://github.com/zulip/zulip-mobile/issues?q=is%3Aclosed+sort%3Aupdated-desc
for some convenience when compiling it.

But perhaps earliest-to-latest would be good to switch to next time -- it'd make it easier when adding a few more between drafting the changelog and cutting the release.

gnprice · 2021-07-19T20:45:13Z

docs/changelog.md

+* (Android) You can now share content from other apps to Zulip. (#117)
+* Fixed a bug where network or server issues could cause an infinite
+  full-screen loading spinner. It now times out after 60 seconds. (#4165)
+* Search screen now opens at the newest messages, not the oldest. (#4858)


I'd skip this in highlights -- it's not yet something we want to encourage people to try out, until we also have fetching more when you scroll up.

gnprice · 2021-07-19T20:49:01Z

docs/changelog.md

+* "Do not mark messages read on scroll" option is now more discoverable.
+  (#4850)


I think it's clearer just to frame this as a new feature. It wasn't really a user-facing feature of the app before, though it was a tool for our use as developers:

The fact that the indicators disappeared made it very conspicuously glitchy, if you were expecting it to work like a proper fully-baked feature.

It was tucked away and explicitly labeled as a debug setting.

Then that lets us use this sentence to describe the feature, instead of referring to it by a name.

gnprice · 2021-07-19T20:51:35Z

docs/changelog.md

+  (#4850)
+* Basic support for polls. More to come! (#3205)
+
+### Highlights for developers


Another item that'd be good here: PR #4795, tools/test jest by default randomly picks Android or iOS codepaths, rather than always iOS.

gnprice · 2021-07-19T20:57:05Z

docs/changelog.md

+* Resolved issues (latest to earliest): #117, #4165, #4858, #4850, #4849,
+  #3205, part of #4309 (PR #4817), #3244, #3452, parts of #4540 and #2366
+  (PR #4590), #4657, #4115



See the v27.162 changelog for a way to incorporate #4818 here. I think it helps to be explicit like that -- here in the developer notes -- about the relationship to the previous release on the main line (v27.164), as well as the latest previous release (v27.165).

gnprice · 2021-07-19T21:02:04Z

docs/changelog.md

+* We're now using @react-native-community/push-notifications-ios instead of
+  two different libraries. (#4115)
+* Resolved issues (latest to earliest): #117, #4165, #4858, #4850, #4849,
+  #3205, part of #4309 (PR #4817), #3244, #3452, parts of #4540 and #2366


Also PR #4635, which added a (small) user-facing feature that we don't seem to have filed an issue for. (Goes right next to #3244, which was fixed in the same PR.)

And between #4115 and #4657, there's
PR #4821 shorter debounce on mark-as-read
PR #4820 fix a BAD_REQUEST
PR #4815 rm "Possible infinite loop"
PR #4797 turn off lazy loading in MainTabsScreen
Those didn't have GitHub issues filed for them, but they each fixed a bug or made a small improvement in user-visible behavior.

So we don't accidentally add an item to languages.js that doesn't have a corresponding entry in messages.js; see zulip#4901 (comment).

chrisbobbe · 2021-07-20T16:54:18Z

Thanks for the review! Revision pushed. I haven't run tools/tx-sync after these tweaks to the translation logic; should I do that?

gnprice

Thanks for the revisions! Looks good except two comments below -- I'll fix those and merge. (One is tiny, and the other just means dropping one commit.)

gnprice · 2021-07-20T20:33:15Z

src/settings/languages.js

+  { tag: 'pt', name: 'Portuguese', nativeName: 'Português' },
+  { tag: 'pt-BR', name: 'Portuguese (Brazil)', nativeName: 'Português (Brasil)' },
+  { tag: 'pt-PT', name: 'Portuguese (Portugal)', nativeName: 'Português (Portugal)' },


Hmm, something seems wrong when we have these three choices all next to each other.

It looks like the state of things:
https://www.transifex.com/zulip/zulip/mobile/
is that the pt-BR translation is quite fragmentary, but the pt and pt-PT translations are much more complete: 11% vs 89% and 64%. So I think the simplest reasonable thing to do for now is to leave the fragmentary one out of it. I'd hope that the pt translation is actually Brazilian Portuguese.

Some previous discussion here of how we might straighten out this area of our translations:
https://chat.zulip.org/#narrow/stream/58-translation/topic/language.20cleanup

gnprice · 2021-07-20T20:34:00Z

docs/changelog.md

+* Basic support for polls. More to come! (#3205)
+


nit: two blank lines between sections

So we don't have to assign valid JavaScript identifiers to these things, which causes confusion. As Anders points out [1], `-` and `_` have separate meanings. So it's best not to choose `_` simply to make a valid JS identifier. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60_.60.20vs.20.60-.60.20in.20locales/near/1234566

Just one of these invalid ones was available to choose in the UI: pt_PT, changed here to pt-PT. If chosen, the app would crash (to `RootErrorBoundary`) with `RangeError: invalid language tag: pt_PT`; see discussion [1]. Since the language setting is persisted, include a migration, so people can recover without uninstalling and reinstalling. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.60_.60.20vs.20.60-.60.20in.20locales/near/1234560

And clarify that it's an "IETF BCP 47 language tag" [1]. `react-intl`'s doc on `IntlProvider` doesn't currently say that this is the kind of thing it wants for the `locale` prop [2]. But it seems likely: - In 2016, a GitHub "collaborator" said, "The locale needs to be a BCP-47 language tag" [3]. - @formatjs's doc on something else, the `IntlMessageFormat` constructor in the `intl-messageformat` package, takes a similarly-named parameter, `locales` that's documented with, "A string with a BCP 47 language tag, or an array of such strings" [4]. And include a migration, to preserve the user's current setting across the rename. [1] https://en.wikipedia.org/wiki/IETF_language_tag [2] https://formatjs.io/docs/react-intl/components#locale-formats-and-messages [3] formatjs/formatjs#204 (comment) [4] https://formatjs.io/docs/intl-messageformat/#intlmessageformat-constructor

So we don't accidentally add an item to languages.js that doesn't have a corresponding entry in messages.js; see zulip#4901 (comment).

This is now 14% translated, which is above our 5% threshold to offer in the UI. (There's already an entry for Danish in messages_en.json, so no need to add one here.)

And remove a now-unnecessary TODO. (We use the correct language tag "en-GB", instead of "en_GB", which is what the TODO instructed.) This is now 96% translated, which is well above our 5% threshold to offer in the UI. (There's already an entry for English (U.K.) in messages_en.json, so no need to add one here.)

Thanks as always to our kind volunteer translators. Note: The recently-added strings are synced across languages in this commit, instead of in a separate "Sync recently-added message strings" commit. That's because I ran `tools/tx-sync` twice after that string was added, and I kept the result from the second time. This was a small mistake that I don't expect to cause real problems, but it does explain what happened with that string.

chrisbobbe · 2021-07-20T20:38:29Z

Thanks for the review!

gnprice · 2021-07-20T20:47:21Z

I haven't run tools/tx-sync after these tweaks to the translation logic; should I do that?

In general I think the mode that tools/tx-sync works best in is to be more or less synchronous -- after running it, you push the results to the Git repo soon afterward. That's because it has side effects in Transifex, through the tx push it runs.

It also should be the case that any commits it makes are pretty much guaranteed to be OK: they're just syncing down what's in Transifex.

So I think the right strategy when using it is probably:

go ahead and push directly the commits it makes, without a PR;
if there are associated changes you make, like adding new languages or changing how some of the system works, that feel like they should go in a PR, then separate those and send them in a PR after pushing the automated sync commits.

Does that make sense? We should probably say something about that in the howto/translations doc 🙂

chrisbobbe · 2021-07-20T21:12:49Z

Does that make sense?

Yes, I think so, thanks!

then separate those and send them in a PR after pushing the automated sync commits

So, while the tx-sync commits should be pushed immediately, there isn't a requirement in general that the tx-sync-and-push task is done just before (and in the same push as) making the new release tag, with tools/bump-version, right?

Or maybe it's OK to do a tx-sync-and-push at any time, but we should be sure to also do one along with tools/bump-version?

gnprice · 2021-07-20T21:56:21Z

Or maybe it's OK to do a tx-sync-and-push at any time, but we should be sure to also do one along with tools/bump-version?

Yes, exactly.

In fact ideally we should be doing one each time we merge a change that adds or changes message strings. That way the new strings get uploaded to Transifex, so that our translation contributors have an opportunity to translate them.

See zulip#4901 (comment) for what prompted this.

We made an explicit choice that this option wouldn't be included even though it's above the numerical threshold, because it's redundant: zulip#4901 (comment) But without a comment in the code, as long as it's above the numerical threshold it'll be natural for a maintainer seeing that to go and add the option here without being aware of that reasoning. Add such a comment.

See zulip#4901 (comment) for what prompted this.

Looks like this got lost in zulip#4901. As Greg points out [1]: > I think this means a regression in the current beta: on upgrade, > we'll effectively forget the user's choice of language, and revert > to the default of English. > The fix should be pretty simple: just add the missing migration. > > For non-beta users, it'll be as if it was never broken. > > For beta users who upgraded to version 166 or 167, and then > upgrade to a version with that fix, the effect will be that we > take the setting they had before the 166 or 167 upgrade, and > revert to that setting. For most of them that will either have no > effect (because they already went and changed the setting back to > the same preferred language), or take them back from English to > the language they want to be using… or both (because their > preferred language was English all along). There's a possibility > that some users will have made a different choice since the > upgrade, and this will clobber that choice. That seems fine; it's > a pretty tolerable glitch for using beta releases of software, and > any other solution would be more complex. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/state.2Esettings.2Elanguage/near/1246410

We made an explicit choice that this option wouldn't be included even though it's above the numerical threshold, because it's redundant: zulip#4901 (comment) But without a comment in the code, as long as it's above the numerical threshold it'll be natural for a maintainer seeing that to go and add the option here without being aware of that reasoning. Add such a comment.

chrisbobbe requested a review from gnprice July 16, 2021 20:14

chrisbobbe force-pushed the pr-changelog-post-165 branch 3 times, most recently from e65ea34 to f2a34ad Compare July 19, 2021 18:52

gnprice reviewed Jul 19, 2021

View reviewed changes

chrisbobbe force-pushed the pr-changelog-post-165 branch from f2a34ad to c9b976d Compare July 20, 2021 16:53

gnprice reviewed Jul 20, 2021

View reviewed changes

gnprice changed the title ~~changelog: Document changes since v27.165.~~ changelog + i18n: Document changes since v27.165; fix language tags Jul 20, 2021

chrisbobbe added 8 commits July 20, 2021 13:35

i18n types: Require that Language objects have message data.

e00d420

So we don't accidentally add an item to languages.js that doesn't have a corresponding entry in messages.js; see zulip#4901 (comment).

changelog: Document changes since v27.165.

08f576e

i18n: Offer Danish as a language choice.

13820be

This is now 14% translated, which is above our 5% threshold to offer in the UI. (There's already an entry for Danish in messages_en.json, so no need to add one here.)

gnprice force-pushed the pr-changelog-post-165 branch from c9b976d to f2de947 Compare July 20, 2021 20:37

gnprice merged commit f2de947 into zulip:master Jul 20, 2021

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Jul 21, 2021

i18n [nfc]: Document translation-sync process more.

537c880

See zulip#4901 (comment) for what prompted this.

chrisbobbe mentioned this pull request Jul 21, 2021

i18n [nfc]: Document translation-sync process more. #4915

Merged

gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this pull request Aug 12, 2021

i18n [nfc]: Document translation-sync process more.

94a9911

See zulip#4901 (comment) for what prompted this.

chrisbobbe mentioned this pull request Aug 13, 2021

settings: Add migration for `state.settings.{locale -> language}. #4959

Merged

chrisbobbe deleted the pr-changelog-post-165 branch November 4, 2021 21:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

changelog + i18n: Document changes since v27.165; fix language tags #4901

changelog + i18n: Document changes since v27.165; fix language tags #4901

chrisbobbe commented Jul 16, 2021 •

edited

Loading

gnprice left a comment

gnprice Jul 19, 2021

gnprice Jul 19, 2021

gnprice left a comment

gnprice Jul 19, 2021

gnprice Jul 19, 2021

gnprice Jul 19, 2021

gnprice Jul 19, 2021

gnprice Jul 19, 2021

gnprice Jul 19, 2021

gnprice Jul 19, 2021

chrisbobbe commented Jul 20, 2021

gnprice left a comment

gnprice Jul 20, 2021

gnprice Jul 20, 2021

chrisbobbe commented Jul 20, 2021

gnprice commented Jul 20, 2021

chrisbobbe commented Jul 20, 2021 •

edited

Loading

gnprice commented Jul 20, 2021

		* "Do not mark messages read on scroll" option is now more discoverable.
		(#4850)

changelog + i18n: Document changes since v27.165; fix language tags #4901

changelog + i18n: Document changes since v27.165; fix language tags #4901

Conversation

chrisbobbe commented Jul 16, 2021 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jul 20, 2021

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jul 20, 2021

gnprice commented Jul 20, 2021

chrisbobbe commented Jul 20, 2021 • edited Loading

gnprice commented Jul 20, 2021

chrisbobbe commented Jul 16, 2021 •

edited

Loading

chrisbobbe commented Jul 20, 2021 •

edited

Loading