From a316cdb1237dbc654c7f0684dbdfe87eeb2ffd96 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 22 Jun 2023 12:47:34 -0700 Subject: [PATCH 01/19] compose: On failed upload, remove compose placeholder instead of replacing We're already showing an error dialog for the failure, so this seems redundant; discussion: https://github.com/zulip/zulip-flutter/pull/201#discussion_r1237926811 --- lib/widgets/compose_box.dart | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index a1c2584bb3..4bc213dd56 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -157,9 +157,7 @@ class ComposeContentController extends ComposeController value = value.replaced( replacementRange, - url == null - ? '[Failed to upload file: $filename]()' // TODO(i18n) - : '[$filename](${url.toString()})'); + url == null ? '' : '[$filename](${url.toString()})'); _uploads.remove(tag); notifyListeners(); // _uploads change could affect validationErrors } From ece8f726cfa9d33117a0e0e9435aa4982d73ddda Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 16 Jun 2023 18:27:38 -0700 Subject: [PATCH 02/19] dialog [nfc]: Return Future from `showDialog` --- lib/widgets/dialog.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/widgets/dialog.dart b/lib/widgets/dialog.dart index a9bc438af2..985e7cd5f9 100644 --- a/lib/widgets/dialog.dart +++ b/lib/widgets/dialog.dart @@ -15,12 +15,12 @@ Widget _dialogActionText(String text) { } // TODO(i18n): title, message, and action-button text -void showErrorDialog({ +Future showErrorDialog({ required BuildContext context, required String title, String? message, }) { - showDialog( + return showDialog( context: context, builder: (BuildContext context) => AlertDialog( title: Text(title), From e979bef29a85947e0fa859d77ed2b9204b58e22f Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 15 Jun 2023 15:54:55 -0700 Subject: [PATCH 03/19] model [nfc]: Make Message a sealed class --- lib/api/model/model.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/api/model/model.dart b/lib/api/model/model.dart index 175a7b850d..6dbdec8d13 100644 --- a/lib/api/model/model.dart +++ b/lib/api/model/model.dart @@ -247,7 +247,7 @@ class Subscription { /// As in the get-messages response. /// /// https://zulip.com/api/get-messages#response -abstract class Message { +sealed class Message { final String? avatarUrl; final String client; final String content; From 9d66962c0decfb7e856e830a383fe0f3a4eb5e73 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 15 Jun 2023 16:14:26 -0700 Subject: [PATCH 04/19] test [nfc]: Have eg.streamMessage() take optional ZulipStream and topic --- test/example_data.dart | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/test/example_data.dart b/test/example_data.dart index 8146678078..ac9670d7f3 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -103,10 +103,11 @@ Map _messagePropertiesFromSender(User? sender) { }; } -// When we have a Stream object, this can take that as an argument. -// Also it can default explicitly to an example stream. +const _stream = stream; + StreamMessage streamMessage( - {User? sender, String? streamName, int? streamId}) { + {User? sender, ZulipStream? stream, String? topic}) { + final effectiveStream = stream ?? _stream(); // The use of JSON here is convenient in order to delegate parts of the data // to helper functions. The main downside is that it loses static typing // of the properties as we're constructing the data. That's probably OK @@ -115,14 +116,14 @@ StreamMessage streamMessage( return StreamMessage.fromJson({ ..._messagePropertiesBase, ..._messagePropertiesFromSender(sender), - 'display_recipient': streamName ?? 'a stream', - 'stream_id': streamId ?? 123, // TODO generate example IDs + 'display_recipient': effectiveStream.name, + 'stream_id': effectiveStream.streamId, 'content': '

This is an example stream message.

