Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

"Mark x unread messages as read" message count is rendered incorrectly #5670

Closed
DanWaLes opened this issue Mar 3, 2023 · 12 comments
Closed

"Mark x unread messages as read" message count is rendered incorrectly #5670

DanWaLes opened this issue Mar 3, 2023 · 12 comments

Comments

@DanWaLes
Copy link

DanWaLes commented Mar 3, 2023 •
edited
Loading

the number of unread messages in a stream isn't correctly rendered on iPhone 5s iOS 12.5.6 Zulip version 27.201. It was introduced sometime last month when i manually installed an update. the button works but the text is incorrect. Updating to iOS 12.5.7 didnt fix.

https://i.postimg.cc/XN1rgRg8/unread-message-count.jpg

to reproduce

  1. connect to internet (if not connected already)
  2. open Zuilp app
  3. click on a stream with unread messages

text of some sort of language feature not working is shown, the button doesnt fit on screen (but is still functional) and the number of unread messages is not displayed
@DanWaLes DanWaLes changed the title unread message count text is wrong "Mark x unread messages as read" message count is rendered incorrectly Mar 3, 2023
@chrisbobbe
Copy link
Contributor

Hmm, thanks for the report! I'm curious, if you go to the in-app settings, what language is selected for the UI? The app is translated into many languages (though incompletely, and we fall back to English where translations are missing), and I wonder if something has gone wrong with a translation.

@DanWaLes
Copy link
Author

DanWaLes commented Mar 3, 2023

language is set as English, i've never changed it. would switching to english uk do a difference? i'm from uk anyway

@chrisbobbe chrisbobbe mentioned this issue Mar 3, 2023
Merged
@gnprice
Copy link
Member

gnprice commented Mar 3, 2023 •
edited
Loading

Thanks for the report. I believe the problem here is, unfortunately, caused by your iOS version not providing some features we rely on. We recently dropped support for iOS versions older than iOS 14, and we're now using platform features that make it possible to accurately handle plurals for translated text.

Given that we dropped support for your version of iOS, it shouldn't have been possible for you to take the upgrade to Zulip — your device should have remained at the last Zulip version that supported it. It looks like we left something out when updating the metadata that would tell your device and the App Store that the new version shouldn't be installed on your version. I'll file an issue for that (→ #5672) and we'll follow up.

@gnprice gnprice mentioned this issue Mar 3, 2023
Closed
@chrisbobbe
Copy link
Contributor

we're now using platform features that make it possible to accurately handle plurals for translated text.

Interesting. I noticed the iOS 12 when I read this report but couldn't think of what would break with this symptom. Do you recall what those platform features were?

@gnprice
Copy link
Member

gnprice commented Mar 3, 2023

On iOS, we use the system's JavaScript implementation (because Apple doesn't allow others), and I'm guessing that this fancy plurals feature is making use of something provided by the JS implementation.

@chrisbobbe
Copy link
Contributor

chrisbobbe commented Mar 3, 2023 •
edited
Loading

Indeed, Intl.PluralRules would be unavailable before iOS 13, I think: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/PluralRules

