CodeAction.disabled

[mjbvz]

Problems

A user selects some text in a JS file and tries to run extract constant (though a keyboard shortcut). The selected text is not extractable. However, there is currently no feedback in the editor about why the selected text could not be extracted.

Also, unless the already user know that JS/TS supports extract constant, they may never discover this refactoring exists at all; it will only show up in the refactor menu when the current selection is extractable.

Desired UX

• If the user explicitly requests to apply a refactoring that cannot be applied (such as extract constant for an invalid selection) let extensions surface a human readable error message describing why the refactoring cannot be applied.

• Provide a way for extensions to show refactorings that they support but are currently not supported in the refactor menu. This would help users understand what refactorings are available, while also seeing that those refactorings are not possible (ideally we'd also find a way to surface the error message here too)

Proposal

With microsoft/TypeScript#35098, we are requesting that TypeScript return the reason why a given JS/TS refactoring is not possible. This issue tracks the VS Code side API required and how information from this new API would be surfaced to users.

My API proposal is to let extensions set an disabled property on the code actions they return:

export class CodeAction {
	...

	disabled?: {
		/**
		 * Human readable description of why the code action is currently disabled.
		 *
		 * This is displayed in the UI.
		 */
		reason: string;
	};
}

I propose including .disabled on the CodeAction class because it would let us reuse the existing code action metadata (title, kind, ...) without having to introduce new classes (such as InvalidCodeAction)

How .disabled would be surfaced to users

Lightbulb menu

This automatic lightbulb menu would drop all error code actions. It would likely be overwhelming to show them all.

I like keeping the lightbulb as showing: here is everything that is possible right now.

Keybinding for code action

There are a few cases here:

• All returned code actions for the keybinding are errors.

  If the user has enabled apply as part of the keybinding, in this case we would want to show the .error from the code action that would have been applied if it were valid.

• A mix of error and valid actions are returned, and the user has not selected to apply the code actions.

  Here we would want to show a full list of actions, including the invalid actions disabled.

• A mix of code error and valid actions are returned, and the user has selected to apply code actions.

  We would apply the code actions as if the error code actions were not there. For example, use "apply": "ifOnly" would apply the valid code action if it is the only valid code action (ignoring the errored ones)

Refactoring menu

We'd show the errored actions in the refactoring menu but as disabled. The goal of this is to improve discoverability.

With the current proposal it would be up to extensions to say which code actions they return in the error state and which ones they simply drop. For example, it may be overwhelming if JS/TS to showed all of its refactorings in the refactor menu all the time (including rarely used ones such as rewrite export). Instead, for the generic code action menu, JS/TS would likely always return the most common actions (such as extract constant) either in a valid or in an error state

[mjbvz]

Copying some extension authors and other potentially interested parties: @DanTup, @jrieken, @DonJayamanne, @DustinCampbell, @akaroml, @kieferrm

The specifics of the proposed API will likely change. I am mainly interested in your feedback on:

• Is surfacing "here's why this refactoring is not possible" something that would be helpful for your users?

• Could your language server / extension generate this error information? Could it do so efficiently?

• How would you expect the error information be used by VS Code? Do the current proposed use cases make sense? Are there any other ones that we should consider?

[DonJayamanne]

Sounds like a good idea, having all refactorings in one place and displayed (almost) always does improve discoverability.

something that would be helpful for your users?

At this stage I don't think so. But it is a change in a positive direction (more exposure).

[DanTup]

Sounds like a good idea to me (it also seems like #45383 would be more reasonable of this is done?).

I think the naming of the .error property is a little weird (the name makes it sound like "we tried this and it failed" rather than "this is the reason this one is not available") but I'm not sure I have anything better.

Is there value in having a way of flagging code actions as unavailable without having to provide a string? I think in a majority of cases it's going to be the same reason ("it's just not valid at this location" - for example if you open the menu in the middle of whitespace, or on a language keyword) so having VS Code provide a consistent (and localised) message without us having to would be nice (I think being able to generally provide refactors that are unavailable is more significant than the specific message shown). Maybe the property could be a boolean | string for ex., so we can flag that it is unavailable, or provide a specific reason it is unavailable.

[jrieken]

I like - the is an easy to adopt and pragmatic change that adds much value. Tho, I also think that error isn't a great name.

[mjbvz]

I've tentatively changed the property name to disabled instead and wired it up in a few places.

Here's showing a custom message if the requested code action (such as extract constant) is disabled:

Here's what disabled code actions look like in the refactor menu:

---

Building on @DanTup's comment that it may be difficult to generate a useful error message, we could change the api to be something like:

disabled?: {
    reason?: string
}

That may be more explicit. If extensions do not provide a reason, we could fall back to the a generic one

[DanTup]

Should it be:

disabled?: boolean | {
    reason?: string
}

? Would be weird to use disabled: {} to indicate disabled. Otherwise, I like it :-)

When disabled items are shown in the menu that do have reasons (eg. if there are multiple, so they don't show in the bubble), will the reasons show in tooltips or something?

[jrieken]

Maybe even more compact: disabled?: true | string. Or we make this not only negative but also have a way to explain why a code action is there. Maybe an over-delivery and not really needed but it could then look like this reasoning: { enabled:boolean, description:string }