[PATCH] D65738: [clangd] Added a TextMate theme parser that updates when the current theme changes.

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

mostly good, please update the patch description.




Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:6
+
+interface TokenColorRule {
+  // Scope is the TextMate scope for this rule.

the interface also needs a documentation.



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:7
+interface TokenColorRule {
+  // Scope is the TextMate scope for this rule.
+  scope: string;

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






Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:9
+  scope: string;
+  // textColor is the color tokens of this scope should have.
+  textColor: string;

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`.



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:13
+
+// Gets a TextMate theme and all its included themes by its name.
+function loadTheme(themeName: string): Promise {

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



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:34
+/**
+ * Recursively parse the TM theme at fullPath. If there are multiple TM scopes
+ * of the same name in the include chain only the earliest entry of the scope 
is

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`)?



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/test/assets/firstIncludedTheme.jsonc:3
+// Some comment
+"include": "secondIncludedTheme.jsonc",
+"tokenColors": [

jvikstrom wrote:
> hokein wrote:
> > nit: any reason use `jsonc` not `json`?
> > 
> 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.
>  
thanks for the explanations, fair enough.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65738/new/

https://reviews.llvm.org/D65738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65738: [clangd] Added a TextMate theme parser that updates when the current theme changes.

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom added inline comments.



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/test/assets/firstIncludedTheme.jsonc:3
+// Some comment
+"include": "secondIncludedTheme.jsonc",
+"tokenColors": [

hokein wrote:
> nit: any reason use `jsonc` not `json`?
> 
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.
 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65738/new/

https://reviews.llvm.org/D65738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65738: [clangd] Added a TextMate theme parser that updates when the current theme changes.

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom updated this revision to Diff 213801.
jvikstrom marked 12 inline comments as done.
jvikstrom added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65738/new/

https://reviews.llvm.org/D65738

Files:
  clang-tools-extra/clangd/clients/clangd-vscode/package.json
  clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
  clang-tools-extra/clangd/clients/clangd-vscode/test/assets/includeTheme.jsonc
  clang-tools-extra/clangd/clients/clangd-vscode/test/assets/simpleTheme.jsonc
  
clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts

Index: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
@@ -0,0 +1,18 @@
+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'});
+  });
+});
Index: clang-tools-extra/clangd/clients/clangd-vscode/test/assets/simpleTheme.jsonc
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/assets/simpleTheme.jsonc
@@ -0,0 +1,17 @@
+{
+// Some comment
+"tokenColors": [
+{
+"scope": "a",
+"settings": {
+"foreground": "#ff"
+}
+},
+{
+"scope": "c",
+"settings": {
+"foreground": "#bcd"
+}
+}
+]
+}
Index: clang-tools-extra/clangd/clients/clangd-vscode/test/assets/includeTheme.jsonc
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/assets/includeTheme.jsonc
@@ -0,0 +1,28 @@
+{
+// 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"
+}
+}
+]
+}
Index: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
@@ -0,0 +1,100 @@
+import * as fs from 'fs';
+import * as jsonc from "jsonc-parser";
+import * as path from 'path';
+import * as vscode from 'vscode';
+
+interface TokenColorRule {
+  // Scope is the TextMate scope for this rule.
+  scope: string;
+  // textColor is the color tokens of this scope should have.
+  textColor: string;
+}
+
+// Gets a TextMate theme and all its included themes by its name.
+function loadTheme(themeName: string): Promise {
+  const extension =
+  vscode.extensions.all.find((extension: vscode.Extension) => {
+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));
+}
+
+/**
+ * Recursively parse the TM theme at fullPath. If there are multiple TM 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): Promise {
+  if 

[PATCH] D65738: [clangd] Added a TextMate theme parser that updates when the current theme changes.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

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




Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:7
+interface TokenColorRule {
+  scope: string, textColor: string,
+}

nit: we need documentation for the fields.



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:11
+// Gets a TextMate theme and all its included themes by its name.
+async function loadTheme(themeName: string): Promise {
+  const extension =

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



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:25
+
+  const extensionInfo = extension.packageJSON.contributes.themes.find(
+  (theme: any) => theme.id === themeName || theme.label === themeName);

nit: name it `themeData` or `themeInfo`?



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:31
+/**
+ * Recursively parse the TM grammar at fullPath. If there are multiple TM 
scopes
+ * of the same name in the include chain only the earliest entry of the scope 
is

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



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:35
+ * @param fullPath The absolute path to the theme.
+ * @param scopeSet A set containing the name of the scopes that have already
+ * been set.

nit: maybe `SeenScopes`?



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:42
+scopeSet = new Set();
+  // FIXME: Add support for themes written as .thTheme.
+  if (path.extname(fullPath) === '.tmTheme')

nit: th => tm



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:45
+return [];
+  // If there is an error opening a file, the TM files that were correctly 
found
+  // and parsed further up the chain should be returned. Otherwise there will 
be

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



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts:94
+  }
+  resolve(data);
+});

nit: return resolve(data).



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/test/assets/firstIncludedTheme.jsonc:3
+// Some comment
+"include": "secondIncludedTheme.jsonc",
+"tokenColors": [

nit: any reason use `jsonc` not `json`?




Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/test/assets/secondIncludedTheme.jsonc:2
+{
+"include": "tmThemeInclude.tmTheme",
+"tokenColors": [

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



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/test/assets/testTheme.jsonc:3
+// Some comment
+"include": "firstIncludedTheme.jsonc",
+"name": "TestTheme",

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)



Comment at: 
clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts:1
+/** The module 'assert' provides assertion methods from node */
+import * as assert from 'assert';

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65738/new/

