Add Xaml command hanler for CreateEventHandler command #51670
Conversation
| namespace Microsoft.VisualStudio.LanguageServices.Xaml.Implementation.LanguageServer.Handler.Commands | ||
| { | ||
| [ExportLspRequestHandlerProvider(StringConstants.XamlLanguageName), Shared] | ||
| [ProvidesMethod(Methods.WorkspaceExecuteCommandName)] |
There was a problem hiding this comment.
I think you just want ProvidesCommand, ProvidesMethod shouldn't be needed
There was a problem hiding this comment.
Good catch! I have removed the ProvidesMethod.
| { | ||
| CommandIdentifier = StringConstants.CreateEventHandlerCommand, | ||
| Arguments = new object[] { textDocument, xamlCompletion.EventDescription }, | ||
| Title = Resources.CreateEventHandler |
There was a problem hiding this comment.
Is the title shown anywhere for completion commands? If it's not we may not need to localize it.
There was a problem hiding this comment.
You have a good point. I am not aware the title is shown anywhere so far. I have removed the string from the resources.
There was a problem hiding this comment.
Small improvement, make it a const string in this class.
In reply to: 587847403 [](ancestors = 587847403)
| // request.Arguments has two argument for CreateEventHandlerCommand | ||
| // Arguments[0]: TextDocumentIdentifier | ||
| // Arguments[1]: XamlEventDescription | ||
| var arguments = new object[] { ((JToken)request.Arguments[1]).ToObject<XamlEventDescription>() }; |
There was a problem hiding this comment.
I don't see arguments[0] being used anywhere, do we actually need that item?
There was a problem hiding this comment.
Yeah it is used in line 29 GetTextDocumentIdentifier.
There was a problem hiding this comment.
Ah I missed that. It looks like if we get the command it must have that argument, so I'd recommend First()
There was a problem hiding this comment.
Sounds good to me.
| internal interface IXamlCommandService : ILanguageService | ||
| { | ||
| /// <summary> | ||
| /// Execute the <paramref name="command"/> with the <paramref name="commandArguments"/> on server |
There was a problem hiding this comment.
on server [](start = 96, length = 9)
That's just an implementation detail that's not relevant to the description of the interface #Closed
There was a problem hiding this comment.
Sure I will update the comment. #Closed
| using System.Collections.Immutable; | ||
|
|
||
| namespace Microsoft.VisualStudio.LanguageServices.Xaml.Features.Commands | ||
| { |
There was a problem hiding this comment.
Feels like this should live under Completion and not Commands. We just happen to pass it around as a command parameter. #Closed
There was a problem hiding this comment.
Yeah I have been moving this struct between Commands and Completion too. Now as you vote for completion I guess completion wins. :) #Closed
| public CreateEventCommandHandlerProvider() | ||
| { | ||
| } | ||
|
|
There was a problem hiding this comment.
Is this actually needed? We're not importing anything in here. #Closed
There was a problem hiding this comment.
This is required by the ExportLspRequestHandlerProvider. I will get warning if I don't have an importing constructor. #Closed
This PR added code to handler when the CreateEventHandler command gets triggered and allow XAML language service to execute the command.
When a XamlCompletionItem has an EventDescription, we will set this CreateEventHandlerCommand on the CompletionItem, and the command will be triggered when the completionItem gets committed.