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

PIG208 · 2025-02-19T00:03:58Z

Similar to #1297, this PR focuses on part of the general chat feature, with some final "do not merge" commits to make it testable.

This includes the following commits:

5ad98d9 compose: Support sending to empty topic
1e31660 compose: Change content input hint text if topic is empty and mandatory
073042a compose [nfc]: Make isTopicVacuous private
dd47653 compose [nfc]: Handle empty topics for fixed destination input hint text
73c46b5 compose test [nfc]: Extract ChannelNarrow variable
0f4e794 action_sheet [nfc]: Hide resolve/unresolve button for empty topic

The remaining commits should be reviewed in #1297, but neither PR blocks each other, as they each focuses on a specific part of the feature.

Screenshots

mandatory topics	non-empty topic	content is focused	content isn't focused
true
false
false (legacy, FL<334)	/
hide resolve

chrisbobbe

Thanks! Small comments from reading the first five commits so far:

193244783 action_sheet [nfc]: Hide resolve/unresolve button for empty topic
acb9b136b compose test [nfc]: Extract ChannelNarrow variable
2961e6c39 compose [nfc]: Handle empty topics for fixed destination input hint text
a50dce4ec compose [nfc]: Make isTopicVacuous private
751f376e8 compose: Change content input hint text if topic is empty and mandatory

