Skip to content

Set user status #1701

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.

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Set user status #1701

wants to merge 13 commits into from

Conversation

sm-sayedi
Copy link
Collaborator

@sm-sayedi sm-sayedi commented Jul 10, 2025

Support for setting the user status.

Rebased on top of #1755, starting from: 88270a8 button: Add ZulipMenuItemButton.subLabel

Figma design: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=5000-52611&t=ZkYDTwsnzsgZJRYr-0

Profile Page

No status set Full status set
Only emoji set Only text set

Set Status Page

No status set - Light No status set - Dark
Full status set - Light Full status set - Dark

Screen recordings

Set status - Success

Set.status.-.Success.mov

Set status - Failure

Set.status.-.Error.mov

Fixes: #198

@sm-sayedi sm-sayedi force-pushed the 198-set-user-status branch from 0d852c1 to 242f877 Compare July 10, 2025 01:15
@gnprice
Copy link
Member

gnprice commented Jul 10, 2025

Great!

Let's make this two PRs: one for the remaining portion of #1629 (i.e. same commits as #1699), and one for setting status. That will help keep the reviews more organized and will help us merge the first part earlier.

@sm-sayedi
Copy link
Collaborator Author

Sure, on it.

@sm-sayedi sm-sayedi force-pushed the 198-set-user-status branch 3 times, most recently from 23cdd3a to f67c992 Compare July 10, 2025 02:42
"@setStatus": {
"description": "The status button label in self-user profile page when status is not set."
},
"noStatusText": "Not status text",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"noStatusText": "Not status text",
"noStatusText": "No status text",

@alya
Copy link
Collaborator

alya commented Jul 10, 2025

The screenshots look good to me, other than the small note above!

@sm-sayedi sm-sayedi marked this pull request as draft July 10, 2025 14:01
@sm-sayedi sm-sayedi force-pushed the 198-set-user-status branch 4 times, most recently from 1405501 to 1ca2ceb Compare July 16, 2025 18:00
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for building this! Comments below; and I see your PR description has a TODO for tests, which I believe is the only reason this is currently marked as a draft.

I've read through the first 5 commits:
a604e22 button: Add ZulipMenuItemButton.subLabel
e3b091a profile: Add button for setting/showing user status in self-user profile
a90f83e model [nfc]: Add UserStatusChange.copyWith method
0104cd1 content: Add emoji property to UserStatusEmoji widget
649232a emoji: Make emoji picker return the selected emoji, for reuse

and part of the 6th:
1ca2ceb user-status: Add page for setting own user status

