Skip to content
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

Get multi-line links working #1303

Merged
merged 16 commits into from
Mar 21, 2018
Merged

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Feb 27, 2018

Fixes #24

@Tyriar Tyriar added the work-in-progress Do not merge label Feb 27, 2018
@Tyriar Tyriar added this to the 3.3.0 milestone Feb 27, 2018
@Tyriar Tyriar self-assigned this Feb 27, 2018
@Tyriar Tyriar removed the work-in-progress Do not merge label Mar 9, 2018
@Tyriar
Copy link
Member Author

Tyriar commented Mar 9, 2018

This is ready for review, multi line links seem to work great with the exception of echoing the link and then sizing the terminal down. This happens because of the workaround for reflow where we retain the text to the right of the terminal.

image

image

Since I plan on adding support for reflow in the next couple of months I don't think it's even worth working around this issue.

@mofux
Copy link
Contributor

mofux commented Mar 9, 2018

Found a couple of issues by running head README.md from the demo. See the attached capture.

kapture 2018-03-09 at 22 02 16

@Tyriar
Copy link
Member Author

Tyriar commented Mar 11, 2018

@mofux should be fixed now, added tests too 👍

@mofux
Copy link
Contributor

mofux commented Mar 13, 2018

I found another bug that is also likely caused by wrapped lines. If you run head README.md twice you should have enough buffer filled to scroll. Scroll the whole way up and hover the upper links, it will work. Then scroll a few lines down so that the first link is outside the viewport. Notice that all the other links won't get detected anymore. I believe this is caused because all links are actually in one line that is wrapped:

kapture 2018-03-13 at 23 19 01

@Tyriar
Copy link
Member Author

Tyriar commented Mar 13, 2018

@mofux I decided to ignore this edge case for now, but you found a case where it looks particularly bad 😛. It happens because we don't bother to backtrack and linkify lines that are outside of the viewport (only initial line (!isWrapped) are linkified). I removed the TODO and make a comment to that effect in b44a11c

I guess if the requested line is the first line and it's flagged as wrapped, we could backtrack and linkify. Then add handling to make sure mouse zones are not created outside the viewport. I'll give it a shot.

@Tyriar Tyriar added the work-in-progress Do not merge label Mar 13, 2018
@Tyriar Tyriar removed the work-in-progress Do not merge label Mar 16, 2018
@Tyriar Tyriar requested a review from mofux March 16, 2018 14:21
@Tyriar
Copy link
Member Author

Tyriar commented Mar 16, 2018

@mofux ok I think that fixes it

@mofux
Copy link
Contributor

mofux commented Mar 16, 2018

Just tested, this is working nicely now! There's one issue left, but it might have to do with the link detection in general. Some links are too long (ending on a bracket), and others span muliple links at once - see the capture below.

kapture 2018-03-16 at 15 33 46

@Tyriar
Copy link
Member Author

Tyriar commented Mar 16, 2018

@mofux yeah it's an issue with the link detection in general. Since we use regex we either have to decide whether to allow brackets (and quotes) and accept cases like this, or break heaps of links such as https://msdn.microsoft.com/en-us/library/windows/desktop/ms684839(v=vs.85).aspx

Tracked in #583

@mofux
Copy link
Contributor

mofux commented Mar 21, 2018

I think we can merge this PR. Let's tackle the link detection issues separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants