Skip to content
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

Integration of JEP-330 #1171

Merged
merged 20 commits into from Apr 30, 2019

Conversation

@sarveshkesharwani
Copy link
Contributor

commented Mar 20, 2019

A feature of JDK-11, here is the link for JEP-330.

@JaroslavTulach
Copy link
Contributor

left a comment

Consider defining the action on the DataObject in Loaders/Actions/.... folder.
Can the module become more heavyweight?
Consider writing some unit test.

"Arguments passed to the main method while running the file."
) {
public String getValue () {
return NbPreferences.forModule(JavaNode.class).get(fileName + "_SINGLE_FILE_RUN_ARGUMENTS", "");

This comment has been minimized.

Copy link
@JaroslavTulach

JaroslavTulach Mar 26, 2019

Contributor

dataObject.getPrimaryFile().setAttribute(...) or getAttribute() would be my preferred location to store these metadata. These two methods were invented exactly for the purpose of associating additional metadata with files.

This comment has been minimized.

Copy link
@sarveshkesharwani

sarveshkesharwani Mar 27, 2019

Author Contributor

Great tip! I wasn't aware of this.

@@ -213,6 +216,48 @@ protected final Sheet createSheet () {
});
sheet.put(ps);

Project parentProject = FileOwnerQuery.getOwner(super.getDataObject().getPrimaryFile());
String fileName = super.getDataObject().getPrimaryFile().getName();
// If any of the parent folders is a project, this menu won't appear.

This comment has been minimized.

Copy link
@JaroslavTulach

JaroslavTulach Mar 26, 2019

Contributor

"this menu" is misleading I think.

@@ -222,6 +222,15 @@
<specification-version>1.58</specification-version>
</run-dependency>
</dependency>
<dependency>
<code-name-base>org.netbeans.modules.extexecution</code-name-base>

This comment has been minimized.

Copy link
@JaroslavTulach

JaroslavTulach Mar 26, 2019

Contributor

Such dependency makes the java.source module more heavyweight. Is that OK, @jlahoda?

This comment has been minimized.

Copy link
@sarveshkesharwani

sarveshkesharwani Mar 27, 2019

Author Contributor

The reason why I was doing this in the first place, is that I was using NbPreferences to store arguments, and VM Options. And NbPreferences require for the accessing class to be in the same module, but now, I am using dataobject's attribute, so I can move this class out of java.source.


@Override
public void invokeAction(String command, Lookup context) throws IllegalArgumentException {
FileObject fileObject = getJavaFileWithoutProjectFromLookup(context);

This comment has been minimized.

Copy link
@JaroslavTulach

JaroslavTulach Mar 26, 2019

Contributor

fileObject can be null.

commandsList.add("bash");
commandsList.add("-c");
}
String javaPath = "\"" + System.getProperty("java.home") + File.separator + "bin" + File.separator + "java\"";

This comment has been minimized.

Copy link
@JaroslavTulach

JaroslavTulach Mar 26, 2019

Contributor

I'd suggest: new File(new File(new File(javaHome), "bin", "java")

* @author Sarvesh Kesharwani
*/
@ServiceProvider(service = ActionProvider.class)
public class SingleJavaSourceRunActionProvider implements ActionProvider {

This comment has been minimized.

Copy link
@JaroslavTulach

JaroslavTulach Mar 26, 2019

Contributor

I am surprised this is an ActionProvider - that is for executing projects. I'd rather create the action via Actions.context or annotations and linked to it in Loaders/Actions/text/java folder in the layer via @ActionReference.

This comment has been minimized.

Copy link
@sarveshkesharwani

sarveshkesharwani Mar 27, 2019

Author Contributor

Thanks for the review @JaroslavTulach . My first instinct was to add this via @ActionReference too, but here, I am making use of the "Run File" which is already added to Project UI's layer. (which is also present for a single file). So, now, if I create a new action with the annotation, wouldn't it mean that we would have two run menus (one for the new action, and the other disabled "Run File")?

sarveshkesharwani added some commits Mar 28, 2019

removing extecxecution dependency in java.source; using dataObjects' …
…attributes to add arguments and vm options
String arguments = argumentsObject != null ? (String) argumentsObject : "";
Object vmOptionsObject = fileObject.getAttribute(FILE_VM_OPTIONS);
String vmOptions = vmOptionsObject != null ? (String) vmOptionsObject : "";
ExecutionDescriptor descriptor = new ExecutionDescriptor().controllable(true).frontWindow(true).

This comment has been minimized.

Copy link
@JaroslavTulach

JaroslavTulach Apr 15, 2019

Contributor

I would extract this code into a separate method and then call this method in the testSingleJavaSourceRun...

File f1 = new File(new File(new File (getDataDir().getAbsolutePath()), "files"), "TestSingleJavaFile.java");
File javaPathFile = new File(new File(new File(System.getProperty("java.home")), "bin"), "java");
commandsList.add(javaPathFile.getAbsolutePath() + " " + f1.getAbsolutePath());
ProcessBuilder pb = new ProcessBuilder(commandsList);

This comment has been minimized.

Copy link
@JaroslavTulach

JaroslavTulach Apr 15, 2019

Contributor

...rather than testing ProcessBuilder I suggest to test the code extracted from the invokeAction.

@sarveshkesharwani sarveshkesharwani force-pushed the sarveshkesharwani:netbeans1783 branch from 5431e43 to f573d4c Apr 17, 2019

@JaroslavTulach
Copy link
Contributor

left a comment

I left few more comments, but otherwise it is almost ready. Approving.

FileObject javaFO = FileUtil.toFileObject(f1);
SingleJavaSourceRunActionProvider runActionProvider = new SingleJavaSourceRunActionProvider();
if (!runActionProvider.isActionEnabled("run.single", Lookup.EMPTY)) {
return;

This comment has been minimized.

Copy link
@JaroslavTulach

JaroslavTulach Apr 18, 2019

Contributor

Suspicious, I'd say.

This comment has been minimized.

Copy link
@sarveshkesharwani

sarveshkesharwani Apr 23, 2019

Author Contributor

Well, this test basically checks if the IDE is running on JDK 11 or above, and if it isn't, then this option is not given to the user. So, in such a case, there is no point running the test too, right?

This comment has been minimized.

Copy link
@JaroslavTulach

JaroslavTulach Apr 23, 2019

Contributor

The following code "checks if the IDE is running on JDK 11 or above" and if not it asserts that the action is disabled and skips further testing.

if (!isJDK11OrNewer()) {
  assertFalse("The action is only enabled on JDK11 and newer", runActionProvider.isActionEnabled(....));
  return;
}

I know the run.single action is disabled on JDKs older than 11, but that is actually something to test. Just my 2 Kč.

This comment has been minimized.

Copy link
@sarveshkesharwani

sarveshkesharwani Apr 24, 2019

Author Contributor

Got it. It does make sense to have test here to see whether the action is enabled or not based on the current version of JDK. I have added it in the latest commit.

*
* @author Sarvesh Kesharwani
*/
public class TestJavaFile extends NbTestCase {

This comment has been minimized.

Copy link
@JaroslavTulach

JaroslavTulach Apr 18, 2019

Contributor

The Test should be at the end of the class name. Try JavaFileTest. Otherwise ant test will skip this file, I think.

This comment has been minimized.

Copy link
@sarveshkesharwani

sarveshkesharwani Apr 23, 2019

Author Contributor

Fixed this in the latest commit.

}
String result = builder.toString();
assertEquals("hello world", result);
} catch (IOException ex) {

This comment has been minimized.

Copy link
@JaroslavTulach

JaroslavTulach Apr 18, 2019

Contributor

The test method can throw Exception including IOException. Just let it be thrown out.

This comment has been minimized.

Copy link
@sarveshkesharwani

sarveshkesharwani Apr 23, 2019

Author Contributor

Fixed this in the latest commit.

*
* @author Sarvesh Kesharwani
*/
public class RunProcess implements Callable<Process> {

This comment has been minimized.

Copy link
@JaroslavTulach

JaroslavTulach Apr 18, 2019

Contributor

I would remove public and let the class just package private - unless it is needed from another package - better to expose less by default.

This comment has been minimized.

Copy link
@sarveshkesharwani

sarveshkesharwani Apr 23, 2019

Author Contributor

Right. I have moved this file to a different package, so there is no longer a need for it to be public.

@junichi11

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

Need squashing?

@sarveshkesharwani

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

Need squashing?

Yes. We can squash while merging though, right?

@junichi11

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

Yes, it's just confirmation for a person who merges this :)

@zimmi

This comment has been minimized.

Copy link

commented Apr 25, 2019

@sarveshkesharwani Thank you for doing this, really cool! :)

@geertjanw

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

It is a great honor for me to squash and merge this amazing enhancement for NetBeans. Thanks!

@geertjanw

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Squashing and merging,

@geertjanw geertjanw merged commit 8836aad into apache:master Apr 30, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

sarveshkesharwani added a commit to sarveshkesharwani/incubator-netbeans that referenced this pull request May 28, 2019

Integration of JEP-330 (apache#1171)
* adding support to run single java files outside projects (JEP-330)

* adding SingleJavaSourceRunActionProvider.java to global lookup

* adding content to SingleJavaSourceRunActionProvider.java

* removing unused methods and variables

* restricting JEP-330 to jdk-11 and above

* fixing check for jdk-11 and execution in linux

* removing unnecessary dependencies in projectui

* fixing the public packages in java.source

* removing extecxecution dependency in java.source; using dataObjects' attributes to add arguments and vm options

* adding test case for JEP-330 run

* using tests to check functionality of SingleJavaSourceRunActionProvider

* removing unused code

* removing unused imports

* testing java.api.common with jdk11

* adding check for jdk11 in test file

* adding quiet option to travis build

* changing RunProcess to package protected; changing TestJavaFile to JavaFileTest

* restoring .travis.yml

* adding test for isActionEnabled; fixing bug in isActionEnabled

@geertjanw geertjanw added the NB11.1 label Jun 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.