msglist: Add "Copy message link" button to action sheet

[sm-sayedi]

A new option is added to the long press action sheet of a message to copy the message link to the clipboard.

Here's an illustration:

Photo	Video

Fixes: #673
Fixes: #732

[Khader-1]

Thank you! Everything looks good, except for a small nit in the comment below.

[Khader-1]

nit: I think both blocks are handling the arrangement steps for the test case so that extra white space feels as an extra

Suggested change

	await setupToMessageActionSheet(tester, message: message, narrow: narrow);
	final store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
	await setupToMessageActionSheet(tester, message: message, narrow: narrow);
	final store = await testBinding.globalStore.perAccount(eg.selfAccount.id);

[sm-sayedi]

Thanks @Khader-1 for the review. Revision pushed. PTAL.

[Khader-1]

Thanks! LGTM.

[alya]

Please be sure to add a still screenshot of visual changes alongside any videos. It is very difficult to review some things on a video (like how the options in the menu look all together, in this case).

[sm-sayedi]

Please be sure to add a still screenshot of visual changes alongside any videos. It is very difficult to review some things on a video (like how the options in the menu look all together, in this case).

@alya An image has been added, sorry for the inconvenience. Please have a look. Next time I should not increase the GIF speed.

[alya]

Thanks! Increased speed may be fine; I'd like to have a still screenshot alongside it regardless, so that it's easy to think about it.

[alya]

I would label the option "Copy link to message" to match the web app, unless we feel like that's too long.

[alya]

It also seems like it should be next to the "Share" option, so maybe we should move "Share" down to be last? We'll probably want to think about the organization more (referring to the web app and the RN app) as we add more options.

[sm-sayedi]

@alya Revision pushed with the changes you suggested. Visuals in the PR comment are also updated. PTAL.

[alya]

Works for me, thanks!

[hackerkid]

@sm-sayedi Thanks for working on this. This looks great :)

2 minor non blocking comments.

1. It was not super obvious what Share did without going through the issue and reading the context. A more explicit version like "Share message text" could be more useful since now it is placed below "Copy link to message" option. I think some users could confuse Share option to "Share link to message".

2. The icon for Copy message text uses the copy icon but the copy link Icon is that of a URL. There is a mismatch in how we uses the icons. In one case we use the icon to highlight the action(copy) but in the second case it is used to highlight the entity(link) to which action is applied. It would be nice to have a uniform design pattern here.

[sm-sayedi]

Thanks @hackerkid for the review!

You're right, there is room for further adjustments. I think @gnprice first suggested replacing "Share" with the "Copy link to message" option in zulip-mobile #2623, but I am not sure if it's still the plan. Regarding the icons, the copy icon was already used for the "Copy message text" option, and for the "Copy link to message" option, I just followed the web in using the link icon. Maybe @terpimost has a suggestion about this!

[alya]

Probably #design is a good place to discuss the icons, for broader visibility. I wouldn't block merging the PR on that, though.

[chrisbobbe]

Thanks, @sm-sayedi! Small comments below.

[chrisbobbe]

action-sheet: Pass the correct `BuildContext` for showing `SnackBar`

Interesting! Thanks for catching this user-facing bug. I've filed #732 for it, so this commit (and the PR description) can point to it.

It looks like context is the context for the action sheet, and at this point the action sheet has been dismissed, so it's a bug to try to do anything with context.

[chrisbobbe]

It would be good to add a regression test for this bug. Reading through test/widgets/clipboard_test.dart, I think this might look something like the following:

--- test/widgets/action_sheet_test.dart
+++ test/widgets/action_sheet_test.dart
@@ -8,6 +8,7 @@ import 'package:flutter_test/flutter_test.dart';
 import 'package:http/http.dart' as http;
 import 'package:zulip/api/model/model.dart';
 import 'package:zulip/api/route/messages.dart';
+import 'package:zulip/model/binding.dart';
 import 'package:zulip/model/compose.dart';
 import 'package:zulip/model/internal_link.dart';
 import 'package:zulip/model/localizations.dart';
@@ -465,6 +466,9 @@ void main() {
     }
 
     testWidgets('success', (WidgetTester tester) async {
+      // for #732 regression check below
+      testBinding.deviceInfoResult = IosDeviceInfo(systemVersion: '16.0');
+
       final message = eg.streamMessage();
       await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message));
       final store = await testBinding.globalStore.perAccount(eg.selfAccount.id);
