Trigger customization notifications on ide, stacking multiple notifications vertically and refactor/add support for more methods in PluginStore - #48

Merged
pras0131 merged 13 commits into
mainfrom
paras/triggerCustomizationNotificationsOnIDE
Oct 4, 2024
Merged

Trigger customization notifications on ide, stacking multiple notifications vertically and refactor/add support for more methods in PluginStore#48
pras0131 merged 13 commits into
mainfrom
paras/triggerCustomizationNotificationsOnIDE

Conversation

@pras0131

@pras0131pras0131 commented Oct 2, 2024

Copy link
Copy Markdown
Contributor

Reference ticket: https://issues.amazon.com/issues/ECLIPSE-297

  • Trigger IDE notifications on customization updates or selection of default foundation.
  • The PR also includes changes to stack multiple notifications vertically and the logic to reposition notifications once any of the notification is closed. The CRUX of the implementation is AbstractNotificationPopup initializeBounds() method, which has been overriden according to our requirements and repositioning of notifications once the older ones starts to flash out.
  • Support for two new methods - putObject and getObject in the PluginStore to ease set/get for more complex objects other than just plain strings.
  • Refactored the customizations preference storage code to let it store the entire customization object, rather than storing each of the individual fields.
Screenshot 2024-10-03 at 10 48 38

@pras0131
pras0131 changed the base branch from main to paras/fetchCustomizationsOctober 2, 2024 13:45
@pras0131pras0131 changed the title Trigger customization notifications on ideTrigger customization notifications on ide and stacking multiple notifications verticallyOct 3, 2024
@pras0131pras0131 changed the title Trigger customization notifications on ide and stacking multiple notifications verticallyTrigger customization notifications on ide, stacking multiple notifications vertically and refactor/add support for more methods in PluginStoreOct 3, 2024
@pras0131
pras0131 requested a review from breedlojOctober 3, 2024 12:45
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());

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
ContributorAuthor

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.

Comment on lines +84 to +86
synchronized (lock) {
activeNotifications.add(this);
}

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
ContributorAuthor

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.

Comment on lines +38 to +40
String jsonValue = GSON.toJson(value);
byte[] byteValue = jsonValue.getBytes(StandardCharsets.UTF_8);
PREFERENCES.putByteArray(key, byteValue);

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
ContributorAuthor

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.

@pras0131
pras0131force-pushed the paras/triggerCustomizationNotificationsOnIDE branch from 6b587c6 to 013f639CompareOctober 4, 2024 13:41
Base automatically changed from paras/fetchCustomizations to mainOctober 4, 2024 22:25
@pras0131
pras0131 merged commit 7e8933a into mainOct 4, 2024
@pras0131
pras0131 deleted the paras/triggerCustomizationNotificationsOnIDE branch October 4, 2024 22:42
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@pras0131@breedloj
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Trigger customization notifications on ide, stacking multiple notifications vertically and refactor/add support for more methods in PluginStore - #48

Merged
pras0131 merged 13 commits into
mainfrom
paras/triggerCustomizationNotificationsOnIDE
Oct 4, 2024
Merged

Trigger customization notifications on ide, stacking multiple notifications vertically and refactor/add support for more methods in PluginStore#48
pras0131 merged 13 commits into
mainfrom
paras/triggerCustomizationNotificationsOnIDE

Conversation

@pras0131

@pras0131pras0131 commented Oct 2, 2024

Copy link
Copy Markdown
Contributor

Reference ticket: https://issues.amazon.com/issues/ECLIPSE-297

  • Trigger IDE notifications on customization updates or selection of default foundation.
  • The PR also includes changes to stack multiple notifications vertically and the logic to reposition notifications once any of the notification is closed. The CRUX of the implementation is AbstractNotificationPopup initializeBounds() method, which has been overriden according to our requirements and repositioning of notifications once the older ones starts to flash out.
  • Support for two new methods - putObject and getObject in the PluginStore to ease set/get for more complex objects other than just plain strings.
  • Refactored the customizations preference storage code to let it store the entire customization object, rather than storing each of the individual fields.
Screenshot 2024-10-03 at 10 48 38

@pras0131
pras0131 changed the base branch from main to paras/fetchCustomizationsOctober 2, 2024 13:45
@pras0131pras0131 changed the title Trigger customization notifications on ideTrigger customization notifications on ide and stacking multiple notifications verticallyOct 3, 2024
@pras0131pras0131 changed the title Trigger customization notifications on ide and stacking multiple notifications verticallyTrigger customization notifications on ide, stacking multiple notifications vertically and refactor/add support for more methods in PluginStoreOct 3, 2024
@pras0131
pras0131 requested a review from breedlojOctober 3, 2024 12:45
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());

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
ContributorAuthor

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.

