From bf7576e1c180b8d2630a7036dfe78e2291ea9a68 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 30 Dec 2024 14:41:48 -0500 Subject: [PATCH 1/7] compose [nfc]: Add a TODO(#1238) --- lib/widgets/compose_box.dart | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 83d923b6b8..ff13f9fbf3 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -264,6 +264,7 @@ class ComposeContentController extends ComposeController // 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) ContentValidationError.tooLong, From 6b5966c62af7e2c034873b7049d5ed5aac2eb77f Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 30 Dec 2024 12:44:18 -0500 Subject: [PATCH 2/7] compose [nfc]: Comment on max-topic-length check about cutting off short 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. --- lib/api/route/messages.dart | 2 +- lib/widgets/compose_box.dart | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/api/route/messages.dart b/lib/api/route/messages.dart index 3374741c51..be6728c790 100644 --- a/lib/api/route/messages.dart +++ b/lib/api/route/messages.dart @@ -156,7 +156,7 @@ class GetMessagesResult { } // https://zulip.com/api/send-message#parameter-topic -const int kMaxTopicLength = 60; +const int kMaxTopicLengthCodePoints = 60; // https://zulip.com/api/send-message#parameter-content const int kMaxMessageLengthCodePoints = 10000; diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index ff13f9fbf3..0ee11bf169 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -85,7 +85,12 @@ class ComposeTopicController extends ComposeController { return [ if (mandatory && textNormalized == kNoTopicTopic) TopicValidationError.mandatoryButEmpty, - if (textNormalized.length > kMaxTopicLength) + + // 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) TopicValidationError.tooLong, ]; } From be04a68b2862077e930f3656bcf2e074147a944f Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 30 Dec 2024 13:03:32 -0500 Subject: [PATCH 3/7] compose test [nfc]: Give enterTopic a dartdoc --- test/widgets/compose_box_test.dart | 1 + 1 file changed, 1 insertion(+) diff --git a/test/widgets/compose_box_test.dart b/test/widgets/compose_box_test.dart index cde7133d74..a0a2da59bd 100644 --- a/test/widgets/compose_box_test.dart +++ b/test/widgets/compose_box_test.dart @@ -80,6 +80,7 @@ void main() { controller = tester.state(find.byType(ComposeBox)).controller; } + /// Set the topic input's text to [topic], using [WidgetTester.enterText]. Future enterTopic(WidgetTester tester, { required ChannelNarrow narrow, required String topic, From cb1eb7de0a29ccf7a60b853a423176d384b0194a Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 22 Jan 2025 14:22:08 -0800 Subject: [PATCH 4/7] compose_box test [nfc]: Make a enterContent helper --- test/widgets/compose_box_test.dart | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/test/widgets/compose_box_test.dart b/test/widgets/compose_box_test.dart index a0a2da59bd..04099ce7a5 100644 --- a/test/widgets/compose_box_test.dart +++ b/test/widgets/compose_box_test.dart @@ -41,9 +41,6 @@ void main() { late FakeApiConnection connection; late ComposeBoxController? controller; - final contentInputFinder = find.byWidgetPredicate( - (widget) => widget is TextField && widget.controller is ComposeContentController); - Future prepareComposeBox(WidgetTester tester, { required Narrow narrow, User? selfUser, @@ -96,6 +93,17 @@ void main() { ..url.path.equals('/api/v1/users/me/${narrow.streamId}/topics'); } + /// A [Finder] for the content input. + /// + /// To enter some text, use [enterContent]. + final contentInputFinder = find.byWidgetPredicate( + (widget) => widget is TextField && widget.controller is ComposeContentController); + + /// Set the content input's text to [content], using [WidgetTester.enterText]. + Future enterContent(WidgetTester tester, String content) async { + await tester.enterText(contentInputFinder, content); + } + group('ComposeContentController', () { group('insertPadded', () { // Like `parseMarkedText` in test/model/autocomplete_test.dart, @@ -245,7 +253,7 @@ void main() { Future checkStartTyping(WidgetTester tester, SendableNarrow narrow) async { connection.prepare(json: {}); - await tester.enterText(contentInputFinder, 'hello world'); + await enterContent(tester, 'hello world'); checkTypingRequest(TypingOp.start, narrow); } @@ -290,7 +298,7 @@ void main() { await checkStartTyping(tester, narrow); connection.prepare(json: {}); - await tester.enterText(contentInputFinder, ''); + await enterContent(tester, ''); checkTypingRequest(TypingOp.stop, narrow); }); @@ -406,7 +414,7 @@ void main() { await prepareComposeBox(tester, narrow: eg.topicNarrow(123, 'some topic'), streams: [eg.stream(streamId: 123)]); - await tester.enterText(contentInputFinder, 'hello world'); + await enterContent(tester, 'hello world'); prepareResponse(456); await tester.tap(find.byTooltip(zulipLocalizations.composeBoxSendTooltip)); @@ -817,7 +825,7 @@ void main() { double? height; for (numLines = 2; numLines <= 1000; numLines++) { final content = List.generate(numLines, (_) => 'foo').join('\n'); - await tester.enterText(contentInputFinder, content); + await enterContent(tester, content); await tester.pump(); final newHeight = tester.getRect(contentInputFinder).height; if (newHeight == height) { From c0b9a5d6a6971b1ef05d9b710811251ced297ffa Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 30 Dec 2024 13:50:37 -0500 Subject: [PATCH 5/7] test [nfc]: Factor out `checkNoErrorDialog` helper With #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. --- test/widgets/app_test.dart | 4 ++-- test/widgets/compose_box_test.dart | 9 +++------ test/widgets/dialog_checks.dart | 7 +++++++ 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/test/widgets/app_test.dart b/test/widgets/app_test.dart index 8c992533ee..af386bfdf1 100644 --- a/test/widgets/app_test.dart +++ b/test/widgets/app_test.dart @@ -280,14 +280,14 @@ void main() { check(ZulipApp.ready).value.isFalse(); await tester.pump(); check(findSnackBarByText(message).evaluate()).isEmpty(); - check(find.byType(AlertDialog).evaluate()).isEmpty(); + checkNoErrorDialog(tester); check(ZulipApp.ready).value.isTrue(); // After app startup, reportErrorToUserBriefly displays a SnackBar. reportErrorToUserBriefly(message, details: details); await tester.pumpAndSettle(); check(findSnackBarByText(message).evaluate()).single; - check(find.byType(AlertDialog).evaluate()).isEmpty(); + checkNoErrorDialog(tester); // Open the error details dialog. await tester.tap(find.text('Details')); diff --git a/test/widgets/compose_box_test.dart b/test/widgets/compose_box_test.dart index 04099ce7a5..5a3a39abd2 100644 --- a/test/widgets/compose_box_test.dart +++ b/test/widgets/compose_box_test.dart @@ -436,8 +436,7 @@ void main() { await setupAndTapSend(tester, prepareResponse: (int messageId) { connection.prepare(json: SendMessageResult(id: messageId).toJson()); }); - final errorDialogs = tester.widgetList(find.byType(AlertDialog)); - check(errorDialogs).isEmpty(); + checkNoErrorDialog(tester); }); testWidgets('ZulipApiException', (tester) async { @@ -506,8 +505,7 @@ void main() { check(call.allowMultiple).equals(true); check(call.type).equals(FileType.media); - final errorDialogs = tester.widgetList(find.byType(AlertDialog)); - check(errorDialogs).isEmpty(); + checkNoErrorDialog(tester); check(controller!.content.text) .equals('see image: [Uploading image.jpg…]()\n\n'); @@ -566,8 +564,7 @@ void main() { check(call.source).equals(ImageSource.camera); check(call.requestFullMetadata).equals(false); - final errorDialogs = tester.widgetList(find.byType(AlertDialog)); - check(errorDialogs).isEmpty(); + checkNoErrorDialog(tester); check(controller!.content.text) .equals('see image: [Uploading image.jpg…]()\n\n'); diff --git a/test/widgets/dialog_checks.dart b/test/widgets/dialog_checks.dart index 504042d07e..af0c6e2963 100644 --- a/test/widgets/dialog_checks.dart +++ b/test/widgets/dialog_checks.dart @@ -1,4 +1,6 @@ +import 'package:checks/checks.dart'; import 'package:flutter/material.dart'; +import 'package:flutter_checks/flutter_checks.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:zulip/widgets/dialog.dart'; @@ -26,6 +28,11 @@ Widget checkErrorDialog(WidgetTester tester, { matching: find.widgetWithText(TextButton, 'OK'))); } +// TODO(#996) update this to check for per-platform flavors of alert dialog +void checkNoErrorDialog(WidgetTester tester) { + check(find.byType(AlertDialog)).findsNothing(); +} + /// In a widget test, check that [showSuggestedActionDialog] was called /// with the right text. /// From e67b0141d243d21c69a2232750fb19d2c6d236a0 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 30 Dec 2024 13:45:51 -0500 Subject: [PATCH 6/7] compose test: Test topic- and content-max-length validation errors --- test/widgets/compose_box_test.dart | 107 +++++++++++++++++++++++++++++ 1 file changed, 107 insertions(+) diff --git a/test/widgets/compose_box_test.dart b/test/widgets/compose_box_test.dart index 5a3a39abd2..ec27585e88 100644 --- a/test/widgets/compose_box_test.dart +++ b/test/widgets/compose_box_test.dart @@ -104,6 +104,12 @@ void main() { await tester.enterText(contentInputFinder, content); } + Future tapSendButton(WidgetTester tester) async { + connection.prepare(json: SendMessageResult(id: 123).toJson()); + await tester.tap(find.byIcon(ZulipIcons.send)); + await tester.pump(Duration.zero); + } + group('ComposeContentController', () { group('insertPadded', () { // Like `parseMarkedText` in test/model/autocomplete_test.dart, @@ -206,6 +212,107 @@ void main() { }); }); + group('length validation', () { + final channel = eg.stream(); + + /// String where there are [n] Unicode code points, + /// >[n] UTF-16 code units, and <[n] "characters" a.k.a. grapheme clusters. + String makeStringWithCodePoints(int n) { + assert(n >= 5); + const graphemeCluster = '👨‍👩‍👦'; + assert(graphemeCluster.runes.length == 5); + assert(graphemeCluster.length == 8); + assert(graphemeCluster.characters.length == 1); + + final result = + graphemeCluster * (n ~/ 5) + + 'a' * (n % 5); + assert(result.runes.length == n); + + return result; + } + + group('content', () { + Future prepareWithContent(WidgetTester tester, String content) 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, content); + } + + Future checkErrorResponse(WidgetTester tester) async { + await tester.tap(find.byWidget(checkErrorDialog(tester, + expectedTitle: 'Message not sent', + expectedMessage: 'Message length shouldn\'t be greater than 10000 characters.'))); + } + + testWidgets('too-long content is rejected', (tester) async { + await prepareWithContent(tester, + makeStringWithCodePoints(kMaxMessageLengthCodePoints + 1)); + await tapSendButton(tester); + await checkErrorResponse(tester); + }); + + // TODO(#1238) unskip + // testWidgets('max-length content not rejected', (tester) async { + // await prepareWithContent(tester, + // makeStringWithCodePoints(kMaxMessageLengthCodePoints)); + // await tapSendButton(tester); + // checkNoErrorDialog(tester); + // }); + + // TODO(#1238) replace with above commented-out test + testWidgets('some content not rejected', (tester) async { + await prepareWithContent(tester, 'a' * kMaxMessageLengthCodePoints); + await tapSendButton(tester); + checkNoErrorDialog(tester); + }); + }); + + group('topic', () { + Future prepareWithTopic(WidgetTester tester, String topic) 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: topic); + await enterContent(tester, 'some content'); + } + + Future checkErrorResponse(WidgetTester tester) async { + await tester.tap(find.byWidget(checkErrorDialog(tester, + expectedTitle: 'Message not sent', + expectedMessage: 'Topic length shouldn\'t be greater than 60 characters.'))); + } + + testWidgets('too-long topic is rejected', (tester) async { + await prepareWithTopic(tester, + makeStringWithCodePoints(kMaxTopicLengthCodePoints + 1)); + await tapSendButton(tester); + await checkErrorResponse(tester); + }); + + // TODO(#1238) unskip + // testWidgets('max-length topic not rejected', (tester) async { + // await prepareWithTopic(tester, + // makeStringWithCodePoints(kMaxTopicLengthCodePoints)); + // await tapSendButton(tester); + // checkNoErrorDialog(tester); + // }); + + // TODO(#1238) replace with above commented-out test + testWidgets('some topic not rejected', (tester) async { + await prepareWithTopic(tester, 'a' * kMaxTopicLengthCodePoints); + await tapSendButton(tester); + checkNoErrorDialog(tester); + }); + }); + }); + group('ComposeBox textCapitalization', () { void checkComposeBoxTextFields(WidgetTester tester, { required bool expectTopicTextField, From a23309a44ca992db2db363adc9f1a76c74d92cd0 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 30 Dec 2024 12:40:11 -0500 Subject: [PATCH 7/7] compose: Enforce max topic/content length by code points, following API Fixes #1238 --- lib/widgets/compose_box.dart | 47 +++++++++++++++++++++++------- test/widgets/compose_box_test.dart | 39 +++++++++++-------------- 2 files changed, 54 insertions(+), 32 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 0ee11bf169..d279c404ad 100644 --- 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 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 get validationErrors => _validationErrors; late List _validationErrors; List _computeValidationErrors(); @@ -40,6 +61,8 @@ abstract class ComposeController 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 { // 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 { 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 _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 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 index ec27585e88..96bc6cb837 100644 --- a/test/widgets/compose_box_test.dart +++ b/test/widgets/compose_box_test.dart @@ -256,20 +256,17 @@ void main() { await checkErrorResponse(tester); }); - // TODO(#1238) unskip - // testWidgets('max-length content not rejected', (tester) async { - // await prepareWithContent(tester, - // makeStringWithCodePoints(kMaxMessageLengthCodePoints)); - // await tapSendButton(tester); - // checkNoErrorDialog(tester); - // }); - - // TODO(#1238) replace with above commented-out test - testWidgets('some content not rejected', (tester) async { - await prepareWithContent(tester, 'a' * kMaxMessageLengthCodePoints); + testWidgets('max-length content not rejected', (tester) async { + await prepareWithContent(tester, + makeStringWithCodePoints(kMaxMessageLengthCodePoints)); await tapSendButton(tester); checkNoErrorDialog(tester); }); + + testWidgets('code points not counted unnecessarily', (tester) async { + await prepareWithContent(tester, 'a' * kMaxMessageLengthCodePoints); + check(controller!.content.debugLengthUnicodeCodePointsIfLong).isNull(); + }); }); group('topic', () { @@ -296,20 +293,18 @@ void main() { await checkErrorResponse(tester); }); - // TODO(#1238) unskip - // testWidgets('max-length topic not rejected', (tester) async { - // await prepareWithTopic(tester, - // makeStringWithCodePoints(kMaxTopicLengthCodePoints)); - // await tapSendButton(tester); - // checkNoErrorDialog(tester); - // }); - - // TODO(#1238) replace with above commented-out test - testWidgets('some topic not rejected', (tester) async { - await prepareWithTopic(tester, 'a' * kMaxTopicLengthCodePoints); + testWidgets('max-length topic not rejected', (tester) async { + await prepareWithTopic(tester, + makeStringWithCodePoints(kMaxTopicLengthCodePoints)); await tapSendButton(tester); checkNoErrorDialog(tester); }); + + testWidgets('code points not counted unnecessarily', (tester) async { + await prepareWithTopic(tester, 'a' * kMaxTopicLengthCodePoints); + check((controller as StreamComposeBoxController) + .topic.debugLengthUnicodeCodePointsIfLong).isNull(); + }); }); });