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

feat: add child combinator ">" (and fix a specificity bug) #233

Merged
merged 7 commits into from
Jul 6, 2024

Conversation

aleclarson
Copy link
Contributor

@aleclarson aleclarson commented Jun 7, 2024

Closes #123

  • Feature: Add support for the child combinator (the > operator). This allows for styling a parent-child relationship specifically.

    // Check for a child combinator (a parent-child relationship)
    if (scopePattern === '>') {
    if (index === parentScopes.length - 1) {
    return false;
    }
    scopePattern = parentScopes[++index];
    scopeMustMatch = true;
    }

    // Child combinators don't affect specificity.
    if (a.parentScopes[aParentIndex] === '>') {
    aParentIndex++;
    }
    if (b.parentScopes[bParentIndex] === '>') {
    bParentIndex++;
    }

  • Bug: If the number of scope names in both rules‘ scope paths are not equal, the parent scope names won‘t be compared at all. Instead, the rule with the longest scope path is preferred. This goes against the TextMate manual (https://macromates.com/manual/en/scope_selectors). In particular, the following line in “Ranking Matches”:

    Rules 1 and 2 applied again to the scope selector when removing the deepest element (in the case of a tie)

    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;

    How did I fix it?
    Do a depth-first, scope-by-scope comparison of the parent scopes, even if one of the rules has a longer scope path.

    // 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;
    }

Tests

Bug: Just because a trie element exists for a more specific scope doesn‘t mean its parent scopes will match, so we need to collect the trie elements with less specific scopes too.

Bug: If the number of scope names in both rules‘ scope paths are not
equal, the parent scope names won‘t be compared at all. Instead, the rule with
the longest scope path is preferred. This goes against the TextMate
manual (https://macromates.com/manual/en/scope_selectors). In
particular, the following line in “Ranking Matches”:
> Rules 1 and 2 applied again to the scope selector when removing the deepest element (in the case of a tie)

Feature: Add support for the child combinator (the `>` operator). This
allows for styling a parent-child relationship specifically.
@aleclarson
Copy link
Contributor Author

@hediet Excuse me if it's rude to tag, but I saw you're the only person to touch src/theme.ts in the last 2 years, so I would very much appreciate a code review if you can find the time! ❤️

Test cases incoming...

@aleclarson aleclarson force-pushed the pr/1 branch 2 times, most recently from 23e9193 to 0e71e37 Compare June 7, 2024 18:50
- One for the new child combinator
- One for bug 1 ("Theme resolving falls back to less specific rules")
- One for bug 2 ("Theme resolving should give deeper scopes higher specificity")
@aleclarson aleclarson changed the title feat: fix 2 bugs and add child combinator ">" feat: add child combinator ">" (and fix 2 bugs) Jun 7, 2024
After trying to reproduce the alleged bug, I realized it‘s not a bug. When a ThemeTrieElement is created, it inherits the `_rulesWithParentScopes` array of its parent element, which is guaranteed to be populated due to the lexicographical sorting of the theme rules in `resolveParsedThemeRules`.
…and update the other bug‘s test case to actually fail on the main branch
@aleclarson aleclarson changed the title feat: add child combinator ">" (and fix 2 bugs) feat: add child combinator ">" (and fix a specificity bug) Jun 7, 2024
@aleclarson
Copy link
Contributor Author

aleclarson commented Jun 7, 2024

Okay tests have been added! Both fail on main branch.

Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very good! Thank you! ❤️

@alexdima alexdima enabled auto-merge (squash) July 6, 2024 08:43
@VSCodeTriageBot VSCodeTriageBot added this to the July 2024 milestone Jul 6, 2024
@alexdima alexdima merged commit 167bbbd into microsoft:main Jul 6, 2024
3 checks passed
sebthom added a commit to sebthom/tm4e that referenced this pull request Jul 6, 2024
sebthom added a commit to sebthom/tm4e that referenced this pull request Jul 7, 2024
sebthom added a commit to eclipse/tm4e that referenced this pull request Jul 8, 2024
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.

TextMate scope selectors: child combinator is not implemented
4 participants