Skip to content

Commit cbc1123

Browse files
authored
fix(theme/#1933): Scope matching precedence (#2301)
__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)
1 parent 6daf28d commit cbc1123

File tree

3 files changed

+229
-157
lines changed

3 files changed

+229
-157
lines changed

src/reason-textmate/ThemeScopes.re

Lines changed: 81 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@ module Scope = {
1313

1414
let ofString = s => String.split_on_char('.', s);
1515

16+
let toString = scope => String.concat(".", scope);
17+
1618
let rec matches = (selector: t, v: t) => {
1719
switch (selector, v) {
1820
| ([], _) => true
@@ -44,6 +46,9 @@ module Scopes = {
4446
|> String.split_on_char(' ')
4547
|> List.map(v => Scope.ofString(String.trim(v)));
4648

49+
let toString = scopes =>
50+
String.concat("\n", scopes |> List.map(Scope.toString));
51+
4752
let rec matches = (selector: t, v: t) => {
4853
switch (selector, v) {
4954
| ([], _) => true
@@ -58,6 +63,22 @@ module Scopes = {
5863
};
5964
};
6065

66+
module ResolvedStyle = {
67+
type t = {
68+
foreground: string,
69+
background: string,
70+
bold: bool,
71+
italic: bool,
72+
};
73+
74+
let default = (~foreground, ~background, ()) => {
75+
foreground,
76+
background,
77+
bold: false,
78+
italic: false,
79+
};
80+
};
81+
6182
module TokenStyle = {
6283
[@deriving show({with_path: false})]
6384
type t = {
@@ -74,6 +95,66 @@ module TokenStyle = {
7495
};
7596
};
7697

98+
let merge = (prev, style) => {
99+
let foreground =
100+
switch (prev.foreground, style.foreground) {
101+
| (Some(v), _) => Some(v)
102+
| (_, Some(v)) => Some(v)
103+
| _ => None
104+
};
105+
106+
let background =
107+
switch (prev.background, style.background) {
108+
| (Some(v), _) => Some(v)
109+
| (_, Some(v)) => Some(v)
110+
| _ => None
111+
};
112+
113+
let bold =
114+
switch (prev.bold, style.bold) {
115+
| (Some(v), _) => Some(v)
116+
| (_, Some(v)) => Some(v)
117+
| _ => None
118+
};
119+
120+
let italic =
121+
switch (prev.italic, style.italic) {
122+
| (Some(v), _) => Some(v)
123+
| (_, Some(v)) => Some(v)
124+
| _ => None
125+
};
126+
127+
{background, foreground, bold, italic};
128+
};
129+
130+
let resolve = (~default: ResolvedStyle.t, style) => {
131+
let foreground =
132+
switch (style.foreground) {
133+
| Some(v) => v
134+
| None => default.foreground
135+
};
136+
137+
let bold =
138+
switch (style.bold) {
139+
| Some(v) => v
140+
| None => default.bold
141+
};
142+
143+
let italic =
144+
switch (style.italic) {
145+
| Some(v) => v
146+
| None => default.italic
147+
};
148+
149+
let background =
150+
switch (style.background) {
151+
| Some(v) => v
152+
| None => default.background
153+
};
154+
155+
ResolvedStyle.{bold, italic, foreground, background};
156+
};
157+
77158
let create =
78159
(
79160
~foreground: option(string)=None,
@@ -96,22 +177,6 @@ module TokenStyle = {
96177
};
97178
};
98179

99-
module ResolvedStyle = {
100-
type t = {
101-
foreground: string,
102-
background: string,
103-
bold: bool,
104-
italic: bool,
105-
};
106-
107-
let default = (~foreground, ~background, ()) => {
108-
foreground,
109-
background,
110-
bold: false,
111-
italic: false,
112-
};
113-
};
114-
115180
module Selector = {
116181
type t = {
117182
scopes: Scopes.t,

src/reason-textmate/TokenTheme.re

Lines changed: 97 additions & 135 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ let create =
4747

4848
let trie =
4949
List.fold_left(
50-
(prev, curr) => {
50+
(prev: Trie.t(selectorWithParents), curr) => {
5151
open Selector;
5252
let {scopes, style} = curr;
5353

@@ -206,150 +206,112 @@ let show = (v: t) => {
206206
);
207207
};
208208

209-
let _applyStyle: (TokenStyle.t, TokenStyle.t) => TokenStyle.t =
210-
(prev: TokenStyle.t, style: TokenStyle.t) => {
211-
let foreground =
212-
switch (prev.foreground, style.foreground) {
213-
| (Some(v), _) => Some(v)
214-
| (_, Some(v)) => Some(v)
215-
| _ => None
216-
};
217-
218-
let background =
219-
switch (prev.background, style.background) {
220-
| (Some(v), _) => Some(v)
221-
| (_, Some(v)) => Some(v)
222-
| _ => None
223-
};
209+
let match = (theme: t, scopes: string) => {
210+
let scopes = Scopes.ofString(scopes) |> List.rev;
224211

225-
let bold =
226-
switch (prev.bold, style.bold) {
227-
| (Some(v), _) => Some(v)
228-
| (_, Some(v)) => Some(v)
229-
| _ => None
212+
let rec calculateStyle = (~parentScopes, ~acc: list(TokenStyle.t), scopes) => {
213+
switch (scopes) {
214+
| [] => acc
215+
| [scope, ...nextScope] =>
216+
// Get the matching path from the Trie
217+
let matchingPath = Trie.matches(theme.trie, scope);
218+
219+
if (matchingPath == []) {
220+
// No matches - let's try the next scope!
221+
calculateStyle(
222+
~parentScopes=[scope, ...parentScopes],
223+
~acc,
224+
nextScope,
225+
);
226+
} else {
227+
let maybeTokenStyle: option(TokenStyle.t) =
228+
matchingPath
229+
|> List.fold_left(
230+
(maybePrev: option(TokenStyle.t), curr) => {
231+
let (_name, selector: option(selectorWithParents)) = curr;
232+
233+
switch (selector) {
234+
// No selector at this node. This can happen when a node is on the
235+
// path to a node with a style. Nothing to do here; continue on.
236+
| None => maybePrev
237+
// We have a selector at this node. Let's check it out.
238+
| Some({style, parents}) =>
239+
let prevStyle =
240+
maybePrev |> Option.value(~default=TokenStyle.default);
241+
242+
// Get the list of matching parent selectors to apply
243+
let parentsScopesToApply =
244+
parents
245+
|> List.filter(selector =>
246+
Selector.matches(selector, parentScopes)
247+
);
248+
249+
switch (parentsScopesToApply, style) {
250+
// Case 1: No parent selectors match AND there is no style. We should continue on.
251+
| ([], None) => None
252+
253+
// Case 2: No parent selectors match, but there is a style at the Node. We should apply the style.
254+
| ([], Some(style)) =>
255+
Some(TokenStyle.merge(prevStyle, style))
256+
257+
// Case 3: We have parent selectors that match, and may or may not have a style at the node.
258+
// Apply the parent styles, and the node style, if applicable.
259+
| (_, maybeStyle) =>
260+
let newStyle =
261+
maybeStyle
262+
|> Option.map(TokenStyle.merge(prevStyle))
263+
|> Option.value(~default=TokenStyle.default);
264+
265+
// Apply any parent selectors that match...
266+
// we should be sorting this by score!
267+
Some(
268+
List.fold_left(
269+
(prev, curr: Selector.t) => {
270+
open Selector;
271+
let {style, _} = curr;
272+
// Reversing the order because the parent style
273+
// should take precedence over previous style
274+
TokenStyle.merge(style, prev);
275+
},
276+
newStyle,
277+
parentsScopesToApply,
278+
),
279+
);
280+
};
281+
};
282+
},
283+
None,
284+
);
285+
286+
let acc =
287+
maybeTokenStyle
288+
|> Option.map(tokenStyle => [tokenStyle, ...acc])
289+
|> Option.value(~default=acc);
290+
291+
calculateStyle(
292+
~parentScopes=[scope, ...parentScopes],
293+
~acc,
294+
nextScope,
295+
);
230296
};
297+
};
298+
};
231299

232-
let italic =
233-
switch (prev.italic, style.italic) {
234-
| (Some(v), _) => Some(v)
235-
| (_, Some(v)) => Some(v)
236-
| _ => None
237-
};
300+
let styles = calculateStyle(~parentScopes=[], ~acc=[], scopes);
238301

239-
{background, foreground, bold, italic};
240-
};
302+
let result: TokenStyle.t =
303+
styles
304+
|> List.fold_left(
305+
(acc, style) => {TokenStyle.merge(acc, style)},
306+
TokenStyle.default,
307+
);
241308

242-
let match = (theme: t, scopes: string) => {
243-
let scopes = Scopes.ofString(scopes) |> List.rev;
244309
let default =
245310
ResolvedStyle.default(
246311
~foreground=theme.defaultForeground,
247312
~background=theme.defaultBackground,
248313
(),
249314
);
250315

251-
let rec f = scopes => {
252-
switch (scopes) {
253-
| [] => default
254-
| [scope, ...scopeParents] =>
255-
// Get the matching path from the Trie
256-
let p = Trie.matches(theme.trie, scope);
257-
258-
switch (p) {
259-
// If there were no matches... try the next scope up.
260-
| [] => f(scopeParents)
261-
// Got matches - we'll apply them in sequence
262-
| _ =>
263-
let result =
264-
List.fold_left(
265-
(prev: option(TokenStyle.t), curr) => {
266-
let (_name, selector: option(selectorWithParents)) = curr;
267-
268-
switch (selector) {
269-
// No selector at this node. This can happen when a node is on the
270-
// path to a node with a style. Nothing to do here; continue on.
271-
| None => prev
272-
// We have a selector at this node. Let's check it out.
273-
| Some({style, parents}) =>
274-
let prevStyle =
275-
switch (prev) {
276-
| None => TokenStyle.default
277-
| Some(v) => v
278-
};
279-
280-
// Get the list of matching parent selectors to apply
281-
let parentsScopesToApply =
282-
parents
283-
|> List.filter(selector =>
284-
Selector.matches(selector, scopeParents)
285-
);
286-
287-
switch (parentsScopesToApply, style) {
288-
// Case 1: No parent selectors match AND there is no style. We should continue on.
289-
| ([], None) => None
290-
// Case 2: No parent selectors match, but there is a style at the Node. We should apply the style.
291-
| ([], Some(style)) => Some(_applyStyle(prevStyle, style))
292-
// Case 3: We have parent selectors that match, and may or may not have a style at the node.
293-
// Apply the parent styles, and the node style, if applicable.
294-
| (_, style) =>
295-
let newStyle =
296-
switch (style) {
297-
| Some(v) => _applyStyle(prevStyle, v)
298-
| None => TokenStyle.default
299-
};
300-
// Apply any parent selectors that match...
301-
// we should be sorting this by score!
302-
Some(
303-
List.fold_left(
304-
(prev, curr: Selector.t) => {
305-
open Selector;
306-
let {style, _} = curr;
307-
// Reversing the order because the parent style
308-
// should take precedence over previous style
309-
_applyStyle(style, prev);
310-
},
311-
newStyle,
312-
parentsScopesToApply,
313-
),
314-
);
315-
};
316-
};
317-
},
318-
None,
319-
p,
320-
);
321-
322-
switch (result) {
323-
| None => f(scopeParents)
324-
| Some(result) =>
325-
let foreground =
326-
switch (result.foreground) {
327-
| Some(v) => v
328-
| None => default.foreground
329-
};
330-
331-
let bold =
332-
switch (result.bold) {
333-
| Some(v) => v
334-
| None => default.bold
335-
};
336-
337-
let italic =
338-
switch (result.italic) {
339-
| Some(v) => v
340-
| None => default.italic
341-
};
342-
343-
let background =
344-
switch (result.background) {
345-
| Some(v) => v
346-
| None => default.background
347-
};
348-
349-
{background, foreground, bold, italic};
350-
};
351-
};
352-
};
353-
};
354-
f(scopes);
316+
TokenStyle.resolve(~default, result);
355317
};

0 commit comments

Comments
 (0)