Client command framework improvements #2730

azatsarynnyy · 2016-10-07T10:36:27Z

What does this PR do?

Introduced contextual commands.
Added ${explorer.current.file.parent.path} macro.
Cleaned up unused resources.
Refactored code in order to simplify it and made it more clear and understandable:

simplified a way of providing new command types;
command types api moved to ide-api to avoid unnecessary dependency on machine extension;
command management extracted from EditCommandsPresenter to the separate facade - CommandManager. So EditCommandsPresenter now doesn't perform several http requests manipulating with commands but only one;
refactored code related to the obsolete terminology CommandPropertyValueProvider -> Macro;
macro can provide its description (it will be really useful for showing it in UI);
macros processing separated from the command execution and extracted to the separate component MacroProcessor since it's used not only for the commands but in debug configurations for example.

What issues does this PR fix or reference?

Mainly, it was done for https://github.com/codenvy/artik-ide/issues/139.

@vparfonov plz review

codenvy-ci · 2016-10-07T11:21:21Z

Build # 654 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/654/ to view the results.

codenvy-ci · 2016-10-07T14:20:08Z

Build # 658 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/658/ to view the results.

codenvy-ci · 2016-10-10T07:50:55Z

Build # 666 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/666/ to view the results.

dkulieshov

Great job

dkulieshov · 2016-10-10T08:45:05Z

ide/che-core-ide-api/src/main/java/org/eclipse/che/ide/api/command/CommandImpl.java

     */
