Support refactors that require user input/options #1164
Comments
|
I think it's a slippery slope: the metadata you're asking for are actually going to become form descriptions, then maybe even UI descriptions (a la XUL or HTML). That's not a trivial topic... ie how can one describe that the "OK" button has to be disabled for an empty field, or a field that contains invalid characters? We need to anticipate that in the protocol because such query will happen as soon as we open the door to server-side defined forms. And soon, step by step, the LSP will start defining many parts of an IDE UI instead of providing language-specific operations. |
I understand the concern, but it doesn't seem like a great reason not to support many types of refactor. If it shouldn't turn into a UI language, that can be enforced without rejecting the whole idea. VS Code has commands for collecting input, but you can't build dialogs/UIs in it (and I presume you never will). For most of these refactors, a single input will do. I don't think "asking the user for a string" (eg. VS Code's Without anything, it will encourage servers to implement things in custom commands just to get input, and that will need duplicating in each editor (and this will probably either be done differently by each language or there will become an "unofficial spec" that editors/servers confirm to to share this). That seems somewhat at-odds with the goals of LSP. (This isn't just theoretical - I've been considering implementing some of these refactors this way for a while, and it came up again recently by someone wanting to contribute the same.)
I agree that common refactors would be better as first-class features - it can result in a better user experience - but there are so many refactors that won't fall into this category, it seems like there should be something to support them. For example, is "Extract Method" common enough? "Extract function"? Maybe. What about Flutter's "Extract Widget"? Presumably not.
Do you mean use a custom command (invoked by the server on the client) that triggers the rename? I've considered this, but LSP doesn't define any well-known commands or a way for the server to know what commands the client has. There's some slightly-related discussion in #1104 that also led me to believe that driving workflows from the server might not be a good idea. |
Probably something like that. I was more wondering whether CodeActions should allow an alternative to textEdits and commands, something like |
That would actually not work for this case, as it doesn't bring the opportunity to capture input. |
|
I was interested in implementing an input-dependent refactor in the Dart language but I was faced between one of two options:
The backend doesn't need to be involved with defining UI, but it could at least expose a schema of the required input. This would be no different from a backend in a traditional client-server app. The backend exposes an API call with several arguments and does not dictate how the client collects this info. In this scenario, the backend is the LSP server and the clients are the editors (VSCode, Eclipse, Sublime, etc). I would rank things in the following order of desirability from a refactoring standpoint:
There are many cases where a standard won't be able to capture the needs of all the different languages. In this case, it seems preferable to at least go with (2) and allow the logic to live in the LSP server backend rather than (3) in the client editor. This option isn't as good as (1) the standard, but it at least allows developers to maintain a single refactoring implementation that is shared across editors. And it won't block implementers from improving the language experience. While standards take a while to get defined, experimentation can happen quickly. This experimentation could also help define standards. The LSP team could pay attention to the non-standard LSP features that are being implemented in category (2) and standardize the most popular ones. Implementers could replace their custom implementations with standards where possible. Because it's less code to maintain, I imagine they'd have a strong incentive to migrate to LSP standards as they are released. |
dbaeumer
added
feature-request
|
I have a bunch of thoughts here! Avoiding GUI In my experience as IDE user, I find GUI-based refactors awkward to use in practice. I have a pretty optimized text editing workflow, with acejump, multiple cursors, home row navigation, etc (and I even don't use vim), and switching to filling-in a form for a complex refactor breaks the flow. The flow is much better if I can apply the refactor in-place, without leaving the editor. Quite a few of the refactors can be handled this way. For example, IntelliJ recently introduces in-place "change signature" (refactor which allows you to change types, names, order and quantity of function's parameters, and updates all the call sites): The basic idea is that we just allow the user to change the signature of a method in an IDE, without invoking refactor explicitly, and then supply a code action to fix the call-sites. The same works for Java's analogue of move between files -- if you select some code, cut it in one file, and paste it into the other, the IDE prompts you with yes/now dialog for updating call-sites. Text-based UIs One thing which I think might be interesting, but which wasn't explored are hacky, text-based gui. For move-to-file, let's say the user has this written: fn foo() { /* cursor here */
}We can show a code action which does this: // move-to: some/file.rs /* cursor here */
fn foo() {
}ie, add a comment which serialized refactoring GUI in a comment. Then the server can show auto-completion for file paths, highlight the element which would be moved using semantic highlighting, etc. To apply the refactor, there's another code action avaialble on the comment, which does the move and removes the comment. Note that we've got composition for free -- if the user wants to move a bunch of things, they can now select the comment, copy-paste it to all target declarations (maybe using multi-cursor) and invoke a series of moves without selecting a file each time afresh. Universal Refactors I am somewhat skeptical about standardizing common refactors. This mostly stems from my values: I seek to build the perfect experience for a specific language in a hypothetical perfect client, rather than building a uniformly good behavior in all clients. I think languages are sufficiently different that even seemingly identical refactors would have various differences if one looks close enough. For example, "Move to file" in a perfect Rust IDE would be "move to module". There's no 1-1 correpondence between files and modules (there may be several modules within one file). The input would be a Prior Art If we do go with GUIs for refactors, the dart protocol approach feels ok-ish from the "perfect experience" POV: For each refactor type, Dart documents a specific JSON format with data to be shown to the user as a feedback, and JSON format for the user's reply. It says nothing how the data is displayed in the UI -- client implements specific GUI for each refactor. This obviously means manual work for each An interesting facet here is that the communication protocol for a refactor is stateless. This works a bit like HTTP GET/POST for a form. Rather than the server recording that user X now views form Y after X does GET Y, the corresponding POST Y contains all the info required to restore the context. |
|
Today we have showMessageRequest which is a somewhat limited form of VSCode showQuickPick. If we aim for 80% fidelity in refactorings, ignoring things like reorder parameters, then if we expanded on the idea of The client could get a list of questions. If the client wants to render them as a form, it can do so. However it does not need to. The client could also prompt each question in a row (vscode quickpicks, etc) until it gets all the responses. The different types of questions can constrained to ones that easy to implement as both text / gui. Once all the questions have been answered the client responds with the answer for each item in the order that it was provided similar to how configuration returns it's results. Additional Specializations that might make sense
|
|
I agree with the general points above and suggest we think about the separation like a regular client-server app. The backend is the LSP server. It does the heavy lifting related to the language and doesn't enforce any UI constraints. We can probably completely disconnect the LSP from any involvement in the UI so we leave the client to decide how to do this. This would support @makarius's use case of text-based refactoring, some of Dart's use cases of GUI-based refactoring, and allow the client flexibility on how to collect that input as @david-driscoll. And if the LSP can expose the schema of the API in the same way a GraphQL or other discoverable APIs, many editors can provide sensible defaults based on the schema. In GraphQL APIs, it's relatively straightforward to auto-generate a UI from the inputs the backend expects by reflecting over the schema. GraphQL is overkill, but it's a good example where you can keep a separation of concerns between client-server without the need for unnecessary boilerplate. If the server exposes the schema of inputs, the client can choose to auto-generate UIs for requesting input or choose to build specialized UIs for some refactors. For example one LSP API call might look like: ExtractClassToFile
It could expose this in some form of standard json structure. The client editor could collect this info in different ways:
To start, we could be conservative about what types we allow. File paths, strings, numbers, and a few primitives could be introduced first before expanding further. |
I don't think a schema for the spec is necessary if we rely on LSP specified operations: To deal with LSP, one would typically use a LSP SDK (vscode-languageserver for node, LSP4J for Java and so on...). Most languages do have introspection mechanism that would allow to read an operation (a method) definition as a schema, and those clients can already generate forms from those operation if they want to. But maybe I misunderstand and you suggested something that is more like allowing a new I don't believe automatic generation of UI from a list of fields is a sufficient solution anyway. (this belief is based on my previous experience in the good old time of SOA and BPM and connecting to services dynamically and generating forms for them). Indeed, a generated form is almost always bad! As soon as user deal with a generated form, they complain almost immediately that "form allows incorrect values", "form doesn't have a file/directory/color-picker", "form should provide a more detailed description for field XXX", "entry should react when entry changes", "entry should be placed beside not under"... No-one is fully happy with them and they quickly feel too dumb. So while a way to describe forms/fields that client should render in a dialog can be interesting and useful to "unlock" some cases; and that it might be worth having included in the spec of However, I cannot really advise of a better way to handle that.
None of them is very satisfying for the users; but concretely, if we want to support advanced refactoring and UIs which are language specific, non-standard and rely on language model elements that are only known by the server; I don't see an alternative to the extreme solution 1 above. And even with solution 1, we may want the client to continuously send the form values on every change to do some extra validation that cannot be done only on client-side (eg does this module exist), so it would also quickly show its limits. |
|
@mickaelistria You make some good point on the pitfalls of automatic code generation. I didn't mean that the server would need to dictate the UI. It would only describe a list of arguments (name, description, type) in the same way a typescript function might define a list of args and types. Or an RPC might do the same. I don't have an opinion on how the LSP could query this schema. The client could choose to generate a UI or it could choose to go a custom route. But I may be getting ahead of myself. Just allowing LSP refactors to accept a map of arbitrary inputs would be very valuable even without a schema declaration. |
|
I think an additive approach makes sense, having the ability for the server to "ask the client for input". There is no implied schema that the client has to implement. The only thing the client has to know is that "the sever wants some input from the user.". That could be expanded to include things like validation (regex or a callback) or a completion list, but strictly speaking they're not required. They make the user experience better, if you're executing "Extract Method" you would want a valid method name and the user would prefer want immediate feedback, but it doesn't have to be that way initially. |
|
Yes let's not make an initial approach overly complex. I like the proposed structures from @david-driscoll. The existing {
"title": "Rename Method",
"command":
{
"command": "rename-method",
"arguments": ["some", "fixed", "parameters"],
"inputs": [
{
"type": "boolean",
"title": "Frobnicate as well?"
},
{
"type": "input",
"title": "New method name"
},
{
"type": "pick",
"values": ["foo", "bar", "baz"],
"title": ""
}
]
}
}the client collects the three inputs from the user and then requests {
"command": "rename-method",
"arguments": ["some", "fixed", "parameters", true, "qux", "foo"]
} |
|
@rwols Thats definitely a nice schema. I feel like there are some more useful ways to extend this. A default field wouldn't go amiss for some the 3 types you mentioned. I'd imagine the client would reply with either an array of strings that the user selected, or optimise to an array of indexes the user selected. Another possible type is a reorder, the client could list the items and the user could reorder them, the server would then respond with an array containing the new order(either values or indexes relative to what was sent to the client) All this input behaviour would need to be in capabilities, this is a quick example of how it could look. Servers would then only request user input types that the client supports. |
|
Come to think of it, We should probably be specifying |
|
I like the latest proposal on the input schema and command execution. We can be conservative with the input types accepted on the first pass. Like string, int, selection, file path. The language several can be responsible for validation which may be less than ideal for UX, but it will at least unblock the use cases. If this standard gets adopted, we can can carefully add new types based on usage. This way clients can provide more optimized UIs for collecting inputs depending on the type. |
|
Relevant bit from JetBrains conf: https://youtu.be/GRmOXuoe648?t=7872 |
|
We are hitting the same problem for 90% of them require very simple user input, yet we have to have custom code on the client side, and we need to duplicate the efforts for different editors. Rather than sitting on it for the perfect solution for every possible case one can imagine, it would be nice to provide something that can first satisfy the most straightforward cases. In my experience, even if such a complex, flexible system is provided, only a few use all that functionality. Most of the others use a small portion of it. And if one decides to provide refactoring without a message box (inline change), that is fine; But that preference shouldn't block others from providing refactoring differently. It looks like we already have excellent suggestions from @david-driscoll, @venkatd. @rwols, @njames93 What do we need to make this happen? It is already a 2-year-old feature request. |
|
It is a specification. I wrote a pretty long message so I split it in sections. Lets not hurry anyone.
While I agree that LSP could implement something simple, Look at it now, the completion response got pretty bloated IMO (to no offense - I wish that there was one preferred way to get completions where I do not have to think about if textEdits field exist prefer that field, unless the client said to support for insertReplaceSupport than prefer InsertReplaceEdit, else fallback to insertText else to label, unless the client said to support item defaults than use textEditText with itemDefaults...) I might not get the correct order, but hope you get my point. I am not saying that that would happen with the proposed "request user input" request. Here is also one POC for `client/getInput` request, just to spark additional ideas for the LSP spec authors.Request: The request is made from the server to the client. Params: type Params = {
placeholder?: string // a placeholder when nothing is selected
initialText?: string // add initial text.
initialSelection?: Range // can be used to preselect the initial text. // maybe unnecessary
items?: InputItem[] // if present the user must choose one item from the list.
data?: LSPAny; // server can put anything here. it will be send back by the client once the user picks an item like info about the textDocument, position, range...
type?: "single" | "multiselect" // defaults to single, // maybe unnecessary
}
type InputItem = string | {
label: string,
value: LSPAny
details?: string // more info about the input item, if have labels like "on" or "off", you can describe what selecting those items would do.
}Response: {
item: InputItem | InputItem[] | null // one item if `type` was `single`, array if `type` was `multiselect`, `null`` if user canceled the request, for example by pressing `esc`.
data?: LSPAny; // that the client received form the server.
}The `client/` namespaceI was thinking about proposing a new namespace(similar to The
Me bashing about the word "pylance" being used in conjunction with "duplicate the efforts for different editors".
By editors you mean “Visual Studio Products and Services” only?
Of course those editors are left with pyright(which is good type cheker), but if I were to open a feature request to pyright to implement inlay hints (which is already implemented in pylance)... I am pretty sure that I would get a negative response, as pyight is just a type checker and not a lsp server. But who forbids me to create a LSP server that use pyright and implement inlay hints and other features in pylance myself, but than again isn't LSP spec meant to reduce the need to re-implement stuff in other editors... unless the server, even if it can run in other editor, forbids that with a license. My point, not a lot of editors would benefit this change in pylance ;) Also keep in mind, Wish you all a good weekend! |
|
I am having a similar issue when it comes to implementing "implement interface" code action. What I need is the interface name to be specified by the user before the code action is actually handled. The LSP must know the interface name a priori, to know the name to search for. |
For Dart we've been exploring something like this in This local command then enumerates the And then finally delegates to the original command on the server with the Currently the code only supports one There's no spec for this yet (because it's still being worked on and behind a flag) but if it's close enough to what others want I would write it up for others to review/contribute to. I'd love to see something standard in LSP, but if that's not going to happen there would still be advantages to aligning languages on an "unofficial" spec for this. Edit: CodeAction JSON looks approximately like this: {
"kind": "refactor",
"title": "Move 'main' to file",
"command": {
"arguments": [
// For Dart, we generally pass arguments as an object so we can
// provide names, so the arguments list here is a single item
{
// These arguments are not defined by parameters but are fixed
"filePath": "/Users/danny/Desktop/dart_sample/bin/main.dart",
"selectionOffset": 7,
"selectionLength": 0,
// These additional "arguments" can be overwritten by user-provided values
// using the info in data.parameters
"arguments": [
"file:///Users/danny/Desktop/dart_sample/bin/main.dart"
]
}
],
"command": "move_top_level_to_file",
"title": "Move 'main' to file"
},
"data": {
// These parameters map to "arguments" above
"parameters": [
{
"kind": "saveUri",
"parameterTitle": "Select a file to move to",
"parameterLabel": "Move to:",
"defaultValue": "file:///Users/danny/Desktop/dart_sample/bin/main.dart",
"actionLabel": "Move",
"filters": {
"Dart": [
"dart"
]
}
}
]
}
}The client uses |
|
@DanTup That looks nice! For example, having a response to [{
"kind": "refactor",
"command" : {
"arguments": [
"leave-me-be-#1",
"leave-me-be-#2",
"/the/replaceable/path/passed/as/a/parameter"
]
},
"data": {
"parameters" : [
{ "What to put here? " }
]
}
}]Is the custom user input still supported for such a case? |
|
@KKoovalsky that's why there is a nested That said, clients should also pass through the "data": {
// These parameters map to "arguments" above
"parameters": [
{
"kind": "something_the_client_doesn't_understand",
"defaultValue": "file:///Users/danny/Desktop/dart_sample/bin/main.dart",
}
]
}Because the client doesn't know what to do with this parameter kind, it should just use |
|
@DanTup Ok, thanks, I see now that in your example |
|
@KKoovalsky correct :-) |
One common request of Dart is a refactor for "move to file" (either moving to an existing file, or a new one). Right now I don't think there's a great way to implement this as far as I can see there's no way to ask the user to pick a file (existing or not).
Slightly related, for Extract Method we would like to ask for a name (there's a related issue at #764 suggesting instead allowing us to trigger a rename at the end, although asking for the name up-front would also work).
There are other refactors that Dart supports in other IDEs that have various other inputs/options (for example "inline method" lets you decide whether to keep or remove the method, and when extracting a method there's an option for whether to replace any other instances of the same statements in the file with a call to the new method).
I'm not sure of the best way to do this, but I thought it was worth starting a discussion (I couldn't find an existing one), but some possible ideas:
window/showMessageRequest)The text was updated successfully, but these errors were encountered: