-
Notifications
You must be signed in to change notification settings - Fork 306
action_sheet: Add "Quote and reply" button 🎉 #201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
a316cdb
compose: On failed upload, remove compose placeholder instead of repl…
chrisbobbe ece8f72
dialog [nfc]: Return Future from `showDialog`
chrisbobbe e979bef
model [nfc]: Make Message a sealed class
chrisbobbe 9d66962
test [nfc]: Have eg.streamMessage() take optional ZulipStream and topic
chrisbobbe 654d396
narrow [nfc]: Implement {Topic,Stream,Sendable}Narrow.ofMessage factory
chrisbobbe 637a6c8
compose: Implement `mention`, for mention syntax
chrisbobbe 763f1f9
compose: Implement `inlineLink`, for Markdown "inline link" syntax
chrisbobbe bb38b4a
compose: Implement quoteAndReply/quoteAndReplyPlaceholder
chrisbobbe 432d8c6
action_sheet [nfc]: Rename a param to stop shadowing
chrisbobbe 046de6d
msglist [nfc]: Convert MessageListPage to StatefulWidget
chrisbobbe f9f5e79
compose: Implement [insertPadded] on ComposeContentController
chrisbobbe 16950fa
compose: Add ComposeContentController.registerQuoteAndReply{Start,End}
chrisbobbe 0276637
compose: Give MessageListPageState access to topic/content controllers
chrisbobbe 1e52dc4
action_sheet [nfc]: Pull out base class for message action sheet buttons
chrisbobbe 90be3e6
api tests: Have FakeHttpClient.send enqueue response handling in a ne…
chrisbobbe 7e05d7c
test: Have eg.streamMessage and eg.dmMessage accept message content
chrisbobbe 33707b6
compose [nfc]: Make ComposeContentController.insertionIndex non-private
chrisbobbe 02fd562
widget tests: Add `checkErrorDialog`
chrisbobbe 406b7d0
action_sheet: Add "Quote and reply" button
chrisbobbe File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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,65 @@ 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}**'; | ||
} | ||
Comment on lines
+182
to
+184
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps doesn't need a reusable function defined here, but:
|
||
|
||
/// 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() ?? ''})'; | ||
} | ||
|
||
/// 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')}'; | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,36 +1,176 @@ | ||
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 context) { | ||
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(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); | ||
}, | ||
child: const Text('Share')), | ||
ShareButton(message: message, messageListContext: context), | ||
if (isComposeBoxOffered) QuoteAndReplyButton( | ||
message: message, | ||
messageListContext: context, | ||
), | ||
]); | ||
}); | ||
} | ||
|
||
abstract class MessageActionSheetMenuItemButton extends StatelessWidget { | ||
MessageActionSheetMenuItemButton({ | ||
super.key, | ||
required this.message, | ||
required this.messageListContext, | ||
}) : assert(messageListContext.findAncestorWidgetOfExactType<MessageListPage>() != null); | ||
|
||
IconData get icon; | ||
String get label; | ||
void Function(BuildContext) get onPressed; | ||
|
||
final Message message; | ||
final BuildContext messageListContext; | ||
|
||
@override | ||
Widget build(BuildContext context) { | ||
return MenuItemButton( | ||
leadingIcon: Icon(icon), | ||
onPressed: () => onPressed(context), | ||
child: Text(label)); | ||
} | ||
} | ||
|
||
class ShareButton extends MessageActionSheetMenuItemButton { | ||
ShareButton({ | ||
super.key, | ||
required super.message, | ||
required super.messageListContext, | ||
}); | ||
|
||
@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); | ||
}; | ||
} | ||
|
||
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(); | ||
} | ||
}; | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Web uses just the name, no ID, when that wouldn't be ambiguous. That looks a bit nicer while the user's still composing and looking at the source, so it'd be nice to do.
Fine to leave that as a TODO, though.