msglist: Display typing indicators on typing. #790

PIG208 · 2024-07-05T21:21:52Z

Display a typing indicator when there are people typing in a SendableNarrow such as DmNarrow and TopicNarrow, in a fashion similar to the web app.

This is stacked on top of #783 and fixes #665.

PIG208 · 2024-07-05T21:29:10Z

Screenshot

Video

Screen_Recording_20240705_172853.mp4

gnprice

Thanks! We'll get to a full review after #783.

On a quick skim, though, here's a pair of initial comments. And one more:

Please add a screenshot of what the typing indicators look like when visible. … It looks like we don't currently mention that in our guide to reviewable PRs, but I've seen @alya mention in a couple of threads in the past that that's helpful for reviewing the UI, even when there's also a video, because it makes it easier to look closely at the key moments in the UI's behavior.

(That guide does have a couple of other good tips on posting screenshots and videos, though.)

gnprice · 2024-07-05T21:59:40Z

assets/l10n/app_en.arb

@@ -479,5 +479,14 @@
      "senderFullName": {"type": "String", "example": "Alice"},
      "numOthers": {"type": "int", "example": "4"}
    }
+  },
+  "typingIndicator": "{numPeople, plural, =1{{typist} is typing} =2{{typist} and {otherTypist} are typing} other{Several people are typing}}",


The "plural" feature in i18n is meant for handling the grammatical effects of a number. It shouldn't be used for trying to handle a number's other effects in the program logic, because its behavior is governed by the language's grammar.

In particular the names like "=1" can be misleading: they're pointers to the relevant number categories in the grammar, which don't always match the names' literal meaning.

Concretely, here, I believe that with this version:

In English, only the "=1" and "other" forms will be used — if the number is 2, that's "other"

In Chinese, only the "other" form will be used, regardless of number

In Russian, the "=1" form will be used not only for 1 but for 21, 31, and many larger numbers

Some useful background reading (the first link is in a JS context, but the underlying concepts are the same as in i18n APIs used in Dart and many other languages):

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/PluralRules
https://cldr.unicode.org/index/cldr-spec/plural-rules
https://www.unicode.org/cldr/charts/45/supplemental/language_plural_rules.html

Thanks! Those are some interesting distinctions. =2 seems to be used for English after modifying the test case.

We can split this into 3 separate strings (or just two strings equivalent to the "=1" and "other" here, respectively) without the "plural" feature.

=2 seems to be used for English after modifying the test case.

Huh, surprising.

In that case I'd guess that in Chinese it'd use the =1 and =2 forms too, if present… but a translator UI, like that in Transifex, generally wouldn't ask the Chinese translators for such forms, because in the normal grammatical behavior of the language (more precisely, in its behavior as recorded in the CLDR database, shown in that giant table at my third link) there's no distinction that would call for such forms. So we'd end up with only an "other" form in our translations.

And I'd expect that in Russian it'd still behave the way I predicted above, using the =1 form more broadly than this bit of program logic intends.

Cross-posted a version of the above to #mobile-team, and then added further findings:
https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/i18n.20plurals.20.2F.20grammatical.20number/near/1848780

gnprice · 2024-07-05T22:03:40Z

test/widgets/message_list_test.dart

+        .data.equals(zulipLocalizations.typingIndicator(
+          expected.length, expected[0].fullName, otherTypist?.fullName ?? ''));


In light of my other comment, let's have these tests use concrete strings for expectations, instead of zulipLocalizations results 🙂 Try changing the tests first, and please confirm whether my prediction above is correct (at least about English).

I think just testing in English will suffice to make the point and confirm that the logic is happening outside of the i18n plural rules.

alya · 2024-07-05T22:16:04Z

Looking at the video, do we specifically have a mobile design for this? If not, I'd be inclined to match the web UI styling. Or if the differences are intentional, we should have a #design thread as to why it's different, and whether to update the web UI.

alya · 2024-07-05T22:18:11Z

Thanks! We'll get to a full review after #783.

On a quick skim, though, here's a pair of initial comments. And one more:

