From 3778a696e414913fd7232e128d8dd1c742e5130a Mon Sep 17 00:00:00 2001 From: sebthom Date: Sun, 7 Jul 2024 19:00:15 +0200 Subject: [PATCH] feat: add Theme child combinator ">" (and fix a specificity bug) This is ported from upstream vscode-textmate PR https://github.com/microsoft/vscode-textmate/pull/233 --- .../internal/theme/ThemeResolvingTest.java | 69 ++++++++++- .../tm4e/core/internal/theme/Theme.java | 43 +++++-- .../core/internal/theme/ThemeTrieElement.java | 110 +++++++++++------- .../internal/theme/ThemeTrieElementRule.java | 5 +- 4 files changed, 168 insertions(+), 59 deletions(-) diff --git a/org.eclipse.tm4e.core.tests/src/main/java/org/eclipse/tm4e/core/internal/theme/ThemeResolvingTest.java b/org.eclipse.tm4e.core.tests/src/main/java/org/eclipse/tm4e/core/internal/theme/ThemeResolvingTest.java index f1864da7a..52690f6ce 100644 --- a/org.eclipse.tm4e.core.tests/src/main/java/org/eclipse/tm4e/core/internal/theme/ThemeResolvingTest.java +++ b/org.eclipse.tm4e.core.tests/src/main/java/org/eclipse/tm4e/core/internal/theme/ThemeResolvingTest.java @@ -20,6 +20,7 @@ import java.util.List; +import org.eclipse.tm4e.core.internal.grammar.ScopeStack; import org.eclipse.tm4e.core.internal.utils.StringUtils; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.MethodOrderer; @@ -347,6 +348,72 @@ public void testRulesWithParentScopes() { @Test @Order(15) + @DisplayName("Theme resolving a rule with child combinator") + public void testRuleWithChildCombinator() throws Exception { + final var theme = createTheme(""" + {"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" } }, + ]}"""); + + final var colorMap = theme.getColorMap(); + + interface Matcher { + String match(String... path); + } + + final Matcher matcher = (String[] path) -> { + final var result = theme.match(ScopeStack.from(path)); + if (result == null || result.foregroundId == 0) + return null; + return colorMap.get(result.foregroundId); + }; + + assertEquals(matcher.match("b", "a"), "#300000", "b a"); + assertEquals(matcher.match("b", "c", "a"), "#200000", "b c a"); + assertEquals(matcher.match("c", "b", "a"), "#400000", "c b a"); + assertEquals(matcher.match("c", "b", "d", "a"), "#200000", "c b d a"); + } + + @Test + @Order(16) + @DisplayName("Theme resolving should give deeper scopes higher specificity (#233)") + public void testGiveDeeperScopesHigherSpecificity() throws Exception { + final var theme = createTheme(""" + {"settings": [ + { "settings": { "foreground": "#100000" } }, + { "scope": "y.z a.b", "settings": { "foreground": "#200000" } }, + { "scope": "x y a.b", "settings": { "foreground": "#300000" } }, + ]}"""); + + final var colorMap = theme.getColorMap(); + + interface Matcher { + String match(String... path); + } + + final Matcher matcher = (String[] path) -> { + final var result = theme.match(ScopeStack.from(path)); + if (result == null || result.foregroundId == 0) + return null; + return colorMap.get(result.foregroundId); + }; + + assertEquals(matcher.match("x", "a.b"), null, "x a.b"); + assertEquals(matcher.match("y", "a.b"), null, "y a.b"); + assertEquals(matcher.match("y.z", "a"), null, "y.z a"); + assertEquals(matcher.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. + assertEquals(matcher.match("x", "y.z", "a.b"), "#200000", "y.z a.b"); + } + + @Test + @Order(17) @DisplayName("Theme resolving issue #38: ignores rules with invalid colors") public void testIssue_38_ignores_rules_with_invalid_colors() throws Exception { final var actual = parseTheme(""" @@ -404,7 +471,7 @@ public void testIssue_38_ignores_rules_with_invalid_colors() throws Exception { } @Test - @Order(16) + @Order(18) @DisplayName("Theme resolving issue #35: Trailing comma in a tmTheme scope selector") public void testIssue_35_Trailing_comma_in_a_tmTheme_scope_selector() throws Exception { final var actual = parseTheme(""" diff --git a/org.eclipse.tm4e.core/src/main/java/org/eclipse/tm4e/core/internal/theme/Theme.java b/org.eclipse.tm4e.core/src/main/java/org/eclipse/tm4e/core/internal/theme/Theme.java index efadec9c7..7c230997d 100644 --- a/org.eclipse.tm4e.core/src/main/java/org/eclipse/tm4e/core/internal/theme/Theme.java +++ b/org.eclipse.tm4e.core/src/main/java/org/eclipse/tm4e/core/internal/theme/Theme.java @@ -98,26 +98,47 @@ public Map getEditorColors() { // custom tm4e code, not from ups effectiveRule.background); } - private boolean _scopePathMatchesParentScopes(@Nullable ScopeStack scopePath, @Nullable final List parentScopeNames) { - if (parentScopeNames == null) { + private boolean _scopePathMatchesParentScopes(@Nullable ScopeStack scopePath, final List parentScopeNames) { + if (parentScopeNames.isEmpty()) { return true; } - var index = 0; - var scopePattern = parentScopeNames.get(index); + // Starting with the deepest parent scope, look for a match in the scope path. + final var parentScopeNamesLen = parentScopeNames.size(); + for (int index = 0; index < parentScopeNamesLen; index++) { + var scopePattern = parentScopeNames.get(index); + boolean scopeMustMatch = false; + + // Check for a child combinator (a parent-child relationship) + if (">".equals(scopePattern)) { + if (index == parentScopeNamesLen - 1) { + // Invalid use of child combinator + return false; + } + scopePattern = parentScopeNames.get(++index); + scopeMustMatch = true; + } - while (scopePath != null) { - if (_matchesScope(scopePath.scopeName, scopePattern)) { - index++; - if (index == parentScopeNames.size()) { - return true; + while (scopePath != null) { + if (_matchesScope(scopePath.scopeName, scopePattern)) { + break; + } + if (scopeMustMatch) { + // If a child combinator was used, the parent scope must match. + return false; } - scopePattern = parentScopeNames.get(index); + scopePath = scopePath.parent; + } + + if (scopePath == null) { + // No more potential matches + return false; } scopePath = scopePath.parent; } - return false; + // All parent scopes were matched. + return true; } private boolean _matchesScope(final String scopeName, final String scopeNamePattern) { diff --git a/org.eclipse.tm4e.core/src/main/java/org/eclipse/tm4e/core/internal/theme/ThemeTrieElement.java b/org.eclipse.tm4e.core/src/main/java/org/eclipse/tm4e/core/internal/theme/ThemeTrieElement.java index 8a230aeec..99966621b 100644 --- a/org.eclipse.tm4e.core/src/main/java/org/eclipse/tm4e/core/internal/theme/ThemeTrieElement.java +++ b/org.eclipse.tm4e.core/src/main/java/org/eclipse/tm4e/core/internal/theme/ThemeTrieElement.java @@ -11,7 +11,6 @@ */ package org.eclipse.tm4e.core.internal.theme; -import static org.eclipse.tm4e.core.internal.utils.MoreCollections.asArrayList; import static org.eclipse.tm4e.core.internal.utils.StringUtils.strArrCmp; import java.util.ArrayList; @@ -56,60 +55,81 @@ public ThemeTrieElement( this._children = children; } - private static List _sortBySpecificity(final List arr) { - if (arr.size() == 1) { - return arr; + private static int _cmpBySpecificity(final ThemeTrieElementRule a, final ThemeTrieElementRule b) { + // 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; } - arr.sort(ThemeTrieElement::_cmpBySpecificity); - return arr; - } - private static int _cmpBySpecificity(final ThemeTrieElementRule a, final ThemeTrieElementRule b) { - if (a.scopeDepth == b.scopeDepth) { - final var aParentScopes = a.parentScopes; - final var bParentScopes = b.parentScopes; - final int aParentScopesLen = aParentScopes == null ? 0 : aParentScopes.size(); - final int bParentScopesLen = bParentScopes == null ? 0 : bParentScopes.size(); - if (aParentScopesLen == bParentScopesLen) { - for (int i = 0; i < aParentScopesLen; i++) { - @SuppressWarnings("null") - final String aScope = aParentScopes.get(i); - @SuppressWarnings("null") - final String bScope = bParentScopes.get(i); - final int aLen = aScope.length(); - final int bLen = bScope.length(); - if (aLen != bLen) { - return bLen - aLen; - } - } + // 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). + int aParentIndex = 0; + int bParentIndex = 0; + + final int aParentScopesSize = a.parentScopes.size(); + final int bParentScopesSize = b.parentScopes.size(); + + while (true) { + // Child combinators don't affect specificity. + if (aParentScopesSize > aParentIndex && ">".equals(a.parentScopes.get(aParentIndex))) { + aParentIndex++; + } + if (bParentScopesSize > bParentIndex && ">".equals(b.parentScopes.get(bParentIndex))) { + bParentIndex++; } - return bParentScopesLen - aParentScopesLen; + + // This is a scope-by-scope comparison, so we need to stop once a rule runs + // out of parent scopes. + if (aParentIndex >= aParentScopesSize || bParentIndex >= bParentScopesSize) { + 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. + final int parentScopeLengthDiff = b.parentScopes.get(bParentIndex).length() - a.parentScopes.get(aParentIndex).length(); + + if (parentScopeLengthDiff != 0) { + return parentScopeLengthDiff; + } + + aParentIndex++; + bParentIndex++; } - return b.scopeDepth - a.scopeDepth; + + // If a depth-first, scope-by-scope comparison resulted in a tie, the rule with + // more parent scopes is considered more specific. + return bParentScopesSize - aParentScopesSize; } public List match(final String scope) { - if ("".equals(scope)) { - return ThemeTrieElement._sortBySpecificity(asArrayList(this._mainRule, this._rulesWithParentScopes)); - } - - final int dotIndex = scope.indexOf('.'); - final String head; - final String tail; - if (dotIndex == -1) { - head = scope; - tail = ""; - } else { - head = scope.substring(0, dotIndex); - tail = scope.substring(dotIndex + 1); - } + if (!scope.isEmpty()) { + final int dotIndex = scope.indexOf('.'); + String head; + String tail; + if (dotIndex == -1) { + head = scope; + tail = ""; + } else { + head = scope.substring(0, dotIndex); + tail = scope.substring(dotIndex + 1); + } - final ThemeTrieElement child = this._children.get(head); - if (child != null) { - return child.match(tail); + final ThemeTrieElement child = this._children.get(head); + if (child != null) { + return child.match(tail); + } } - return ThemeTrieElement._sortBySpecificity(asArrayList(this._mainRule, this._rulesWithParentScopes)); + final var rules = new ArrayList<>(this._rulesWithParentScopes); + rules.add(this._mainRule); + rules.sort(ThemeTrieElement::_cmpBySpecificity); + return rules; } public void insert(final int scopeDepth, final String scope, @Nullable final List parentScopes, final int fontStyle, diff --git a/org.eclipse.tm4e.core/src/main/java/org/eclipse/tm4e/core/internal/theme/ThemeTrieElementRule.java b/org.eclipse.tm4e.core/src/main/java/org/eclipse/tm4e/core/internal/theme/ThemeTrieElementRule.java index d157a47fc..c7a1c0766 100644 --- a/org.eclipse.tm4e.core/src/main/java/org/eclipse/tm4e/core/internal/theme/ThemeTrieElementRule.java +++ b/org.eclipse.tm4e.core/src/main/java/org/eclipse/tm4e/core/internal/theme/ThemeTrieElementRule.java @@ -12,6 +12,7 @@ package org.eclipse.tm4e.core.internal.theme; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Objects; @@ -28,7 +29,7 @@ public class ThemeTrieElementRule { public int scopeDepth; - public final @Nullable List parentScopes; + public final List parentScopes; public int fontStyle; public int foreground; public int background; @@ -36,7 +37,7 @@ public class ThemeTrieElementRule { public ThemeTrieElementRule(final int scopeDepth, final @Nullable List parentScopes, final int fontStyle, final int foreground, final int background) { this.scopeDepth = scopeDepth; - this.parentScopes = parentScopes; + this.parentScopes = parentScopes == null ? Collections.emptyList() : parentScopes; this.fontStyle = fontStyle; this.foreground = foreground; this.background = background;