Skip to content

Commit

Permalink
fix(theme/#1933): Scope matching precedence (#2301)
Browse files Browse the repository at this point in the history
__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)
  • Loading branch information
bryphe authored Aug 17, 2020
1 parent 6daf28d commit cbc1123
Show file tree
Hide file tree
Showing 3 changed files with 229 additions and 157 deletions.
97 changes: 81 additions & 16 deletions src/reason-textmate/ThemeScopes.re
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ module Scope = {

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

let toString = scope => String.concat(".", scope);

let rec matches = (selector: t, v: t) => {
switch (selector, v) {
| ([], _) => true
Expand Down Expand Up @@ -44,6 +46,9 @@ module Scopes = {
|> String.split_on_char(' ')
|> List.map(v => Scope.ofString(String.trim(v)));

let toString = scopes =>
String.concat("\n", scopes |> List.map(Scope.toString));

let rec matches = (selector: t, v: t) => {
switch (selector, v) {
| ([], _) => true
Expand All @@ -58,6 +63,22 @@ module Scopes = {
};
};

module ResolvedStyle = {
type t = {
foreground: string,
background: string,
bold: bool,
italic: bool,
};

let default = (~foreground, ~background, ()) => {
foreground,
background,
bold: false,
italic: false,
};
};

module TokenStyle = {
[@deriving show({with_path: false})]
type t = {
Expand All @@ -74,6 +95,66 @@ module TokenStyle = {
};
};

let merge = (prev, style) => {
let foreground =
switch (prev.foreground, style.foreground) {
| (Some(v), _) => Some(v)
| (_, Some(v)) => Some(v)
| _ => None
};

let background =
switch (prev.background, style.background) {
| (Some(v), _) => Some(v)
| (_, Some(v)) => Some(v)
| _ => None
};

let bold =
switch (prev.bold, style.bold) {
| (Some(v), _) => Some(v)
| (_, Some(v)) => Some(v)
| _ => None
};

let italic =
switch (prev.italic, style.italic) {
| (Some(v), _) => Some(v)
| (_, Some(v)) => Some(v)
| _ => None
};

{background, foreground, bold, italic};
};

let resolve = (~default: ResolvedStyle.t, style) => {
let foreground =
switch (style.foreground) {
| Some(v) => v
| None => default.foreground
};

let bold =
switch (style.bold) {
| Some(v) => v
| None => default.bold
};

let italic =
switch (style.italic) {
| Some(v) => v
| None => default.italic
};

let background =
switch (style.background) {
| Some(v) => v
| None => default.background
};

ResolvedStyle.{bold, italic, foreground, background};
};

let create =
(
~foreground: option(string)=None,
Expand All @@ -96,22 +177,6 @@ module TokenStyle = {
};
};

module ResolvedStyle = {
type t = {
foreground: string,
background: string,
bold: bool,
italic: bool,
};

let default = (~foreground, ~background, ()) => {
foreground,
background,
bold: false,
italic: false,
};
};

module Selector = {
type t = {
scopes: Scopes.t,
Expand Down
232 changes: 97 additions & 135 deletions src/reason-textmate/TokenTheme.re
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ let create =

let trie =
List.fold_left(
(prev, curr) => {
(prev: Trie.t(selectorWithParents), curr) => {
open Selector;
let {scopes, style} = curr;

Expand Down Expand Up @@ -206,150 +206,112 @@ let show = (v: t) => {
);
};

let _applyStyle: (TokenStyle.t, TokenStyle.t) => TokenStyle.t =
(prev: TokenStyle.t, style: TokenStyle.t) => {
let foreground =
switch (prev.foreground, style.foreground) {
| (Some(v), _) => Some(v)
| (_, Some(v)) => Some(v)
| _ => None
};

let background =
switch (prev.background, style.background) {
| (Some(v), _) => Some(v)
| (_, Some(v)) => Some(v)
| _ => None
};
let match = (theme: t, scopes: string) => {
let scopes = Scopes.ofString(scopes) |> List.rev;

let bold =
switch (prev.bold, style.bold) {
| (Some(v), _) => Some(v)
| (_, Some(v)) => Some(v)
| _ => None
let rec calculateStyle = (~parentScopes, ~acc: list(TokenStyle.t), scopes) => {
switch (scopes) {
| [] => acc
| [scope, ...nextScope] =>
// Get the matching path from the Trie
let matchingPath = Trie.matches(theme.trie, scope);

if (matchingPath == []) {
// No matches - let's try the next scope!
calculateStyle(
~parentScopes=[scope, ...parentScopes],
~acc,
nextScope,
);
} else {
let maybeTokenStyle: option(TokenStyle.t) =
matchingPath
|> List.fold_left(
(maybePrev: option(TokenStyle.t), curr) => {
let (_name, selector: option(selectorWithParents)) = curr;

switch (selector) {
// No selector at this node. This can happen when a node is on the
// path to a node with a style. Nothing to do here; continue on.
| None => maybePrev
// We have a selector at this node. Let's check it out.
| Some({style, parents}) =>
let prevStyle =
maybePrev |> Option.value(~default=TokenStyle.default);

// Get the list of matching parent selectors to apply
let parentsScopesToApply =
parents
|> List.filter(selector =>
Selector.matches(selector, parentScopes)
);

switch (parentsScopesToApply, style) {
// Case 1: No parent selectors match AND there is no style. We should continue on.
| ([], None) => None

// Case 2: No parent selectors match, but there is a style at the Node. We should apply the style.
| ([], Some(style)) =>
Some(TokenStyle.merge(prevStyle, style))

// Case 3: We have parent selectors that match, and may or may not have a style at the node.
// Apply the parent styles, and the node style, if applicable.
| (_, maybeStyle) =>
let newStyle =
maybeStyle
|> Option.map(TokenStyle.merge(prevStyle))
|> Option.value(~default=TokenStyle.default);

// Apply any parent selectors that match...
// we should be sorting this by score!
Some(
List.fold_left(
(prev, curr: Selector.t) => {
open Selector;
let {style, _} = curr;
// Reversing the order because the parent style
// should take precedence over previous style
TokenStyle.merge(style, prev);
},
newStyle,
parentsScopesToApply,
),
);
};
};
},
None,
);

let acc =
maybeTokenStyle
|> Option.map(tokenStyle => [tokenStyle, ...acc])
|> Option.value(~default=acc);

calculateStyle(
~parentScopes=[scope, ...parentScopes],
~acc,
nextScope,
);
};
};
};

let italic =
switch (prev.italic, style.italic) {
| (Some(v), _) => Some(v)
| (_, Some(v)) => Some(v)
| _ => None
};
let styles = calculateStyle(~parentScopes=[], ~acc=[], scopes);

{background, foreground, bold, italic};
};
let result: TokenStyle.t =
styles
|> List.fold_left(
(acc, style) => {TokenStyle.merge(acc, style)},
TokenStyle.default,
);

let match = (theme: t, scopes: string) => {
let scopes = Scopes.ofString(scopes) |> List.rev;
let default =
ResolvedStyle.default(
~foreground=theme.defaultForeground,
~background=theme.defaultBackground,
(),
);

let rec f = scopes => {
switch (scopes) {
| [] => default
| [scope, ...scopeParents] =>
// Get the matching path from the Trie
let p = Trie.matches(theme.trie, scope);

switch (p) {
// If there were no matches... try the next scope up.
| [] => f(scopeParents)
// Got matches - we'll apply them in sequence
| _ =>
let result =
List.fold_left(
(prev: option(TokenStyle.t), curr) => {
let (_name, selector: option(selectorWithParents)) = curr;

switch (selector) {
// No selector at this node. This can happen when a node is on the
// path to a node with a style. Nothing to do here; continue on.
| None => prev
// We have a selector at this node. Let's check it out.
| Some({style, parents}) =>
let prevStyle =
switch (prev) {
| None => TokenStyle.default
| Some(v) => v
};

// Get the list of matching parent selectors to apply
let parentsScopesToApply =
parents
|> List.filter(selector =>
Selector.matches(selector, scopeParents)
);

switch (parentsScopesToApply, style) {
// Case 1: No parent selectors match AND there is no style. We should continue on.
| ([], None) => None
// Case 2: No parent selectors match, but there is a style at the Node. We should apply the style.
| ([], Some(style)) => Some(_applyStyle(prevStyle, style))
// Case 3: We have parent selectors that match, and may or may not have a style at the node.
// Apply the parent styles, and the node style, if applicable.
| (_, style) =>
let newStyle =
switch (style) {
| Some(v) => _applyStyle(prevStyle, v)
| None => TokenStyle.default
};
// Apply any parent selectors that match...
// we should be sorting this by score!
Some(
List.fold_left(
(prev, curr: Selector.t) => {
open Selector;
let {style, _} = curr;
// Reversing the order because the parent style
// should take precedence over previous style
_applyStyle(style, prev);
},
newStyle,
parentsScopesToApply,
),
);
};
};
},
None,
p,
);

switch (result) {
| None => f(scopeParents)
| Some(result) =>
let foreground =
switch (result.foreground) {
| Some(v) => v
| None => default.foreground
};

let bold =
switch (result.bold) {
| Some(v) => v
| None => default.bold
};

let italic =
switch (result.italic) {
| Some(v) => v
| None => default.italic
};

let background =
switch (result.background) {
| Some(v) => v
| None => default.background
};

{background, foreground, bold, italic};
};
};
};
};
f(scopes);
TokenStyle.resolve(~default, result);
};
Loading

0 comments on commit cbc1123

Please sign in to comment.