Please add a screenshot of what the typing indicators look like when visible. … It looks like we don't currently mention that in our guide to reviewable PRs, but I've seen @alya mention in a couple of threads in the past that that's helpful for reviewing the UI, even when there's also a video, because it makes it easier to look closely at the key moments in the UI's behavior.
(That guide does have a couple of other good tips on posting screenshots and videos, though.)

That's an old version of the documentation. I think it's more clear in the current version.

gnprice · 2024-07-05T22:19:55Z

That's an old version of the documentation. I think it's more clear in the current version.

Ah indeed, thanks — sorry for the confusion. I thought I remembered seeing such a writeup, so I was a bit puzzled not to find it; I didn't notice I'd landed on the "stable" version of the docs 🙂

PIG208 · 2024-07-05T23:03:15Z

Updated the PR to use separate translation strings and a design that looks more similar to the one used in the web app.

Design comparison

web	Flutter

alya · 2024-07-06T00:27:58Z

I think we should include the ... on mobile, too...

PIG208 · 2024-07-08T19:44:46Z

Updated Design:

alya · 2024-07-08T22:48:48Z

Works for me!

chrisbobbe

Thanks! Small comments below.

chrisbobbe · 2024-07-23T06:37:44Z

lib/widgets/message_list.dart

+    };
+
+    return Padding(
+      padding: const EdgeInsets.only(left: 16, top: 2),


Since left and right aren't equal, this looks like a place to use EdgeInsetsDirectional.

chrisbobbe · 2024-07-23T06:38:25Z

lib/widgets/message_list.dart

+    return Padding(
+      padding: const EdgeInsets.only(left: 16, top: 2),
+      child: Text(text,
+        textAlign: TextAlign.left,


TextAlign.start, I think.

chrisbobbe · 2024-07-23T06:44:48Z

test/widgets/message_list_test.dart

+        .data.equals(expected);
+    }
+
+    testTyping(WidgetTester tester, SendableNarrow narrow) async {


I think we only normally use the name testFoo when the function itself calls test/testWidgets. I see that checkTyping is already taken, though, by the helper just above this. Hmm.

Could find a different name, or maybe not use a helper and just cover the dm and topic cases with something like this (untested):

for (final (description, narrow, message) in [ ('dm', …, …), ('topic', …, …), ]) { testWidgets(description, …

chrisbobbe · 2024-07-23T06:46:54Z

lib/widgets/message_list.dart

+
+    final typistNames = model!.typistIdsInNarrow(narrow)
+      .where((id) => id != store.selfUserId)
+      .map((id) => store.users[id]!.fullName)


I think technically this should have an "(unknown user)" fallback, for #716? I acknowledge that it would be rare and silly for a nonexistent user to show up as typing, though.

chrisbobbe · 2024-07-23T06:49:47Z

assets/l10n/app_en.arb

@@ -483,5 +483,24 @@
  "notifSelfUser": "You",
  "@notifSelfUser": {
    "description": "Display name for the user themself, to show after replying in an Android notification"
+  },
+  "onePersonTyping": "{typist} is typing...",


We probably want the "…" character instead of "...", right?

PIG208 · 2024-07-23T21:36:45Z

Updated the PR. Thanks for the review @chrisbobbe!

chrisbobbe

Thanks! Small comments below, and I'll mark this for Greg's review.

chrisbobbe · 2024-07-25T23:03:16Z

lib/widgets/message_list.dart

@@ -530,6 +534,68 @@ class MarkAsReadWidget extends StatelessWidget {
  }
 }

+class _TypingStatusState extends State<TypingStatusWidget> with PerAccountStoreAwareStateMixin<TypingStatusWidget> {


Normally we organize stateful-widget code by putting the widget first and then the state.

Relatedly, follow the normal pattern for the relationship between widget and state classes' names: widget Foo has state _FooState.

chrisbobbe · 2024-07-25T23:10:48Z

lib/widgets/message_list.dart

+          color: HslColor(0, 0, 53), fontStyle: FontStyle.italic)),
+    );


Suggested change

color: HslColor(0, 0, 53), fontStyle: FontStyle.italic)),

);

color: HslColor(0, 0, 53), fontStyle: FontStyle.italic)));

chrisbobbe · 2024-07-25T23:13:34Z

lib/widgets/message_list.dart

