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

Performant Bracket Pair Colorization #129231

Merged
merged 18 commits into from
Aug 16, 2021
Merged

Performant Bracket Pair Colorization #129231

merged 18 commits into from
Aug 16, 2021

Conversation

hediet
Copy link
Member

@hediet hediet commented Jul 23, 2021

Before with some extension (~10.000ms):

recording

After:

recording

image

(<1 ms, more than 10k times faster)

More ToDos: #130767

"editor.bracketPairColorization.enabled": true will enable this new feature!

@hediet hediet self-assigned this Jul 23, 2021
@Tyriar
Copy link
Member

Tyriar commented Jul 23, 2021

So this is getting built in instead of being in an extension? Have we thought about using dynamic imports for optional features like this so the bundle size doesn't keep growing?

@hediet
Copy link
Member Author

hediet commented Jul 23, 2021

So this is getting built in instead of being in an extension?

Yes, see discussion here.

Properly exposing tokens to the extension host is a very hard problem - basically, the tokens would need to be recomputed by the extension host.
Synchronizing color information back to the editor is also very hard, as a single change can cause all following brackets to change their position & their color.

This implemenation in the editor core solves both problems by efficiently reusing the existing token information and synchronously providing the decorations when the view requests them.

Have we thought about using dynamic imports for optional features like this so the bundle size doesn't keep growing?

That is a nice idea!
However, this implementation could potentially also drive the existing bracket pair matching, which could increase accuracy:

image

