Show user status in UI #1702

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Open

sm-sayedi wants to merge 7 commits into zulip:main from sm-sayedi:197-P2

+534 −60

Collaborator

sm-sayedi commented Jul 10, 2025

The remaining half of #1629. For related images, please see #1629 description.

Fixes: #197

This was referenced Jul 10, 2025

Set user status #1701

Draft

Show user status #1699

Closed

gnprice changed the title ~~Track user status - Part 2~~ Show user status in UI

gnprice mentioned this pull request

Track user status #197

Open

gnprice added the integration review label

Member

gnprice commented Jul 10, 2025

Copying the review status from #1629 (per #1629 (review)): I've read the first commit except for its tests:
463b938 msglist: Show user status emoji

and the comments I had there have been resolved. Remaining to review are those tests, and the other 4 commits:
14093b2 recent-dms: Show user status emoji in recent DMs page
646f9d9 new-dm: Show user status emoji
846261a autocomplete: Show user status emoji in user-mention autocomplete
e9f682e profile: Show user status

gnprice reviewed

View reviewed changes

Member

gnprice left a comment

Thanks again for building this! Comments below.

I've read through the whole of the first commit:
463b938 msglist: Show user status emoji

and the non-test changes in the other commits:
14093b2 recent-dms: Show user status emoji in recent DMs page
646f9d9 new-dm: Show user status emoji
846261a autocomplete: Show user status emoji in user-mention autocomplete
e9f682e profile: Show user status

For this round, I skipped the tests in the later commits because I think a number of my comments on the first commit's tests will apply to those too. So please go ahead and revise the later tests too where applicable.

test/widgets/message_list_test.dart Outdated