(The sixth looks more complicated; I notice it has logic about the input's focus state.)

chrisbobbe · 2025-03-12T00:59:19Z

lib/widgets/action_sheet.dart

+  final message = store.messages[someMessageIdInTopic] as StreamMessage?;
+  // TODO: check for other cases that may disallow this action (e.g.: time
+  //   limit for editing topics).
+  final disallowResolveUnresolve =
+    // ignore: unnecessary_null_comparison // null topic names soon to be enabled
+    message != null && message.topic.displayName == null;
+  if (someMessageIdInTopic != null && !disallowResolveUnresolve) {


Could we check topic.displayName instead of getting the Message object from the store? It seems simpler if so.

chrisbobbe · 2025-03-12T01:12:01Z

test/widgets/compose_box_test.dart

+      testWidgets('with non-empty topic', (tester) async {
+        await prepare(tester,
+          narrow: TopicNarrow(channel.streamId, TopicName('topic')),
+          mandatoryTopics: false);


Does mandatoryTopics make any difference for a topic-narrow compose box? A topic-narrow compose box doesn't offer a topic input.

Yes, because the hint text of the content input displays the topic name (e.g.: "Message #channel > general chat").

Oh, but actually no, because regardless if you can send to the topic, topics being mandatory does not affect its display name.

chrisbobbe · 2025-03-12T01:27:19Z

test/widgets/compose_box_test.dart

-        narrow: TopicNarrow(channel.streamId, TopicName('topic')));
-      checkComposeBoxHintTexts(tester,
-        contentHintText: 'Message #${channel.name} > topic');
+    group('to TopicNarrow', () {


compose [nfc]: Handle empty topics for fixed destination input hint text

Commit-message nit: isn't the fixed-destination compose box also used for DMs? How about "for topic-narrow content input hint text", or similar, if it fits.

chrisbobbe · 2025-03-12T01:41:12Z

lib/widgets/compose_box.dart

@@ -167,6 +167,32 @@ class ComposeTopicController extends ComposeController<TopicValidationError> {
  /// that certain strings are not empty but also indicate the absence of a topic.
  bool get _isTopicVacuous => textNormalized == kNoTopicTopic;

+  /// The send destination as a string.


compose: Change content input hint text if topic is empty and mandatory Previously, "Message #stream > (no topic)" would appear as the hint text when the topic input is empty but mandatory. […]

The commit message says what the hint text used to be; how about also saying what it is now?

PIG208 · 2025-03-12T22:52:17Z

Thanks for the review! I have updated the PR. With a question on #1364 (comment) about whether we should use "vacuous" in user-facing strings.

chrisbobbe

Thanks! One small comment below, and I'll mark for Greg's review.

chrisbobbe · 2025-03-19T21:31:31Z

lib/widgets/compose_box.dart

+  /// The send destination as a string.
+  ///
+  /// This returns a string formatted like "#stream name" when topics are
+  /// [mandatory] but the trimmed input is empty.
+  ///
+  /// Otherwise, returns a string formatted like "#stream name > topic name".
+  // No i18n of the use of "#" and ">" strings; those are part of how
+  // Zulip expresses channels and topics, not any normal English punctuation,
+  // so don't make sense to translate. See:
+  //   https://github.com/zulip/zulip-flutter/pull/1148#discussion_r1941990585
+  String getDestinationString({required String streamName}) {


Looking at the name and the first line of the dartdoc, I wonder if "as a string" is the right level of abstraction. I see that the dartdoc refines the type by saying it's formatted in a specific way…oh and I guess the reader can reasonably infer from that that it's suitable as a UI string.

But could we remove all that indirection by just saying (in its name and doc) that it's the string for the content-input hint text? It could probably be a private helper for that, right?

Yeah, good point. This was placed next to _isTopicVacuous because its implementation logic used to be more closely related to _isTopicVacuous. But now moving it to _StreamContentInput as a private helper should be a good call.

gnprice

Thanks both!

Here's a review from reading up through:
85104e1 action_sheet [nfc]: Hide resolve/unresolve button for empty topic
b612c79 compose test [nfc]: Extract ChannelNarrow variable
77fa5a0 compose [nfc]: Handle empty topics for topic-narrow input hint text
a7ff94c compose [nfc]: Make isTopicVacuous private and move some of its dartdoc

and part of the next commit:
442d112 compose: Change content input hint text if topic is empty and mandatory

I haven't yet read:
dcaf165 compose: Support sending to empty topic

I think a good solution for a couple of my comments below may be clearest by illustration. I'll send a draft PR shortly to do that.

gnprice · 2025-03-20T22:07:13Z

test/widgets/action_sheet_test.dart

@@ -358,19 +362,20 @@ void main() {
    }) async {
      final effectiveChannel = channel ?? someChannel;
      final effectiveMessages = messages ?? [someMessage];
-      assert(effectiveMessages.every((m) => m.topic.apiName == topic));
+      final topicName = TopicName(topic);


This naming is confusing because topic, the plain string, is just as much of a "name" as the TopicName is.

In the non-test code, the solution is that we consistently use TopicName for the type and just "topic" in naming variables/parameters/etc.

Here in the test code, we're a bit looser because it's convenient to throw plain strings around. But when both a plain string and a TopicName for the same thing are desired, the convention elsewhere in test code is to name the string something like topicStr. That way the thing with the canonical type TopicName doesn't get displaced to a less-canonical name.

In this case, the simplest fix is probably to have this showFromAppBar helper just take a TopicName. Only a handful of call sites pass this parameter.

gnprice · 2025-03-20T22:08:18Z

test/widgets/action_sheet_test.dart


      connection.prepare(json: eg.newestGetMessagesResult(
        foundOldest: true, messages: effectiveMessages).toJson());
      await tester.pumpWidget(TestZulipApp(accountId: eg.selfAccount.id,
        child: MessageListPage(
-          initNarrow: eg.topicNarrow(effectiveChannel.streamId, topic))));
+          initNarrow: eg.topicNarrow(effectiveChannel.streamId, topicName.apiName))));


nit: once you have a TopicName around, no need for this test helper:

Suggested change

initNarrow: eg.topicNarrow(effectiveChannel.streamId, topicName.apiName))));

initNarrow: TopicNarrow(effectiveChannel.streamId, topicName))));

(modulo the naming of topicName)

gnprice · 2025-03-20T22:11:54Z

test/widgets/compose_box_test.dart

