realm [nfc]: Move processTopicLikeServer to substore, from API code

This way, the method's implementation can look up zulipFeatureLevel
and realmEmptyTopicDisplayName for itself, rather than have them
looked up in advance by the caller and passed in.

Then as a further consequence, the method only looks up
realmEmptyTopicDisplayName if and when it determines it actually needs
that information.  That makes the lookup compatible with the check we
have in the main realmEmptyTopicDisplayName getter to assert that it's
only consulted when it should in fact be needed, on old servers.
A hack in the message store, and a similar hack on the message list,
therefore become unnecessary.

Author: gnprice

lib/api/model/model.dart

@@ -837,53 +837,6 @@ extension type const TopicName(String _value) {
   /// using [canonicalize].
   bool isSameAs(TopicName other) => canonicalize() == other.canonicalize();
 
-  /// Process this topic to match how it would appear on a message object from
-  /// the server.
-  ///
-  /// This returns the [TopicName] the server would be predicted to include
-  /// in a message object resulting from sending to this [TopicName]
-  /// in a [sendMessage] request.
-  ///
-  /// This [TopicName] is required to have no leading or trailing whitespace.
-  ///
-  /// For a client that supports empty topics, when FL>=334, the server converts
-  /// `store.realmEmptyTopicDisplayName` to an empty string; when FL>=370,
-  /// the server converts "(no topic)" to an empty string as well.
-  ///
-  /// See API docs:
-  ///   https://zulip.com/api/send-message#parameter-topic
-  TopicName processLikeServer({
-    required int zulipFeatureLevel,
-    required String? realmEmptyTopicDisplayName,
-  }) {
-    assert(_value.trim() == _value);
-    // TODO(server-10) simplify this away
-    if (zulipFeatureLevel < 334) {
-      // From the API docs:
-      // > Before Zulip 10.0 (feature level 334), empty string was not a valid
-      // > topic name for channel messages.
-      assert(_value.isNotEmpty);
-      return this;
-    }
-
-    // TODO(server-10) simplify this away
-    if (zulipFeatureLevel < 370 && _value == kNoTopicTopic) {
-      // From the API docs:
-      // > Before Zulip 10.0 (feature level 370), "(no topic)" was not
-      // > interpreted as an empty string.
-      return TopicName(kNoTopicTopic);
-    }
-
-    if (_value == kNoTopicTopic || _value == realmEmptyTopicDisplayName) {
-      // From the API docs:
-      // > When "(no topic)" or the value of realm_empty_topic_display_name
-      // > found in the POST /register response is used for [topic],
-      // > it is interpreted as an empty string.
-      return TopicName('');
-    }
-    return TopicName(_value);
-  }
-
   TopicName.fromJson(this._value);
 
   String toJson() => apiName;

lib/model/message.dart

@@ -12,6 +12,7 @@ import '../api/route/messages.dart';
 import '../log.dart';
 import 'binding.dart';
 import 'message_list.dart';
+import 'realm.dart';
 import 'store.dart';
 
 const _apiSendMessage = sendMessage; // Bit ugly; for alternatives, see: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20PerAccountStore.20methods/near/1545809
@@ -102,20 +103,12 @@ class _EditMessageRequestStatus {
   final String newContent;
 }
 
-class MessageStoreImpl extends PerAccountStoreBase with MessageStore, _OutboxMessageStore {
-  MessageStoreImpl({required super.core, required String? realmEmptyTopicDisplayName})
-    : _realmEmptyTopicDisplayName = realmEmptyTopicDisplayName,
-      // There are no messages in InitialSnapshot, so we don't have
+class MessageStoreImpl extends HasRealmStore with MessageStore, _OutboxMessageStore {
+  MessageStoreImpl({required super.realm})
+    : // There are no messages in InitialSnapshot, so we don't have
       // a use case for initializing MessageStore with nonempty [messages].
       messages = {};
 
-  // This copy of the realm setting is here to bypass the feature-level check
-  // on the usual getter for it.  See discussion:
-  //   https://github.com/zulip/zulip-flutter/pull/1472#discussion_r2099069276
-  // TODO move [TopicName.processLikeServer] to a substore, eliminating this
-  @override
-  final String? _realmEmptyTopicDisplayName; // TODO(#668): update this realm setting
-
   @override
   final Map<int, Message> messages;
 
@@ -771,9 +764,7 @@ class DmOutboxMessage extends OutboxMessage<DmConversation> {
 }
 
 /// Manages the outbox messages portion of [MessageStore].
-mixin _OutboxMessageStore on PerAccountStoreBase {
-  String? get _realmEmptyTopicDisplayName;
-
+mixin _OutboxMessageStore on HasRealmStore {
   late final UnmodifiableMapView<int, OutboxMessage> outboxMessages =
     UnmodifiableMapView(_outboxMessages);
   final Map<int, OutboxMessage> _outboxMessages = {};
@@ -918,22 +909,21 @@ mixin _OutboxMessageStore on PerAccountStoreBase {
   }
 
   TopicName _processTopicLikeServer(TopicName topic) {
-    return topic.processLikeServer(
-      // Processing this just once on creating the outbox message
-      // allows an uncommon bug, because either of these values can change.
-      // During the outbox message's life, a topic processed from
-      // "(no topic)" could become stale/wrong when zulipFeatureLevel
-      // changes; a topic processed from "general chat" could become
-      // stale/wrong when realmEmptyTopicDisplayName changes.
-      //
-      // Shrug. The same effect is caused by an unavoidable race:
-      // an admin could change the name of "general chat"
-      // (i.e. the value of realmEmptyTopicDisplayName)
-      // concurrently with the user making the send request,
-      // so that the setting in effect by the time the request arrives
-      // is different from the setting the client last heard about.
-      zulipFeatureLevel: zulipFeatureLevel,
-      realmEmptyTopicDisplayName: _realmEmptyTopicDisplayName);
+    // Processing this just once on creating the outbox message
+    // allows an uncommon bug, because either of the values
+    // [zulipFeatureLevel] or [realmEmptyTopicDisplayName] can change.
+    // During the outbox message's life, a topic processed from
+    // "(no topic)" could become stale/wrong when zulipFeatureLevel
+    // changes; a topic processed from "general chat" could become
+    // stale/wrong when realmEmptyTopicDisplayName changes.
+    //
+    // Shrug. The same effect is caused by an unavoidable race:
+    // an admin could change the name of "general chat"
+    // (i.e. the value of realmEmptyTopicDisplayName)
+    // concurrently with the user making the send request,
+    // so that the setting in effect by the time the request arrives
+    // is different from the setting the client last heard about.
+    return processTopicLikeServer(topic);
   }
 
   void _handleOutboxDebounce(int localMessageId) {

lib/model/realm.dart

@@ -35,6 +35,51 @@ mixin RealmStore on PerAccountStoreBase {
   List<CustomProfileField> get customProfileFields;
   /// For docs, please see [InitialSnapshot.emailAddressVisibility].
   EmailAddressVisibility? get emailAddressVisibility;
+
+  /// Process the given topic to match how it would appear
+  /// on a message object from the server.
+  ///
+  /// This returns the [TopicName] the server would be predicted to include
+  /// in a message object resulting from sending to the given [TopicName]
+  /// in a [sendMessage] request.
+  ///
+  /// The [TopicName] is required to have no leading or trailing whitespace.
+  ///
+  /// For a client that supports empty topics, when FL>=334, the server converts
+  /// `store.realmEmptyTopicDisplayName` to an empty string; when FL>=370,
+  /// the server converts "(no topic)" to an empty string as well.
+  ///
+  /// See API docs:
+  ///   https://zulip.com/api/send-message#parameter-topic
+  TopicName processTopicLikeServer(TopicName topic) {
+    final apiName = topic.apiName;
+    assert(apiName.trim() == apiName);
+    // TODO(server-10) simplify this away
+    if (zulipFeatureLevel < 334) {
+      // From the API docs:
+      // > Before Zulip 10.0 (feature level 334), empty string was not a valid
+      // > topic name for channel messages.
+      assert(apiName.isNotEmpty);
+      return topic;
+    }
+
+    // TODO(server-10) simplify this away
+    if (zulipFeatureLevel < 370 && apiName == kNoTopicTopic) {
+      // From the API docs:
+      // > Before Zulip 10.0 (feature level 370), "(no topic)" was not
+      // > interpreted as an empty string.
+      return TopicName(kNoTopicTopic);
+    }
+
+    if (apiName == kNoTopicTopic || apiName == realmEmptyTopicDisplayName) {
+      // From the API docs:
+      // > When "(no topic)" or the value of realm_empty_topic_display_name
+      // > found in the POST /register response is used for [topic],
+      // > it is interpreted as an empty string.
+      return TopicName('');
+    }
+    return topic;
+  }
 }
 
 mixin ProxyRealmStore on RealmStore {

lib/model/store.dart

@@ -483,11 +483,12 @@ class PerAccountStore extends PerAccountStoreBase with
       selfUserId: account.userId,
     );
     final channels = ChannelStoreImpl(initialSnapshot: initialSnapshot);
+    final realm = RealmStoreImpl(core: core, initialSnapshot: initialSnapshot);
     return PerAccountStore._(
       core: core,
       groups: UserGroupStoreImpl(core: core,
         groups: initialSnapshot.realmUserGroups),
-      realm: RealmStoreImpl(core: core, initialSnapshot: initialSnapshot),
+      realm: realm,
       emoji: EmojiStoreImpl(
         core: core, allRealmEmoji: initialSnapshot.realmEmoji),
       userSettings: initialSnapshot.userSettings,
@@ -509,8 +510,7 @@ class PerAccountStore extends PerAccountStoreBase with
         realmPresenceDisabled: initialSnapshot.realmPresenceDisabled,
         initial: initialSnapshot.presences),
       channels: channels,
-      messages: MessageStoreImpl(core: core,
-        realmEmptyTopicDisplayName: initialSnapshot.realmEmptyTopicDisplayName),
+      messages: MessageStoreImpl(realm: realm),
       unreads: Unreads(
         initial: initialSnapshot.unreadMsgs,
         core: core,

lib/widgets/message_list.dart

@@ -778,11 +778,7 @@ class _MessageListState extends State<MessageList> with PerAccountStoreAwareStat
     if (narrow is TopicNarrow) {
       // Normalize topic name.  See #1717.
       narrow = TopicNarrow(narrow.streamId,
-        narrow.topic.processLikeServer(
-          zulipFeatureLevel: store.zulipFeatureLevel,
-          realmEmptyTopicDisplayName: store.zulipFeatureLevel > 334
-            ? store.realmEmptyTopicDisplayName
-            : null),
+        store.processTopicLikeServer(narrow.topic),
         with_: narrow.with_);
       if (narrow != widget.narrow) {
         SchedulerBinding.instance.scheduleFrameCallback((_) {

test/api/model/model_test.dart

@@ -227,31 +227,6 @@ void main() {
 
       doCheck(eg.t('✔ a'),          eg.t('✔ b'),          false);
     });
-
-    test('processLikeServer', () {
-      final emptyTopicDisplayName = eg.defaultRealmEmptyTopicDisplayName;
-      void doCheck(TopicName topic, TopicName expected, int zulipFeatureLevel) {
-        check(topic.processLikeServer(
-          zulipFeatureLevel: zulipFeatureLevel,
-          realmEmptyTopicDisplayName: emptyTopicDisplayName),
-        ).equals(expected);
-      }
-
-      check(() => eg.t('').processLikeServer(
-        zulipFeatureLevel: 333,
-        realmEmptyTopicDisplayName: emptyTopicDisplayName),
-      ).throws<void>();
-      doCheck(eg.t('(no topic)'),          eg.t('(no topic)'),          333);
-      doCheck(eg.t(emptyTopicDisplayName), eg.t(emptyTopicDisplayName), 333);
-      doCheck(eg.t('other topic'),         eg.t('other topic'),         333);
-
-      doCheck(eg.t(''),                    eg.t(''),                    334);
-      doCheck(eg.t('(no topic)'),          eg.t('(no topic)'),          334);
-      doCheck(eg.t(emptyTopicDisplayName), eg.t(''),                    334);
-      doCheck(eg.t('other topic'),         eg.t('other topic'),         334);
-
-      doCheck(eg.t('(no topic)'),          eg.t(''),                    370);
-    });
   });
 
   group('DmMessage', () {

test/model/realm_test.dart

@@ -47,4 +47,32 @@ void main() {
       check(store.customProfileFields.map((f) => f.id)).deepEquals([2, 0, 1]);
     });
   });
+
+  test('processTopicLikeServer', () {
+    final emptyTopicDisplayName = eg.defaultRealmEmptyTopicDisplayName;
+
+    TopicName process(TopicName topic, int zulipFeatureLevel) {
+      final account = eg.selfAccount.copyWith(zulipFeatureLevel: zulipFeatureLevel);
+      final store = eg.store(account: account, initialSnapshot: eg.initialSnapshot(
+        zulipFeatureLevel: zulipFeatureLevel,
+        realmEmptyTopicDisplayName: emptyTopicDisplayName));
+      return store.processTopicLikeServer(topic);
+    }
+
+    void doCheck(TopicName topic, TopicName expected, int zulipFeatureLevel) {
+      check(process(topic, zulipFeatureLevel)).equals(expected);
+    }
+
+    check(() => process(eg.t(''), 333)).throws<void>();
+    doCheck(eg.t('(no topic)'),          eg.t('(no topic)'),          333);
+    doCheck(eg.t(emptyTopicDisplayName), eg.t(emptyTopicDisplayName), 333);
+    doCheck(eg.t('other topic'),         eg.t('other topic'),         333);
+
+    doCheck(eg.t(''),                    eg.t(''),                    334);
+    doCheck(eg.t('(no topic)'),          eg.t('(no topic)'),          334);
+    doCheck(eg.t(emptyTopicDisplayName), eg.t(''),                    334);
+    doCheck(eg.t('other topic'),         eg.t('other topic'),         334);
+
+    doCheck(eg.t('(no topic)'),          eg.t(''),                    370);
+  });
 }