+  Widget build(BuildContext context) {
+    final store = PerAccountStoreWidget.of(context);
+    final narrow = widget.narrow;
+    const placeholder = SizedBox(height: 8);


When the typing status is not shown, this puts an extra 8px between the last message and the mark-as-read button. If that's not part of the design, let's avoid it.

Came across https://chat.zulip.org/#narrow/stream/48-mobile/topic/messages.20shift.20on.20mark-as-read, and I think the typing indicators have the same issue of causing a message shift. I wonder if we want to do something similar to #562.

The follow-up discussion is here: https://chat.zulip.org/#narrow/stream/48-mobile/topic/messages.20shift.20on.20typing.20indicator/near/1925986

gnprice

Thanks @PIG208 for implementing this, and @chrisbobbe for the previous review!

Generally this looks good. Comments below, in addition to those from Chris above.

gnprice · 2024-07-26T00:08:24Z

lib/api/route/events.dart

-      'stream_typing_notifications': false, // TODO implement
+      'stream_typing_notifications': true,


This should get its own commit and own commit message, since it's changing what we get from the server (whereas the rest of the commit has a very different character, adding UI).

I think the effect is that it's just causing us to get more TypingEvents, and our code already handles the new kind?

Yes, that's correct.

commit-structure nit: put this commit before the main commit, so that the main commit can be accurate in claiming to fix the issue #665

(otherwise, #665 isn't actually fully working as of that commit)

gnprice · 2024-07-26T00:10:26Z

lib/widgets/message_list.dart

@@ -530,6 +534,68 @@ class MarkAsReadWidget extends StatelessWidget {
  }
 }

+class _TypingStatusState extends State<TypingStatusWidget> with PerAccountStoreAwareStateMixin<TypingStatusWidget> {


Relatedly, follow the normal pattern for the relationship between widget and state classes' names: widget Foo has state _FooState.

gnprice · 2024-07-26T00:14:18Z

lib/widgets/message_list.dart

+    const placeholder = SizedBox(height: 8);
+    if (narrow is! SendableNarrow) return placeholder;
+
+    final localization = ZulipLocalizations.of(context);


nit: plural, not singular — this is an object that contains a whole set of localizations

gnprice · 2024-07-26T00:16:18Z

assets/l10n/app_en.arb

+  },
+  "onePersonTyping": "{typist} is typing…",
+  "@onePersonTyping": {
+    "description": "Text to display when there is a user typing.",


nit:

Suggested change

"description": "Text to display when there is a user typing.",

"description": "Text to display when there is one user typing.",

In the two other cases below, I'd also say "there is a user typing".

Do you mean "there is one user typing"? Or "there are two users typing" and etc?

Ah sorry, that was unclear: the text below is fine for those other cases. What I mean is, I would say all three of these situations are accurately described with "there is a user typing". So the text for this case should be more specific in order to distinguish it.

gnprice · 2024-07-26T00:19:01Z

lib/widgets/message_list.dart

+
+    final localization = ZulipLocalizations.of(context);
+    final typistNames = model!.typistIdsInNarrow(narrow)
+      .where((id) => id != store.selfUserId)


Can we do this filtering on the model instead? That will simplify this build method's code, I think, as well as mean less work for it to do (by helping it skip allocating a new list).

gnprice · 2024-07-26T00:27:22Z

test/widgets/message_list_test.dart

@@ -981,4 +981,70 @@ void main() {
      });
    });
  });
