general chat #1: add support at most places #1297

PIG208 · 2025-01-22T23:53:16Z

~~Stacked on top of #1342.~~

This addresses part of #1250.

This is the bare minimum needed for making TopicName.displayName nullable before we can work on the more intricate details on how parts like sending from compose box or live hint text updates work.

The last three commits enable support for empty topics for testing. When merging, we leave those three commits out because we haven't yet finished working other other parts mentioned above. This allows us to review and merge related changes without interrupting beta releases with user-visible changes.

See the screenshots for what changes are included here.

PIG208 · 2025-02-08T01:26:42Z

Screenshots:

Autocomplete

Message list page

Compose box

(sending to empty topics from channel narrow is not supported on this branch; more in #1364)

chrisbobbe

Thanks, and sorry for the delay in my review! Small comments below.

chrisbobbe · 2025-02-24T23:02:01Z

lib/model/store.dart

+  final String? _realmEmptyTopicDisplayName; // TODO(#668): update this realm setting
+  String get realmEmptyTopicDisplayName {
+    assert(_realmEmptyTopicDisplayName != null); // TODO(log)
+    return _realmEmptyTopicDisplayName ?? 'general chat';
+  }


nit: getter first: https://github.com/flutter/flutter/blob/master/docs/contributing/Style-guide-for-Flutter-repo.md#order-other-class-members-in-a-way-that-makes-sense

chrisbobbe · 2025-02-24T23:06:15Z

test/widgets/autocomplete_test.dart

-      checkTopicShown(topic3, store, expected: true);
+      checkTopicShown('Topic one',   store, expected: false);
+      checkTopicShown('Topic two',   store, expected: true);
+      checkTopicShown('Topic three', store, expected: true);


autocomplete test [nfc]: Use plain strings for finding topic instead of inferring it from a TopicName Signed-off-by: Zixuan James Li <[email protected]>

Commit-message nit: I think "inferring" isn't quite the right word; if you have a TopicName and you want its display name, you just look at its displayName field; no inference is needed

chrisbobbe · 2025-02-24T23:12:33Z

lib/model/autocomplete.dart

@@ -921,7 +921,7 @@ class TopicAutocompleteView extends AutocompleteView<TopicAutocompleteQuery, Top
  }

  TopicAutocompleteResult? _testTopic(TopicAutocompleteQuery query, TopicName topic) {
-    if (query.testTopic(topic)) {
+    if (query.testTopic(store, topic)) {


autocomplete: Handle autocompleting to empty topic Signed-off-by: Zixuan James Li <[email protected]>

This commit is NFC, right?

chrisbobbe · 2025-02-24T23:16:41Z

lib/model/autocomplete.dart

+    // This checks for inequality because there is nothing to autocomplete to
+    // when `raw` already matches the topic exactly.
    return topic.displayName != raw


How about:

// Skip this option if it matches the query exactly

(a little less verbose)

chrisbobbe · 2025-02-24T23:17:41Z

lib/widgets/inbox.dart

@@ -537,13 +537,16 @@ class _TopicItem extends StatelessWidget {
              child: Text(
                style: TextStyle(
                  fontSize: 17,
+                  // ignore: unnecessary_null_comparison // null topic names soon to be enabled
+                  fontStyle: topic.displayName == null ? FontStyle.italic : null,


msglist: Display realmEmptyTopicDisplayName where empty topic appears Signed-off-by: Zixuan James Li <[email protected]>

This is another NFC commit, right? (And it doesn't actually do what the summary line says it does, because empty topics don't appear anywhere yet?)

Yeah, these commits are structured in a way so that they are not user visible without the final "do not merge" series. This way we can start merging part of these changes while still have a way to test them.

It will do what the summary says, but only after we allow empty topics in our data structures.

chrisbobbe · 2025-02-24T23:19:02Z

lib/widgets/message_list.dart

@@ -366,8 +366,11 @@ class MessageListAppBarTitle extends StatelessWidget {
    return Row(
      mainAxisSize: MainAxisSize.min,
      children: [
-        Flexible(child: Text(topic.displayName, style: const TextStyle(


msglist: Display realmEmptyTopicDisplayName where empty topic appears Signed-off-by: Zixuan James Li <[email protected]>

(Same comment about this being an NFC commit.)

chrisbobbe · 2025-02-24T23:31:17Z

lib/api/route/messages.dart

@@ -90,6 +92,7 @@ Future<GetMessagesResult> getMessages(ApiConnection connection, {
  bool? applyMarkdown,
  // bool? useFirstUnreadAnchor // omitted because deprecated
 }) {
+  final supportsEmptyTopics = connection.zulipFeatureLevel! >= 334; // TODO(server-10)


clientGravatar looks like another client-capability param, but that one is exposed to the caller of this binding. I think we should maybe also expose supportsEmptyTopics, to make the binding as thin as possible? (Here and in other bindings)

Ended up rewriting most of the last commit, and added a bunch of new tests.

PIG208 · 2025-02-26T03:53:55Z

Thanks for the review! This should be ready again.

chrisbobbe · 2025-03-04T00:07:49Z

Thanks, looks mostly good! For these NFC commits:

[…]
autocomplete [nfc]: Handle autocompleting to empty topic
[…]
inbox [nfc]: Display realmEmptyTopicDisplayName where empty topic appears
msglist [nfc]: Display realmEmptyTopicDisplayName where empty topic appears
[…]

I see that you've explained them in the PR discussion:

#1297 (comment)

Yeah, these commits are structured in a way so that they are not user visible without the final "do not merge" series. This way we can start merging part of these changes while still have a way to test them.

It will do what the summary says, but only after we allow empty topics in our data structures.

But the commit messages themselves look confusing; they say NFC but they also read as descriptions of a user-facing change. How about adding a brief note in each commit-message body saying that the described change doesn't actually happen until we allow empty topics in our data structures, which will come in a later series of commits.

Anyway, marking for Greg's review. :)

gnprice

Thanks to you both! Generally this looks good; comments below.

(I haven't read the "do not merge" commits.)

gnprice · 2025-03-04T20:00:20Z

lib/model/autocomplete.dart

@@ -939,10 +943,16 @@ class TopicAutocompleteView extends AutocompleteView<TopicAutocompleteQuery, Top
 class TopicAutocompleteQuery extends AutocompleteQuery {
  TopicAutocompleteQuery(super.raw);

-  bool testTopic(TopicName topic) {
+  bool testTopic(PerAccountStore store, TopicName topic) {


nit: candidate result first, then the contextual data — follow pattern of other testFoo methods on other FooAutocompleteQuery classes

gnprice · 2025-03-04T23:31:14Z

lib/model/store.dart

@@ -414,6 +417,11 @@ class PerAccountStore extends ChangeNotifier with EmojiStore, UserStore, Channel
  /// For docs, please see [InitialSnapshot.realmWaitingPeriodThreshold].
  final int realmWaitingPeriodThreshold;  // TODO(#668): update this realm setting
  final int maxFileUploadSizeMib; // No event for this.
+  String get realmEmptyTopicDisplayName {
+    assert(_realmEmptyTopicDisplayName != null); // TODO(log)


This calls for a comment explaining why this condition is supposed to be unreachable — it doesn't seem obvious.

Or maybe clearest: a bit of dartdoc saying not to access this unless zulipFeatureLevel is at least such-and-such. That's the idea for why this shouldn't happen, right?

gnprice · 2025-03-04T23:31:43Z

lib/model/store.dart

+  String get realmEmptyTopicDisplayName {
+    assert(_realmEmptyTopicDisplayName != null); // TODO(log)
+    return _realmEmptyTopicDisplayName ?? 'general chat';
+  }
+  final String? _realmEmptyTopicDisplayName; // TODO(#668): update this realm setting


nit: set this group off with blank lines

gnprice · 2025-03-04T23:36:09Z

test/widgets/autocomplete_test.dart

+      await tester.enterText(topicInputFinder, 'general ch');
+      await tester.enterText(topicInputFinder, 'general cha');
+      await tester.pumpAndSettle();
+
+      check(find.text(eg.defaultRealmEmptyTopicDisplayName)).findsOne();


This test is already relying on the value of eg.defaultRealmEmptyTopicDisplayName being what it is (or something like it), namely that it contains "general chat", in order for the search steps to work.

So the test should instead give explicitly the value of realmEmptyTopicDisplayName that it's counting on. Then it can use that in this check too.

gnprice · 2025-03-04T23:42:18Z

lib/model/autocomplete.dart

+    // Skip this option if it matches the query exactly.
    return topic.displayName != raw


This comment seems to just restate what the line after it says. Cleanest to leave it out.

gnprice · 2025-03-04T23:44:58Z

lib/widgets/inbox.dart

                  fontSize: 17,
+                  fontStyle: topic.displayName == null ? FontStyle.italic : null,
                  height: (20 / 17),


nit: keep fontSize and height next to each other — the "17" in height is a reference to the "17" in fontSize

(And even if we hadn't written height that way, those two properties are closely related and one naturally wants to read them together)

gnprice · 2025-03-04T23:45:46Z

test/widgets/inbox_test.dart

+        unreadMessages: [eg.streamMessage(stream: channel, topic: '')]);
+
+      check(find.text(eg.defaultRealmEmptyTopicDisplayName)).findsOne();


nit: keep checks joined into same stanza after the setup that leads to them

Suggested change

unreadMessages: [eg.streamMessage(stream: channel, topic: '')]);

check(find.text(eg.defaultRealmEmptyTopicDisplayName)).findsOne();

unreadMessages: [eg.streamMessage(stream: channel, topic: '')]);

check(find.text(eg.defaultRealmEmptyTopicDisplayName)).findsOne();

gnprice · 2025-03-04T23:50:28Z

lib/widgets/message_list.dart

+              style: recipientHeaderTextStyle(context).copyWith(
+                fontStyle: topic.displayName == null ? FontStyle.italic : null,


Instead of copying the style object right after making it, let's have this recipientHeaderTextStyle helper take a parameter. Can just be fontStyle directly.

gnprice · 2025-03-04T23:53:48Z

lib/widgets/message_list.dart

@@ -339,8 +339,9 @@ class MessageListAppBarTitle extends StatelessWidget {
    return Row(
      mainAxisSize: MainAxisSize.min,
      children: [
-        Flexible(child: Text(topic.displayName, style: const TextStyle(
+        Flexible(child: Text(topic.displayName ?? store.realmEmptyTopicDisplayName, style: TextStyle(


Let's have a test for this part too. Should be quick, I think; can go in the "app bar" group.

PIG208 · 2025-03-05T04:13:25Z

Updated the PR. Thanks for the review!

Signed-off-by: Zixuan James Li <[email protected]>

instead of reading TopicName.displayName Signed-off-by: Zixuan James Li <[email protected]>

Signed-off-by: Zixuan James Li <[email protected]>

While this appears to be a user facing change, it's not visible yet, not until TopicName.displayName becomes nullable. This is a part of a series of changes to handle empty topics. Signed-off-by: Zixuan James Li <[email protected]>

…ears While this appears to be a user facing change, it's not visible yet, not until TopicName.displayName becomes nullable. This is a part of a series of changes to handle empty topics. Signed-off-by: Zixuan James Li <[email protected]>

…ppears While this appears to be a user facing change, it's not visible yet, not until TopicName.displayName becomes nullable. This is a part of a series of changes to handle empty topics. Signed-off-by: Zixuan James Li <[email protected]>

gnprice · 2025-03-10T18:44:14Z

Thanks! Looks good; merging.

PIG208 force-pushed the pr-general-chat branch 11 times, most recently from da55117 to b38c9bc Compare January 29, 2025 23:59

PIG208 force-pushed the pr-general-chat branch from b38c9bc to 4bdbc85 Compare January 30, 2025 23:47

PIG208 marked this pull request as ready for review January 30, 2025 23:48

This comment was marked as outdated.

Sign in to view

PIG208 force-pushed the pr-general-chat branch from 4bdbc85 to 02a284d Compare February 6, 2025 18:50

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Feb 6, 2025

PIG208 requested a review from chrisbobbe February 6, 2025 18:56

PIG208 assigned chrisbobbe Feb 6, 2025

This comment was marked as resolved.

Sign in to view

PIG208 removed the request for review from chrisbobbe February 7, 2025 21:35

PIG208 removed the maintainer review PR ready for review by Zulip maintainers label Feb 7, 2025

PIG208 force-pushed the pr-general-chat branch 2 times, most recently from b3f2081 to bc157cf Compare February 8, 2025 01:22

PIG208 force-pushed the pr-general-chat branch 2 times, most recently from 49751d6 to d88c355 Compare February 8, 2025 01:34

PIG208 requested a review from chrisbobbe February 8, 2025 01:38

PIG208 added maintainer review PR ready for review by Zulip maintainers and removed maintainer review PR ready for review by Zulip maintainers labels Feb 8, 2025

PIG208 removed the request for review from chrisbobbe February 10, 2025 22:08

PIG208 mentioned this pull request Feb 22, 2025

general chat #2: support sending to empty topic from channel narrow, hide resolve/unresolve conditionally #1364

Merged

PIG208 changed the title ~~general chat #1: add support in most places~~ general chat #1: add support at most places Feb 22, 2025

chrisbobbe reviewed Feb 24, 2025

View reviewed changes

PIG208 force-pushed the pr-general-chat branch from bc3377d to cb1334d Compare February 26, 2025 02:40

PIG208 requested a review from chrisbobbe February 26, 2025 03:53

PIG208 mentioned this pull request Feb 28, 2025

general chat #3: support alternative styling for topic input on edits/focus/blur #1365

Merged

chrisbobbe requested review from gnprice and removed request for chrisbobbe March 4, 2025 00:08

chrisbobbe assigned gnprice and unassigned chrisbobbe Mar 4, 2025

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 Mar 4, 2025

PIG208 force-pushed the pr-general-chat branch from cb1334d to 6b838e1 Compare March 4, 2025 04:23

gnprice reviewed Mar 4, 2025

View reviewed changes

PIG208 force-pushed the pr-general-chat branch 3 times, most recently from 9b72b20 to 61fe655 Compare March 5, 2025 01:09

PIG208 added 6 commits March 10, 2025 11:41

store: Add realmEmptyTopicDisplayName

6fec187

Signed-off-by: Zixuan James Li <[email protected]>

autocomplete test [nfc]: Use plain strings for finding topic

303649a

instead of reading TopicName.displayName Signed-off-by: Zixuan James Li <[email protected]>

autocomplete test [nfc]: Remove checkTopicShown helper

a2bdbc8

Signed-off-by: Zixuan James Li <[email protected]>

gnprice force-pushed the pr-general-chat branch from 61fe655 to 8e21f98 Compare March 10, 2025 18:44

gnprice merged commit 8e21f98 into zulip:main Mar 10, 2025

PIG208 deleted the pr-general-chat branch March 10, 2025 18:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

general chat #1: add support at most places #1297

general chat #1: add support at most places #1297

PIG208 commented Jan 22, 2025 •

edited

Loading

This comment was marked as outdated.

This comment was marked as resolved.

PIG208 commented Feb 8, 2025 •

edited

Loading

chrisbobbe left a comment

chrisbobbe Feb 24, 2025

chrisbobbe Feb 24, 2025

chrisbobbe Feb 24, 2025

chrisbobbe Feb 24, 2025

chrisbobbe Feb 24, 2025

PIG208 Feb 26, 2025 •

edited

Loading

chrisbobbe Feb 24, 2025

chrisbobbe Feb 24, 2025

PIG208 Feb 26, 2025

PIG208 commented Feb 26, 2025

chrisbobbe commented Mar 4, 2025 •

edited

Loading

gnprice left a comment

gnprice Mar 4, 2025

gnprice Mar 4, 2025

gnprice Mar 4, 2025

gnprice Mar 4, 2025

gnprice Mar 4, 2025

gnprice Mar 4, 2025

gnprice Mar 4, 2025

gnprice Mar 4, 2025

gnprice Mar 4, 2025

gnprice Mar 4, 2025

PIG208 commented Mar 5, 2025

gnprice commented Mar 10, 2025

		// Skip this option if it matches the query exactly.
		return topic.displayName != raw

		unreadMessages: [eg.streamMessage(stream: channel, topic: '')]);

		check(find.text(eg.defaultRealmEmptyTopicDisplayName)).findsOne();

		style: recipientHeaderTextStyle(context).copyWith(
		fontStyle: topic.displayName == null ? FontStyle.italic : null,

general chat #1: add support at most places #1297

general chat #1: add support at most places #1297

Conversation

PIG208 commented Jan 22, 2025 • edited Loading

This comment was marked as outdated.

This comment was marked as resolved.

PIG208 commented Feb 8, 2025 • edited Loading

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 Feb 26, 2025 • 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

PIG208 commented Feb 26, 2025

chrisbobbe commented Mar 4, 2025 • edited Loading

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 Mar 5, 2025

gnprice commented Mar 10, 2025

PIG208 commented Jan 22, 2025 •

edited

Loading

PIG208 commented Feb 8, 2025 •

edited

Loading

PIG208 Feb 26, 2025 •

edited

Loading

chrisbobbe commented Mar 4, 2025 •

edited

Loading