feature/Notifications by aseemxs · Pull Request #390 · Amazon-Q-Developer/amazon-q-eclipse

aseemxs · 2025-03-06T22:47:56Z

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.

… local server

…Optional being used in declaration

breedloj · 2025-03-11T22:12:58Z

+ message,
+ checked -> {
+ if (checked) {
+ Activator.getPluginStore().put("notificationSkipFlag", "true");


I think we will need this to be more granular - e.g. per notification type.

breedloj · 2025-03-11T22:23:28Z

Please add additional details to your PR title & description, including screenshots of the various notification types.

…tion

floralph · 2025-03-14T18:00:05Z

 void ssoTokenChanged(SsoTokenChangedParams params);
+
+ @JsonNotification("aws/window/showNotification")
+ void showNotification(NotificationParams params);


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.

floralph · 2025-03-14T18:02:07Z

+ });
+ }
+
+ private NotificationSeverity determineNotificationSeverity(final MessageType type) {


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.

floralph · 2025-03-14T18:05:06Z

 List<String> commands = new ArrayList<>();
 commands.add(serverCommand.toString());
- commands.add(lspInstallResult.getServerCommandArgs());
+ //commands.add(lspInstallResult.getServerCommandArgs());


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.

floralph · 2025-03-14T18:05:55Z

@@ -0,0 +1,3 @@
+package software.aws.toolkits.eclipse.amazonq.lsp.model;
+
+public record NotificationAction(String text, String type) { }


Notification actions may include a URI property (wait for the Flare protocol updates).

floralph · 2025-03-17T00:17:07Z

+ * @param lspClient The LSP client to use for communication with the server
+ * @return The NotificationConfiguration instance
+ */
+	public static synchronized NotificationConfiguration getInstance(AmazonQLspClient lspClient) {


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.

floralph · 2025-03-17T00:19:14Z

+ * Initialize static client states that won't change during the Eclipse session.
+ */
+	private void initializeStaticClientStates() {
+ clientStates.put("IDE/VERSION", Collections.singletonList(getEclipseVersion()));


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.

floralph · 2025-03-17T00:50:54Z

+
+	private final AmazonQLspClient lspClient;
+
+	private final List<String> contexts = new ArrayList<>();


Consider using a HashSet for contexts.

floralph · 2025-03-17T00:54:56Z

+ * 
+ * @return A map representing the current notification configuration
+ */
+	public synchronized Map<String, Object> getConfiguration() {


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.

floralph · 2025-03-17T00:55:41Z

+ scheduler.schedule(() -> {
+ try {
+ lspClient.didChangeConfiguration(null);
+ Activator.getLogger().info("sent didChangeConfiguration notification");


typo: Capitalize "sent"

aseemxs · 2025-03-06T22:47:56Z

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.

… local server

…Optional being used in declaration

breedloj · 2025-03-11T22:12:58Z

+ message,
+ checked -> {
+ if (checked) {
+ Activator.getPluginStore().put("notificationSkipFlag", "true");


I think we will need this to be more granular - e.g. per notification type.

breedloj · 2025-03-11T22:23:28Z

Please add additional details to your PR title & description, including screenshots of the various notification types.

…tion

floralph · 2025-03-14T18:00:05Z

 void ssoTokenChanged(SsoTokenChangedParams params);
+
+ @JsonNotification("aws/window/showNotification")
+ void showNotification(NotificationParams params);


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.

floralph · 2025-03-14T18:02:07Z

+ });
+ }
+
+ private NotificationSeverity determineNotificationSeverity(final MessageType type) {


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.

floralph · 2025-03-14T18:05:06Z

 List<String> commands = new ArrayList<>();
 commands.add(serverCommand.toString());
- commands.add(lspInstallResult.getServerCommandArgs());
+ //commands.add(lspInstallResult.getServerCommandArgs());


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.

floralph · 2025-03-14T18:05:55Z

@@ -0,0 +1,3 @@
+package software.aws.toolkits.eclipse.amazonq.lsp.model;
+
+public record NotificationAction(String text, String type) { }


Notification actions may include a URI property (wait for the Flare protocol updates).

floralph · 2025-03-17T00:17:07Z

+ * @param lspClient The LSP client to use for communication with the server
+ * @return The NotificationConfiguration instance
+ */
+	public static synchronized NotificationConfiguration getInstance(AmazonQLspClient lspClient) {


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.

floralph · 2025-03-17T00:19:14Z

+ * Initialize static client states that won't change during the Eclipse session.
+ */
+	private void initializeStaticClientStates() {
+ clientStates.put("IDE/VERSION", Collections.singletonList(getEclipseVersion()));


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.

floralph · 2025-03-17T00:50:54Z

+
+	private final AmazonQLspClient lspClient;
+
+	private final List<String> contexts = new ArrayList<>();


Consider using a HashSet for contexts.

floralph · 2025-03-17T00:54:56Z

+ * 
+ * @return A map representing the current notification configuration
+ */
+	public synchronized Map<String, Object> getConfiguration() {


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.

floralph · 2025-03-17T00:55:41Z

+ scheduler.schedule(() -> {
+ try {
+ lspClient.didChangeConfiguration(null);
+ Activator.getLogger().info("sent didChangeConfiguration notification");


typo: Capitalize "sent"

aseemxs · 2025-03-06T22:47:56Z

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.

… local server

…Optional being used in declaration

breedloj · 2025-03-11T22:12:58Z

+ message,
+ checked -> {
+ if (checked) {
+ Activator.getPluginStore().put("notificationSkipFlag", "true");


I think we will need this to be more granular - e.g. per notification type.

breedloj · 2025-03-11T22:23:28Z

Please add additional details to your PR title & description, including screenshots of the various notification types.

…tion

floralph · 2025-03-14T18:00:05Z

 void ssoTokenChanged(SsoTokenChangedParams params);
+
+ @JsonNotification("aws/window/showNotification")
+ void showNotification(NotificationParams params);


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.

floralph · 2025-03-14T18:02:07Z

+ });
+ }
+
+ private NotificationSeverity determineNotificationSeverity(final MessageType type) {


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.

floralph · 2025-03-14T18:05:06Z

 List<String> commands = new ArrayList<>();
 commands.add(serverCommand.toString());
- commands.add(lspInstallResult.getServerCommandArgs());
+ //commands.add(lspInstallResult.getServerCommandArgs());


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.

floralph · 2025-03-14T18:05:55Z

@@ -0,0 +1,3 @@
+package software.aws.toolkits.eclipse.amazonq.lsp.model;
+
+public record NotificationAction(String text, String type) { }


Notification actions may include a URI property (wait for the Flare protocol updates).

floralph · 2025-03-17T00:17:07Z

+ * @param lspClient The LSP client to use for communication with the server
+ * @return The NotificationConfiguration instance
+ */
+	public static synchronized NotificationConfiguration getInstance(AmazonQLspClient lspClient) {


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.

floralph · 2025-03-17T00:19:14Z

+ * Initialize static client states that won't change during the Eclipse session.
+ */
+	private void initializeStaticClientStates() {
+ clientStates.put("IDE/VERSION", Collections.singletonList(getEclipseVersion()));


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.

floralph · 2025-03-17T00:50:54Z

+
+	private final AmazonQLspClient lspClient;
+
+	private final List<String> contexts = new ArrayList<>();


Consider using a HashSet for contexts.

floralph · 2025-03-17T00:54:56Z

+ * 
+ * @return A map representing the current notification configuration
+ */
+	public synchronized Map<String, Object> getConfiguration() {


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.

floralph · 2025-03-17T00:55:41Z

+ scheduler.schedule(() -> {
+ try {
+ lspClient.didChangeConfiguration(null);
+ Activator.getLogger().info("sent didChangeConfiguration notification");


typo: Capitalize "sent"

aseemxs · 2025-03-06T22:47:56Z

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.

… local server

…Optional being used in declaration

breedloj · 2025-03-11T22:12:58Z

+ message,
+ checked -> {
+ if (checked) {
+ Activator.getPluginStore().put("notificationSkipFlag", "true");


I think we will need this to be more granular - e.g. per notification type.

breedloj · 2025-03-11T22:23:28Z

Please add additional details to your PR title & description, including screenshots of the various notification types.

…tion

floralph · 2025-03-14T18:00:05Z

 void ssoTokenChanged(SsoTokenChangedParams params);
+
+ @JsonNotification("aws/window/showNotification")
+ void showNotification(NotificationParams params);


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.

floralph · 2025-03-14T18:02:07Z

+ });
+ }
+
+ private NotificationSeverity determineNotificationSeverity(final MessageType type) {


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.

floralph · 2025-03-14T18:05:06Z

 List<String> commands = new ArrayList<>();
 commands.add(serverCommand.toString());
- commands.add(lspInstallResult.getServerCommandArgs());
+ //commands.add(lspInstallResult.getServerCommandArgs());


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.

floralph · 2025-03-14T18:05:55Z

@@ -0,0 +1,3 @@
+package software.aws.toolkits.eclipse.amazonq.lsp.model;
+
+public record NotificationAction(String text, String type) { }


Notification actions may include a URI property (wait for the Flare protocol updates).

floralph · 2025-03-17T00:17:07Z

+ * @param lspClient The LSP client to use for communication with the server
+ * @return The NotificationConfiguration instance
+ */
+	public static synchronized NotificationConfiguration getInstance(AmazonQLspClient lspClient) {


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.

floralph · 2025-03-17T00:19:14Z

+ * Initialize static client states that won't change during the Eclipse session.
+ */
+	private void initializeStaticClientStates() {
+ clientStates.put("IDE/VERSION", Collections.singletonList(getEclipseVersion()));


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.

floralph · 2025-03-17T00:50:54Z

+
+	private final AmazonQLspClient lspClient;
+
+	private final List<String> contexts = new ArrayList<>();


Consider using a HashSet for contexts.

floralph · 2025-03-17T00:54:56Z

+ * 
+ * @return A map representing the current notification configuration
+ */
+	public synchronized Map<String, Object> getConfiguration() {


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.

floralph · 2025-03-17T00:55:41Z

+ scheduler.schedule(() -> {
+ try {
+ lspClient.didChangeConfiguration(null);
+ Activator.getLogger().info("sent didChangeConfiguration notification");


typo: Capitalize "sent"

aseemxs · 2025-03-06T22:47:56Z

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.

… local server

…Optional being used in declaration

breedloj · 2025-03-11T22:12:58Z

+ message,
+ checked -> {
+ if (checked) {
+ Activator.getPluginStore().put("notificationSkipFlag", "true");


I think we will need this to be more granular - e.g. per notification type.

breedloj · 2025-03-11T22:23:28Z

Please add additional details to your PR title & description, including screenshots of the various notification types.

…tion

floralph · 2025-03-14T18:00:05Z

 void ssoTokenChanged(SsoTokenChangedParams params);
+
+ @JsonNotification("aws/window/showNotification")
+ void showNotification(NotificationParams params);


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.

floralph · 2025-03-14T18:02:07Z

+ });
+ }
+
+ private NotificationSeverity determineNotificationSeverity(final MessageType type) {


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.

floralph · 2025-03-14T18:05:06Z

 List<String> commands = new ArrayList<>();
 commands.add(serverCommand.toString());
- commands.add(lspInstallResult.getServerCommandArgs());
+ //commands.add(lspInstallResult.getServerCommandArgs());


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.

floralph · 2025-03-14T18:05:55Z

@@ -0,0 +1,3 @@
+package software.aws.toolkits.eclipse.amazonq.lsp.model;
+
+public record NotificationAction(String text, String type) { }


Notification actions may include a URI property (wait for the Flare protocol updates).

floralph · 2025-03-17T00:17:07Z

+ * @param lspClient The LSP client to use for communication with the server
+ * @return The NotificationConfiguration instance
+ */
+	public static synchronized NotificationConfiguration getInstance(AmazonQLspClient lspClient) {


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.

floralph · 2025-03-17T00:19:14Z

+ * Initialize static client states that won't change during the Eclipse session.
+ */
+	private void initializeStaticClientStates() {
+ clientStates.put("IDE/VERSION", Collections.singletonList(getEclipseVersion()));


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.

floralph · 2025-03-17T00:50:54Z

+
+	private final AmazonQLspClient lspClient;
+
+	private final List<String> contexts = new ArrayList<>();


Consider using a HashSet for contexts.

floralph · 2025-03-17T00:54:56Z

+ * 
+ * @return A map representing the current notification configuration
+ */
+	public synchronized Map<String, Object> getConfiguration() {


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.

floralph · 2025-03-17T00:55:41Z

+ scheduler.schedule(() -> {
+ try {
+ lspClient.didChangeConfiguration(null);
+ Activator.getLogger().info("sent didChangeConfiguration notification");


typo: Capitalize "sent"

aseemxs · 2025-03-06T22:47:56Z

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.

… local server

…Optional being used in declaration

breedloj · 2025-03-11T22:12:58Z

+ message,
+ checked -> {
+ if (checked) {
+ Activator.getPluginStore().put("notificationSkipFlag", "true");


I think we will need this to be more granular - e.g. per notification type.

breedloj · 2025-03-11T22:23:28Z

Please add additional details to your PR title & description, including screenshots of the various notification types.

…tion

floralph · 2025-03-14T18:00:05Z

 void ssoTokenChanged(SsoTokenChangedParams params);
+
+ @JsonNotification("aws/window/showNotification")
+ void showNotification(NotificationParams params);


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.

floralph · 2025-03-14T18:02:07Z

+ });
+ }
+
+ private NotificationSeverity determineNotificationSeverity(final MessageType type) {


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.

floralph · 2025-03-14T18:05:06Z

 List<String> commands = new ArrayList<>();
 commands.add(serverCommand.toString());
- commands.add(lspInstallResult.getServerCommandArgs());
+ //commands.add(lspInstallResult.getServerCommandArgs());


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.

floralph · 2025-03-14T18:05:55Z

@@ -0,0 +1,3 @@
+package software.aws.toolkits.eclipse.amazonq.lsp.model;
+
+public record NotificationAction(String text, String type) { }


Notification actions may include a URI property (wait for the Flare protocol updates).

floralph · 2025-03-17T00:17:07Z

+ * @param lspClient The LSP client to use for communication with the server
+ * @return The NotificationConfiguration instance
+ */
+	public static synchronized NotificationConfiguration getInstance(AmazonQLspClient lspClient) {


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.

floralph · 2025-03-17T00:19:14Z

+ * Initialize static client states that won't change during the Eclipse session.
+ */
+	private void initializeStaticClientStates() {
+ clientStates.put("IDE/VERSION", Collections.singletonList(getEclipseVersion()));


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.

floralph · 2025-03-17T00:50:54Z

+
+	private final AmazonQLspClient lspClient;
+
+	private final List<String> contexts = new ArrayList<>();


Consider using a HashSet for contexts.

floralph · 2025-03-17T00:54:56Z

+ * 
+ * @return A map representing the current notification configuration
+ */
+	public synchronized Map<String, Object> getConfiguration() {


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.

floralph · 2025-03-17T00:55:41Z

+ scheduler.schedule(() -> {
+ try {
+ lspClient.didChangeConfiguration(null);
+ Activator.getLogger().info("sent didChangeConfiguration notification");


typo: Capitalize "sent"

aseemxs · 2025-03-06T22:47:56Z

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.

… local server

…Optional being used in declaration

breedloj · 2025-03-11T22:12:58Z

+ message,
+ checked -> {
+ if (checked) {
+ Activator.getPluginStore().put("notificationSkipFlag", "true");


I think we will need this to be more granular - e.g. per notification type.

breedloj · 2025-03-11T22:23:28Z

Please add additional details to your PR title & description, including screenshots of the various notification types.

…tion

floralph · 2025-03-14T18:00:05Z

 void ssoTokenChanged(SsoTokenChangedParams params);
+
+ @JsonNotification("aws/window/showNotification")
+ void showNotification(NotificationParams params);


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.

floralph · 2025-03-14T18:02:07Z

+ });
+ }
+
+ private NotificationSeverity determineNotificationSeverity(final MessageType type) {


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.

floralph · 2025-03-14T18:05:06Z

 List<String> commands = new ArrayList<>();
 commands.add(serverCommand.toString());
- commands.add(lspInstallResult.getServerCommandArgs());
+ //commands.add(lspInstallResult.getServerCommandArgs());


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.

floralph · 2025-03-14T18:05:55Z

@@ -0,0 +1,3 @@
+package software.aws.toolkits.eclipse.amazonq.lsp.model;
+
+public record NotificationAction(String text, String type) { }


Notification actions may include a URI property (wait for the Flare protocol updates).

floralph · 2025-03-17T00:17:07Z

+ * @param lspClient The LSP client to use for communication with the server
+ * @return The NotificationConfiguration instance
+ */
+	public static synchronized NotificationConfiguration getInstance(AmazonQLspClient lspClient) {


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.

floralph · 2025-03-17T00:19:14Z

+ * Initialize static client states that won't change during the Eclipse session.
+ */
+	private void initializeStaticClientStates() {
+ clientStates.put("IDE/VERSION", Collections.singletonList(getEclipseVersion()));


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.

floralph · 2025-03-17T00:50:54Z

+
+	private final AmazonQLspClient lspClient;
+
+	private final List<String> contexts = new ArrayList<>();


Consider using a HashSet for contexts.

floralph · 2025-03-17T00:54:56Z

+ * 
+ * @return A map representing the current notification configuration
+ */
+	public synchronized Map<String, Object> getConfiguration() {


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.

floralph · 2025-03-17T00:55:41Z

+ scheduler.schedule(() -> {
+ try {
+ lspClient.didChangeConfiguration(null);
+ Activator.getLogger().info("sent didChangeConfiguration notification");


typo: Capitalize "sent"

aseemxs · 2025-03-06T22:47:56Z

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.

… local server

…Optional being used in declaration

breedloj · 2025-03-11T22:12:58Z

+ message,
+ checked -> {
+ if (checked) {
+ Activator.getPluginStore().put("notificationSkipFlag", "true");


I think we will need this to be more granular - e.g. per notification type.

breedloj · 2025-03-11T22:23:28Z

Please add additional details to your PR title & description, including screenshots of the various notification types.

…tion

floralph · 2025-03-14T18:00:05Z

 void ssoTokenChanged(SsoTokenChangedParams params);
+
+ @JsonNotification("aws/window/showNotification")
+ void showNotification(NotificationParams params);


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.

floralph · 2025-03-14T18:02:07Z

+ });
+ }
+
+ private NotificationSeverity determineNotificationSeverity(final MessageType type) {


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.

floralph · 2025-03-14T18:05:06Z

 List<String> commands = new ArrayList<>();
 commands.add(serverCommand.toString());
- commands.add(lspInstallResult.getServerCommandArgs());
+ //commands.add(lspInstallResult.getServerCommandArgs());


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.

floralph · 2025-03-14T18:05:55Z

@@ -0,0 +1,3 @@
+package software.aws.toolkits.eclipse.amazonq.lsp.model;
+
+public record NotificationAction(String text, String type) { }


Notification actions may include a URI property (wait for the Flare protocol updates).

floralph · 2025-03-17T00:17:07Z

+ * @param lspClient The LSP client to use for communication with the server
+ * @return The NotificationConfiguration instance
+ */
+	public static synchronized NotificationConfiguration getInstance(AmazonQLspClient lspClient) {


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.

floralph · 2025-03-17T00:19:14Z

+ * Initialize static client states that won't change during the Eclipse session.
+ */
+	private void initializeStaticClientStates() {
+ clientStates.put("IDE/VERSION", Collections.singletonList(getEclipseVersion()));


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.

floralph · 2025-03-17T00:50:54Z

+
+	private final AmazonQLspClient lspClient;
+
+	private final List<String> contexts = new ArrayList<>();


Consider using a HashSet for contexts.

floralph · 2025-03-17T00:54:56Z

+ * 
+ * @return A map representing the current notification configuration
+ */
+	public synchronized Map<String, Object> getConfiguration() {


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.

floralph · 2025-03-17T00:55:41Z

+ scheduler.schedule(() -> {
+ try {
+ lspClient.didChangeConfiguration(null);
+ Activator.getLogger().info("sent didChangeConfiguration notification");


typo: Capitalize "sent"

aseemxs added 2 commits March 4, 2025 14:31

Initial implementation of notifications feature without connection to…
0730975
… local server

Connection to local server established and removed errors related to …
ff4e3bf
…Optional being used in declaration

aseemxs requested a review from breedloj March 6, 2025 22:48

aseemxs changed the title ~~Notifications poc aseem~~feature/NotificationsMar 10, 2025

aseemxs added 2 commits March 11, 2025 12:34

Different types of severity added
005b6ee

Checkstyle corrections
a747467

breedloj reviewed Mar 11, 2025
View reviewed changes

aseemxs added 2 commits March 12, 2025 11:36

Notification configuration added
85a3f4b

Added didChangeConfiguration to AmazonQLspClient.java
d89007e

aseemxs marked this pull request as draft March 14, 2025 18:08

Resolved comments on PR#390, Resolved errors in NotificationConfigura…
6c5a9cd
…tion

floralph reviewed Mar 17, 2025
View reviewed changes

aseemxs added 2 commits March 4, 2025 14:31

Initial implementation of notifications feature without connection to…
0730975
… local server

Connection to local server established and removed errors related to …
ff4e3bf
…Optional being used in declaration

aseemxs requested a review from breedloj March 6, 2025 22:48

aseemxs changed the title ~~Notifications poc aseem~~feature/NotificationsMar 10, 2025

aseemxs added 2 commits March 11, 2025 12:34

Different types of severity added
005b6ee

Checkstyle corrections
a747467

breedloj reviewed Mar 11, 2025
View reviewed changes

aseemxs added 2 commits March 12, 2025 11:36

Notification configuration added
85a3f4b

Added didChangeConfiguration to AmazonQLspClient.java
d89007e

aseemxs marked this pull request as draft March 14, 2025 18:08

Resolved comments on PR#390, Resolved errors in NotificationConfigura…
6c5a9cd
…tion

floralph reviewed Mar 17, 2025
View reviewed changes

aseemxs added 2 commits March 4, 2025 14:31

Initial implementation of notifications feature without connection to…
0730975
… local server

Connection to local server established and removed errors related to …
ff4e3bf
…Optional being used in declaration

aseemxs requested a review from breedloj March 6, 2025 22:48

aseemxs changed the title ~~Notifications poc aseem~~feature/NotificationsMar 10, 2025

aseemxs added 2 commits March 11, 2025 12:34

Different types of severity added
005b6ee

Checkstyle corrections
a747467

breedloj reviewed Mar 11, 2025
View reviewed changes

aseemxs added 2 commits March 12, 2025 11:36

Notification configuration added
85a3f4b

Added didChangeConfiguration to AmazonQLspClient.java
d89007e

aseemxs marked this pull request as draft March 14, 2025 18:08

Resolved comments on PR#390, Resolved errors in NotificationConfigura…
6c5a9cd
…tion

floralph reviewed Mar 17, 2025
View reviewed changes

aseemxs added 2 commits March 4, 2025 14:31

Initial implementation of notifications feature without connection to…
0730975
… local server

Connection to local server established and removed errors related to …
ff4e3bf
…Optional being used in declaration

aseemxs requested a review from breedloj March 6, 2025 22:48

aseemxs changed the title ~~Notifications poc aseem~~feature/NotificationsMar 10, 2025

aseemxs added 2 commits March 11, 2025 12:34

Different types of severity added
005b6ee

Checkstyle corrections
a747467

breedloj reviewed Mar 11, 2025
View reviewed changes

aseemxs added 2 commits March 12, 2025 11:36

Notification configuration added
85a3f4b

Added didChangeConfiguration to AmazonQLspClient.java
d89007e

aseemxs marked this pull request as draft March 14, 2025 18:08

Resolved comments on PR#390, Resolved errors in NotificationConfigura…
6c5a9cd
…tion

floralph reviewed Mar 17, 2025
View reviewed changes

aseemxs added 2 commits March 4, 2025 14:31

Initial implementation of notifications feature without connection to…
0730975
… local server

Connection to local server established and removed errors related to …
ff4e3bf
…Optional being used in declaration

aseemxs requested a review from breedloj March 6, 2025 22:48

aseemxs changed the title ~~Notifications poc aseem~~feature/NotificationsMar 10, 2025

aseemxs added 2 commits March 11, 2025 12:34

Different types of severity added
005b6ee

Checkstyle corrections
a747467

breedloj reviewed Mar 11, 2025
View reviewed changes

aseemxs added 2 commits March 12, 2025 11:36

Notification configuration added
85a3f4b

Added didChangeConfiguration to AmazonQLspClient.java
d89007e

aseemxs marked this pull request as draft March 14, 2025 18:08

Resolved comments on PR#390, Resolved errors in NotificationConfigura…
6c5a9cd
…tion

floralph reviewed Mar 17, 2025
View reviewed changes

aseemxs added 2 commits March 4, 2025 14:31

Initial implementation of notifications feature without connection to…
0730975
… local server

Connection to local server established and removed errors related to …
ff4e3bf
…Optional being used in declaration

aseemxs requested a review from breedloj March 6, 2025 22:48

aseemxs changed the title ~~Notifications poc aseem~~feature/NotificationsMar 10, 2025

aseemxs added 2 commits March 11, 2025 12:34

Different types of severity added
005b6ee

Checkstyle corrections
a747467

breedloj reviewed Mar 11, 2025
View reviewed changes

aseemxs added 2 commits March 12, 2025 11:36

Notification configuration added
85a3f4b

Added didChangeConfiguration to AmazonQLspClient.java
d89007e

aseemxs marked this pull request as draft March 14, 2025 18:08

Resolved comments on PR#390, Resolved errors in NotificationConfigura…
6c5a9cd
…tion

floralph reviewed Mar 17, 2025
View reviewed changes

aseemxs added 2 commits March 4, 2025 14:31

Initial implementation of notifications feature without connection to…
0730975
… local server

Connection to local server established and removed errors related to …
ff4e3bf
…Optional being used in declaration

aseemxs requested a review from breedloj March 6, 2025 22:48

aseemxs changed the title ~~Notifications poc aseem~~feature/NotificationsMar 10, 2025

aseemxs added 2 commits March 11, 2025 12:34

Different types of severity added
005b6ee

Checkstyle corrections
a747467

breedloj reviewed Mar 11, 2025
View reviewed changes

aseemxs added 2 commits March 12, 2025 11:36

Notification configuration added
85a3f4b

Added didChangeConfiguration to AmazonQLspClient.java
d89007e

aseemxs marked this pull request as draft March 14, 2025 18:08

Resolved comments on PR#390, Resolved errors in NotificationConfigura…
6c5a9cd
…tion

floralph reviewed Mar 17, 2025
View reviewed changes

aseemxs added 2 commits March 4, 2025 14:31

Initial implementation of notifications feature without connection to…
0730975
… local server

Connection to local server established and removed errors related to …
ff4e3bf
…Optional being used in declaration

aseemxs requested a review from breedloj March 6, 2025 22:48

aseemxs changed the title ~~Notifications poc aseem~~feature/NotificationsMar 10, 2025

aseemxs added 2 commits March 11, 2025 12:34

Different types of severity added
005b6ee

Checkstyle corrections
a747467

breedloj reviewed Mar 11, 2025
View reviewed changes

aseemxs added 2 commits March 12, 2025 11:36

Notification configuration added
85a3f4b

Added didChangeConfiguration to AmazonQLspClient.java
d89007e

aseemxs marked this pull request as draft March 14, 2025 18:08

Resolved comments on PR#390, Resolved errors in NotificationConfigura…
6c5a9cd
…tion

floralph reviewed Mar 17, 2025
View reviewed changes

		@@ -0,0 +1,3 @@
		package software.aws.toolkits.eclipse.amazonq.lsp.model;

		public record NotificationAction(String text, String type) { }


		private final AmazonQLspClient lspClient;

		private final List<String> contexts = new ArrayList<>();

Uh oh!

Conversation

aseemxs commented Mar 6, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

breedloj commented Mar 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

aseemxs commented Mar 6, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

breedloj commented Mar 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

aseemxs commented Mar 6, 2025•
edited
Loading

aseemxs commented Mar 6, 2025•
edited
Loading

aseemxs commented Mar 6, 2025•
edited
Loading