+
+  group('TypingStatus', () {


separate nit: where possible we try to keep tests in the same order as the code they're testing, to help with navigating between tests and code under test

Here MarkAsReadWidget is already out of order, so moving that would be a good small prep commit. Then this can go next to that.

gnprice · 2024-07-26T00:28:31Z

lib/widgets/message_list.dart

+  }
+}
+
+class TypingStatusWidget extends StatefulWidget {


nit (a thought prompted by looking at the order of the tests): it'd be most logical for this to come just above MarkAsReadWidget, rather than just below — since that matches how it appears in the UI

(That would be the opposite order from their call sites, but that's because the code at the call sites happens to be written in a context where it's counting from the bottom.)

gnprice · 2024-07-26T00:32:38Z

test/widgets/message_list_test.dart

+    final dmMessage = eg.dmMessage(from: eg.otherUser, to: [eg.selfUser]);
+    final dmNarrow = DmNarrow.withUsers(
+      users.map((u) => u.userId).toList(),
+      selfUserId: eg.selfUser.userId);


This message doesn't appear in this narrow. For that, the to argument should have the other users. (The code below assumes the message is in the narrow, so this affects the realism of the test data.)

Then once you do that, there's a handy constructor DmNarrow.ofMessage.

gnprice · 2024-07-26T00:35:08Z

test/widgets/message_list_test.dart

+      matching: find.byType(Text)
+    );
+
+    checkTyping(WidgetTester tester, TypingEvent event, {required String expected}) async {


nit: function definition should have explicit return type, so it doesn't look like a function call

(hmm, we should really look to see if there's a lint rule for that, and turn it on — seems eminently lintable)

gnprice · 2024-07-26T00:36:45Z

test/widgets/message_list_test.dart

+      check(finder.evaluate()).single.has((x) => x.widget, 'widget').isA<Text>()
+        .data.equals(expected);


tester.widget can simplify this:

Suggested change

check(finder.evaluate()).single.has((x) => x.widget, 'widget').isA<Text>()

.data.equals(expected);

check(tester.widget<Text>(finder)).data.equals(expected);

PIG208 · 2024-07-26T01:38:47Z

Thanks for the reviews! Updated the PR and left some questions.

gnprice

Thanks for the revision! Comments below.

gnprice · 2024-07-26T21:31:57Z

lib/model/typing_status.dart

  Iterable<int> typistIdsInNarrow(SendableNarrow narrow) =>
-    _timerMapsByNarrow[narrow]?.keys ?? [];
+    _timerMapsByNarrow[narrow]?.keys.where((id) => id != selfUserId) ?? [];


Ah, in #790 (comment) I didn't just mean doing the filtering in the model code rather than the UI code — I meant doing so in the model data structure itself. That is, just never have an entry here where the key is the self-user.

That way we get to avoid the where. Which in turn gets us out of scanning through the whole list if there are more than two.

gnprice · 2024-07-26T21:34:25Z

lib/model/typing_status.dart

-        allRecipientIds: event.recipientIds!..sort(), selfUserId: selfUserId),
+        allRecipientIds: event.recipientIds!.toList()..sort(), selfUserId: selfUserId),


No need for the copy (i.e. this toList call). In principle it's not ideal to be mutating the list because in principle something else could be looking at it… but (a) nothing else actually does look at this, because it's a specific feature that this model is entirely responsible for, and (b) really it ought to be sorted anyway, so this is just doing a public service for anything else that might be using the data.

(The ideal version of this would be that we sort at deserialization time.)

The DmNarrow.ofMessage factory creates a unmodifiable list, which motivates the toList call here, because we can't mutate the list.

I think we might not be using DmNarrow.ofMessage in the application code, so tweaking the tests can be a solution. But it would be helpful if there is a way to statically discover issues like this.

Resolved by only sorting them when deserializing TypingEvent, and skipping the sort call from TypingStatus.

gnprice · 2024-07-26T21:36:31Z

lib/api/route/events.dart

-      'stream_typing_notifications': false, // TODO implement
+      'stream_typing_notifications': true,


commit-structure nit: put this commit before the main commit, so that the main commit can be accurate in claiming to fix the issue #665