Comment on lines 55 to 63

    
              /// Finder for [UserStatusEmoji] widget.

              ///

              /// Use [type] to specify the exact emoji child widget. It can be either

              /// [UnicodeEmojiWidget] or [ImageEmojiWidget].

              Finder findStatusEmoji(Type type) {

                assert(type == UnicodeEmojiWidget || type == ImageEmojiWidget);

                return find.ancestor(

                  of: find.byType(type),

                  matching: find.byType(UserStatusEmoji));

Member

gnprice Jul 16, 2025

This seems like it's clearer if just inlined at its call sites.

The implementation (minus the assert, which becomes unnecessary) isn't much longer than the call; and it's more transparent about what it's doing. I also don't feel like this is particularly encapsulating any knowledge of the details of how the UserStatusEmoji widget works.

Member

gnprice Jul 16, 2025

Also, after doing that: how about just find.byType(UserStatusEmoji)? It's not clear to me what's gained by adding the condition that the widget have a UnicodeEmojiWidget descendant, or an ImageEmojiWidget descendant.

Collaborator Author

sm-sayedi Jul 17, 2025

Also, after doing that: how about just find.byType(UserStatusEmoji)?

Copying #1629 (comment) here: 🙂

As we discussed in this week’s check-in call, UserStatusEmoji for many cases can contain only SizedBox.shrink(), including when there’s no status set for a user. So only using find.byType(UserStatusEmoji) will pass the tests even when we don’t pass status for a user.

test/widgets/message_list_test.dart Outdated

Comment on lines 66 to 68

    
              void checkUserStatusEmoji(Finder emojiFinder, {required bool isAnimated}) {

                check((emojiFinder

                  .evaluate().first.widget as UserStatusEmoji).neverAnimate).equals(!isAnimated);

Member

gnprice Jul 16, 2025

This also feels like it'd be clearer inlined. The actual check is all about neverAnimate, but the name doesn't sound like that — it sounds like it's going to be doing some other, more general, check.

test/widgets/message_list_test.dart Outdated

Comment on lines 67 to 68

    
                check((emojiFinder

                  .evaluate().first.widget as UserStatusEmoji).neverAnimate).equals(!isAnimated);

Member

gnprice Jul 16, 2025

Instead of finder.evaluate(), use tester explicitly:

Suggested change

      
              check((emojiFinder
          
                .evaluate().first.widget as UserStatusEmoji).neverAnimate).equals(!isAnimated);
          
              check(tester.firstWidget<UserStatusEmoji>(emojiFinder).neverAnimate)
          
                .equals(!isAnimated);

That way it's conceptually clearer what's going on. (The implementation of finder.evaluate will end up using the same WidgetTester instance anyway, getting hold of it behind the scenes via a singleton.)

Collaborator Author

sm-sayedi Jul 17, 2025

The main motivation for not using tester was to minimize the number of params passed to the helper methods (checkStatusEmoji and others in the next commits). Now that it helps in making the tests clearer, so going to use that in the new revision.🙂

test/widgets/message_list_test.dart Outdated

    
            @@ -91,6 +110,7 @@ void main() {
          
                  if (mutedUserIds != null) {

                    await store.setMutedUsers(mutedUserIds);

                  }

                  await store.changeUserStatuses(userStatuses ?? []);

Member

gnprice Jul 16, 2025

How about having the individual test cases do this? That way we avoid adding yet another feature to this shared helper. (It probably should have fewer features already — that'd help make fewer things for the reader of each test case to potentially have to think about.)

test/widgets/message_list_test.dart Outdated

Comment on lines 1805 to 1809

    
                        userStatuses: [

                          (

                            user1.userId,

                            UserStatusChange(

                              text: OptionSome('Busy'),

Member

gnprice Jul 16, 2025

Hmm, these tuples are a bit of a pain to read.

How about making changeUserStatuses instead take a Map? That seems semantically appropriate, as there's no reason to have the same user ID more than once in the same call. Then:

Suggested change

      
                      userStatuses: [
          
                        (
          
                          user1.userId,
          
                          UserStatusChange(
          
                            text: OptionSome('Busy'),
          
                      userStatuses: {
          
                        user1.userId: UserStatusChange(
          
                          text: OptionSome('Busy'),

lib/widgets/autocomplete.dart Outdated

    
                            Flexible(child: labelWidget),

                            if (option case UserMentionAutocompleteResult(:var userId))

                              UserStatusEmoji(userId: userId, size: 18,

                                padding: const EdgeInsetsDirectional.only(start: 5.0))]),

Member

gnprice Jul 16, 2025

nit: similarly:

Suggested change

      
                              padding: const EdgeInsetsDirectional.only(start: 5.0))]),
          
                              padding: const EdgeInsetsDirectional.only(start: 5.0)),
          
                        ]),

lib/widgets/autocomplete.dart Outdated

Comment on lines 316 to 323

    
                        children: [

                          labelWidget,

                          Row(children: [

                            Flexible(child: labelWidget),

                            if (option case UserMentionAutocompleteResult(:var userId))

                              UserStatusEmoji(userId: userId, size: 18,

                                padding: const EdgeInsetsDirectional.only(start: 5.0))]),

                          if (sublabelWidget != null) sublabelWidget,

                        ])),

Member

gnprice Jul 16, 2025

I think this new logic is best included as part of labelWidget. Conceptually it's closely tied to the user's name, which is part of the label.

Probably in fact the cleanest home for this is in the switch (option) above, which is already the place where we inspect the details of option.

lib/widgets/profile.dart Outdated

    
            @@ -73,17 +75,28 @@ class ProfilePage extends StatelessWidget {
          
                        ),

                        // TODO write a test where the user is muted; check this and avatar

                        TextSpan(text: store.userDisplayName(userId, replaceIfMuted: false)),

                        UserStatusEmoji.asWidgetSpan(

                          userId: userId,

                          fontSize: 20,

Member

gnprice Jul 16, 2025

nit: this says fontSize: 20 but the presence circle above says fontSize: nameStyle.fontSize!; should say the same thing both places, since the intent is for them to be the same

lib/widgets/profile.dart Outdated

    
            @@ -47,6 +48,7 @@ class ProfilePage extends StatelessWidget {
          
                  if (user == null) {

                    return const _ProfileErrorPage();

                  }

                  final userStatus = store.getUserStatus(userId);

Member

gnprice Jul 16, 2025

nit: put this next to displayEmail; conceptually they're here for the very same reason

lib/widgets/profile.dart Outdated

Comment on lines 91 to 92

    
                          color: DesignVariables.of(context).userStatusText)),

                    if (displayEmail != null)

Member

gnprice Jul 16, 2025

Is this blank line intended?

The items just above here feel to me closely related to the items just below, and in particular just as closely related as other items that are grouped next to each other.


          test store [nfc]: Make changeUserStatuses take a Map instead of a Record

ae2ff96

sm-sayedi force-pushed the 197-P2 branch from e9f682e to 8e447ae Compare

July 17, 2025 17:51


          msglist: Show user status emoji

b8a3dce

sm-sayedi force-pushed the 197-P2 branch 2 times, most recently from eccaa7a to 686130f Compare

July 17, 2025 18:22

Collaborator Author

sm-sayedi commented Jul 17, 2025

Thanks @gnprice for the detailed review. Revision pushed, PTAL.

Note: CI is failing with the following issue:

Run flutter pub get
Resolving dependencies...
The current Flutter SDK version is 0.0.0-unknown.

Because zulip requires Flutter SDK version >=3.33.0-1.0.pre.832, version solving failed.


You can try the following suggestion to make the pubspec resolve:
* Try using the Flutter SDK version: 3.35.0-0.0.pre. 
Failed to update packages.
Error: Process completed with exit code 1.

I tried pushing several times, but it didn't solve the problem.

sm-sayedi requested a review from gnprice

July 17, 2025 18:26

sm-sayedi added 5 commits

July 17, 2025 23:10


          recent-dms: Show user status emoji in recent DMs page

3d7bfdb

Status emojis are only shown for self-1:1 and 1:1 conversation items.
They're ignored for group conversations as that's what the Web does.


          new-dm: Show user status emoji

9a9c794


          autocomplete [nfc]: Make MentionAutocompleteItem visibleForTesting

3ff6c2c


          autocomplete: Show user status emoji in user-mention autocomplete

5337ea6


          profile: Show user status

cedb204

Fixes: zulip#197

sm-sayedi force-pushed the 197-P2 branch from 686130f to cedb204 Compare

July 17, 2025 18:46

gnprice mentioned this pull request

CI broken by lack of recent tags in Flutter main/master branch #1710

Open

Member

gnprice commented Jul 17, 2025

Yeah, that CI failure is unrelated. Filed as #1710, and I'll fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration review