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

Add 'selection' option to render whitespace #77533

Merged
merged 4 commits into from
Jul 22, 2019
Merged

Conversation

RMacfarlane
Copy link
Contributor

@RMacfarlane RMacfarlane commented Jul 17, 2019

#1477

whitespace_selection

Since whitespace is rendered as part of the line content and not on the selection layer, and since render line already responds to selection changes in some cases, the change I made is to view line rendering. Now, if the setting is on, any selections that are on the line are part of the line data for rendering. When applying whitespace, it checks the selections to see if the whitespace character is part of that range or not.

@RMacfarlane RMacfarlane self-assigned this Jul 17, 2019
@RMacfarlane RMacfarlane requested a review from alexdima July 17, 2019 16:47
@RMacfarlane RMacfarlane added this to the July 2019 milestone Jul 17, 2019
Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

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

🎉 Nice work! You've done all the right changes, there are 3 things I would change:

  • stay away from undefined (personal preference)
  • don't use Range because that contains line numbers and create a new class that only contains the data the render line code needs
  • remove the inner loop in the render whitespace (the one where using .some(...))

src/vs/editor/common/viewLayout/viewLineRenderer.ts Outdated Show resolved Hide resolved
src/vs/editor/common/viewLayout/viewLineRenderer.ts Outdated Show resolved Hide resolved
src/vs/editor/common/viewLayout/viewLineRenderer.ts Outdated Show resolved Hide resolved
src/vs/editor/common/viewLayout/viewLineRenderer.ts Outdated Show resolved Hide resolved
src/vs/editor/browser/viewParts/lines/viewLine.ts Outdated Show resolved Hide resolved
src/vs/editor/browser/widget/diffReview.ts Outdated Show resolved Hide resolved
@RMacfarlane
Copy link
Contributor Author

Thanks for the great feedback! 👍 I’ve added a LineRange class, updated to using null, and eliminated the inner loop of checking selections.

Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

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

👍 Really nice! I just found one little bug! Please merge once you cover that!

for (let charIndex = fauxIndentLength; charIndex < len; charIndex++) {
const chCode = lineContent.charCodeAt(charIndex);

if (currentSelection && charIndex > currentSelection.endOffset) {
Copy link
Member

Choose a reason for hiding this comment

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

Here the check should be charIndex >= currentSelection.endOffset. Reverse-engineered steps to observe a bug:

  • have some text * (<space>, *, <space>).
  • create 3 multiple cursors before the first space, before the * and before the second space.
  • press shift+right
  • the whitespace is not painted in the second <space> due to this check.

This case could serve as a good unit test.

Actual:
image

Expected:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! I added a test for this

@@ -540,6 +603,11 @@ function _applyRenderWhitespace(lineContent: string, len: number, continuesWithW
isInWhitespace = false;
}

// If rendering whitespace on selection, check that the charIndex falls within a selection
if (isInWhitespace && selections) {
isInWhitespace = !!currentSelection && currentSelection.startOffset <= charIndex && currentSelection.endOffset > charIndex;
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: small style inconsistency, use here currentSelection && ... instead of !!currentSelection && ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removing the !! causes a typescript error since isInWhitespace should be a boolean, so will leave as is

@rebornix
Copy link
Member

Found a tiny bug with multi cursor, looks like related to how the selection boundaries are validated

    vscode.languages.registerDocumentLinkProvider('*', {
        provideDocumentLinks: (document: vscode.TextDocument) => {
            return [{
                range: new vscode.Range(0, 0, 0, 2),
                target: vscode.Uri.parse('command:extension.uidialog')
            }];
        }
    })

Indentations are all whitespaces. Put a cursor before v of vscode and another cursor before p of provideDocumentLinks. Then shift+arrow-down then shift+arrow-right

multi-cursor-merge

when the end of the first cursor meets the beginning of the second cursor, the last whitespace is not rendered.

@rebornix
Copy link
Member

It's already merged. I'll create a new issue.

@RMacfarlane
Copy link
Contributor Author

@rebornix Nice find! Seems to be an existing problem unrelated to this change

@RMacfarlane RMacfarlane deleted the rmacfarlane/whitespace branch September 13, 2019 18:56
@github-actions github-actions bot locked and limited conversation to collaborators Mar 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants