diff --git a/src/tests/themes.test.ts b/src/tests/themes.test.ts index 830fdf2..c7bdea5 100644 --- a/src/tests/themes.test.ts +++ b/src/tests/themes.test.ts @@ -606,6 +606,63 @@ test('Theme resolving rules with parent scopes', () => { assertThemeEqual(actual, expected); }); +test('Theme resolving a rule with child combinator', () => { + let theme = Theme.createFromRawTheme({ + settings: [ + { settings: { foreground: '#100000' } }, + { scope: 'b a', settings: { foreground: '#200000' } }, + { scope: 'b > a', settings: { foreground: '#300000' } }, + { scope: 'c > b > a', settings: { foreground: '#400000' } }, + { scope: 'a', settings: { foreground: '#500000' } }, + ] + }); + + const colorMap = theme.getColorMap(); + const match = (...path: string[]) => { + const result = theme.match(ScopeStack.from(...path)); + if (!result) { + return null; + } + return colorMap[result.foregroundId]; + }; + + assert.equal(match('b', 'a'), '#300000', 'b a'); + assert.equal(match('b', 'c', 'a'), '#200000', 'b c a'); + assert.equal(match('c', 'b', 'a'), '#400000', 'c b a'); + assert.equal(match('c', 'b', 'd', 'a'), '#200000', 'c b d a'); +}); + +test('Theme resolving should give deeper scopes higher specificity (#233)', () => { + let theme = Theme.createFromRawTheme({ + settings: [ + { settings: { foreground: '#100000' } }, + { scope: 'y.z a.b', settings: { foreground: '#200000' } }, + { scope: 'x y a.b', settings: { foreground: '#300000' } }, + ] + }); + + const colorMap = theme.getColorMap(); + const defaults = theme.getDefaults(); + + const match = (...path: string[]) => { + const result = theme.match(ScopeStack.from(...path)); + if (!result || result.foregroundId === 0) { + return null; + } + return colorMap[result.foregroundId]; + }; + + // Sanity check + assert.equal(match('x', 'a.b'), null, 'x a.b'); + assert.equal(match('y', 'a.b'), null, 'y a.b'); + assert.equal(match('y.z', 'a'), null, 'y.z a'); + assert.equal(match('x', 'y', 'a.b'), '#300000', 'x y a.b'); + + // Even though the "x y a.b" rule has more scopes in its path, the "y.z a.b" rule has + // a deeper match, so it should take precedence. + assert.equal(match('x', 'y.z', 'a.b'), '#200000', 'y.z a.b'); +}); + test('Theme resolving issue #38: ignores rules with invalid colors', () => { let actual = parseTheme({ settings: [{ diff --git a/src/theme.ts b/src/theme.ts index 54f8356..f37fe9a 100644 --- a/src/theme.ts +++ b/src/theme.ts @@ -161,26 +161,46 @@ export class ScopeStack { } } -function _scopePathMatchesParentScopes(scopePath: ScopeStack | null, parentScopes: ScopeName[] | null): boolean { - if (parentScopes === null) { +function _scopePathMatchesParentScopes(scopePath: ScopeStack | null, parentScopes: readonly ScopeName[]): boolean { + if (parentScopes.length === 0) { return true; } - let index = 0; - let scopePattern = parentScopes[index]; + // Starting with the deepest parent scope, look for a match in the scope path. + for (let index = 0; index < parentScopes.length; index++) { + let scopePattern = parentScopes[index]; + let scopeMustMatch = false; - while (scopePath) { - if (_matchesScope(scopePath.scopeName, scopePattern)) { - index++; - if (index === parentScopes.length) { - return true; + // Check for a child combinator (a parent-child relationship) + if (scopePattern === '>') { + if (index === parentScopes.length - 1) { + // Invalid use of child combinator + return false; } - scopePattern = parentScopes[index]; + scopePattern = parentScopes[++index]; + scopeMustMatch = true; + } + + while (scopePath) { + if (_matchesScope(scopePath.scopeName, scopePattern)) { + break; + } + if (scopeMustMatch) { + // If a child combinator was used, the parent scope must match. + return false; + } + scopePath = scopePath.parent; + } + + if (!scopePath) { + // No more potential matches + return false; } scopePath = scopePath.parent; } - return false; + // All parent scopes were matched. + return true; } function _matchesScope(scopeName: ScopeName, scopePattern: ScopeName): boolean { @@ -427,17 +447,19 @@ export class ColorMap { } } +const emptyParentScopes= Object.freeze([]); + export class ThemeTrieElementRule { scopeDepth: number; - parentScopes: ScopeName[] | null; + parentScopes: readonly ScopeName[]; fontStyle: number; foreground: number; background: number; - constructor(scopeDepth: number, parentScopes: ScopeName[] | null, fontStyle: number, foreground: number, background: number) { + constructor(scopeDepth: number, parentScopes: readonly ScopeName[] | null, fontStyle: number, foreground: number, background: number) { this.scopeDepth = scopeDepth; - this.parentScopes = parentScopes; + this.parentScopes = parentScopes || emptyParentScopes; this.fontStyle = fontStyle; this.foreground = foreground; this.background = background; @@ -489,55 +511,77 @@ export class ThemeTrieElement { this._rulesWithParentScopes = rulesWithParentScopes; } - private static _sortBySpecificity(arr: ThemeTrieElementRule[]): ThemeTrieElementRule[] { - if (arr.length === 1) { - return arr; - } - arr.sort(this._cmpBySpecificity); - return arr; - } - private static _cmpBySpecificity(a: ThemeTrieElementRule, b: ThemeTrieElementRule): number { - if (a.scopeDepth === b.scopeDepth) { - const aParentScopes = a.parentScopes; - const bParentScopes = b.parentScopes; - let aParentScopesLen = aParentScopes === null ? 0 : aParentScopes.length; - let bParentScopesLen = bParentScopes === null ? 0 : bParentScopes.length; - if (aParentScopesLen === bParentScopesLen) { - for (let i = 0; i < aParentScopesLen; i++) { - const aLen = aParentScopes![i].length; - const bLen = bParentScopes![i].length; - if (aLen !== bLen) { - return bLen - aLen; - } - } - } - return bParentScopesLen - aParentScopesLen; + // First, compare the scope depths of both rules. The “scope depth” of a rule is + // the number of segments (delimited by dots) in the rule's deepest scope name + // (i.e. the final scope name in the scope path delimited by spaces). + if (a.scopeDepth !== b.scopeDepth) { + return b.scopeDepth - a.scopeDepth; } - return b.scopeDepth - a.scopeDepth; + + // Traverse the parent scopes depth-first, comparing the specificity of both + // rules' parent scopes, which matches the behavior described by ”Ranking Matches” + // in TextMate 1.5's manual: https://macromates.com/manual/en/scope_selectors + // Start at index 0 for both rules, since the parent scopes were reversed + // beforehand (i.e. index 0 is the deepest parent scope). + let aParentIndex = 0; + let bParentIndex = 0; + + while (true) { + // Child combinators don't affect specificity. + if (a.parentScopes[aParentIndex] === '>') { + aParentIndex++; + } + if (b.parentScopes[bParentIndex] === '>') { + bParentIndex++; + } + + // This is a scope-by-scope comparison, so we need to stop once a rule runs + // out of parent scopes. + if (aParentIndex >= a.parentScopes.length || bParentIndex >= b.parentScopes.length) { + break; + } + + // When sorting by scope name specificity, it's safe to treat a longer parent + // scope as more specific. If both rules' parent scopes match a given scope + // path, the longer parent scope will always be more specific. + const parentScopeLengthDiff = + b.parentScopes[bParentIndex].length - a.parentScopes[aParentIndex].length; + + if (parentScopeLengthDiff !== 0) { + return parentScopeLengthDiff; + } + + aParentIndex++; + bParentIndex++; + } + + // If a depth-first, scope-by-scope comparison resulted in a tie, the rule with + // more parent scopes is considered more specific. + return b.parentScopes.length - a.parentScopes.length; } public match(scope: ScopeName): ThemeTrieElementRule[] { - if (scope === '') { - return ThemeTrieElement._sortBySpecificity(([]).concat(this._mainRule).concat(this._rulesWithParentScopes)); + if (scope !== '') { + let dotIndex = scope.indexOf('.') + let head: string + let tail: string + if (dotIndex === -1) { + head = scope + tail = '' + } else { + head = scope.substring(0, dotIndex) + tail = scope.substring(dotIndex + 1) + } + + if (this._children.hasOwnProperty(head)) { + return this._children[head].match(tail); + } } - let dotIndex = scope.indexOf('.'); - let head: string; - let tail: string; - if (dotIndex === -1) { - head = scope; - tail = ''; - } else { - head = scope.substring(0, dotIndex); - tail = scope.substring(dotIndex + 1); - } - - if (this._children.hasOwnProperty(head)) { - return this._children[head].match(tail); - } - - return ThemeTrieElement._sortBySpecificity(([]).concat(this._mainRule).concat(this._rulesWithParentScopes)); + const rules = this._rulesWithParentScopes.concat(this._mainRule); + rules.sort(ThemeTrieElement._cmpBySpecificity); + return rules; } public insert(scopeDepth: number, scope: ScopeName, parentScopes: ScopeName[] | null, fontStyle: number, foreground: number, background: number): void { diff --git a/src/utils.ts b/src/utils.ts index 46ae13b..7881b37 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -105,7 +105,7 @@ export function strcmp(a: string, b: string): number { return 0; } -export function strArrCmp(a: string[] | null, b: string[] | null): number { +export function strArrCmp(a: readonly string[] | null, b: readonly string[] | null): number { if (a === null && b === null) { return 0; }