Provide query example for metadata using context menu #19

Vorago · 2017-01-10T13:19:24Z

No description provided.

FylmTM · 2017-01-10T14:13:01Z

...neueda/jetbrains/plugin/graphdb/jetbrains/ui/datasource/metadata/actions/MetadataAction.java

+        DataSourcesComponent dataSourcesComponent = project.getComponent(DataSourcesComponent.class);
+        Optional<DataSourceApi> dataSource = dataSourcesComponent.getDataSourceContainer().findDataSource(dataSourceUuid);
+
+        executeQueryEvent.executeQuery(dataSource.get(), payload);


Let's not blindly use .get(), but actually check for a data source and do nothing if data source is not present (for some wierd reason).

FylmTM · 2017-01-10T14:13:55Z

...a/jetbrains/plugin/graphdb/jetbrains/ui/datasource/metadata/actions/MetadataActionGroup.java

+                return new AnAction[]{};
+        }
+    }
+


It's common practice for all of you to put empty line before closing class bracket? I should put some checkstyle rule for that :)

FylmTM · 2017-01-10T14:15:37Z

...a/jetbrains/plugin/graphdb/jetbrains/ui/datasource/metadata/actions/MetadataActionGroup.java

+    public AnAction[] getChildren(@Nullable AnActionEvent e) {
+        switch (type) {
+            case RELATIONSHIP_TYPES:
+                return new AnAction[]{new MetadataRelationshipAction(label, dataSourceUuid, "Query this relationship", "", NEO4J)};


label -> relationshipType

FylmTM · 2017-01-10T14:15:51Z

...a/jetbrains/plugin/graphdb/jetbrains/ui/datasource/metadata/actions/MetadataActionGroup.java

+            case LABELS:
+                return new AnAction[]{new MetadataLabelAction(label, dataSourceUuid, "Query this label", "", NEO4J)};
+            case PROPERTY_KEYS:
+                return new AnAction[]{new MetadataPropertyKeyAction(label, dataSourceUuid, "Query this property", "", NEO4J)};


label -> propertyKey

FylmTM · 2017-01-10T15:45:33Z

...ava/com/neueda/jetbrains/plugin/graphdb/jetbrains/ui/datasource/tree/ContextMenuService.java

+
+    private String extractUuid(Object dsUserObject) {
+        if (dsUserObject instanceof DataSourceV1) {
+            return ((DataSourceV1) dsUserObject).getUUID();


Use interface DataSourceApi

- Tests implemented - Refactoring needed

FylmTM

As discussed - while it's working, we can do better by changing approach.

FylmTM · 2017-01-12T10:16:45Z

...va/com/neueda/jetbrains/plugin/graphdb/jetbrains/ui/datasource/metadata/dto/ContextMenu.java

+        return dataSourceApi;
+    }
+
+    public String getData() {


There is possibility that data will be null. Either mark with annotation @Nullable (from intellij platform) or add Optional.

FylmTM · 2017-01-12T10:17:59Z

...ava/com/neueda/jetbrains/plugin/graphdb/jetbrains/ui/datasource/tree/ContextMenuService.java

+import javax.swing.tree.TreePath;
+import java.util.Optional;
+
+public class ContextMenuService {


I would question about coupling this service to data sources.
But let's leave it as it is for now.

FylmTM · 2017-01-12T10:18:24Z

...ava/com/neueda/jetbrains/plugin/graphdb/jetbrains/ui/datasource/tree/ContextMenuService.java

+        }
+    }
+
+    private TreeNodeModelApi extractUserObject(TreeNode node) {


FylmTM · 2017-01-12T10:21:34Z

...eueda/jetbrains/plugin/graphdb/jetbrains/ui/datasource/tree/DataSourcesTreeMouseAdapter.java

+
+    private void popup(ContextMenu dto, Tree tree) {
+        DataContext dataContext = DataManager.getInstance().getDataContext(tree);
+        if (dto.getMetadataType() == Neo4jTreeNodeType.LABEL


I think that switch will be better here instead of ifs (because we are using enums)

FylmTM · 2017-01-12T10:27:59Z

...ava/com/neueda/jetbrains/plugin/graphdb/jetbrains/ui/datasource/tree/ContextMenuService.java

+                || type == Neo4jTreeNodeType.PROPERTY_KEY)) {
+            return new ContextMenu(type, model.getDataSourceApi(), model.getText().get());
+        } else if (type == Neo4jTreeNodeType.DATASOURCE) {
+            return new ContextMenu(type, model.getDataSourceApi());


We are creating data source context menu here.

FylmTM · 2017-01-12T10:28:33Z

...om/neueda/jetbrains/plugin/graphdb/jetbrains/ui/datasource/tree/DataSourceTreeNodeModel.java

+
+    @Override
+    public Optional<ContextMenu> getContextMenu() {
+        return Optional.of(new ContextMenu(getType(), dataSourceApi));


And also data source context menu here. Why in 2 places?

FylmTM · 2017-01-12T10:30:05Z

...ain/java/com/neueda/jetbrains/plugin/graphdb/jetbrains/ui/datasource/tree/TreeNodeModel.java

+        return Optional.ofNullable(contextMenu);
+    }
+
+    public void setContextMenu(ContextMenu contextMenu) {


I think that this setter is not used?

FylmTM · 2017-01-12T10:30:57Z

...ain/java/com/neueda/jetbrains/plugin/graphdb/jetbrains/ui/datasource/tree/TreeNodeModel.java

+        this.value = value;
+    }
+
+    public TreeNodeModel(Neo4jTreeNodeType type, DataSourceApi dataSourceApi, String value) {


Constructors should be organized into a tree, with prepareContextMenu() only in one of them (most general one).

FylmTM · 2017-01-12T10:34:52Z

...ain/java/com/neueda/jetbrains/plugin/graphdb/jetbrains/ui/datasource/tree/TreeNodeModel.java

+import javax.swing.*;
+import java.util.Optional;
+
+public class TreeNodeModel implements TreeNodeModelApi {


Let's move metadata specific logic to a "MetadataTreeNodeModel" (similar to how DataSourceTreeNodeModel works).

It's looks a bit odd when you extend from TreeNodeModel and already have some built-in logic that you override by doing method overrides.

I would even consider to create "RootTreeNodeModel" for tree root specifically.

FylmTM · 2017-01-12T10:36:08Z

...ueda/jetbrains/plugin/graphdb/jetbrains/ui/datasource/metadata/DataSourceMetadataUiTest.java

+import static java.util.Collections.singletonList;
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class DataSourceMetadataUiTest {


I like this test. Using swing objects, but just not rendering them.

FylmTM

Bunch of comments, but most important one is about why we have context menu creation in multiple places.

- Changed arg order: key first, then value

Vorago requested a review from FylmTM January 10, 2017 13:45

FylmTM reviewed Jan 10, 2017

View reviewed changes

Vorago added 7 commits January 11, 2017 13:44

Added context menu for labels

528f55b

Added relationship context menu

0caf80e

- Tests implemented - Refactoring needed

Tested and working

449ab1e

Refactoring

3bf6a09

Fixes after rebase

115b2e3

Review comments fixed

52cf391

Added interface for TreeNode userObject

fa50293

Vorago force-pushed the feature-menu branch from c266e91 to fa50293 Compare January 11, 2017 11:44

FylmTM suggested changes Jan 11, 2017

View reviewed changes

Refactoring

08fe7b5

FylmTM reviewed Jan 12, 2017

View reviewed changes

FylmTM suggested changes Jan 12, 2017

View reviewed changes

Vorago added 2 commits January 12, 2017 15:55

More refactoring

3e04e75

Small refactoring

3b10c4d

- Changed arg order: key first, then value

FylmTM approved these changes Jan 12, 2017

View reviewed changes

FylmTM merged commit b4d9f11 into master Jan 12, 2017

Provide query example for metadata using context menu #19

Provide query example for metadata using context menu #19

Uh oh!

Conversation

Vorago commented Jan 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FylmTM Jan 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

FylmTM 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!

FylmTM left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

FylmTM Jan 10, 2017 •

edited

Loading