ui: Use "channel" or "stream" based on server feature level #5830

chrisbobbe · 2024-03-05T02:06:52Z

Done at the level of TranslationProvider, which (conveniently) can
access the feature level of the active account if any.

Fixes: #5827

gnprice

Thanks @chrisbobbe for taking care of this! Glad to see this is a lot cleaner (thanks to the idea you described on Friday and implemented here) than I was originally imagining.

The changes all look good modulo the TODOs you have, and a few small things below.

gnprice · 2024-03-05T03:21:09Z

static/translations/messages_en.json

  "stream": "stream",
+  "channel": "channel",


nit in commit message:

The messages_en.json changes were done with a handy Perl one-liner that Greg helped me work out: perl -i -ne 'print; print if (s/stream/channel/g || s/Stream/Channel/g)' static/translations/messages_en.json

The command can be made a bit more readable by breaking across multiple lines. Two ways to do that:

A single-quoted string (such as the argument to -e here) can have newlines in it. And Perl, like JS, has a syntax that generally isn't affected by newlines or indentation.

A line in a shell command can end in a backslash \ (outside of any quoted string), and in that case both the backslash and the newline right after it are effectively deleted so that the next line forms more of the same command.

Here, I might write:

$ perl -i -ne ' print; print if (s/stream/channel/g || s/Stream/Channel/g); ' static/translations/messages_en.json

Cool; thanks!

gnprice · 2024-03-05T03:25:35Z

src/autocomplete/WildcardMentionItem.js

