Touchable links in messages #204

gnprice · 2023-06-21T22:16:07Z

Fixes #71.
Stacked atop #203.

This is a draft. The branch needs some cleanup, but more fundamentally:

I want to write some tests for this code, because the content parsing in particular gains some complexity of a kind it didn't need before. That turns out to be a bit of a yak shave.
The way I handle making the links respond both to a tap (to follow the link) and a long-press (to copy the URL) feels like a bit of a hack. I may make some further attempts to clean it up, and should probably also find or start an upstream Flutter discussion of how to do this (i.e. how to have a TextSpan respond to two different gestures). I might end up separating the long-press into a separate PR from the main functionality of tapping to follow the link.

But from a user perspective, this version already works nicely. And between the two points above, plus just iterating on refactorings to get this code to be as clean as it is (at the end of the branch) in this version, it's been a bit over a week since I sat down and wrote up a version that works nicely from a user perspective. So I figured it'd be good to share this branch as a demo, and a preview of what the resulting code will look like.

I also have a draft branch that does much of the yak shave to be able to write tests for content parsing. I might send a PR first that lays that groundwork, and also adds tests for some of the existing content-parsing code, and then return to this after that.

chrisbobbe

Thanks, this direction looks good! One small comment below.

chrisbobbe · 2023-06-23T20:38:45Z

lib/widgets/content.dart

