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.

Sign up for GitHub

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

Jump to bottom

CodeActionScope #41782

Merged
merged 18 commits into from
Jan 22, 2018
Merged

CodeActionScope #41782

merged 18 commits into from
Jan 22, 2018

Conversation

mjbvz
Copy link
Contributor

@mjbvz mjbvz commented Jan 18, 2018 •
edited

Fixes #41048

Problem
We'd like to improve the UX for refactorings. Two specific ideas:

  • A refactorings context menu
  • Allow a keybinding to a specific refactoring, such as extract method

Proposal
Introduce a new CodeActionScope type. This identifies the class/type of a given refactoring. All refactoring code actions for example would have a scope of refactor.*, with an extract method refactoring action having a scope of refactor.extract.method

  • For the refactoring context menu, we use CodeActionScope to filter out any code actions that are not refactorings. The CodeActionContext will also have a scope field on it that a CodeActionProvider can use to determine which code actions to compute and return

  • For keybindings, we can setup a keybinding such as:

    {
	"key": "ctrl+shift+.",
	"command": "editor.action.codeAction",
	"args": [
		"refactor.extract"
	]
}

    where "refactor" is treated as the code action scope. Only code actions of the type "refactor.*" should be shown

Todo
Still a lot to do on this PR

  • Tests
  • Hookup scope for CodeActionContext
  • Create an new editor.action.codeAction command instead of overloading the existing editor.action.quickFix command

@mjbvz mjbvz self-assigned this Jan 18, 2018
@mjbvz mjbvz requested a review from jrieken January 18, 2018 02:17
jrieken
jrieken requested changes Jan 19, 2018
View reviewed changes
src/vs/vscode.d.ts Outdated
*
* Specifies the type of a code action. A scope is a hierarchical list of identifiers separated by `.`, e.g. `"refactoring.extract.function"`.
*/
export class CodeActionScope {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe CodeActionKind is the better name because this is about the 'character' of a code action and because we have been using ...Kind already for similar purposes...

src/vs/vscode.d.ts Outdated
/**
* Base scope for refactoring extraction inline.
*/
public static readonly RefactorInline: CodeActionScope;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if the Refactor-prefix is needed? Isn't this implicit?

src/vs/vscode.d.ts Outdated
*
* Does not modify the current scope object.
*/
public add(parts: string): CodeActionScope;
Copy link
Member

Choose a reason for hiding this comment

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

better append

src/vs/vscode.d.ts Outdated
*
* @param other Scope to check.
*/
public contains(other: CodeActionScope): boolean;
Copy link
Member

Choose a reason for hiding this comment

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

Do we have an use-case for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is used for the requestedScope logic in the TS refactoring code action provider so that we can skip computing code actions that vscode will filter out later

src/vs/vscode.d.ts Outdated
*
* Actions not within this scope are filtered out before being shown by the lightbulb.
*/
readonly requestedScope?: CodeActionScope;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use a for with for select-semantics, like only?

src/vs/editor/contrib/quickFix/quickFixModel.ts Outdated
@@ -36,12 +37,12 @@ export class QuickFixOracle {
this._disposables = dispose(this._disposables);
}

trigger(type: 'manual' | 'auto'): void {
trigger(type: 'manual' | 'auto', trigger?: CodeActionTrigger): void {
Copy link
Member

Choose a reason for hiding this comment

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

getting a little messy... can we fold both args into one request object and pass that along?

src/vs/vscode.d.ts Outdated
* Base scope for refactoring extraction inline.
*/
public static readonly RefactorInline: CodeActionScope;

Copy link
Member

Choose a reason for hiding this comment

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

We should also have static readonly Rewrite for things like 'Convert if-else into ternary statement' etc, etc

Copy link
Member

Choose a reason for hiding this comment

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

and also maybe CleanUp category?

src/vs/vscode.d.ts Outdated
/**
* Empty scope.
*/
public static readonly Empty: CodeActionScope;
Copy link
Member

Choose a reason for hiding this comment

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

no public

@mjbvz
Copy link
Contributor Author

mjbvz commented Jan 19, 2018 •
edited

Thanks for the feedback. Pushed change that updates the names as suggested. I'll also look into cleaning up the trigger implementation flow.

The new change also allows for callers of editor.action.codeAction to control if/when a code action is applied automatically using an autoApply flag:

{
		"key": "ctrl+shift+.",
		"command": "editor.action.codeAction",
		"args": [
			"refactor.extract",
			{ "autoApply": true }
		]
	}
  • "autoApply": undefined - Default behavior. Automatically accept code action if there is only one, otherwise show context menu
  • "autoApply": true - Always accept the first code action
  • "autoApply": false - Never automatically accept the first code action, always show the context menu even if there is just a single item

ghost
ghost suggested changes Jan 21, 2018
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I think cAmEl is good for naming class's. Like codeActionKind

@mjbvz
Copy link
Contributor Author

mjbvz commented Jan 22, 2018

Most recent change makes the keybindings for this more like insertSnippet:

{
		"key": "ctrl+shift+.",
		"command": "editor.action.codeAction",
		"args": {
			"kind": "refactor.extract",
			"apply": "ifSingle"
		}
}
  • "apply": "ifSingle" - Default behavior. Automatically accept code action if there is only one, otherwise show context menu
  • "apply": "first" - Always accept the first code action
  • "apply": "never" - Never automatically accept the first code action, always show the context menu even if there is just a single item

@mjbvz mjbvz added this to the January 2018 milestone Jan 22, 2018
@mjbvz mjbvz merged commit eccf728 into microsoft:master Jan 22, 2018
@mjbvz
Copy link
Contributor Author

mjbvz commented Jan 22, 2018

Will follow up with changes to expose a new "refactor" action with keybinding, as well as hooking up more of the TS quick fixes / refactorings to use CodeActionKind

@mjbvz mjbvz mentioned this pull request Jan 26, 2018
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jrieken jrieken jrieken requested changes

Assignees

@mjbvz mjbvz

Labels
None yet
Projects
None yet
Milestone
January 2018
Development

Successfully merging this pull request may close these issues.

Propose refactoring API
2 participants
@mjbvz @jrieken