Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CodeAction.disabled #85160

Open
mjbvz opened this issue Nov 19, 2019 · 13 comments
Open

CodeAction.disabled #85160

mjbvz opened this issue Nov 19, 2019 · 13 comments

Comments

@mjbvz
Copy link
Contributor

@mjbvz mjbvz commented Nov 19, 2019

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

This comment has been minimized.

Copy link
Contributor Author

@mjbvz mjbvz commented Nov 19, 2019

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

This comment has been minimized.

Copy link
Contributor

@DonJayamanne DonJayamanne commented Nov 20, 2019

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

This comment has been minimized.

Copy link
Contributor

@DanTup DanTup commented Nov 20, 2019

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

This comment has been minimized.

Copy link
Member

@jrieken jrieken commented Nov 20, 2019

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 mjbvz changed the title CodeAction.error CodeAction.disabled Nov 21, 2019
mjbvz added a commit that referenced this issue Nov 21, 2019
mjbvz added a commit that referenced this issue Nov 21, 2019
@mjbvz

This comment has been minimized.

Copy link
Contributor Author

@mjbvz mjbvz commented Nov 21, 2019

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:

Screen Shot 2019-11-20 at 3 07 54 PM

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

Screen Shot 2019-11-20 at 3 07 59 PM


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

This comment has been minimized.

Copy link
Contributor

@DanTup DanTup commented Nov 21, 2019

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

This comment has been minimized.

Copy link
Member

@jrieken jrieken commented Nov 21, 2019

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 }

@mjbvz mjbvz mentioned this issue Dec 3, 2019
2 of 2 tasks complete
@akaroml

This comment has been minimized.

Copy link
Member

@akaroml akaroml commented Dec 3, 2019

I just noticed this proposal. It will definitely serve the purpose of reporting reasons. Besides, we could use this as a way of exposing features from the Java language server. In this way, we can provide a full list of code actions that are supported and enable/disable them according to the current context. This way we make the trigger of code actions more deterministic and it's also great for feature discoverability.

@testforstephen @Eskibear @jdneo

@mjbvz mjbvz modified the milestones: November 2019, December 2019 Dec 3, 2019
mjbvz added a commit that referenced this issue Dec 12, 2019
For #85160

Using an object is more explict with property names and will let us introduce additional properties in the future if needed
@mjbvz

This comment has been minimized.

Copy link
Contributor Author

@mjbvz mjbvz commented Dec 12, 2019

Change the api to:

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

This is more explicit and would allow us the add additional properties related to disablement in the future (say a disabled.documentation property that links to documentation)

@DanTup With this proposal, you would always need to provide a reason why a refactoring is disabled. The disabled.reason can be a generic error message such as "refactoring not available" if your extension cannot generate specific ones

@mjbvz mjbvz added the api label Dec 12, 2019
@DanTup

This comment has been minimized.

Copy link
Contributor

@DanTup DanTup commented Dec 12, 2019

What's the reason to require extensions to provide generic text over VS Code doing it? Having one in VS Code would not only give some consistency, but it could be localised along with the rest of the UI. I suspect 100% of mine will use a generic message (for now at least), as the language server doesn't provide a reason why they're not available.

@mjbvz

This comment has been minimized.

Copy link
Contributor Author

@mjbvz mjbvz commented Dec 12, 2019

@DanTup We really want to encourage extensions to provide a useful reason if they disable a refactoring, even if it's something simple like current selection is not extractable

@akaroml

This comment has been minimized.

Copy link
Member

@akaroml akaroml commented Dec 16, 2019

I wonder how the disabled message would be rendered? The only example I found is this:
image

But a disabled menu item is not able to be selected, and hovering does not lead to any state change or anything new presented.

@mjbvz

This comment has been minimized.

Copy link
Contributor Author

@mjbvz mjbvz commented Dec 20, 2019

@akaroml The ways .disabled is currently surfaced is covered in the release notes: https://code.visualstudio.com/updates/v1_41#_codeactiondisabled

We do want to improve this by including the disabled reason in the context menu itself, but are still trying to figure out a clean way to do this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.