-
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
Trigger customization notifications on ide, stacking multiple notifications vertically and refactor/add support for more methods in PluginStore #48
Conversation
…ustomization preference storage
PluginStore.put(Constants.CUSTOMIZATION_STORAGE_INTERNAL_KEY, this.selectedCustomisationArn); | ||
Display.getCurrent().asyncExec(() -> showNotification(Constants.DEFAULT_Q_FOUNDATION_DISPLAY_NAME)); | ||
} else if (Objects.nonNull(this.getSelectedCustomization()) && StringUtils.isNotBlank(this.getSelectedCustomization().getName())) { | ||
PluginStore.putObject(Constants.CUSTOMIZATION_STORAGE_INTERNAL_KEY, this.getSelectedCustomization()); |
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.
Can we not just store the ARN?
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.
Earlier I was just storing ARN only but I need to have name (or maybe description as well later) for showing notifications. So instead of creating 3 different getter/setter for all of these, I changed the implementation to just have a single setter/getter.
synchronized (lock) { | ||
activeNotifications.add(this); | ||
} |
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.
Having a synchronized
block here adds no value since the underlying call to add
is thread safe since you're using a CopyOnWriteArrayList
. It would make sense if you are trying to perform several operations together in an atomic unit of work.
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.
makes sense, I can actually remove the synchronized block. Thanks for this.
String jsonValue = GSON.toJson(value); | ||
byte[] byteValue = jsonValue.getBytes(StandardCharsets.UTF_8); | ||
PREFERENCES.putByteArray(key, byteValue); |
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.
While this technically can be done serializing entire objects does present additional risks. In general I would like to be convinced that use cases require it, vs. binding relational configuration under a common key namespace, e.g.:
notifications.foo
: 123
notifications.bar
: 456
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.
For the same reason which I described here https://github.com/aws/amazon-q-eclipse/pull/48/files#r1787324480
Also, currently when LSP server requests for aws.q, we only return customization object but maybe in near future - we might be returning more than just one single row item. Then in those kindof cases, it would be better to just do a single lookup and return it.
…clipse into paras/fetchCustomizations
6b587c6
to
013f639
Compare
…paras/triggerCustomizationNotificationsOnIDE
Add missing import missed during merge conflict
Reference ticket: https://issues.amazon.com/issues/ECLIPSE-297