Skip to content

msglist: Make links touchable, opening in browser #71

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

Closed
gnprice opened this issue Apr 19, 2023 · 0 comments · Fixed by #204
Closed

msglist: Make links touchable, opening in browser #71

gnprice opened this issue Apr 19, 2023 · 0 comments · Fixed by #204
Assignees
Labels
a-content Parsing and rendering Zulip HTML content, notably message contents a-msglist The message-list screen, except what's label:a-content
Milestone

Comments

@gnprice
Copy link
Member

gnprice commented Apr 19, 2023

Many Zulip messages have links in them. We should recognize when the user taps such a link, and follow the link for them.

For this issue, it's OK not to try to recognize links that are internal to the Zulip realm (that'll be #73); we can just open all links as generic URLs, in a browser.

The tricky parts here are about managing the GestureRecognizer objects for recognizing taps on the links.

First, there's a quirk in Flutter's API for using a gesture recognizer on text spans: see flutter/flutter#10623 and in particular flutter/flutter#10623 (comment) . I have a draft branch from back in December that deals with that; the key lines are this in _buildInlineNode:

   if (node is TextNode) {
-    return TextSpan(text: node.text);
+    return TextSpan(text: node.text, recognizer: recognizer);

and then recognizer is a parameter passed down recursively by _buildInlineNode and everything else that builds InlineSpans out of other InlineSpans.

Second, there's the question of where those recognizers should live as state. In the draft branch, we render a LinkNode with a StatefulWidget, which creates a recognizer in initState. That was good enough for a proof-of-concept of solving the first issue, but isn't the layout we want. Here's my notes from there on the way forward:

  } else if (node is LinkNode) {
    // TODO this shouldn't be a widget -- should participate in paragraph layout
    //   Likely this means: enclosing BlockContentNodeWidget becomes stateful;
    //   on initState, it walks the inline nodes to find links, and then creates
    //   appropriate recognizers; a data structure of those gets passed down
    //   through _buildInlineNode; when reaching a link here at this spot,
    //   we look up the appropriate recognizer.
    //
    //   We'd still have _buildInlineNode argument for the individual
    //   actually-applicable recognizer, in addition to the registry of them for
    //   the paragraph.
    //
    //   Perhaps instead of BlockContentNodeWidget being the home of the
    //   recognizers, it's its appropriate children/descendants which contain
    //   lists of inline nodes directly: so Paragraph, ListItem, Heading, etc.
    //   Then put the relevant logic in a mixin so those can all reuse it.
    return WidgetSpan(child: Link(node: node));

Better yet, walking the inline nodes to find links should happen within parseContent. From brief discussion later in chat:

We will later need to add some kind of context object that gets threaded through the recursion when parsing message content.

We'll […] need that in order to accumulate on a block node a list of the inline links inside it, in order to make links clickable (because the gesture handler needs to live on a widget).

@gnprice gnprice added this to the Alpha milestone May 27, 2023
@gnprice gnprice added a-msglist The message-list screen, except what's label:a-content a-content Parsing and rendering Zulip HTML content, notably message contents and removed m-alpha labels May 27, 2023
@gnprice gnprice self-assigned this Jun 14, 2023
gnprice added a commit that referenced this issue Jun 27, 2023
This abstraction doesn't pull a lot of weight yet, but it will
when we start tracking LinkNode descendants, for #71.
gnprice added a commit to gnprice/zulip-flutter that referenced this issue Jun 29, 2023
At this stage we don't yet *do* anything with this information,
like open the linked URL when the user touches the link (zulip#71).
That will require some more infrastructure, which we build next.
gnprice added a commit to gnprice/zulip-flutter that referenced this issue Jun 29, 2023
This gives us the information that the corresponding widgets will
need at build time in order to create appropriate gesture recognizers
for touching the links, as part of implementing zulip#71.
gnprice added a commit to gnprice/zulip-flutter that referenced this issue Jul 6, 2023
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.
gnprice added a commit to gnprice/zulip-flutter that referenced this issue Jul 6, 2023
gnprice added a commit to gnprice/zulip-flutter that referenced this issue Jul 6, 2023
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.
@gnprice gnprice closed this as completed in c365525 Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-content Parsing and rendering Zulip HTML content, notably message contents a-msglist The message-list screen, except what's label:a-content
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

1 participant