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

chrisbobbe · 2024-12-30T20:04:31Z

Done by computing String.runes (the number of Unicode code points) unless we know that the result can't exceed the threshold number of code points. In particular, we don't compute it unless String.length (the number of UTF-16 code units) exceeds the threshold number of code points.

PIG208

Thanks for the PR! I read all the commits and it looks good to me overall. Left some comments.

PIG208 · 2025-01-03T06:55:23Z

test/widgets/compose_box_test.dart

@@ -95,6 +95,23 @@ void main() {
      ..url.path.equals('/api/v1/users/me/${narrow.streamId}/topics');
  }

+  /// Set the content input's text to [content], using [WidgetTester.enterText].
+  Future<void> enterContent(WidgetTester tester, {
+    required ChannelNarrow narrow,


Looks like this is unused. For entering content, just having await tester.enterText(contentInputFinder, content); should be fine.

It gets two callers in this commit, and two more in a later commit.

At the branch tip:

$ git grep enterContent -- test/widgets/compose_box_test.dart test/widgets/compose_box_test.dart: Future<void> enterContent(WidgetTester tester, { test/widgets/compose_box_test.dart: await enterContent(tester, narrow: narrow, content: content); test/widgets/compose_box_test.dart: await enterContent(tester, narrow: narrow, content: 'a' * kMaxMessageLengthCodePoints); test/widgets/compose_box_test.dart: await enterContent(tester, narrow: narrow, content: 'some content'); test/widgets/compose_box_test.dart: await enterContent(tester, narrow: narrow, content: 'some content');

I like the clear parallelism when it's used alongside enterTopic:

await enterTopic(tester, narrow: narrow, topic: 'some topic'); await enterContent(tester, narrow: narrow, content: content);

PIG208 · 2025-01-03T11:46:40Z

lib/widgets/compose_box.dart

+  /// 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].
+  int? get lengthUnicodeCodePointsIfLong => _lengthUnicodeCodePointsIfLong;


What if we maintain the length internally, and expose only a method that tests if the content is too long? We already have access to the length limit via maxLengthUnicodeCodePoints and hiding this cache might simplify the checks a bit.

Sure, makes sense; I can remove this public getter.

and expose only a method that tests if the content is too long

How about keeping the validationErrors getter as the way consumers get this information, without adding a new method?

chrisbobbe · 2025-01-17T22:40:14Z

Thanks for the review! Revision pushed, this time atop #1290 for the sake of CI.

PIG208 · 2025-01-17T23:09:51Z

Thanks! This revision looks good to me.

chrisbobbe · 2025-01-17T23:18:08Z

Thanks for the review!

gnprice

Thanks for fixing this! The implementation all looks good to me; small comments on the tests.

gnprice · 2025-01-22T06:55:26Z

test/widgets/compose_box_test.dart

@@ -95,6 +96,23 @@ void main() {
      ..url.path.equals('/api/v1/users/me/${narrow.streamId}/topics');
  }

+  /// Set the content input's text to [content], using [WidgetTester.enterText].
+  Future<void> enterContent(WidgetTester tester, {
+    required ChannelNarrow narrow,


This parameter doesn't seem to be used.

(Maybe that's what @PIG208 meant at https://github.com/zulip/zulip-flutter/pull/1239/files#r1901513335 ?)

Oh right. I thought I was referring to the helper as a whole when re-reviewing, but initially the comment was brought up because of this parameter.

gnprice · 2025-01-22T06:56:51Z

test/widgets/compose_box_test.dart

+    final contentInputFinder = find.byWidgetPredicate(
+      (widget) => widget is TextField && widget.controller is ComposeContentController);


Suggested change

final contentInputFinder = find.byWidgetPredicate(

(widget) => widget is TextField && widget.controller is ComposeContentController);

This is equivalent to the finder already defined above, right?

gnprice · 2025-01-22T07:03:27Z

test/widgets/compose_box_test.dart

+      doTest('too-long content is rejected',
+        content: makeStringWithCodePoints(kMaxMessageLengthCodePoints + 1), expectError: true);


nit: since this helper creates a test case, find a name for it starting with "test", like test and testWidgets

(But also this is probably clearer if the logic these tests have in common goes into a helper function they each call, rather than having them wrapped like this.)

gnprice · 2025-01-22T07:06:23Z

test/widgets/compose_box_test.dart

+        content: makeStringWithCodePoints(kMaxMessageLengthCodePoints + 1), expectError: true);
+
+      doTest('max-length content not rejected',
+        content: makeStringWithCodePoints(kMaxMessageLengthCodePoints), expectError: false);


nit: lines too long: the key information at expectError: is past 80 columns

gnprice · 2025-01-22T07:11:07Z

test/widgets/compose_box_test.dart

+      const graphemeCluster = '👨‍👩‍👦';
+      assert(graphemeCluster.runes.length == 5);
+      assert(graphemeCluster.length == 8);
+      assert(graphemeCluster.characters.length == 1);


The fact about .characters isn't really used here, right? This could be any single emoji code point (for any of the emoji of the past decade or so, in the range U+10000 and up) and it'd have more UTF-16 code units than code points.

I guess it is helpful that the tests below confirm that at M+1 code points we start rejecting the value even when it's less than M "characters" / grapheme clusters. So maybe the thing to do is to just add that to this function's doc: the number of characters in that sense is less than [n].

chrisbobbe · 2025-01-22T23:03:22Z

Thanks for the review! Revision pushed.

We have the same comment where we check the content length: if (textNormalized.length > kMaxMessageLengthCodePoints) and it applies here, too; the API doc on max_topic_length in /register also says it's in Unicode code points: https://zulip.com/api/register-queue And give our max-topic-length variable a more descriptive name, again like we do with kMaxMessageLengthCodePoints.

With zulip#996, these tests will have to start checking for separate per-platform flavors of alert dialog. Best if they all do so through this one codepath.

Fixes zulip#1238

gnprice · 2025-01-23T05:55:12Z

Thanks! All looks good; merging.

chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Dec 30, 2024

chrisbobbe requested a review from PIG208 December 30, 2024 20:04

chrisbobbe assigned PIG208 Dec 30, 2024

ARYPROGRAMMER mentioned this pull request Dec 31, 2024

feat: prevent typing over-long topic names #1230

Open

chrisbobbe mentioned this pull request Jan 2, 2025

store: Handle invalid API key on register-queue #1183

Open

PIG208 reviewed Jan 3, 2025

View reviewed changes

chrisbobbe force-pushed the pr-topic-content-length-code-points branch from e18196d to fe080cb Compare January 17, 2025 22:40

PIG208 assigned gnprice and unassigned PIG208 Jan 17, 2025

PIG208 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 Jan 17, 2025

chrisbobbe requested a review from gnprice January 17, 2025 23:18

gnprice reviewed Jan 22, 2025

View reviewed changes

chrisbobbe force-pushed the pr-topic-content-length-code-points branch from fe080cb to c2e7a38 Compare January 22, 2025 23:03

chrisbobbe added 7 commits January 22, 2025 21:52

compose [nfc]: Add a TODO(zulip#1238)

bf7576e

compose test [nfc]: Give enterTopic a dartdoc

be04a68

compose_box test [nfc]: Make a enterContent helper

cb1eb7d

test [nfc]: Factor out checkNoErrorDialog helper

c0b9a5d

With zulip#996, these tests will have to start checking for separate per-platform flavors of alert dialog. Best if they all do so through this one codepath.

compose test: Test topic- and content-max-length validation errors

e67b014

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

a23309a

Fixes zulip#1238

gnprice force-pushed the pr-topic-content-length-code-points branch from c2e7a38 to a23309a Compare January 23, 2025 05:55

gnprice merged commit a23309a into zulip:main Jan 23, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

chrisbobbe commented Dec 30, 2024

PIG208 left a comment

PIG208 Jan 3, 2025

chrisbobbe Jan 17, 2025

chrisbobbe Jan 17, 2025

PIG208 Jan 3, 2025

chrisbobbe Jan 17, 2025

chrisbobbe commented Jan 17, 2025 •

edited

Loading

PIG208 commented Jan 17, 2025

chrisbobbe commented Jan 17, 2025

gnprice left a comment

gnprice Jan 22, 2025

PIG208 Jan 22, 2025

gnprice Jan 22, 2025

gnprice Jan 22, 2025

gnprice Jan 22, 2025

gnprice Jan 22, 2025

gnprice Jan 22, 2025

chrisbobbe commented Jan 22, 2025

gnprice commented Jan 23, 2025

		final contentInputFinder = find.byWidgetPredicate(
		(widget) => widget is TextField && widget.controller is ComposeContentController);

		doTest('too-long content is rejected',
		content: makeStringWithCodePoints(kMaxMessageLengthCodePoints + 1), expectError: true);

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

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

Conversation

chrisbobbe commented Dec 30, 2024

PIG208 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

chrisbobbe commented Jan 17, 2025 • edited Loading

PIG208 commented Jan 17, 2025

chrisbobbe commented Jan 17, 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

Choose a reason for hiding this comment

chrisbobbe commented Jan 22, 2025

gnprice commented Jan 23, 2025

chrisbobbe commented Jan 17, 2025 •

edited

Loading