(otherwise, #665 isn't actually fully working as of that commit)

gnprice · 2024-07-26T21:38:40Z

lib/widgets/message_list.dart

+    final typistIds = model!.typistIdsInNarrow(narrow).toList();
+    if (typistIds.isEmpty) return const SizedBox();
+    final String text = switch (typistIds) {
+      [final firstTypist] =>


#790 (comment) (a reply I made yesterday to a question after your revision — echoing here just so it remains visible in this review)

Rewritten that part of code. It ended up looking a bit verbose, but it does save the .toList call or unnecessary .map's.

Great. It may look a little verbose… but I think in actual line count (for this build method) it's the shortest version yet 🙂 Sometimes just writing things very concretely pays off.

gnprice · 2024-07-26T21:43:21Z

test/widgets/message_list_test.dart

+    final dmMessage = eg.dmMessage(from: eg.selfUser, to: [eg.otherUser]);
+    final dmNarrow = DmNarrow.ofMessage(dmMessage, selfUserId: eg.selfUser.userId);


Let's keep test data realistic — so if we're going to have eg.thirdUser and eg.fourthUser typing in this DM narrow, they should be members of it. (following up on #790 (comment) )

PIG208 · 2024-07-27T06:15:17Z

Updated the PR. Thanks!

PIG208 · 2024-07-29T20:00:21Z

Rebased to resolve a conflict with 0d577a0

PIG208 · 2024-07-29T21:29:10Z

pushed to rebase and address #790 (comment)

gnprice

Hmm, in the current revision it doesn't look like my comments at #790 (review) have been addressed. Perhaps you rebased a different version of the branch than you intended?

gnprice · 2024-07-29T22:26:15Z

assets/l10n/app_en.arb

+  "twoPeopleTyping": "{typist} and {otherTypist} are typing…",
+  "@twoPeopleTyping": {
+    "description": "Text to display when there is one user typing.",


This description doesn't match the text; it's also the same as the description of a different string, which makes it unhelpful for translators.

I guess #790 (comment) left things still confusing? The previous revision was fine on this file.

Right, I thought #790 (comment) meant to have all three use this same description string.

PIG208 · 2024-07-30T00:25:18Z

I think a previous update was lost to a hard reset. Digging my reflog right now.

PIG208 · 2024-07-30T00:37:48Z

This should be fixed now.

gnprice

Thanks for the revision! This all looks great — just nits in this round.

gnprice · 2024-07-30T06:13:33Z

lib/api/model/events.dart

@@ -900,16 +901,16 @@ class TypingEvent extends Event {
    required this.recipientIds,
    required this.streamId,
    required this.topic,
-  });
+  }) : assert(isSortedWithoutDuplicates(recipientIds ?? []));


nit:

Suggested change

}) : assert(isSortedWithoutDuplicates(recipientIds ?? []));

}) : assert(recipientIds == null || isSortedWithoutDuplicates(recipientIds));

