Closed mjbvz closed 5 years ago
related LSP discussion https://github.com/Microsoft/language-server-protocol/issues/613
/cc @minestarks
A FileRangeRequestArgs
might be more appropriate than FileLocationRequestArgs
, so that you can pass in the currently selected span.
I think in VS this would roughly map to an implementation of GetSpanOfEnclosing
. I don't see any causes for concern with the proposed command / response schema.
Here's some thoughts on how expansion could work:
Walk the ast from the current position up to the root node, adding a selection to the smart select response when encountering:
param: number
)class Foo {
bar(a, b) {
if (a === b) {
return true;
}
return false;
}
}
Starting on true
in return true
true
return true;
if
statement (without braces)if
statementbar
(without braces)bar
methodclass Foo {
bar(a, b) {
if (a === b) {
return true
}
return false;
}
}
Starting on a
in a === b
a
a === b
bar
(without braces)bar
methodexport interface IService {
_serviceBrand: any;
open(host: number, data: any): Promise<any>;
}
Starting on host
host
host: number
host: number, data: any
open(host: number, data: any): Promise<any>;
IService
(without braces)IService
interfaceimport { x as y, z } from './z';
import { b } from './';
console.log(1);
Starting on x
in x as y
x
x as y
x as y, z
{ x as y, z }
import { x as y, z } from './z';
We've refined our API on the VS Code side and have decided not to include the Kind
in v1. We've also moved to use a linked list structure instead of an array. Here's an update proposal that reflects this:
interface SelectionRangeRequest extends FileLocationRequest {
command: "selectionRange";
arguments: SelectionRangeRequestArgs;
}
interface SelectionRangeRequestArgs extends FileLocationRequestArgs { }
interface SelectionRangeResponse {
body?: SelectionRange;
}
interface SelectionRange {
textSpan: TextSpan;
parent?: SelectionRange;
}
The VS Code api also takes a list of positions in the file and we return a list of selection ranges for each of those positions. This avoids extra requests in some cases. We should consider doing this in the TS api as well, as I don't think there's any downside. This would look like:
interface SelectionRangeRequest extends FileLocationRequest {
command: "selectionRange";
arguments: SelectionRangeRequestArgs;
}
interface SelectionRangeRequestArgs extends FileRequestArgs {
locations: Location[];
}
interface SelectionRangeResponse {
body?: SelectionRange[];
}
interface SelectionRange {
textSpan: TextSpan;
parent?: SelectionRange;
}
Here's the VS Code api: https://github.com/microsoft/vscode/blob/caafa5b6920b0e4b8ce2a8300a61568372da0f7b/src/vs/vscode.d.ts#L3791
/cc @jrieken
@mjbvz when you mention that blocks should be selected without braces, do you want a) the selection to be begin and end at non-whitespace characters, or do you want b) all characters including whitespace between the braces to be selected?
a | b |
---|---|
B I think. That's how we've implemented the feature for VS Code's json and html support at least
Can I get your input on these?
type X = { /*|*/x?: string };
type M = { [K in keyof /*|*/any]: string };
const x = { /*|*/y };
function f(...ar/*|*/gs) {}
function f(
/*|*/a,
b,
) {}
const s = `one two ${
/*|*/three
} four`;
Also, I was looking at an existing smart select test in VS Code, and it looks like there are cases where you expect to get a range with an identical parent range? My approach so far includes deduping identical ranges (e.g., in a whole file containing a single function declaration with no surrounding trivia, the function declaration range is the same as the whole file range) on TypeScript's side. Let me know if you'd rather me keep syntactically distinct but positionally redundant ranges in the list.
De-duplicating the ranges sounds correct to me. @jrieken can you confirm if smart select providers should de-duplicate or not?
Some thoughts on those examples:
type X = { /*|*/x?: string };
x
x?
x?: string
{ x?: string }
type X = { x?: string };
type M = { [K in keyof /*|*/any]: string };
any
K in keyof any
[K in keyof any]
[K in keyof any]: string
{ [K in keyof any]: string }
type M = { [K in keyof any]: string };
const x = { /*|*/y };
y
{ y }
const x = { y };
function f(...ar/*|*/gs) {}
args
...args
function f(...args) {}
function f(
/*|*/a,
b,
) {}
a
a,
b,
function f(
/*|*/a,
b,
) {}
const s = `one two ${
/*|*/three
} four`;
${
/*|*/three
}
one two ${
/*|*/three
} four
`one two ${
/*|*/three
} four`
const s = `one two ${
/*|*/three
} four`;
@jrieken can you confirm if smart select providers should de-duplicate or not?
It's kind of optional because the editor will deduplicate ranges anyways - so if it hinders an elegant implementation I won't do it.
@mjbvz @jrieken thanks! So, in my earlier question, I had swapped my "a" and "b" labels such that the screenshots demonstrated the opposite of the options I had written, but I assumed when you responded "B," you were referring to the picture. (I've since edited the question to be consistent.)
If your answer is still "B," I'm trying to figure out the logical rule for: when do we select all the way up to braces vs. leaving trivia unselected? E.g., in the if statement we want to select all the way up to the braces, but in the import clause ({ x as y, z }
) and most of the object-like syntaxes I sent you, we only select up to the whitespace.
I've been trying to think through these scenarios in terms of the probable desired outcomes a user has when expanding a selection, and how easy it is to achieve those outcomes given a particular selection. When braces are on the same line, it's a bit of a toss-up:
I want a multi-line object | Backspace, Backspace, Delete, Return | Return |
I want to start my single-line object over | (just start typing) | Space, Space, Left Arrow |
I want {} |
Backspace, Backspace, Delete | Backspace |
When the braces span multiple lines, selecting all the way up to the braces seems to be more versatile:
I want a multi-line object type | (just start typing) | Return |
I want a single-line object type | Backspace, Cmd+Backspace, Backspace, Space, Delete, Delete? | Space, Space, Left Arrow |
I want {} |
Same as above plus one more Delete | Backspace |
Now, the really painful sequences above can be avoided by simply expanding the selection one more time to include the braces, then typing {
. However, for class declaration bodies and function parameters, that’s not an option:
I want to rewrite my parameters on multiple lines | (just start typing) | Return |
I want to delete my parameters or rewrite them on a single line | (painful) | Backspace |
These scenarios lead me to suggest one of two possible governing rules:
There are possibly other heuristics that could be considered (like the difference between objects, which have a stop just outside the braces, and classes, which select the braces plus the text class ClassName
), but that might be too unintuitive.
By the way, I'm in Redmond (visiting from SF) until Friday afternoon, so if it's easier to discuss in person, feel free to ping me or come by the TypeScript team room.
Updating thread with results of in-person meeting for posterity: we agreed to go with option 2 above and see how it feels once we have it wired up with VS Code.
One other selection stop to consider for multiline cases: select the entire line including leading whitespace.
For code such as:
function foo(
/**/a,
b
)
This would result in the selections:
I've checked in a stubbed out VS Code implementation with https://github.com/Microsoft/vscode/commit/f635233740f2bb44d9e4663f4152f1980ce0bb2b
This is based on the second version of the api proposal, the one that takes a Location[]
and returns a SelectionRange[]
Cool, it works with my WIP branch!
It does look like the ranges provided by tsserver are getting merged with Code's other guesses, e.g. when on Syn/**/taxKind
, I verified that tsserver says “select SyntaxKind
,” but the first selection is just Syntax
. Not sure if this is intentional or not, guess that's up to you all.
but the first selection is just Syntax. Not sure if this is intentional or not, guess that's up to you all.
Yeah, that's VSCode itself, expanding words along separators, like aA
,_
, or -
Ok @mjbvz @jrieken, do you want to give it a try? I'm getting the feeling that I could find little bits of syntax to nitpick and customize forever, but I'm pretty sure at this stage I've covered every example listed in this issue discussion plus some. If you don't find any glaring issues, I'll open a PR. Thanks!
Thanks @andrewbranch. I've done some initial testing and ran into one problem. With the cursor at the /**/
in the code:
const z = 1;
interface foo {
bar(): void/**/
}
In this case, the first returned range is:
"textSpan": {
"start": {
"line": 4,
"offset": 9
},
"end": {
"line": 4,
"offset": 9
}
},
which causes VS Code's selection range conversion to fail. The top returned range must contain the initial position.
I'll keep testing and share other feedback
Looking good overall. A few other notes:
const callback/**/ = this.fetchCallback(serverRequest.seq);
There's currently a stop where just the assignment expression is selected:
I don't think this step is necessary. We could jump right to selecting the entire const
statement
Doc comments are currently not handled. I think these docs should either be included when the element being documented is or as a separate step. For example:
const x = 1;
/**
* docs
*/
function foo/**/() {}
Would have some stop with the selection:
Invalid selection range also returned for the case:
function foo() {
// bla/**/
}
Few more cases may need to consider with explicit separate rules:
JSX <elements attributes={
express + ions}> and static text inside those </elements>
`string literals with ${
expres + sions} inside them`
Generic<Ty|P|E, "Arguments">
Also, should this enrich resulting range objects with minimal context description?
When IDE extends selection, imagine a tooltip fade in for few seconds, indicating WHAT is selected:
Could be helpful in complex code/JSON spanning multiple screen heights.
@mjbvz @jrieken I just found a case where adding VS Code interlacing its own expansions (specifically the one where it wants to select a full line) produces a pretty weird sequence:
You can see the expansion where [K in keyof P]:
actually gets removed, which I think should never be the case when expanding. TypeScript is giving instructions to expand through the whole type on the right-hand side of the colon, but once that type spans multiple lines, I guess VS Code wants the whole line to be highlighted first. But that line taken on its own is semantically nonsensical.
I would suggest for VS Code to:
Simpler example:
I have a general fix for this in vscode that prevents any line selection range that isn’t fully contained by the next range in the list from being added. Will add a test after lunch and PR.
Background
VS Code has proposed a smart selection API (https://github.com/Microsoft/vscode/issues/63935) that allows language extensions to intelligently control how selections are expanded and collapsed.
Just as an example, consider a simple class:
If the user starts with their cursor in the
a
ina === b
expression, progressively runningexpand selection
may result in the following selections:a
a === b
bar
methodbar
method declarationFoo
classThe exact selections we want may differ.
You can find the current state of the VS Code API proposal in
vscode.proposed.d.ts
under the nameselection range provider
Proposal
I think we should investigate supporting smart select for JavaScript and TypeScript users, but do so in a cross editor compatible way. The current proposal is derived from VS Code's needs so we should make sure it will work for other editors too
The current VS Code smart selection API proposal takes a position in a document and returns a list of potential expansion ranges. Each returned range also has some metadata about the range's kind (class, expression, statement, interface, ...)
On the TS Server side, a possible API would be:
We also need to determine which selection ranges we should target for a first iteration. A few basic ones:
/cc @DanielRosenwasser, @jrieken, @amcasey