[clangd] Added a TextMate theme parser to the vscode extension.

[jvikstrom]

Summary

Adds a TextMate parser module to the vscode extension. It parses a theme into an array of a pair of TextMate scopes and text colors.

[hokein]

Haven't looked at the patch in details.

Looks like the patch is doing different things, and the test just covers a small set of the code.

1. find and parse the theme definition files json ;
2. define related data structures to save the TM scopes and do the transformation;
3. handle changes of the theme;

could we narrow it further? I think we could just implement 1) for this patch.

clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts

11 ↗

(On Diff #213319)

The color word here is ambiguous -- we have different colors in DecorationRenderOptions, e.g. color, backgroundColor, borderColor, I believe you meant color, maybe we could just a more descriptive name, like highlightColor, or TextColor?

I think we may want to use a structure here (color is one of the field), so that the code is extensible to support more options in DecorationRenderOptions.

12 ↗

(On Diff #213319)

what does the index mean here?

119 ↗

(On Diff #213319)

I think we may define a type/interface (like TokenColorRule) for the entries (tokenColors) in the theme file, and have a function that parsing the content and returning an array of TokenColorRule?

148 ↗

(On Diff #213319)

note that json is one of the theme format files, vscode also supports .tmTheme. We probably don't support tmTheme for now (just add a fixme).

[jvikstrom]

Narrowed CL down to loading/parsing a text mate theme.

[jvikstrom]

In D65738#1614897, @hokein wrote:

Haven't looked at the patch in details.

Looks like the patch is doing different things, and the test just covers a small set of the code.

1. find and parse the theme definition files json ;
2. define related data structures to save the TM scopes and do the transformation;
3. handle changes of the theme;

could we narrow it further? I think we could just implement 1) for this patch.

Removed everything that wasn't loading a TM theme. Also added an interface for a TM scope that we parse into (which I also added a test for).
The getFullNamedTheme function is not used (or exported) anywhere for now though.

[hokein]

clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts

6 ↗

(On Diff #213528)

I think we should not use the namespace in typescript. The namespace in TS refers to “Internal modules”.

Each TS file is a separate module, so semantically namespace your code, use separate files. For semantic highlighting feature, I think we put all into one file semantic-highlighting.ts.

7 ↗

(On Diff #213528)

TokenColorRule seems a bit clearer to me.

24 ↗

(On Diff #213528)

we are introducing 2~3 helper functions here, I think we could simplify it like below.

function loadTheme(themeName) : Promise<ScopeColorRule[]> {
   // iterate all extensions and find the file path of the theme
   // call parseThemeFile().
}

function parseThemeFile(themeFilePath) : Promise<ScopeColorRule[]> {
  // read the file and parse the file content (recusively)
}

And for the test, we could save a small theme file in the test directory, and pass the file path to the parseThemeFile function.

75 ↗

(On Diff #213528)

nit: we should filter the .tmTheme out in the code (just checking the extension of the fullpath).

93 ↗

(On Diff #213528)

nit: use the full name for the parameters, resolve, reject.

96 ↗

(On Diff #213528)

if we have err, we will run both rej and res.

[jvikstrom]

Renamed file to semantic-highlighting.ts. Added test for parsing theme files. Inlined the parsing into the loading function.

[jvikstrom]

Clarified comments.

[hokein]

thanks, looks simpler now, just a few more nits.

clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts

7 ↗

(On Diff #213568)

nit: we need documentation for the fields.

11 ↗

(On Diff #213568)

do we need async here? since we don't use any await in the function body.

25 ↗

(On Diff #213568)

nit: name it themeData or themeInfo?

31 ↗

(On Diff #213568)

nit: textmate grammar is another different thing, the file is the textmate theme.

35 ↗

(On Diff #213568)

nit: maybe SeenScopes?

42 ↗

(On Diff #213568)

nit: th => tm

45 ↗

(On Diff #213568)

I think it would be more suitable to move this comment in the catch block below, we don't throw this error.

94 ↗

(On Diff #213568)

nit: return resolve(data).

clang-tools-extra/clangd/clients/clangd-vscode/test/assets/firstIncludedTheme.jsonc

3 ↗

(On Diff #213568)

nit: any reason use jsonc not json?

clang-tools-extra/clangd/clients/clangd-vscode/test/assets/secondIncludedTheme.jsonc

2 ↗

(On Diff #213568)

since we don't support tmTheme now, I'd prefer just dropping it from the test.

clang-tools-extra/clangd/clients/clangd-vscode/test/assets/testTheme.jsonc

3 ↗

(On Diff #213568)

I think using one-level test is sufficient, how about the tests below?

• simpleTheme.json (a simple non-include theme)
• includeTheme.json (one-level include them)

clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts

1 ↗

(On Diff #213568)

this comment doesn't seem to provide much information, I'd remove it.

[jvikstrom]

Address comments.

clang-tools-extra/clangd/clients/clangd-vscode/test/assets/firstIncludedTheme.jsonc

3 ↗

(On Diff #213568)

By default vscode will bind .json files to normal json files which don't allow comments. So if you'd try to run the tests without having set .json to bind to json with comments than it will be a "compile error" because of vscode not allowing comments in .json.

[hokein]

mostly good, please update the patch description.

clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts

6 ↗

(On Diff #213801)

the interface also needs a documentation.

7 ↗

(On Diff #213801)

maybe: // A TextMate scope that specifies the context of the token, e.g. "entity.name.function.cpp"

9 ↗

(On Diff #213801)

I know the current name was my suggestion, but rethinking the name here, I think it'd be better to align with the name used by vscode (to avoid confusion), just use foreground.

13 ↗

(On Diff #213801)

just // Get all token color rules provided by the theme

34 ↗

(On Diff #213801)

nit: I think there is no need to explicitly mention the Recursively.

we are using TextMate and TM to refer the same thing in the source file, could we use a consistent way (just use TextMate)?

clang-tools-extra/clangd/clients/clangd-vscode/test/assets/firstIncludedTheme.jsonc

3 ↗

(On Diff #213568)

thanks for the explanations, fair enough.

clang-tools-extra/trunk/clangd/clients/clangd-vscode/package.json

     "scripts": {
         "vscode:prepublish": "tsc -p ./",
         "compile": "tsc -watch -p ./",
         "postinstall": "node ./node_modules/vscode/bin/install",
         "format": "clang-format --style=LLVM -i --glob=\"{src,test}/*.ts\"",
         "test": "node ./node_modules/vscode/bin/test"
     },
     "dependencies": {
+        "jsonc-parser": "^2.1.0",
         "vscode-languageclient": "^5.3.0-next.6",
         "vscode-languageserver": "^5.3.0-next.6"
     },
     "devDependencies": {
         "@types/mocha": "^2.2.32",
         "@types/node": "^6.0.40",
-        "mocha": "^5.2.0",
         "clang-format": "1.2.4",
+        "mocha": "^5.2.0",
         "typescript": "^2.0.3",
         "vscode": "^1.1.0"
     },
     "repository": {
         "type": "svn",
         "url": "http://llvm.org/svn/llvm-project/clang-tools-extra/trunk/clangd/clients/clangd-vscode/"
     },
     "contributes": {

clang-tools-extra/trunk/clangd/clients/clangd-vscode/src/semantic-highlighting.ts

+import * as fs from 'fs';
+import * as jsonc from "jsonc-parser";
+import * as path from 'path';
+import * as vscode from 'vscode';
+
+// A rule for how to color TextMate scopes.
+interface TokenColorRule {
+  // A TextMate scope that specifies the context of the token, e.g.
+  // "entity.name.function.cpp".
+  scope: string;
+  // foreground is the color tokens of this scope should have.
+  foreground: string;
+}
+
+// Get all token color rules provided by the theme.
+function loadTheme(themeName: string): Promise<TokenColorRule[]> {
+  const extension =
+      vscode.extensions.all.find((extension: vscode.Extension<any>) => {
+        const contribs = extension.packageJSON.contributes;
+        if (!contribs || !contribs.themes)
+          return false;
+        return contribs.themes.some((theme: any) => theme.id === themeName ||
+                                                    theme.label === themeName);
+      });
+
+  if (!extension) {
+    return Promise.reject('Could not find a theme with name: ' + themeName);
+  }
+
+  const themeInfo = extension.packageJSON.contributes.themes.find(
+      (theme: any) => theme.id === themeName || theme.label === themeName);
+  return parseThemeFile(path.join(extension.extensionPath, themeInfo.path));
+}
+
+/**
+ * Parse the TextMate theme at fullPath. If there are multiple TextMate scopes
+ * of the same name in the include chain only the earliest entry of the scope is
+ * saved.
+ * @param fullPath The absolute path to the theme.
+ * @param seenScopes A set containing the name of the scopes that have already
+ *     been set.
+ */
+export async function parseThemeFile(
+    fullPath: string, seenScopes?: Set<string>): Promise<TokenColorRule[]> {
+  if (!seenScopes)
+    seenScopes = new Set();
+  // FIXME: Add support for themes written as .tmTheme.
+  if (path.extname(fullPath) === '.tmTheme')
+    return [];
+  try {
+    const contents = await readFileText(fullPath);
+    const parsed = jsonc.parse(contents);
+    const rules: TokenColorRule[] = [];
+    // To make sure it does not crash if tokenColors is undefined.
+    if (!parsed.tokenColors)
+      parsed.tokenColors = [];
+    parsed.tokenColors.forEach((rule: any) => {
+      if (!rule.scope || !rule.settings || !rule.settings.foreground)
+        return;
+      const textColor = rule.settings.foreground;
+      // Scopes that were found further up the TextMate chain should not be
+      // overwritten.
+      const addColor = (scope: string) => {
+        if (seenScopes.has(scope))
+          return;
+        rules.push({scope, foreground : textColor});
+        seenScopes.add(scope);
+      };
+      if (rule.scope instanceof Array) {
+        return rule.scope.forEach((s: string) => addColor(s));
+      }
+      addColor(rule.scope);
+    });
+
+    if (parsed.include)
+      // Get all includes and merge into a flat list of parsed json.
+      return [
+        ...(await parseThemeFile(
+            path.join(path.dirname(fullPath), parsed.include), seenScopes)),
+        ...rules
+      ];
+    return rules;
+  } catch (err) {
+    // If there is an error opening a file, the TextMate files that were
+    // correctly found and parsed further up the chain should be returned.
+    // Otherwise there will be no highlightings at all.
+    console.warn('Could not open file: ' + fullPath + ', error: ', err);
+  }
+
+  return [];
+}
+
+function readFileText(path: string): Promise<string> {
+  return new Promise((resolve, reject) => {
+    fs.readFile(path, 'utf8', (err, data) => {
+      if (err) {
+        return reject(err);
+      }
+      return resolve(data);
+    });
+  });
+}

clang-tools-extra/trunk/clangd/clients/clangd-vscode/test/assets/includeTheme.jsonc

+{
+    // Some comment
+    "include": "simpleTheme.jsonc",
+    "name": "TestTheme",
+    "type": "dark",
+    "colors": {
+        "dropdown.background": "#fff"
+    },
+    "tokenColors": [
+        {
+            "settings": {
+                "foreground": "#fff"
+            }
+        },
+        {
+            "scope": "a",
+            "settings": {
+                "foreground": "#fff"
+            }
+        },
+        {
+            "scope": ["a", "b"],
+            "settings": {
+                "foreground": "#000"
+            }
+        }
+    ]
+}

clang-tools-extra/trunk/clangd/clients/clangd-vscode/test/assets/simpleTheme.jsonc

+{
+    // Some comment
+    "tokenColors": [
+        {
+            "scope": "a",
+            "settings": {
+                "foreground": "#ff0000"
+            }
+        },
+        {
+            "scope": "c",
+            "settings": {
+                "foreground": "#bcd"
+            }
+        }
+    ]
+}

clang-tools-extra/trunk/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts

+import * as assert from 'assert';
+import * as path from 'path';
+
+import * as TM from '../src/semantic-highlighting';
+
+suite('TextMate Tests', () => {
+  test('Parses arrays of textmate themes.', async () => {
+    const themePath =
+        path.join(__dirname, '../../test/assets/includeTheme.jsonc');
+    const scopeColorRules = await TM.parseThemeFile(themePath);
+    const getScopeRule = (scope: string) =>
+        scopeColorRules.find((v) => v.scope === scope);
+    assert.equal(scopeColorRules.length, 3);
+    assert.deepEqual(getScopeRule('a'), {scope : 'a', textColor : '#fff'});
+    assert.deepEqual(getScopeRule('b'), {scope : 'b', textColor : '#000'});
+    assert.deepEqual(getScopeRule('c'), {scope : 'c', textColor : '#bcd'});
+  });
+});