action_sheet: Add "Quote and reply" button 🎉 #201

chrisbobbe · 2023-06-17T01:40:42Z

Fixes: #116

chrisbobbe · 2023-06-17T01:41:53Z

lib/model/narrow.dart

+  static SendableNarrow ofMessage(Message message, {required int selfUserId}) {
+    switch (message) {
+      case StreamMessage(:var streamId, :var subject):
+        return TopicNarrow(streamId, subject);
+      case DmMessage(:var displayRecipient):
+        return DmNarrow(
+          allRecipientIds: displayRecipient.map((r) => r.id).toList(),
+          selfUserId: selfUserId,
+        );
+    }
+  }


I wasn't sure if this would be best as a static method on SendableNarrow, or a helper function defined at toplevel, or even a method on Message.

I think here on SendableNarrow is good. In fact I think the most idiomatic thing would be to make it a factory constructor, rather than a static method.

That caused me to wonder how a factory constructor differs anyway from just a static method with return type of the class. Here's a writeup I found helpful:
https://dash-overflow.net/articles/factory/
Nothing that affects this example, I think, but anyway I think a factory constructor is most idiomatic.

I wouldn't want to make it a method on Message because Message is part of the API bindings, and this isn't.

A further improvement would be to add [TopicNarrow.ofMessage] and [DmNarrow.ofMessage], and then this one can just switch on the type of the message and call those.

That way a call site that knows it has a DmMessage can say DmNarrow.ofMessage(message) and know that it'll get a DmNarrow.

Makes sense.

chrisbobbe · 2023-06-17T01:43:25Z

lib/model/compose.dart

+String mention(User user, {bool silent = false}) {
+  return '@${silent ? '_' : ''}**${user.fullName}|${user.userId}**';
+}


Perhaps doesn't need a reusable function defined here, but:

Having tests for this logic can be nice

We'll also be writing mentions for @-mention typeahead/autocomplete #49 / Support silent @-mention autocomplete #129.

chrisbobbe · 2023-06-17T01:44:54Z

lib/widgets/compose_box.dart

@@ -158,8 +159,8 @@ class ComposeContentController extends ComposeController<ContentValidationError>
    value = value.replaced(
      replacementRange,
      url == null
-        ? '[Failed to upload file: $filename]()' // TODO(i18n)
-        : '[$filename](${url.toString()})');
+        ? inlineLink('Failed to upload file: $filename', null) // TODO(i18n)


When I came back to this code, I wondered how helpful this "failure" content is. It'd be just as easy to delete the "loading" placeholder, instead of replacing it with this.

(Same for quote-and-reply.)

(We'll already be showing an error dialog about the failure, I think.)

Yeah, just deleting the placeholder makes sense to me.

chrisbobbe · 2023-06-17T01:46:38Z

lib/model/compose.dart

+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));
+  final senderPart = '${mention(sender!, silent: true)} ${inlineLink('said', url)}: '; // TODO(i18n) ?
+  return '$senderPart*(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);
+  final senderLine = '${mention(sender!, silent: true)} ${inlineLink('said', url)}:\n'; // TODO(i18n) ?
+  return senderLine + wrapWithBacktickFence(content: rawContent, infoString: 'quote');
+}


Could write tests for these, I suppose 😅—but they mostly just use code that already has test coverage.

I think my question above about having the message ID in the link perhaps answers this 🙂.

Because the various pieces that these functions put together already have their own test coverage, these might not need more than a single test case each. But that single test case would provide coverage of how these functions do put those pieces together, like the presence of /near/12345 in the link and the use of silent mentions.

Ah yeah, makes sense. For the "expected" string, I guess we have a choice to make it a more-or-less literal string, or put that together using inlineLink and friends since they're covered already.

I've gone the more-or-less literal string route for my next revision but would happily switch to the other.

chrisbobbe · 2023-06-17T01:50:25Z

lib/widgets/compose_box.dart

+  void _insertPadded(String newText) { // ignore: unused_element
+    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.
+    }
+    final paddingAfter = text.substring(i.start).startsWith('\n') ? '' : '\n';
+    value = value.replaced(i, paddingBefore + newText + paddingAfter);
+  }
+


Would be reasonable to add some tests for this; I can do that if you like.