Comment on lines +84 to +86
synchronized (lock) {
activeNotifications.add(this);
}

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
ContributorAuthor

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.

Comment on lines +38 to +40
String jsonValue = GSON.toJson(value);
byte[] byteValue = jsonValue.getBytes(StandardCharsets.UTF_8);
PREFERENCES.putByteArray(key, byteValue);

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
ContributorAuthor

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.

@pras0131
pras0131force-pushed the paras/triggerCustomizationNotificationsOnIDE branch from 6b587c6 to 013f639CompareOctober 4, 2024 13:41
Base automatically changed from paras/fetchCustomizations to mainOctober 4, 2024 22:25
@pras0131
pras0131 merged commit 7e8933a into mainOct 4, 2024
@pras0131
pras0131 deleted the paras/triggerCustomizationNotificationsOnIDE branch October 4, 2024 22:42
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@pras0131@breedloj
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Trigger customization notifications on ide, stacking multiple notifications vertically and refactor/add support for more methods in PluginStore - #48

Merged
pras0131 merged 13 commits into
mainfrom
paras/triggerCustomizationNotificationsOnIDE
Oct 4, 2024
Merged

Trigger customization notifications on ide, stacking multiple notifications vertically and refactor/add support for more methods in PluginStore#48
pras0131 merged 13 commits into
mainfrom
paras/triggerCustomizationNotificationsOnIDE

Conversation

@pras0131

@pras0131pras0131 commented Oct 2, 2024

Copy link
Copy Markdown
Contributor

Reference ticket: https://issues.amazon.com/issues/ECLIPSE-297

  • Trigger IDE notifications on customization updates or selection of default foundation.
  • The PR also includes changes to stack multiple notifications vertically and the logic to reposition notifications once any of the notification is closed. The CRUX of the implementation is AbstractNotificationPopup initializeBounds() method, which has been overriden according to our requirements and repositioning of notifications once the older ones starts to flash out.
  • Support for two new methods - putObject and getObject in the PluginStore to ease set/get for more complex objects other than just plain strings.
  • Refactored the customizations preference storage code to let it store the entire customization object, rather than storing each of the individual fields.
Screenshot 2024-10-03 at 10 48 38

@pras0131
pras0131 changed the base branch from main to paras/fetchCustomizationsOctober 2, 2024 13:45
@pras0131pras0131 changed the title Trigger customization notifications on ideTrigger customization notifications on ide and stacking multiple notifications verticallyOct 3, 2024
@pras0131pras0131 changed the title Trigger customization notifications on ide and stacking multiple notifications verticallyTrigger customization notifications on ide, stacking multiple notifications vertically and refactor/add support for more methods in PluginStoreOct 3, 2024
@pras0131
pras0131 requested a review from breedlojOctober 3, 2024 12:45
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());

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
ContributorAuthor

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.

Comment on lines +84 to +86
synchronized (lock) {
activeNotifications.add(this);
}

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
ContributorAuthor

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.

Comment on lines +38 to +40
String jsonValue = GSON.toJson(value);
byte[] byteValue = jsonValue.getBytes(StandardCharsets.UTF_8);
PREFERENCES.putByteArray(key, byteValue);

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
ContributorAuthor

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.

@pras0131
pras0131force-pushed the paras/triggerCustomizationNotificationsOnIDE branch from 6b587c6 to 013f639CompareOctober 4, 2024 13:41
Base automatically changed from paras/fetchCustomizations to mainOctober 4, 2024 22:25
@pras0131
pras0131 merged commit 7e8933a into mainOct 4, 2024
@pras0131
pras0131 deleted the paras/triggerCustomizationNotificationsOnIDE branch October 4, 2024 22:42
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@pras0131@breedloj
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Trigger customization notifications on ide, stacking multiple notifications vertically and refactor/add support for more methods in PluginStore - #48

Merged
pras0131 merged 13 commits into
mainfrom
paras/triggerCustomizationNotificationsOnIDE
Oct 4, 2024
Merged

Trigger customization notifications on ide, stacking multiple notifications vertically and refactor/add support for more methods in PluginStore#48
pras0131 merged 13 commits into
mainfrom
paras/triggerCustomizationNotificationsOnIDE

Conversation

@pras0131

@pras0131pras0131 commented Oct 2, 2024

