MessageBox: Extract in-line message links from the message content and show them as footlinks

[preetmishra]

The initial commits extract in-line message links from the message content and add them below it to improve the reading experience.

The commits are small and incremental in order to make reviewing manageable. Though, they could be squashed before merging.

The last commit adds in-line message links to MsgInfoView (needs discussion; see in-line comment).

Fixes #618 and #452 partially.

[neiljp]

@preetmishra I've left some feedback, but once we have the data from the message, we can try different approaches. I'm personally in favo[u]r of including the links in the messages, but we can also have it in other places, much like we show details of reactions in multiple places.

If we're concerned about knowing reactions are present etc, we could show eg. the total number of reactions and number of each type - that'll also help reconcile with how they're shown in the messages?

[preetmishra]

@neiljp Thanks for the feedback. I have incorporated your suggestions in the latest update.

[neiljp]

@preetmishra I think this will greatly improve the legibility of messages with links in, so I'm really looking forward to merging this 👍

Regarding commit structure, while in the bigger picture I know we're going to show links in a different place, reading just the first commit it feels strange that you remove message details in that one commit (and only add it back in later in another). From a practical point of view, that makes it much more critical (functionality-wise) were I to cherry-pick commits, in terms of where I would split them. From that perspective it would be clearer if the underlying functionality were adjusted & tested first, and then a migration of available higher levels move towards using that functionality, ultimately in the view in one commit. Does that make sense? Additional functionality (extra data in MsgInfoView) could be a separate commit, since that's not a 'migration' (pending the ellipsis wrap style I mention below, perhaps)

For the footlinks, other than my mention of indenting, other ideas are:

• Would it be less 'intense' and provide more contrast, if the [1] and 1: were in the footlink style, and the text and link were in the link style?
• Do we gain anything from having long links 'wrap', at least in the footlink (compared to the MsgInvoView); should we just automatically trim it with an ellipsis in narrow windows (via urwid Text)? If so a FAQ entry would be helpful. Pasting a link from a wrapped line is painful and in narrow windows I'm not going to paste 2, 3, 4+ lines of text to reform it into one long URL. This would reduce the wrapped text to just show one line, and emphasize that we have a list of references, rather than a complicated list of wrapped links - which is what it looks like now on narrow screens!)

The MsgInfoView change is not great right now. Particularly if we do the second bullet point above (which would remove links), then we definitely need the link itself. Did you try a layout like the following? The link lines would be full-width - maybe requiring a bit of a hack in the PopUpView category sense, but for now how does this look?

1: some link text
http://here/is/the/link/itself
2: file.png
http://some/link/on/this/server/file.png
3: google.com
http://google.com

The last two are maybe what you were asking in a point that I tried to answer here, ie. to use the 'last' part of a url - was that what you had in mind?

[neiljp]

Another potentially straightforward change would be to simplify/compact any duplicate links, so that we don't end up with

1: google.com
2: duckduckgo.com
3: google.com

but rather just the first two and a re-use of [1] in the text in place of [3].

[preetmishra]

@neiljp Thanks for the comprehensive and thorough review.

I have reworked the commit structure. The first commit should only have the migration that is required from the previous view. Next, the subsequent commits should only have higher-level functionalities incrementally. The 4th and the 5th commit could be squashed together (I have split them for review).

Further, I have:

• Wrapped links in the footlinks with ellipses to save screen state.
• Handled duplicate links - MessageBox: Extract in-line message links from the message content and show them as footlinks #703 (comment).
• Styled link indexes differently than link/text.
• Trimmed link text to only show the last segment of the link if the text is redundant.
• Modified/hacked PopUpView to show message links in MsgInfoView properly.
• Used the suggested style in MessageBox: Extract in-line message links from the message content and show them as footlinks #703 (review) for MsgInfoView.

[neiljp]

@preetmishra I've re-reviewed the first 3 commits, since they should be merge-able on their own more easily, given what we discussed with regard to #695 on czo. With some minor adjustments I'd love to get those commits into 0.5.2.

[neiljp]