https://reviews.llvm.org/D65738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65738: [clangd] Added a TextMate theme parser that updates when the current theme changes.

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom updated this revision to Diff 213568.
jvikstrom added a comment.

Clarified comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65738/new/

https://reviews.llvm.org/D65738

Files:
  clang-tools-extra/clangd/clients/clangd-vscode/package.json
  clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
  
clang-tools-extra/clangd/clients/clangd-vscode/test/assets/firstIncludedTheme.jsonc
  
clang-tools-extra/clangd/clients/clangd-vscode/test/assets/secondIncludedTheme.jsonc
  clang-tools-extra/clangd/clients/clangd-vscode/test/assets/testTheme.jsonc
  
clang-tools-extra/clangd/clients/clangd-vscode/test/assets/tmThemeInclude.tmTheme
  
clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts

Index: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
@@ -0,0 +1,17 @@
+/** The module 'assert' provides assertion methods from node */
+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/testTheme.jsonc');
+const scopeColorRules = await TM.parseThemeFile(themePath);
+const getScopeRule = (scope: string) =>
+scopeColorRules.find((v) => v.scope === scope);
+assert.equal(scopeColorRules.length, 2);
+assert.deepEqual(getScopeRule('a'), {scope : 'a', textColor : '#fff'});
+assert.deepEqual(getScopeRule('b'), {scope : 'b', textColor : '#000'});
+  });
+});
\ No newline at end of file
Index: clang-tools-extra/clangd/clients/clangd-vscode/test/assets/tmThemeInclude.tmTheme
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/assets/tmThemeInclude.tmTheme
@@ -0,0 +1,5 @@
+
+http://www.apple.com/DTDs/PropertyList-1.0.dtd;>
+
+
+
Index: clang-tools-extra/clangd/clients/clangd-vscode/test/assets/testTheme.jsonc
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/assets/testTheme.jsonc
@@ -0,0 +1,16 @@
+{
+// Some comment
+"include": "firstIncludedTheme.jsonc",
+"name": "TestTheme",
+"type": "dark",
+"colors": {
+"dropdown.background": "#fff"
+},
+"tokenColors": [
+{
+"settings": {
+"foreground": "#fff"
+}
+}
+]
+}
Index: clang-tools-extra/clangd/clients/clangd-vscode/test/assets/secondIncludedTheme.jsonc
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/assets/secondIncludedTheme.jsonc
@@ -0,0 +1,11 @@
+{
+"include": "tmThemeInclude.tmTheme",
+"tokenColors": [
+{
+"scope": "a",
+"settings": {
+"foreground": "#ff"
+}
+}
+]
+}
Index: clang-tools-extra/clangd/clients/clangd-vscode/test/assets/firstIncludedTheme.jsonc
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/assets/firstIncludedTheme.jsonc
@@ -0,0 +1,18 @@
+{
+// Some comment
+"include": "secondIncludedTheme.jsonc",
+"tokenColors": [
+{
+"scope": "a",
+"settings": {
+"foreground": "#fff"
+}
+},
+{
+"scope": ["a", "b"],
+"settings": {
+"foreground": "#000"
+}
+}
+]
+}
Index: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
@@ -0,0 +1,97 @@
+import * as fs from 'fs';
+import * as jsonc from "jsonc-parser";
+import * as path from 'path';
+import * as vscode from 'vscode';
+
+interface TokenColorRule {
+  scope: string, textColor: string,
+}
+
+// Gets a TextMate theme and all its included themes by its name.
+async function loadTheme(themeName: string): Promise {
+  const extension =
+  vscode.extensions.all.find((extension: vscode.Extension) => {
+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 extensionInfo 

[PATCH] D65738: [clangd] Added a TextMate theme parser that updates when the current theme changes.

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom updated this revision to Diff 213567.
jvikstrom marked 6 inline comments as done.
jvikstrom added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65738/new/

https://reviews.llvm.org/D65738

Files:
  clang-tools-extra/clangd/clients/clangd-vscode/package.json
  clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
  
clang-tools-extra/clangd/clients/clangd-vscode/test/assets/firstIncludedTheme.jsonc
  
clang-tools-extra/clangd/clients/clangd-vscode/test/assets/secondIncludedTheme.jsonc
  clang-tools-extra/clangd/clients/clangd-vscode/test/assets/testTheme.jsonc
  
clang-tools-extra/clangd/clients/clangd-vscode/test/assets/tmThemeInclude.tmTheme
  
clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts

Index: clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/semantic-highlighting.test.ts
@@ -0,0 +1,17 @@
+/** The module 'assert' provides assertion methods from node */
+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/testTheme.jsonc');
+const scopeColorRules = await TM.parseThemeFile(themePath);
+const getScopeRule = (scope: string) =>
+scopeColorRules.find((v) => v.scope === scope);
+assert.equal(scopeColorRules.length, 2);
+assert.deepEqual(getScopeRule('a'), {scope : 'a', textColor : '#fff'});
+assert.deepEqual(getScopeRule('b'), {scope : 'b', textColor : '#000'});
+  });
+});
\ No newline at end of file
Index: clang-tools-extra/clangd/clients/clangd-vscode/test/assets/tmThemeInclude.tmTheme
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/assets/tmThemeInclude.tmTheme
@@ -0,0 +1,5 @@
+
+http://www.apple.com/DTDs/PropertyList-1.0.dtd;>
+
+
+
Index: clang-tools-extra/clangd/clients/clangd-vscode/test/assets/testTheme.jsonc
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/assets/testTheme.jsonc
@@ -0,0 +1,16 @@
+{
+// Some comment
+"include": "firstIncludedTheme.jsonc",
+"name": "TestTheme",
+"type": "dark",
+"colors": {
+"dropdown.background": "#fff"
+},
+"tokenColors": [
+{
+"settings": {
+"foreground": "#fff"
+}
+}
+]
+}
Index: clang-tools-extra/clangd/clients/clangd-vscode/test/assets/secondIncludedTheme.jsonc
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/assets/secondIncludedTheme.jsonc
@@ -0,0 +1,11 @@
+{
+"include": "tmThemeInclude.tmTheme",
+"tokenColors": [
+{
+"scope": "a",
+"settings": {
+"foreground": "#ff"
+}
+}
+]
+}
Index: clang-tools-extra/clangd/clients/clangd-vscode/test/assets/firstIncludedTheme.jsonc
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/assets/firstIncludedTheme.jsonc
@@ -0,0 +1,18 @@
+{
+// Some comment
+"include": "secondIncludedTheme.jsonc",
+"tokenColors": [
+{
+"scope": "a",
+"settings": {
+"foreground": "#fff"
+}
+},
+{
+"scope": ["a", "b"],
+"settings": {
+"foreground": "#000"
+}
+}
+]
+}
Index: clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/semantic-highlighting.ts
@@ -0,0 +1,94 @@
+import * as fs from 'fs';
+import * as jsonc from "jsonc-parser";
+import * as path from 'path';
+import * as vscode from 'vscode';
+
+interface TokenColorRule {
+  scope: string, textColor: string,
+}
+
+// Gets a TextMate theme by its name and all its included themes.
+async function loadTheme(themeName: string): Promise {
+  const extension =
+  vscode.extensions.all.find((extension: vscode.Extension) => {
+const contribs = extension.packageJSON.contributes;
+if (!contribs || !contribs.themes)
+  return false;
+return contribs.themes.some((theme: any) => theme.id === themeName ||
+theme.label === 

[PATCH] D65738: [clangd] Added a TextMate theme parser that updates when the current theme changes.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:6
+
+export namespace SemanticHighlighting {
+interface ScopeColorRule {

I think we should not use the namespace in typescript. The `namespace` in TS 
refers to [“Internal 
modules”](https://www.typescriptlang.org/docs/handbook/namespaces.html). 

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`. 



Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:7
+export namespace SemanticHighlighting {
+interface ScopeColorRule {
+  scope: string, textColor: string,

TokenColorRule seems a bit clearer to me.



Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:24
+ */
+export function themesToScopeColors(themeContents: any[]): ScopeColorRule[] {
+  const ruleMap: Map = new Map();

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

```
function loadTheme(themeName) : Promise {
   // iterate all extensions and find the file path of the theme
   // call parseThemeFile().
}

function parseThemeFile(themeFilePath) : Promise {
  // 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.




Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:75
+const contents = await readFileText(fullPath);
+// FIXME: Add support for themes written as .thTheme.
+const parsed = jsonc.parse(contents);

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



Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:93
+function readFileText(path: string): Promise {
+  return new Promise((res, rej) => {
+fs.readFile(path, 'utf8', (err, data) => {

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



Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:96
+  if (err) {
+rej(err);
+  }

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65738/new/

https://reviews.llvm.org/D65738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65738: [clangd] Added a TextMate theme parser that updates when the current theme changes.

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65738/new/

https://reviews.llvm.org/D65738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65738: [clangd] Added a TextMate theme parser that updates when the current theme changes.

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom updated this revision to Diff 213528.
jvikstrom marked 2 inline comments as done.
jvikstrom added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65738/new/

https://reviews.llvm.org/D65738

Files:
  clang-tools-extra/clangd/clients/clangd-vscode/package.json
  clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts
  clang-tools-extra/clangd/clients/clangd-vscode/test/TextMate.test.ts

Index: clang-tools-extra/clangd/clients/clangd-vscode/test/TextMate.test.ts
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/TextMate.test.ts
@@ -0,0 +1,27 @@
+/** The module 'assert' provides assertion methods from node */
+import * as assert from 'assert';
+
+import {SemanticHighlighting} from '../src/TextMate';
+
+suite('TextMate Tests', () => {
+  test('Parses arrays of textmate themes.', () => {
+const themes = [
+  {}, {
+tokenColors : [
+  {}, {scope : 'a'},
+  {scope : [ 'b', 'c', 'd' ], settings : {background : '#000'}},
+  {scope : 'a', settings : {foreground : '#000'}},
+  {scope : [ 'a', 'b', 'c' ], settings : {foreground : '#fff'}}
+]
+  },
+  {tokenColors : [ {scope : 'b', settings : {foreground : '#ff'}} ]}
+];
+const scopeColorRules = SemanticHighlighting.themesToScopeColors(themes);
+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 : '#ff'});
+assert.deepEqual(getScopeRule('c'), {scope : 'c', textColor : '#fff'});
+  });
+});
\ No newline at end of file
Index: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts
@@ -0,0 +1,102 @@
+import * as fs from 'fs';
+import * as jsonc from "jsonc-parser";
+import * as path from 'path';
+import * as vscode from 'vscode';
+
+export namespace SemanticHighlighting {
+interface ScopeColorRule {
+  scope: string, textColor: string,
+}
+
+async function getNamedThemeScopeColors(themeName: string):
+Promise {
+  const contents = await getFullNamedTheme(themeName);
+  return themesToScopeColors(contents);
+}
+
+/**
+ * Convert an array of TextMate theme contents into the an array of all scopes'
+ * colors. If there are duplicate entries of a scope only the latest occurence
+ * of the scope is kept.
+ * @param themeContents An entry in this array is either the theme's (or an
+ * included theme's) TextMate definition.
+ */
+export function themesToScopeColors(themeContents: any[]): ScopeColorRule[] {
+  const ruleMap: Map = new Map();
+  themeContents.forEach((content) => {
+if (!content.tokenColors)
+  return;
+content.tokenColors.forEach((rule: any) => {
+  if (!rule.scope || !rule.settings || !rule.settings.foreground)
+return;
+  const textColor = rule.settings.foreground;
+  if (rule.scope instanceof Array) {
+rule.scope.forEach((s: string) => ruleMap.set(s, textColor));
+return;
+  }
+  ruleMap.set(rule.scope, textColor);
+});
+  });
+
+  return Array.from(ruleMap.entries())
+  .map(([ scope, textColor ]) => ({scope, textColor}));
+}
+
+// Gets a TextMate theme by its name and all its included themes.
+async function getFullNamedTheme(themeName: string): Promise {
+  const extension =
+  vscode.extensions.all.find((extension: vscode.Extension) => {
+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 extensionInfo = extension.packageJSON.contributes.themes.find(
+  (theme: any) => theme.id === themeName || theme.label === themeName);
+  return recursiveGetTextMateGrammarPath(
+  path.join(extension.extensionPath, extensionInfo.path));
+}
+
+// TM grammars can include other TM grammars, this function recursively gets all
+// of them.
+async function recursiveGetTextMateGrammarPath(fullPath: string):
+Promise {
+  // If there is an error opening a file, the TM files that were correctly found
+  // and parsed further up the chain should be returned. Otherwise there will be
+  // no highlightings at all.
+  try {
+const contents = await readFileText(fullPath);
+// FIXME: Add support for themes written as .thTheme.
+const parsed = 

[PATCH] D65738: [clangd] Added a TextMate theme parser that updates when the current theme changes.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

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.




Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:11
+  // Mapping from a clangd scope index to a color hex string.
+  private colors: string[];
+  // The current best matching scope for every index.

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`.



Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:12
+  private colors: string[];
+  // The current best matching scope for every index.
+  private colorScopes: string[];

what does the `index` mean here?



Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:119
+// Gets a TextMate theme by its name and all its included themes.
+async function getFullNamedTheme(themeName: string): Promise {
+  const extension =

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`?





Comment at: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts:148
+const contents = await readFileText(fullPath);
+const parsed = jsonc.parse(contents);
+if (parsed.include)

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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65738/new/

https://reviews.llvm.org/D65738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65738: [clangd] Added a TextMate theme parser that updates when the current theme changes.

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom created this revision.
jvikstrom added reviewers: hokein, ilya-biryukov.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay.
Herald added a project: clang.

Adds a TextMate parser module to the vscode extension. It watches for changes 
to the vscode configuration and updates the colors when the current theme is 
changed.
The colors are saved in a singleton to allow for easy access when doing 
coloring later.

The TMColor class maps all scopes to the clangd TextMate scope indexes and 
discards any theme colors that are not relevant.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65738

Files:
  clang-tools-extra/clangd/clients/clangd-vscode/package.json
  clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts
  clang-tools-extra/clangd/clients/clangd-vscode/test/TextMate.test.ts

Index: clang-tools-extra/clangd/clients/clangd-vscode/test/TextMate.test.ts
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/test/TextMate.test.ts
@@ -0,0 +1,32 @@
+/** The module 'assert' provides assertion methods from node */
+import * as assert from 'assert';
+
+import * as vscode from 'vscode';
+import {SemanticHighlighting} from '../src/TextMate';
+
+// TODO: add tests
+suite("Extension Tests", () => {
+  test('overrides for more specific themes', () => {
+const scopes = [ 'a.b.c.d', 'a.b.f', 'a' ];
+const colorPairs = [
+  [ [ 'a.b.c', 'a.b.d' ], '1' ],
+  [ 'a.b', '2' ],
+  [ 'a.b.c.d', '3' ],
+  [ 'a', '4' ],
+];
+const tm = new SemanticHighlighting.TMColors(scopes);
+colorPairs.forEach((p) => tm.setColor(p[0], p[1] as string));
+assert.deepEqual(tm.getColor(0), '3');
+assert.deepEqual(tm.getColor(1), '2');
+assert.deepEqual(tm.getColor(2), '4');
+  });
+  test('Sets an instance of TMColors on setup.', async () => {
+const scopes = [
+  'variable',
+];
+const disp = await SemanticHighlighting.setupTMScopes(scopes, () => {});
+assert.notEqual(SemanticHighlighting.TMColorProvider.get().getColors(),
+undefined);
+disp.dispose();
+  });
+});
\ No newline at end of file
Index: clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts
===
--- /dev/null
+++ clang-tools-extra/clangd/clients/clangd-vscode/src/TextMate.ts
@@ -0,0 +1,174 @@
+import * as fs from 'fs';
+import * as jsonc from "jsonc-parser";
+import * as path from 'path';
+import * as vscode from 'vscode';
+
+export namespace SemanticHighlighting {
+export class TMColors {
+  // The clangd tm scopes.
+  private scopes: string[];
+  // Mapping from a clangd scope index to a color hex string.
+  private colors: string[];
+  // The current best matching scope for every index.
+  private colorScopes: string[];
+  constructor(scopes: string[]) {
+this.scopes = scopes;
+this.colors = this.scopes.map(() => '#000');
+this.colorScopes = this.scopes.map(() => '');
+  }
+
+  setColor(scope: string|Array, color: string) {
+if (scope instanceof Array) {
+  scope.forEach((s: string) => this.setColor(s, color));
+  return;
+}
+
+// Find the associated clangd scope(s) index for this scope.
+// If "scope" is a candiate for a clangd scope the clangd scope must have
+// "scope" as a prefix.
+const allCandidates =
+this.scopes.map((s, i) => ({s : s, i : i}))
+.filter(({s}) => s.substr(0, scope.length) === scope);
+// If this scope is more specific than any of current scopes for the clangd
+// scopes it should be replaced. As both options are prefixes of the clangd
+// scope it's enough to compare lengths.
+allCandidates.forEach(({i}) => {
+  if (scope.length > this.colorScopes[i].length) {
+this.colorScopes[i] = scope;
+this.colors[i] = color;
+  }
+});
+  }
+
+  getColor(idx: number) { return this.colors[idx]; }
+}
+
+// Singleton for reading/writing TM scopes/colors.
+export class TMColorProvider {
+  private static instance: TMColorProvider = new TMColorProvider();
+  private colors: TMColors = undefined;
+  static get() { return TMColorProvider.instance; }
+
+  setColors(colors: TMColors) { this.colors = colors; }
+  getColors(): TMColors { return this.colors; }
+}
+
+/**
+ * @param scopes The TextMate scopes clangd sends on initialize.
+ * @param cb A callback that is called every time the theme changes and the new
+ * theme has been loaded (not called on the first load).
+ */
+export async function setupTMScopes(scopes: string[],
+cb: Function): Promise {
+  let oldThemeName = '';
+  async function setTMColors() {
+const name =
+vscode.workspace.getConfiguration('workbench').get('colorTheme');
+if (typeof name != 'string') {
+  console.warn('The current theme name is not a string, is: ' +
+