Copy link
Copy Markdown
Contributor

Reference ticket: https://issues.amazon.com/issues/ECLIPSE-297

  • Trigger IDE notifications on customization updates or selection of default foundation.
  • The PR also includes changes to stack multiple notifications vertically and the logic to reposition notifications once any of the notification is closed. The CRUX of the implementation is AbstractNotificationPopup initializeBounds() method, which has been overriden according to our requirements and repositioning of notifications once the older ones starts to flash out.
  • Support for two new methods - putObject and getObject in the PluginStore to ease set/get for more complex objects other than just plain strings.
  • Refactored the customizations preference storage code to let it store the entire customization object, rather than storing each of the individual fields.
Screenshot 2024-10-03 at 10 48 38

@pras0131
pras0131 changed the base branch from main to paras/fetchCustomizationsOctober 2, 2024 13:45
@pras0131pras0131 changed the title Trigger customization notifications on ideTrigger customization notifications on ide and stacking multiple notifications verticallyOct 3, 2024
@pras0131pras0131 changed the title Trigger customization notifications on ide and stacking multiple notifications verticallyTrigger customization notifications on ide, stacking multiple notifications vertically and refactor/add support for more methods in PluginStoreOct 3, 2024
@pras0131
pras0131 requested a review from breedlojOctober 3, 2024 12:45
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());

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
ContributorAuthor

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.

Comment on lines +84 to +86
synchronized (lock) {
activeNotifications.add(this);
}

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
ContributorAuthor

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.

Comment on lines +38 to +40
String jsonValue = GSON.toJson(value);
byte[] byteValue = jsonValue.getBytes(StandardCharsets.UTF_8);
PREFERENCES.putByteArray(key, byteValue);

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
ContributorAuthor

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.

@pras0131
pras0131force-pushed the paras/triggerCustomizationNotificationsOnIDE branch from 6b587c6 to 013f639CompareOctober 4, 2024 13:41
Base automatically changed from paras/fetchCustomizations to mainOctober 4, 2024 22:25
@pras0131
pras0131 merged commit 7e8933a into mainOct 4, 2024
@pras0131
pras0131 deleted the paras/triggerCustomizationNotificationsOnIDE branch October 4, 2024 22:42
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@pras0131@breedloj
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Trigger customization notifications on ide, stacking multiple notifications vertically and refactor/add support for more methods in PluginStore - #48

Merged
pras0131 merged 13 commits into
mainfrom
paras/triggerCustomizationNotificationsOnIDE
Oct 4, 2024
Merged

Trigger customization notifications on ide, stacking multiple notifications vertically and refactor/add support for more methods in PluginStore#48
pras0131 merged 13 commits into
mainfrom
paras/triggerCustomizationNotificationsOnIDE

Conversation

@pras0131

@pras0131pras0131 commented Oct 2, 2024

Copy link
Copy Markdown
Contributor

Reference ticket: https://issues.amazon.com/issues/ECLIPSE-297

  • Trigger IDE notifications on customization updates or selection of default foundation.
  • The PR also includes changes to stack multiple notifications vertically and the logic to reposition notifications once any of the notification is closed. The CRUX of the implementation is AbstractNotificationPopup initializeBounds() method, which has been overriden according to our requirements and repositioning of notifications once the older ones starts to flash out.
  • Support for two new methods - putObject and getObject in the PluginStore to ease set/get for more complex objects other than just plain strings.
  • Refactored the customizations preference storage code to let it store the entire customization object, rather than storing each of the individual fields.
Screenshot 2024-10-03 at 10 48 38

@pras0131
pras0131 changed the base branch from main to paras/fetchCustomizationsOctober 2, 2024 13:45
@pras0131pras0131 changed the title Trigger customization notifications on ideTrigger customization notifications on ide and stacking multiple notifications verticallyOct 3, 2024
@pras0131pras0131 changed the title Trigger customization notifications on ide and stacking multiple notifications verticallyTrigger customization notifications on ide, stacking multiple notifications vertically and refactor/add support for more methods in PluginStoreOct 3, 2024
@pras0131
pras0131 requested a review from breedlojOctober 3, 2024 12:45
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());

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
ContributorAuthor

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.

Comment on lines +84 to +86
synchronized (lock) {
activeNotifications.add(this);
}

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
ContributorAuthor

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.

Comment on lines +38 to +40
String jsonValue = GSON.toJson(value);
byte[] byteValue = jsonValue.getBytes(StandardCharsets.UTF_8);
PREFERENCES.putByteArray(key, byteValue);

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
ContributorAuthor

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.