@preetmishra I'm using this now, and I actually wonder if we should avoid showing links in the footlinks if they're just eg. example.com with no text - we don't gain anything? The user doesn't gain any information other than perhaps continuity of numbering - seeing example.com twice is actually more text than we had before! It also exposes how we're implementing the auto link/text association, after a message author just wrote what looks like a link with no description. They could still be shown in other contexts (popups).

[preetmishra]

Updated with the comment in #703 (comment).

[preetmishra]

Strange! I pushed the commits successfully when GitHub was down, but they were not showing up. I had to --amend --no-edit my commit and force push again.

[neiljp]

@preetmishra Still loving this 👍 This may look like lot of text feedback, but hopefully should be relatively small to update. Some of this back and forth is just since we're refining what the feature should be as we go!

I found another test case we should likely add, and a few examples that show a surprising result now that we're simplifying the links with no text - which suggests we might wish to hide any no-text link from the footlinks, but set the message text to the href? (see discussion inline)

As per a comment not in this review, I think if we can avoid needing the link_index variable then it might be simpler - if it's clearer what I meant now; it seems feasible?

Structurally the PR feels ever so slightly unbalanced to have the smaller refining commits later where there is at least one refinement in the first commit (removal of duplicates), which could also combine with the rstrip addition perhaps (including the extra test I suggested?), assuming that's mainly to make near-similar links easier to combine? This is not that significant to change, but you can probably see how the 'story' of the commits reads well right now with the simplest implementation and refining in later steps, and the more of that the better as long as each commit holds together :)

[neiljp]

Regarding the hiding of all full-text links, I should clarify that if there is a 'file name' then this works well as the 'text' substitute, but otherwise I'm not sure that another part of the URL makes sense on its own.

[preetmishra]

@neiljp Thanks for the review and discovering those examples. They helped me greatly to improve the last segment and the skip footlink bit. 👍

I have removed the link_index attribute and moved to only using the message_links, and added tests for the trailing slash bit. Moreover, I have reworked the last segment detection bit and the related skip footlink. Hopefully, they should be more reliable now.

Thanks for helping me with the commit structure. They do seem to tell a 'story' with the incremental changes and look coherent when compared to what we started with. 👍 The duplicate links excerpt is necessary to have with the initial commit otherwise the OrderedDict keeps getting overridden and we end up with rather funny link indexes.

Re hiding of all full-text links, see #703 (comment).

[neiljp]

@preetmishra One remaining behavioral point - after running with it and looking at #commits (re chat.zulip.org -> http://chat.zulip not being a branch name).

Re the tests, I do wonder if there are footlink tests we're missing which match to others in the soup2markup tests. There are so many now, it's challenging to check the pairs - we could generate test cases to work with both in future, I suppose! (TODO comment?)

Regarding structure (tests across commits), maybe footlink tests added in later commits make sense to include earlier, defining the earlier behavior, to be changed later (if they don't duplicate another parametrization or are otherwise meaningless/redundant earlier). This is minor at this point, but worth thinking about in future.

[preetmishra]

@neiljp Thanks for the review! In the latest update, I have:

• Fixed the link text behaviour that you mentioned here and we talked about on CZO. Now, example.com http://example.com https://example.com come out as example.com [1] http://example.com [1] https://example.com [2] (without any footlink) and the last segment is only used when the link text has something else than just the 'domain name'.
• Improved the tests and added the mentioned TODO comment.
• Restructured the tests to amend them incrementally as the refinements are added.

[neiljp]

@preetmishra Thanks for the update and all your work on this 👍 I've just merged this! 🎉

The only outstanding thing I think we could tidy up is the cases where we show the last entry (or not), but that's definitely fine tuning now, and we have a lot of test cases that we can re-examine to determine if we want different behavior. On that note, it might be worth investigating if we can incorporate the ids inline (pytest.param?), since we have so many parametrizations in soup2markup now!

The popup and link-opening work will be great to build off this. Perhaps we can use the same parametrizations as a fixture?

[preetmishra]

@neiljp Thanks for the merge! I'll proceed with the follow-ups now. 👍