-      testWidgets('with empty topic', (tester) async {
-        await prepare(tester, narrow: ChannelNarrow(channel.streamId));
+    group('to ChannelNarrow, topics not mandatory', () {
+      final narrow = ChannelNarrow(channel.streamId);


compose test [nfc]: Extract ChannelNarrow variable

This is sufficiently tiny and self-explanatory that it's best just squashed into the next commit, the one that makes use of it in neighboring code.

gnprice · 2025-03-20T22:16:49Z

test/widgets/compose_box_test.dart

+          narrow: TopicNarrow(channel.streamId, TopicName('')));
+        checkComposeBoxHintTexts(tester, contentHintText:
+          'Message #${channel.name} > ${eg.defaultRealmEmptyTopicDisplayName}');
+      }, skip: true); // null topic names soon to be enabled


Yeah, these skipped tests are increasingly making me wish we already had feature flags (#1409) so that we could use one of those instead 🙂

Even once we have feature flags handy, this approach where there are a bunch of skips and lint-ignores (that all get removed at the end) will still be great for features that land in a single PR, or a couple of PRs that land in quick succession. But for something spread across a longer time like this, it'd be better to have these tests active as soon as they're merged.

(This isn't a reason to block merging the PR, though.)

gnprice · 2025-03-20T22:19:06Z

test/widgets/compose_box_test.dart

@@ -326,11 +329,15 @@ void main() {

    Future<void> prepare(WidgetTester tester, {
      required Narrow narrow,
+      bool? mandatoryTopics,


nit: this should appear in the commit where it gets used (currently it's part of an earlier unrelated commit)

gnprice · 2025-03-20T22:37:58Z

lib/widgets/compose_box.dart

+    final vacuousTopicDisplayName =
+      // TODO(server-10): simplify away conditional
+      textNormalized.isEmpty ? store.realmEmptyTopicDisplayName : textNormalized;


This logic is tricky and nonlocal in a way I'd rather avoid. It makes sense only after you think through the fact that for textNormalized to be empty, we must be on a new server supporting "general chat", because otherwise we'd have set it to "(no topic)".

gnprice · 2025-03-20T22:42:16Z

lib/widgets/compose_box.dart

+    final textTrimmed = widget.controller.topic.text.trim();
+    if (textTrimmed.isNotEmpty) {
+      return zulipLocalizations.composeBoxChannelContentHint(
+        '#$streamName > $textTrimmed');
+    }
+
+    assert(widget.controller.topic._isTopicVacuous);


I don't like how there are two similar but not quite aligned conditions governing this logic:

textTrimmed empty vs. not empty

topic vacuous vs. not vacuous

The assert helps. But there's still an overlap case where the topic is vacuous, yet textTrimmed is not empty. That's if the user has entered literally "(no topic)" or "general chat".

How about we instead handle that overlap case like the other vacuous cases, instead of the other not-literally-empty cases? That's the way the validation handles it, after all — so the send button will be disabled. Then I think we no longer need the textTrimmed.isNotEmpty condition.

gnprice · 2025-03-20T23:38:42Z

I think a good solution for a couple of my comments below may be clearest by illustration. I'll send a draft PR shortly to do that.

→ #1425

PIG208 · 2025-03-21T20:27:30Z

Thanks! #1425 is super helpful. The PR has been updated.

gnprice

Thanks! Generally this all looks good now; a few comments below.

gnprice · 2025-03-21T22:54:20Z

lib/widgets/compose_box.dart

+    bool result = textNormalized.isEmpty
+      // We keep checking for '(no topic)' regardless of the feature level
+      // because it remains equivalent to an empty topic even when FL >= 334.
+      // This can change in the future:
+      //   https://chat.zulip.org/#narrow/channel/412-api-documentation/topic/.28realm_.29mandatory_topics.20behavior/near/2062391
+      || textNormalized == kNoTopicTopic;


nit:

Suggested change

bool result = textNormalized.isEmpty

// We keep checking for '(no topic)' regardless of the feature level

// because it remains equivalent to an empty topic even when FL >= 334.

// This can change in the future:

// https://chat.zulip.org/#narrow/channel/412-api-documentation/topic/.28realm_.29mandatory_topics.20behavior/near/2062391

|| textNormalized == kNoTopicTopic;

if (textNormalized.isEmpty) return true;

// We keep checking for '(no topic)' regardless of the feature level

// because it remains equivalent to an empty topic even when FL >= 334.

// This can change in the future:

// https://chat.zulip.org/#narrow/channel/412-api-documentation/topic/.28realm_.29mandatory_topics.20behavior/near/2062391

if (textNormalized == kNoTopicTopic) return true;

Alternatively, could put the whole getter in the form of a boolean expression. It's the mixture of forms that feels a bit harder to read.

Added a link to the updated API documentation (send-message#parameter-topic) and remove the block of comment about how "(no topic)" check can change "in the future".

gnprice · 2025-03-21T23:03:35Z

test/widgets/compose_box_test.dart

+
+      testWidgets('legacy: with empty topic', (tester) async {
+        await prepare(tester, narrow: narrow, mandatoryTopics: true,
+          zulipFeatureLevel: 333);


Suggested change

zulipFeatureLevel: 333);

zulipFeatureLevel: 333); // TODO(server-10)

Usually tests don't need TODO-server comments, because usually (if they're about old-server code at all) their point is to test some behavior that we're going to delete when we drop support for the old server — so the test will break, and we'll naturally find it so as to clean it out too.

But here this is identical to a non-legacy test above, except for the feature level it sets. (I believe its purpose is to test that some old-server code we have doesn't have any effect in a certain case.) So it looks like it would continue to pass if we deleted that code and didn't notice this test. Therefore that normal logic doesn't apply, and it should get a TODO-server comment.

Similarly one of the tests above.

gnprice · 2025-03-21T23:08:01Z

test/widgets/compose_box_test.dart

+          contentHintText: 'Message #${channel.name}');
+      });
+
+      testWidgets('legacy: with empty topic', (tester) async {


nit: the ordering of this among the other test cases doesn't seem logical: it's separating these two non-legacy test cases from each other, but without getting the benefit of being right next to the non-legacy test case that directly corresponds to it.

Either after all three cases, or right after the non-legacy "with empty topic", would be a logical home.

gnprice · 2025-03-21T23:08:45Z

test/widgets/compose_box_test.dart

+          contentHintText: 'Message #${channel.name}');
+      });
+
+      testWidgets('with non-empty but vacuous topic', (tester) async {


It looks like this case is missing a legacy variant (with (no topic) as the contents of the topic input).

Just caught up with zulip/zulip#34030 (server code to interpret (no topic) as an empty string. I think this check will stick around, and the test will be non-legacy. Adding that in the new revision.

PIG208 · 2025-03-21T23:48:48Z

Pushed an update and replied to some comments. Thanks for the review!

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]>

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. A test is skipped because the server does not send empty topics to the client without "empty_topic_name" client capability. Signed-off-by: Zixuan James Li <[email protected]>

This method is trivial for now, but provides a home for logic to be added next. In particular this separates the computation of how the topic should appear in the hint text from what the topic should be in the API, in `destination`.

Previously, "Message #stream > (no topic)" would appear as the hint text when the topic input is empty but mandatory. Now, it is shown as "Message #stream" instead, since the "(no topic)" isn't allowed when topics are mandatory. Co-authored-by: Zixuan James Li <[email protected]>

This does not rely on TopicName.displayName being non-nullable or "empty_topic_name" client capability, so it is not an NFC change. The key change that allows sending to empty topic is that `textNormalized` can now be actually empty, instead of being converted to "(no topic)" with `_computeTextNormalized`. --- This is accompanied with a content input hint text change, so that "Message #stream > general chat" appears appropriately when we make TopicName.displayName nullable. --- Previously, "Message #stream > (no topic)" was the hint text for content input as long as the topic input is empty and topics are not mandatory. Showing the default topic does not help create incentive for the user to pick a topic first. So only show it when they intend to leave the topic empty. See discussion: https://chat.zulip.org/#narrow/channel/530-mobile-design/topic/general.20chat.20design.20.23F1297/near/2088870 --- This does not aim to implement hint text changes to the topic input, which is always "Topic". We will handle that as a follow-up. Signed-off-by: Zixuan James Li <[email protected]>

gnprice · 2025-03-24T20:03:35Z

Thanks! Looks good; merging, with those "do not merge" commits at the end stripped off:
cca33c4 do not merge; api: Make displayName nullable
7646cb4 do not merge; api: Indicate support for handling empty topics

This was referenced Feb 20, 2025

Get compose box hint texts ready for general chat #1342

Merged

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

Merged

PIG208 force-pushed the pr-general-chat-2 branch from 5382b42 to d2a820f Compare February 22, 2025 00:14

PIG208 marked this pull request as ready for review February 22, 2025 00:16

PIG208 force-pushed the pr-general-chat-2 branch from d2a820f to 61d5a9c Compare February 22, 2025 00:22

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

PIG208 requested a review from chrisbobbe February 22, 2025 00:35

PIG208 assigned chrisbobbe Feb 22, 2025

PIG208 changed the title ~~general chat #2: support sending to empty topic, hide resolve/unresolve conditionally~~ general chat #2: support sending to empty topic from channel narrow, hide resolve/unresolve conditionally Feb 22, 2025

PIG208 force-pushed the pr-general-chat-2 branch from 61d5a9c to 5bfdaaa Compare February 22, 2025 01:06

PIG208 removed the request for review from chrisbobbe February 26, 2025 02:40

PIG208 unassigned chrisbobbe Feb 26, 2025

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

This comment was marked as resolved.

Sign in to view

PIG208 force-pushed the pr-general-chat-2 branch 2 times, most recently from dab6fce to 2e7db9c Compare February 26, 2025 22:31

PIG208 requested a review from chrisbobbe February 26, 2025 22:31

PIG208 assigned chrisbobbe Feb 26, 2025

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

PIG208 force-pushed the pr-general-chat-2 branch from 2e7db9c to 8c3db69 Compare February 26, 2025 23:01

PIG208 mentioned this pull request Feb 28, 2025

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

Merged

PIG208 force-pushed the pr-general-chat-2 branch 6 times, most recently from 4382c1e to f5eb2f9 Compare March 6, 2025 04:43

PIG208 force-pushed the pr-general-chat-2 branch from f5eb2f9 to 2b4285b Compare March 10, 2025 18:53

chrisbobbe reviewed Mar 12, 2025

View reviewed changes

PIG208 force-pushed the pr-general-chat-2 branch from 2b4285b to 6d3b73a Compare March 12, 2025 02:50

PIG208 force-pushed the pr-general-chat-2 branch from 6d3b73a to e67783a Compare March 12, 2025 22:49

PIG208 requested a review from chrisbobbe March 18, 2025 19:01

chrisbobbe reviewed Mar 19, 2025

View reviewed changes

chrisbobbe requested a review from gnprice March 19, 2025 21:36

chrisbobbe assigned gnprice and unassigned chrisbobbe Mar 19, 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 19, 2025

This comment was marked as resolved.

Sign in to view

PIG208 force-pushed the pr-general-chat-2 branch from e67783a to fb6963c Compare March 20, 2025 01:00

gnprice reviewed Mar 20, 2025

View reviewed changes

gnprice mentioned this pull request Mar 20, 2025

Adjust compose hints toward "general chat" #1425

Closed

PIG208 force-pushed the pr-general-chat-2 branch 3 times, most recently from 0131f00 to a579537 Compare March 21, 2025 20:26

gnprice reviewed Mar 21, 2025

View reviewed changes

PIG208 force-pushed the pr-general-chat-2 branch from a579537 to 7646cb4 Compare March 21, 2025 23:47

PIG208 and others added 5 commits March 24, 2025 12:59

gnprice force-pushed the pr-general-chat-2 branch from 7646cb4 to 769cc7d Compare March 24, 2025 20:03

gnprice merged commit 769cc7d into zulip:main Mar 24, 2025

PIG208 deleted the pr-general-chat-2 branch March 24, 2025 21:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

PIG208 commented Feb 19, 2025 •

edited

Loading

This comment was marked as resolved.

chrisbobbe left a comment

chrisbobbe Mar 12, 2025

chrisbobbe Mar 12, 2025

PIG208 Mar 12, 2025 •

edited

Loading

chrisbobbe Mar 12, 2025

chrisbobbe Mar 12, 2025

PIG208 commented Mar 12, 2025

chrisbobbe left a comment

chrisbobbe Mar 19, 2025

PIG208 Mar 19, 2025

This comment was marked as resolved.

gnprice left a comment

gnprice Mar 20, 2025

gnprice Mar 20, 2025

gnprice Mar 20, 2025

gnprice Mar 20, 2025

gnprice Mar 20, 2025

gnprice Mar 20, 2025

gnprice Mar 20, 2025

gnprice Mar 20, 2025

gnprice commented Mar 20, 2025

PIG208 commented Mar 21, 2025

gnprice left a comment

gnprice Mar 21, 2025

PIG208 Mar 21, 2025

gnprice Mar 21, 2025

gnprice Mar 21, 2025

gnprice Mar 21, 2025

PIG208 Mar 21, 2025

PIG208 commented Mar 21, 2025

gnprice commented Mar 24, 2025

	initNarrow: eg.topicNarrow(effectiveChannel.streamId, topicName.apiName))));
	initNarrow: TopicNarrow(effectiveChannel.streamId, topicName))));

	zulipFeatureLevel: 333);
	zulipFeatureLevel: 333); // TODO(server-10)

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

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

Conversation

PIG208 commented Feb 19, 2025 • edited Loading

This comment was marked as resolved.

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

PIG208 Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Mar 12, 2025

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

This comment was marked as resolved.

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

gnprice commented Mar 20, 2025

PIG208 commented Mar 21, 2025

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

PIG208 commented Mar 21, 2025

gnprice commented Mar 24, 2025

PIG208 commented Feb 19, 2025 •

edited

Loading

PIG208 Mar 12, 2025 •

edited

Loading