@pras0131
pras0131force-pushed the paras/triggerCustomizationNotificationsOnIDE branch from 6b587c6 to 013f639CompareOctober 4, 2024 13:41
Base automatically changed from paras/fetchCustomizations to mainOctober 4, 2024 22:25
@pras0131
pras0131 merged commit 7e8933a into mainOct 4, 2024
@pras0131
pras0131 deleted the paras/triggerCustomizationNotificationsOnIDE branch October 4, 2024 22:42
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@pras0131@breedloj
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Trigger customization notifications on ide, stacking multiple notifications vertically and refactor/add support for more methods in PluginStore - #48

Merged
pras0131 merged 13 commits into
mainfrom
paras/triggerCustomizationNotificationsOnIDE
Oct 4, 2024
Merged

Trigger customization notifications on ide, stacking multiple notifications vertically and refactor/add support for more methods in PluginStore#48
pras0131 merged 13 commits into
mainfrom
paras/triggerCustomizationNotificationsOnIDE

Conversation

@pras0131

@pras0131pras0131 commented Oct 2, 2024

Copy link
Copy Markdown
Contributor

Reference ticket: https://issues.amazon.com/issues/ECLIPSE-297

  • Trigger IDE notifications on customization updates or selection of default foundation.
  • The PR also includes changes to stack multiple notifications vertically and the logic to reposition notifications once any of the notification is closed. The CRUX of the implementation is AbstractNotificationPopup initializeBounds() method, which has been overriden according to our requirements and repositioning of notifications once the older ones starts to flash out.
  • Support for two new methods - putObject and getObject in the PluginStore to ease set/get for more complex objects other than just plain strings.
  • Refactored the customizations preference storage code to let it store the entire customization object, rather than storing each of the individual fields.
Screenshot 2024-10-03 at 10 48 38

@pras0131
pras0131 changed the base branch from main to paras/fetchCustomizationsOctober 2, 2024 13:45
@pras0131pras0131 changed the title Trigger customization notifications on ideTrigger customization notifications on ide and stacking multiple notifications verticallyOct 3, 2024
@pras0131pras0131 changed the title Trigger customization notifications on ide and stacking multiple notifications verticallyTrigger customization notifications on ide, stacking multiple notifications vertically and refactor/add support for more methods in PluginStoreOct 3, 2024
@pras0131
pras0131 requested a review from breedlojOctober 3, 2024 12:45
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());

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
ContributorAuthor

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.

Comment on lines +84 to +86
synchronized (lock) {
activeNotifications.add(this);
}

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
ContributorAuthor

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.

Comment on lines +38 to +40
String jsonValue = GSON.toJson(value);
byte[] byteValue = jsonValue.getBytes(StandardCharsets.UTF_8);
PREFERENCES.putByteArray(key, byteValue);

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
ContributorAuthor

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.

@pras0131
pras0131force-pushed the paras/triggerCustomizationNotificationsOnIDE branch from 6b587c6 to 013f639CompareOctober 4, 2024 13:41
Base automatically changed from paras/fetchCustomizations to mainOctober 4, 2024 22:25
@pras0131
pras0131 merged commit 7e8933a into mainOct 4, 2024
@pras0131
pras0131 deleted the paras/triggerCustomizationNotificationsOnIDE branch October 4, 2024 22:42
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@pras0131@breedloj
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Trigger customization notifications on ide, stacking multiple notifications vertically and refactor/add support for more methods in PluginStore - #48

Merged
pras0131 merged 13 commits into
mainfrom
paras/triggerCustomizationNotificationsOnIDE
Oct 4, 2024
Merged

Trigger customization notifications on ide, stacking multiple notifications vertically and refactor/add support for more methods in PluginStore#48
pras0131 merged 13 commits into
mainfrom
paras/triggerCustomizationNotificationsOnIDE

Conversation

@pras0131

@pras0131pras0131 commented Oct 2, 2024

Copy link
Copy Markdown
Contributor

Reference ticket: https://issues.amazon.com/issues/ECLIPSE-297

  • Trigger IDE notifications on customization updates or selection of default foundation.
  • The PR also includes changes to stack multiple notifications vertically and the logic to reposition notifications once any of the notification is closed. The CRUX of the implementation is AbstractNotificationPopup initializeBounds() method, which has been overriden according to our requirements and repositioning of notifications once the older ones starts to flash out.
  • Support for two new methods - putObject and getObject in the PluginStore to ease set/get for more complex objects other than just plain strings.
  • Refactored the customizations preference storage code to let it store the entire customization object, rather than storing each of the individual fields.