I think that's a bit clearer, because in the situation where recipientIds is null it's not that that's a way of expressing that the list of recipients is empty — rather it means it doesn't make sense to talk about recipients (because it's a stream/channel message).

gnprice · 2024-07-30T06:15:30Z

lib/api/model/events.dart


  static Object? _readSenderId(Map<Object?, Object?> json, String key) {
    return (json['sender'] as Map<String, dynamic>)['user_id'];
  }

  static List<int>? _recipientIdsFromJson(Object? json) {
    if (json == null) return null;
-    return (json as List<Object?>).map(
-      (item) => (item as Map<String, Object?>)['user_id'] as int).toList();
+    return (json as Iterable<Object?>).map(


nit:

Suggested change

return (json as Iterable<Object?>).map(

return (json as List<Object?>).map(

That keeps this code resembling the generated deserialization code when it comes to boring parts like casting pieces of the data to their expected types. There isn't some expected situation where this would be a non-list iterable, right?

gnprice · 2024-07-30T06:18:06Z

lib/model/typing_status.dart

@@ -3,6 +3,7 @@ import 'dart:async';
 import 'package:flutter/foundation.dart';

 import '../api/model/events.dart';
+import '../log.dart';


nit: this happens a commit sooner than it should

(so the linter would warn at that commit)

gnprice · 2024-07-30T06:20:28Z

lib/api/model/events.dart

-    return (json as List<Object?>).map(
-      (item) => (item as Map<String, Object?>)['user_id'] as int).toList();
+    return (json as Iterable<Object?>).map(
+      (item) => (item as Map<String, Object?>)['user_id'] as int).toList()..sort();


commit-message nit:

events: Sort recipientIds for TypingEvent.

The prefix we've used for changes here is api:, so let's stay consistent with that.

A command like git log --oneline lib/api/model/events.dart is very handy for looking to see what precedents are for this kind of question.

gnprice · 2024-07-30T06:22:16Z

lib/model/typing_status.dart

@@ -38,6 +39,10 @@ class TypingStatus extends ChangeNotifier {
  }

  bool _addTypist(SendableNarrow narrow, int typistUserId) {
+    if (typistUserId == selfUserId) {


commit-message nit:

model: Do not allow adding self as typist.

For files in lib/model/, we normally make the prefix more specific than this — here, typing_status: would be good.

(I'd be tempted to shorten it to typing:, but that's too confusing with the concept of the type system and type checker.)

gnprice · 2024-07-30T06:26:49Z

test/widgets/message_list_test.dart

@@ -843,227 +1130,4 @@ void main() {
        ..status.equals(AnimationStatus.dismissed);
    });
  });
-
-  group('MarkAsReadWidget', () {


commit-message nit:

test [nfc]: Reorder MarkAsReadWidget test group.

Prefix should be more specific: "msglist test [nfc]:".

(Same story as at #787 (comment) .)

gnprice · 2024-07-30T06:40:13Z

lib/widgets/message_list.dart

+    final typistIds = model!.typistIdsInNarrow(narrow).toList();
+    if (typistIds.isEmpty) return const SizedBox();
+    final String text = switch (typistIds) {
+      [final firstTypist] =>


Great. It may look a little verbose… but I think in actual line count (for this build method) it's the shortest version yet 🙂 Sometimes just writing things very concretely pays off.

So that its location within the test groups matches where its implementation is defined. Signed-off-by: Zixuan James Li <[email protected]>

We added support for stream typing notifications, and this change ensures that we get TypingEvents for them. Signed-off-by: Zixuan James Li <[email protected]>

DmNarrow expects them to be sorted, so doing it early during deserialization helps. Because of this TypingStatus no longer needs to modify the recipientIds list. Signed-off-by: Zixuan James Li <[email protected]>

The server usually does not send events notifying the user themself typing. But having this check gives us a stronger guarantee that the maps do not contain the self user. Signed-off-by: Zixuan James Li <[email protected]>

Because we don't have a Figma design yet, this revision supports a basic design similar to the web app when there are people typing. Fixes zulip#665. Signed-off-by: Zixuan James Li <[email protected]>

PIG208 · 2024-07-30T18:47:48Z

Updated the PR. Thanks for the review!

gnprice · 2024-07-30T18:54:38Z

Thanks! Looks good — merging.

PIG208 marked this pull request as ready for review July 5, 2024 21:21

PIG208 force-pushed the typing-ui branch 2 times, most recently from 2b24264 to fc9db66 Compare July 5, 2024 21:24

gnprice reviewed Jul 5, 2024

View reviewed changes

PIG208 force-pushed the typing-ui branch from fc9db66 to ba78d03 Compare July 5, 2024 22:25

PIG208 force-pushed the typing-ui branch from ba78d03 to dbdf6e8 Compare July 5, 2024 23:03

PIG208 force-pushed the typing-ui branch 5 times, most recently from cd62fea to 617d7a7 Compare July 8, 2024 19:32

PIG208 force-pushed the typing-ui branch 2 times, most recently from d3767d4 to 1a9d547 Compare July 15, 2024 21:22

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Jul 16, 2024

chrisbobbe reviewed Jul 23, 2024

View reviewed changes

PIG208 force-pushed the typing-ui branch 3 times, most recently from fd00b9e to 3bf08e1 Compare July 23, 2024 21:34

chrisbobbe reviewed Jul 25, 2024

View reviewed changes

chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Jul 25, 2024

gnprice reviewed Jul 26, 2024

View reviewed changes

PIG208 force-pushed the typing-ui branch from 3bf08e1 to 8c20602 Compare July 26, 2024 01:34

gnprice reviewed Jul 26, 2024

View reviewed changes

PIG208 force-pushed the typing-ui branch from 8c20602 to b6bf142 Compare July 29, 2024 19:59

PIG208 force-pushed the typing-ui branch from b6bf142 to 6d61d1d Compare July 29, 2024 21:28

gnprice reviewed Jul 29, 2024

View reviewed changes

gnprice mentioned this pull request Jul 30, 2024

lint: Enable always_declare_return_types; fix existing places they're missing #851

Merged

PIG208 force-pushed the typing-ui branch from 6d61d1d to d10f229 Compare July 30, 2024 00:33

gnprice reviewed Jul 30, 2024

View reviewed changes

PIG208 force-pushed the typing-ui branch from d10f229 to 2208f61 Compare July 30, 2024 18:21

PIG208 added 3 commits July 30, 2024 14:22

msglist test [nfc]: Reorder MarkAsReadWidget test group.

f802d5a

So that its location within the test groups matches where its implementation is defined. Signed-off-by: Zixuan James Li <[email protected]>

register: Ask for stream_typing_notifications.

2e2872b

We added support for stream typing notifications, and this change ensures that we get TypingEvents for them. Signed-off-by: Zixuan James Li <[email protected]>

api: Sort recipientIds for TypingEvent.

be722d2

DmNarrow expects them to be sorted, so doing it early during deserialization helps. Because of this TypingStatus no longer needs to modify the recipientIds list. Signed-off-by: Zixuan James Li <[email protected]>

PIG208 force-pushed the typing-ui branch from 2208f61 to 71ef85a Compare July 30, 2024 18:23

PIG208 added 2 commits July 30, 2024 14:33

typing_status: Do not allow adding self as typist.

5d0e790

The server usually does not send events notifying the user themself typing. But having this check gives us a stronger guarantee that the maps do not contain the self user. Signed-off-by: Zixuan James Li <[email protected]>

msglist: Display typing indicators on typing.

5c70c76

Because we don't have a Figma design yet, this revision supports a basic design similar to the web app when there are people typing. Fixes zulip#665. Signed-off-by: Zixuan James Li <[email protected]>

PIG208 force-pushed the typing-ui branch from 71ef85a to 5c70c76 Compare July 30, 2024 18:34

gnprice merged commit 5c70c76 into zulip:main Jul 30, 2024
1 check passed

PIG208 deleted the typing-ui branch July 30, 2024 19:22

chrisbobbe mentioned this pull request Aug 1, 2024

msglist: Find web's dark-theme color for typing-indicator text; add to MessageListTheme #858

Closed

		.data.equals(zulipLocalizations.typingIndicator(
		expected.length, expected[0].fullName, otherTypist?.fullName ?? ''));

	color: HslColor(0, 0, 53), fontStyle: FontStyle.italic)),
	);
	color: HslColor(0, 0, 53), fontStyle: FontStyle.italic)));

		'stream_typing_notifications': false, // TODO implement
		'stream_typing_notifications': true,

	"description": "Text to display when there is a user typing.",
	"description": "Text to display when there is one user typing.",

		check(finder.evaluate()).single.has((x) => x.widget, 'widget').isA<Text>()
		.data.equals(expected);

	check(finder.evaluate()).single.has((x) => x.widget, 'widget').isA<Text>()
	.data.equals(expected);
	check(tester.widget<Text>(finder)).data.equals(expected);

		allRecipientIds: event.recipientIds!..sort(), selfUserId: selfUserId),
		allRecipientIds: event.recipientIds!.toList()..sort(), selfUserId: selfUserId),

		final dmMessage = eg.dmMessage(from: eg.selfUser, to: [eg.otherUser]);
		final dmNarrow = DmNarrow.ofMessage(dmMessage, selfUserId: eg.selfUser.userId);

	}) : assert(isSortedWithoutDuplicates(recipientIds ?? []));
	}) : assert(recipientIds == null \|\| isSortedWithoutDuplicates(recipientIds));

	return (json as Iterable<Object?>).map(
	return (json as List<Object?>).map(

msglist: Display typing indicators on typing. #790

msglist: Display typing indicators on typing. #790

Conversation

PIG208 commented Jul 5, 2024

PIG208 commented Jul 5, 2024 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

gnprice Jul 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alya commented Jul 5, 2024

alya commented Jul 5, 2024

gnprice commented Jul 5, 2024

PIG208 commented Jul 5, 2024

alya commented Jul 6, 2024

PIG208 commented Jul 8, 2024

alya commented Jul 8, 2024

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Jul 23, 2024

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Jul 26, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Jul 27, 2024

PIG208 commented Jul 29, 2024

PIG208 commented Jul 29, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Jul 30, 2024

PIG208 commented Jul 30, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Jul 5, 2024 •

edited

Loading

gnprice Jul 5, 2024 •

edited

Loading