+  try {
+    url = store.account.realmUrl.resolve(urlString);
+  } on FormatException {
+    debugPrint('link in content failed to parse: $urlString'); // TODO(log)


Would also be good to give user feedback here, I think, and perhaps also in the case (below this) where parsing succeeds but launching fails for whatever reason.

Sure, good thought.

Thanks for the review!

gnprice · 2023-07-03T20:33:17Z

Just pushed an updated version. Still a draft, but:

The branch is substantially cleaned up; this has basically the sequence of changes I'd want to merge, modulo the things that remain to change about the code.
- The last two commits remain a WIP demo, as they indicate. The one before that adds long-press support, which I expect to end up leaving for a follow-up issue as foreshadowed above. The 7 commits before that are the sequence of changes I'd want to merge.
Rebased atop the changes merged in several groundwork PRs, some of which changes previously appeared in this PR:

The main remaining yak to shave is testing. This requires some sort of mock for the part where we go and invoke url_launcher to try to open the URL the link points to. I'll probably take the approach recommended upstream here:
https://docs.flutter.dev/packages-and-plugins/plugins-in-tests
by wrapping url_launcher. Specifically I'll probably put that in a "binding" class akin to DataBinding (if not shoehorned into DataBinding itself).

Also open is giving the user feedback when parsing or launching fails (as discussed above at #204 (comment) ), and finding or filing an upstream issue about getting the normal open-a-browser interaction on Android, where the system gives the user a choice of browser.

chrisbobbe

Thanks, looking forward to this!

chrisbobbe · 2023-07-03T22:39:28Z

lib/widgets/content.dart

+      child: InlineContent(
+        // If an @-mention is inside a link, let the @-mention override it.
+        recognizer: null,  // TODO make @-mentions tappable, for info on user
+        // An @-mention can't have an embedded link, can it?  // TODO fact-check that.


// An @-mention can't have an embedded link, can it? // TODO fact-check that.

Wouldn't this require a user's full_name to contain Markdown inline-link syntax? I'm not sure if servers would parse such syntax in an @-mention, but if they do, I think we shouldn't give it an open-URL handler. An @-mention span shouldn't have a behavior where 0.0001% of the time it takes you to some website of a user's choosing, instead of opening the user-profile view in the app.

gnprice · 2023-07-04T00:54:17Z

OK, and here it is with tests. Came out pretty nicely, I think, using the "binding" pattern.

chrisbobbe

Nice!

chrisbobbe · 2023-07-05T21:09:26Z

lib/widgets/message_list.dart

@@ -172,12 +171,13 @@ class _MessageListState extends State<MessageList> {
      // TODO: Offer `ScrollViewKeyboardDismissBehavior.interactive` (or
      //   similar) if that is ever offered:
      //     https://github.com/flutter/flutter/issues/57609#issuecomment-1355340849
-      keyboardDismissBehavior: Platform.isIOS
+      keyboardDismissBehavior: switch (Theme.of(context).platform) {


platform: Use Flutter "target" platform, not actual Platform

Neat!

chrisbobbe · 2023-07-05T21:23:12Z

test/model/binding.dart

+
+  /// The value that `ZulipBinding.instance.launchUrl()` should return.
+  ///
+  /// See also [takeLaunchUrlCalls].
+  bool launchUrlResult = true;
+
+  /// Consume the log of calls made to `ZulipBinding.instance.launchUrl()`.
+  ///
+  /// This returns a list of the arguments to all calls made
+  /// to `ZulipBinding.instance.launchUrl()` since the last call to
+  /// either this method or [reset].
+  ///
+  /// See also [launchUrlResult].
+  List<({Uri url, url_launcher.LaunchMode mode})> takeLaunchUrlCalls() {
+    final result = _launchUrlCalls;
+    _launchUrlCalls = null;
+    return result ?? [];
+  }
+  List<({Uri url, url_launcher.LaunchMode mode})>? _launchUrlCalls;
+
+  @override
+  Future<bool> launchUrl(
+    Uri url, {
+    url_launcher.LaunchMode mode = url_launcher.LaunchMode.platformDefault,
+  }) async {
+    (_launchUrlCalls ??= []).add((url: url, mode: mode));
+    return launchUrlResult;
+  }


Cool! One obstacle that's been in the way of testing the compose box's upload-and-attach buttons is that they use Flutter plugins. This pattern seems helpful for addressing that, too.

Yeah, definitely.

If we did end up splitting the bindings into several mixin classes for code organization, I'm imagining the split would look like

data

notifications

miscellaneous plugins that have one or two relevant methods each, where the plugins behind those compose-box buttons would be a prime example (and url_launcher is another prime example).

gnprice · 2023-07-06T00:51:47Z

OK, now ready! PTAL.

As anticipated, I dropped the last few commits:
66973f1 wip long-press on links too, to copy; TODO discuss upstream
d53d775 WIP pattern-match on BlockContentNode; hold off until IDE stops missing references
58ab2f6 Revert "WIP pattern-match on BlockContentNode; hold off until IDE stops missing references", mostly

chrisbobbe · 2023-07-06T19:15:16Z

Thanks, this looks good!

Here are two things I noticed in testing. Not that they need to block merging though, but what do you think:

If you have a short paragraph ending in a link, the space between the link and the end margin is also tappable. For example, if I added this check in the 'multiple links in paragraph' test, it would pass:

      await tester.tapAt(base.translate(15*fontSize, 0)); // "foo bar baz"    X
      check(TestZulipBinding.instance.takeLaunchUrlCalls())
        .single.equals((url: Uri.parse('https://b/'), mode: expectedModeAndroid));

When you have linkified text and a corresponding image preview below it, like you get with an image-upload interaction, tapping the image opens the lightbox, but tapping the text instead calls launchUrl. I think that parallels the web app, actually, but I noticed it as a difference with zulip-mobile, which looks for clues in the a element that you might want to open the lightbox. (Specifically, it checks the href against a small handful of filename extensions, like jpg.)

gnprice · 2023-07-06T19:48:16Z

Thanks for the review! I'll merge.

If you have a short paragraph ending in a link, the space between the link and the end margin is also tappable.

Hmm, wacky.

Please go ahead and file that as a bug after this is merged.

A variation on that scenario would be when the paragraph wraps over multiple lines, and the last line is short and ends with a link. I'd guess the same bug would reproduce there. It sounds like the last TextSpan's recognizer is inappropriately being applied through the end of the line when the TextSpan visually doesn't extend that far, so this may be an upstream Flutter bug.

When you have linkified text and a corresponding image preview below it, like you get with an image-upload interaction, tapping the image opens the lightbox, but tapping the text instead calls launchUrl. I think that parallels the web app, actually, but I noticed it as a difference with zulip-mobile, which looks for clues in the a element that you might want to open the lightbox. (Specifically, it checks the href against a small handful of filename extensions, like jpg.)

Interesting.

That zulip-mobile behavior seems nicer for the user, when it works. That logic is pretty bad, though; for example it would match image-looking URLs on external sites (for which the lightbox seems inappropriate), and conversely it's not clear where the list of extensions comes from and whether it might miss some perfectly good uploaded images for which the server cheerfully embeds a preview and the lightbox would work fine.

I think that can be a follow-up issue too.

This makes it possible to test this sort of logic. See docs for [defaultTargetPlatform]: https://api.flutter.dev/flutter/foundation/defaultTargetPlatform.html

We'll need some more bindings in this spirit. It's not clear there'll be enough of them that we need to split them into mixins across multiple files, like Flutter upstream does, so we'll try just putting them all in a single bindings class. Start by giving that one class a more generic name to fit.

This will give us a convenient way to manage "context" information down through the recursion.

This doesn't yet do much, but will.

At this stage the recognizer is always null and so this doesn't do anything. But it provides us the infrastructure with which to start recognizing taps on links, zulip#71.

Fixes: zulip#71

This is a lot better than the default, though still not ideal. Comments describe a couple of upstream bugs that we should file or comment on.

chrisbobbe · 2023-07-07T20:47:23Z

Makes sense; I've just filed those issues:

chrisbobbe reviewed Jun 23, 2023

View reviewed changes

This was referenced Jun 29, 2023

Record links when parsing content #208

Merged

Refactor content widgets toward touchable links #209

Merged

gnprice force-pushed the pr-links branch from 3544b28 to a16fdd1 Compare July 3, 2023 20:24

chrisbobbe reviewed Jul 3, 2023

View reviewed changes

gnprice force-pushed the pr-links branch from a16fdd1 to fbe5230 Compare July 4, 2023 00:53

chrisbobbe reviewed Jul 5, 2023

View reviewed changes

chrisbobbe mentioned this pull request Jul 5, 2023

Add "Copy to clipboard" button to message action sheet #213

Merged

gnprice force-pushed the pr-links branch from fbe5230 to 622c7f9 Compare July 6, 2023 00:50

gnprice marked this pull request as ready for review July 6, 2023 00:51

gnprice added 10 commits July 6, 2023 12:48

platform: Use Flutter "target" platform, not actual Platform

f669f16

This makes it possible to test this sort of logic. See docs for [defaultTargetPlatform]: https://api.flutter.dev/flutter/foundation/defaultTargetPlatform.html

deps: Add url_launcher

db44a9b

binding: Wrap url_launcher's launchUrl

491a249

content [nfc]: Add _InlineContentBuilder around building an inline span

93d5abf

This will give us a convenient way to manage "context" information down through the recursion.

content [nfc]: Pull out build methods for link, strong, emph

8f3f0d6

content [nfc]: Add _BlockInlineContainer widget, as a stub

91e202a

This doesn't yet do much, but will.

content [nfc]: Track recognizer on inline tree, apply to text spans

f869088

At this stage the recognizer is always null and so this doesn't do anything. But it provides us the infrastructure with which to start recognizing taps on links, zulip#71.

content: Make links touchable

c365525

Fixes: zulip#71

content: Attempt to open external app for links on Android

71ec0cc

This is a lot better than the default, though still not ideal. Comments describe a couple of upstream bugs that we should file or comment on.

gnprice force-pushed the pr-links branch from 622c7f9 to 71ec0cc Compare July 6, 2023 19:48

gnprice merged commit 71ec0cc into zulip:main Jul 6, 2023

gnprice deleted the pr-links branch July 6, 2023 19:49

gnprice mentioned this pull request Jul 7, 2023

Tappable links' touch target can extend through whitespace #214

Closed

chrisbobbe mentioned this pull request Jul 7, 2023

Linkified text above image preview should open the lightbox, like the image preview itself #215

Open

gnprice mentioned this pull request Sep 2, 2023

profile: Implement profile screen for users #287

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Touchable links in messages #204

Touchable links in messages #204

gnprice commented Jun 21, 2023

chrisbobbe left a comment

chrisbobbe Jun 23, 2023

gnprice Jun 24, 2023

gnprice commented Jul 3, 2023

chrisbobbe left a comment

chrisbobbe Jul 3, 2023

gnprice commented Jul 4, 2023

chrisbobbe left a comment

chrisbobbe Jul 5, 2023

chrisbobbe Jul 5, 2023

gnprice Jul 5, 2023

gnprice commented Jul 6, 2023

chrisbobbe commented Jul 6, 2023

gnprice commented Jul 6, 2023

chrisbobbe commented Jul 7, 2023

Touchable links in messages #204

Touchable links in messages #204

Conversation

gnprice commented Jun 21, 2023

chrisbobbe left a comment

Choose a reason for hiding this comment

chrisbobbe Jun 23, 2023

Choose a reason for hiding this comment

gnprice Jun 24, 2023

Choose a reason for hiding this comment

gnprice commented Jul 3, 2023

chrisbobbe left a comment

Choose a reason for hiding this comment

chrisbobbe Jul 3, 2023

Choose a reason for hiding this comment

gnprice commented Jul 4, 2023

chrisbobbe left a comment

Choose a reason for hiding this comment

chrisbobbe Jul 5, 2023

Choose a reason for hiding this comment

chrisbobbe Jul 5, 2023

Choose a reason for hiding this comment

gnprice Jul 5, 2023

Choose a reason for hiding this comment

gnprice commented Jul 6, 2023

chrisbobbe commented Jul 6, 2023

gnprice commented Jul 6, 2023

chrisbobbe commented Jul 7, 2023