gnprice

Thanks for building this! Comments below.

gnprice · 2023-06-21T19:17:16Z

test/example_data.dart

+  {User? sender, ZulipStream? inStream, String? topic}) {
+  final effectiveStream = inStream ?? stream();


nit:

Suggested change

{User? sender, ZulipStream? inStream, String? topic}) {

final effectiveStream = inStream ?? stream();

{User? sender, ZulipStream? inStream, String? topic}) {

final effectiveStream = inStream ?? stream();

(i.e. keep the indentation of that parameters line)

When the parameters are immediately juxtaposed with the body like this, it's helpful to give them an extra level of indentation so they don't look like part of the body.

(Similarly when indenting the second of two lines in an if-condition, for example.)

gnprice · 2023-06-21T19:24:04Z

lib/model/narrow.dart

+      case DmMessage(:var displayRecipient):
+        return DmNarrow(
+          allRecipientIds: displayRecipient.map((r) => r.id).toList(),
+          selfUserId: selfUserId,


Yeah, this is a useful helper to have.

When adding the helper, it should be used to replace the existing places where we do the same thing. These can be found among the call sites of the TopicNarrow and DmNarrow constructors.

Seeing one of those reminds me that this can be made to look a bit cleaner by consuming DmMessage.allRecipientIds, rather than displayRecipient.

Makes sense.

These can be found among the call sites of the TopicNarrow and DmNarrow constructors.

I see that a call site of the DmNarrow constructor makes the allRecipientIds list with .toList(growable: false). If DmNarrow is meant to be immutable, then that seems helpful: you don't want that list to get anything added to it.

…I guess you also don't want any of its elements to be replaced or removed, either. Would a list made with List.unmodifiable be better? What if DmNarrow.allRecipientIds were an Iterable instead of a List?

Would a list made with List.unmodifiable be better?

Yeah, that'd be pretty reasonable.

What if DmNarrow.allRecipientIds were an Iterable instead of a List?

I think DmNarrow wants to ask that the allRecipientIds it's passed is a List, because the expectation is that it's all materialized and there's not any work that needs to be done to iterate through them.

It could provide the allRecipientIds property as an Iterable instead of a List, but I'm not sure that's useful enough for the small complication it adds.

gnprice · 2023-06-21T19:34:30Z

lib/model/narrow.dart

+  static SendableNarrow ofMessage(Message message, {required int selfUserId}) {
+    switch (message) {
+      case StreamMessage(:var streamId, :var subject):
+        return TopicNarrow(streamId, subject);
+      case DmMessage(:var displayRecipient):
+        return DmNarrow(
+          allRecipientIds: displayRecipient.map((r) => r.id).toList(),
+          selfUserId: selfUserId,
+        );
+    }
+  }


I think here on SendableNarrow is good. In fact I think the most idiomatic thing would be to make it a factory constructor, rather than a static method.

That caused me to wonder how a factory constructor differs anyway from just a static method with return type of the class. Here's a writeup I found helpful:
https://dash-overflow.net/articles/factory/
Nothing that affects this example, I think, but anyway I think a factory constructor is most idiomatic.

I wouldn't want to make it a method on Message because Message is part of the API bindings, and this isn't.

gnprice · 2023-06-21T19:35:55Z

lib/model/narrow.dart

+  static SendableNarrow ofMessage(Message message, {required int selfUserId}) {
+    switch (message) {
+      case StreamMessage(:var streamId, :var subject):
+        return TopicNarrow(streamId, subject);
+      case DmMessage(:var displayRecipient):
+        return DmNarrow(
+          allRecipientIds: displayRecipient.map((r) => r.id).toList(),
+          selfUserId: selfUserId,
+        );
+    }
+  }


A further improvement would be to add [TopicNarrow.ofMessage] and [DmNarrow.ofMessage], and then this one can just switch on the type of the message and call those.

That way a call site that knows it has a DmMessage can say DmNarrow.ofMessage(message) and know that it'll get a DmNarrow.

gnprice · 2023-06-21T19:41:36Z

test/model/narrow_test.dart

+  group('SendableNarrow', () {
+    test('stream message', () {
+      final stream = eg.stream();
+      final message = eg.streamMessage(inStream: stream);


Hmm, having to call this argument inStream: instead of simply stream: looks pretty weird to me. I think that's going to regularly be confusing when trying to use this helper.

I'm guessing the reason for that name is for the sake of the implementation, where it wants to refer to the outer stream (aka eg.stream) and that would get shadowed.

This name seems confusing enough for callers, though, that I think it'd be worth some hackery at the implementation to avoid it. One solution would be to add a private alias at the global level of the file, which could look like this:

const _stream = stream; StreamMessage streamMessage( {User? sender, ZulipStream? stream, String? topic}) { final effectiveStream = stream ?? _stream();

Ah OK, that's better!

gnprice · 2023-06-21T20:28:30Z

lib/widgets/action_sheet.dart

+    // 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 = MessageListPageState.composeBoxControllerOf(messageListContext);


nit:

Suggested change

ComposeBoxController? composeBoxController = MessageListPageState.composeBoxControllerOf(messageListContext);

ComposeBoxController? composeBoxController =

MessageListPageState.composeBoxControllerOf(messageListContext);

I know we've talked about avoiding "cliffhanger" newlines following Flutter-repo style, but I think this line is too long for that.

Plus as a bonus:

Suggested change

ComposeBoxController? composeBoxController = MessageListPageState.composeBoxControllerOf(messageListContext);

ComposeBoxController composeBoxController = MessageListPageState.composeBoxControllerOf(messageListContext)!;

when the line is a reasonable length, it becomes reasonable to put an important one-character bit of syntax at the end of it. And this seems like a more on-point place for that ! than the next line.

gnprice · 2023-06-21T20:32:29Z

lib/widgets/message_list.dart

+  /// don't call this in a build method.
+  static ComposeBoxController? composeBoxControllerOf(BuildContext context) {
+    final messageListPageState = context.findAncestorStateOfType<MessageListPageState>();
+    assert(messageListPageState != null, 'No MessageListPageState ancestor');


Oh, related to leaving the state private:

Suggested change

assert(messageListPageState != null, 'No MessageListPageState ancestor');

assert(messageListPageState != null, 'No MessageListPage ancestor');

the error message can just stick to the public name, of the widget.

(See for comparison PerAccountStoreWidget.of.)

gnprice · 2023-06-21T20:43:44Z

lib/widgets/action_sheet.dart

+      switch (e) {
+        case ZulipApiException():
+          errorMessage = e.message;
+      }
+    }
+
+    if (fetchedMessage == null && errorMessage == null) {
+      errorMessage = 'Could not fetch message source.';
+    }


Slightly easier to follow, I think, would be:

Suggested change

switch (e) {

case ZulipApiException():

errorMessage = e.message;

}

}

if (fetchedMessage == null && errorMessage == null) {

errorMessage = 'Could not fetch message source.';

}

switch (e) {

case ZulipApiException():

errorMessage = e.message;

default:

errorMessage = 'Could not fetch message source.';

}

}

Can then add an assert(errorMessage != null) inside the if (fetchedMessage == null) case below.

gnprice · 2023-06-21T20:44:05Z

lib/widgets/action_sheet.dart

+      // TODO specific messages for common errors, like network errors
+      //   (support with reusable code)


This comment can probably best go inside the switch in the catch above.

gnprice · 2023-06-21T20:46:58Z

lib/widgets/action_sheet.dart

+
+  @override String get label => 'Quote and reply';
+
+  @override VoidCallback get onPressed => () async {


There's a lot here in this method, and it'd be real nice to have a few tests for it. Would you try writing some?

I think after #30 we may have all the pieces we need in order to be able to write a reasonable test for code like this. If you run into something that makes it hard to test, let's discuss in chat.

Sure! I'm happy to report that I've gotten a reasonable-looking "happy path" test in my next revision. It does these things:

renders a message list with a message and a compose box

simulates a long-press on the message to open the message action sheet

simulates a tap on the "Quote and reply" button

checks the compose-input value before and after the fetch-raw-Markdown request

It's 72 lines of code, which seems like a lot. I'd like to see how we could shape the code for good support of additional tests, such as to exercise when the fetch-raw-Markdown request fails. (And also I guess to check that we don't offer quote-and-reply at all when it can't be supported; i.e., when we don't offer a compose box for the narrow.) Maybe we can go over this in the office today?

chrisbobbe · 2023-06-23T19:30:07Z

Thanks for the review! Revision pushed. In particular, please see #201 (comment) about something we might discuss in the office today. 🙂

…acing We're already showing an error dialog for the failure, so this seems redundant; discussion: zulip#201 (comment)

gnprice

Thanks!

I'll aim to give this revision a full review after the weekend, but here's comments on the handy new insertPadded tests.

(We also talked about the new widget test in the office today as discussed above.)

gnprice · 2023-06-24T00:25:20Z

test/widgets/compose_box_test.dart

+      group('insert at end', () {
+        checkInsertPadded('one empty line; insert one line',


Let's have a convention that when the helper creates its own test cases (calls test), its name is like testFoo, and checkFoo is for helpers one calls inside an existing test case. That will help with correctly keeping all the test computations inside test cases.

gnprice · 2023-06-24T00:27:44Z

test/widgets/compose_box_test.dart

+        checkInsertPadded('one empty line; insert one line',
+          '^\n',     'a\n',    'a\n^\n');
+        checkInsertPadded('two empty lines; insert one line',
+          '^\n\n',   'a\n',    'a\n^\n\n');


Hmm interesting. These two cases don't feel like desired behavior — they mean that you don't end up with a blank line between the inserted text and your cursor.

I think I'd want the expected output here to instead be
a\n\n^\n
a\n\n^\n\n

or else possibly
a\n\n^
a\n\n^\n
.

(This is one of the ways unit tests are useful!)

Similarly some of the cases in the "insert in middle" group.

chrisbobbe · 2023-06-27T19:34:16Z

Thanks for the review! Revision pushed.

gnprice

Thanks! The changes in response to my comments above look good.

Great to see the widget tests too. Here's comments on those. These will be among our first widget tests, and our very first widget tests that really exercise a UI.

gnprice · 2023-06-27T21:06:30Z

test/widgets/action_sheet_test.dart

+      final messageContent = tester.widget<MessageContent>(
+        find.byWidgetPredicate((Widget widget) => widget is MessageContent && widget.message.id == message.id));
+      await tester.longPress(find.byWidget(messageContent));


The widget messageContent is used only for passing back to a find.byWidget. So the original finder could be used directly, instead of going through tester.widget.

Also we've constructed things so there's only one message in sight, so the message-ID check seems redundant. This could therefore simplify to:

Suggested change

final messageContent = tester.widget<MessageContent>(

find.byWidgetPredicate((Widget widget) => widget is MessageContent && widget.message.id == message.id));

await tester.longPress(find.byWidget(messageContent));

await tester.longPress(find.byType(MessageContent));

The widget messageContent is used only for passing back to a find.byWidget. So the original finder could be used directly, instead of going through tester.widget.

I think my intent here was to error (in the tester.widget call) before accidentally passing a finder to tester.longPress that would find multiple widgets, since it's not clear what tester.longPress would naturally do with such a finder.

…Upon reading the code, I've discovered that it throws a descriptive error, so we don't need our own defensive code. 😅

Also we've constructed things so there's only one message in sight, so the message-ID check seems redundant. […]

I think in particular it helps that that construction of the message list (with just one message in sight) is done locally in this same function. So your proposal seems good.

gnprice · 2023-06-27T21:06:32Z

test/widgets/action_sheet_test.dart

+void main() {
+  group('QuoteAndReplyButton', () {
+    TestDataBinding.ensureInitialized();


nit:

Suggested change

void main() {

group('QuoteAndReplyButton', () {

TestDataBinding.ensureInitialized();

void main() {

TestDataBinding.ensureInitialized();

group('QuoteAndReplyButton', () {

Idiomatic to initialize the bindings at the top of main, I think.

gnprice · 2023-06-27T21:07:07Z

test/widgets/action_sheet_test.dart

+    /// Simulates loading a [MessageListPage] and long-pressing on [message].
+    Future<void> setupToMessageActionSheet(WidgetTester tester, {


This can go up at the top level of the file — should be relevant to a wide range of action-sheet tests, beyond this particular button.

gnprice · 2023-06-27T21:08:22Z

test/widgets/action_sheet_test.dart

+      return tester.widgetList<ComposeBox>(find.byType(ComposeBox))
+        .single.controllerKey?.currentState;


As the tester.widgetList doc suggests, simplify to widget when you expect only one:

Suggested change

return tester.widgetList<ComposeBox>(find.byType(ComposeBox))

.single.controllerKey?.currentState;

return tester.widget<ComposeBox>(find.byType(ComposeBox))

.controllerKey?.currentState;

gnprice · 2023-06-27T21:18:15Z

test/widgets/action_sheet_test.dart

+      required Message message,
+      required String rawContent,
+    }) {
+      final c = ComposeContentController()


This one-letter name is a bit cryptic.

Perhaps "expected"?

Yeah; hmm. I guess it's a builder for an expected value, which it provides at .value, rather than anything we want to make expectations on directly. My preference would actually be to not store it in a variable at all, and in an earlier draft I had this:

check(contentController).value.equals( (ComposeContentController() ..value = valueBefore ..insertPadded(quoteAndReply(store, message: message, rawContent: rawContent)) ).value);

But that code doesn't actually produce the TextEditingValue that we want; in particular, it hasn't set its selection to be the last index of the text. To do that, we need to read what that last index is, and I didn't find a way to do that with the .. "cascade notation".

And so (quoting from the current revision):

if (!valueBefore.selection.isValid) { // (At the end of the process, we focus the input, which puts a cursor // at the end if there wasn't a cursor before.) c.selection = TextSelection.collapsed(offset: c.text.length); }

gnprice · 2023-06-27T21:24:32Z

test/widgets/action_sheet_test.dart

+
+      final valueBefore = contentController.value;
+      prepareRawContentResponseSuccess(store, message: message, rawContent: 'Hello world');
+      await tapQuoteAndReplyButton(tester); // should exist; this is a stream narrow


Similarly, can cut this comment:

Suggested change

await tapQuoteAndReplyButton(tester); // should exist; this is a stream narrow

await tapQuoteAndReplyButton(tester);

gnprice · 2023-06-27T21:34:42Z

test/widgets/dialog_checks.dart

+  // TODO: Can we give a more helpful, to-the-point error message than this:
+  //
+  //   The following TestFailure was thrown running a test:
+  //   Expected: a Iterable<AlertDialog> that:
+  //     has length that:
+  //       equals <1>
+  //   Actual: a Iterable<AlertDialog> that:
+  //     has length that:
+  //     Actual: <0>
+  //     Which: are not equal
+  check(alertDialogList).length.equals(1);


Similarly to above, can use tester.widget instead of widgetList, and get the same effect.

The error message will be different from the one quoted here — probably less verbose but not otherwise more or less helpful. I think that's fine. If a test fails for this reason, the stack trace (especially with the name checkErrorDialog) will pretty quickly make it clear that the problem was that there is no error dialog and one was expected.

gnprice · 2023-06-27T22:17:56Z

test/widgets/dialog_checks.dart

+  required String expectedTitle,
+  String? expectedMessage,
+}) {
+  final alertDialogList = tester.widgetList<AlertDialog>(find.byWidgetPredicate((widget) {


I think probably simpler than having a big predicate passed to byWidgetPredicate would be to first find the dialog widget, and then inspect its title and content.

After all, there should be only one AlertDialog at a time — if there's one that matches the test's expectations, but somehow also a second one, then we're happy for that to fail the test too.

That can look like this:

final dialog = tester.widget<AlertDialog>(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))); }

As a bonus, if the matching fails, the stack trace will now identify whether (a) there was no AlertDialog at all, (b) it lacked the expected title, or (c) it lacked the expected message.

gnprice · 2023-06-27T22:19:38Z

test/widgets/dialog_checks.dart

+    matching: find.byWidgetPredicate((widget) {
+      return widget is TextButton && widget.child is Text && (widget.child as Text).data == 'OK';


Suggested change

matching: find.byWidgetPredicate((widget) {

return widget is TextButton && widget.child is Text && (widget.child as Text).data == 'OK';

matching: find.widgetWithText(TextButton, 'OK')));

gnprice · 2023-06-27T22:21:48Z

test/widgets/compose_box_checks.dart

+import 'package:zulip/widgets/compose_box.dart';
+
+extension ComposeContentControllerChecks on Subject<ComposeContentController> {
+  Subject<TextEditingValue> get value => has((c) => c.value, 'value');


This underlying getter is on ChangeNotifier, so it can go in a more general extension in flutter_checks.dart.

ValueNotifier, not ChangeNotifier, I think, but yes: good idea. 🙂

chrisbobbe · 2023-06-27T23:18:43Z

Thanks for the review! Revision pushed, and please see #201 (comment).

gnprice

Thanks! Small comments on the changes in the latest revision compared to the previous; those changes otherwise LGTM.

I still need to give this a full review following the revision last week. I'll see about doing that now.

gnprice · 2023-06-28T21:06:21Z

test/widgets/action_sheet_test.dart

+
+      final valueBefore = contentController.value;
+      prepareRawContentResponseSuccess(store, message: message, rawContent: 'Hello world');
+      await tapQuoteAndReplyButton(tester); // should exist; this is a DM narrow


Suggested change

await tapQuoteAndReplyButton(tester); // should exist; this is a DM narrow

await tapQuoteAndReplyButton(tester);

gnprice · 2023-06-28T21:08:09Z

test/widgets/action_sheet_test.dart

+      required Message message,
+      required String rawContent,
+    }) {
+      final c = ComposeContentController() // a builder for our expected TextEditingValue


Perhaps just call it builder?

chrisbobbe · 2023-06-28T21:55:46Z

Thanks! Revision pushed, with those tweaks.

gnprice

OK! Finished a full review; comments below, all pretty small.

gnprice · 2023-06-28T21:14:26Z

lib/model/compose.dart

+String mention(User user, {bool silent = false}) {
+  return '@${silent ? '_' : ''}**${user.fullName}|${user.userId}**';


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.

gnprice · 2023-06-28T21:21:06Z

test/model/compose_test.dart

+  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…]()');


Let's also have a third case here that demonstrates using a relative URL string — specifically a path-absolute-URL string, since Zulip regularly uses those for things like uploaded images.

gnprice · 2023-06-28T21:25:20Z

lib/model/compose.dart

+  final senderLine = '${mention(sender!, silent: true)} ${inlineLink('said', url)}:\n'; // TODO(i18n) ?
+  return senderLine + wrapWithBacktickFence(content: rawContent, infoString: 'quote');


Can use more interpolation, and rely on string-literal coalescing to keep the pieces nicely arranged on separate lines in the source:

Suggested change

final senderLine = '${mention(sender!, silent: true)} ${inlineLink('said', url)}:\n'; // TODO(i18n) ?

return senderLine + wrapWithBacktickFence(content: rawContent, infoString: 'quote');

return '${mention(sender!, silent: true)} ${inlineLink('said', url)}:\n' // TODO(i18n) ?

'${wrapWithBacktickFence(content: rawContent, infoString: 'quote')}';

Similarly in quoteAndReplyPlaceholder above.

gnprice · 2023-06-28T21:26:25Z

lib/model/compose.dart

+
+/// What we show while fetching the target message's raw Markdown.
+String quoteAndReplyPlaceholder(PerAccountStore store, {
+  required Message message


nit:

Suggested change

required Message message

required Message message,

gnprice · 2023-06-28T21:30:08Z

lib/widgets/action_sheet.dart

  showDraggableScrollableModalBottomSheet(
    context: context,
-    builder: (BuildContext context) {
+    builder: (BuildContext innerContext) {


Suggested change

builder: (BuildContext innerContext) {

builder: (BuildContext _) {

We're no longer using this inner context at all, so a name like _ would help clarify that the reader can ignore it.

That's true as of this later commit:

action_sheet [nfc]: Pull out base class for message action sheet buttons

so I'll plan to make this change there, instead of here in this commit.

Oh, I think maybe you put this comment on that commit I referred to, and GitHub just doesn't make it obvious whether it was meant for that commit or

action_sheet [nfc]: Rename a param to stop shadowing

or even some other commit.

Oh, I think maybe you put this comment on that commit I referred to, and GitHub just doesn't make it obvious whether it was meant for that commit or

As far as GitHub sees, I put the comment on the PR's overall diff. My usual review workflow is that I read the changes commit by commit, in git log --stat -p --reverse, but make comments all on the one main page of the "Files changed" tab on the PR in GitHub.

When I made the comment, I was looking at the "Rename a param to stop shadowing" commit, but also consulted the branch tip's version in the IDE to confirm what the state of things was at the end.

I think possibly an ideal version of this branch would have that "Rename a param to stop shadowing" commit happen later after that "Pull out base class" commit, or not at all. But wherever in the branch you find to cleanly make the change is fine.

gnprice · 2023-06-28T21:35:54Z

lib/widgets/compose_box.dart

+      value = value.replaced(i, paddingBefore + newText);
+      value = value.copyWith(selection: TextSelection.collapsed(offset: value.selection.start + 1));


Suggested change

value = value.replaced(i, paddingBefore + newText);

value = value.copyWith(selection: TextSelection.collapsed(offset: value.selection.start + 1));

value = value.replaced(i, paddingBefore + newText)

.copyWith(selection: TextSelection.collapsed(offset: value.selection.start + 1));

The value setter notifies listeners, so it's cleanest to avoid calling it with a partially-computed intermediate value.

… Hmm I see, but the second line is using value.selection, and wants the value of that after the first update.

Is there a clean way to predict, before making the first update, where we're going to want to place the caret? If not, then calling the setter twice like this is an OK workaround.

gnprice · 2023-06-28T21:46:09Z

test/widgets/compose_box_test.dart

+        testInsertPadded('middle of line',
+          'a^a\n',       'b\n', 'a\n\nb\n\n^a\n');


Let's also have a test case that represents what happens if you promptly insert another item from the output state of this one. I think this covers it:

testInsertPadded('start of non-empty line, after empty line', 'b\n\n^a\n', 'c\n', 'b\n\nc\n\n^a\n');

chrisbobbe · 2023-06-28T22:50:28Z

Thanks! Revision pushed.

…acing We're already showing an error dialog for the failure, so this seems redundant; discussion: zulip#201 (comment)

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.)

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 zulip#116 quote-and-reply, coming up.

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.

We're about to add another button, for quote-and-reply zulip#116.

…w 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.

Fixes: zulip#116

gnprice · 2023-07-01T00:12:04Z

Thanks! All looks good; merging.

chrisbobbe added the a-compose Compose box, autocomplete, attaching files/images label Jun 17, 2023

chrisbobbe requested a review from gnprice June 17, 2023 01:40

chrisbobbe commented Jun 17, 2023

View reviewed changes

gnprice reviewed Jun 21, 2023

View reviewed changes

chrisbobbe force-pushed the pr-quote-and-reply branch from 00dad6a to 10953f1 Compare June 23, 2023 19:30

gnprice reviewed Jun 24, 2023

View reviewed changes

chrisbobbe force-pushed the pr-quote-and-reply branch from 10953f1 to ffc6a82 Compare June 27, 2023 19:34

gnprice reviewed Jun 27, 2023

View reviewed changes

chrisbobbe force-pushed the pr-quote-and-reply branch from ffc6a82 to 9df7ad8 Compare June 27, 2023 23:18

gnprice reviewed Jun 28, 2023

View reviewed changes

chrisbobbe mentioned this pull request Jun 28, 2023

@-mention autocomplete UI #207

Merged

chrisbobbe force-pushed the pr-quote-and-reply branch from 9df7ad8 to f9179ed Compare June 28, 2023 21:55

gnprice reviewed Jun 28, 2023

View reviewed changes

chrisbobbe force-pushed the pr-quote-and-reply branch from f9179ed to 2def4b0 Compare June 28, 2023 22:50

chrisbobbe added 10 commits June 30, 2023 17:11

compose: On failed upload, remove compose placeholder instead of repl…

a316cdb

…acing We're already showing an error dialog for the failure, so this seems redundant; discussion: zulip#201 (comment)

dialog [nfc]: Return Future from showDialog

ece8f72

model [nfc]: Make Message a sealed class

e979bef

test [nfc]: Have eg.streamMessage() take optional ZulipStream and topic

9d66962

narrow [nfc]: Implement {Topic,Stream,Sendable}Narrow.ofMessage factory

654d396

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.)

compose: Implement mention, for mention syntax

637a6c8

compose: Implement quoteAndReply/quoteAndReplyPlaceholder

bb38b4a

action_sheet [nfc]: Rename a param to stop shadowing

432d8c6

msglist [nfc]: Convert MessageListPage to StatefulWidget

046de6d

chrisbobbe added 9 commits June 30, 2023 17:11

compose: Implement [insertPadded] on ComposeContentController

f9f5e79

compose: Add ComposeContentController.registerQuoteAndReply{Start,End}

16950fa

compose: Give MessageListPageState access to topic/content controllers

0276637

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.

action_sheet [nfc]: Pull out base class for message action sheet buttons

1e52dc4

We're about to add another button, for quote-and-reply zulip#116.

test: Have eg.streamMessage and eg.dmMessage accept message content

7e05d7c

compose [nfc]: Make ComposeContentController.insertionIndex non-private

33707b6

widget tests: Add checkErrorDialog

02fd562

action_sheet: Add "Quote and reply" button

406b7d0

Fixes: zulip#116

gnprice force-pushed the pr-quote-and-reply branch from 2def4b0 to 406b7d0 Compare July 1, 2023 00:12

gnprice merged commit 406b7d0 into zulip:main Jul 1, 2023

chrisbobbe deleted the pr-quote-and-reply branch July 1, 2023 00:16

		{User? sender, ZulipStream? inStream, String? topic}) {
		final effectiveStream = inStream ?? stream();

	ComposeBoxController? composeBoxController = MessageListPageState.composeBoxControllerOf(messageListContext);
	ComposeBoxController? composeBoxController =
	MessageListPageState.composeBoxControllerOf(messageListContext);

	ComposeBoxController? composeBoxController = MessageListPageState.composeBoxControllerOf(messageListContext);
	ComposeBoxController composeBoxController = MessageListPageState.composeBoxControllerOf(messageListContext)!;

	assert(messageListPageState != null, 'No MessageListPageState ancestor');
	assert(messageListPageState != null, 'No MessageListPage ancestor');

		// TODO specific messages for common errors, like network errors
		// (support with reusable code)


		@override String get label => 'Quote and reply';

		@override VoidCallback get onPressed => () async {

		group('insert at end', () {
		checkInsertPadded('one empty line; insert one line',

		/// Simulates loading a [MessageListPage] and long-pressing on [message].
		Future<void> setupToMessageActionSheet(WidgetTester tester, {

		return tester.widgetList<ComposeBox>(find.byType(ComposeBox))
		.single.controllerKey?.currentState;

	await tapQuoteAndReplyButton(tester); // should exist; this is a stream narrow
	await tapQuoteAndReplyButton(tester);

		matching: find.byWidgetPredicate((widget) {
		return widget is TextButton && widget.child is Text && (widget.child as Text).data == 'OK';

	matching: find.byWidgetPredicate((widget) {
	return widget is TextButton && widget.child is Text && (widget.child as Text).data == 'OK';
	matching: find.widgetWithText(TextButton, 'OK')));

		String mention(User user, {bool silent = false}) {
		return '@${silent ? '_' : ''}${user.fullName}\|${user.userId}';

		final senderLine = '${mention(sender!, silent: true)} ${inlineLink('said', url)}:\n'; // TODO(i18n) ?
		return senderLine + wrapWithBacktickFence(content: rawContent, infoString: 'quote');

	builder: (BuildContext innerContext) {
	builder: (BuildContext _) {

		value = value.replaced(i, paddingBefore + newText);
		value = value.copyWith(selection: TextSelection.collapsed(offset: value.selection.start + 1));

		testInsertPadded('middle of line',
		'a^a\n', 'b\n', 'a\n\nb\n\n^a\n');

action_sheet: Add "Quote and reply" button 🎉 #201

action_sheet: Add "Quote and reply" button 🎉 #201

Conversation

chrisbobbe commented Jun 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jun 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jun 23, 2023

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jun 27, 2023

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jun 27, 2023

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jun 28, 2023

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jun 28, 2023

gnprice commented Jul 1, 2023

chrisbobbe Jun 17, 2023 •

edited

Loading

chrisbobbe Jun 22, 2023 •

edited

Loading

chrisbobbe Jun 27, 2023 •

edited

Loading

chrisbobbe Jun 27, 2023 •

edited

Loading