feature/Notifications #390
Conversation
…Optional being used in declaration
plugin/src/software/aws/toolkits/eclipse/amazonq/lsp/AmazonQLspClientImpl.java
Outdated
Show resolved
Hide resolved
plugin/src/software/aws/toolkits/eclipse/amazonq/lsp/AmazonQLspClientImpl.java
Outdated
Show resolved
Hide resolved
plugin/src/software/aws/toolkits/eclipse/amazonq/lsp/AmazonQLspClientImpl.java
Outdated
Show resolved
Hide resolved
plugin/src/software/aws/toolkits/eclipse/amazonq/lsp/AmazonQLspClientImpl.java
Outdated
Show resolved
Hide resolved
plugin/src/software/aws/toolkits/eclipse/amazonq/lsp/AmazonQLspClientImpl.java
Outdated
Show resolved
Hide resolved
| message, | ||
| checked -> { | ||
| if (checked) { | ||
| Activator.getPluginStore().put("notificationSkipFlag", "true"); |
There was a problem hiding this comment.
I think we will need this to be more granular - e.g. per notification type.
plugin/src/software/aws/toolkits/eclipse/amazonq/lsp/model/EventIdentifier.java
Show resolved
Hide resolved
|
Please add additional details to your PR title & description, including screenshots of the various notification types. |
| @@ -19,5 +20,10 @@ public interface AmazonQLspClient extends LanguageClient { | |||
|
|
|||
| @JsonNotification("aws/identity/ssoTokenChanged") | |||
| void ssoTokenChanged(SsoTokenChangedParams params); | |||
|
|
|||
| @JsonNotification("aws/window/showNotification") | |||
| void showNotification(NotificationParams params); | |||
There was a problem hiding this comment.
Nothing to worry about now, but I may try to convert this to showNotifications (if Flare will agree) and send an array of Notification instead of just one.
| }); | ||
| } | ||
|
|
||
| private NotificationSeverity determineNotificationSeverity(final MessageType type) { |
There was a problem hiding this comment.
Severity will be moved to a PresentationHint, which means you'll be able to define the string values however you want. Does Java have a way of parsing strings into enums? If so, this can probably be simplified when the notification protocol changes are made and the server starts sending the updated notification format.
Or based on breedloj@ comment below, maybe Severity isn't needed at all.
| @@ -36,7 +36,10 @@ public QLspConnectionProvider() throws IOException { | |||
| var serverCommand = Paths.get(lspInstallResult.getServerDirectory(), lspInstallResult.getServerCommand()); | |||
| List<String> commands = new ArrayList<>(); | |||
| commands.add(serverCommand.toString()); | |||
| commands.add(lspInstallResult.getServerCommandArgs()); | |||
| //commands.add(lspInstallResult.getServerCommandArgs()); | |||
There was a problem hiding this comment.
Be sure to add a comment here about you're doing this for development on the feature branch and this should not be checked into main. We don't want this making it into prod.
| @@ -0,0 +1,3 @@ | |||
| package software.aws.toolkits.eclipse.amazonq.lsp.model; | |||
|
|
|||
| public record NotificationAction(String text, String type) { } | |||
There was a problem hiding this comment.
Notification actions may include a URI property (wait for the Flare protocol updates).
| * @param lspClient The LSP client to use for communication with the server | ||
| * @return The NotificationConfiguration instance | ||
| */ | ||
| public static synchronized NotificationConfiguration getInstance(AmazonQLspClient lspClient) { |
There was a problem hiding this comment.
Consider splitting this to explicit instantiation instead of lazy instantiation so that not all code that wants to call getInstance must also have an instance of AmazonQLspClient easily available as well.
| * Initialize static client states that won't change during the Eclipse session. | ||
| */ | ||
| private void initializeStaticClientStates() { | ||
| clientStates.put("IDE/VERSION", Collections.singletonList(getEclipseVersion())); |
There was a problem hiding this comment.
Consider defining these as constants in a single location (e.g. constants at the top of this class or a separate NotificationClientStates class) so they're easily discoverable and documentable in the code. The runbook will want to link to the code for explicit client state definitions and descriptions rather than being reproduced (yet another place to keep in sync) in the runbook.
Also consider camelCase for the literal client state names as people tend not to like SCREAMING_SNAKE_CASE anymore.
|
|
||
| private final AmazonQLspClient lspClient; | ||
|
|
||
| private final List<String> contexts = new ArrayList<>(); |
There was a problem hiding this comment.
Consider using a HashSet for contexts.
| * | ||
| * @return A map representing the current notification configuration | ||
| */ | ||
| public synchronized Map<String, Object> getConfiguration() { |
There was a problem hiding this comment.
Not sure this needs to be public. Will client code need this?
The format of this will be a little different, but doesn't need to be updated now.
| scheduler.schedule(() -> { | ||
| try { | ||
| lspClient.didChangeConfiguration(null); | ||
| Activator.getLogger().info("sent didChangeConfiguration notification"); |
These changes are related to an attempt of moving notifications to a common approach. This will be done in integration with Flare server.
Notification-poc
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.