Skip to content

Naive fix for #1413 #1414

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/com/goide/runconfig/GoRunConfigurationBase.java
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ public abstract class GoRunConfigurationBase<RunningState extends GoRunningState
private static final String PARAMETERS_NAME = "parameters";
private static final String PASS_PARENT_ENV = "pass_parent_env";

@NotNull private String myFilePath = "";
@NotNull private String myWorkingDirectory = "";
@NotNull private String myParams = "";
@NotNull private Map<String, String> myCustomEnvironment = ContainerUtil.newHashMap();
Expand Down Expand Up @@ -194,4 +195,14 @@ public String getWorkingDirectory() {
public void setWorkingDirectory(@NotNull String workingDirectory) {
myWorkingDirectory = workingDirectory;
}

@NotNull
public String getFilePath() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need it in base class? Why do you need duplicates of this methods in GoTestRunConfiguration?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change to move filePath up to the base is probably unnecessary now. I had originally used this to point to the app root directory until I looked up the source root and eliminated the file path from the run app config entirely. Also, I totally overlooked the TestRun configs so I will fix these tomorrow.

Like I said, totally naive approach and I have never done Idea plugin work before so there are probably much better ways to do this later.

return myFilePath;
}

public void setFilePath(@NotNull String filePath) {
myFilePath = filePath;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

public class GoRunConfigurationProducerBase<T extends GoRunConfigurationWithMain> extends RunConfigurationProducer<T> implements Cloneable {
public class GoRunConfigurationProducerBase<T extends GoRunConfigurationBase> extends RunConfigurationProducer<T> implements Cloneable {
protected GoRunConfigurationProducerBase(@NotNull ConfigurationType configurationType) {
super(configurationType);
}
Expand Down
18 changes: 4 additions & 14 deletions src/com/goide/runconfig/GoRunConfigurationWithMain.java
Original file line number Diff line number Diff line change
Expand Up @@ -38,27 +38,25 @@
public abstract class GoRunConfigurationWithMain<T extends GoRunningState> extends GoRunConfigurationBase<T> {
private static final String FILE_PATH_NAME = "file_path";

@NotNull private String myFilePath = "";

public GoRunConfigurationWithMain(String name, GoModuleBasedConfiguration configurationModule, ConfigurationFactory factory) {
super(name, configurationModule, factory);
myFilePath = getWorkingDirectory();
setFilePath(getWorkingDirectory());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have defined the field in base class, so store the field there and remove storing duplicates and field initialisation in concrete run configurations

}

@Override
public void readExternal(@NotNull Element element) throws InvalidDataException {
super.readExternal(element);
String filePathValue = JDOMExternalizerUtil.getFirstChildValueAttribute(element, FILE_PATH_NAME);
if (filePathValue != null) {
myFilePath = filePathValue;
setFilePath(filePathValue);
}
}

@Override
public void writeExternal(Element element) throws WriteExternalException {
super.writeExternal(element);
if (StringUtil.isNotEmpty(myFilePath)) {
JDOMExternalizerUtil.addElementWithValueAttribute(element, FILE_PATH_NAME, myFilePath);
if (StringUtil.isNotEmpty(getFilePath())) {
JDOMExternalizerUtil.addElementWithValueAttribute(element, FILE_PATH_NAME, getFilePath());
}
}

Expand All @@ -83,12 +81,4 @@ public void checkConfiguration() throws RuntimeConfigurationException {
}
}

@NotNull
public String getFilePath() {
return myFilePath;
}

public void setFilePath(@NotNull String filePath) {
myFilePath = filePath;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
package com.goide.runconfig.application;

import com.goide.runconfig.GoModuleBasedConfiguration;
import com.goide.runconfig.GoRunConfigurationWithMain;
import com.goide.runconfig.ui.GoRunConfigurationEditorForm;
import com.goide.runconfig.GoRunConfigurationBase;
import com.goide.runconfig.ui.GoApplicationRunConfigurationEditorForm;
import com.intellij.execution.configurations.ConfigurationType;
import com.intellij.execution.configurations.ModuleBasedConfiguration;
import com.intellij.execution.configurations.RunConfiguration;
Expand All @@ -28,21 +28,21 @@
import com.intellij.openapi.project.Project;
import org.jetbrains.annotations.NotNull;

public class GoApplicationConfiguration extends GoRunConfigurationWithMain<GoApplicationRunningState> {
public GoApplicationConfiguration(Project project, String name, @NotNull ConfigurationType configurationType) {
public class GoApplicationRunConfiguration extends GoRunConfigurationBase<GoApplicationRunningState> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GoRunConfigurationWithMain now has the only implementation and it seems to be redundant.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I will clean all of this up tomorrow. Thanks for the feedback, and let me know if you have something better in the works. This was all just a quick hack on my part and I can see that there is probably a much more elegant way to do this eventually.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any in works yet, I do the plugin just a day in a week. But I'd definitely like to cooperate with you and think of something more elegant and covering as much as possible cases solution :)

public GoApplicationRunConfiguration(Project project, String name, @NotNull ConfigurationType configurationType) {
super(name, new GoModuleBasedConfiguration(project), configurationType.getConfigurationFactories()[0]);
}

@NotNull
@Override
protected ModuleBasedConfiguration createInstance() {
return new GoApplicationConfiguration(getProject(), getName(), GoApplicationRunConfigurationType.getInstance());
return new GoApplicationRunConfiguration(getProject(), getName(), GoApplicationRunConfigurationType.getInstance());
}

@NotNull
@Override
public SettingsEditor<? extends RunConfiguration> getConfigurationEditor() {
return new GoRunConfigurationEditorForm(getProject());
return new GoApplicationRunConfigurationEditorForm(getProject());
}

@NotNull
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

import com.goide.runconfig.GoRunConfigurationProducerBase;

public class GoApplicationRunConfigurationProducer extends GoRunConfigurationProducerBase<GoApplicationConfiguration> implements Cloneable {
public class GoApplicationRunConfigurationProducer extends GoRunConfigurationProducerBase<GoApplicationRunConfiguration> implements Cloneable {
public GoApplicationRunConfigurationProducer() {
super(GoApplicationRunConfigurationType.getInstance());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ public GoApplicationRunConfigurationType() {
addFactory(new GoConfigurationFactoryBase(this) {
@NotNull
public RunConfiguration createTemplateConfiguration(Project project) {
return new GoApplicationConfiguration(project, "Go", getInstance());
return new GoApplicationRunConfiguration(project, "Go", getInstance());
}
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,19 @@
import com.intellij.execution.process.ProcessOutput;
import com.intellij.execution.runners.ExecutionEnvironment;
import com.intellij.openapi.module.Module;
import com.intellij.openapi.roots.ModuleRootManager;
import com.intellij.openapi.util.io.FileUtil;
import com.intellij.openapi.vfs.VirtualFile;
import org.jetbrains.annotations.NotNull;

import java.io.File;
import java.io.IOException;

public class GoApplicationRunningState extends GoRunningState<GoApplicationConfiguration> {
public class GoApplicationRunningState extends GoRunningState<GoApplicationRunConfiguration> {
private File myTempFile;

public GoApplicationRunningState(@NotNull ExecutionEnvironment env, @NotNull Module module,
@NotNull GoApplicationConfiguration configuration) {
@NotNull GoApplicationRunConfiguration configuration) {
super(env, module, configuration);
}

Expand All @@ -52,12 +54,13 @@ protected ProcessHandler startProcess() throws ExecutionException {
}
try {
ProcessOutput processOutput = new ProcessOutput();
VirtualFile[] sourceRoots = ModuleRootManager.getInstance(myModule).getSourceRoots(false); //Eventually to support multiple module dependencies
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And after a lot of code this is actually "fix", right? Well, I can't accept it unfortunately. Despite of strange inheritance and duplicating code you've made, here are several problems:

  1. Lost an ability to configure main file, so it's impossible to use run configuration in the projects that have several main files.
  2. Lost an ability to configure working directory. User can configure it but you will replace it anyway.
  3. Source roots. Well, it's just wrong :) Despite of the fact that sourceRoots could be empty (especially in Small IDEs) and you'll get IOOBE in this case. Source roots have no any semantic sense in Go support, first source root could point anywhere, any user can mark any directory or even project root as source root and it doesn't affect (and shouldn't) go-support part, especially it shouldn't affect unconfigurable run configurations.

boolean success = GoExecutor.in(myModule)
.addParameters("build", "-o", myTempFile.getAbsolutePath(), myConfiguration.getFilePath())
.addParameters("build", "-o", myTempFile.getAbsolutePath())
.withWorkDirectory(sourceRoots[0].getCanonicalPath())
.withProcessOutput(processOutput)
.showOutputOnError()
.execute();

if (!success) {
throw new ExecutionException("Build failure. `go build` is finished with exit code " + processOutput.getExitCode());
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?xml version="1.0" encoding="UTF-8"?>
<form xmlns="http://www.intellij.com/uidesigner/form/" version="1" bind-to-class="com.goide.runconfig.ui.GoApplicationRunConfigurationEditorForm">
<grid id="27dc6" binding="myComponent" layout-manager="GridLayoutManager" row-count="2" column-count="1" same-size-horizontally="false" same-size-vertically="false" hgap="-1" vgap="-1">
<margin top="0" left="0" bottom="0" right="0"/>
<constraints>
<xy x="20" y="20" width="657" height="194"/>
</constraints>
<properties/>
<border type="none"/>
<children>
<vspacer id="632bf">
<constraints>
<grid row="1" column="0" row-span="1" col-span="1" vsize-policy="6" hsize-policy="1" anchor="0" fill="2" indent="0" use-parent-layout="false"/>
</constraints>
</vspacer>
<nested-form id="b9fff" form-file="com/goide/runconfig/ui/GoCommonSettingsPanel.form" binding="myCommonSettingsPanel">
<constraints>
<grid row="0" column="0" row-span="1" col-span="1" vsize-policy="3" hsize-policy="3" anchor="1" fill="1" indent="0" use-parent-layout="false"/>
</constraints>
</nested-form>
</children>
</grid>
</form>
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/*
* Copyright 2013-2014 Sergey Ignatov, Alexander Zolotov
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.goide.runconfig.ui;

import com.goide.runconfig.application.GoApplicationRunConfiguration;
import com.intellij.openapi.options.ConfigurationException;
import com.intellij.openapi.options.SettingsEditor;
import com.intellij.openapi.project.Project;
import org.jetbrains.annotations.NotNull;

import javax.swing.*;

public class GoApplicationRunConfigurationEditorForm extends SettingsEditor<GoApplicationRunConfiguration> {
private JPanel myComponent;
private GoCommonSettingsPanel myCommonSettingsPanel;


public GoApplicationRunConfigurationEditorForm(@NotNull final Project project) {
super(null);
myCommonSettingsPanel.init(project);
}

@Override
protected void resetEditorFrom(@NotNull GoApplicationRunConfiguration configuration) {
myCommonSettingsPanel.resetEditorFrom(configuration);
}

@Override
protected void applyEditorTo(@NotNull GoApplicationRunConfiguration configuration) throws ConfigurationException {
myCommonSettingsPanel.applyEditorTo(configuration);
}

@NotNull
@Override
protected JComponent createEditor() {
return myComponent;
}

@Override
protected void disposeEditor() {
myComponent.setVisible(false);
}
}