UserItem: Remove UserItemRaw hack #5508

chrisbobbe · 2022-09-27T10:23:18Z

Before this, we were co-opting UserItemRaw to represent wildcard mentions @all and @everyone in the people autocomplete, pretending that both of those was somehow a "user item".

Stop doing that, and make a more dedicated UI for these, with some small improvements:

While we're at it, make some improvements:

- Show the Font Awesome bullhorn icon, like the web app.
- Re: the explanatory text, previously "(Notify everyone)":
  - It now says "Notify stream" when composing a stream message, and
    "Notify recipients" when composing a PM, following the web app.
  - It doesn't have parentheses anymore. The web app uses them to
    distinguish the explanatory text from the special string that
    triggers the mention, as in "all (Notify stream)". We already do
    that by putting the explanatory text on a new line, in a smaller
    font.
  - It's now wired up for translation.
- We now show "stream", not just "all" and "everyone", fixing #4325.
- A wildcard mention will now show up if your search term matches
  the translation of it in your chosen language.
- We now use shared code (`typeahead.query_matches_string`)
  for match-modulo-diacritics logic.

Screenshots below.

(edit: description updated to reflect current revision.)

Fixes: #4325

chrisbobbe · 2022-09-27T10:25:40Z

Before/after:

alya · 2022-09-27T17:03:08Z

Hm, on the webapp we have both the icons and the text aligned -- I wonder if we should try aligning the text here as well, even though it'll leave some extra space?

chrisbobbe · 2022-10-05T03:03:06Z

Yeah, I agree. Can we try shrinking the user avatars down to the size of the icons? That would also shrink the height of the user rows, letting us show more of them at a time.

alya · 2022-10-05T19:10:48Z

Sure, seems like a fine direction to try -- thanks!

gnprice

Thanks @chrisbobbe for fixing this! Generally looks great; comments below.

gnprice · 2022-10-27T17:30:02Z

src/outbox/outboxActions.js