@@ -473,6 +477,14 @@ void main() {
       await tapCopyMessageTextButton(tester);
       await tester.pump(Duration.zero);
       check(await Clipboard.getData('text/plain')).isNotNull().text.equals('Hello world');
+
+      // Regression check for #732
+      final snackBar = tester.widget<SnackBar>(find.byType(SnackBar));
+      check(snackBar.behavior).equals(SnackBarBehavior.floating);
+      final zulipLocalizations = GlobalLocalizations.zulipLocalizations;
+      tester.widget(find.descendant(matchRoot: true,
+        of: find.byWidget(snackBar.content),
+        matching: find.text(zulipLocalizations.successMessageCopied)));
     });
 
     testWidgets('request has an error', (WidgetTester tester) async {

[chrisbobbe]

Then in the later commit adding "Copy message link", we can have a similar check, but it doesn't need "regression check" comments because it doesn't have a history of being affected by #732. (It's just a nice check to have, since it's easy to get the code wrong, like with the other button.)

[chrisbobbe]

Cool. I think I'd like to rename successMessageCopied to successMessageTextCopied, too.

I think it would also be good to make that UI string more parallel to this existing "Copy message text" button. Instead of "Message Copied" (which has the wrong capitalization anyway), it could say "Message text copied".

Changing the user-facing text won't be NFC, so if it goes in this commit, the commit should be unmarked [nfc]. I don't think I have a preference if that change goes here or in a different commit.

[chrisbobbe]

action-sheet: Move the "Share" option to the bottom of options

Let's also move the 'ShareButton' group in the tests, so the tests still match the order of the code they're testing.

[chrisbobbe]

Let's make a new string in zulipLocalizations, probably with the name successMessageLinkCopied, saying "Message link copied", similar to the "Message text copied" from my previous comment. The successLinkCopied string is meant for a different interaction (long-pressing a link that appears in rendered message content).

[chrisbobbe]

Is there a reason to compute expectedLink before simulating the button tap? If narrowLink had any side effects on the store it's passed, then this wouldn't be quite representative of the interaction we're trying to test. I think narrowLink doesn't have such side effects (and is unlikely to grow any). But if we call it after the test has finished processing the button tap, then it's just something we don't have to think about.

[sm-sayedi]

Thanks @chrisbobbe for the review! Revision pushed! Added one comment below. PTAL.

[sm-sayedi]

This test (and the next test for CopyMessageLinkButton) still passes with the old code where the wrong BuildContext is passed! In the test environment, that context seems to be mounted even Navigator.of(context).pop() is called before.

[chrisbobbe]

Oh interesting; thanks for noticing this. I'm not sure what could be going on there. I'm pretty sure I would have tried this when I suggested the regression test in #713 (comment) . One thing I'm curious about: does #732 still reproduce with the old code when you run the app?

I guess it's possible that some other change happened that ended up fixing it.

…aha! On version 0.0.16 on iOS, #732 actually doesn't reproduce anymore! So I guess that's what happened.

We still want to pass the correct BuildContext, though, since it's reasonable for it to be unmounted after the action sheet is popped, and it would be a bug to do things with it after it's unmounted. But this should go in its own commit, without the Fixes line. An earlier commit can add the regression check, and also leave out the Fixes line, and just say that the bug seems to have been fixed at some point after we discovered it.

[sm-sayedi]

One thing I'm curious about: does #732 still reproduce with the old code when you run the app?

Yeah, the bug is still there when we run the app on a real device (checked on my Android 12).

On version 0.0.16 on iOS, #732 actually doesn't reproduce anymore!

It is reproduced on Android devices with sdkInt <= 32. If we put some logging when testing with a real device, the BuildContext is unmounted, but with tests, it is still mounted. So the bug still exists, it seems that the BuildContext popping mechanism is kind of weird in the testing environment.

[chrisbobbe]

Ah OK; thanks for checking that.

Are you seeing the bug with both the "Copy message text" button and the "Copy link to message" button? One difference is that the former waits for a network request between the .pop() and the copyWithPopup. I wonder if the context is unmounted asynchronously, some duration after the .pop(), and that duration is racing with the network request. (Sometimes one finishes first, sometimes the other.)

I think this might be what's happening. On my iPhone, if I edit the "Copy message text" code to remove the await fetchRawContentWithFeedback (hard-coding rawContent to some string instead), then I replace it with await Future.delayed(Duration(milliseconds: n));, the bug reproduces when n is large (like 1000) but not when n is small (like 10). When it reproduces, context.mounted is false, as you've seen too; when it doesn't reproduce, it's true.

If this seems right to you, then I think the fix is to make the test simulate the network request taking much longer than the unmounting that it's racing against. I'm having trouble getting this to work properly, but it should be possible, and Greg may be able to help if you have trouble with it too.

[gnprice]

Yep, following that hypothesis, here's a (hacky) diff that succeeds in reproducing the issue after git checkout @~~~ -- lib to get the code from before the bugfix, but passes after the bugfix:

diff --git test/api/fake_api.dart test/api/fake_api.dart
index 1cbbffcdc..5e4411133 100644
--- test/api/fake_api.dart
+++ test/api/fake_api.dart
@@ -94,7 +94,7 @@ class FakeHttpClient extends http.BaseClient {
         return Future(() => throw exception);
       case _PreparedSuccess(:var bytes, :var httpStatus):
         final byteStream = http.ByteStream.fromBytes(bytes);
-        return Future(() => http.StreamedResponse(
+        return Future.delayed(Duration(milliseconds: 1000), () => http.StreamedResponse(
           byteStream, httpStatus, request: request));
     }
   }
diff --git test/widgets/action_sheet_test.dart test/widgets/action_sheet_test.dart
index db7852627..b54fa947d 100644
--- test/widgets/action_sheet_test.dart
+++ test/widgets/action_sheet_test.dart
@@ -411,7 +411,9 @@ void main() {
 
       prepareRawContentResponseSuccess(store, message: message, rawContent: 'Hello world');
       await tapCopyMessageTextButton(tester);
-      await tester.pump(Duration.zero);
+      for (int i = 0; i < 11; i++) {
+        await tester.pump(const Duration(milliseconds: 100));
+      }
       check(await Clipboard.getData('text/plain')).isNotNull().text.equals('Hello world');
 
       // regression check for #732

I'll see about wrapping the fake_api.dart side of that into an option you can pass to FakeApiConnection.prepare.

Then @sm-sayedi please experiment and try to simplify those magic numbers of 1000ms, 100ms, 11 iterations — don't need to spend a ton of time doing so, but if we can it'd be nice to reduce it to something that sheds a bit more light on just how this gets triggered.

[gnprice]

OK, I pushed a draft commit to the tip of this PR:
571df4b wip fake_api [nfc]: Add delay option to connection.prepare; TODO test

I'll need to write a test for that test code, and then can send it as a PR. In the meantime the only thing it's missing is such a test, so @sm-sayedi you can build atop that and plan to rebase atop the finished version once it lands.

With that commit in place, a somewhat less hacky diff that reproduces the issue is:

diff --git test/widgets/action_sheet_test.dart test/widgets/action_sheet_test.dart
index db7852627..ea91f632e 100644
--- test/widgets/action_sheet_test.dart
+++ test/widgets/action_sheet_test.dart
@@ -90,7 +90,7 @@ void main() {
     // 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:
+    (store.connection as FakeApiConnection).prepare(delay: Duration(seconds: 1), json:
       GetMessageResult(message: eg.streamMessage(contentMarkdown: rawContent)).toJson());
   }
 
@@ -411,7 +411,9 @@ void main() {
 
       prepareRawContentResponseSuccess(store, message: message, rawContent: 'Hello world');
       await tapCopyMessageTextButton(tester);
-      await tester.pump(Duration.zero);
+      for (int i = 0; i < 11; i++) {
+        await tester.pump(const Duration(milliseconds: 100));
+      }
       check(await Clipboard.getData('text/plain')).isNotNull().text.equals('Hello world');
 
       // regression check for #732

Then the prepareRawContentResponseSuccess should take an optional duration to configure that behavior; I'll leave that for you to do as part of writing the test for the bugfix.

[gnprice]

Sent that delay feature as #770.

[sm-sayedi]

Are you seeing the bug with both the "Copy message text" button and the "Copy link to message" button?

Yeah, for the "Copy link to message" button the bug is present both in manual and widget testing. As you explained in #713 comment, .pop() takes a small amount of time to finish. When testing on my phone, it took around 20 milliseconds for the context to be unmounted.
For the "Copy message text" button the bug is only present in widget testing (which is resolved in the latest revision). In manual testing, await fetchRawContentWithFeedback gives the BuildContext enough time to be unmounted.

[chrisbobbe]

Thanks! I replied above (#713 (comment) ) with thoughts on #732, otherwise LGTM.

[chrisbobbe]

Marking for Greg's review, as I think there may be a way forward regarding #732, see: #713 (comment)

[gnprice]

Thanks @sm-sayedi, and thanks everyone for the previous reviews! This code all looks great modulo one small comment below.

We'll also want to resolve the subthread above, to get a working test for that bug fix. The hypothesis at #713 (comment) makes sense to me. I'll see if I can quick whip up a way to make a FakeApiConnection request take a long (simulated) time to complete.

[gnprice]

Let's split this into its own test case (its own separate testWidgets call).

That will duplicate a few lines from this test case, but that's OK. It'll help make both test cases easier to read and to understand what they're saying, by telling one story at a time instead of telling two stories at once.

[sm-sayedi]

Thanks @chrisbobbe and @gnprice for the reviews. Revision pushed. PTAL.

One comment in the sub-thread above and a few below.

[sm-sayedi]

I wasn't able to make this test not pass when the wrong context was passed. The approach that you suggested for the "Copy message text" button doesn't quite apply here as we don't await for any operation as we do in "Copy message text" (await fetchRawContentWithFeedback).

[gnprice]

Yeah, I think the lack of such an await means that if this "Copy message link" button were to pass the wrong context to copyWithPopup, it would be only a latent bug and not a live one.

That is, the outward behavior would still be completely correct, and it'd be a bug only in the sense that the code would be misleading and would be susceptible to starting to do the wrong thing in the future as a result of otherwise-innocent changes. Like introducing a step with an await.

It'd still be good to have a test that would catch such a breakage. After all, the main purpose of a test is precisely to catch when an innocent-seeming change accidentally breaks something.

But I don't see a way to write such a test that I'd actually expect to successfully break in that case. It'd involve somehow causing any futures that the button's onPress method awaited to take a long (simulated) time to complete, while the navigation animation pressed on ahead… and I don't see a way to target those delays like that, given that there isn't currently anything it actually does await on and there's no way to predict what (if anything) it might someday in the future grow a need to await on.

So it's fine to not have that form of test for this new button.

[sm-sayedi]

Suggested change

	for (int i = 0; i < 5; i++) {
	await tester.pump(const Duration(milliseconds: 100));
	}
	await tester.pump(const Duration(milliseconds: 500));

I wonder why this line doesn't behave the same!!

[gnprice]

The first version will advance the simulated clock by 100ms, then render a frame; then advance another 100ms, then render another frame; and so on 5 times.

The second version will advance the simulated clock by 500ms, and then render a frame.

So the first version renders four frames before the 500ms mark, in addition to one frame at it. Each of those frames will rebuild widgets that need rebuilding, which may add and remove things from the tree. If one of those frames causes the action sheet's context to get removed from the tree, then that'd do it.

[sm-sayedi]

Just rebased this PR on top of #777 because of the build errors mentioned in #771, so the first two commits are from #777.

[gnprice]

Thanks @sm-sayedi for the revision! Looks good; merging, with two adjustments:

As described below, one of these test cases can be left out. I revised the commit to do that.

I also added a new commit with some small clarifications:
aeb6a94 action-sheet test: Simplify and explain a bit more the #732 repro test

including clearly labeling that whole test case as a regression test for #732, and taking material from #713 (comment) . Please take a look at those changes.

[gnprice]

This check is redundant with the same check in the test case above it. Best to leave it out, so that each test is focused on one question and it's therefore easier to understand what the test is about.

[gnprice]

Given the discussion above at #713 (comment) , I think there isn't really anything this test case is testing that isn't covered by the tests for copyWithPopup.

In the regression test above, we're testing that the button calls copyWithPopup in a right way, rather than a wrong way. But this test currently doesn't really have a way to distinguish a wrong vs. a right way of calling that function. So at most it's testing that we do call copyWithPopup. I think that isn't worth the cost of having this test here, and having to reread and try to understand it when changing things. (Possibly it could be worth the cost if we found a really concise, simple way of expressing this test.) So let's just leave this test case out.