', 'content_type': 'text/html', 'flags': [], 'id': 1234567, // TODO generate example IDs - 'subject': 'example topic', + 'subject': topic ?? 'example topic', 'timestamp': 1678139636, 'type': 'stream', }); From 654d39601449203c511fd965104b002805880355 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 15 Jun 2023 16:17:47 -0700 Subject: [PATCH 05/19] narrow [nfc]: Implement {Topic,Stream,Sendable}Narrow.ofMessage factory And use it in the few places where we're doing the same thing. (*Almost* the same thing: in [DmNarrow.ofMessage], we make the `allRecipientIds` list unmodifiable instead of just not-growable.) --- lib/model/narrow.dart | 20 ++++++++++++++ lib/widgets/message_list.dart | 6 ++--- test/model/narrow_test.dart | 50 +++++++++++++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 4 deletions(-) diff --git a/lib/model/narrow.dart b/lib/model/narrow.dart index 1b640f6665..c3b8ea5025 100644 --- a/lib/model/narrow.dart +++ b/lib/model/narrow.dart @@ -18,6 +18,15 @@ sealed class Narrow { /// A non-interleaved narrow, completely specifying a place to send a message. sealed class SendableNarrow extends Narrow { + factory SendableNarrow.ofMessage(Message message, {required int selfUserId}) { + switch (message) { + case StreamMessage(): + return TopicNarrow.ofMessage(message); + case DmMessage(): + return DmNarrow.ofMessage(message, selfUserId: selfUserId); + } + } + MessageDestination get destination; } @@ -74,6 +83,10 @@ class StreamNarrow extends Narrow { class TopicNarrow extends Narrow implements SendableNarrow { const TopicNarrow(this.streamId, this.topic); + factory TopicNarrow.ofMessage(StreamMessage message) { + return TopicNarrow(message.streamId, message.subject); + } + final int streamId; final String topic; @@ -126,6 +139,13 @@ class DmNarrow extends Narrow implements SendableNarrow { assert(allRecipientIds.contains(selfUserId)), _selfUserId = selfUserId; + factory DmNarrow.ofMessage(DmMessage message, {required int selfUserId}) { + return DmNarrow( + allRecipientIds: List.unmodifiable(message.allRecipientIds), + selfUserId: selfUserId, + ); + } + /// The user IDs of everyone in the conversation, sorted. /// /// Each message in the conversation is sent by one of these users diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 36b5bb31ba..de0315bb58 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -278,7 +278,7 @@ class StreamTopicRecipientHeader extends StatelessWidget { return GestureDetector( onTap: () => Navigator.push(context, MessageListPage.buildRoute(context: context, - narrow: TopicNarrow(message.streamId, message.subject))), + narrow: TopicNarrow.ofMessage(message))), child: ColoredBox( color: _kStreamMessageBorderColor, child: Row(mainAxisAlignment: MainAxisAlignment.start, children: [ @@ -319,9 +319,7 @@ class DmRecipientHeader extends StatelessWidget { behavior: HitTestBehavior.translucent, onTap: () { final store = PerAccountStoreWidget.of(context); - final narrow = DmNarrow( - allRecipientIds: message.allRecipientIds.toList(growable: false), - selfUserId: store.account.userId); + final narrow = DmNarrow.ofMessage(message, selfUserId: store.account.userId); Navigator.push(context, MessageListPage.buildRoute(context: context, narrow: narrow)); }, diff --git a/test/model/narrow_test.dart b/test/model/narrow_test.dart index 552b1e6901..be15e3adf2 100644 --- a/test/model/narrow_test.dart +++ b/test/model/narrow_test.dart @@ -8,6 +8,29 @@ import '../example_data.dart' as eg; import 'narrow_checks.dart'; void main() { + group('SendableNarrow', () { + test('ofMessage: stream message', () { + final message = eg.streamMessage(); + final actual = SendableNarrow.ofMessage(message, selfUserId: eg.selfUser.userId); + check(actual).equals(TopicNarrow.ofMessage(message)); + }); + + test('ofMessage: DM message', () { + final message = eg.dmMessage(from: eg.selfUser, to: [eg.otherUser]); + final actual = SendableNarrow.ofMessage(message, selfUserId: eg.selfUser.userId); + check(actual).equals(DmNarrow.ofMessage(message, selfUserId: eg.selfUser.userId)); + }); + }); + + group('TopicNarrow', () { + test('ofMessage', () { + final stream = eg.stream(); + final message = eg.streamMessage(stream: stream); + final actual = TopicNarrow.ofMessage(message); + check(actual).equals(TopicNarrow(stream.streamId, message.subject)); + }); + }); + group('DmNarrow', () { test('constructor assertions', () { check(() => DmNarrow(allRecipientIds: [2, 12], selfUserId: 2)).returnsNormally(); @@ -19,6 +42,33 @@ void main() { check(() => DmNarrow(allRecipientIds: [], selfUserId: 2)).throws(); }); + test('ofMessage: self-dm', () { + final message = eg.dmMessage(from: eg.selfUser, to: []); + final actual = DmNarrow.ofMessage(message, selfUserId: eg.selfUser.userId); + check(actual).equals(DmNarrow( + allRecipientIds: [eg.selfUser.userId], + selfUserId: eg.selfUser.userId)); + check(() => { + actual.allRecipientIds[0] = eg.otherUser.userId + }).throws(); // "Cannot modify an unmodifiable list" + }); + + test('ofMessage: 1:1', () { + final message = eg.dmMessage(from: eg.selfUser, to: [eg.otherUser]); + final actual = DmNarrow.ofMessage(message, selfUserId: eg.selfUser.userId); + check(actual).equals(DmNarrow( + allRecipientIds: [eg.selfUser.userId, eg.otherUser.userId]..sort(), + selfUserId: eg.selfUser.userId)); + }); + + test('ofMessage: group', () { + final message = eg.dmMessage(from: eg.selfUser, to: [eg.otherUser, eg.thirdUser]); + final actual = DmNarrow.ofMessage(message, selfUserId: eg.selfUser.userId); + check(actual).equals(DmNarrow( + allRecipientIds: [eg.selfUser.userId, eg.otherUser.userId, eg.thirdUser.userId]..sort(), + selfUserId: eg.selfUser.userId)); + }); + test('otherRecipientIds', () { check(DmNarrow(allRecipientIds: [1, 2, 3], selfUserId: 2)) .otherRecipientIds.deepEquals([1, 3]); From 637a6c8687f95b6277f5a1675d8e478ab93f97b5 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 15 Jun 2023 16:20:46 -0700 Subject: [PATCH 06/19] compose: Implement `mention`, for mention syntax --- lib/model/compose.dart | 7 +++++++ test/model/compose_test.dart | 6 ++++++ 2 files changed, 13 insertions(+) diff --git a/lib/model/compose.dart b/lib/model/compose.dart index 9e9691fd1c..eff74c7572 100644 --- a/lib/model/compose.dart +++ b/lib/model/compose.dart @@ -1,5 +1,6 @@ import 'dart:math'; +import '../api/model/model.dart'; import '../api/model/narrow.dart'; import 'narrow.dart'; import 'store.dart'; @@ -175,3 +176,9 @@ Uri narrowLink(PerAccountStore store, Narrow narrow, {int? nearMessageId}) { return store.account.realmUrl.replace(fragment: fragment.toString()); } + +// TODO like web, use just the name, no ID, when that wouldn't be ambiguous. +// It looks nicer while the user's still composing and looking at the source. +String mention(User user, {bool silent = false}) { + return '@${silent ? '_' : ''}**${user.fullName}|${user.userId}**'; +} diff --git a/test/model/compose_test.dart b/test/model/compose_test.dart index 77472ab564..97773e440c 100644 --- a/test/model/compose_test.dart +++ b/test/model/compose_test.dart @@ -300,4 +300,10 @@ hello // TODO other Narrow subclasses as we add them: // starred, mentioned; searches; arbitrary }); + + test('mention', () { + final user = eg.user(userId: 123, fullName: 'Full Name'); + check(mention(user, silent: false)).equals('@**Full Name|123**'); + check(mention(user, silent: true)).equals('@_**Full Name|123**'); + }); } From 763f1f91baaabe1135698358a6a0101124c204a7 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 15 Jun 2023 17:12:04 -0700 Subject: [PATCH 07/19] compose: Implement `inlineLink`, for Markdown "inline link" syntax And use it in the one place where we've been creating inline links. This is a small amount of code, but drawing it out into a helper gives a convenient place to write down its shortcomings. We'll also use this for #116 quote-and-reply, coming up. --- lib/model/compose.dart | 22 ++++++++++++++++++++++ lib/widgets/compose_box.dart | 5 +++-- test/model/compose_test.dart | 7 +++++++ 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/lib/model/compose.dart b/lib/model/compose.dart index eff74c7572..f8c90b1272 100644 --- a/lib/model/compose.dart +++ b/lib/model/compose.dart @@ -182,3 +182,25 @@ Uri narrowLink(PerAccountStore store, Narrow narrow, {int? nearMessageId}) { String mention(User user, {bool silent = false}) { return '@${silent ? '_' : ''}**${user.fullName}|${user.userId}**'; } + +/// https://spec.commonmark.org/0.30/#inline-link +/// +/// The "link text" is made by enclosing [visibleText] in square brackets. +/// If [visibleText] has unexpected features, such as square brackets, the +/// result may be surprising. +/// +/// The part between "(" and ")" is just a "link destination" (no "link title"). +/// That destination is simply the stringified [destination], if provided. +/// If that has parentheses in it, the result may be surprising. +// TODO: Try harder to guarantee output that creates an inline link, +// and in particular, the intended one. We could help with this by escaping +// square brackets, perhaps with HTML character references: +// https://github.com/zulip/zulip-flutter/pull/201#discussion_r1237951626 +// It's also tricky because nearby content can thwart the inline-link syntax. +// From the spec: +// > Backtick code spans, autolinks, and raw HTML tags bind more tightly +// > than the brackets in link text. Thus, for example, [foo`]` could not be +// > a link text, since the second ] is part of a code span. +String inlineLink(String visibleText, Uri? destination) { + return '[$visibleText](${destination?.toString() ?? ''})'; +} diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 4bc213dd56..206facc7de 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -6,6 +6,7 @@ import 'package:image_picker/image_picker.dart'; import '../api/route/messages.dart'; import '../model/autocomplete.dart'; +import '../model/compose.dart'; import '../model/narrow.dart'; import 'dialog.dart'; import 'store.dart'; @@ -135,7 +136,7 @@ class ComposeContentController extends ComposeController int registerUploadStart(String filename) { final tag = _nextUploadTag; _nextUploadTag += 1; - final placeholder = '[Uploading $filename...]()'; // TODO(i18n) + final placeholder = inlineLink('Uploading $filename...', null); // TODO(i18n) _uploads[tag] = (filename: filename, placeholder: placeholder); notifyListeners(); // _uploads change could affect validationErrors value = value.replaced(_insertionIndex(), '$placeholder\n\n'); @@ -157,7 +158,7 @@ class ComposeContentController extends ComposeController value = value.replaced( replacementRange, - url == null ? '' : '[$filename](${url.toString()})'); + url == null ? '' : inlineLink(filename, url)); _uploads.remove(tag); notifyListeners(); // _uploads change could affect validationErrors } diff --git a/test/model/compose_test.dart b/test/model/compose_test.dart index 97773e440c..887ddf02ea 100644 --- a/test/model/compose_test.dart +++ b/test/model/compose_test.dart @@ -306,4 +306,11 @@ hello check(mention(user, silent: false)).equals('@**Full Name|123**'); check(mention(user, silent: true)).equals('@_**Full Name|123**'); }); + + test('inlineLink', () { + check(inlineLink('CZO', Uri.parse('https://chat.zulip.org/'))).equals('[CZO](https://chat.zulip.org/)'); + check(inlineLink('Uploading file.txt…', null)).equals('[Uploading file.txt…]()'); + check(inlineLink('IMG_2488.png', Uri.parse('/user_uploads/2/a3/ucEMyjxk90mcNF0y9rmW5XKO/IMG_2488.png'))) + .equals('[IMG_2488.png](/user_uploads/2/a3/ucEMyjxk90mcNF0y9rmW5XKO/IMG_2488.png)'); + }); } From bb38b4a784ea1d4b833f8420103cc2f54dce066d Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 15 Jun 2023 17:16:07 -0700 Subject: [PATCH 08/19] compose: Implement quoteAndReply/quoteAndReplyPlaceholder --- lib/model/compose.dart | 34 ++++++++++++++++++++++++++++++++++ test/model/compose_test.dart | 20 ++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/lib/model/compose.dart b/lib/model/compose.dart index f8c90b1272..4f915a7153 100644 --- a/lib/model/compose.dart +++ b/lib/model/compose.dart @@ -204,3 +204,37 @@ String mention(User user, {bool silent = false}) { String inlineLink(String visibleText, Uri? destination) { return '[$visibleText](${destination?.toString() ?? ''})'; } + +/// What we show while fetching the target message's raw Markdown. +String quoteAndReplyPlaceholder(PerAccountStore store, { + required Message message, +}) { + final sender = store.users[message.senderId]; + assert(sender != null); + final url = narrowLink(store, + SendableNarrow.ofMessage(message, selfUserId: store.account.userId), + nearMessageId: message.id); + return '${mention(sender!, silent: true)} ${inlineLink('said', url)}: ' // TODO(i18n) ? + '*(loading message ${message.id})*\n'; // TODO(i18n) ? +} + +/// Quote-and-reply syntax. +/// +/// The result looks like it does in Zulip web: +/// +/// @_**Iago|5** [said](link to message): +/// ```quote +/// message content +/// ``` +String quoteAndReply(PerAccountStore store, { + required Message message, + required String rawContent, +}) { + final sender = store.users[message.senderId]; + assert(sender != null); + final url = narrowLink(store, + SendableNarrow.ofMessage(message, selfUserId: store.account.userId), + nearMessageId: message.id); + return '${mention(sender!, silent: true)} ${inlineLink('said', url)}:\n' // TODO(i18n) ? + '${wrapWithBacktickFence(content: rawContent, infoString: 'quote')}'; +} diff --git a/test/model/compose_test.dart b/test/model/compose_test.dart index 887ddf02ea..59ab726c9b 100644 --- a/test/model/compose_test.dart +++ b/test/model/compose_test.dart @@ -313,4 +313,24 @@ hello check(inlineLink('IMG_2488.png', Uri.parse('/user_uploads/2/a3/ucEMyjxk90mcNF0y9rmW5XKO/IMG_2488.png'))) .equals('[IMG_2488.png](/user_uploads/2/a3/ucEMyjxk90mcNF0y9rmW5XKO/IMG_2488.png)'); }); + + test('quoteAndReply / quoteAndReplyPlaceholder', () { + final sender = eg.user(userId: 123, fullName: 'Full Name'); + final stream = eg.stream(streamId: 1, name: 'test here'); + final message = eg.streamMessage(sender: sender, stream: stream, topic: 'some topic'); + final store = eg.store(); + store.addStream(stream); + store.addUser(sender); + + check(quoteAndReplyPlaceholder(store, message: message)).equals(''' +@_**Full Name|123** [said](${eg.selfAccount.realmUrl}#narrow/stream/1-test-here/topic/some.20topic/near/${message.id}): *(loading message ${message.id})* +'''); + + check(quoteAndReply(store, message: message, rawContent: 'Hello world!')).equals(''' +@_**Full Name|123** [said](${eg.selfAccount.realmUrl}#narrow/stream/1-test-here/topic/some.20topic/near/${message.id}): +```quote +Hello world! +``` +'''); + }); } From 432d8c61a6b9981f71941a10b36c914d5129ae53 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 15 Jun 2023 10:17:50 -0700 Subject: [PATCH 09/19] action_sheet [nfc]: Rename a param to stop shadowing --- lib/widgets/action_sheet.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/widgets/action_sheet.dart b/lib/widgets/action_sheet.dart index 2fb9a27356..b840cba82c 100644 --- a/lib/widgets/action_sheet.dart +++ b/lib/widgets/action_sheet.dart @@ -7,7 +7,7 @@ import 'draggable_scrollable_modal_bottom_sheet.dart'; void showMessageActionSheet({required BuildContext context, required Message message}) { showDraggableScrollableModalBottomSheet( context: context, - builder: (BuildContext context) { + builder: (BuildContext innerContext) { return Column(children: [ MenuItemButton( leadingIcon: Icon(Icons.adaptive.share), @@ -20,7 +20,7 @@ void showMessageActionSheet({required BuildContext context, required Message mes // reopen (if it was open at the time of this // `showMessageActionSheet` call) and cover a large part of the // share sheet. - Navigator.of(context).pop(); + Navigator.of(innerContext).pop(); // TODO: to support iPads, we're asked to give a // `sharePositionOrigin` param, or risk crashing / hanging: From 046de6dcdfeed8238cdc7b93e9d7327257cc2f2c Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 15 Jun 2023 17:20:52 -0700 Subject: [PATCH 10/19] msglist [nfc]: Convert MessageListPage to StatefulWidget --- lib/widgets/message_list.dart | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index de0315bb58..b9d6384b4e 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -14,7 +14,7 @@ import 'page.dart'; import 'sticky_header.dart'; import 'store.dart'; -class MessageListPage extends StatelessWidget { +class MessageListPage extends StatefulWidget { const MessageListPage({super.key, required this.narrow}); static Route buildRoute({required BuildContext context, required Narrow narrow}) { @@ -24,10 +24,15 @@ class MessageListPage extends StatelessWidget { final Narrow narrow; + @override + State createState() => _MessageListPageState(); +} + +class _MessageListPageState extends State { @override Widget build(BuildContext context) { return Scaffold( - appBar: AppBar(title: MessageListAppBarTitle(narrow: narrow)), + appBar: AppBar(title: MessageListAppBarTitle(narrow: widget.narrow)), body: Builder( builder: (BuildContext context) => Center( child: Column(children: [ @@ -41,9 +46,9 @@ class MessageListPage extends StatelessWidget { removeBottom: true, child: Expanded( - child: MessageList(narrow: narrow))), + child: MessageList(narrow: widget.narrow))), - ComposeBox(narrow: narrow), + ComposeBox(narrow: widget.narrow), ])))); } } From f9f5e79dc88728fa7a3574dd5622d2e935d9c497 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 16 Jun 2023 17:08:14 -0700 Subject: [PATCH 11/19] compose: Implement [insertPadded] on ComposeContentController --- lib/widgets/compose_box.dart | 29 ++++++++ test/widgets/compose_box_test.dart | 108 +++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+) create mode 100644 test/widgets/compose_box_test.dart diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 206facc7de..79c1f0b267 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -129,6 +129,35 @@ class ComposeContentController extends ComposeController : TextRange.collapsed(text.length); } + /// Inserts [newText] in [text], setting off with an empty line before and after. + /// + /// Assumes [newText] is not empty and consists entirely of complete lines + /// (each line ends with a newline). + /// + /// Inserts at [_insertionIndex]. If that's zero, no empty line is added before. + /// + /// If there is already an empty line before or after, does not add another. + void insertPadded(String newText) { // ignore: unused_element + assert(newText.isNotEmpty); + assert(newText.endsWith('\n')); + final i = _insertionIndex(); + final textBefore = text.substring(0, i.start); + final String paddingBefore; + if (textBefore.isEmpty || textBefore == '\n' || textBefore.endsWith('\n\n')) { + paddingBefore = ''; // At start of input, or just after an empty line. + } else if (textBefore.endsWith('\n')) { + paddingBefore = '\n'; // After a complete but non-empty line. + } else { + paddingBefore = '\n\n'; // After an incomplete line. + } + if (text.substring(i.start).startsWith('\n')) { + final partial = value.replaced(i, paddingBefore + newText); + value = partial.copyWith(selection: TextSelection.collapsed(offset: partial.selection.start + 1)); + } else { + value = value.replaced(i, '$paddingBefore$newText\n'); + } + } + /// Tells the controller that a file upload has started. /// /// Returns an int "tag" that should be passed to registerUploadEnd on the diff --git a/test/widgets/compose_box_test.dart b/test/widgets/compose_box_test.dart new file mode 100644 index 0000000000..76fc263307 --- /dev/null +++ b/test/widgets/compose_box_test.dart @@ -0,0 +1,108 @@ +import 'package:checks/checks.dart'; +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:zulip/widgets/compose_box.dart'; + +void main() { + group('ComposeContentController', () { + group('insertPadded', () { + // Like `parseMarkedText` in test/model/autocomplete_test.dart, + // but a bit different -- could maybe deduplicate some. + parseMarkedText(String markedText) { + final textBuffer = StringBuffer(); + int? insertionPoint; + int i = 0; + for (final char in markedText.codeUnits) { + if (char == 94 /* ^ */) { + if (insertionPoint != null) { + throw Exception('Test error: too many ^ in input'); + } + insertionPoint = i; + continue; + } + textBuffer.writeCharCode(char); + i++; + } + if (insertionPoint == null) { + throw Exception('Test error: expected ^ in input'); + } + return TextEditingValue(text: textBuffer.toString(), selection: TextSelection.collapsed(offset: insertionPoint)); + } + + /// Test the given `insertPadded` call, in a convenient format. + /// + /// In valueBefore, represent the insertion point as "^". + /// In expectedValue, represent the collapsed selection as "^". + testInsertPadded(String description, String valueBefore, String textToInsert, String expectedValue) { + test(description, () { + final controller = ComposeContentController(); + controller.value = parseMarkedText(valueBefore); + controller.insertPadded(textToInsert); + check(controller.value).equals(parseMarkedText(expectedValue)); + }); + } + + // TODO(?) exercise the part of insertPadded that chooses the insertion + // point based on [TextEditingValue.selection], which may be collapsed, + // expanded, or null (what they call !TextSelection.isValid). + + testInsertPadded('empty; insert one line', + '^', 'a\n', 'a\n\n^'); + testInsertPadded('empty; insert two lines', + '^', 'a\nb\n', 'a\nb\n\n^'); + + group('insert at end', () { + testInsertPadded('one empty line; insert one line', + '\n^', 'a\n', '\na\n\n^'); + testInsertPadded('two empty lines; insert one line', + '\n\n^', 'a\n', '\n\na\n\n^'); + testInsertPadded('one line, incomplete; insert one line', + 'a^', 'b\n', 'a\n\nb\n\n^'); + testInsertPadded('one line, complete; insert one line', + 'a\n^', 'b\n', 'a\n\nb\n\n^'); + testInsertPadded('multiple lines, last is incomplete; insert one line', + 'a\nb^', 'c\n', 'a\nb\n\nc\n\n^'); + testInsertPadded('multiple lines, last is complete; insert one line', + 'a\nb\n^', 'c\n', 'a\nb\n\nc\n\n^'); + testInsertPadded('multiple lines, last is complete; insert two lines', + 'a\nb\n^', 'c\nd\n', 'a\nb\n\nc\nd\n\n^'); + }); + + group('insert at start', () { + testInsertPadded('one empty line; insert one line', + '^\n', 'a\n', 'a\n\n^'); + testInsertPadded('two empty lines; insert one line', + '^\n\n', 'a\n', 'a\n\n^\n'); + testInsertPadded('one line, incomplete; insert one line', + '^a', 'b\n', 'b\n\n^a'); + testInsertPadded('one line, complete; insert one line', + '^a\n', 'b\n', 'b\n\n^a\n'); + testInsertPadded('multiple lines, last is incomplete; insert one line', + '^a\nb', 'c\n', 'c\n\n^a\nb'); + testInsertPadded('multiple lines, last is complete; insert one line', + '^a\nb\n', 'c\n', 'c\n\n^a\nb\n'); + testInsertPadded('multiple lines, last is complete; insert two lines', + '^a\nb\n', 'c\nd\n', 'c\nd\n\n^a\nb\n'); + }); + + group('insert in middle', () { + testInsertPadded('middle of line', + 'a^a\n', 'b\n', 'a\n\nb\n\n^a\n'); + testInsertPadded('start of non-empty line, after empty line', + 'b\n\n^a\n', 'c\n', 'b\n\nc\n\n^a\n'); + testInsertPadded('end of non-empty line, before non-empty line', + 'a^\nb\n', 'c\n', 'a\n\nc\n\n^b\n'); + testInsertPadded('start of non-empty line, after non-empty line', + 'a\n^b\n', 'c\n', 'a\n\nc\n\n^b\n'); + testInsertPadded('text start; one empty line; insertion point; one empty line', + '\n^\n', 'a\n', '\na\n\n^'); + testInsertPadded('text start; one empty line; insertion point; two empty lines', + '\n^\n\n', 'a\n', '\na\n\n^\n'); + testInsertPadded('text start; two empty lines; insertion point; one empty line', + '\n\n^\n', 'a\n', '\n\na\n\n^'); + testInsertPadded('text start; two empty lines; insertion point; two empty lines', + '\n\n^\n\n', 'a\n', '\n\na\n\n^\n'); + }); + }); + }); +} From 16950fa80aacdf57aeb4f9b9c5d8b51df06411c5 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 15 Jun 2023 17:34:48 -0700 Subject: [PATCH 12/19] compose: Add ComposeContentController.registerQuoteAndReply{Start,End} --- lib/widgets/compose_box.dart | 54 ++++++++++++++++++++++++++++++++++-- 1 file changed, 51 insertions(+), 3 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 79c1f0b267..bc53fe6364 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -4,10 +4,12 @@ import 'package:flutter/material.dart'; import 'package:flutter/services.dart'; import 'package:image_picker/image_picker.dart'; +import '../api/model/model.dart'; import '../api/route/messages.dart'; import '../model/autocomplete.dart'; import '../model/compose.dart'; import '../model/narrow.dart'; +import '../model/store.dart'; import 'dialog.dart'; import 'store.dart'; @@ -84,16 +86,17 @@ class ComposeTopicController extends ComposeController { enum ContentValidationError { empty, tooLong, + quoteAndReplyInProgress, uploadInProgress; - // Later: quote-and-reply in progress - String message() { switch (this) { case ContentValidationError.tooLong: return "Message length shouldn't be greater than 10000 characters."; case ContentValidationError.empty: return 'You have nothing to send!'; + case ContentValidationError.quoteAndReplyInProgress: + return 'Please wait for the quotation to complete.'; case ContentValidationError.uploadInProgress: return 'Please wait for the upload to complete.'; } @@ -105,8 +108,10 @@ class ComposeContentController extends ComposeController _update(); } + int _nextQuoteAndReplyTag = 0; int _nextUploadTag = 0; + final Map _quoteAndReplies = {}; final Map _uploads = {}; /// A probably-reasonable place to insert Markdown, such as for a file upload. @@ -137,7 +142,7 @@ class ComposeContentController extends ComposeController /// Inserts at [_insertionIndex]. If that's zero, no empty line is added before. /// /// If there is already an empty line before or after, does not add another. - void insertPadded(String newText) { // ignore: unused_element + void insertPadded(String newText) { assert(newText.isNotEmpty); assert(newText.endsWith('\n')); final i = _insertionIndex(); @@ -158,6 +163,46 @@ class ComposeContentController extends ComposeController } } + /// Tells the controller that a quote-and-reply has started. + /// + /// Returns an int "tag" that should be passed to registerQuoteAndReplyEnd on + /// success or failure + int registerQuoteAndReplyStart(PerAccountStore store, {required Message message}) { + final tag = _nextQuoteAndReplyTag; + _nextQuoteAndReplyTag += 1; + final placeholder = quoteAndReplyPlaceholder(store, message: message); + _quoteAndReplies[tag] = (messageId: message.id, placeholder: placeholder); + notifyListeners(); // _quoteAndReplies change could affect validationErrors + insertPadded(placeholder); + return tag; + } + + /// Tells the controller that a quote-and-reply has ended, with success or error. + /// + /// To indicate success, pass [rawContent]. + /// If that is null, failure is assumed. + void registerQuoteAndReplyEnd(PerAccountStore store, int tag, { + required Message message, + String? rawContent, + }) { + final val = _quoteAndReplies[tag]; + assert(val != null, 'registerQuoteAndReplyEnd called twice for same tag'); + final int startIndex = text.indexOf(val!.placeholder); + final replacementText = rawContent == null + ? '' + : quoteAndReply(store, message: message, rawContent: rawContent); + if (startIndex >= 0) { + value = value.replaced( + TextRange(start: startIndex, end: startIndex + val.placeholder.length), + replacementText, + ); + } else if (replacementText != '') { // insertPadded requires non-empty string + insertPadded(replacementText); + } + _quoteAndReplies.remove(tag); + notifyListeners(); // _quoteAndReplies change could affect validationErrors + } + /// Tells the controller that a file upload has started. /// /// Returns an int "tag" that should be passed to registerUploadEnd on the @@ -209,6 +254,9 @@ class ComposeContentController extends ComposeController if (textNormalized.length > kMaxMessageLengthCodePoints) ContentValidationError.tooLong, + if (_quoteAndReplies.isNotEmpty) + ContentValidationError.quoteAndReplyInProgress, + if (_uploads.isNotEmpty) ContentValidationError.uploadInProgress, ]; From 027663757b5ef113b44050ceb30f4554f6f14201 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 16 Jun 2023 14:50:09 -0700 Subject: [PATCH 13/19] compose: Give MessageListPageState access to topic/content controllers Then, widgets that have MessageListPageState in their ancestry will be able to set the topic and content inputs as part of managing a quote-and-reply interaction. --- lib/widgets/compose_box.dart | 33 +++++++++++++++++++++++++-------- lib/widgets/message_list.dart | 14 +++++++++++++- 2 files changed, 38 insertions(+), 9 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index bc53fe6364..6681423a95 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -851,12 +851,18 @@ class _ComposeBoxLayout extends StatelessWidget { ])))); } } +abstract class ComposeBoxController extends State { + ComposeTopicController? get topicController; + ComposeContentController get contentController; + FocusNode get contentFocusNode; +} + /// A compose box for use in a stream narrow. /// /// This offers a text input for the topic to send to, /// in addition to a text input for the message content. class _StreamComposeBox extends StatefulWidget { - const _StreamComposeBox({required this.narrow}); + const _StreamComposeBox({super.key, required this.narrow}); /// The narrow on view in the message list. final StreamNarrow narrow; @@ -865,9 +871,14 @@ class _StreamComposeBox extends StatefulWidget { State<_StreamComposeBox> createState() => _StreamComposeBoxState(); } -class _StreamComposeBoxState extends State<_StreamComposeBox> { +class _StreamComposeBoxState extends State<_StreamComposeBox> implements ComposeBoxController<_StreamComposeBox> { + @override ComposeTopicController get topicController => _topicController; final _topicController = ComposeTopicController(); + + @override ComposeContentController get contentController => _contentController; final _contentController = ComposeContentController(); + + @override FocusNode get contentFocusNode => _contentFocusNode; final _contentFocusNode = FocusNode(); @override @@ -906,7 +917,7 @@ class _StreamComposeBoxState extends State<_StreamComposeBox> { } class _FixedDestinationComposeBox extends StatefulWidget { - const _FixedDestinationComposeBox({required this.narrow}); + const _FixedDestinationComposeBox({super.key, required this.narrow}); final SendableNarrow narrow; @@ -914,8 +925,13 @@ class _FixedDestinationComposeBox extends StatefulWidget { State<_FixedDestinationComposeBox> createState() => _FixedDestinationComposeBoxState(); } -class _FixedDestinationComposeBoxState extends State<_FixedDestinationComposeBox> { +class _FixedDestinationComposeBoxState extends State<_FixedDestinationComposeBox> implements ComposeBoxController<_FixedDestinationComposeBox> { + @override ComposeTopicController? get topicController => null; + + @override ComposeContentController get contentController => _contentController; final _contentController = ComposeContentController(); + + @override FocusNode get contentFocusNode => _contentFocusNode; final _contentFocusNode = FocusNode(); @override @@ -945,19 +961,20 @@ class _FixedDestinationComposeBoxState extends State<_FixedDestinationComposeBox } class ComposeBox extends StatelessWidget { - const ComposeBox({super.key, required this.narrow}); + const ComposeBox({super.key, this.controllerKey, required this.narrow}); + final GlobalKey? controllerKey; final Narrow narrow; @override Widget build(BuildContext context) { final narrow = this.narrow; if (narrow is StreamNarrow) { - return _StreamComposeBox(narrow: narrow); + return _StreamComposeBox(key: controllerKey, narrow: narrow); } else if (narrow is TopicNarrow) { - return _FixedDestinationComposeBox(narrow: narrow); + return _FixedDestinationComposeBox(key: controllerKey, narrow: narrow); } else if (narrow is DmNarrow) { - return _FixedDestinationComposeBox(narrow: narrow); + return _FixedDestinationComposeBox(key: controllerKey, narrow: narrow); } else if (narrow is AllMessagesNarrow) { return const SizedBox.shrink(); } else { diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index b9d6384b4e..5bd50c63d9 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -22,6 +22,16 @@ class MessageListPage extends StatefulWidget { builder: (context) => MessageListPage(narrow: narrow)); } + /// A [ComposeBoxController], if this [MessageListPage] offers a compose box. + /// + /// Uses the inefficient [BuildContext.findAncestorStateOfType]; + /// don't call this in a build method. + static ComposeBoxController? composeBoxControllerOf(BuildContext context) { + final messageListPageState = context.findAncestorStateOfType<_MessageListPageState>(); + assert(messageListPageState != null, 'No MessageListPage ancestor'); + return messageListPageState!._composeBoxKey.currentState; + } + final Narrow narrow; @override @@ -29,6 +39,8 @@ class MessageListPage extends StatefulWidget { } class _MessageListPageState extends State { + final GlobalKey _composeBoxKey = GlobalKey(); + @override Widget build(BuildContext context) { return Scaffold( @@ -48,7 +60,7 @@ class _MessageListPageState extends State { child: Expanded( child: MessageList(narrow: widget.narrow))), - ComposeBox(narrow: widget.narrow), + ComposeBox(controllerKey: _composeBoxKey, narrow: widget.narrow), ])))); } } From 1e52dc4f4b711bc8243bed84b0e8dbe16b3fa892 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 16 Jun 2023 15:16:24 -0700 Subject: [PATCH 14/19] action_sheet [nfc]: Pull out base class for message action sheet buttons We're about to add another button, for quote-and-reply #116. --- lib/widgets/action_sheet.dart | 77 ++++++++++++++++++++++++----------- 1 file changed, 54 insertions(+), 23 deletions(-) diff --git a/lib/widgets/action_sheet.dart b/lib/widgets/action_sheet.dart index b840cba82c..28daad427e 100644 --- a/lib/widgets/action_sheet.dart +++ b/lib/widgets/action_sheet.dart @@ -7,30 +7,61 @@ import 'draggable_scrollable_modal_bottom_sheet.dart'; void showMessageActionSheet({required BuildContext context, required Message message}) { showDraggableScrollableModalBottomSheet( context: context, - builder: (BuildContext innerContext) { + builder: (BuildContext _) { return Column(children: [ - MenuItemButton( - leadingIcon: Icon(Icons.adaptive.share), - onPressed: () async { - // Close the message action sheet; we're about to show the share - // sheet. (We could do this after the sharing Future settles, but - // on iOS I get impatient with how slowly our action sheet - // dismisses in that case.) - // TODO(#24): Fix iOS bug where this call causes the keyboard to - // reopen (if it was open at the time of this - // `showMessageActionSheet` call) and cover a large part of the - // share sheet. - Navigator.of(innerContext).pop(); - - // TODO: to support iPads, we're asked to give a - // `sharePositionOrigin` param, or risk crashing / hanging: - // https://pub.dev/packages/share_plus#ipad - // Perhaps a wart in the API; discussion: - // https://github.com/zulip/zulip-flutter/pull/12#discussion_r1130146231 - // TODO: Share raw Markdown, not HTML - await Share.shareWithResult(message.content); - }, - child: const Text('Share')), + ShareButton(message: message), ]); }); } + +abstract class MessageActionSheetMenuItemButton extends StatelessWidget { + const MessageActionSheetMenuItemButton({ + super.key, + required this.message, + }); + + IconData get icon; + String get label; + void Function(BuildContext) get onPressed; + + final Message message; + + @override + Widget build(BuildContext context) { + return MenuItemButton( + leadingIcon: Icon(icon), + onPressed: () => onPressed(context), + child: Text(label)); + } +} + +class ShareButton extends MessageActionSheetMenuItemButton { + const ShareButton({ + super.key, + required super.message, + }); + + @override get icon => Icons.adaptive.share; + + @override get label => 'Share'; + + @override get onPressed => (BuildContext context) async { + // Close the message action sheet; we're about to show the share + // sheet. (We could do this after the sharing Future settles, but + // on iOS I get impatient with how slowly our action sheet + // dismisses in that case.) + // TODO(#24): Fix iOS bug where this call causes the keyboard to + // reopen (if it was open at the time of this + // `showMessageActionSheet` call) and cover a large part of the + // share sheet. + Navigator.of(context).pop(); + + // TODO: to support iPads, we're asked to give a + // `sharePositionOrigin` param, or risk crashing / hanging: + // https://pub.dev/packages/share_plus#ipad + // Perhaps a wart in the API; discussion: + // https://github.com/zulip/zulip-flutter/pull/12#discussion_r1130146231 + // TODO: Share raw Markdown, not HTML + await Share.shareWithResult(message.content); + }; +} From 90be3e612cf64e5dca6c6f96c5f09b3b3cc9948c Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 23 Jun 2023 11:50:48 -0700 Subject: [PATCH 15/19] api tests: Have FakeHttpClient.send enqueue response handling in a new task The Future returned by FakeHttpClient.send is created with either Future.value or Future.error, which means it'll complete in a *microtask*: https://web.archive.org/web/20170704074724/https://webdev.dartlang.org/articles/performance/event-loop#event-queue-new-future That's too soon, as a simulation for a real API response coming over an HTTP connection. In the live code that this simulates, the Future completes in a new task. So, mimic that behavior. --- test/api/fake_api.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/api/fake_api.dart b/test/api/fake_api.dart index 11a8fc6432..924f4c758b 100644 --- a/test/api/fake_api.dart +++ b/test/api/fake_api.dart @@ -73,10 +73,10 @@ class FakeHttpClient extends http.BaseClient { lastRequest = request; switch (response) { case _PreparedException(:var exception): - return Future.error(exception); + return Future(() => throw exception); case _PreparedSuccess(:var bytes, :var httpStatus): final byteStream = http.ByteStream.fromBytes(bytes); - return Future.value(http.StreamedResponse( + return Future(() => http.StreamedResponse( byteStream, httpStatus, request: request)); } } From 7e05d7c21e8bf08eadc53a3ab6af51deb10523f8 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Fri, 23 Jun 2023 12:00:42 -0700 Subject: [PATCH 16/19] test: Have eg.streamMessage and eg.dmMessage accept message content --- test/example_data.dart | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/test/example_data.dart b/test/example_data.dart index ac9670d7f3..fa61f11894 100644 --- a/test/example_data.dart +++ b/test/example_data.dart @@ -103,10 +103,25 @@ Map _messagePropertiesFromSender(User? sender) { }; } +Map _messagePropertiesFromContent(String? content, String? contentMarkdown) { + if (contentMarkdown != null) { + assert(content == null); + return { + 'content': contentMarkdown, + 'content_type': 'text/x-markdown', + }; + } else { + return { + 'content': content ?? '

This is an example message.

', + 'content_type': 'text/html', + }; + } +} + const _stream = stream; StreamMessage streamMessage( - {User? sender, ZulipStream? stream, String? topic}) { + {User? sender, ZulipStream? stream, String? topic, String? content, String? contentMarkdown}) { final effectiveStream = stream ?? _stream(); // The use of JSON here is convenient in order to delegate parts of the data // to helper functions. The main downside is that it loses static typing @@ -116,11 +131,9 @@ StreamMessage streamMessage( return StreamMessage.fromJson({ ..._messagePropertiesBase, ..._messagePropertiesFromSender(sender), + ..._messagePropertiesFromContent(content, contentMarkdown), 'display_recipient': effectiveStream.name, 'stream_id': effectiveStream.streamId, - - 'content': '

This is an example stream message.

', - 'content_type': 'text/html', 'flags': [], 'id': 1234567, // TODO generate example IDs 'subject': topic ?? 'example topic', @@ -133,17 +146,17 @@ StreamMessage streamMessage( /// /// See also: /// * [streamMessage], to construct an example stream message. -DmMessage dmMessage({required User from, required List to}) { +DmMessage dmMessage( + {required User from, required List to, String? content, String? contentMarkdown}) { assert(!to.any((user) => user.userId == from.userId)); return DmMessage.fromJson({ ..._messagePropertiesBase, ..._messagePropertiesFromSender(from), + ..._messagePropertiesFromContent(content, contentMarkdown), 'display_recipient': [from, ...to] .map((u) => {'id': u.userId, 'email': u.email, 'full_name': u.fullName}) .toList(growable: false), - 'content': '

This is an example DM.

', - 'content_type': 'text/html', 'flags': [], 'id': 1234567, // TODO generate example IDs 'subject': '', From 33707b66157d5d560549bdc3e04714efa953750f Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 26 Jun 2023 17:38:22 -0700 Subject: [PATCH 17/19] compose [nfc]: Make ComposeContentController.insertionIndex non-private --- lib/widgets/compose_box.dart | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index 6681423a95..356a2c028f 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -124,7 +124,7 @@ class ComposeContentController extends ComposeController /// gives the end of the whole text. /// /// Expressed as a collapsed `TextRange` at the index. - TextRange _insertionIndex() { + TextRange insertionIndex() { final TextRange selection = value.selection; final String text = value.text; return selection.isValid @@ -139,13 +139,13 @@ class ComposeContentController extends ComposeController /// Assumes [newText] is not empty and consists entirely of complete lines /// (each line ends with a newline). /// - /// Inserts at [_insertionIndex]. If that's zero, no empty line is added before. + /// Inserts at [insertionIndex]. If that's zero, no empty line is added before. /// /// If there is already an empty line before or after, does not add another. void insertPadded(String newText) { assert(newText.isNotEmpty); assert(newText.endsWith('\n')); - final i = _insertionIndex(); + final i = insertionIndex(); final textBefore = text.substring(0, i.start); final String paddingBefore; if (textBefore.isEmpty || textBefore == '\n' || textBefore.endsWith('\n\n')) { @@ -213,7 +213,7 @@ class ComposeContentController extends ComposeController final placeholder = inlineLink('Uploading $filename...', null); // TODO(i18n) _uploads[tag] = (filename: filename, placeholder: placeholder); notifyListeners(); // _uploads change could affect validationErrors - value = value.replaced(_insertionIndex(), '$placeholder\n\n'); + value = value.replaced(insertionIndex(), '$placeholder\n\n'); return tag; } @@ -228,7 +228,7 @@ class ComposeContentController extends ComposeController final int startIndex = text.indexOf(placeholder); final replacementRange = startIndex >= 0 ? TextRange(start: startIndex, end: startIndex + placeholder.length) - : _insertionIndex(); + : insertionIndex(); value = value.replaced( replacementRange, From 02fd562b8c75249616abdcf17521a9683b1b2d27 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 27 Jun 2023 12:29:27 -0700 Subject: [PATCH 18/19] widget tests: Add `checkErrorDialog` --- test/widgets/dialog_checks.dart | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 test/widgets/dialog_checks.dart diff --git a/test/widgets/dialog_checks.dart b/test/widgets/dialog_checks.dart new file mode 100644 index 0000000000..84593fb59d --- /dev/null +++ b/test/widgets/dialog_checks.dart @@ -0,0 +1,26 @@ +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; + +/// In a widget test, check that showErrorDialog was called with the right text. +/// +/// Checks for an error dialog matching an expected title +/// and, optionally, matching an expected message. Fails if none is found. +/// +/// On success, returns the widget's "OK" button. +/// Dismiss the dialog by calling `tester.tap(find.byWidget(okButton))`. +Widget checkErrorDialog(WidgetTester tester, { + required String expectedTitle, + String? expectedMessage, +}) { + final dialog = tester.widget(find.byType(AlertDialog)); + tester.widget(find.descendant(matchRoot: true, + of: find.byWidget(dialog.title!), matching: find.text(expectedTitle))); + if (expectedMessage != null) { + tester.widget(find.descendant(matchRoot: true, + of: find.byWidget(dialog.content!), matching: find.text(expectedMessage))); + } + + return tester.widget( + find.descendant(of: find.byWidget(dialog), + matching: find.widgetWithText(TextButton, 'OK'))); +} From 406b7d017632f8810016205194e1714e75607a2c Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 14 Jun 2023 17:52:46 -0700 Subject: [PATCH 19/19] action_sheet: Add "Quote and reply" button Fixes: #116 --- lib/widgets/action_sheet.dart | 117 +++++++++++++- test/flutter_checks.dart | 6 + test/widgets/action_sheet_test.dart | 223 +++++++++++++++++++++++++++ test/widgets/compose_box_checks.dart | 16 ++ 4 files changed, 358 insertions(+), 4 deletions(-) create mode 100644 test/widgets/action_sheet_test.dart create mode 100644 test/widgets/compose_box_checks.dart diff --git a/lib/widgets/action_sheet.dart b/lib/widgets/action_sheet.dart index 28daad427e..a40ed4995a 100644 --- a/lib/widgets/action_sheet.dart +++ b/lib/widgets/action_sheet.dart @@ -1,30 +1,50 @@ import 'package:flutter/material.dart'; import 'package:share_plus/share_plus.dart'; +import '../api/exception.dart'; import '../api/model/model.dart'; +import '../api/route/messages.dart'; +import 'compose_box.dart'; +import 'dialog.dart'; import 'draggable_scrollable_modal_bottom_sheet.dart'; +import 'message_list.dart'; +import 'store.dart'; +/// Show a sheet of actions you can take on a message in the message list. +/// +/// Must have a [MessageListPage] ancestor. void showMessageActionSheet({required BuildContext context, required Message message}) { + // The UI that's conditioned on this won't live-update during this appearance + // of the action sheet (we avoid calling composeBoxControllerOf in a build + // method; see its doc). But currently it will be constant through the life of + // any message list, so that's fine. + final isComposeBoxOffered = MessageListPage.composeBoxControllerOf(context) != null; showDraggableScrollableModalBottomSheet( context: context, builder: (BuildContext _) { return Column(children: [ - ShareButton(message: message), + ShareButton(message: message, messageListContext: context), + if (isComposeBoxOffered) QuoteAndReplyButton( + message: message, + messageListContext: context, + ), ]); }); } abstract class MessageActionSheetMenuItemButton extends StatelessWidget { - const MessageActionSheetMenuItemButton({ + MessageActionSheetMenuItemButton({ super.key, required this.message, - }); + required this.messageListContext, + }) : assert(messageListContext.findAncestorWidgetOfExactType() != null); IconData get icon; String get label; void Function(BuildContext) get onPressed; final Message message; + final BuildContext messageListContext; @override Widget build(BuildContext context) { @@ -36,9 +56,10 @@ abstract class MessageActionSheetMenuItemButton extends StatelessWidget { } class ShareButton extends MessageActionSheetMenuItemButton { - const ShareButton({ + ShareButton({ super.key, required super.message, + required super.messageListContext, }); @override get icon => Icons.adaptive.share; @@ -65,3 +86,91 @@ class ShareButton extends MessageActionSheetMenuItemButton { await Share.shareWithResult(message.content); }; } + +class QuoteAndReplyButton extends MessageActionSheetMenuItemButton { + QuoteAndReplyButton({ + super.key, + required super.message, + required super.messageListContext, + }); + + @override get icon => Icons.format_quote_outlined; + + @override get label => 'Quote and reply'; + + @override get onPressed => (BuildContext bottomSheetContext) async { + // Close the message action sheet. We'll show the request progress + // in the compose-box content input with a "[Quoting…]" placeholder. + Navigator.of(bottomSheetContext).pop(); + + // This will be null only if the compose box disappeared after the + // message action sheet opened, and before "Quote and reply" was pressed. + // Currently a compose box can't ever disappear, so this is impossible. + ComposeBoxController composeBoxController = + MessageListPage.composeBoxControllerOf(messageListContext)!; + final topicController = composeBoxController.topicController; + if ( + topicController != null + && topicController.textNormalized == kNoTopicTopic + && message is StreamMessage + ) { + topicController.value = TextEditingValue(text: message.subject); + } + final tag = composeBoxController.contentController + .registerQuoteAndReplyStart(PerAccountStoreWidget.of(messageListContext), + message: message, + ); + + Message? fetchedMessage; + String? errorMessage; + // TODO, supported by reusable code: + // - (?) Retry with backoff on plausibly transient errors. + // - If request(s) take(s) a long time, show snackbar with cancel + // button, like "Still working on quote-and-reply…". + // On final failure or success, auto-dismiss the snackbar. + try { + fetchedMessage = await getMessageCompat(PerAccountStoreWidget.of(messageListContext).connection, + messageId: message.id, + applyMarkdown: false, + ); + if (fetchedMessage == null) { + errorMessage = 'That message does not seem to exist.'; + } + } catch (e) { + switch (e) { + case ZulipApiException(): + errorMessage = e.message; + // TODO specific messages for common errors, like network errors + // (support with reusable code) + default: + errorMessage = 'Could not fetch message source.'; + } + } + + if (!messageListContext.mounted) return; + + if (fetchedMessage == null) { + assert(errorMessage != null); + // TODO(?) give no feedback on error conditions we expect to + // flag centrally in event polling, like invalid auth, + // user/realm deactivated. (Support with reusable code.) + await showErrorDialog(context: messageListContext, + title: 'Quotation failed', message: errorMessage); + } + + if (!messageListContext.mounted) return; + + // This will be null only if the compose box disappeared during the + // quotation request. Currently a compose box can't ever disappear, + // so this is impossible. + composeBoxController = MessageListPage.composeBoxControllerOf(messageListContext)!; + composeBoxController.contentController + .registerQuoteAndReplyEnd(PerAccountStoreWidget.of(messageListContext), tag, + message: message, + rawContent: fetchedMessage?.content, + ); + if (!composeBoxController.contentFocusNode.hasFocus) { + composeBoxController.contentFocusNode.requestFocus(); + } + }; +} diff --git a/test/flutter_checks.dart b/test/flutter_checks.dart index 46cbe75b73..998281e508 100644 --- a/test/flutter_checks.dart +++ b/test/flutter_checks.dart @@ -2,8 +2,14 @@ import 'dart:ui'; import 'package:checks/checks.dart'; +import 'package:flutter/foundation.dart'; import 'package:flutter/painting.dart'; + +extension ValueNotifierChecks on Subject> { + Subject get value => has((c) => c.value, 'value'); +} + extension TextStyleChecks on Subject { Subject get inherit => has((t) => t.inherit, 'inherit'); Subject?> get fontVariations => has((t) => t.fontVariations, 'fontVariations'); diff --git a/test/widgets/action_sheet_test.dart b/test/widgets/action_sheet_test.dart new file mode 100644 index 0000000000..bb3b92a352 --- /dev/null +++ b/test/widgets/action_sheet_test.dart @@ -0,0 +1,223 @@ +import 'package:checks/checks.dart'; +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:zulip/api/model/model.dart'; +import 'package:zulip/api/route/messages.dart'; +import 'package:zulip/model/compose.dart'; +import 'package:zulip/model/narrow.dart'; +import 'package:zulip/model/store.dart'; +import 'package:zulip/widgets/compose_box.dart'; +import 'package:zulip/widgets/content.dart'; +import 'package:zulip/widgets/message_list.dart'; +import 'package:zulip/widgets/store.dart'; +import '../api/fake_api.dart'; + +import '../example_data.dart' as eg; +import '../flutter_checks.dart'; +import '../model/binding.dart'; +import '../model/test_store.dart'; +import 'compose_box_checks.dart'; +import 'dialog_checks.dart'; + +/// Simulates loading a [MessageListPage] and long-pressing on [message]. +Future setupToMessageActionSheet(WidgetTester tester, { + required Message message, + required Narrow narrow, +}) async { + addTearDown(TestDataBinding.instance.reset); + + await TestDataBinding.instance.globalStore.add(eg.selfAccount, eg.initialSnapshot); + final store = await TestDataBinding.instance.globalStore.perAccount(eg.selfAccount.id); + store.addUser(eg.user(userId: message.senderId)); + if (message is StreamMessage) { + store.addStream(eg.stream(streamId: message.streamId)); + } + final connection = store.connection as FakeApiConnection; + + // prepare message list data + connection.prepare(json: GetMessagesResult( + anchor: message.id, + foundNewest: true, + foundOldest: true, + foundAnchor: true, + historyLimited: false, + messages: [message], + ).toJson()); + + await tester.pumpWidget( + MaterialApp( + home: GlobalStoreWidget( + child: PerAccountStoreWidget( + accountId: eg.selfAccount.id, + child: MessageListPage(narrow: narrow))))); + + // global store, per-account store, and message list get loaded + await tester.pumpAndSettle(); + + // request the message action sheet + await tester.longPress(find.byType(MessageContent)); + // sheet appears onscreen; default duration of bottom-sheet enter animation + await tester.pump(const Duration(milliseconds: 250)); +} + +void main() { + TestDataBinding.ensureInitialized(); + + group('QuoteAndReplyButton', () { + ComposeBoxController? findComposeBoxController(WidgetTester tester) { + return tester.widget(find.byType(ComposeBox)) + .controllerKey?.currentState; + } + + Widget? findQuoteAndReplyButton(WidgetTester tester) { + return tester.widgetList(find.byIcon(Icons.format_quote_outlined)).singleOrNull; + } + + void prepareRawContentResponseSuccess(PerAccountStore store, { + required Message message, + required String rawContent, + }) { + // Prepare fetch-raw-Markdown response + // TODO: Message should really only differ from `message` + // in its content / content_type, not in `id` or anything else. + (store.connection as FakeApiConnection).prepare(json: + GetMessageResult(message: eg.streamMessage(contentMarkdown: rawContent)).toJson()); + } + + void prepareRawContentResponseError(PerAccountStore store) { + final fakeResponseJson = { + 'code': 'BAD_REQUEST', + 'msg': 'Invalid message(s)', + 'result': 'error', + }; + (store.connection as FakeApiConnection).prepare(httpStatus: 400, json: fakeResponseJson); + } + + /// Simulates tapping the quote-and-reply button in the message action sheet. + /// + /// Checks that there is a quote-and-reply button. + Future tapQuoteAndReplyButton(WidgetTester tester) async { + final quoteAndReplyButton = findQuoteAndReplyButton(tester); + check(quoteAndReplyButton).isNotNull(); + await tester.tap(find.byWidget(quoteAndReplyButton!)); + } + + void checkLoadingState(PerAccountStore store, ComposeContentController contentController, { + required TextEditingValue valueBefore, + required Message message, + }) { + check(contentController).value.equals((ComposeContentController() + ..value = valueBefore + ..insertPadded(quoteAndReplyPlaceholder(store, message: message)) + ).value); + check(contentController).validationErrors.contains(ContentValidationError.quoteAndReplyInProgress); + } + + void checkSuccessState(PerAccountStore store, ComposeContentController contentController, { + required TextEditingValue valueBefore, + required Message message, + required String rawContent, + }) { + final builder = ComposeContentController() + ..value = valueBefore + ..insertPadded(quoteAndReply(store, message: message, rawContent: rawContent)); + if (!valueBefore.selection.isValid) { + // (At the end of the process, we focus the input, which puts a cursor + // at text's end, if there was no cursor at the time.) + builder.selection = TextSelection.collapsed(offset: builder.text.length); + } + check(contentController).value.equals(builder.value); + check(contentController).not(it()..validationErrors.contains(ContentValidationError.quoteAndReplyInProgress)); + } + + testWidgets('in stream narrow', (WidgetTester tester) async { + final message = eg.streamMessage(); + await setupToMessageActionSheet(tester, message: message, narrow: StreamNarrow(message.streamId)); + final store = await TestDataBinding.instance.globalStore.perAccount(eg.selfAccount.id); + + final composeBoxController = findComposeBoxController(tester)!; + final contentController = composeBoxController.contentController; + + final valueBefore = contentController.value; + prepareRawContentResponseSuccess(store, message: message, rawContent: 'Hello world'); + await tapQuoteAndReplyButton(tester); + checkLoadingState(store, contentController, valueBefore: valueBefore, message: message); + await tester.pump(Duration.zero); // message is fetched; compose box updates + check(composeBoxController.contentFocusNode.hasFocus).isTrue(); + checkSuccessState(store, contentController, + valueBefore: valueBefore, message: message, rawContent: 'Hello world'); + }); + + testWidgets('in topic narrow', (WidgetTester tester) async { + final message = eg.streamMessage(); + await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message)); + final store = await TestDataBinding.instance.globalStore.perAccount(eg.selfAccount.id); + + final composeBoxController = findComposeBoxController(tester)!; + final contentController = composeBoxController.contentController; + + final valueBefore = contentController.value; + prepareRawContentResponseSuccess(store, message: message, rawContent: 'Hello world'); + await tapQuoteAndReplyButton(tester); + checkLoadingState(store, contentController, valueBefore: valueBefore, message: message); + await tester.pump(Duration.zero); // message is fetched; compose box updates + check(composeBoxController.contentFocusNode.hasFocus).isTrue(); + checkSuccessState(store, contentController, + valueBefore: valueBefore, message: message, rawContent: 'Hello world'); + }); + + testWidgets('in DM narrow', (WidgetTester tester) async { + final message = eg.dmMessage(from: eg.selfUser, to: [eg.otherUser]); + await setupToMessageActionSheet(tester, + message: message, narrow: DmNarrow.ofMessage(message, selfUserId: eg.selfUser.userId)); + final store = await TestDataBinding.instance.globalStore.perAccount(eg.selfAccount.id); + + final composeBoxController = findComposeBoxController(tester)!; + final contentController = composeBoxController.contentController; + + final valueBefore = contentController.value; + prepareRawContentResponseSuccess(store, message: message, rawContent: 'Hello world'); + await tapQuoteAndReplyButton(tester); + checkLoadingState(store, contentController, valueBefore: valueBefore, message: message); + await tester.pump(Duration.zero); // message is fetched; compose box updates + check(composeBoxController.contentFocusNode.hasFocus).isTrue(); + checkSuccessState(store, contentController, + valueBefore: valueBefore, message: message, rawContent: 'Hello world'); + }); + + testWidgets('request has an error', (WidgetTester tester) async { + final message = eg.streamMessage(); + await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message)); + final store = await TestDataBinding.instance.globalStore.perAccount(eg.selfAccount.id); + + final composeBoxController = findComposeBoxController(tester)!; + final contentController = composeBoxController.contentController; + + final valueBefore = contentController.value = TextEditingValue.empty; + prepareRawContentResponseError(store); + await tapQuoteAndReplyButton(tester); + checkLoadingState(store, contentController, valueBefore: valueBefore, message: message); + await tester.pump(Duration.zero); // error arrives; error dialog shows + + await tester.tap(find.byWidget(checkErrorDialog(tester, + expectedTitle: 'Quotation failed', + expectedMessage: 'That message does not seem to exist.', + ))); + + check(contentController.value).equals(const TextEditingValue( + // The placeholder was removed. (A newline from the placeholder's + // insertPadded remains; I guess ideally we'd try to prevent that.) + text: '\n', + + // (At the end of the process, we focus the input.) + selection: TextSelection.collapsed(offset: 1), // + )); + }); + + testWidgets('not offered in AllMessagesNarrow (composing to reply is not yet supported)', (WidgetTester tester) async { + final message = eg.streamMessage(); + await setupToMessageActionSheet(tester, message: message, narrow: const AllMessagesNarrow()); + check(findQuoteAndReplyButton(tester)).isNull(); + }); + }); +} diff --git a/test/widgets/compose_box_checks.dart b/test/widgets/compose_box_checks.dart new file mode 100644 index 0000000000..03aabbe8ee --- /dev/null +++ b/test/widgets/compose_box_checks.dart @@ -0,0 +1,16 @@ +import 'package:checks/checks.dart'; +import 'package:zulip/model/autocomplete.dart'; +import 'package:zulip/widgets/compose_box.dart'; + +extension ComposeContentControllerChecks on Subject { + Subject> get validationErrors => has((c) => c.validationErrors, 'validationErrors'); +} + +extension AutocompleteIntentChecks on Subject { + Subject get syntaxStart => has((i) => i.syntaxStart, 'syntaxStart'); + Subject get query => has((i) => i.query, 'query'); +} + +extension UserMentionAutocompleteResultChecks on Subject { + Subject get userId => has((r) => r.userId, 'userId'); +}