From 91699172c6fbd87293e1fcff65cc7ac7527ff65a Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Fri, 25 Apr 2025 18:54:10 -0400 Subject: [PATCH 1/8] test: Bump recentZulipFeatureLevel to FL 382, from FL 278 To make sure that recentZulipFeatureLevel is both recent and reasonable, the new value is taken from the changelog, at the time this was committed: https://github.com/zulip/zulip/blob/c6660fbea/api_docs/changelog.md?plain=1#L23 As a result, this assumes support for empty topics in our app code. To make sure we are aware of possible behavior changes in the app code we test against. Here's a helpful `git grep` command and its result (thanks to "zulipFeatureLevel" being quite a greppable name): ``` $ git grep 'zulipFeatureLevel [<>]' lib lib/api/model/narrow.dart: final supportsOperatorDm = zulipFeatureLevel >= 177; // TODO(server-7) lib/api/model/narrow.dart: final supportsOperatorWith = zulipFeatureLevel >= 271; // TODO(server-9) lib/model/autocomplete.dart: final isChannelWildcardAvailable = store.zulipFeatureLevel >= 247; // TODO(server-9) lib/model/autocomplete.dart: final isTopicWildcardAvailable = store.zulipFeatureLevel >= 224; // TODO(server-8) lib/model/compose.dart: final isChannelWildcardAvailable = store.zulipFeatureLevel >= 247; // TODO(server-9) lib/model/compose.dart: final isTopicWildcardAvailable = store.zulipFeatureLevel >= 224; // TODO(server-8) lib/model/store.dart: if (zulipFeatureLevel >= 163) { // TODO(server-7) lib/model/store.dart: bool get isUnsupported => zulipFeatureLevel < kMinSupportedZulipFeatureLevel; lib/widgets/action_sheet.dart: final supportsUnmutingTopics = store.zulipFeatureLevel >= 170; lib/widgets/action_sheet.dart: final supportsFollowingTopics = store.zulipFeatureLevel >= 219; lib/widgets/action_sheet.dart: final markAsUnreadSupported = store.zulipFeatureLevel >= 155; // TODO(server-6) lib/widgets/actions.dart: final useLegacy = store.zulipFeatureLevel < 155; // TODO(server-6) lib/widgets/actions.dart: assert(PerAccountStoreWidget.of(context).zulipFeatureLevel >= 155); // TODO(server-6) lib/widgets/autocomplete.dart: final isChannelWildcardAvailable = store.zulipFeatureLevel >= 247; // TODO(server-9) lib/widgets/compose_box.dart: if (store.zulipFeatureLevel < 334) { lib/widgets/compose_box.dart: if (store.zulipFeatureLevel >= 334) { ``` We can tell that this bump only affects 2 entries from above: ``` lib/widgets/compose_box.dart: if (store.zulipFeatureLevel < 334) { lib/widgets/compose_box.dart: if (store.zulipFeatureLevel >= 334) { ``` All are related to the FL 334 (general chat) changes. --- test/example_data.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/example_data.dart b/test/example_data.dart index fc3acfc5a4..7b517aca79 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -77,7 +77,7 @@ final Uri realmUrl = Uri.parse('https://chat.example/'); Uri get _realmUrl => realmUrl; const String recentZulipVersion = '9.0'; -const int recentZulipFeatureLevel = 278; +const int recentZulipFeatureLevel = 382; const int futureZulipFeatureLevel = 9999; const int ancientZulipFeatureLevel = kMinSupportedZulipFeatureLevel - 1; From 7f203273d57ec7985a7e1c39c40fbe20c90f51b5 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Fri, 25 Apr 2025 18:54:10 -0400 Subject: [PATCH 2/8] store [nfc]: Assert that FL>=334 when accessing realmEmptyTopicName --- lib/model/store.dart | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/model/store.dart b/lib/model/store.dart index b3b1b62b98..300b7dc22f 100644 --- a/lib/model/store.dart +++ b/lib/model/store.dart @@ -577,6 +577,7 @@ class PerAccountStore extends PerAccountStoreBase with ChangeNotifier, EmojiStor /// be empty otherwise. // TODO(server-10) simplify this String get realmEmptyTopicDisplayName { + assert(zulipFeatureLevel >= 334); assert(_realmEmptyTopicDisplayName != null); // TODO(log) return _realmEmptyTopicDisplayName ?? 'general chat'; } From 5700b42961097ef44cb44edae34da95993703309 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Mon, 24 Mar 2025 19:17:54 -0400 Subject: [PATCH 3/8] compose: Fix empty topics not being shown correctly in hint text Before, if the user chooses an empty topic, the hint text might appear as: "#issues > ". Now, with this fix, the hint text will be (correctly): "#issues > general chat" This buggy logic was introduced in 769cc7df, which assumed that TopicName.displayName is `null` when the topic is empty. TopicName that came from the server are guaranteed to be non-empty, but here our code can construct an empty TopicName, breaking this assumption. To enable this fix while the rest of the app still relies on TopicName.displayName being non-nullable, switch to using plain strings here for now. Later once TopicName.displayName becomes nullable, we'll go back to constructing a TopicName here. --- lib/widgets/compose_box.dart | 12 ++++++------ test/widgets/compose_box_test.dart | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 11d27d5402..1c03cf34b2 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -624,7 +624,7 @@ class _StreamContentInputState extends State<_StreamContentInput> { } /// The topic name to show in the hint text, or null to show no topic. - TopicName? _hintTopic() { + String? _hintTopicStr() { if (widget.controller.topic.isTopicVacuous) { if (widget.controller.topic.mandatory) { // The chosen topic can't be sent to, so don't show it. @@ -639,7 +639,7 @@ class _StreamContentInputState extends State<_StreamContentInput> { } } - return TopicName(widget.controller.topic.textNormalized); + return widget.controller.topic.textNormalized; } @override @@ -649,15 +649,15 @@ class _StreamContentInputState extends State<_StreamContentInput> { final streamName = store.streams[widget.narrow.streamId]?.name ?? zulipLocalizations.unknownChannelName; - final hintTopic = _hintTopic(); - final hintDestination = hintTopic == null + final hintTopicStr = _hintTopicStr(); + final hintDestination = hintTopicStr == null // No i18n of this use of "#" and ">" string; 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 ? '#$streamName' - // ignore: dead_null_aware_expression // null topic names soon to be enabled - : '#$streamName > ${hintTopic.displayName ?? store.realmEmptyTopicDisplayName}'; + : '#$streamName > ' + '${hintTopicStr.isEmpty ? store.realmEmptyTopicDisplayName : hintTopicStr}'; return _TypingNotifier( destination: TopicNarrow(widget.narrow.streamId, diff --git a/test/widgets/compose_box_test.dart b/test/widgets/compose_box_test.dart index 65740a8d1e..44be72d3ff 100644 --- a/test/widgets/compose_box_test.dart +++ b/test/widgets/compose_box_test.dart @@ -404,7 +404,7 @@ void main() { topicHintText: 'Topic', contentHintText: 'Message #${channel.name} > ' '${eg.defaultRealmEmptyTopicDisplayName}'); - }, skip: true); // null topic names soon to be enabled + }); testWidgets('legacy: with empty topic, content input has focus', (tester) async { await prepare(tester, narrow: narrow, mandatoryTopics: false, From 25eace6ba100498e33010f5fc4ec272c7aa1e0cb Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Mon, 14 Apr 2025 16:11:20 -0400 Subject: [PATCH 4/8] compose [nfc]: Convert _TopicInput to a StatefulWidget --- lib/widgets/compose_box.dart | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 1c03cf34b2..eee6d30cdc 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -670,12 +670,17 @@ class _StreamContentInputState extends State<_StreamContentInput> { } } -class _TopicInput extends StatelessWidget { +class _TopicInput extends StatefulWidget { const _TopicInput({required this.streamId, required this.controller}); final int streamId; final StreamComposeBoxController controller; + @override + State<_TopicInput> createState() => _TopicInputState(); +} + +class _TopicInputState extends State<_TopicInput> { @override Widget build(BuildContext context) { final zulipLocalizations = ZulipLocalizations.of(context); @@ -687,18 +692,18 @@ class _TopicInput extends StatelessWidget { ).merge(weightVariableTextStyle(context, wght: 600)); return TopicAutocomplete( - streamId: streamId, - controller: controller.topic, - focusNode: controller.topicFocusNode, - contentFocusNode: controller.contentFocusNode, + streamId: widget.streamId, + controller: widget.controller.topic, + focusNode: widget.controller.topicFocusNode, + contentFocusNode: widget.controller.contentFocusNode, fieldViewBuilder: (context) => Container( padding: const EdgeInsets.only(top: 10, bottom: 9), decoration: BoxDecoration(border: Border(bottom: BorderSide( width: 1, color: designVariables.foreground.withFadedAlpha(0.2)))), child: TextField( - controller: controller.topic, - focusNode: controller.topicFocusNode, + controller: widget.controller.topic, + focusNode: widget.controller.topicFocusNode, textInputAction: TextInputAction.next, style: topicTextStyle, decoration: InputDecoration( From b4344c723f162b8b183a0fcab7a5a671c393bb6b Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Mon, 14 Apr 2025 17:41:40 -0400 Subject: [PATCH 5/8] compose: Change topic input hint text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is similar to web's behavior. When topics are not mandatory: - an alternative hint text "Enter a topic (skip for “general chat”)" is shown when the topic input has focus; - an opaque placeholder text (e.g.: "general chat") is shown if the user skipped to content input; Because the topic input is always shown in a message list page channel narrow (assuming permission to send messages), this also adds an initial state: - a short hint text, "Topic", is shown if the user hasn't interacted with topic or content inputs at all, or when the user unfocused topic input without moving focus to content input. This only changes the topic input's hint text. See CZO discussion for design details: https://chat.zulip.org/#narrow/channel/530-mobile-design/topic/general.20chat.20design.20.23F1297/near/2106736 --- assets/l10n/app_en.arb | 7 + lib/generated/l10n/zulip_localizations.dart | 6 + .../l10n/zulip_localizations_ar.dart | 5 + .../l10n/zulip_localizations_en.dart | 5 + .../l10n/zulip_localizations_ja.dart | 5 + .../l10n/zulip_localizations_nb.dart | 5 + .../l10n/zulip_localizations_pl.dart | 5 + .../l10n/zulip_localizations_ru.dart | 5 + .../l10n/zulip_localizations_sk.dart | 5 + .../l10n/zulip_localizations_uk.dart | 5 + lib/widgets/compose_box.dart | 156 +++++++++++++++++- test/flutter_checks.dart | 2 + test/widgets/compose_box_test.dart | 67 +++++++- 13 files changed, 268 insertions(+), 10 deletions(-) diff --git a/assets/l10n/app_en.arb b/assets/l10n/app_en.arb index ea2e10cff3..d2d7b53033 100644 --- a/assets/l10n/app_en.arb +++ b/assets/l10n/app_en.arb @@ -379,6 +379,13 @@ "@composeBoxTopicHintText": { "description": "Hint text for topic input widget in compose box." }, + "composeBoxEnterTopicOrSkipHintText": "Enter a topic (skip for “{defaultTopicName}”)", + "@composeBoxEnterTopicOrSkipHintText": { + "description": "Hint text for topic input widget in compose box when topics are optional.", + "placeholders": { + "defaultTopicName": {"type": "String", "example": "general chat"} + } + }, "composeBoxUploadingFilename": "Uploading {filename}…", "@composeBoxUploadingFilename": { "description": "Placeholder in compose box showing the specified file is currently uploading.", diff --git a/lib/generated/l10n/zulip_localizations.dart b/lib/generated/l10n/zulip_localizations.dart index e326703e4b..6181c7b39b 100644 --- a/lib/generated/l10n/zulip_localizations.dart +++ b/lib/generated/l10n/zulip_localizations.dart @@ -622,6 +622,12 @@ abstract class ZulipLocalizations { /// **'Topic'** String get composeBoxTopicHintText; + /// Hint text for topic input widget in compose box when topics are optional. + /// + /// In en, this message translates to: + /// **'Enter a topic (skip for “{defaultTopicName}”)'** + String composeBoxEnterTopicOrSkipHintText(String defaultTopicName); + /// Placeholder in compose box showing the specified file is currently uploading. /// /// In en, this message translates to: diff --git a/lib/generated/l10n/zulip_localizations_ar.dart b/lib/generated/l10n/zulip_localizations_ar.dart index 8d36fa6bd0..f26b9d017c 100644 --- a/lib/generated/l10n/zulip_localizations_ar.dart +++ b/lib/generated/l10n/zulip_localizations_ar.dart @@ -316,6 +316,11 @@ class ZulipLocalizationsAr extends ZulipLocalizations { @override String get composeBoxTopicHintText => 'Topic'; + @override + String composeBoxEnterTopicOrSkipHintText(String defaultTopicName) { + return 'Enter a topic (skip for “$defaultTopicName”)'; + } + @override String composeBoxUploadingFilename(String filename) { return 'Uploading $filename…'; diff --git a/lib/generated/l10n/zulip_localizations_en.dart b/lib/generated/l10n/zulip_localizations_en.dart index a74a2e1eaf..bfb14645d5 100644 --- a/lib/generated/l10n/zulip_localizations_en.dart +++ b/lib/generated/l10n/zulip_localizations_en.dart @@ -316,6 +316,11 @@ class ZulipLocalizationsEn extends ZulipLocalizations { @override String get composeBoxTopicHintText => 'Topic'; + @override + String composeBoxEnterTopicOrSkipHintText(String defaultTopicName) { + return 'Enter a topic (skip for “$defaultTopicName”)'; + } + @override String composeBoxUploadingFilename(String filename) { return 'Uploading $filename…'; diff --git a/lib/generated/l10n/zulip_localizations_ja.dart b/lib/generated/l10n/zulip_localizations_ja.dart index c11a3fae23..35088555e2 100644 --- a/lib/generated/l10n/zulip_localizations_ja.dart +++ b/lib/generated/l10n/zulip_localizations_ja.dart @@ -316,6 +316,11 @@ class ZulipLocalizationsJa extends ZulipLocalizations { @override String get composeBoxTopicHintText => 'Topic'; + @override + String composeBoxEnterTopicOrSkipHintText(String defaultTopicName) { + return 'Enter a topic (skip for “$defaultTopicName”)'; + } + @override String composeBoxUploadingFilename(String filename) { return 'Uploading $filename…'; diff --git a/lib/generated/l10n/zulip_localizations_nb.dart b/lib/generated/l10n/zulip_localizations_nb.dart index d23bd323fd..ae79d99ea9 100644 --- a/lib/generated/l10n/zulip_localizations_nb.dart +++ b/lib/generated/l10n/zulip_localizations_nb.dart @@ -316,6 +316,11 @@ class ZulipLocalizationsNb extends ZulipLocalizations { @override String get composeBoxTopicHintText => 'Topic'; + @override + String composeBoxEnterTopicOrSkipHintText(String defaultTopicName) { + return 'Enter a topic (skip for “$defaultTopicName”)'; + } + @override String composeBoxUploadingFilename(String filename) { return 'Uploading $filename…'; diff --git a/lib/generated/l10n/zulip_localizations_pl.dart b/lib/generated/l10n/zulip_localizations_pl.dart index e1a6bd45f4..15f61bd882 100644 --- a/lib/generated/l10n/zulip_localizations_pl.dart +++ b/lib/generated/l10n/zulip_localizations_pl.dart @@ -323,6 +323,11 @@ class ZulipLocalizationsPl extends ZulipLocalizations { @override String get composeBoxTopicHintText => 'Wątek'; + @override + String composeBoxEnterTopicOrSkipHintText(String defaultTopicName) { + return 'Enter a topic (skip for “$defaultTopicName”)'; + } + @override String composeBoxUploadingFilename(String filename) { return 'Przekazywanie $filename…'; diff --git a/lib/generated/l10n/zulip_localizations_ru.dart b/lib/generated/l10n/zulip_localizations_ru.dart index 78b68e812a..e4248179e2 100644 --- a/lib/generated/l10n/zulip_localizations_ru.dart +++ b/lib/generated/l10n/zulip_localizations_ru.dart @@ -324,6 +324,11 @@ class ZulipLocalizationsRu extends ZulipLocalizations { @override String get composeBoxTopicHintText => 'Тема'; + @override + String composeBoxEnterTopicOrSkipHintText(String defaultTopicName) { + return 'Enter a topic (skip for “$defaultTopicName”)'; + } + @override String composeBoxUploadingFilename(String filename) { return 'Загрузка $filename…'; diff --git a/lib/generated/l10n/zulip_localizations_sk.dart b/lib/generated/l10n/zulip_localizations_sk.dart index 193ac26d8e..baded578ba 100644 --- a/lib/generated/l10n/zulip_localizations_sk.dart +++ b/lib/generated/l10n/zulip_localizations_sk.dart @@ -316,6 +316,11 @@ class ZulipLocalizationsSk extends ZulipLocalizations { @override String get composeBoxTopicHintText => 'Topic'; + @override + String composeBoxEnterTopicOrSkipHintText(String defaultTopicName) { + return 'Enter a topic (skip for “$defaultTopicName”)'; + } + @override String composeBoxUploadingFilename(String filename) { return 'Uploading $filename…'; diff --git a/lib/generated/l10n/zulip_localizations_uk.dart b/lib/generated/l10n/zulip_localizations_uk.dart index c1898631fd..50b1029dd7 100644 --- a/lib/generated/l10n/zulip_localizations_uk.dart +++ b/lib/generated/l10n/zulip_localizations_uk.dart @@ -325,6 +325,11 @@ class ZulipLocalizationsUk extends ZulipLocalizations { @override String get composeBoxTopicHintText => 'Тема'; + @override + String composeBoxEnterTopicOrSkipHintText(String defaultTopicName) { + return 'Enter a topic (skip for “$defaultTopicName”)'; + } + @override String composeBoxUploadingFilename(String filename) { return 'Завантаження $filename…'; diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index eee6d30cdc..fe576f95fc 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -681,16 +681,115 @@ class _TopicInput extends StatefulWidget { } class _TopicInputState extends State<_TopicInput> { + void _topicOrContentFocusChanged() { + setState(() { + final status = widget.controller.topicInteractionStatus; + if (widget.controller.topicFocusNode.hasFocus) { + // topic input gains focus + status.value = ComposeTopicInteractionStatus.isEditing; + } else if (widget.controller.contentFocusNode.hasFocus) { + // content input gains focus + status.value = ComposeTopicInteractionStatus.hasChosen; + } else { + // neither input has focus, the new value of topicInteractionStatus + // depends on its previous value + if (status.value == ComposeTopicInteractionStatus.isEditing) { + // topic input loses focus + status.value = ComposeTopicInteractionStatus.notEditingNotChosen; + } else { + // content input loses focus; stay in hasChosen + assert(status.value == ComposeTopicInteractionStatus.hasChosen); + } + } + }); + } + + void _topicInteractionStatusChanged() { + setState(() { + // The actual state lives in widget.controller.topicInteractionStatus + }); + } + + @override + void initState() { + super.initState(); + widget.controller.topicFocusNode.addListener(_topicOrContentFocusChanged); + widget.controller.contentFocusNode.addListener(_topicOrContentFocusChanged); + widget.controller.topicInteractionStatus.addListener(_topicInteractionStatusChanged); + } + + @override + void didUpdateWidget(covariant _TopicInput oldWidget) { + super.didUpdateWidget(oldWidget); + if (oldWidget.controller != widget.controller) { + oldWidget.controller.topicFocusNode.removeListener(_topicOrContentFocusChanged); + widget.controller.topicFocusNode.addListener(_topicOrContentFocusChanged); + oldWidget.controller.contentFocusNode.removeListener(_topicOrContentFocusChanged); + widget.controller.contentFocusNode.addListener(_topicOrContentFocusChanged); + oldWidget.controller.topicInteractionStatus.removeListener(_topicInteractionStatusChanged); + widget.controller.topicInteractionStatus.addListener(_topicInteractionStatusChanged); + } + } + + @override + void dispose() { + widget.controller.topicFocusNode.removeListener(_topicOrContentFocusChanged); + widget.controller.contentFocusNode.removeListener(_topicOrContentFocusChanged); + widget.controller.topicInteractionStatus.removeListener(_topicInteractionStatusChanged); + super.dispose(); + } + @override Widget build(BuildContext context) { final zulipLocalizations = ZulipLocalizations.of(context); final designVariables = DesignVariables.of(context); - TextStyle topicTextStyle = TextStyle( + final store = PerAccountStoreWidget.of(context); + + final topicTextStyle = TextStyle( fontSize: 20, height: 22 / 20, color: designVariables.textInput.withFadedAlpha(0.9), ).merge(weightVariableTextStyle(context, wght: 600)); + // TODO(server-10) simplify away + final emptyTopicsSupported = store.zulipFeatureLevel >= 334; + + final String hintText; + TextStyle hintStyle = topicTextStyle.copyWith( + color: designVariables.textInput.withFadedAlpha(0.5)); + + if (store.realmMandatoryTopics) { + // Something short and not distracting. + hintText = zulipLocalizations.composeBoxTopicHintText; + } else { + switch (widget.controller.topicInteractionStatus.value) { + case ComposeTopicInteractionStatus.notEditingNotChosen: + // Something short and not distracting. + hintText = zulipLocalizations.composeBoxTopicHintText; + case ComposeTopicInteractionStatus.isEditing: + // The user is actively interacting with the input. Since topics are + // not mandatory, show a long hint text mentioning that they can be + // left empty. + hintText = zulipLocalizations.composeBoxEnterTopicOrSkipHintText( + emptyTopicsSupported + ? store.realmEmptyTopicDisplayName + : kNoTopicTopic); + case ComposeTopicInteractionStatus.hasChosen: + // The topic has likely been chosen. Since topics are not mandatory, + // show the default topic display name as if the user has entered that + // when they left the input empty. + if (emptyTopicsSupported) { + hintText = store.realmEmptyTopicDisplayName; + hintStyle = topicTextStyle.copyWith(fontStyle: FontStyle.italic); + } else { + hintText = kNoTopicTopic; + hintStyle = topicTextStyle; + } + } + } + + final decoration = InputDecoration(hintText: hintText, hintStyle: hintStyle); + return TopicAutocomplete( streamId: widget.streamId, controller: widget.controller.topic, @@ -706,10 +805,7 @@ class _TopicInputState extends State<_TopicInput> { focusNode: widget.controller.topicFocusNode, textInputAction: TextInputAction.next, style: topicTextStyle, - decoration: InputDecoration( - hintText: zulipLocalizations.composeBoxTopicHintText, - hintStyle: topicTextStyle.copyWith( - color: designVariables.textInput.withFadedAlpha(0.5)))))); + decoration: decoration))); } } @@ -1382,17 +1478,67 @@ sealed class ComposeBoxController { } } +/// Represent how a user has interacted with topic and content inputs. +/// +/// State-transition diagram: +/// +/// ``` +/// (default) +/// Topic input │ Content input +/// lost focus. ▼ gained focus. +/// ┌────────────► notEditingNotChosen ────────────┐ +/// │ │ │ +/// │ Topic input │ │ +/// │ gained focus. │ │ +/// │ ◄─────────────────────────┘ ▼ +/// isEditing ◄───────────────────────────── hasChosen +/// │ Focus moved from ▲ │ ▲ +/// │ content to topic. │ │ │ +/// │ │ │ │ +/// └──────────────────────────────────────┘ └─────┘ +/// Focus moved from Content input loses focus +/// topic to content. without topic input gaining it. +/// ``` +/// +/// This state machine offers the following invariants: +/// - When topic input has focus, the status must be [isEditing]. +/// - When content input has focus, the status must be [hasChosen]. +/// - When neither input has focus, and content input was the last +/// input among the two to be focused, the status must be [hasChosen]. +/// - Otherwise, the status must be [notEditingNotChosen]. +enum ComposeTopicInteractionStatus { + /// The topic has likely not been chosen if left empty, + /// and is not being actively edited. + /// + /// When in this status neither the topic input nor the content input has focus. + notEditingNotChosen, + + /// The topic is being actively edited. + /// + /// When in this status, the topic input must have focus. + isEditing, + + /// The topic has likely been chosen, even if it is left empty. + /// + /// When in this status, the topic input must have no focus; + /// the content input might have focus. + hasChosen, +} + class StreamComposeBoxController extends ComposeBoxController { StreamComposeBoxController({required PerAccountStore store}) : topic = ComposeTopicController(store: store); final ComposeTopicController topic; final topicFocusNode = FocusNode(); + final ValueNotifier topicInteractionStatus = + ValueNotifier(ComposeTopicInteractionStatus.notEditingNotChosen); @override void dispose() { topic.dispose(); topicFocusNode.dispose(); + topicInteractionStatus.dispose(); super.dispose(); } } diff --git a/test/flutter_checks.dart b/test/flutter_checks.dart index df2777aac6..295dcde7b9 100644 --- a/test/flutter_checks.dart +++ b/test/flutter_checks.dart @@ -83,6 +83,7 @@ extension TextStyleChecks on Subject { Subject get inherit => has((t) => t.inherit, 'inherit'); Subject get color => has((t) => t.color, 'color'); Subject get fontSize => has((t) => t.fontSize, 'fontSize'); + Subject get fontStyle => has((t) => t.fontStyle, 'fontStyle'); Subject get fontWeight => has((t) => t.fontWeight, 'fontWeight'); Subject get letterSpacing => has((t) => t.letterSpacing, 'letterSpacing'); Subject?> get fontVariations => has((t) => t.fontVariations, 'fontVariations'); @@ -228,6 +229,7 @@ extension ThemeDataChecks on Subject { extension InputDecorationChecks on Subject { Subject get hintText => has((x) => x.hintText, 'hintText'); + Subject get hintStyle => has((x) => x.hintStyle, 'hintStyle'); } extension TextFieldChecks on Subject { diff --git a/test/widgets/compose_box_test.dart b/test/widgets/compose_box_test.dart index 44be72d3ff..0048b38073 100644 --- a/test/widgets/compose_box_test.dart +++ b/test/widgets/compose_box_test.dart @@ -372,7 +372,8 @@ void main() { await enterTopic(tester, narrow: narrow, topic: ''); await tester.pump(); checkComposeBoxHintTexts(tester, - topicHintText: 'Topic', + topicHintText: 'Enter a topic ' + '(skip for “${eg.defaultRealmEmptyTopicDisplayName}”)', contentHintText: 'Message #${channel.name}'); }); @@ -382,7 +383,7 @@ void main() { await enterTopic(tester, narrow: narrow, topic: ''); await tester.pump(); checkComposeBoxHintTexts(tester, - topicHintText: 'Topic', + topicHintText: 'Enter a topic (skip for “(no topic)”)', contentHintText: 'Message #${channel.name}'); }); @@ -391,6 +392,40 @@ void main() { await enterTopic(tester, narrow: narrow, topic: eg.defaultRealmEmptyTopicDisplayName); await tester.pump(); + checkComposeBoxHintTexts(tester, + topicHintText: 'Enter a topic ' + '(skip for “${eg.defaultRealmEmptyTopicDisplayName}”)', + contentHintText: 'Message #${channel.name}'); + }); + + testWidgets('with empty topic, topic input has focus, then content input gains focus', (tester) async { + await prepare(tester, narrow: narrow, mandatoryTopics: false); + await enterTopic(tester, narrow: narrow, topic: ''); + await tester.pump(); + checkComposeBoxHintTexts(tester, + topicHintText: 'Enter a topic ' + '(skip for “${eg.defaultRealmEmptyTopicDisplayName}”)', + contentHintText: 'Message #${channel.name}'); + + await enterContent(tester, ''); + await tester.pump(); + checkComposeBoxHintTexts(tester, + topicHintText: eg.defaultRealmEmptyTopicDisplayName, + contentHintText: 'Message #${channel.name} > ' + '${eg.defaultRealmEmptyTopicDisplayName}'); + }); + + testWidgets('with empty topic, topic input has focus, then loses it', (tester) async { + await prepare(tester, narrow: narrow, mandatoryTopics: false); + await enterTopic(tester, narrow: narrow, topic: ''); + await tester.pump(); + checkComposeBoxHintTexts(tester, + topicHintText: 'Enter a topic ' + '(skip for “${eg.defaultRealmEmptyTopicDisplayName}”)', + contentHintText: 'Message #${channel.name}'); + + FocusManager.instance.primaryFocus!.unfocus(); + await tester.pump(); checkComposeBoxHintTexts(tester, topicHintText: 'Topic', contentHintText: 'Message #${channel.name}'); @@ -401,9 +436,11 @@ void main() { await enterContent(tester, ''); await tester.pump(); checkComposeBoxHintTexts(tester, - topicHintText: 'Topic', + topicHintText: eg.defaultRealmEmptyTopicDisplayName, contentHintText: 'Message #${channel.name} > ' '${eg.defaultRealmEmptyTopicDisplayName}'); + check(tester.widget(topicInputFinder)).decoration.isNotNull() + .hintStyle.isNotNull().fontStyle.equals(FontStyle.italic); }); testWidgets('legacy: with empty topic, content input has focus', (tester) async { @@ -412,8 +449,27 @@ void main() { await enterContent(tester, ''); await tester.pump(); checkComposeBoxHintTexts(tester, - topicHintText: 'Topic', + topicHintText: '(no topic)', contentHintText: 'Message #${channel.name} > (no topic)'); + check(tester.widget(topicInputFinder)).decoration.isNotNull() + .hintStyle.isNotNull().fontStyle.isNull(); + }); + + testWidgets('with empty topic, content input has focus, then topic input gains focus', (tester) async { + await prepare(tester, narrow: narrow, mandatoryTopics: false); + await enterContent(tester, ''); + await tester.pump(); + checkComposeBoxHintTexts(tester, + topicHintText: eg.defaultRealmEmptyTopicDisplayName, + contentHintText: 'Message #${channel.name} > ' + '${eg.defaultRealmEmptyTopicDisplayName}'); + + await enterTopic(tester, narrow: narrow, topic: ''); + await tester.pump(); + checkComposeBoxHintTexts(tester, + topicHintText: 'Enter a topic ' + '(skip for “${eg.defaultRealmEmptyTopicDisplayName}”)', + contentHintText: 'Message #${channel.name}'); }); testWidgets('with non-empty topic', (tester) async { @@ -421,7 +477,8 @@ void main() { await enterTopic(tester, narrow: narrow, topic: 'new topic'); await tester.pump(); checkComposeBoxHintTexts(tester, - topicHintText: 'Topic', + topicHintText: 'Enter a topic ' + '(skip for “${eg.defaultRealmEmptyTopicDisplayName}”)', contentHintText: 'Message #${channel.name} > new topic'); }); }); From 070e7206220726ee8808d7e91a71da808123fadc Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Mon, 14 Apr 2025 17:45:42 -0400 Subject: [PATCH 6/8] compose: Change content input hint text statefully Before, the content input, when empty, shows the "#stream > general chat" hint text as long as it has focus, and shows "#stream" as the hint text when it loses focus. Now, when the content input is empty, it still shows "#stream > general chat" when it gains focus, except that it will keep showing it even after losing focus, until the user moves focus to the topic input. --- lib/widgets/compose_box.dart | 22 +++++++++++++++++----- test/widgets/compose_box_test.dart | 17 +++++++++++++++++ 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index fe576f95fc..433f113da9 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -596,11 +596,18 @@ class _StreamContentInputState extends State<_StreamContentInput> { }); } + void _topicInteractionStatusChanged() { + setState(() { + // The relevant state lives on widget.controller.topicInteractionStatus itself. + }); + } + @override void initState() { super.initState(); widget.controller.topic.addListener(_topicChanged); widget.controller.contentFocusNode.addListener(_contentFocusChanged); + widget.controller.topicInteractionStatus.addListener(_topicInteractionStatusChanged); } @override @@ -614,12 +621,17 @@ class _StreamContentInputState extends State<_StreamContentInput> { oldWidget.controller.contentFocusNode.removeListener(_contentFocusChanged); widget.controller.contentFocusNode.addListener(_contentFocusChanged); } + if (widget.controller.topicInteractionStatus != oldWidget.controller.topicInteractionStatus) { + oldWidget.controller.topicInteractionStatus.removeListener(_topicInteractionStatusChanged); + widget.controller.topicInteractionStatus.addListener(_topicInteractionStatusChanged); + } } @override void dispose() { widget.controller.topic.removeListener(_topicChanged); widget.controller.contentFocusNode.removeListener(_contentFocusChanged); + widget.controller.topicInteractionStatus.removeListener(_topicInteractionStatusChanged); super.dispose(); } @@ -630,11 +642,11 @@ class _StreamContentInputState extends State<_StreamContentInput> { // The chosen topic can't be sent to, so don't show it. return null; } - if (!widget.controller.contentFocusNode.hasFocus) { - // Do not fall back to a vacuous topic unless the user explicitly chooses - // to do so (by skipping topic input and moving focus to content input), - // so that the user is not encouraged to use vacuous topic when they - // have not interacted with the inputs at all. + if (widget.controller.topicInteractionStatus.value != + ComposeTopicInteractionStatus.hasChosen) { + // Do not fall back to a vacuous topic unless the user explicitly + // chooses to do so, so that the user is not encouraged to use vacuous + // topic before they have interacted with the inputs at all. return null; } } diff --git a/test/widgets/compose_box_test.dart b/test/widgets/compose_box_test.dart index 0048b38073..08106f8be0 100644 --- a/test/widgets/compose_box_test.dart +++ b/test/widgets/compose_box_test.dart @@ -472,6 +472,23 @@ void main() { contentHintText: 'Message #${channel.name}'); }); + testWidgets('with empty topic, content input has focus, then loses it', (tester) async { + await prepare(tester, narrow: narrow, mandatoryTopics: false); + await enterContent(tester, ''); + await tester.pump(); + checkComposeBoxHintTexts(tester, + topicHintText: eg.defaultRealmEmptyTopicDisplayName, + contentHintText: 'Message #${channel.name} > ' + '${eg.defaultRealmEmptyTopicDisplayName}'); + + FocusManager.instance.primaryFocus!.unfocus(); + await tester.pump(); + checkComposeBoxHintTexts(tester, + topicHintText: eg.defaultRealmEmptyTopicDisplayName, + contentHintText: 'Message #${channel.name} > ' + '${eg.defaultRealmEmptyTopicDisplayName}'); + }); + testWidgets('with non-empty topic', (tester) async { await prepare(tester, narrow: narrow, mandatoryTopics: false); await enterTopic(tester, narrow: narrow, topic: 'new topic'); From 4c198de6ea9c5d23e1e156cb21e4989c3eaac0d9 Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Tue, 21 Jan 2025 11:28:24 -0500 Subject: [PATCH 7/8] api: Make displayName nullable --- lib/api/model/model.dart | 6 +----- lib/model/autocomplete.dart | 2 -- lib/widgets/action_sheet.dart | 4 +--- lib/widgets/autocomplete.dart | 2 -- lib/widgets/compose_box.dart | 13 +++++-------- lib/widgets/inbox.dart | 2 -- lib/widgets/message_list.dart | 4 ---- test/api/model/model_checks.dart | 2 +- test/widgets/action_sheet_test.dart | 5 ++--- test/widgets/autocomplete_test.dart | 8 ++++---- test/widgets/compose_box_test.dart | 2 +- test/widgets/inbox_test.dart | 2 +- test/widgets/message_list_test.dart | 6 +++--- 13 files changed, 19 insertions(+), 39 deletions(-) diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index 4c12403b85..6b2cb4dd02 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -581,11 +581,7 @@ extension type const TopicName(String _value) { /// The string this topic is displayed as to the user in our UI. /// /// At the moment this always equals [apiName]. - /// In the future this will become null for the "general chat" topic (#1250), - /// so that UI code can identify when it needs to represent the topic - /// specially in the way prescribed for "general chat". - // TODO(#1250) carry out that plan - String get displayName => _value; + String? get displayName => _value.isEmpty ? null : _value; /// The key to use for "same topic as" comparisons. String canonicalize() => apiName.toLowerCase(); diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index 073255bc9d..8fee7620dc 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -942,13 +942,11 @@ class TopicAutocompleteQuery extends AutocompleteQuery { bool testTopic(TopicName topic, PerAccountStore store) { // TODO(#881): Sort by match relevance, like web does. - // ignore: unnecessary_null_comparison // null topic names soon to be enabled if (topic.displayName == null) { return store.realmEmptyTopicDisplayName.toLowerCase() .contains(raw.toLowerCase()); } return topic.displayName != raw - // ignore: unnecessary_non_null_assertion // null topic names soon to be enabled && topic.displayName!.toLowerCase().contains(raw.toLowerCase()); } diff --git a/lib/widgets/action_sheet.dart b/lib/widgets/action_sheet.dart index 88114d48bb..6aa7239a0c 100644 --- a/lib/widgets/action_sheet.dart +++ b/lib/widgets/action_sheet.dart @@ -304,9 +304,7 @@ void showTopicActionSheet(BuildContext context, { // TODO: check for other cases that may disallow this action (e.g.: time // limit for editing topics). - if (someMessageIdInTopic != null - // ignore: unnecessary_null_comparison // null topic names soon to be enabled - && topic.displayName != null) { + if (someMessageIdInTopic != null && topic.displayName != null) { optionButtons.add(ResolveUnresolveButton(pageContext: pageContext, topic: topic, someMessageIdInTopic: someMessageIdInTopic)); diff --git a/lib/widgets/autocomplete.dart b/lib/widgets/autocomplete.dart index a31369c3d9..a1956295eb 100644 --- a/lib/widgets/autocomplete.dart +++ b/lib/widgets/autocomplete.dart @@ -416,13 +416,11 @@ class TopicAutocomplete extends AutocompleteField { } void setTopic(TopicName newTopic) { - // ignore: dead_null_aware_expression // null topic names soon to be enabled value = TextEditingValue(text: newTopic.displayName ?? ''); } } @@ -636,7 +635,7 @@ class _StreamContentInputState extends State<_StreamContentInput> { } /// The topic name to show in the hint text, or null to show no topic. - String? _hintTopicStr() { + TopicName? _hintTopic() { if (widget.controller.topic.isTopicVacuous) { if (widget.controller.topic.mandatory) { // The chosen topic can't be sent to, so don't show it. @@ -651,7 +650,7 @@ class _StreamContentInputState extends State<_StreamContentInput> { } } - return widget.controller.topic.textNormalized; + return TopicName(widget.controller.topic.textNormalized); } @override @@ -661,15 +660,14 @@ class _StreamContentInputState extends State<_StreamContentInput> { final streamName = store.streams[widget.narrow.streamId]?.name ?? zulipLocalizations.unknownChannelName; - final hintTopicStr = _hintTopicStr(); - final hintDestination = hintTopicStr == null + final hintTopic = _hintTopic(); + final hintDestination = hintTopic == null // No i18n of this use of "#" and ">" string; 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 ? '#$streamName' - : '#$streamName > ' - '${hintTopicStr.isEmpty ? store.realmEmptyTopicDisplayName : hintTopicStr}'; + : '#$streamName > ${hintTopic.displayName ?? store.realmEmptyTopicDisplayName}'; return _TypingNotifier( destination: TopicNarrow(widget.narrow.streamId, @@ -842,7 +840,6 @@ class _FixedDestinationContentInput extends StatelessWidget { // 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 - // ignore: dead_null_aware_expression // null topic names soon to be enabled '#$streamName > ${topic.displayName ?? store.realmEmptyTopicDisplayName}'); case DmNarrow(otherRecipientIds: []): // The self-1:1 thread. diff --git a/lib/widgets/inbox.dart b/lib/widgets/inbox.dart index a8c0c12b59..0f6a5c75a1 100644 --- a/lib/widgets/inbox.dart +++ b/lib/widgets/inbox.dart @@ -546,14 +546,12 @@ class _TopicItem extends StatelessWidget { style: TextStyle( fontSize: 17, height: (20 / 17), - // ignore: unnecessary_null_comparison // null topic names soon to be enabled fontStyle: topic.displayName == null ? FontStyle.italic : null, // TODO(design) check if this is the right variable color: designVariables.labelMenuButton, ), maxLines: 2, overflow: TextOverflow.ellipsis, - // ignore: dead_null_aware_expression // null topic names soon to be enabled topic.displayName ?? store.realmEmptyTopicDisplayName))), const SizedBox(width: 12), if (hasMention) const _IconMarker(icon: ZulipIcons.at_sign), diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index ae7c51b67c..98beda3205 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -326,10 +326,8 @@ class MessageListAppBarTitle extends StatelessWidget { return Row( mainAxisSize: MainAxisSize.min, children: [ - // ignore: dead_null_aware_expression // null topic names soon to be enabled Flexible(child: Text(topic.displayName ?? store.realmEmptyTopicDisplayName, style: TextStyle( fontSize: 13, - // ignore: unnecessary_null_comparison // null topic names soon to be enabled fontStyle: topic.displayName == null ? FontStyle.italic : null, ).merge(weightVariableTextStyle(context)))), if (icon != null) @@ -1144,13 +1142,11 @@ class StreamMessageRecipientHeader extends StatelessWidget { child: Row( children: [ Flexible( - // ignore: dead_null_aware_expression // null topic names soon to be enabled child: Text(topic.displayName ?? store.realmEmptyTopicDisplayName, // TODO: Give a way to see the whole topic (maybe a // long-press interaction?) overflow: TextOverflow.ellipsis, style: recipientHeaderTextStyle(context, - // ignore: unnecessary_null_comparison // null topic names soon to be enabled fontStyle: topic.displayName == null ? FontStyle.italic : null, ))), const SizedBox(width: 4), diff --git a/test/api/model/model_checks.dart b/test/api/model/model_checks.dart index 8791c1b9d9..1a17f70f60 100644 --- a/test/api/model/model_checks.dart +++ b/test/api/model/model_checks.dart @@ -26,7 +26,7 @@ extension ZulipStreamChecks on Subject { extension TopicNameChecks on Subject { Subject get apiName => has((x) => x.apiName, 'apiName'); - Subject get displayName => has((x) => x.displayName, 'displayName'); + Subject get displayName => has((x) => x.displayName, 'displayName'); } extension StreamConversationChecks on Subject { diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart index 8aeeec4eed..deda8078e2 100644 --- a/test/widgets/action_sheet_test.dart +++ b/test/widgets/action_sheet_test.dart @@ -372,7 +372,6 @@ void main() { final topicRow = find.descendant( of: find.byType(ZulipAppBar), matching: find.text( - // ignore: dead_null_aware_expression // null topic names soon to be enabled effectiveTopic.displayName ?? eg.defaultRealmEmptyTopicDisplayName)); await tester.longPress(topicRow); // sheet appears onscreen; default duration of bottom-sheet enter animation @@ -393,7 +392,7 @@ void main() { await tester.longPress(find.descendant( of: find.byType(RecipientHeader), - matching: find.text(effectiveMessage.topic.displayName))); + matching: find.text(effectiveMessage.topic.displayName!))); // sheet appears onscreen; default duration of bottom-sheet enter animation await tester.pump(const Duration(milliseconds: 250)); } @@ -457,7 +456,7 @@ void main() { messages: [message]); check(findButtonForLabel('Mark as resolved')).findsNothing(); check(findButtonForLabel('Mark as unresolved')).findsNothing(); - }, skip: true); // null topic names soon to be enabled + }); testWidgets('show from recipient header', (tester) async { await prepare(); diff --git a/test/widgets/autocomplete_test.dart b/test/widgets/autocomplete_test.dart index 484c3b2454..b4ff007a8d 100644 --- a/test/widgets/autocomplete_test.dart +++ b/test/widgets/autocomplete_test.dart @@ -415,7 +415,7 @@ void main() { await tester.tap(find.text('Topic three')); await tester.pumpAndSettle(); check(tester.widget(topicInputFinder).controller!.text) - .equals(topic3.name.displayName); + .equals(topic3.name.displayName!); check(find.text('Topic one' )).findsNothing(); check(find.text('Topic two' )).findsNothing(); check(find.text('Topic three')).findsOne(); // shown in `_TopicInput` once @@ -473,7 +473,7 @@ void main() { await tester.pumpAndSettle(); check(find.text('some display name')).findsOne(); - }, skip: true); // null topic names soon to be enabled + }); testWidgets('match realmEmptyTopicDisplayName in autocomplete', (tester) async { final topic = eg.getStreamTopicsEntry(name: ''); @@ -486,7 +486,7 @@ void main() { await tester.pumpAndSettle(); check(find.text('general chat')).findsOne(); - }, skip: true); // null topic names soon to be enabled + }); testWidgets('autocomplete to realmEmptyTopicDisplayName sets topic to empty string', (tester) async { final topic = eg.getStreamTopicsEntry(name: ''); @@ -502,6 +502,6 @@ void main() { await tester.tap(find.text('general chat')); await tester.pump(Duration.zero); check(controller.value).text.equals(''); - }, skip: true); // null topic names soon to be enabled + }); }); } diff --git a/test/widgets/compose_box_test.dart b/test/widgets/compose_box_test.dart index 08106f8be0..f979c687de 100644 --- a/test/widgets/compose_box_test.dart +++ b/test/widgets/compose_box_test.dart @@ -563,7 +563,7 @@ void main() { narrow: TopicNarrow(channel.streamId, TopicName(''))); checkComposeBoxHintTexts(tester, contentHintText: 'Message #${channel.name} > ${eg.defaultRealmEmptyTopicDisplayName}'); - }, skip: true); // null topic names soon to be enabled + }); }); testWidgets('to DmNarrow with self', (tester) async { diff --git a/test/widgets/inbox_test.dart b/test/widgets/inbox_test.dart index 9fa5de1bf3..c91b70cb44 100644 --- a/test/widgets/inbox_test.dart +++ b/test/widgets/inbox_test.dart @@ -314,7 +314,7 @@ void main() { subscriptions: [(eg.subscription(channel))], unreadMessages: [eg.streamMessage(stream: channel, topic: '')]); check(find.text(eg.defaultRealmEmptyTopicDisplayName)).findsOne(); - }, skip: true); // null topic names soon to be enabled + }); group('topic visibility', () { final channel = eg.stream(); diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 047a036cb5..816cde5948 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -204,7 +204,7 @@ void main() { messageCount: 1); checkAppBarChannelTopic( channel.name, eg.defaultRealmEmptyTopicDisplayName); - }, skip: true); // null topic names soon to be enabled + }); testWidgets('has channel-feed action for topic narrows', (tester) async { final pushedRoutes = >[]; @@ -1015,7 +1015,7 @@ void main() { await tester.pump(); check(findInMessageList('stream name')).single; check(findInMessageList(eg.defaultRealmEmptyTopicDisplayName)).single; - }, skip: true); // null topic names soon to be enabled + }); testWidgets('show general chat for empty topics without channel name', (tester) async { await setupMessageListPage(tester, @@ -1024,7 +1024,7 @@ void main() { await tester.pump(); check(findInMessageList('stream name')).isEmpty(); check(findInMessageList(eg.defaultRealmEmptyTopicDisplayName)).single; - }, skip: true); // null topic names soon to be enabled + }); testWidgets('show topic visibility icon when followed', (tester) async { await setupMessageListPage(tester, From 000cc84e16d0f5ff64406d88f40a7aa813aa3bed Mon Sep 17 00:00:00 2001 From: Zixuan James Li Date: Thu, 16 Jan 2025 15:07:49 -0500 Subject: [PATCH 8/8] api: Indicate support for handling empty topics Look for `allow_empty_topic_name` and `empty_topic_name` under "Feature level 334" in the API Changelog to verify the affected routes: https://zulip.com/api/changelog To keep the API bindings thin, instead of setting `allow_empty_topic_name` for the callers, we require the callers to pass the appropriate values instead. Fixes: #1250 --- lib/api/route/channels.dart | 6 ++++- lib/api/route/events.dart | 1 + lib/api/route/messages.dart | 9 +++++++ lib/model/autocomplete.dart | 4 ++- lib/model/message_list.dart | 2 ++ lib/widgets/actions.dart | 1 + test/api/route/messages_test.dart | 41 +++++++++++++++++++++++++++-- test/model/autocomplete_test.dart | 17 ++++++++++++ test/model/message_list_test.dart | 4 +++ test/widgets/action_sheet_test.dart | 12 +++++++++ test/widgets/message_list_test.dart | 2 ++ 11 files changed, 95 insertions(+), 4 deletions(-) diff --git a/lib/api/route/channels.dart b/lib/api/route/channels.dart index bfa46f5ab8..8ae2076038 100644 --- a/lib/api/route/channels.dart +++ b/lib/api/route/channels.dart @@ -7,8 +7,12 @@ part 'channels.g.dart'; /// https://zulip.com/api/get-stream-topics Future getStreamTopics(ApiConnection connection, { required int streamId, + required bool allowEmptyTopicName, }) { - return connection.get('getStreamTopics', GetStreamTopicsResult.fromJson, 'users/me/$streamId/topics', {}); + assert(allowEmptyTopicName, '`allowEmptyTopicName` should only be true'); + return connection.get('getStreamTopics', GetStreamTopicsResult.fromJson, 'users/me/$streamId/topics', { + 'allow_empty_topic_name': allowEmptyTopicName, + }); } @JsonSerializable(fieldRename: FieldRename.snake) diff --git a/lib/api/route/events.dart b/lib/api/route/events.dart index bbd7be5a0a..bd14521c74 100644 --- a/lib/api/route/events.dart +++ b/lib/api/route/events.dart @@ -18,6 +18,7 @@ Future registerQueue(ApiConnection connection) { 'user_avatar_url_field_optional': false, // TODO(#254): turn on 'stream_typing_notifications': true, 'user_settings_object': true, + 'empty_topic_name': true, }, }); } diff --git a/lib/api/route/messages.dart b/lib/api/route/messages.dart index 449bf9fd80..ccafdbce45 100644 --- a/lib/api/route/messages.dart +++ b/lib/api/route/messages.dart @@ -16,6 +16,7 @@ part 'messages.g.dart'; Future getMessageCompat(ApiConnection connection, { required int messageId, bool? applyMarkdown, + required bool allowEmptyTopicName, }) async { final useLegacyApi = connection.zulipFeatureLevel! < 120; if (useLegacyApi) { @@ -25,6 +26,7 @@ Future getMessageCompat(ApiConnection connection, { numBefore: 0, numAfter: 0, applyMarkdown: applyMarkdown, + allowEmptyTopicName: allowEmptyTopicName, // Hard-code this param to `true`, as the new single-message API // effectively does: @@ -37,6 +39,7 @@ Future getMessageCompat(ApiConnection connection, { final response = await getMessage(connection, messageId: messageId, applyMarkdown: applyMarkdown, + allowEmptyTopicName: allowEmptyTopicName, ); return response.message; } on ZulipApiException catch (e) { @@ -57,10 +60,13 @@ Future getMessageCompat(ApiConnection connection, { Future getMessage(ApiConnection connection, { required int messageId, bool? applyMarkdown, + required bool allowEmptyTopicName, }) { + assert(allowEmptyTopicName, '`allowEmptyTopicName` should only be true'); assert(connection.zulipFeatureLevel! >= 120); return connection.get('getMessage', GetMessageResult.fromJson, 'messages/$messageId', { if (applyMarkdown != null) 'apply_markdown': applyMarkdown, + 'allow_empty_topic_name': allowEmptyTopicName, }); } @@ -89,8 +95,10 @@ Future getMessages(ApiConnection connection, { required int numAfter, bool? clientGravatar, bool? applyMarkdown, + required bool allowEmptyTopicName, // bool? useFirstUnreadAnchor // omitted because deprecated }) { + assert(allowEmptyTopicName, '`allowEmptyTopicName` should only be true'); return connection.get('getMessages', GetMessagesResult.fromJson, 'messages', { 'narrow': resolveApiNarrowForServer(narrow, connection.zulipFeatureLevel!), 'anchor': RawParameter(anchor.toJson()), @@ -99,6 +107,7 @@ Future getMessages(ApiConnection connection, { 'num_after': numAfter, if (clientGravatar != null) 'client_gravatar': clientGravatar, if (applyMarkdown != null) 'apply_markdown': applyMarkdown, + 'allow_empty_topic_name': allowEmptyTopicName, }); } diff --git a/lib/model/autocomplete.dart b/lib/model/autocomplete.dart index 8fee7620dc..034199521d 100644 --- a/lib/model/autocomplete.dart +++ b/lib/model/autocomplete.dart @@ -904,7 +904,9 @@ class TopicAutocompleteView extends AutocompleteView _fetch() async { assert(!_isFetching); _isFetching = true; - final result = await getStreamTopics(store.connection, streamId: streamId); + final result = await getStreamTopics(store.connection, streamId: streamId, + allowEmptyTopicName: true, + ); _topics = result.topics.map((e) => e.name); _isFetching = false; return _startSearch(); diff --git a/lib/model/message_list.dart b/lib/model/message_list.dart index 2e4c4fb600..275aa66363 100644 --- a/lib/model/message_list.dart +++ b/lib/model/message_list.dart @@ -495,6 +495,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { anchor: AnchorCode.newest, numBefore: kMessageListFetchBatchSize, numAfter: 0, + allowEmptyTopicName: true, ); if (this.generation > generation) return; _adjustNarrowForTopicPermalink(result.messages.firstOrNull); @@ -567,6 +568,7 @@ class MessageListView with ChangeNotifier, _MessageSequence { includeAnchor: false, numBefore: kMessageListFetchBatchSize, numAfter: 0, + allowEmptyTopicName: true, ); } catch (e) { hasFetchError = true; diff --git a/lib/widgets/actions.dart b/lib/widgets/actions.dart index ad4557be08..61032d81e1 100644 --- a/lib/widgets/actions.dart +++ b/lib/widgets/actions.dart @@ -260,6 +260,7 @@ abstract final class ZulipAction { fetchedMessage = await getMessageCompat(PerAccountStoreWidget.of(context).connection, messageId: messageId, applyMarkdown: false, + allowEmptyTopicName: true, ); if (fetchedMessage == null) { errorMessage = zulipLocalizations.errorMessageDoesNotSeemToExist; diff --git a/test/api/route/messages_test.dart b/test/api/route/messages_test.dart index aff49bd8af..2a881d2145 100644 --- a/test/api/route/messages_test.dart +++ b/test/api/route/messages_test.dart @@ -20,10 +20,12 @@ void main() { required bool expectLegacy, required int messageId, bool? applyMarkdown, + required bool allowEmptyTopicName, }) async { final result = await getMessageCompat(connection, messageId: messageId, applyMarkdown: applyMarkdown, + allowEmptyTopicName: allowEmptyTopicName, ); if (expectLegacy) { check(connection.lastRequest).isA() @@ -35,6 +37,7 @@ void main() { 'num_before': '0', 'num_after': '0', if (applyMarkdown != null) 'apply_markdown': applyMarkdown.toString(), + 'allow_empty_topic_name': allowEmptyTopicName.toString(), 'client_gravatar': 'true', }); } else { @@ -43,6 +46,7 @@ void main() { ..url.path.equals('/api/v1/messages/$messageId') ..url.queryParameters.deepEquals({ if (applyMarkdown != null) 'apply_markdown': applyMarkdown.toString(), + 'allow_empty_topic_name': allowEmptyTopicName.toString(), }); } return result; @@ -57,6 +61,7 @@ void main() { expectLegacy: false, messageId: message.id, applyMarkdown: true, + allowEmptyTopicName: true, ); check(result).isNotNull().jsonEquals(message); }); @@ -71,6 +76,7 @@ void main() { expectLegacy: false, messageId: message.id, applyMarkdown: true, + allowEmptyTopicName: true, ); check(result).isNull(); }); @@ -92,6 +98,7 @@ void main() { expectLegacy: true, messageId: message.id, applyMarkdown: true, + allowEmptyTopicName: true, ); check(result).isNotNull().jsonEquals(message); }); @@ -113,6 +120,7 @@ void main() { expectLegacy: true, messageId: message.id, applyMarkdown: true, + allowEmptyTopicName: true, ); check(result).isNull(); }); @@ -124,11 +132,13 @@ void main() { FakeApiConnection connection, { required int messageId, bool? applyMarkdown, + required bool allowEmptyTopicName, required Map expected, }) async { final result = await getMessage(connection, messageId: messageId, applyMarkdown: applyMarkdown, + allowEmptyTopicName: allowEmptyTopicName, ); check(connection.lastRequest).isA() ..method.equals('GET') @@ -145,7 +155,11 @@ void main() { await checkGetMessage(connection, messageId: 1, applyMarkdown: true, - expected: {'apply_markdown': 'true'}); + allowEmptyTopicName: true, + expected: { + 'apply_markdown': 'true', + 'allow_empty_topic_name': 'true', + }); }); }); @@ -155,7 +169,21 @@ void main() { await checkGetMessage(connection, messageId: 1, applyMarkdown: false, - expected: {'apply_markdown': 'false'}); + allowEmptyTopicName: true, + expected: { + 'apply_markdown': 'false', + 'allow_empty_topic_name': 'true', + }); + }); + }); + + test('allow empty topic name', () { + return FakeApiConnection.with_((connection) async { + connection.prepare(json: fakeResult.toJson()); + await checkGetMessage(connection, + messageId: 1, + allowEmptyTopicName: true, + expected: {'allow_empty_topic_name': 'true'}); }); }); @@ -164,6 +192,7 @@ void main() { connection.prepare(json: fakeResult.toJson()); check(() => getMessage(connection, messageId: 1, + allowEmptyTopicName: true, )).throws(); }); }); @@ -255,12 +284,14 @@ void main() { required int numAfter, bool? clientGravatar, bool? applyMarkdown, + required bool allowEmptyTopicName, required Map expected, }) async { final result = await getMessages(connection, narrow: narrow, anchor: anchor, includeAnchor: includeAnchor, numBefore: numBefore, numAfter: numAfter, clientGravatar: clientGravatar, applyMarkdown: applyMarkdown, + allowEmptyTopicName: allowEmptyTopicName, ); check(connection.lastRequest).isA() ..method.equals('GET') @@ -279,11 +310,13 @@ void main() { await checkGetMessages(connection, narrow: const CombinedFeedNarrow().apiEncode(), anchor: AnchorCode.newest, numBefore: 10, numAfter: 20, + allowEmptyTopicName: true, expected: { 'narrow': jsonEncode([]), 'anchor': 'newest', 'num_before': '10', 'num_after': '20', + 'allow_empty_topic_name': 'true', }); }); }); @@ -294,6 +327,7 @@ void main() { await checkGetMessages(connection, narrow: [ApiNarrowDm([123, 234])], anchor: AnchorCode.newest, numBefore: 10, numAfter: 20, + allowEmptyTopicName: true, expected: { 'narrow': jsonEncode([ {'operator': 'pm-with', 'operand': [123, 234]}, @@ -301,6 +335,7 @@ void main() { 'anchor': 'newest', 'num_before': '10', 'num_after': '20', + 'allow_empty_topic_name': 'true', }); }); }); @@ -312,11 +347,13 @@ void main() { narrow: const CombinedFeedNarrow().apiEncode(), anchor: const NumericAnchor(42), numBefore: 10, numAfter: 20, + allowEmptyTopicName: true, expected: { 'narrow': jsonEncode([]), 'anchor': '42', 'num_before': '10', 'num_after': '20', + 'allow_empty_topic_name': 'true', }); }); }); diff --git a/test/model/autocomplete_test.dart b/test/model/autocomplete_test.dart index 16b92d98d4..cab073db48 100644 --- a/test/model/autocomplete_test.dart +++ b/test/model/autocomplete_test.dart @@ -3,6 +3,7 @@ import 'dart:convert'; import 'package:checks/checks.dart'; import 'package:flutter/widgets.dart'; +import 'package:http/http.dart' as http; import 'package:test/scaffolding.dart'; import 'package:zulip/api/model/initial_snapshot.dart'; import 'package:zulip/api/model/model.dart'; @@ -19,6 +20,7 @@ import 'package:zulip/widgets/compose_box.dart'; import '../api/fake_api.dart'; import '../example_data.dart' as eg; import '../fake_async.dart'; +import '../stdlib_checks.dart'; import 'test_store.dart'; import 'autocomplete_checks.dart'; @@ -1026,6 +1028,21 @@ void main() { check(done).isTrue(); }); + test('TopicAutocompleteView getStreamTopics request', () async { + final store = eg.store(); + final connection = store.connection as FakeApiConnection; + + connection.prepare(json: GetStreamTopicsResult( + topics: [eg.getStreamTopicsEntry(name: '')], + ).toJson()); + TopicAutocompleteView.init(store: store, streamId: 1000, + query: TopicAutocompleteQuery('foo')); + check(connection.lastRequest).isA() + ..method.equals('GET') + ..url.path.equals('/api/v1/users/me/1000/topics') + ..url.queryParameters['allow_empty_topic_name'].equals('true'); + }); + group('TopicAutocompleteQuery.testTopic', () { final store = eg.store(); void doCheck(String rawQuery, String topic, bool expected) { diff --git a/test/model/message_list_test.dart b/test/model/message_list_test.dart index 4aedea1d03..539dbeaba7 100644 --- a/test/model/message_list_test.dart +++ b/test/model/message_list_test.dart @@ -82,6 +82,7 @@ void main() { bool? includeAnchor, required int numBefore, required int numAfter, + required bool allowEmptyTopicName, }) { check(connection.lastRequest).isA() ..method.equals('GET') @@ -92,6 +93,7 @@ void main() { if (includeAnchor != null) 'include_anchor': includeAnchor.toString(), 'num_before': numBefore.toString(), 'num_after': numAfter.toString(), + 'allow_empty_topic_name': allowEmptyTopicName.toString(), }); } @@ -126,6 +128,7 @@ void main() { anchor: 'newest', numBefore: kMessageListFetchBatchSize, numAfter: 0, + allowEmptyTopicName: true, ); } @@ -238,6 +241,7 @@ void main() { includeAnchor: false, numBefore: kMessageListFetchBatchSize, numAfter: 0, + allowEmptyTopicName: true, ); }); diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart index deda8078e2..7d6a5205bb 100644 --- a/test/widgets/action_sheet_test.dart +++ b/test/widgets/action_sheet_test.dart @@ -1155,6 +1155,18 @@ void main() { await setupToMessageActionSheet(tester, message: message, narrow: const StarredMessagesNarrow()); check(findQuoteAndReplyButton(tester)).isNull(); }); + + testWidgets('handle empty topic', (tester) async { + final message = eg.streamMessage(); + await setupToMessageActionSheet(tester, + message: message, narrow: TopicNarrow.ofMessage(message)); + + prepareRawContentResponseSuccess(message: message, rawContent: 'Hello world'); + await tapQuoteAndReplyButton(tester); + check(connection.lastRequest).isA() + .url.queryParameters['allow_empty_topic_name'].equals('true'); + await tester.pump(Duration.zero); + }); }); group('MarkAsUnread', () { diff --git a/test/widgets/message_list_test.dart b/test/widgets/message_list_test.dart index 816cde5948..f4de7b54ae 100644 --- a/test/widgets/message_list_test.dart +++ b/test/widgets/message_list_test.dart @@ -331,6 +331,7 @@ void main() { 'anchor': AnchorCode.newest.toJson(), 'num_before': kMessageListFetchBatchSize.toString(), 'num_after': '0', + 'allow_empty_topic_name': 'true', }); }); @@ -363,6 +364,7 @@ void main() { 'anchor': AnchorCode.newest.toJson(), 'num_before': kMessageListFetchBatchSize.toString(), 'num_after': '0', + 'allow_empty_topic_name': 'true', }); }); });