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

fix linkifier after shrinking #1769

Closed
wants to merge 23 commits into from

Conversation

jerch
Copy link
Member

@jerch jerch commented Oct 25, 2018

Fix for microsoft/vscode#61708.

To get this working, I had to add a partial fix of the trim behavior of Buffer.translateBufferLineToString for lines that are longer than the current Terminal.cols and get new content (contains a generalized version of this commit 48bd634, thanks to @dnfield). Reverted since it introduces to many side effects.

@Tyriar Tyriar self-requested a review October 25, 2018 22:13
@Tyriar Tyriar added this to the 3.9.0 milestone Oct 25, 2018
src/Linkifier.test.ts Show resolved Hide resolved
src/Buffer.test.ts Outdated Show resolved Hide resolved
if (endIndex[1] >= this._terminal.cols) {
endIndex[1] = this._terminal.cols - 1;
}
const visibleLength = (endIndex[0] - bufferIndex[0]) * this._terminal.cols - bufferIndex[1] + endIndex[1];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does using visibleLength mean URLs will be broken if the viewport is shrunk?

Copy link
Member Author

@jerch jerch Oct 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

visibleLength is just there to give the marker the length to underline. This makes sure that the underline does not over- or underflow the real end of the URL. So after shrinking the underline should stick to chars that really belong to the URL. Side effect of this - after enlarging inserted empty cells are also underlined. To get rid of this, the underline marker would have to support multiple chunks to underline.

@Tyriar
Copy link
Member

Tyriar commented Oct 25, 2018

I'm not sure it's working for me?

After making wider:

screen shot 2018-10-25 at 4 22 11 pm

After shrinking:

screen shot 2018-10-25 at 4 22 24 pm

@jerch
Copy link
Member Author

jerch commented Oct 25, 2018

@Tyriar Can you test again? Now it gets very hacky, it also pulls in the problem of the empty cell representation. 😒
Without the last change it only works for enlarging as long as you stay within the original line length.

Not sure were this here will lead, kinda dont want to change the impl of translateBufferLineToString itself.

@jerch jerch closed this Oct 26, 2018
@jerch jerch reopened this Oct 26, 2018
// we cannot directly use uri.length here since stringIndexToBufferIndex would
// skip empty cells and stop at the next cell with real content
// instead we fetch the index of the last char in uri and advance to the next cell
const endIndex = this._terminal.buffer.stringIndexToBufferIndex(rowIndex, stringIndex + uri.length - 1, true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a problem case with always trimming:

screen shot 2018-10-26 at 10 13 44 am

What if we only trim the whitespace inside the shadowed content, so here the 2 spaces would still remain. This would open up a problem with echoing this and then sizing down twice:

screen shot 2018-10-26 at 10 17 17 am

But maybe that's a fine compromise until reflow as links without resizing will work then?

@jerch
Copy link
Member Author

jerch commented Oct 30, 2018

@Tyriar Im not sure anymore if this is fixable in a sane way without altering the trimming behavior (this will touch the empty cell representation and the isWrapped handling as well). I am able to fix it with appending real empty cells with enlarging and not trimming lines under any circumstances (as done in the reverted commits, which partly implements #1685). But this comes with unknown side effects that are not covered by test cases which is really bad and therefore not feasible.
Imho your suggestion to only trim spaces in the shadowed content just moves the problem to the end of the shadowed line and shows the same awkward faulty behavior after shrinking. Enlarging wont work at all.
I suggest to stall this PR until we got reflow support since it will most likely also cover those trim awkwardities. Currently our buffer representation of wrapped stuff seems to be broken beyond repair to me. 😒
Another way to fix this with our current buffer representation of wrapped stuff is to get trimming based on #1685 working, I think thats what most other emulators do that dont support reflow.

NB: Also found another issue with isWrapped - if you skip through multiline commands at the shell the isWrapped flag never gets lifted again - any Buffer.translateBufferLineToString consumer always gets those lines as one line afterwards, even if they contain single short separate lines.

😒 😒

@jerch
Copy link
Member Author

jerch commented Oct 30, 2018

The last commit works regarding the linkifier as far as I have tested it, but contains those empty cells after enlarging, hmm. (Also a few tests dont pass since they test for the "faulty" behavior).
Imho this is to radical to be done within this PR, also it contains the empty cell thing only partially, which introduces more state handling issues if not done globally.

@jerch
Copy link
Member Author

jerch commented Nov 7, 2018

Currently stalled by #1775.

@jerch jerch self-assigned this Nov 15, 2018
@Tyriar Tyriar added the work-in-progress Do not merge label Nov 24, 2018
@jerch jerch removed this from the 3.9.0 milestone Nov 29, 2018
@jerch jerch closed this Dec 16, 2018
@jerch jerch reopened this Dec 16, 2018
@jerch
Copy link
Member Author

jerch commented Dec 16, 2018

@Tyriar With #1775 the issue you described should be gone, also the original issue microsoft/vscode#61708 should be fixed now.

The line now sticks correctly to the first and the last cell of the url char, even for surrogates and combining chars. Only weird behavior still is the "space extension" to the right after enlarging the terminal. Note that this is only a visual glitch, c&p of the url does not insert any spaces (returns correct url).

Up for review.

@jerch jerch removed the work-in-progress Do not merge label Dec 16, 2018
@jerch jerch added this to the 3.10.0 milestone Dec 16, 2018
@jerch
Copy link
Member Author

jerch commented Jan 1, 2019

Partially blocked by #1864.

Reminder: With reflow in place remove the hidden content workarounds as we dont need them anymore.

@jerch jerch modified the milestones: 3.10.0, 3.11.0 Jan 1, 2019
@Tyriar Tyriar modified the milestones: 3.11.0, 3.12.0 Feb 1, 2019
@jerch jerch added the work-in-progress Do not merge label Feb 1, 2019
@Tyriar Tyriar removed this from the 3.12.0 milestone Mar 4, 2019
@Tyriar
Copy link
Member

Tyriar commented May 21, 2019

Moved to #2115

@Tyriar Tyriar closed this May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work-in-progress Do not merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants