Implement configurations related to customization updates #39
Conversation
…n-q-eclipse into paras/showCustomizationsDialog
|
|
||
| public CustomizationUtil() { | ||
| try { | ||
| amazonQLspServer = LspProvider.getAmazonQServer().get(); |
There was a problem hiding this comment.
This blocking call should not be placed into the constructor. This will not resolve until the LSP is downloaded and started up, and if we are waiting on the main thread it can cause deadlock. See #35
| } else { | ||
| output.add(null); | ||
| } |
There was a problem hiding this comment.
yes because we are not handling other configurations as of now. Didn't want to handle it by fetching first index hardcoding. Also according to the LSP spec, The request can fetch several configuration settings in one roundtrip. The order of the returned configuration settings correspond to the order of the passed ConfigurationItems (e.g. the first item in the response is the result for the first configuration item in the params).
| Map<String, Object> updatedSettings = new HashMap<>(); | ||
| Map<String, String> internalMap = new HashMap<>(); | ||
| internalMap.put("customization", this.selectedCustomisationArn); | ||
| updatedSettings.put("aws.q", internalMap); |
There was a problem hiding this comment.
Let's put this value aws.q in a const somewhere since we need to use it in multiple places.
| Map<String, String> internalMap = new HashMap<>(); | ||
| internalMap.put(Constants.LSP_CUSTOMIZATION_CONFIGURATION_KEY, this.selectedCustomisationArn); | ||
| updatedSettings.put(Constants.LSP_CONFIGURATION_KEY, internalMap); | ||
| CustomizationUtil.triggerChangeConfigurationNotification(updatedSettings); |
There was a problem hiding this comment.
I think we will still want this to be processed in a separate thread, due to the downstream blocking call.
There was a problem hiding this comment.
You mean we should not even wait for LspProvider.getAmazonQServer() to complete?
There was a problem hiding this comment.
made the change to let it be executed in a separate thread.
|
Can you please merge |
This PR is dependent on this one #33, that's why can't merge it to main. I forked this one from paras/showCustomizationsDialog branch. I formatted on my local for checkstyle as well and pushed to this branch. |
…ed constants to a separate file
pras0131
force-pushed
the
paras/lspConfigurations
branch
from
88194fc to
f5a8163
Compare
Reference ticket: https://issues.amazon.com/issues/ECLIPSE-295
The PR aims to handle two scenarios:
Tested the above two functionalities by taking a pull of aws/language-servers#462 branch in my local and running the same locally.