-const englishCanonicalStringOf = (type: WildcardMentionType): string => {
+const englishCanonicalStringOf = (
+  type: WildcardMentionType,
+  channelTerminologyUsed: boolean,


nit: perhaps useChannelTerminology

When I first saw this name, I read it as indicating which "channel terminology" is being used. Then I was puzzled it was a boolean instead of some enum: which way is encoded as true, and which as false?

Then I realized what you actually mean is whether "channel terminology" is being used.

Also "used" sounds like "has been used" as much as "is being used", and here this is really about telling this code what it should use rather than informing it about what has been used (or even is being used elsewhere). So an imperative works well.

Interesting; yeah, I agree. Thanks!

gnprice · 2024-03-05T03:29:12Z

src/autocomplete/WildcardMentionItem.js

  const handlePress = useCallback(() => {
-    onPress(type, serverCanonicalStringOf(type));
-  }, [onPress, type]);
+    onPress(type, serverCanonicalStringOf(type, channelTerminologyUsed));


Hmm, I guess this makes one spot where the switch is actually changing a request we make to the server! Not only the UI we show to the user.

It'd be good to flag this in… #api design, I guess. The server should make sure that it starts accepting the new form, either at the same time as the web UI switches over or before then. No harm in accepting it early — it can keep accepting the old form, and probably should do so for a good while into the future anyway — so probably it should go ahead and do so early, to reduce the risk of forgetting and having it do so only later.

(If the server were tardy in starting to accept the new @channel form for wildcard mentions, we could always deal, but it'd make the conditionals here more complicated.)

Ah yeah, makes sense. I started a discussion in #api design: https://chat.zulip.org/#narrow/stream/378-api-design/topic/stream.2Fchannel.20rename.3A.20.40stream.20wildcard.20mention/near/1752174

gnprice · 2024-03-05T03:34:00Z

src/boot/streamChannelRenamesMap.js

+ * instead of
+ *
+ *   "Notify stream": "Notify stream",
+ *
+ * and likewise for all the other languages.


Suggested change

* instead of

*

* "Notify stream": "Notify stream",

*

* and likewise for all the other languages.

* instead of

*

* "Notify stream": "Notify stream",

* "Notify channel": "Notify channel",

*

* and likewise for all the other languages.

I think that makes it a bit easier to see how the mapping is all happening on the message-ID side, and not the translated-string side.

gnprice · 2024-03-05T03:43:14Z

src/boot/streamChannelRenamesMap.js

+export const streamChannelRenamesMap: {| [string]: string |} = {
+  stream: 'channel',
+  'Notify stream': 'Notify channel',
+  'Who can access the stream?': 'Who can access the channel?',


FWIW here's the command I used just now to help check this — a variation on the Perl one-liner that edited the messages JSON file:
<static/translations/messages_en.json perl -lne 'next unless (my ($k) = /^ *(".*"): \1/); $_ = $k; next unless (s/stream/channel/g || s/Stream/Channel/g); print "$k: $_,"'

I took that command's output, copy-pasted it between the braces of this object literal, deleted what had been here, and reformatted. The result was identical to what you have… except it added a line

'No messages in topic: {streamAndTopic}': 'No messages in topic: {channelAndTopic}',

which indeed shouldn't be there.

It also occurs to me that that {streamAndTopic} string might be worth including but commented out, as a bit of a reminder of why this map isn't just a replaceAll or the like.

gnprice · 2024-03-05T03:50:30Z

src/boot/TranslationProvider.js

+          || /en[-$]/.test(language)
+          || messageId === message


Suggested change

|| /en[-$]/.test(language)

|| messageId === message

|| /^en($|-)/.test(language)

|| messageId === message

The version with [-$] will match only if there's an actual $ character, which I think isn't what you intended. (It ends up doing the same thing because for language en the messageId === message condition will always hold. But clearer when the regexp part already does what that part is meant to do.)

Also I think you only want to match from the beginning (with ^), if there's somehow another match. (E.g. because of a three-letter language name, which some less-common languages have.)

gnprice · 2024-03-05T03:59:01Z

src/boot/TranslationProvider.js

+        if (
+          renamedMessage !== renamedMessageId
+          || /en[-$]/.test(language)
+          || messageId === message
+        ) {
+          // Prefer using `renamedMessage`, the one with "channel", when:
+          // - It doesn't match its American-English message ID, thanks to
+          //   our kind volunteer translators. Or:
+          // - It does match its American-English message ID, but:
+          //   - It's still appropriate to show because the UI is set to
+          //     English. Or:
+          //   - The message with "stream" isn't translated either. Might as
+          //     well show the "channel" one even though it'll be in English.
+          return [messageId, renamedMessage];
+        } else {
+          // Otherwise, use `message`, the one with "stream".
+          return [messageId, message];
+        }


The bodies of these conditionals are thankfully quite short and clear. So it may make the logic easier to read if we break the conditions up a bit more, even if it means duplicating the bodies some.

Here's one possibility:

if (…) { // The newfangled "channel" string hasn't been translated yet, // but the older "stream" string has. Consider falling back to that. if (…) { // The language is a variety of English. Prefer the newer terminology, // even though awaiting translation. (Most of our strings don't change at all // between one English and another.) return …; } // Use the translation we have, even of the older terminology. // (In many languages the translations have used an equivalent // of "channel" all along anyway.) return …; } return …;

chrisbobbe · 2024-03-06T23:27:59Z

Thanks for the review! Revision pushed.

chrisbobbe · 2024-03-06T23:28:39Z

And converted to draft, to help prevent accidentally merging this without doing the TODOs in the commit message.

gnprice · 2024-03-06T23:58:40Z

Thanks! All looks good modulo those TODOs.

I think you can go ahead and fill in the todo-server comments with a reference to 9.0. This is one issue that seems pretty solidly like we're going to make it part of the 9.0 release. But then the other two just mean we're waiting for the server-side changes to be ready before we can merge.

I guess one thing that would be good, since we're waiting, is to get the "channel" strings into Transifex so that people can translate them whenever they get to doing some Zulip translation. So perhaps pull out a commit that just adds to messages_en.json without yet adding the logic that uses the new strings, and then we can merge that and do tools/tx-sync.

chrisbobbe · 2024-03-07T21:14:57Z

Makes sense! Done; revision pushed.

gnprice · 2024-03-07T23:57:18Z

Thanks! All LGTM.

I'll go ahead and merge that first commit:
50c9656 i18n: Offer UI strings with "channel" terminology to translators
and run tools/tx-sync.

Then the remaining changes are just waiting for the server-side changes to be ready, so that we know what feature level to condition on (and can confirm that accepting @**channel** happens before then, so that just one threshold feature level suffices).

gnprice · 2024-04-05T17:13:15Z

The server change to accept @**channel** was merged:

wildcard-mentions: Add "channel" as a stream wildcard option. zulip#29311

So that will indeed happen before the other server-side changes; and this PR is now just waiting for those other server-side changes to land so we can determine the feature-level threshold.

gnprice · 2024-04-25T22:20:51Z

The server-side UI changes have landed — the feature level to use is 255:
https://chat.zulip.org/api/changelog#changes-in-zulip-90

So this is now ripe to update for merge.

(It looks like the main commit's commit message has a couple of elements that would have belonged instead in the commit 50c9656 that we merged earlier; ah well.)

chrisbobbe · 2024-04-26T00:09:13Z

(It looks like the main commit's commit message has a couple of elements that would have belonged instead in the commit 50c9656 that we merged earlier; ah well.)

Ah yeah, the Perl command?

chrisbobbe · 2024-04-26T00:11:03Z

Thanks for the bump! Revision pushed, using that feature level, and I've unmarked as draft.

This explicitness will help us avoid a shadowed variable soon.

Done at the level of TranslationProvider, which (conveniently) can access the feature level of the active account if any. Fixes: zulip#5827

gnprice · 2024-04-26T00:26:57Z

Thanks! LGTM; merging.

Ah yeah, the Perl command?

Yeah, exactly — the bit about updating messages_en.json.

Oops; this was missed because I didn't do end-to-end testing before we merged zulip#5830.

chrisbobbe requested a review from gnprice March 5, 2024 02:06

gnprice reviewed Mar 5, 2024

View reviewed changes

chrisbobbe force-pushed the pr-stream-channel-rename branch from deb6019 to 1dfc3e7 Compare March 6, 2024 23:27

chrisbobbe marked this pull request as draft March 6, 2024 23:28

chrisbobbe force-pushed the pr-stream-channel-rename branch from 1dfc3e7 to 51e1370 Compare March 7, 2024 21:14

gnprice mentioned this pull request Apr 24, 2024

autocomplete: Support wildcard @-mentions zulip/zulip-flutter#234

Open

chrisbobbe force-pushed the pr-stream-channel-rename branch from 51e1370 to a5ed7dc Compare April 26, 2024 00:10

chrisbobbe marked this pull request as ready for review April 26, 2024 00:10

chrisbobbe added 2 commits April 25, 2024 17:25

i18n [nfc]: s/messages/messagesByLanguage

4bbf876

This explicitness will help us avoid a shadowed variable soon.

ui: Use "channel" or "stream" based on server feature level

7c35635

Done at the level of TranslationProvider, which (conveniently) can access the feature level of the active account if any. Fixes: zulip#5827

gnprice force-pushed the pr-stream-channel-rename branch from a5ed7dc to 7c35635 Compare April 26, 2024 00:26

gnprice merged commit 7c35635 into zulip:main Apr 26, 2024
1 check passed

chrisbobbe deleted the pr-stream-channel-rename branch April 26, 2024 00:33

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Apr 26, 2024

ui: Condition UI-string "channel" terminology on >=255, not >255

3b85a50

Oops; this was missed because I didn't do end-to-end testing before we merged zulip#5830.

chrisbobbe mentioned this pull request Apr 26, 2024

ui: Condition UI-string "channel" terminology on >=255, not >255 #5864

Merged

neiljp mentioned this pull request Jun 2, 2024

Migrate from 'Stream' to 'Channel' naming zulip/zulip-terminal#1510

Open

8 tasks

gnprice mentioned this pull request Oct 8, 2024

Button Label Inconsistent with naming of Channels and Streams #5898

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ui: Use "channel" or "stream" based on server feature level #5830

ui: Use "channel" or "stream" based on server feature level #5830

chrisbobbe commented Mar 5, 2024 •

edited

Loading

gnprice left a comment

gnprice Mar 5, 2024

chrisbobbe Mar 5, 2024

gnprice Mar 5, 2024

chrisbobbe Mar 5, 2024

gnprice Mar 5, 2024

gnprice Mar 5, 2024

chrisbobbe Mar 5, 2024

gnprice Mar 5, 2024

gnprice Mar 5, 2024

gnprice Mar 5, 2024

gnprice Mar 5, 2024

gnprice Mar 5, 2024

chrisbobbe commented Mar 6, 2024

chrisbobbe commented Mar 6, 2024

gnprice commented Mar 6, 2024

chrisbobbe commented Mar 7, 2024

gnprice commented Mar 7, 2024

gnprice commented Apr 5, 2024

gnprice commented Apr 25, 2024

chrisbobbe commented Apr 26, 2024

chrisbobbe commented Apr 26, 2024 •

edited

Loading

gnprice commented Apr 26, 2024

ui: Use "channel" or "stream" based on server feature level #5830

ui: Use "channel" or "stream" based on server feature level #5830

Conversation

chrisbobbe commented Mar 5, 2024 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Mar 6, 2024

chrisbobbe commented Mar 6, 2024

gnprice commented Mar 6, 2024

chrisbobbe commented Mar 7, 2024

gnprice commented Mar 7, 2024

gnprice commented Apr 5, 2024

gnprice commented Apr 25, 2024

chrisbobbe commented Apr 26, 2024

chrisbobbe commented Apr 26, 2024 • edited Loading

gnprice commented Apr 26, 2024

chrisbobbe commented Mar 5, 2024 •

edited

Loading

chrisbobbe commented Apr 26, 2024 •

edited

Loading