Skip to content
This repository has been archived by the owner on Aug 29, 2021. It is now read-only.

Highlight #34

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions notes/highlight request.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
# Highlight Range Request

request command: `mcfunction/highlightRange`

request flow: Client -> Server -> Client

Request data:

```ts
{
document: //Document: I forget what the TS type is
startLine: number,
endLine: number
}
```

Response data:

```ts
{
scopes: [
{
line: number,
scopes: string[]
Copy link
Owner

Choose a reason for hiding this comment

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

So when the client recieves this request, it should highlight each occurance of a line number in every open document in the correct colours for the scopes?

How does that make any sense?

Surely we need to send ranges with scopes

}
];
}
```

# Highlight Text Request

request command: `mfunction/highlightText`

request flow: Client -> Server -> Client

Request data:

```ts
{
text: string[]; //Split lines
}
```

Response data:

```ts
{
scopes: [
{
line: number,
scopes: string[]
}
];
}
```
48 changes: 48 additions & 0 deletions notes/scope format.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
## Some notes

Make sure that the begin and end are relative to the start of the line

You _generally_ should not have multiple of the same scopes inside one another (directly)

## Rules

- Arguments should have the surrounding scope `["argument", "parser name"]`.
- Example: `summon ~ ~ ~ zombie`

```ts
{
end: 7,
scopes: ["argument", "minecraft:block_pos"],
start: 12
}
```

- Scopes of multiple names should be separated by `-`
- Example: in `say @e[name="foo",tag=bar]` there should be

```ts
{
end: 18,
scopes: ["kvpair-separator", "separator"],
start: 17
}
```

## Various scopes

Copy link
Owner

Choose a reason for hiding this comment

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

The number of scopes here seems very complicated. You might also want to see microsoft/language-server-protocol#124. Having read through the arguments there, I have realised that it's probably better if the server sends highlighting, because we can then send it for each line (This is only better for our server)

Copy link
Owner

Choose a reason for hiding this comment

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

Additionally, we might want to only support a more simple highlighting method initially until microsoft/language-server-protocol#18 is resolved, which seems to be proceeding in microsoft/vscode-languageserver-node#367. We don't want to make a massively complex system which becomes deprecated in a few weeks.

`"kvpair"`: A key-value pair, like `foo:bar` or `foo=bar`
`"*x*-separator"`: A separator between same scopes, like `"kvpair-separator"` would be `,` for NBT
`"*x*-*y*-separator"`: A separator between different scopes, like `"key-value-separator"` would be `:` for NBT
`"argument"`: A command argument. This shouldn't be used anywhere else
`"key"`: A key, like foo in `foo:bar` and `foo=bar`
`"value"`: A value, like bar in `foo:bar` and `foo=bar`
`"quote"`: A quote character, IE `"`
`"separator"`: Should accompany `*x*-separator` and `*x*-*y*-separator*`
`"*x*-start"`: Start of a value with characters as start and end markers (like `{` and `[`). An accompanying `"start"` should also exist
`"*x*-end"`: Same as `"*x*-start"`
`"punctuation"`: Should exist on all characters used as a separator or start/end
`"string"`: If the scoped value is a string
`"quoted"`: If the string is quoted
`"unquoted"`: If the string is unquoted
`"prefix"`: If the character is a prefix
`"suffix"`: Same as `"prefix"`
20 changes: 20 additions & 0 deletions src/misc_functions/highlight_util.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { SubAction } from "../types";

export interface HighlightScope {
Copy link
Owner

Choose a reason for hiding this comment

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

This should probably go into src/types.ts

end: number;
scopes: string[];
start: number;
}

export function actionFromScopes(scopes: HighlightScope[]): SubAction[] {
return scopes.map<SubAction>(actionFromScope);
}