Screenshot 2024-10-03 at 10 48 38

@pras0131
pras0131 changed the base branch from main to paras/fetchCustomizationsOctober 2, 2024 13:45
@pras0131pras0131 changed the title Trigger customization notifications on ideTrigger customization notifications on ide and stacking multiple notifications verticallyOct 3, 2024
@pras0131pras0131 changed the title Trigger customization notifications on ide and stacking multiple notifications verticallyTrigger customization notifications on ide, stacking multiple notifications vertically and refactor/add support for more methods in PluginStoreOct 3, 2024
@pras0131
pras0131 requested a review from breedlojOctober 3, 2024 12:45
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());

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
ContributorAuthor

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.

Comment on lines +84 to +86
synchronized (lock) {
activeNotifications.add(this);
}

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
ContributorAuthor

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.

Comment on lines +38 to +40
String jsonValue = GSON.toJson(value);
byte[] byteValue = jsonValue.getBytes(StandardCharsets.UTF_8);
PREFERENCES.putByteArray(key, byteValue);

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
ContributorAuthor

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.

@pras0131
pras0131force-pushed the paras/triggerCustomizationNotificationsOnIDE branch from 6b587c6 to 013f639CompareOctober 4, 2024 13:41
Base automatically changed from paras/fetchCustomizations to mainOctober 4, 2024 22:25
@pras0131
pras0131 merged commit 7e8933a into mainOct 4, 2024
@pras0131
pras0131 deleted the paras/triggerCustomizationNotificationsOnIDE branch October 4, 2024 22:42
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@pras0131@breedloj
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Trigger customization notifications on ide, stacking multiple notifications vertically and refactor/add support for more methods in PluginStore - #48

Merged
pras0131 merged 13 commits into
mainfrom
paras/triggerCustomizationNotificationsOnIDE
Oct 4, 2024
Merged

Trigger customization notifications on ide, stacking multiple notifications vertically and refactor/add support for more methods in PluginStore#48
pras0131 merged 13 commits into
mainfrom
paras/triggerCustomizationNotificationsOnIDE

Conversation

@pras0131

@pras0131pras0131 commented Oct 2, 2024

Copy link
Copy Markdown
Contributor

Reference ticket: https://issues.amazon.com/issues/ECLIPSE-297

  • Trigger IDE notifications on customization updates or selection of default foundation.
  • The PR also includes changes to stack multiple notifications vertically and the logic to reposition notifications once any of the notification is closed. The CRUX of the implementation is AbstractNotificationPopup initializeBounds() method, which has been overriden according to our requirements and repositioning of notifications once the older ones starts to flash out.
  • Support for two new methods - putObject and getObject in the PluginStore to ease set/get for more complex objects other than just plain strings.
  • Refactored the customizations preference storage code to let it store the entire customization object, rather than storing each of the individual fields.
Screenshot 2024-10-03 at 10 48 38

@pras0131
pras0131 changed the base branch from main to paras/fetchCustomizationsOctober 2, 2024 13:45
@pras0131pras0131 changed the title Trigger customization notifications on ideTrigger customization notifications on ide and stacking multiple notifications verticallyOct 3, 2024
@pras0131pras0131 changed the title Trigger customization notifications on ide and stacking multiple notifications verticallyTrigger customization notifications on ide, stacking multiple notifications vertically and refactor/add support for more methods in PluginStoreOct 3, 2024
@pras0131
pras0131 requested a review from breedlojOctober 3, 2024 12:45
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());

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
ContributorAuthor

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.

Comment on lines +84 to +86
synchronized (lock) {
activeNotifications.add(this);
}

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
ContributorAuthor

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.

Comment on lines +38 to +40
String jsonValue = GSON.toJson(value);
byte[] byteValue = jsonValue.getBytes(StandardCharsets.UTF_8);
PREFERENCES.putByteArray(key, byteValue);

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
ContributorAuthor

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.

@pras0131
pras0131force-pushed the paras/triggerCustomizationNotificationsOnIDE branch from 6b587c6 to 013f639CompareOctober 4, 2024 13:41
Base automatically changed from paras/fetchCustomizations to mainOctober 4, 2024 22:25
@pras0131
pras0131 merged commit 7e8933a into mainOct 4, 2024
@pras0131
pras0131 deleted the paras/triggerCustomizationNotificationsOnIDE branch October 4, 2024 22:42
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@pras0131@breedloj