UI set away status

[borisyankov]

Adds a Profile tab and allows setting user's away status and status text.

[borisyankov]

Ready.

[gnprice]

Thanks @borisyankov !

Add OwnAvatar component

This comment still applies: #3267 (comment)

Add AwayStatusSwitch component

+type Props = {|
+  ...PropsFromState,
+  dispatch: Dispatch,
+|};

• On another PR we discussed this name PropsFromState, which should be something else.

• I'm also curious what the intended use and meaning of the type is exactly. The dispatch prop is also provided by connect... so if this gets called something like PropsFromConnect, then it should probably go there. Also if the idea is that the props mentioned explicitly in Props will correspond to props the caller can, or should, pass in, then dispatch doesn't belong in that list.

• Separately: this in the commit message is good information that I think would be best captured in jsdoc on the component:

     * retrieves the current user's `user status` data and presents it
     * allows by switching it to control the `away` status

Add ProfileCard component

Similarly, this would be good to capture as jsdoc on the component:

    This is very similar to `AccountDetails` but used to show the
    current users account.

Rework AccountDetails to be more generic

Move "Send private message" out of AccountDetails.
This allows us to reuse AccountDetails in ProfileCard but not
show the "Send private message" button that is not appropriate there.

• Hmm interesting. Shouldn't this go before we create ProfileCard, then? Or at least before we wire it up in the navigation.

• Also this looks in the code like it will change the layout -- the button was previously inside a ComponentList. Haven't yet compared visually.

AccountDetails: show user status text if one is set

+    // backgtoundColor: 'red',

