AWS Explorer #4

zhangzhx · 2017-07-31T17:17:19Z

First batch for AWS Explorer in IntelliJ.

Redesigned the Explorer with the IntelliJ tree framework in the package com.intellij.ide.util.treeView
Added two extra status node for showing errors and empty.

Empty Node

Error Node

kiiadi · 2017-08-07T15:21:37Z

aws-intellij-toolkit.iml

@@ -0,0 +1,12 @@
+<?xml version="1.0" encoding="UTF-8"?>


Do we need the .iml file?

If it is not necessary for importing the project, we can remove this. I added .iml files in .gitignore.

kiiadi · 2017-08-07T15:24:48Z

src/main/java/com/amazonaws/intellij/ui/explorer/ExplorerToolbar.java

+import com.intellij.util.ui.UIUtil;
+import org.jetbrains.annotations.NonNls;
+
+import javax.annotation.Nonnull;


Should we use jet-brains annotations for this instead?

kiiadi · 2017-08-07T15:27:05Z

src/main/java/com/amazonaws/intellij/ui/ui/widgets/AwsRegionPanel.java

+ */
+public class AwsRegionPanel {
+    private JPanel regionPanel;
+    private com.intellij.openapi.ui.ComboBox<AwsRegion> regionCombo;


Is there a reason this is the FQ class name?

Imported the class and removed the FQ class name

kiiadi · 2017-08-07T15:27:51Z

src/main/java/com/amazonaws/intellij/ui/ui/widgets/AwsRegionPanel.java

+            }
+        });
+
+        for (AwsRegion region : AwsRegionManager.INSTANCE.getRegions()) {


Can we use forEach instead ? 😄

kiiadi · 2017-08-07T15:29:20Z

src/main/java/com/amazonaws/intellij/ui/ui/widgets/AwsRegionPanel.java

+    }
+
+    private void selectRegion(String regionId) {
+        for (AwsRegion region : AwsRegionManager.INSTANCE.getRegions()) {


stream().filter(r -> r.getId().equals(regionId)).findFirst().ifPresent(r -> regionCombo.setSelectedItem(r));

kiiadi · 2017-08-07T15:42:58Z

src/main/kotlin/com/amazonaws/intellij/lambda/explorer/AwsExplorerLambdaNode.kt

+class AwsExplorerLambdaRootNode(project: Project?, region: String):
+        AwsExplorerServiceRootNode<FunctionConfiguration>(project, "AWS Lambda", region, LAMBDA_SERVICE_ICON) {
+
+    private var client: AWSLambda = AWSLambdaClientBuilder.standard()


Can this be a val ?

The original concern is when region gets updated so we need to create a new client. Our current implementation is to redraw the Tree when region gets updated to avoid potential bugs for updating the Tree. Therefore, I can change this to val.

kiiadi · 2017-08-07T15:45:47Z

src/main/kotlin/com/amazonaws/intellij/ui/explorer/AwsExplorerNode.kt

+class AwsExplorerRootNode(project: Project?, region: String):
+        AwsExplorerNode<String>(project, "ROOT", region, AWS_ICON) {
+
+    override fun getChildren(): MutableCollection<out AbstractTreeNode<String>> {


Does this have to be a MutableCollection ? Can we make it immutable?

kiiadi · 2017-08-07T15:48:39Z

src/main/kotlin/com/amazonaws/intellij/ui/explorer/AwsExplorerService.kt

+/**
+ * Created by zhaoxiz on 7/27/17.
+ */
+enum class AwsExplorerService(val serviceId: String) {


Does it make sense to have this more like an extension point like @abrooksv did with the Credential Management? Then every time we add a new service (that has it's own 'resources' to display in the explorer) it can register as a provider here?

That was my first version of the Explorer but dropped that after talking to @abrooksv. The reason was that:

The Extension Point has a low performance.

We are not planning to provide different IntelliJ plugins based on services like Eclipse do. We want to integrate all the services in one plugin. We provide different plugins based on different languages.

kiiadi · 2017-08-07T15:49:02Z

src/main/kotlin/com/amazonaws/intellij/ui/explorer/AwsExplorerService.kt

+    },
+    ;
+
+    abstract fun buildServiceRoodNode(project: Project?, region: String): AbstractTreeNode<String>


rood -> root

Woops, corrected.

kiiadi · 2017-08-07T15:53:41Z

src/main/kotlin/com/amazonaws/intellij/ui/explorer/AwsExplorerNode.kt

+    }
+
+    // This method may throw RuntimeException, must handle it
+    abstract fun loadResources(): MutableList<Resource>


Should this throw an exception? Is there a better way of communicating this? What if it returned a nullable MutableList? Alternatively you can do this with an Either if the error is actually important

https://discuss.kotlinlang.org/t/disjoint-unions-in-kotlin/413

The loadResources() method mostly would throw an AmazonServiceException which is a RuntimeException.
The return type is MutableList<Resource>, so no null value is returned.
I think whether to use Either or not depends where do we want to handle this exception. I personally prefer to current way as the loadResources method should only be called in the super class when loading specific resources like S3 buckets or Lambda functions. The super class would handle the exceptions to report in the UI.

abrooksv

Overall I would look into if we can convert more of the mutable stuff to immutable.

Also remove all the author blocks

abrooksv · 2017-08-07T22:37:58Z

src/main/java/com/amazonaws/intellij/ui/explorer/ExplorerToolbar.java

+/**
+ * Created by zhaoxiz on 7/20/17.
+ */
+public class ExplorerToolbar {


We should rename this since it is not just the toolbar, but the tree too. If its the full tool window then call it ExplorerToolWindow?

Done. ExplorerToolWindow better fits the class name.

abrooksv · 2017-08-07T22:38:48Z

src/main/java/com/amazonaws/intellij/ui/explorer/ExplorerToolbar.java

+    public ExplorerToolbar(@NonNls Project project, @NonNls Wrapper wrapper) {
+        this.project = project;
+        this.regionProvider = AwsDefaultRegionProvider.getInstance(project);
+        this.wrapper = wrapper;


How does the wrapper get into the mainPanel?

The mainPanel is only for the toolbar. The wrapper panel is for the tree. The wrapper panel is created in the class AwsExplorerFactory, the wrapper is passed in as a parameter so that when region changes, we can update the tree panel in the wrapper.

val simpleToolWindowPanel = SimpleToolWindowPanel(true, false) val wrapperPane = Wrapper() simpleToolWindowPanel.setToolbar(ExplorerToolWindow(project, wrapperPane).mainPanel) simpleToolWindowPanel.setContent(wrapperPane)

abrooksv · 2017-08-07T22:40:48Z

src/main/java/com/amazonaws/intellij/ui/explorer/ExplorerToolbar.form

@@ -0,0 +1,11 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<form xmlns="http://www.intellij.com/uidesigner/form/" version="1" bind-to-class="com.amazonaws.intellij.ui.explorer.ExplorerToolbar">


I am not seeing much reason to bind this to a form since it is an empty panel. Do you see much reason to keep this as a bound .form instead of just writing it in Kotlin?

Removed this form.

abrooksv · 2017-08-07T22:42:39Z

src/main/java/com/amazonaws/intellij/ui/ui/widgets/AwsRegionPanel.java

+    private ComboBox<AwsRegion> regionCombo;
+
+    public AwsRegionPanel(String defaultRegion) {
+


delete blank line

abrooksv · 2017-08-07T22:48:14Z

src/main/java/com/amazonaws/intellij/ui/ui/widgets/AwsRegionPanel.java

+            }
+        });
+
+        AwsRegionManager.INSTANCE.getRegions().forEach((region)-> {regionCombo.addItem(region);});


Look into setting the ComboBox to use a CollectionComboBoxModel

This will remove the need for the stream, and also using the model is typesafe

abrooksv · 2017-08-07T23:03:54Z

src/main/kotlin/com/amazonaws/intellij/lambda/explorer/AwsExplorerLambdaNode.kt

+class AwsExplorerLambdaRootNode(project: Project?, region: String):
+        AwsExplorerServiceRootNode<FunctionConfiguration>(project, "AWS Lambda", region, LAMBDA_SERVICE_ICON) {
+
+    private val client: AWSLambda = AWSLambdaClientBuilder.standard()


add TODO to move to a client manager so we don't lose it

Addressed. Will send a separate PR for ClientManager

abrooksv · 2017-08-07T23:04:21Z

src/main/kotlin/com/amazonaws/intellij/lambda/explorer/AwsExplorerLambdaNode.kt

+            .withRegion(region)
+            .build()
+
+    override fun loadResources(): MutableList<FunctionConfiguration> {


Should not be mutable list

abrooksv · 2017-08-07T23:06:40Z

src/main/kotlin/com/amazonaws/intellij/ui/explorer/AwsExplorerFactory.kt

+
+    override fun createToolWindowContent(project: Project, toolWindow: ToolWindow) {
+        val simpleToolWindowPanel = SimpleToolWindowPanel(true, false)
+        val wrapperPane = Wrapper()


Wrapper out here seems weird inversion of control. Can we move it into the tool panel?

abrooksv · 2017-08-07T23:07:36Z

src/main/kotlin/com/amazonaws/intellij/ui/explorer/AwsExplorerNode.kt

+ * Created by zhaoxiz on 7/27/17.
+ */
+
+abstract class AwsExplorerNode<T>(project: Project?, value: T, val region: String, val awsIcon: Icon?):


is there an interface T is?

Is there an interface T? What do you mean?

Opps thought I deleted this comment because I saw there wasn't

Question was is there better type bounds for T, basically

abrooksv · 2017-08-07T23:14:47Z

src/main/kotlin/com/amazonaws/intellij/ui/explorer/AwsExplorerNode.kt

+    val cache: ClearableLazyValue<MutableCollection<AwsExplorerNode<*>>>
+
+    init {
+        cache = object : ClearableLazyValue<MutableCollection<AwsExplorerNode<*>>>() {


Why does this need to be mutable?

Replaced with Immutable.

abrooksv · 2017-08-10T21:50:03Z

src/main/java/com/amazonaws/intellij/ui/ui/widgets/AwsRegionPanel.java

-        AwsRegionManager.INSTANCE.getRegions().forEach((region)-> {regionCombo.addItem(region);});
+        regionCombo.setModel(regionModel);
+
+        AwsRegionManager.INSTANCE.getRegions().forEach(regionModel::add);


you can just say regionModel.add(AwsRegionManager.INSTANCE.getRegions())

or in the constructor of the collection pass it

abrooksv · 2017-08-10T21:51:29Z

src/main/kotlin/com/amazonaws/intellij/aws/AwsResourceManager.kt


 private class AwsResources(val region: String) {
-    val s3Client: AmazonS3 = AmazonS3Client() //AmazonS3ClientBuilder.standard().withRegion(region).build()
+    val s3Client: AmazonS3 = AmazonS3Client() //AmazonS3ClientBuilder.standard().withRegion(region).parse()


What is the commented out part for?

Forgot to address this. This is because a rename to the method build() -> parse(), but somehow IntelliJ replaces all the occurrences of build to parse. This code would be replaced anyway, so leave it as is for now

abrooksv · 2017-08-10T21:55:48Z

src/main/java/com/amazonaws/intellij/ui/ui/widgets/AwsRegionPanel.java

    private ComboBox<AwsRegion> regionCombo;
+    private final CollectionComboBoxModel<AwsRegion> regionModel = new CollectionComboBoxModel<>();

    public AwsRegionPanel(String defaultRegion) {


Can this just take the AwsRegion type as the parameter? Saves us the filter and find later

abrooksv · 2017-08-10T22:02:40Z

src/main/java/com/amazonaws/intellij/ui/ui/widgets/AwsRegionPanel.java


    public AwsRegionPanel(String defaultRegion) {
-
        regionCombo.setRenderer(new DefaultListCellRenderer() {


lets look into ListCellRendererWrapper. It is type safe and the comment says it is less ugly 😸

Done. Thanks for the info :)

abrooksv · 2017-08-10T22:06:32Z

src/main/kotlin/com/amazonaws/intellij/core/region/AwsRegionManager.kt

 object AwsRegionManager {
-
-    private val regions = mutableListOf<AwsRegion>()
+    var regions: List<AwsRegion>


this should be val

abrooksv · 2017-08-10T22:15:52Z

src/main/kotlin/com/amazonaws/intellij/ui/explorer/ExplorerToolWindow.kt

+import javax.swing.tree.DefaultMutableTreeNode
+import javax.swing.tree.DefaultTreeModel
+
+class ExplorerToolWindow(val project: Project, val treePanelWrapper: Wrapper) {


Make this extend the simple tool window and call setContent in init{}

Done. Removed the form file also

…ome other small refactors.

abrooksv

Just change the XML node name in the persistent component

…o-gennerated headers comments.

AWS Explorer

fa8f0f9

zhangzhx requested review from abrooksv and kiiadi July 31, 2017 17:18

kiiadi reviewed Aug 7, 2017

View reviewed changes

Address feedback.

f4bc62c

abrooksv reviewed Aug 7, 2017

View reviewed changes

Address a lot of feedback for kotlin.

9105362

abrooksv reviewed Aug 10, 2017

View reviewed changes

Remove all the form and java files, refactor the RegionPanel UI and s…

3b3ca20

…ome other small refactors.

abrooksv approved these changes Aug 16, 2017

View reviewed changes

Changed the region setting in aws.xml to 'region' and removed the aut…

c756742

…o-gennerated headers comments.

kiiadi approved these changes Aug 16, 2017

View reviewed changes

Merge branch 'credManagement-for-explorer' into aws-explorer

2ee671c

zhangzhx merged commit 76d39a1 into credManagement-for-explorer Aug 17, 2017

abrooksv deleted the aws-explorer branch August 17, 2017 01:09

hunterwerlla pushed a commit that referenced this pull request Oct 21, 2019

Dispose the ecs run config UI correctly (#4)

2456d6d

		@@ -0,0 +1,11 @@
		<?xml version="1.0" encoding="UTF-8"?>
		<form xmlns="http://www.intellij.com/uidesigner/form/" version="1" bind-to-class="com.amazonaws.intellij.ui.explorer.ExplorerToolbar">

		private ComboBox<AwsRegion> regionCombo;

		public AwsRegionPanel(String defaultRegion) {


		public AwsRegionPanel(String defaultRegion) {

		regionCombo.setRenderer(new DefaultListCellRenderer() {

AWS Explorer #4

AWS Explorer #4

Uh oh!

Conversation

zhangzhx commented Jul 31, 2017

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abrooksv left a comment

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!

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