I think the fact that 35129e5 (our first use of the fancy plurals feature) came after 783e56b (requiring iOS 14+) was basically a lucky accident (modulo #5672). I don't recall considering platform support as a factor in using a Format.JS message with the plural syntax.

In that compatability table from MDN, I see a sub-feature of Intl.PluralRules that it looks like won't be available before iOS 15.4. That's Intl.PluralRules.prototype.selectRange(), and thankfully it doesn't seem to be used (rg selectRange node_modules/\@formatjs/ comes up empty), because we don't yet require iOS 15.4+.

@DanWaLes
Copy link
Author

DanWaLes commented Mar 3, 2023

normally apps which aren't compatible with my device will get a message saying that they require a higher version of iOS to update or be installed. installing updates for Zulip does not have that message.
how much features require > iOS 12? if it's just the button text i'd suggest fixing it. if it's anything more then the app store should prevent the updates and installation from being possible.
Apple still gives updates to iPhone 5s. they dropped iPhone 5 a while back

@gnprice
Copy link
Member

gnprice commented Mar 4, 2023

normally apps which aren't compatible with my device will get a message saying that they require a higher version of iOS to update or be installed. installing updates for Zulip does not have that message.

Yes. That is our mistake, as I mentioned above: it's #5672.

how much features require > iOS 12? if it's just the button text i'd suggest fixing it.

It's this and several other places where we show some text in the UI that involves a number. Older versions of the app would show one form of text when the number was 1 and another when it was more than 1 (like "1 unread message" and " unread messages"). But this makes it impossible to translate accurately for many languages — for example in some languages, the noun should look the same for 11 and 21 and 101 and so on as it does for 1.

(A bit like how in English, when the number is an ordinal, one uses "th" for 4, 5, 6, and so on, but then when the number reaches 21 one repeats the "st/nd/rd" pattern: 21st, 22nd, 23rd, just like 1st, 2nd, 3rd.)

It's possible there may be other features too. We don't have the resources to test the app on very old versions of iOS, so when things break there we don't necessarily find out.

Apple still gives updates to iPhone 5s. they dropped iPhone 5 a while back

Yeah, they're continuing to provide bugfix and security updates for it, by continuing the iOS 12.x series. That's great, and I wish more phone vendors would do so for as long a period as Apple does. But they're no longer providing new major versions, which means you don't get the new functionality of iOS 13 and later.

@DanWaLes
Copy link
Author

DanWaLes commented Mar 4, 2023

so will i be left with a button with incorrect text forever, seeing as the direction is to only support iOS 14+?

@gnprice
Copy link
Member

gnprice commented Mar 4, 2023

I think we'll make one last release for iOS 12+ that just rolls back these pieces of text, so that devices on iOS 12 and 13 can go back to something that works for them.

Then future releases will be iOS 14+ and will have the text that works for more languages.

@DanWaLes
Copy link
Author

DanWaLes commented Mar 4, 2023

Thanks

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Mar 9, 2023
@chrisbobbe
ios: Re-support iOS 12 by removing translatable plurals in UI strings
04b1627 
This is the change we intend to cherry-pick as a v27.202 stable
release atop v27.201, to address two failures of ours:

- When the UI started including FormatJS messages with this
  plural-handling syntax (35129e5 / cc53dd5), we didn't realize
  that, on iOS 12, they would render in a way that's not meant to
  face users, with characters like "}" in them. That's issue zulip#5670.
- When we intended to de-support iOS 12 and 13 (783e56b), we
  didn't actually do so; the App Store still offered the app for iOS
  12 and 13. That's issue zulip#5672, fixed in `main` (e93d47f /
  PR zulip#5673).

So, this cherry-pick release will be the last one that supports iOS
12 and 13, and the text will be human-friendly for users stuck on
those old iOS versions.

To keep it simple, just use English text for everybody. Without the
new plurals feature, a grammatically correct translation will be
impossible in some languages anyway; see discussion:
  zulip#5670 (comment)

Fixes: zulip#5670
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Mar 10, 2023
@chrisbobbe
ios: Re-support iOS 12 by removing translatable plurals in UI strings
f838a35 
This change is intended for a stable release atop v27.201, to fix a
pair of mistakes we made:

- When the UI started including FormatJS messages with this
  plural-handling syntax (35129e5 / cc53dd5), we didn't realize
  that, on iOS 12, they would render in a way that's not meant to
  face users, with characters like "}" in them. That's issue zulip#5670.
- When we intended to de-support iOS 12 and 13 (783e56b), we
  didn't actually do so; the App Store still offered the app for iOS
  12 and 13. That's issue zulip#5672, fixed in `main` (e93d47f /
  PR zulip#5673).

So, this release will be the last one that supports iOS 12 and 13,
and the text will be human-friendly for users stuck on iOS 12.

To keep it simple, just use English text for everybody. It would be
better to use even flawed translations for the messages, but it
would be hard to wrangle Transifex (or a Git merge) into giving us
the right mix of v27.201's translations for everything else and the
older translations for those two since-deleted strings "1 unread
message" and "{unreadCount} unread messages".

Fixes: zulip#5670
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Mar 11, 2023
@chrisbobbe
ios: Cherry-pick change to enforce iOS 14+ requirement
3fb3826 
This cherry-picks e93d47f, which landed in `main` as a fix for
issue zulip#5672 ("App Store thinks we support iOS 12 when we don't").

This is planned for a v27.203 release, to follow the recent v27.202
release, which fixed the iOS 12 bug zulip#5670 after we realized our
mistake in letting zulip#5672 happen.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Mar 11, 2023
@chrisbobbe
ios: Revert iOS 12 bugfix from v27.202 release
bb8543b 
This reverts commit f838a35, which fixed the iOS 12 bug zulip#5670.
That bugfix involved a tradeoff: it disabled translation on three
user-facing messages (one being the prominent "X unreads" message in
`UnreadNotice`), replacing them with hard-coded strings in English.

That was an improvement for iOS 12 users, since they were affected
by zulip#5670. But in the previous commit, we cherry-picked our commit
from `main` that enforced our intended iOS 14+ requirement. So,
while iOS 12 users can still get the zulip#5670 fix in the v27.202
release (as the last release available to them), now we can make a
followup release v27.203 that puts the translations back, and won't
clobber iOS 12 users' installs with a version that has the zulip#5670
bug.
@chrisbobbe
Copy link
Contributor

This was fixed in f838a35, released in v27.202.

@DanWaLes, please update to v27.202; you should find that the message is English instead of nonsense, and that it tells you how many unreads there are. 🙂

Closing as fixed, but if there's an issue with the fix, please let us know and we can reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@gnprice @chrisbobbe @DanWaLes