-
Notifications
You must be signed in to change notification settings - Fork 4
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
Fetch customizations from LSP server #43
Conversation
@@ -26,4 +34,17 @@ public static void triggerChangeConfigurationNotification(final Map<String, Obje | |||
} | |||
} | |||
|
|||
public static CompletableFuture<List<Customization>> listCustomizations() { | |||
GetConfigurationFromServerParams params = new GetConfigurationFromServerParams(); | |||
params.setSection("aws.q"); |
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.
Sorry not too familiar with the params yet. Where are these sections defined? I don't see them in the high level manifests / plugin.xml.
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.
These don't need to be defined in manifest/plugin.xml. This is what LSP server for customization expects as an input. References: https://github.com/aws/language-servers/blob/main/server/aws-lsp-codewhisperer/src/language-server/configuration/qConfigurationServer.ts#L32 , https://github.com/aws/language-server-runtimes/blob/3a3c3f629dcba752a977ab1f1bf9e99dc0dc9f02/runtimes/protocol/getConfigurationFromServer.ts#L10
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.
Ah I see.
@@ -64,7 +72,6 @@ public void fill(final Menu menu, final int index) { | |||
@Override | |||
public void widgetSelected(final SelectionEvent e) { | |||
CustomizationDialog dialog = new CustomizationDialog(shell); | |||
// TODO: This mock will be replaced by an actual call to LSP | |||
dialog.setCustomisationResponse(getCustomizations()); |
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.
nit: customisation -> customization. keep it consistent
customizations.add(new Customization("customization-arn-2", "Customization 2", "Code Whisperer customization 2")); | ||
customizations.add(new Customization("customization-arn-3", "Customization 3", "Code Whisperer customization 3")); | ||
try { | ||
customizations = CustomizationUtil.listCustomizations().get(); |
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.
Is there any way to make this populate asynchronously? This would block the UI thread, no?
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.
nice catch! I updated the code to fetch the customizations async and not block the UI thread to open the dialog.
Reference ticket: https://issues.amazon.com/issues/ECLIPSE-296
The PR aims to fetch customizations from the LSP server and removing the hardcoded customizations.
There is also a change in one of the conditions to save the preference - if someone has selected customization radio button but no customizations are available, then do not save the preference.