• You'll want to take this out, presumably.

  (Also it's misspelled.)

+    const statusText = userStatus[user.user_id] && userStatus[user.user_id].status_text;
[...]
+          {statusText !== undefined && (
+            <RawLabel style={[styles.largerText, componentStyles.statusText]} text={statusText} />
+          )}

• This logic feels to me like it's calling for a (very simple) selector. Best if the component doesn't have to deal with the nuances of what it means for the whole object to be missing from the map, vs. the object to be present but have undefined for status_text.

  E.g. getUserStatusText: (state: GlobalState, userId: number) => string | void, making use of the kind of signature discussed in Fix caching in "for narrow" selectors #3015.

Add UserStatusScreen

    Values are imported from a separate file as Prettier does not
    'like' unicode and messes up the formatting.

Huh indeed!

Less euphemistically: Prettier has a bug in handling Unicode. 😛

I suspect that it's specifically about non-BMP code points, which includes most emoji -- those are the code points that in JavaScript (and Java) if you're not careful you end up treating as if they're two code points. Like the RN bug that underlay #2787.

Searching their tracker, this looks a lot like prettier/prettier#770. ... But that was supposedly fixed in Prettier 1.3.0, and we're on 1.12.1. Hm.

Anyway:

• At a minimum, there should be a comment inside that tiny file saying why it's there -- it's a peculiar way for the code to be, and so it should provide an answer to the reader's natural question.

• Better, let's be good citizens and do something about the issue 😉 . Looks like there are newer releases of Prettier -- see if the issue goes away on upgrade? If not, file a bug with Prettier upstream.

    Possible presets are given for a one-click entry.

Good idea!

+  getInitialStatusText = () => {
+    const { selfUserDetail, userStatus } = this.props;
+    const selfUserStatus = userStatus[selfUserDetail.user_id];
+    return (selfUserStatus && selfUserStatus.status_text) || '';
+  };

Another use case for that selector 🙂

+  handleStatusTextUpdate = () => {
+    const { dispatch } = this.props;
+    const { statusText } = this.state;
+    dispatch(updateUserStatusText(statusText));
+    dispatch(navigateBack());
+  };
+
+  handleStatusTextClear = () => {
+    const { dispatch } = this.props;
+    dispatch(updateUserStatusText(''));
+    dispatch(navigateBack());
+  };

Hmm -- perhaps the "clear" case should change the text to '', and then call the "update" case?

+  handleStatusTextChangeText = (statusText: string) => {
+    this.setState({
+      statusText,
+    });
+  };

This is a bit confusing -- it's basically named after one call site, the onChangeText of the input, but then we use it in another place too where onChangeText is definitely not what just happened.

Perhaps it'd be clearer to name it after what it does? E.g. setStatusTextState. In particular, if it weren't for the unfortunate issue of identity across re-renders, I think we wouldn't have a name for this at all and would just write e.g. statusText => this.setState({ statusText }).

... Hmm, in fact it looks like you are already giving up identity across re-render!

            onPress={() => {
              this.handleStatusTextChangeText(statusSuggestion);
            }}

If you can fix that reasonably, that'd be good.

If you can't... might as well at least make it clearer to read, by writing the setState directly.

+export default [
+  '📅 In a meeting',
+  '🚌 Commuting',
+  '🤒 Out sick',
+  '🌴 Vacationing',
+  '🏠 Working remotely',
+];

• These should probably get translated. See LanguagePicker (since Issue #2611. settings: Build a better UI for choosing language #3231) for an example of how to do that cleanly.

  Hmm, more precisely: I think in your current version, OptionButton will try to translate them for the labels, but the actual value entered will be untranslated. (And no translation will happen anyway because they're not in the messages list.) Anyway, we should translate them.

• Separately: some of these feel like they ought to go with an "away" state. But this UI is no more awkward than the webapp seems to be for this, so shrug.

Add Set a status button to ProfileCard

Backticks mean code 🙂 . You want double-quotes, I think.

Also there's an unclosed backtick in the commit-message body. Small but it sticks out at me.

Add getAvatarFromUser and getAvatarFromMessage

Good idea!

• What about in Avatar? Very similar logic there too.

-            avatarUrl={message.avatar_url || getGravatarFromEmail(message.sender_
email)}
+            avatarUrl={getAvatarFromMessage(message)}

• This code differs slightly from old to new: in the old version, an avatar_url of '' would fail over to getGravatarFromEmail. Was that case impossible? Or, is the new behavior the right thing in that case anyway? Deserves a few words in the commit message.

• Also, that would have been impossible in the first place if that file had @flow strict-local. A good followup would be to go and make it that way now. Looks like there's just one obstacle to that, and it looks like an easy one.

[borisyankov]

• On another PR we discussed this name PropsFromState, which should be something else.

That was written at the time when I wrote the other commit. We have agreed on PropsFromConnect but also it does not matter for this commit so I reverted it to the usual Props.

[borisyankov]

Hmm -- perhaps the "clear" case should change the text to '', and then call the "update" case?

We close the screen after the button press, so that step is not needed.

[borisyankov]

About this: #3267 (comment)

I did simplify the OwnAvatar even further, removing the shape as prop.
After refactoring the avatar/gravatar logic in a later commit it is almost as simple as just using Avatar (maybe 1 line of code difference). We need the component to get the data out of the user anyway.

Also, currently we don't have a way to not show the PresenceStatusIndicator in Avatar but we don't want it in OwnAvatar.

These are the reasons that justify this component's existence for me.

[borisyankov]

As for whether Avatar is needed, I am not sure, maybe we can refactor it later indeed.

Perhaps that TextAvatar path in Avatar never gets used at all?

It is currently used in TitleGroup. It makes sense to make the usage in that file and in GroupPmConversationItem the same. There is some (minor) value in abstracting away how an avatar is rendered by just using Avatar in both places.

[borisyankov]

Searching their tracker, this looks a lot like prettier/prettier#770. ... But that was supposedly fixed in Prettier 1.3.0, and we're on 1.12.1. Hm.

I did update to the latest one 1.16.4 and tried it, the same result.

[borisyankov]

• Hmm interesting. Shouldn't this go before we create ProfileCard, then? Or at least before we wire it up in the navigation.

I did move it before the creation of ProfileCard and moved the changes to this file to its creation.

• Also this looks in the code like it will change the layout -- the button was previously inside a ComponentList. Haven't yet compared visually.

It doesn't change visually.

[borisyankov]

I did create several user status selectors (with docs) and did rework the other commits to use them.

[borisyankov]

One key thing that's still open is the UserStatusScreen portion of this UI issue:

The new ProfileCard and UserStatusScreen have a problem with vertical overflow -- they can be taller than the screen, and then they don't seem to scroll. [... in UserStatusScreen] the buttons get covered up by the keyboard, which makes the UI extremely confusing unless you already know (or guess) that they're hiding there and try closing the keyboard.

I did rework the UserStatusScreen - now it uses a FlatList that only takes as much of the screen as there is space. Also, the buttons are now always on screen. That seems to work quite well on smaller screens.

For ProfileCard I replaced the View with a ScrollView and that is enough to fix the now rare case that we need to scroll this screen (after making sure in previous changes to not add extra space)

I was thinking of this comment, which is about code in Lightbox (quoted above the comment):

This code differs slightly from old to new: in the old version, an avatar_url of '' would fail over to getGravatarFromEmail. Was that case impossible? Or, is the new behavior the right thing in that case anyway? Deserves a few words in the commit message.

Supposedly impossible. We either have an actual avatar url or null.

Also, that would have been impossible in the first place if that file had @flow strict-local. A good followup would be to go and make it that way now. Looks like there's just one obstacle to that, and it looks like an easy one.

I think we have @flow strict-local now or we are talking about different files?

Other items that potentially we can leave for followups, but would be good to take care of:

In the pre-supplied values for UserStatusScreen:

These should probably get translated. See LanguagePicker (since #3231) for an example of how to do that cleanly.
Hmm, more precisely: I think in your current version, OptionButton will try to translate them for the labels, but the actual value entered will be untranslated. (And no translation will happen anyway because they're not in the messages list.) Anyway, we should translate them.

We should ship as-is but will do this in subsequent commit.

In "Redesign AccountDetails to fit screens better": it'd be good to write "200" just once, as it's an arbitrary size that it's important to keep matching between the two places we mention it. Just a local variable like const avatarSize = 200 would do nicely, I think.

Done.

About the Prettier issue in UserStatusScreen:

Better, let's be good citizens and do something about the issue 😉 . Looks like there are newer releases of Prettier -- see if the issue goes away on upgrade? If not, file a bug with Prettier upstream.

Tested with the new version and it wasn't fixed.
But I did try many variations in order to reproduce a minimal verrsion for a bug report.
A breakthrough! It seems the default eslint/prettier settings do not cause the issue.
I'll continue testing to see what is the exact thing that causes it.

[gnprice]

Thanks again @borisyankov for these revisions!

On UserStatusScreen, I'm still seeing the buttons obscured by the keyboard, and no scroll. This is
with commit b32103e (the one you pushed a few minutes ago), with a FlatList there. Screenshot:

[gnprice]

Huh, the plot thickens! It's not 100% of the time -- here's another screenshot, where the buttons are nicely visible:

And the suggestions list scrolls.

That's what I saw immediately after switching apps back to Zulip, from the system settings where I was browsing around for some details.

I'll poke a bit more. Perhaps you also have ideas?

[gnprice]

Hmm, now it mostly doesn't reproduce, in either of two emulators. I'm not sure what the pattern is.

Some observations and tentative patterns:

• I observed it several times after reloading in the dev popup menu, and after re-running react-native run-android to reinstall a current build of the APK. So that rules out some ways it could have been just stale code from a different version of the branch.
  • But, at some point the Metro bundler (react-native start) crashed and I restarted it... and I think I haven't seen it since then. So possibly the bundler was serving stale code? ... alternately with serving good code? Grr, if so.
• I think I only ever saw it when navigating to UserStatusScreen was the very first time the keyboard showed up since starting the app. That could be a clue.
• The keyboard often behaved funny, as it sometimes does in debug builds in the emulator.
  • E.g., I'd hit r r to reload... and that would bring up the keyboard, and it'd stay up when the app loaded and showed the main nav screen, even though the keyboard makes no sense there.
  • So it's possible whatever bug this was only applies in the emulator and/or in a debug build.

[gnprice]

Cool, I'm going to go ahead and merge -- 🤞 that that puzzling symptom won't recur in the wild. If it does, we'll debug it then.

---

Thanks again for the revisions!

I think you have several followup items, some of them quick -- and you mentioned some in your last comment.

To the question in your last comment:

I think we have @flow strict-local now or we are talking about different files?

I meant Lightbox.js. Which doesn't yet, so this is one open followup item.

[borisyankov]

I meant Lightbox.js. Which doesn't yet, so this is one open followup item.

Oh yeah, sure, will do that next.

Working on a few follow-up commits, but seeing the agreed changes have not made it to the web yet, we would be fine for that release without them (different icon for 'unavailable' etc.)

[gnprice]

And merged! Thanks again for all your work on this.

I went and split up a couple of the commits that needed splitting. In one case, one of the needed commits was basically one of the commits in @jainkuniya 's #3362... so I cherry-picked that commit here 😝 .

There are various follow-up items mentioned in the thread above, and there are some further cleanups to the avatar logic that I have in some terse notes locally but I don't think I've described. I'll try to collect all those followups in a comment after this one, so they don't fall through the cracks. (I know you're already working on some of them.)

And then I'll make a new release with this change!

[gnprice]

Follow-up items, with details in various comments above (Ctrl+F is effective after you tell GitHub to expand the middle of the thread; that widget in turn can be found by Ctrl+F for "hidden item"):

• Lightbox strict-local
• Translations for pre-supplied values for UserStatusScreen
• File a bug with Prettier upstream for that Unicode (perhaps specifically non-BMP) issue

---

Other follow-up items to clean up the *Avatar* components:

• A number of props are redundantly described across several components out of UserAvatar, UserAvatarWithPresence, and OwnAvatar; in types, jsdoc, and defaults. I think this is the root cause of a number of specifics mentioned below.
  • Fortunately, this is a solvable problem! OwnAvatar and UserAvatarWithPresence can defer to UserAvatar for these details, getting things DRY.
  • For examples, see the various *Input* components that wrap Input. In particular InputWithClearButton, PasswordInput, and commits a15c446 and 31523e1 on the late MultilineInput.
• Absolute vs. potentially-relative URLs:
  • UserAvatar takes absolute only, while UserAvatarWithPresence takes potentially-relative. They should be made consistent, one way or the other -- pushing the call to getAvatarUrl either inward to UserAvatar or outward to callers.
  • Inward makes OwnAvatar still simpler, and it better aligns UserAvatar's interface with its name. So I think that's probably preferable.
  • Also the jsdoc on UserAvatar falsely says it takes potentially-relative.
• Defaults for shape:
  • Redundant defaults to rounded at several layers -- should be at just one, similar to size in this thread.
  • Divergent defaults, to rounded and circle. There's no reason showing the self-user's own avatar should inherently be associated with a different shape from showing another user's -- so the choice of circle is better put at the caller.
    • (With the *Input* technique mentioned above for typing a wrapper component, it becomes basically zero-boilerplate for OwnAvatar to offer the shape prop, which helps here.)
• Type for shape: it's written as string on the inner component, but really has a more specific type.
• Defaults of bogus empty-string values, in UserAvatarWithPresence for avatarUrl, email, and realm.

---

Other follow-up items:

• The logic in getMediumAvatar is fairly messy. For one thing -- why is there a //g option on that regexp? That seems like if it had any effect it would be a bug.
• realm_message_retention_days: ?number in InitialDataRealm can't be quite right; see below.
• I plan to make a pass over the various jsdoc involved in this branch for some small style adjustments.
• I'll do a tx push and pull; that will conveniently sync the new strings into all the languages' message catalogs, as well as present them to our kind translators.

---

Other comments:

• In one of today's revisions I noticed the commit that narrowed the avatar_url types changed from saying string | void to saying string | null -- because it turns out the server sends null. That is good information that I'm glad we learned! That's the kind of detail that I hope to learn and record when making the effort to write down good specific types. 🙂

  • Hmm, in fact that reminds me of a general point: there is no undefined in JSON. There's null, and there's a property just not existing. So any time we have a ?foo or a foo | void in our types for data we get from the server, that's almost certainly mistaken.
  • At a quick grep, I see just one remaining example of that: realm_message_retention_days: ?number in InitialDataRealm. So that'd be good to clear up too.

• Thanks for this description:

  What is funny, we still needed it to have the state be mutated before the updateStatusText call.
  The state mutations are batched so that could have been expected.
  I wasn't sure setTimeout guarantees us the state would be mutated, though I think it does.
  I also experimented with setState and a second parameter being a call to updateStatusText - that is now guaranteed to be after the mutation.
  Instead of keeping this, I reworked the code a bit to be more straight-forward and not require that deep knowledge of how state mutations work.

  This all makes sense! And I like the resulting code.

• Thanks for this change in AccountDetails! I appreciated the additional cleanup:

  I did split this into two commits.
  The new one is called:
  'AccountDetails: Use getAvatarFromUser and realm within'

  If you look back I think you'll see that that the split I was asking for was something different and independent:

  The commit is a bit confusing because it silently makes a mostly-unrelated (though also good) change: it cuts where we'd been showing the presence indicator overlaid on the avatar.

  So I took care of that split before I merged today. That was the commit I took from accountDetails: Do not show presenceStatus indicator in avatar. #3362 . 😉

[gnprice]

Oh, one other small cleanup that this is likely a good time for, which I just happened to see in the codebase:

$ rg -iA1 fixme.*realmbot
src/title/TitlePrivate.js
21:    // $FlowFixMe: sort out RealmBot
22-    const avatarUrl: string | null = user.avatar_url;

src/pm-conversations/PmConversationList.js
53:            // $FlowFixMe: sort out RealmBot
54-            const avatarUrl: string | null = user.avatar_url;

In reality I think cross-realm bots have avatars? But we're always falling back to gravatars for them. Seems the data is getting misplaced somewhere.