(In this case, I would rather match the two outer parentheses, as [...] should limit the scope of the inner ()

@KMFSousa
Copy link

This looks related to issue: #96899

@hediet hediet force-pushed the hediet/bracketPairColorizer branch from 0676fbe to 9fd6956 Compare August 6, 2021 08:19
@hediet hediet marked this pull request as ready for review August 6, 2021 08:23
@hediet hediet requested a review from rebornix August 6, 2021 08:24
@hediet
Copy link
Member Author

hediet commented Aug 6, 2021

@rebornix I think this is ready for review. The last details took me quite a while.

I will iterate on the performance. I had it at 50ms for checker.ts, but now it is 160ms again after I made it more correct and supported nested languages.

I think the most important points for the review are:

  • editor.bracketPairColorization.enabled defaults to false. It should be easy to verify that in this case, the new code does nothing - so when bracket pair colorization is turned off, there is absolutely no performance hit and everything should work as usual.
  • This feature only changes very little existing code, so it does not add a lot of debt
  • I suggest to review each commit individually.
  • You don't need to check the feature itself for correctness, I will iterate on it.

Also, I will add more tests.

Thanks!

@@ -0,0 +1,104 @@
/*---------------------------------------------------------------------------------------------
Copy link
Member

@bpasero bpasero Aug 6, 2021

Choose a reason for hiding this comment

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

Maybe move utilities that are editor agnostic into base with some tests? I.e. we have map.ts.

@kieferrm kieferrm mentioned this pull request Aug 8, 2021
91 tasks
@hediet hediet force-pushed the hediet/bracketPairColorizer branch from 33bef27 to 8bbb559 Compare August 10, 2021 09:59
@miguelsolorio
Copy link
Contributor

In terms of color, here are the ones I would use:

For dark, I'd keep the existing colors you have, and for light I'd use the following colors:

  • #9a6d03
  • #C433BF
  • #0879BF

CleanShot 2021-08-10 at 17 30 37@2x

And here are the color contrast ratios for each item on their respective backgrounds:

CleanShot 2021-08-10 at 17 28 13@2x

@hediet
Copy link
Member Author

hediet commented Aug 11, 2021

Thanks for the colors @misolori! However, now even I have troubles distinguishing the colors of the light theme.

image

image

The colors for the dark theme have a better relative contrast:

image

@hediet hediet requested a review from alexdima August 11, 2021 08:24
@hediet hediet merged commit 7aa0f8e into main Aug 16, 2021
@hediet hediet deleted the hediet/bracketPairColorizer branch August 16, 2021 11:32
@hediet
Copy link
Member Author

hediet commented Aug 16, 2021

👍 Only P0 is to add a limit for super large files.

Limit is 50k lines with an average of 100 characters per line, so that it just accepts checker.ts.
Maybe the limit is to high and needs to be tweaked later.

@KROSF
Copy link

KROSF commented Aug 17, 2021

image

There are some problems with closing jsx tags

image
same problem with html

@IllusionMH
Copy link
Contributor

@KROSF It's better to create separate issue as this is PR that already merged.
Looks like / is not colored and therefore whole ligature doesn't get color. Try to set "editor.fontLigatures": false

@sanket-bhalerao
Copy link

is showHorizontalScopeLine is going to be a part of this?

@IllusionMH
Copy link
Contributor

@sanket-bhalerao better to discuss it in #131001

br3ndonland added a commit to br3ndonland/dotfiles that referenced this pull request Aug 25, 2021
microsoft/vscode#96899
microsoft/vscode#129231

Built-in bracket pair colorization will be released in VSCode 1.60.
This commit will disable bracket pair colorization, because colors
do not match the editor theme.
@dtinth
Copy link
Contributor

dtinth commented Aug 26, 2021

How to change colors

Up to 6 colors can be specified, according to the code in this PR.

  "workbench.colorCustomizations": {
    "editorBracketHighlight.foreground1": "#896c74",
    "editorBracketHighlight.foreground2": "#6c8289",
    "editorBracketHighlight.foreground3": "#736c89",
    "editorBracketHighlight.foreground4": "#74896c",
    "editorBracketHighlight.foreground5": "#896c82",
    "editorBracketHighlight.foreground6": "#89736c"
  }

@carlosmgilp
Copy link

carlosmgilp commented Sep 3, 2021

How to change colors

Up to 6 colors can be specified, according to the code in this PR.

{
  "workbench.colorCustomizations": {
    "editorBracketHighlight.foreground1": "#896c74",
    "editorBracketHighlight.foreground2": "#6c8289",
    "editorBracketHighlight.foreground3": "#736c89",
    "editorBracketHighlight.foreground4": "#74896c",
    "editorBracketHighlight.foreground5": "#896c82",
    "editorBracketHighlight.foreground6": "#89736c"
  }
}

Hello how are you.
I'm trying to customize the colors and it doesn't work for me.
Try this way and it doesn't work.
You could explain well how to do it.
Copy paste the example and it doesn't work either.
I have vscode 1.60.0 installed
Thanks.

@gjsjohnmurray
Copy link
Contributor

I'm trying to customize the colors and it doesn't work for me.

@carlosmgilp did you try the instructions at https://code.visualstudio.com/docs/getstarted/themes#_customizing-a-color-theme ?

@carlosmgilp
Copy link

I'm trying to customize the colors and it doesn't work for me.

@carlosmgilp did you try the instructions at https://code.visualstudio.com/docs/getstarted/themes#_customizing-a-color-theme ?

Hello, how are you? Thank you for your answer.
Excuse my English, I only speak Spanish.
Well, I'm trying to do it like that, but if you look, when writing it doesn't show me the option of "editorBracketHighlight.foregroundn"
I don't know if I'm doing something wrong.

image

These are the errors it shows me.

image

Thanks.

@miguelsolorio
Copy link
Contributor

@carlosmgilp you need to remove the extra brackets on line 96 & 105:

image

@carlosmgilp
Copy link

@carlosmgilp you need to remove the extra brackets on line 96 & 105:

Hello brother, how are you? Thank you so much for your help and everyone's.
It works great now.
Thank you.

@carlosmgilp
Copy link

Is there a way to activate the colored line between each bracket.
Beforehand thank you very much.

image

@carlosmgilp
Copy link

Hello how are you.
I wanted to make an inquiry.
Because when I have part of my code commented, the parentheses are still shown in color.
Is that a bug or I have to configure something else.
I am using AutoLISP as code.

image

@hediet
Copy link
Member Author

hediet commented Sep 9, 2021

See #132249.

@carlosmgilp
Copy link

Hi @hediet thanks for your help.
I will publish it in the other post.
Thank you.

@github-actions github-actions bot locked and limited conversation to collaborators Sep 30, 2021
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.

None yet