-    protected CommandConfiguration(@NotNull CommandType type, @NotNull String name, @Nullable Map<String, String> attributes) {
+    public CommandImpl(String name, String commandLine, String type) {
+        this.name = name;


I guess this can be replaced by a public CommandImpl(String name, String commandLine, String type, Map<String, String> attributes) constructor call, what do you think?

of course
thx

dkulieshov · 2016-10-10T08:48:02Z

ide/che-core-ide-api/src/main/java/org/eclipse/che/ide/api/command/CommandImpl.java

-        this.attributes = attributes;
+        this.commandLine = commandLine;
+        this.type = type;
+        this.attributes = new HashMap<>(attributes);


Do you think we can use an unmodifiable map here?

we can )
fixed

dkulieshov · 2016-10-10T08:49:09Z

ide/che-core-ide-api/src/main/java/org/eclipse/che/ide/api/command/CommandImpl.java

+
+    /** Creates copy of the given {@link Command}. */
+    public CommandImpl(Command command) {
+        this.name = command.getName();


Probably we can use a constructor call here, can't we?

dkulieshov · 2016-10-10T08:52:34Z

ide/che-core-ide-api/src/main/java/org/eclipse/che/ide/api/command/CommandImpl.java

        return type;
    }

+    @Override
    public Map<String, String> getAttributes() {
        if (attributes == null) {


I assume this lazy initialization should never happen as we will always initialize this field in class constructors, agree?

agree
fixed

dkulieshov · 2016-10-10T09:00:53Z

...che-core-ide-api/src/main/java/org/eclipse/che/ide/api/machine/MachineServiceClientImpl.java

+    public Promise<List<MachineDto>> getMachines(String workspaceId) {
+        return asyncRequestFactory.createGetRequest(baseHttpUrl + workspaceId + "/machine")
+                                  .header(ACCEPT, APPLICATION_JSON)
+                                  .loader(loaderFactory.newLoader("Getting info about bound machines..."))


I might be wrong but shouldn't such kind of message be mapped by a localized constants routines?

Yes, it should be a localization constant but it should be done for all gwt-clients for che services. May be in a separate PR in future.

I guess we need a dedicated task to fix this throughout the sources because this is not really related to this very issue

dkulieshov · 2016-10-10T09:20:40Z

ide/che-core-ide-api/src/main/java/org/eclipse/che/ide/api/command/CommandPage.java

+     * <p>{@inheritDoc}
+     */
+    @Override
+    void go(final AcceptsOneWidget container);


Looks odd, I guess that comes from inheritance?

yeah, it comes from the super interface
fixed, thx

dkulieshov · 2016-10-10T09:27:39Z

ide/che-core-ide-app/src/main/java/org/eclipse/che/ide/macro/MacroProcessorImpl.java

+        return expandMacros(promise, stringContainer, macroRegistry.getMacros().iterator());
+    }
+
+    private Promise<String> expandMacros(Promise<String> promise,


I would recommend to avoid recursion here because that makes the logic a bit complicated in my humble and subjective opinion but I'm curious about yours.

Yeah, I agree with your opinion Dmitry. But I believe it should be done in a separate task since I've just moved this code 'as-is' from another class during refactoring. And I don't want to modify it in this PR.
IMO In general, this algo of expanding macros wants to be rewritten to be simpler for understanding and easier for supporting.

dkulieshov · 2016-10-10T09:30:11Z

ide/che-core-ide-app/src/main/java/org/eclipse/che/ide/macro/MacroProcessorImpl.java

+        };
+    }
+
+    private class StringContainer {


If I'm not mistaken this can be a static nested class

you aren't mistaken of course
let it be a static nested class
fixed

dkulieshov · 2016-10-10T09:59:49Z

...t/src/main/java/org/eclipse/che/ide/extension/machine/client/command/CommandManagerImpl.java

+        return workspaceServiceClient.addCommand(appContext.getWorkspaceId(), commandDto).then(new Function<WorkspaceDto, CommandImpl>() {
+            @Override
+            public CommandImpl apply(WorkspaceDto arg) throws FunctionException {
+                final CommandImpl newCommand = new CommandImpl(commandDto.getName(),


I guess we can use new CommandImpl(commandDto); here, what do you think?

good catch
thx

dkulieshov · 2016-10-10T10:12:16Z

...n-maven-ide/src/main/java/org/eclipse/che/plugin/maven/client/command/MavenCommandModel.java

+
+        String workingDirectory = null;
+
+        if (cmd.hasArgument("-f")) {


I think such kind of routines should be moved into CommandLine class so we could run something like workingDirectory = cmd.nextTo("-f"); and get all we need, that would properly encapsulate the logic, what do you think?

Good idea for the further improvements of CommandLine class. 👍
I think it should be done in a separate task since I've just moved this code 'as-is' from another class during refactoring.

Of course! It's just a suggestion to keep in mind for the next time

codenvy-ci · 2016-10-10T17:20:53Z

Build # 679 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/679/ to view the results.

codenvy-ci · 2016-10-11T07:31:27Z

Build # 680 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/680/ to view the results.

vparfonov · 2016-10-11T06:56:22Z

ide/che-core-ide-api/src/main/java/org/eclipse/che/ide/api/macro/Macro.java

+     *
+     * @return a promise that resolves to the real value associated with macro
+     */
+    Promise<String> expand();


Not clear name of method for me maybe better something like "explicate or evaluate"

I think it's clear enough because it's a general terminology.

From https://en.wikipedia.org/wiki/Macro_(computer_science):

they are called "macros" because a big block of code can be expanded from a small sequence of characters

codenvy-ci · 2016-10-11T10:14:20Z

Build # 683 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/683/ to view the results.

Client command framework improvements 1. Introduced contextual commands. 2. Added ${explorer.current.file.parent.path} macro. 3. Cleaned up unused resources. 4. Refactored code in order to simplify it and made it more clear and understandable: - simplified a way of providing new command types; - command types api moved to ide-api to avoid unnecessary dependency on machine extension; - command management extracted from EditCommandsPresenter to the separate facade - CommandManager. So EditCommandsPresenter now doesn't perform several http requests manipulating with commands but only one; - refactored code related to the obsolete terminology CommandPropertyValueProvider -> Macro; - macro can provide its description (it will be really useful for showing it in UI); - macros processing separated from the command execution and extracted to the separate component MacroProcessor since it's used not only for the commands but in debug configurations for example.

Client command framework improvements

5e0764b

Merge branch 'master' into commands

101b2bc

Merge branch 'master' into commands

d42a98a

dkulieshov reviewed Oct 10, 2016

View reviewed changes

Fix notes

e0268e3

azatsarynnyy added 2 commits October 11, 2016 09:46

Move macros's descriptions to localization constants

00d5d49

Merge branch 'master' into commands

b893ff4

vparfonov reviewed Oct 11, 2016

View reviewed changes

vparfonov approved these changes Oct 11, 2016

View reviewed changes

Fix build

d6f6aa1

azatsarynnyy merged commit 2ba8cdf into master Oct 11, 2016

azatsarynnyy deleted the commands branch October 11, 2016 08:38

azatsarynnyy added this to the 5.0.0-M6 milestone Oct 11, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Client command framework improvements #2730

Client command framework improvements #2730

azatsarynnyy commented Oct 7, 2016 •

edited

Loading

codenvy-ci commented Oct 7, 2016

codenvy-ci commented Oct 7, 2016

codenvy-ci commented Oct 10, 2016

dkulieshov left a comment

dkulieshov Oct 10, 2016 •

edited

Loading

azatsarynnyy Oct 10, 2016

dkulieshov Oct 10, 2016

azatsarynnyy Oct 10, 2016

dkulieshov Oct 10, 2016

azatsarynnyy Oct 10, 2016

dkulieshov Oct 10, 2016

azatsarynnyy Oct 10, 2016

dkulieshov Oct 10, 2016 •

edited

Loading

azatsarynnyy Oct 11, 2016

dkulieshov Oct 11, 2016

dkulieshov Oct 10, 2016

azatsarynnyy Oct 10, 2016

dkulieshov Oct 10, 2016

azatsarynnyy Oct 10, 2016

dkulieshov Oct 10, 2016

azatsarynnyy Oct 10, 2016

dkulieshov Oct 10, 2016

azatsarynnyy Oct 10, 2016

dkulieshov Oct 10, 2016

azatsarynnyy Oct 11, 2016

dkulieshov Oct 11, 2016

codenvy-ci commented Oct 10, 2016

codenvy-ci commented Oct 11, 2016

vparfonov Oct 11, 2016

azatsarynnyy Oct 11, 2016

codenvy-ci commented Oct 11, 2016

Client command framework improvements #2730

Client command framework improvements #2730

Conversation

azatsarynnyy commented Oct 7, 2016 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

codenvy-ci commented Oct 7, 2016

codenvy-ci commented Oct 7, 2016

codenvy-ci commented Oct 10, 2016

dkulieshov left a comment

Choose a reason for hiding this comment

dkulieshov Oct 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkulieshov Oct 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenvy-ci commented Oct 10, 2016

codenvy-ci commented Oct 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenvy-ci commented Oct 11, 2016

azatsarynnyy commented Oct 7, 2016 •

edited

Loading

dkulieshov Oct 10, 2016 •

edited

Loading

dkulieshov Oct 10, 2016 •

edited

Loading