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 83d923b6b8..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(); @@ -85,7 +111,11 @@ class ComposeTopicController extends ComposeController { return [ if (mandatory && textNormalized == kNoTopicTopic) TopicValidationError.mandatoryButEmpty, - if (textNormalized.length > kMaxTopicLength) + + if ( + _lengthUnicodeCodePointsIfLong != null + && _lengthUnicodeCodePointsIfLong! > maxLengthUnicodeCodePoints + ) TopicValidationError.tooLong, ]; } @@ -120,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; @@ -261,10 +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. - if (textNormalized.length > kMaxMessageLengthCodePoints) + if ( + _lengthUnicodeCodePointsIfLong != null + && _lengthUnicodeCodePointsIfLong! > maxLengthUnicodeCodePoints + ) ContentValidationError.tooLong, if (_quoteAndReplies.isNotEmpty) 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 cde7133d74..96bc6cb837 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, @@ -80,6 +77,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, @@ -95,6 +93,23 @@ 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); + } + + 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, @@ -197,6 +212,102 @@ 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); + }); + + 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', () { + 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); + }); + + 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(); + }); + }); + }); + group('ComposeBox textCapitalization', () { void checkComposeBoxTextFields(WidgetTester tester, { required bool expectTopicTextField, @@ -244,7 +355,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); } @@ -289,7 +400,7 @@ void main() { await checkStartTyping(tester, narrow); connection.prepare(json: {}); - await tester.enterText(contentInputFinder, ''); + await enterContent(tester, ''); checkTypingRequest(TypingOp.stop, narrow); }); @@ -405,7 +516,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)); @@ -427,8 +538,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 { @@ -497,8 +607,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'); @@ -557,8 +666,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'); @@ -816,7 +924,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) { 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. ///