-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
🎁 Add "Surround with snippet" to quick fix menu #152718
🎁 Add "Surround with snippet" to quick fix menu #152718
Conversation
c38a5c4
to
6798db9
Compare
@jrieken The workflow failure seems to be unrelated to the changes here. |
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
6798db9
to
0dcd685
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so far. This is already good but I think we can make this more smooth. Less click ftw!
|
||
export class SurroundWithSnippetCodeActionProvider extends Disposable implements CodeActionProvider { | ||
private static readonly codeAction: CodeAction = { | ||
kind: CodeActionKind.QuickFix.value, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Refactor
-type should be used here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
} | ||
const snippets = await this.core.getSurroundableSnippets(); | ||
return { | ||
actions: snippets.length ? [SurroundWithSnippetCodeActionProvider.codeAction] : [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will just invoke the picker but I think it would be better to directly show the snippets. Only show the picker when there is too much to pick from (>7 or so).
For each surroundable snippet you can use InsertSnippetAction and bind the snippet-argument accordingly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introduced this constant value to limit the number of refactor menu items:
const MAX_SNIPPETS_ON_CODE_ACTIONS_MENU = 6;
|
||
async runEditorCommand(accessor: ServicesAccessor, editor: ICodeEditor, ...args: any[]) { | ||
const model = this._editor.getModel(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only extract this into a seprate function, e.g function surroundateSnippet(accessor, model)
and leave the action as-is otherwise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't fully understand this commment. So, I took this as a hint to refactor from class-based to function-based implementation. So, I extracted the functions out and added an accessor: ServicesAccessor
parameter to them to help resolve their dependencies.
} | ||
} | ||
|
||
registerEditorContribution(options.id, SurroundWithSnippetCodeActionProvider); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, forget to mention that we need something better than an editor contribution. Those get created per editor but languageFeaturesService.codeActionProvider.register
is gobal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After going through different parts of the codebase, trying to find similar implementations, I've changed the solution from IEditorContribution
to IWorkbenchContribution
. I couldn't find any better solution with the same life-cycle as a workbench contribution, so please let me know if I should use another approach for this.
Signed-off-by: Babak K. Shandiz <[email protected]>
Another note: fresh from the press we are working on supporting snippets with workspace edits (#145374). So, this feature could be build even better, e.g don't have commands but use a workspace edit (for each surround-with snippet) that uses the new snippet edit support. |
… contribution Signed-off-by: Babak K. Shandiz <[email protected]>
@jrieken After changing from editor contribution to workbench contribution, strangely, context key data were not available to check the command precondition at You can check it by replacing the function canExecute(accessor: ServicesAccessor): boolean {
return accessor.get(IContextKeyService).contextMatchesRules(options.precondition);
} |
Signed-off-by: Babak K. Shandiz <[email protected]>
As I see, we can use SnippetController2.get(editor)?.insert(snippet.codeSnippet, { clipboardText }); into this: const selections = editor?.getSelections();
if (!selections) {
return;
}
performSnippetEdit(editor, snippet.codeSnippet, selections); @jrieken What do you think? |
I was thinking different and that this can be done simpler. The code actions should not use the
|
…` method Signed-off-by: Babak K. Shandiz <[email protected]>
…` resolves $SELECTION variable Signed-off-by: Babak K. Shandiz <[email protected]>
…romEdits` method Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
@jrieken Thanks for the precise details. I've made the changes. However, there was a bug, I think, with the suite('createEditsAndSnippetsFromEdits', function () {
...
test('with $SELECTION variable', function () {
... Could you please review the changes? |
@jrieken Can't we also change the overflow command to use the new |
…rroundable snippets * remove `canExecute` which isn't needed - the "framework" makes sure we only called when it makes sense * tweak styles to my preference and lipstick, try to contain things over loose functions and objects,
@babakks There are some things that I want to tweak before merging - mostly code style/shape. It looks like you need to enable this for this PR. |
The "Allow edits..." is enabled. Also, I now added your account to my fork collaborators. Is there any other permission? |
@jrieken I just merged this with your latest changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank @babakks
I have pushed a few style tweaks. Not to diss you but just to match my preferences since I will be maintaining this. Also note that I have another PR out that adds support for usage timestamps. With that the code actions can also be sorted based on frequently used snippets 🎉
@@ -535,6 +535,17 @@ export class SnippetSession { | |||
const parser = new SnippetParser(); | |||
const snippet = new TextmateSnippet(); | |||
|
|||
// snippet variables resolver | |||
const resolver = new CompositeSnippetVariableResolver([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this could have been fixed with some OvertypingCapturer
-logic but this also seems reasonable.
It finally worked - ran into some permission issues before 🤷. Excited to see this land |
Awesome 👏 ! |
@jrieken You're welcome. Thank you. |
…quick-fixes 🎁 Add "Surround with snippet" to quick fix menu
This PR fixes #150678
Summary of changes:
SurroundWithSnippet
.EditorAction2
is added to expose the command.CodeActionProvider
is added to expose the command as a code action (quick fix).