compose: Enforce max topic/content length by code points, following API

Fixes zulip#1238
chrisbobbe · Jan 17, 2025 · fe080cb · fe080cb
1 parent 2ce23fe
commit fe080cb
Show file tree

Hide file tree

Showing 2 changed files with 64 additions and 22 deletions.
diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart
@@ -28,10 +28,31 @@ const double _composeButtonSize = 44;
 ///
 /// Subclasses must ensure that [_update] is called in all exposed constructors.
 abstract class ComposeController<ErrorT> extends TextEditingController {
+  int get maxLengthUnicodeCodePoints;
+
   String get textNormalized => _textNormalized;
   late String _textNormalized;
   String _computeTextNormalized();
 
+  /// Length of [textNormalized] in Unicode code points
+  /// if it might exceed [maxLengthUnicodeCodePoints], else null.
+  ///
+  /// Use this instead of [String.length]
+  /// to enforce a max length expressed in code points.
+  /// [String.length] is conservative and may cut the user off too short.
+  ///
+  /// Counting code points ([String.runes])
+  /// is more expensive than getting the number of UTF-16 code units
+  /// ([String.length]), so we avoid it when the result definitely won't exceed
+  /// [maxLengthUnicodeCodePoints].
+  late int? _lengthUnicodeCodePointsIfLong;
+  @visibleForTesting
+  int? get debugLengthUnicodeCodePointsIfLong => _lengthUnicodeCodePointsIfLong;
+  int? _computeLengthUnicodeCodePointsIfLong() =>
+    _textNormalized.length > maxLengthUnicodeCodePoints
+      ? _textNormalized.runes.length
+      : null;
+
   List<ErrorT> get validationErrors => _validationErrors;
   late List<ErrorT> _validationErrors;
   List<ErrorT> _computeValidationErrors();
@@ -40,6 +61,8 @@ abstract class ComposeController<ErrorT> extends TextEditingController {
 
   void _update() {
     _textNormalized = _computeTextNormalized();
+    // uses _textNormalized, so comes after _computeTextNormalized()
+    _lengthUnicodeCodePointsIfLong = _computeLengthUnicodeCodePointsIfLong();
     _validationErrors = _computeValidationErrors();
     hasValidationErrors.value = _validationErrors.isNotEmpty;
   }
@@ -74,6 +97,9 @@ class ComposeTopicController extends ComposeController<TopicValidationError> {
   //   https://zulip.com/help/require-topics
   final mandatory = true;
 
+  // TODO(#307) use `max_topic_length` instead of hardcoded limit
+  @override final maxLengthUnicodeCodePoints = kMaxTopicLengthCodePoints;
+
   @override
   String _computeTextNormalized() {
     String trimmed = text.trim();
@@ -86,11 +112,10 @@ class ComposeTopicController extends ComposeController<TopicValidationError> {
       if (mandatory && textNormalized == kNoTopicTopic)
         TopicValidationError.mandatoryButEmpty,
 
-      // textNormalized.length is the number of UTF-16 code units, while the server
-      // API expresses the max in Unicode code points. So this comparison will
-      // be conservative and may cut the user off shorter than necessary.
-      // TODO(#1238) stop cutting off shorter than necessary
-      if (textNormalized.length > kMaxTopicLengthCodePoints)
+      if (
+        _lengthUnicodeCodePointsIfLong != null
+        && _lengthUnicodeCodePointsIfLong! > maxLengthUnicodeCodePoints
+      )
         TopicValidationError.tooLong,
     ];
   }
@@ -125,6 +150,9 @@ class ComposeContentController extends ComposeController<ContentValidationError>
     _update();
   }
 
+  // TODO(#1237) use `max_message_length` instead of hardcoded limit
+  @override final maxLengthUnicodeCodePoints = kMaxMessageLengthCodePoints;
+
   int _nextQuoteAndReplyTag = 0;
   int _nextUploadTag = 0;
 
@@ -266,11 +294,10 @@ class ComposeContentController extends ComposeController<ContentValidationError>
       if (textNormalized.isEmpty)
         ContentValidationError.empty,
 
-      // normalized.length is the number of UTF-16 code units, while the server
-      // API expresses the max in Unicode code points. So this comparison will
-      // be conservative and may cut the user off shorter than necessary.
-      // TODO(#1238) stop cutting off shorter than necessary
-      if (textNormalized.length > kMaxMessageLengthCodePoints)
+      if (
+        _lengthUnicodeCodePointsIfLong != null
+        && _lengthUnicodeCodePointsIfLong! > maxLengthUnicodeCodePoints
+      )
         ContentValidationError.tooLong,
 
       if (_quoteAndReplies.isNotEmpty)

diff --git a/test/widgets/compose_box_test.dart b/test/widgets/compose_box_test.dart
@@ -260,13 +260,20 @@ void main() {
       doTest('too-long content is rejected',
         content: makeStringWithCodePoints(kMaxMessageLengthCodePoints + 1), expectError: true);
 
-      // TODO(#1238) unskip
-      // doTest('max-length content not rejected',
-      //   content: makeStringWithCodePoints(kMaxMessageLengthCodePoints), expectError: false);
+      doTest('max-length content not rejected',
+        content: makeStringWithCodePoints(kMaxMessageLengthCodePoints), expectError: false);
 
-      // TODO(#1238) replace with above commented-out test
-      doTest('some content not rejected',
-        content: 'a' * kMaxMessageLengthCodePoints, expectError: false);
+      testWidgets('code points not counted unnecessarily', (tester) async {
+        TypingNotifier.debugEnable = false;
+        addTearDown(TypingNotifier.debugReset);
+
+        final narrow = ChannelNarrow(channel.streamId);
+        await prepareComposeBox(tester, narrow: narrow, streams: [channel]);
+        await enterTopic(tester, narrow: narrow, topic: 'some topic');
+        await enterContent(tester, narrow: narrow, content: 'a' * kMaxMessageLengthCodePoints);
+
+        check(controller!.content.debugLengthUnicodeCodePointsIfLong).isNull();
+      });
     });
 
     group('topic', () {
@@ -294,13 +301,21 @@ void main() {
       doTest('too-long topic is rejected',
         topic: makeStringWithCodePoints(kMaxTopicLengthCodePoints + 1), expectError: true);
 
-      // TODO(#1238) unskip
-      // doTest('max-length topic not rejected',
-      //   topic: makeStringWithCodePoints(kMaxTopicLengthCodePoints), expectError: false);
+      doTest('max-length topic not rejected',
+        topic: makeStringWithCodePoints(kMaxTopicLengthCodePoints), expectError: false);
 
-      // TODO(#1238) replace with above commented-out test
-      doTest('some topic not rejected',
-        topic: 'a' * kMaxTopicLengthCodePoints, expectError: false);
+      testWidgets('code points not counted unnecessarily', (tester) async {
+        TypingNotifier.debugEnable = false;
+        addTearDown(TypingNotifier.debugReset);
+
+        final narrow = ChannelNarrow(channel.streamId);
+        await prepareComposeBox(tester, narrow: narrow, streams: [channel]);
+        await enterTopic(tester, narrow: narrow, topic: 'a' * kMaxTopicLengthCodePoints);
+        await enterContent(tester, narrow: narrow, content: 'some content');
+
+        check((controller as StreamComposeBoxController)
+          .topic.debugLengthUnicodeCodePointsIfLong).isNull();
+      });
     });
   });