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

test(textmate): Show used theme colour #2255

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

CrossR
Copy link
Member

@CrossR CrossR commented Aug 6, 2020

Just a test to help look into stuff like #1933 / #1879.

We can look at the colours being produced more easily now and see the difference between themes:

image

I.e. OneDark always produces the same #bbbbbb colour for everything here. This was just a quick test to check it was reproducible, so I'll now extend it to tokens : (___), colour: (__) from (__) so we know which scope is being used to give us that colour.

@CrossR
Copy link
Member Author

CrossR commented Aug 6, 2020

Other point of interest/nice comparison here is the VSCode Ctrl-Shift-P Inspect Editor tokens/scopes:

image
image

The scopes are the exact same*, so they are all valid and complete (ignore the text.html.derivative vs basic, that is just the syntax file I loaded).

Gruvbox managed to match against text.html entity.name.tag, which does exist as a HTML/XML specific part and is the same colour shown above.
OneDark however matches against entity.name.tag, and should have a colour....but we don't have that.

Looking into it and with help from the comment over in #1933, it looks like a lower priority scope is causing the higher priority ones to be ignored. In that case it was a general "scope" one, but for OneDark and HTML, its the "meta.tag" one. Removing that gives me slightly better results.

image

Its then possible that is causing further issues like the text to not be matched correctly etc. But removing that option does fix the div to start reporting correctly.

Still trying to get my head around what that means need changing though!

(* The scopes loaded aren't quite the exact same...we have $1 etc in ours. I haven't done enough digging to check if that is just a result of the CLI tool, or if we are infact not replacing those variables with what is needed).

@bryphe
Copy link
Member

bryphe commented Aug 7, 2020

Awesome, thanks for investigating this @CrossR ! Huge help.

This is the module where we do the matching: https://github.com/onivim/oni2/blob/master/src/reason-textmate/TokenTheme.re

And we have some basic test cases around the matching here:

describe("match", ({test, _}) => {

The scopes loaded aren't quite the exact same...we have $1 etc in ours.

This is an interesting observation - I think it might be a bug with the textmate parsing - we might be missing a case where we need to replace a part of the token name with a match group in the regex.

So it looks like two separate issues - these are possible the root cause of several highlighting issues, as well:

@CrossR
Copy link
Member Author

CrossR commented Aug 7, 2020

I've had a poke around the HTML grammar, and this is the part we are looking at:

"begin": "(?i)(</)(address|article|aside|blockquote|body|button|caption|colgroup|datalist|dd|details|dialog|div|dl|dt|fieldset|figcaption|figure|footer|form|head|header|hgroup|html|h[1-6]|label|legend|li|main|map|menu|meter|nav|ol|optgroup|option|output|p|pre|progress|section|select|slot|summary|table|tbody|td|template|textarea|tfoot|th|thead|tr|ul)(?=\\s|/?>)",
"beginCaptures": {
"1": {
"name": "punctuation.definition.tag.begin.html"
},
"2": {
"name": "entity.name.tag.html"
}
},
"end": ">",
"endCaptures": {
"0": {
"name": "punctuation.definition.tag.end.html"
}
},
"name": "meta.tag.structure.$2.end.html",
"patterns": [
{
"include": "#attribute"
}
]
},

However, I don't really see anywhere that the capture groups are really dealt with as part of the regex creation, it looks like its a TODO that might have gotten lost/an open issue?

{
captureGroups: None,
raw: str,
regex,
anchorCache,
compiledAnchorCache,
hasAnchorA: anchorA,
hasAnchorG: anchorG,
hasUnresolvedBackReferences,
};

captureGroups is only referenced twice, that None and the type def.

I would guess, since we are missing those capture groups, we then can't replace the $2 with the contents of that capture group.

I'll loop back to that later, and try and pin down what is also causing the scope matching issue as well, since this is two separate issues of making scopes and matching scopes. I've added a failing test for this specific part at least now.

bryphe added a commit that referenced this pull request Aug 17, 2020
__Issue:__ Our scope-selection strategy was not correct in some cases - particularly, the precedence in which we would apply the scopes.

For example, in #1933 - with the dracula theme - there'd be a case where a token would have the following textmate scopes:
- `support.function.console.js`
- `meta.function-call.js`
- `source.js`

For the dracula theme, there is a `source` selector that is intended to be a fallback - however, it was taking precedence over some of the other theme selectors that should've been applied, like `meta.function-call`.

This would cause the entire source file to be highlighted with the `source` scope selector, instead of the more specific ones.

__Defect:__ We apply selectors in reverse order (first, we apply any selectors matching `source.js`, and then `meta.function-call.js source.js`, etc...) - this gives us right precedence. However, if we had a match, we were failing to 'fall-back' to a more specific selector.

__Fix:__ Add a test case to reproduce, and fall back to override with more specific scopes.

This looks to fix a couple of related issues:
- #1933 - no syntax highlighting with OneDark / Dracula for some files
- #2006  - incorrect syntax highlighting for tsx files
- #1879  - html syntax highlighting not displayed
- #1878 - nested syntax highlights not shown

Thanks @CrossR for the helpful tool improvements in #2255 and @thismat for the investigation to narrow down the issue in #1933 !

`dracula` theme - __before__:

![image](https://user-images.githubusercontent.com/13532591/90297301-e2a6a000-de42-11ea-9380-178dc6345bf8.png)

`dracula` theme - __after__:

![image](https://user-images.githubusercontent.com/13532591/90297344-feaa4180-de42-11ea-999c-95936b4e3369.png)
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