-      getUsersAndWildcards(state.users),
+      [
+        // TODO: Refactor so we don't make these fake user objects.
+        { user_id: makeUserId(-1), full_name: 'all', email: '(Notify everyone)' },
+        { user_id: makeUserId(-2), full_name: 'everyone', email: '(Notify everyone)' },
+        ...state.users,


outboxActions [nfc]: Inline getUsersAndWildcards call We're about to remove this hacky function's only caller; inline it here, making sure to keep a comment that says we don't like the hack.

"only other caller", perhaps? Seems like there was this caller and also another one.

I've updated this commit to inline the function at both callers and remove the function.

gnprice · 2022-10-28T00:22:26Z

src/users/__tests__/userHelpers-test.js

-    const shouldMatch = [
-      { user_id: -1, full_name: 'all', email: '(Notify everyone)' },
-      someGuyUser,
-    ];


Huh, it's interesting that these test cases only had one wildcard in the expected results, not two, while the logic we're deleting in getUsersAndWildcards was adding two pseudo-users. The search query in this test case is just '', matching everything. How does the old code pick just one of the two wildcard pseudo-users?

… Oh. I see. It's this:

const candidates = getUniqueUsers([...startWith, ...contains, ...matchesEmail]);

… where:

export const getUniqueUsers = ( users: $ReadOnlyArray<AutocompleteOption>, ): $ReadOnlyArray<AutocompleteOption> => uniqby(users, 'email');

… and it's relying on the fact that the two pseudo-users have the same very fake "email" value as each other, namely "(Notify everyone)". Yikes. Great to be deleting that.

I guess we still need the getUniqueUsers call, because the other thing it's doing is deduping users that matched several different ways.

Though one thing we can now do is switch it to use the user ID rather than email… no, better: it should just use === on the user objects.

gnprice · 2022-10-28T19:34:15Z

src/common/UserAvatarWithPresence.js

+    // Callers don't make sure that the user is one we actually have data
+    // for: e.g., we're helping identify a PM narrow by showing one of the
+    // PM recipients, but that recipient's user record doesn't exist because
+    // it was deleted after the conversation started.
+    // TODO: Instead of null, show a legible placeholder so the user knows
+    //   how to interpret the lack of data. Or make callers do that, and
+    //   insist that userId refers to a real user, so we can use
+    //   getUserForId instead of tryGetUserForId.


Same remark as here: #5531 (comment)

gnprice · 2022-10-28T19:41:10Z

src/autocomplete/WildcardMentionItem.js

+export const serverCanonicalStringOf = englishCanonicalStringOf;
+
+export const descriptionOf = (


nit: can drop exports

gnprice · 2022-10-28T19:56:07Z

src/autocomplete/WildcardMentionItem.js

+export const getFilteredSuggestions = (
+  query: string,
+  destinationNarrow: Narrow,
+  _: GetText,
+): $ReadOnlyArray<WildcardMentionType> =>
+  Array.from(WildcardMentionType.members()).filter(type =>


The old code would return just one of @-all or @-everyone, if both matched. That seems helpful, since they're synonyms.

I guess web shows both of them, plus @-stream (which I think is also a synonym -- user docs seem to not mention it.) Still, taking up multiple spots in the autocomplete list -- especially on a mobile screen -- seems unhelpful and worth avoiding.

Sure, I can have it take the first match in [all, everyone, stream]. Is that a good order, or should we take the first match in [stream, all, everyone] or something else? (We'd exclude "stream" when the destination narrow isn't a stream or topic narrow, of course.)

If I change the code so it just shows one match, I see an odd thing where "all" shows up as the only wildcard-mention suggestion, when my actual query was "stream". That's because "all" matches by its descriptionOf(type, …), which is _('Notify stream'). We should probably prevent that by removing the description search.

Sounds good to me.

The help center mentions @-all, sometimes also @-everyone, and never @-stream that I can see:
https://zulip.com/help/mention-a-user-or-group
https://zulip.com/help/pm-mention-alert-notifications
So I think the right order of preference is [all, everyone, stream].

gnprice · 2022-10-28T20:21:12Z

src/autocomplete/WildcardMentionItem.js

+    /* $FlowFixMe[prop-missing] - Type query_matches_source_attrs. In
+       particular, find a way to require `source` to have string values at
+       the keys given in `match_attrs`. */
+    typeahead.query_matches_source_attrs(


I think probably the right answer here is actually to dissolve this typeahead.query_matches_source_attrs function. It's over-abstracted already, and the trickiness of its type isn't even the biggest improvement the code would get by getting rid of it.

I wrote up a longer discussion showing how the call sites would improve, then realized that would be best turned into a quick PR to actually make said changes:
zulip/zulip#23375

Thanks! I've published that change to NPM and my next revision uses typeahead.query_matches_string instead of typeahead.query_matches_source_attrs.

…e ways See zulip#5508 (comment)

chrisbobbe · 2022-11-03T20:29:07Z

Thanks for the reviews! Revision pushed. Here's the "after" photo for this revision, and I expect main still looks like the "before" photo in #5508 (comment).

gnprice

Thanks for the revision! I've just read all the changes except in the one main commit:

e49119c PeopleAutocomplete: Stop using fake user-like objects for wildcard mentions

Everything looks good except (that one commit I haven't read yet and) one comment below, which calls for some more refactoring in the emoji data.

Looking at the list of commits, I think this series would be good to break out as its own quick PR:
cddc2f9 UserGroupItem [nfc]: Inline styles.listItem here
70d50de UserItem [nfc]: Inline globalStyles.listItem here
0fe1cd7 UserItem [nfc]: Remove repetition in styles.wrapper
03c99c5 people autocomplete: Make users and user groups more uniform
05668e3 PeopleAutocomplete: Give user and user-group items 48+ px touch target
It looks all ready to merge, and it has a bit of a different job from the rest of the branch -- it's making the layout/design improvements to the autocomplete popup that were discussed above at #5508 (comment) .

gnprice · 2022-11-03T23:41:36Z

src/emoji/emojiSelectors.js

  getActiveImageEmojiById,
  emojis =>
    Object.keys(emojis).map(id => ({
+      reaction_type: 'realm_emoji',
      emoji_type: 'image',


This will be inaccurate in the case of the Zulip extra emoji :zulip:, where the reaction_type should be zulip_extra_emoji. This doesn't currently have a practical effect, because typeahead.get_emoji_matcher only looks to see if the reaction type is unicode_emoji or not; but it could in the future, because realm_emoji vs. zulip_extra_emoji have completely different meanings for emoji_code.

Looking at how this data is built up, I think a good solution would be:

The objects we get from getActiveImageEmojiById should have a reaction_type property.

Similarly for the objects we pass around between the other selectors in this file.

The type we have for those is ImageEmojiType, which is defined in modelTypes and used to describe parts of the API. So we can split that type:

The existing definition can be better named RealmEmoji. In the API, it's always used for a realm emoji, not a Zulip extra emoji. (See 9d78140, which in retrospect should have made this split instead of renaming.)

Then ImageEmoji can be the name of a type that just adds reaction_type: ReactionType to that.

(In emojiSelectors we're already creating new objects anyway, to parse source_url. So we might as well add reaction_type while we're at it.)

The changes to add reaction_type can happen in a commit or two of their own prior to taking the shared upgrade. The inexact object type in typeahead.js.flow reflects the fact that the shared code isn't bothered by additional properties.

…e ways See zulip#5508 (comment)

chrisbobbe · 2022-11-12T02:04:40Z

Thanks for the review! Revision pushed, but this one isn't for review; I'm just pausing after separating out that other PR before doing the data fix you point to in #5508 (comment). I've taken the following screenshots at this point, since I don't expect them to be affected by that fix.

Before/after:

With ImageEmoji being the same as RealmEmoji except for an added reaction_type property. See discussion: zulip#5508 (comment)

…e ways See zulip#5508 (comment)

chrisbobbe · 2022-11-14T19:27:58Z

OK, and now I've pushed a fix for the emoji data issue you pointed out. Thanks; this is ready for review. 🙂

gnprice

Thanks for the revision! Looks good -- small comments below.

gnprice · 2022-11-15T04:02:15Z

src/types.js

-export type EmojiForShared = {| emoji_type: EmojiType, emoji_name: string, emoji_code: string |};
+export type EmojiForShared = {|
+  reaction_type: ReactionType,


This change, and the other changes it drives, should be possible to put in a commit that comes before the @zulip/shared upgrade, so that the latter is purely a dependency upgrade.

That's reflected in the inexact object type in that library's type definitions:
#5508 (comment)

gnprice · 2022-11-15T04:05:24Z

src/autocomplete/WildcardMentionItem.js

@@ -0,0 +1,198 @@
+/* @flow strict-local */


PeopleAutocomplete: Stop using fake user-like objects for wildcard mentions As the comments point out, we've been using a hack where we make fake "user" objects for wildcard mentions @all and @everyone, so we can enlist UserItemRaw to show items for these in the people autocomplete.

nit: do disarm these @-mentions so that GitHub doesn't interpret them:
https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#mentioning-people

One convenient form is "@-all" / "@-everyone".

- We now show "stream", not just "all" and "everyone", fixing #4325.

nit: https://github.com/zulip/zulip-mobile/blob/main/docs/style.md#fixes-format

(Fine to have it in prose here, but please also include the regular form -- makes it easier to spot when scanning the commit message)

nit: do disarm these @-mentions so that GitHub doesn't interpret them

Ooh, good catch. These didn't cross my mind as problematic; I guess either GitHub recognizes "all" and "everyone" in some special way, or maybe I've accidentally at-mentioned people with those strings as their usernames (oops!).

Yeah, I was curious and so I expanded those commits where they appear in the main PR page here on GitHub, and hovered on the mentions -- they do in fact get recognized as mentions, of these organizations:
https://github.com/all
https://github.com/everyone

The latter has a logo for its avatar, and a description:

We are Everyone, a digital communications agency based in Glasgow, Scotland.

Both of them appear to be ordinary non-special organizations, though as it happens not individuals.

gnprice · 2022-11-15T04:12:19Z

src/autocomplete/PeopleAutocomplete.js

      ),
    }: Section<UserGroup>),
+    ({
+      data: wildcardMentionForQuery != null ? [wildcardMentionForQuery] : [],


It looks like we're not actually using in this component the fact that there's just one of these, right?

In that case, likely simpler to have the helper function return the list, and have it just be an internal fact about that function that it only ever returns at most one item.

alya · 2022-11-15T06:27:43Z

I've taken the following screenshots at this point, since I don't expect them to be affected by that fix.

I'm not following the details of the discussion, so sorry if this was already mentioned, but the "all" looks shifted to the left compared to everything else in the "after" screenshot.

gnprice · 2022-11-15T18:11:34Z

I've taken the following screenshots at this point, since I don't expect them to be affected by that fix.

I'm not following the details of the discussion, so sorry if this was already mentioned, but the "all" looks shifted to the left compared to everything else in the "after" screenshot.

Yeah, that part was fixed by #5551. I believe the "after" of this PR, now that #5551 is merged, will look just like it did in the previous revision:
#5508 (comment)

gnprice · 2022-11-15T18:15:25Z

(FWIW @chrisbobbe for next time when splitting out part of a PR that's readier for merge, my suggestion would be: move that part to the start of the branch, send it as its own PR, but keep the main PR based on top of that so that the tip of the PR still has all the changes. At least in a case like this one where the split-out changes are needed in order to get the final screenshot to look right, or more generally where the changes in the main PR depend on those in the split-out PR.)

chrisbobbe · 2022-11-15T20:06:26Z

(FWIW @chrisbobbe for next time when splitting out part of a PR that's readier for merge, my suggestion would be: move that part to the start of the branch, send it as its own PR, but keep the main PR based on top of that so that the tip of the PR still has all the changes. At least in a case like this one where the split-out changes are needed in order to get the final screenshot to look right, or more generally where the changes in the main PR depend on those in the split-out PR.)

Sure, in retrospect I think that would've made sense in this case. Although, this time you did say "break out" the commits as a new PR, in part because they had a "different job from the rest of the branch". 😛

With ImageEmoji being the same as RealmEmoji except for an added reaction_type property. See discussion: zulip#5508 (comment)

…e ways See zulip#5508 (comment)

chrisbobbe · 2022-11-16T01:17:11Z

Thanks for the review! Revision pushed.

gnprice · 2022-11-17T02:13:26Z

OK, this all looks good! With just one commit-message nit:

shared: Upgrade to 0.0.16

This brings a nice change to the typeahead code: the overabstracted
function query_matches_source_attrs is removed, and
query_matches_string is exported instead:
  https://github.com/zulip/zulip/pull/23375

In zulip/zulip#23412, released in 0.0.16, we corrected the type for
`typeahead.get_emoji_matcher` to reflect that the code assumes a
reaction_type property on emoji objects. So, update our
EmojiForShared type to include that property.

That last paragraph can just be deleted, I think -- it's describing a change that now occurs in the previous commit, and is described there. I guess the latter doesn't mention the specific zulip/zulip PR, so you could move that information there.

I would say I'll just fix that up and merge, but this is a rare occasion when I have urgent work: #5545, where the server side is actively being iterated on right now and we want to get both server and client side merged tonight for tomorrow's Zulip Server 6.0 release. So instead, please merge at will after handling the above comment.

With ImageEmoji being the same as RealmEmoji except for an added reaction_type property. See discussion: zulip#5508 (comment)

The relevant Flow definition in @zulip/shared's typeahead.js.flow will soon change, when we take a @zulip/shared upgrade, so that reaction_type will be required (see zulip/zulip#23412). There's no problem adding it to EmojiForShared before taking the upgrade, though, since the @zulip/shared definition is an inexact object, meaning it isn't bothered by additional properties.

This brings a nice change to the typeahead code: the overabstracted function query_matches_source_attrs is removed, and query_matches_string is exported instead: zulip/zulip#23375

@zulip/shared implemented this in zulip/zulip@21778, and it was released (with a needed Flow-type fix) in 0.0.16, which we just upgraded to in the previous commit. From reading code, I believe this doesn't change the emoji-matcher behavior that we offer to the user, so marking NFC.

Making sure to keep comments saying we don't like these hacky fake user objects.

We'll use this soon so we can follow the web app in describing @-all / @-everyone in the PeopleAutocomplete as "Notify stream" or "Notify recipients", according to the type of conversation narrow (topic or PM).

…ntions As the comments point out, we've been using a hack where we make fake "user" objects for wildcard mentions @-all and @-everyone, so we can enlist UserItemRaw to show items for these in the people autocomplete. Clear out that hack and make a new UI component for showing wildcard mentions in the autocomplete. While we're at it, make some improvements: - Show the Font Awesome bullhorn icon, like the web app. - Re: the explanatory text, previously "(Notify everyone)": - It now says "Notify stream" when composing a stream message, and "Notify recipients" when composing a PM, following the web app. - It doesn't have parentheses anymore. The web app uses them to distinguish the explanatory text from the special string that triggers the mention, as in "all (Notify stream)". We already do that by putting the explanatory text on a new line, in a smaller font. - It's now wired up for translation. - We now show "stream", not just "all" and "everyone", fixing zulip#4325. - A wildcard mention will now show up if your search term matches the translation of it in your chosen language. - We now use shared code (`typeahead.query_matches_string`) for match-modulo-diacritics logic. Fixes: zulip#4325

This type saved us the trouble of faking every last boring detail of a whole UserOrBot object, in the world where we used fake user-like objects for wildcard mentions. We stopped using those in the previous commit, so we don't need this type anymore, and we can just say we're passing around whole UserOrBot objects because that's what we're doing.

…e ways See zulip#5508 (comment)

chrisbobbe · 2022-11-17T02:35:24Z

Done, thanks for the review!

With ImageEmoji being the same as RealmEmoji except for an added reaction_type property. See discussion: zulip#5508 (comment)

…e ways See zulip#5508 (comment)

chrisbobbe requested review from alya and gnprice September 27, 2022 10:23

chrisbobbe mentioned this pull request Oct 28, 2022

presence: Replace "unavailable" status with "invisible mode" #5531

Merged

gnprice reviewed Oct 28, 2022

View reviewed changes

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Nov 3, 2022

user autocomplete: Use uniq to deduplicate items that match multipl…

60715e9

…e ways See zulip#5508 (comment)

chrisbobbe force-pushed the pr-remove-user-item-raw branch from 05b6cfe to 60715e9 Compare November 3, 2022 20:27

gnprice reviewed Nov 4, 2022

View reviewed changes

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Nov 12, 2022

user autocomplete: Use uniq to deduplicate items that match multipl…

7745673

…e ways See zulip#5508 (comment)

chrisbobbe force-pushed the pr-remove-user-item-raw branch from 60715e9 to 7745673 Compare November 12, 2022 02:01

This was referenced Nov 12, 2022

Fix layout issues in user autocomplete #5551

Merged

@stream doesn't appear in the autocomplete list #4325

Closed

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Nov 14, 2022

user autocomplete: Use uniq to deduplicate items that match multipl…

f853559

…e ways See zulip#5508 (comment)

chrisbobbe force-pushed the pr-remove-user-item-raw branch from 7745673 to f853559 Compare November 14, 2022 19:27

gnprice reviewed Nov 15, 2022

View reviewed changes

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Nov 16, 2022

user autocomplete: Use uniq to deduplicate items that match multipl…

9d748a0

…e ways See zulip#5508 (comment)

chrisbobbe force-pushed the pr-remove-user-item-raw branch from f853559 to 9d748a0 Compare November 16, 2022 01:14

chrisbobbe added 10 commits November 16, 2022 18:29

emoji types: Split ImageEmojiType into RealmEmoji and ImageEmoji

a992902

With ImageEmoji being the same as RealmEmoji except for an added reaction_type property. See discussion: zulip#5508 (comment)

shared: Upgrade to 0.0.16

7315ad5

This brings a nice change to the typeahead code: the overabstracted function query_matches_source_attrs is removed, and query_matches_string is exported instead: zulip/zulip#23375

userHelpers [nfc]: Inline getUsersAndWildcards

14185e9

Making sure to keep comments saying we don't like these hacky fake user objects.

PeopleAutocomplete [nfc]: Plumb destinationNarrow for wildcard mentions

33a6e3d

We'll use this soon so we can follow the web app in describing @-all / @-everyone in the PeopleAutocomplete as "Notify stream" or "Notify recipients", according to the type of conversation narrow (topic or PM).

UserItem [nfc]: Remove last use of UserItemRaw and dissolve

06b74ae

user autocomplete: Use uniq to deduplicate items that match multipl…

2d73026

…e ways See zulip#5508 (comment)

chrisbobbe force-pushed the pr-remove-user-item-raw branch from 9d748a0 to 2d73026 Compare November 17, 2022 02:34

chrisbobbe merged commit 2d73026 into zulip:main Nov 17, 2022

chrisbobbe deleted the pr-remove-user-item-raw branch November 17, 2022 02:35

BrandonNgoranNtam pushed a commit to BrandonNgoranNtam/zulip-mobile that referenced this pull request Nov 22, 2022

user autocomplete: Use uniq to deduplicate items that match multipl…

c754ffb

…e ways See zulip#5508 (comment)

		export const serverCanonicalStringOf = englishCanonicalStringOf;

		export const descriptionOf = (

Uh oh!

UserItem: Remove UserItemRaw hack #5508

UserItem: Remove UserItemRaw hack #5508

Uh oh!

Conversation

chrisbobbe commented Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Sep 27, 2022

Uh oh!

alya commented Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alya commented Oct 5, 2022

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Nov 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Nov 14, 2022

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alya commented Nov 15, 2022

Uh oh!

gnprice commented Nov 15, 2022

Uh oh!

gnprice commented Nov 15, 2022

Uh oh!

chrisbobbe commented Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Nov 16, 2022

Uh oh!

chrisbobbe commented Sep 27, 2022 •

edited

Loading

alya commented Sep 27, 2022 •

edited

Loading

chrisbobbe commented Oct 5, 2022 •

edited

Loading

chrisbobbe commented Nov 3, 2022 •

edited

Loading

chrisbobbe commented Nov 12, 2022 •

edited

Loading

chrisbobbe commented Nov 15, 2022 •

edited

Loading