Comment on lines 405 to 410
children: [
Text(label,
style: const TextStyle(fontSize: 20, height: 24 / 20)
.merge(weightVariableTextStyle(context, wght: _labelWght()))),
if (subLabel != null)
Text.rich(subLabel!,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when there isn't room for both of these? (Always a question with a Row.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching. They would overflow. Fixed in the new revision, with both of them taking half the space and ellipsized.

class _SetStatusButton extends StatelessWidget {
const _SetStatusButton({required this.userId});

final int userId;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is always the self-user ID, right? (Otherwise, it wouldn't make sense to set status.)

Seems clearest to not take it as a parameter, then; this widget's build method can look it up for itself.

@@ -809,6 +809,62 @@
"@userRoleUnknown": {
"description": "Label for UserRole.unknown"
},
"status": "Status",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This key is too general — there could easily be several places where we use the same string "Status" in English, and then the right translations might not be the same in some languages.

Instead, can say:

Suggested change
"status": "Status",
"statusButton": "Status",

Comment on lines 816 to 818
"setStatus": "Set status",
"@setStatus": {
"description": "The status button label in self-user profile page when status is not set. Also, title for the page where user status is set."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This "also" is a good sign that this should be two separate strings 🙂 even though they happen to have the same value in English.

For the first… perhaps statusButtonAction.

For the second, setStatusPageTitle. (See examples of other "…PageTitle" strings.)

Comment on lines 836 to 838
"userStatusBusy": "Busy",
"@userStatusBusy": {
"description": "Label for one of the suggested user statuses with status text 'Busy', in setting user status page."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't so much a label as a suggested actual value for the status. (If the user chooses it, it'll set their status text to the value of this string, right? Not to the English "Busy".)

So:

Suggested change
"userStatusBusy": "Busy",
"@userStatusBusy": {
"description": "Label for one of the suggested user statuses with status text 'Busy', in setting user status page."
"userStatusBusy": "Busy",
"@userStatusBusy": {
"description": "A suggested user status text, 'Busy'."

Comment on lines 40 to 41
class _SetStatusPageState extends State<SetStatusPage> {
List<UserStatus> _statusSuggestions(ZulipLocalizations localizations) => [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: put this method down below the state fields and the lifecycle methods; it's basically part of the work of the build method, so put it next to that

Comment on lines 180 to 184
padding: const EdgeInsets.only(
// In Figma design, this is 16px, but we compensate for that in
// the icon button below.
left: 8,
top: 8, right: 10,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left/right should be start/end (so it works correctly in RTL locales)

Comment on lines 212 to 214
: icon!;
},
child: Icon(ZulipIcons.smile, size: 24)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This icon doesn't really play the role of "child", since it commonly won't even appear at all.

The small optimization that comes from using ValueListenableBuilder.child here can be matched (and exceeded) by using const:

Suggested change
: icon!;
},
child: Icon(ZulipIcons.smile, size: 24)),
: const Icon(ZulipIcons.smile, size: 24);
}),

Expanded(child: TextField(
controller: statusTextController,
minLines: 1,
maxLines: 2,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this different from omitting minLines?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, interesting. I looked at the doc on minLines before making my comment, and it seemed pretty unclear on this point, but didn't see those bits of the doc on maxLines.

It looks like the key logic in the implementation is this:
https://github.com/flutter/flutter/blob/e2441683275879df1ac0311e02ff868410599ab1/packages/flutter/lib/src/rendering/editable.dart#L2425-L2433

    final double preferredHeight = switch (maxLines) {
      null => math.max(_textPainter.height, preferredLineHeight * (minLines ?? 0)),
      1 => _textPainter.height,
      final int maxLines => clampDouble(
        _textPainter.height,
        preferredLineHeight * (minLines ?? maxLines),
        preferredLineHeight * maxLines,
      ),
    };

So when maxLines is non-null, the effective default for minLines is that it equals maxLines. When maxLines is null, though, the effective default for minLines is 0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting piece of code. However, I think that, at least for me, it didn't make sense the first time to know that I had to specify minLines, for maxLines to work as the maximum number of lines.😀

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah agreed — the behavior is a bit nonintuitive, and the docs aren't super clear about it.

Comment on lines 206 to 209
final emoji = switch(change.emoji) {
OptionNone<StatusEmoji?>() => oldStatus.emoji,
OptionSome<StatusEmoji?>(:var value) => value,
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
final emoji = switch(change.emoji) {
OptionNone<StatusEmoji?>() => oldStatus.emoji,
OptionSome<StatusEmoji?>(:var value) => value,
};
final emoji = change.emoji.or(oldStatus.emoji);

That's equivalent, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, indeed; thanks!

@sm-sayedi sm-sayedi force-pushed the 198-set-user-status branch 2 times, most recently from 764b856 to ea1d711 Compare July 17, 2025 23:24
@sm-sayedi
Copy link
Collaborator Author

Thanks @gnprice for the review. New revision pushed. Please have a look.

@sm-sayedi sm-sayedi requested a review from gnprice July 17, 2025 23:34
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the revision! Comments below.

Like in the previous round above, I've read the first N-1 commits:
88f8299 button: Add ZulipMenuItemButton.subLabel
03d3999 button [nfc]: Make ZulipMenuItemButton.onPressed optional
f65ee25 profile: Add button for setting/showing user status in self-user profile
5d50ee5 model [nfc]: Add UserStatusChange.copyWith method
c9b5972 content: Add emoji property to UserStatusEmoji widget
4feb05b emoji [nfc]: Make emoji picker return the selected emoji, for reuse

and part of the last:
ea1d711 user-status: Add page for setting own user status

@@ -393,13 +395,29 @@ class ZulipMenuItemButton extends StatelessWidget {
foregroundColor: _labelColor(designVariables),
splashFactory: NoSplash.splashFactory,
).copyWith(backgroundColor: _backgroundColor(designVariables)),
overflowAxis: Axis.vertical,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm interesting. What's the effect of this? I'm finding its doc a bit opaque 🙂

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally, the child widget of MenuItemButton would overflow horizontally, even if it's a simple widget like Text with a longer string. Setting overflowAxis: Axis.vertical wraps the child in an Expanded widget internally, thus preventing the overflow.

https://github.com/flutter/flutter/blob/440713c3b2878048be2661a6705b219c51f72c95/packages/flutter/lib/src/material/menu_anchor.dart#L2821-L2858

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would "overflow horizontally" mean concretely?

I guess the other part I'm trying to understand is: the argument sounds like it means the button now overflows vertically, instead of horizontally. What does that look like?

Copy link
Collaborator Author

@sm-sayedi sm-sayedi Jul 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By "overflow horizontally", I meant that it shows that striped pattern:
Screenshot 2025-07-19 at 8 53 07 PM
And when overflowAxis: Axis.vertical is set, it will expand vertically:
Screenshot 2025-07-19 at 8 56 41 PM
Now Text.overflow will also work:
Screenshot 2025-07-19 at 11 01 41 PM

For more context, here's the upstream PR that added MenuItemButton.overflowAxis: flutter/flutter#143932

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks. The name still seems confusing to me (since the two behaviors don't seem at all symmetric), but that explanation is helpful.

It looks like this comment was the origin of the name that got merged: flutter/flutter#143932 (comment)
but the previous drafts already had the "direction" naming that doesn't seem to match what it really does.

Expanded(child: TextField(
controller: statusTextController,
minLines: 1,
maxLines: 2,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah agreed — the behavior is a bit nonintuitive, and the docs aren't super clear about it.

Comment on lines 91 to 92
final values = [
(localizations.userStatusBusy, '1f6e0', 'working_on_it'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the data we have from the server is missing one of these emoji, I think better to leave it out from the suggestions than to try to fall back to a hard-coded name. It's likely in that case that the hard-coded name won't work either.

(continuing from #1701 (comment))

Comment on lines 106 to 107
emojiName: store.allEmojiCandidates()
.firstWhereOrNull((e) => e.emojiCode == emojiCode)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this iterates through the whole list of all known emoji, repeatedly for each of the suggestions we want to offer. Let's avoid doing that. 🙂

Instead, have a prep commit add a little method to EmojiStore to offer the API we need in order to do this efficiently. I think that can look like String? getUnicodeEmojiNameByCode(String emojiCode).

controller: statusTextController,
minLines: 1,
maxLines: 2,
maxLength: 60,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah interesting. Where does this max length come from?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: The limit on the size of the message is 60 characters.

It's in the updateStatus API docs, and I forgot to include it in the previous revisions.😀

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, I see. I guess let's have a short comment here with that link. That way it's clear why that's there and has the value it has; and, crucially, it's easy for someone reading in the future to check whether 60 is still the right answer then.

textCapitalization: TextCapitalization.sentences,
style: TextStyle(fontSize: 19, height: 24 / 19),
decoration: InputDecoration(
counterText: '', // TODO: should we show counter?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a question for #mobile-design :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks. This comment can link to that, then.

physics: AlwaysScrollableScrollPhysics(), // TODO: necessary?
padding: EdgeInsets.symmetric(vertical: 6),
child: Column(children: [
for (final status in statusSuggestions(context))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's pull this out as a local variable: final suggestions = statusSuggestions(context);, and then this iterates through that local.

That way the call to this helper method is a bit more visible when reading this build method's logic.

Comment on lines +37 to +39
/// https://zulip.com/api/update-status
Future<void> updateStatus(ApiConnection connection, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add API bindings in a separate commit

Comment on lines 37 to 41
/// https://zulip.com/api/update-status
Future<void> updateStatus(ApiConnection connection, {
required UserStatus status,
}) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the server API, both the text and the emoji are optional — either can be omitted and then only the other one will be set. So UserStatusChange seems like a closer fit for that.

@sm-sayedi sm-sayedi force-pushed the 198-set-user-status branch from ea1d711 to 60c9baf Compare July 19, 2025 19:46
@sm-sayedi
Copy link
Collaborator Author

Thanks @gnprice for the review. New revision pushed.

@sm-sayedi sm-sayedi requested a review from gnprice July 19, 2025 19:49
@sm-sayedi sm-sayedi force-pushed the 198-set-user-status branch from 60c9baf to bef6e76 Compare July 19, 2025 19:56
@gnprice
Copy link
Member

gnprice commented Jul 22, 2025

Thanks! Those changes all look good.

As you mention in the description, this PR still needs some tests. :-) There's also more for me to read in the final/main commit.

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, and I've finished reading all but the tests in the last commit. Comments below.

The only behavior change that these comments call for is in the size of the candidate emoji. So I plan to include this in today's release; I'll add a squashable commit to fix that emoji size, unless you're online and have made a revision by then.

Comment on lines 53 to 56
// Ignore updating [statusChange] for the additional updates with the
// same value from TextField. For example, when a character is deleted.
if (text == newStatus.text) return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this comment. When a character is deleted, that's a new value for the text, right?

I believe the point here is for when something other than the text changes: namely the selection, or the composing range.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a character is deleted, the listener is called twice with the same value. But the point you mentioned is also an important case for that. So I will include it in the comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. My guess would be that when that happens, it's because one of those other properties changed too — and the listener is getting one call for the text changing, and another for the selection or the composing range changing. (Or perhaps one of the latter is changing at the same time as the text, then promptly changing back.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this makes sense.

Comment on lines +71 to +75
// 3. This listener is called as a result of the change in status
// text field.
if (text == statusTextController.text) return;

statusTextController.text = text;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yeah, this structure is doing something a bit delicate — there's two ChangeNotifiers here that both have a copy of the same data, namely the text the user has entered in the text field, and they're updating each other in both directions. So it ends up needing to carefully avoid an infinite loop of updating each other.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version works, and it's not necessary to rework it. But FWIW I think my solution here would have been:

  • Give each piece of data a single source of truth, responsible for notifying listeners on changes; the text lives in the TextEditingController, so that's what things listen to for updates to the text.
  • Then for the emoji, put it directly on the state and use setState when updating.

The main thing this design is accomplishing relative to that one is that it avoids rebuilding the overall page when the text or emoji changes, instead using ValueListenableBuilder on the few smaller widgets that actually need to update. Those rebuilds are probably OK — I think this build method isn't doing anything all that expensive — but it's true that it's nice to avoid them.

A variation of my above solution which would accomplish that is:

  • For the emoji, give it its own ValueNotifier (and don't call setState on updates).
  • Then to avoid having to double-wrap with ValueListenableBuilder (for both the text and emoji), say something like emojiAndTextListenable = Listenable.merge([statusTextController, statusEmoji]) to make one listenable for both pieces, and use ListenableBuilder with that.

Comment on lines 90 to 92
final values = [
(localizations.userStatusBusy, '1f6e0'),
(localizations.userStatusInAMeeting, '1f4c5'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for nicely aligning these columns. One trick that can make that simpler: when one of the columns is all the same width, like the emoji codes here, put that column first. Then the subsequent columns are naturally aligned.

(Or when one of the columns is nearly a constant width, put it first, and then less spacing is required to align the next column.)

Comment on lines 154 to 155
Option<T> toOption<T>({required T recent, required T old}) =>
recent == old ? OptionNone() : OptionSome(recent);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"to option" seems much more general than what this is really about — the meaning of this is specific to the "change" concept found in UserStatusChange.

How about asChange?

);
}

Option<T> toOption<T>({required T recent, required T old}) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"recent" doesn't feel right for this parameter — it suggests this is a value that has already become the actual value, and in fact has maybe been replaced by some other value since then.

The right name would be "new". That's a keyword, but could call this new_ — compare TopicNarrow.with_.

I think it'd actually make most sense for it to be positional: it's the direct argument which this is interpreting, and the other argument is more of a background fact. So (including my other comment about the method's own name):

Suggested change
Option<T> toOption<T>({required T recent, required T old}) =>
Option<T> asChange<T>(T new_, {required T old}) =>

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I was first writing this method, I wanted to use new_, but I thought it may not look good as a named parameter. But now that you suggested it to be a positional parameter, looks good.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah.

FWIW I think even with the name visible to callers, it'd be better to have a name like new_ that has the right word with a bit of funny-looking punctuation than a name like recent where the word doesn't mean the right thing.

Comment on lines 159 to 153
final designVariables = DesignVariables.of(context);
final localizations = ZulipLocalizations.of(context);
final suggestions = statusSuggestions(context);

return Scaffold(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this is a different kind of thing from the steps above (they're quick O(1) data lookups, while this is doing a bit of computation of its own), so set it off with a blank line:

Suggested change
final designVariables = DesignVariables.of(context);
final localizations = ZulipLocalizations.of(context);
final suggestions = statusSuggestions(context);
return Scaffold(
final designVariables = DesignVariables.of(context);
final localizations = ZulipLocalizations.of(context);
final suggestions = statusSuggestions(context);
return Scaffold(

Comment on lines +197 to +189
// In Figma design, this is 4px, be we compensate for that in
// [SingleChildScrollView.padding] below.
bottom: 0),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm but that doesn't do the same thing, does it? Once you scroll, that other padding scrolls away.

Why not do this the same way as in the design?

… Ah, the point is that there's also an InsetShadowBox for those same 6px. Which requires the scroll view to handle the padding; and also means that even when scrolled, the shadow keeps the scroll content visually separated from the text field.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the Figma design, there is in fact no shadow. But I thought it may look better when scrolled, especially that now we use the shadow in multiple places in UI. 🙂

textCapitalization: TextCapitalization.sentences,
style: TextStyle(fontSize: 19, height: 24 / 19),
decoration: InputDecoration(
counterText: '',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

include a 1- or 2-line comment with a link to the thread about this (per #1701 (comment))

top: 6, bottom: 6,
color: designVariables.mainBackground,
child: SingleChildScrollView(
physics: AlwaysScrollableScrollPhysics(), // TODO: necessary?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How might a future reader resolve this TODO — what are you uncertain about in figuring out whether this is necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a better place for me to write that comment was in a GitHub comment. What I meant was that in most devices, the suggestions list occupies a portion of the screen height, so it doesn't provide that nice scroll animation when dragged. So will it be better to make it always scrollable?! We have a somewhat similar case in the "See who reacted" sheet, where if there is only one reactor, when the list is dragged, it has the scroll animation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Yeah, I agree, a GitHub comment is a good place for that question.

And then now that we're here in a GitHub subthread, I think actually the ideal place for it is in #mobile-design 🙂 — that way Alya or Vlad or others can readily see it and chime in.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

child: Row(
spacing: 8,
children: [
UserStatusEmoji(emoji: status.emoji!, size: 19),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be 24px in size, according to the Figma design.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In particular that makes these candidate emoji the same size as the chosen emoji at the top. See the "Full status set" screenshots, where these are smaller, and compare with the corresponding screen in Figma, where they're the same size.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, in Figma the box size of those emojis is 24px. But looking at the "Typography" section of the "Properties" panel, there are two properties with different values: Size: 19px and Line height: 24px. The same property values are for the actual text of status suggestions. So I thought I would use 19px as the size of the emoji, as that's what we did in other parts of UI where status emoji is shown, so we gave it the same size as the text it appeared with.

Screenshot 2025-07-26 at 4 49 36 AM

And for the chosen emoji in the top, in the before-current revision of the Figma design, it was 24px, the same as the "smile" placeholder icon. But in the new revision, the placeholder icon stayed 24px, and the selected emoji size became 19px. But even then, I preferred to use the previous 24px size for the emoji too, to align it with the placeholder icon. 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so UserStatusEmoji.size (and UnicodeEmojiWidget.size) specify the size the emoji's box will have in the layout. That corresponds to the 24x24 square in that screenshot, so size: 24.

The "Typography" section says "Size: 19px" referring to a font size. But the font size to use to get the desired box size depends on the font. See the implementation of UnicodeEmojiWidget — it'll use a font size of 24px with Apple Color Emoji, and (14.5/17) * 24px =~ 20.5px with Noto Color Emoji (so on iOS and Android respectively). I'm not sure what font is being used for the emoji when Figma renders the design.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. With a 24px size for the status suggestion emoji, it fills the 24x24 box entirely, unlike the design in Figma, which is smaller than that due to size: 19px.
Screenshot 2025-07-26 at 6 55 55 AM

In other places of UI where we show emoji, we used the font size of the neighboring text for the emoji size, instead of its line height. For example:

Flexible(
child: Text(message is Message
? store.senderDisplayName(message as Message,
replaceIfMuted: showAsMuted)
: store.userDisplayName(message.senderId),
style: TextStyle(
fontSize: 18,
height: (22 / 18),
color: showAsMuted
? designVariables.title.withFadedAlpha(0.5)
: designVariables.title,
).merge(weightVariableTextStyle(context, wght: 600)),
overflow: TextOverflow.ellipsis)),
UserStatusEmoji(userId: message.senderId, size: 18,
padding: const EdgeInsetsDirectional.only(start: 5.0)),

child: Text.rich(
TextSpan(text: store.userDisplayName(userId), children: [
UserStatusEmoji.asWidgetSpan(userId: userId, fontSize: 17,
textScaler: MediaQuery.textScalerOf(context)),
]),
style: TextStyle(
fontSize: 17,
height: 19 / 17,
color: designVariables.textMessage,
).merge(weightVariableTextStyle(context, wght: 500)))),

Or maybe to stay consistent and use line height for the emoji size, we may consider changing other places in the UI to also use the same criterion. 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be a good question to take to #mobile-design 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(with screenshots of the different alternatives)

@gnprice gnprice force-pushed the 198-set-user-status branch 2 times, most recently from 32b97be to 41529a9 Compare July 24, 2025 20:52
@gnprice
Copy link
Member

gnprice commented Jul 24, 2025

(I rebased atop the merged version of #1702, and then atop main past the refactors in #1735 and #1736.)

@gnprice gnprice force-pushed the 198-set-user-status branch from 41529a9 to 9d3abbd Compare July 25, 2025 01:47
@gnprice
Copy link
Member

gnprice commented Jul 25, 2025

Added two commits at the end:
862b48a (squash) user-status: Update a test on profile page
9d3abbd (squash) user-status: Adjust candidate emoji size to 24 on set-status screen

@gnprice gnprice mentioned this pull request Jul 25, 2025
@sm-sayedi sm-sayedi force-pushed the 198-set-user-status branch from 9d3abbd to 1bd7e60 Compare July 26, 2025 03:04
@sm-sayedi sm-sayedi marked this pull request as ready for review July 26, 2025 03:23
@sm-sayedi sm-sayedi added the integration review Added by maintainers when PR may be ready for integration label Jul 26, 2025
@sm-sayedi sm-sayedi requested a review from gnprice July 26, 2025 03:24
@sm-sayedi
Copy link
Collaborator Author

sm-sayedi commented Jul 26, 2025

Thanks @gnprice for all the previous reviews. This is now out of Draft with tests included. Please have a look.

Note: The CI is failing because of the Flutter upgrade in #1755 which this PR is rebased on.

@sm-sayedi sm-sayedi force-pushed the 198-set-user-status branch 3 times, most recently from 5ae151f to 79139b2 Compare July 26, 2025 18:32
await tester.tap(saveButtonFinder);
await testNavObserver.pumpPastTransition(tester);
// check(currentRoute).isNotNull().isA<MaterialAccountWidgetRoute>()
// .page.isA<ProfilePage>();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commented-out check is not working for no obvious reason. It shows the "Profile page" route to be a MaterialPageRoute instead of a MaterialAccountWidgetRoute.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, where does this route come from, and why would it be one or the other?

It looks like this will be the route that setupPage put there with TestZulipApp, in this code:

    await tester.pumpWidget(TestZulipApp(
      accountId: eg.selfAccount.id,
      navigatorObservers: [testNavObserver, ?navigatorObserver],
      child: ProfilePage(userId: eg.selfUser.userId)));

I think you should be able to get the intended effect of this check by just looking for a widget, not at routes: like check(find.byType(ProfilePage)).findsOne().

@sm-sayedi sm-sayedi force-pushed the 198-set-user-status branch from 79139b2 to 28ac364 Compare July 27, 2025 05:53
sm-sayedi added 13 commits July 28, 2025 22:19
And update Flutter's supporting libraries to match.
In self-user profile page, this also removes the user status information
where shown in regular (non-self-user) profile page, as the newly-added
button already shows the same information.
This is useful when we want to show a status emoji that we already know
about, instead of relying on `userId` to get the emoji for the user.
For example in the next commits, in setting user status page, where a
list of status suggestions are shown.
Instead of using the selected emoji deep down the widget tree, simply
return it where the emoji picker sheet is opened, to use it for
different purposes.
This way, we can use the instance of TestNavigatorObserver to wait for
the route transition to complete, instead of creating a different object
of TransitionDurationObserver for the same purpose.
The assert was for not letting the coming emoji data collide with
popular emoji data. This change will make it easier for callers to
provide data of any emojis. If the data contains one/more of popular
emojis, the data from serverEmojiDataPopular will replace it, as it is
the source of truth for popular emojis in test code.
@sm-sayedi sm-sayedi force-pushed the 198-set-user-status branch from 28ac364 to a7544c9 Compare July 28, 2025 18:41
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Comments below.

}

@visibleForTesting
Map<String, String> statusCodesToText(ZulipLocalizations localizations) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name sounds more general than what the function actually is about. That makes it potentially confusing when it appears in the global namespace.

In general a good way to address that is by making it a method on some relevant class, which effectively puts the name into a namespace that clarifies the context. Here, a good class would be SetStatusPage.

await tester.tap(saveButtonFinder);
await testNavObserver.pumpPastTransition(tester);
// check(currentRoute).isNotNull().isA<MaterialAccountWidgetRoute>()
// .page.isA<ProfilePage>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, where does this route come from, and why would it be one or the other?

It looks like this will be the route that setupPage put there with TestZulipApp, in this code:

    await tester.pumpWidget(TestZulipApp(
      accountId: eg.selfAccount.id,
      navigatorObservers: [testNavObserver, ?navigatorObserver],
      child: ProfilePage(userId: eg.selfUser.userId)));

I think you should be able to get the intended effect of this check by just looking for a widget, not at routes: like check(find.byType(ProfilePage)).findsOne().

Comment on lines +406 to +408
String? getUnicodeEmojiNameByCode(String emojiCode) =>
// TODO(log) if null; not supposed to happen
_serverEmojiData?[emojiCode]?.firstOrNull;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Well, why is it not supposed to happen? 🙂

I think there are actually three different answers for the three different ways this can be null.

  • _serverEmojiData still null: this means this method is being called when we don't yet have the server emoji data. That's something that can happen, but it's a problem if it happens often; so good to log. Compare _generateAllCandidates.

  • emojiCode isn't a key there. Given the way this method is used, that means the hard-coded list of suggested status emoji is out of date. Definitely would want to log that. Similar to a case in _generatePopularCandidates.

  • emojiCode is present, but the value is an empty list. That is an error by the server. The format of this data is documented at https://zulip.com/api/register-queue under server_emoji_data_url; and

    each value is the corresponding emoji names for this emoji, with the canonical name for the emoji always appearing first.

    So there must be a first value.

    I think we can just ignore the case where the list is empty; in general we're not trying to be a validator for the server's conformance to the Zulip API, and this is an unlikely bug for the server to have. On the "crunchy shell" principle, we could also have ServerEmojiData.fromJson enforce that.

Comment on lines +351 to +356
test('server emoji data present, emoji code corresponds to empty emoji name list', () {
final store = prepare(unicodeEmoji: {
'1f4c5': [],
});
check(store.getUnicodeEmojiNameByCode('1f516')).isNull();
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In particular let's not have this test case; an empty list shouldn't reach this code in the first place.


// Inspired by test code in the Flutter tree:
// https://github.com/flutter/flutter/blob/53082f65b/packages/flutter/test/widgets/observer_tester.dart
// https://github.com/flutter/flutter/blob/53082f65b/packages/flutter/test/widgets/navigator_test.dart

/// A trivial observer for testing the navigator.
class TestNavigatorObserver extends NavigatorObserver {
class TestNavigatorObserver extends TransitionDurationObserver{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nav test: Make TestNavigatorObserver extend TransitionDurationObserver

This way, we can use the instance of TestNavigatorObserver to wait for
the route transition to complete, instead of creating a different object
of TransitionDurationObserver for the same purpose.

Cool, good idea.

Comment on lines +505 to +506
checkButtonEnabled(tester, saveButtonFinder, expected: true);
await tester.tap(saveButtonFinder);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "enabled" check is redundant:

Suggested change
checkButtonEnabled(tester, saveButtonFinder, expected: true);
await tester.tap(saveButtonFinder);
await tester.tap(saveButtonFinder);

If it weren't, then the tap wouldn't have any effect; and the next lines after this check for effects it should have had.

Comment on lines +511 to +517
await store.changeUserStatus(eg.selfUser.userId, UserStatusChange(
text: OptionSome('Busy'),
emoji: OptionSome(StatusEmoji(emojiName: 'working_on_it',
emojiCode: '1f6e0', reactionType: ReactionType.unicodeEmoji))));
await tester.pump();
check(find.text('\u{1f6e0}')).findsOne();
check(findText('Busy', includePlaceholders: false)).findsOne();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't really check anything about the set-status page. Instead it's effectively repeating a test you've added separately for the profile page. So it can be left out.

Comment on lines +196 to +197
group('no status set, buttons are disabled', () {
testWidgets('emoji is added -> buttons are enabled', (tester) async {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of test cases in this group and the next one; and they feel pretty repetitive, totalling about 260 lines of code. It feels like a lot to read compared to the complexity of what it's testing, so I skimmed (until I got to the test cases for what the buttons do) rather than spend the time to read them all fully.

I don't think this area is critical to test, so let's not spend effort really polishing this set of tests; it's OK for them to go in while being somewhat repetitive and hard to read, because it'd be OK for this to go in without having most of this set of tests. But I'll leave one or two comments where I think it might be easy to cut them down.

checkButtonEnabled(tester, saveButtonFinder, expected: true);
});

testWidgets('emoji & text are added, then removed -> buttons are enabled, then disabled', (tester) async {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a test just below for the buttons getting disabled on hitting clear; and other tests above in this group for them getting enabled in the first place on adding emoji or text. So it seems like everything this test is covering is already covered by more-specific tests, and it can be cut.

(Which is especially nice since it's the longest one in the group, at just over 30 lines.)

Comment on lines +451 to +454
await tester.pump();

checkButtonEnabled(tester, clearButtonFinder, expected: true);
checkButtonEnabled(tester, saveButtonFinder, expected: true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two helpers almost always get called in pairs, for the clear and save buttons. How about combining those into one helper?

Then those call sites can generally all be joined to the stanzas above them, following the usual pattern I've mentioned before where a round of "set up, then check" is one stanza. (In fact that'd be a good change even with the two separate helper calls, but especially so with just one.) So:

Suggested change
await tester.pump();
checkButtonEnabled(tester, clearButtonFinder, expected: true);
checkButtonEnabled(tester, saveButtonFinder, expected: true);
await tester.pump();
checkButtonsEnabled(tester, expectClear: true, expectSave: true);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set own user status
3 participants