export function actionFromScope(scope: HighlightScope): SubAction {
return {
data: scope,
high: scope.end,
Copy link
Owner

@Levertion Levertion Jun 23, 2018

Choose a reason for hiding this comment

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

If start and end are included in the SubAction, why are they in HighlightScope. Wouldn't it be better for this function to be something like:

export function createHighlight(scopes: string[], // Or HighlightInnner if we want to support additional properties
    low:number,
    high: number
): SubAction {return {type:"highlight",data: scopes, low, high};}

low: scope.start,
type: "highlight"
};
}
1 change: 1 addition & 0 deletions src/misc_functions/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@ export * from "./returnhelper";
export * from "./security";
export * from "./setup";
export * from "./translation";
export * from "./highlight_util";
9 changes: 8 additions & 1 deletion src/parsers/literal.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { CompletionItemKind } from "vscode-languageserver";
import { ReturnHelper } from "../misc_functions";
import { actionFromScope, ReturnHelper } from "../misc_functions";
import { Parser } from "../types";

const parser: Parser = {
Expand All @@ -19,6 +19,13 @@ const parser: Parser = {
if (reader.string.substring(begin, end) === literal) {
reader.cursor = end;
if (reader.peek() === " " || !reader.canRead()) {
helper.addActions(
Copy link
Owner

Choose a reason for hiding this comment

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

This being an action is actually quite awkward, because if there is ambiguity, then what should happen? Text can only be highlighted in one colour.

Additionally, what about in the case of failures. Do we include the mangled scopes. What about if parsing one thing failed with some highlighting, but something else suceeded. How do we prioritise the values.
Maybe helper.merge could manage this if the merge argument fails, but that would make helper.merge O(n) of actions.

actionFromScope({
end: reader.cursor,
scopes: ["argument", "literal"],
start: begin
})
);
return helper.succeed();
Copy link
Owner

Choose a reason for hiding this comment

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

This can be updated to helper.addActions(...).suceed()

}
}
Expand Down
25 changes: 23 additions & 2 deletions src/test/parsers/literal.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ describe("literalArgumentParser", () => {
defined(literalArgumentParser.parse(reader, properties)),
true,
[],
["test"]
["test"],
1
);
assert.strictEqual(reader.cursor, 4);
});
Expand All @@ -28,10 +29,30 @@ describe("literalArgumentParser", () => {
defined(literalArgumentParser.parse(reader, properties)),
true,
[],
[]
[],
1
);
assert.strictEqual(reader.cursor, 4);
});
it("should return the correct highlight data", () => {
const reader = new StringReader("test");
const out = literalArgumentParser.parse(reader, properties);
assert.deepStrictEqual(
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe we need a better way of managing testing actions: actions should be tested inside each test, not as a seperate test. Maybe an AssertActions type which takes over numActions in assertReturn. Additionally assertReturn's signature should use the object pattern, because there are two many arguments. If you don't mind, I could do that in this branch.

out.actions.filter(v => v.type === "highlight"),
[
{
data: {
end: 4,
scopes: ["argument", "literal"],
start: 0
},
high: 4,
low: 0,
type: "highlight"
}
]
);
});
});
describe("literal not matching", () => {
it("should fail when the first character doesn't match", () => {
Expand Down
5 changes: 3 additions & 2 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { CompletionItemKind } from "vscode-languageserver";
import { BlankCommandError, CommandError } from "./brigadier_components/errors";
import { StringReader } from "./brigadier_components/string_reader";
import { CommandNodePath, Datapack, GlobalData } from "./data/types";
import { HighlightScope } from "./misc_functions";

//#region Document
export interface FunctionInfo {
Expand Down Expand Up @@ -110,10 +111,10 @@ export interface StoredParseResult {
interface SubActionBase<U extends string, T> extends DataInterval<T> {
type: U;
}

export type SubAction =
| SubActionBase<"hover", string>
| SubActionBase<"format", string>;
| SubActionBase<"format", string>
| SubActionBase<"highlight", HighlightScope>;
Copy link
Owner

Choose a reason for hiding this comment

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

Would you mind alphabetising these (I know they weren't alphabetical previously)

// | SubActionBase<"rename", RenameRequest>;
//#endregion
export type Success = true;
Expand Down