apache / netbeans

Apache NetBeans
https://netbeans.apache.org/
Apache License 2.0
2.62k stars 840 forks source link

Do not suggest to install nb-javac on JDK 14+ #2108

Closed jlahoda closed 4 years ago

graben commented 4 years ago

@jlahoda : This way it needs changing every half year after new JDK release?

geertjanw commented 4 years ago

We’re working specifically on 12.0 right now. And 12.0 will always only support up to JDK 14 and not beyond. But this covers 14 or beyond.

jlahoda commented 4 years ago

@jlahoda : This way it needs changing every half year after new JDK release?

It may need changes when we add a new nb-javac (which will support e.g. 14). But until we add a new nb-javac, there shouldn't be a need to change.

Note SourceVersion.RELEASE_14 will not cease to exist in JDK 15 - there still exists SourceVersion.RELEASE_2.

graben commented 4 years ago

@jlahoda : This way it needs changing every half year after new JDK release?

It may need changes when we add a new nb-javac (which will support e.g. 14). But until we add a new nb-javac, there shouldn't be a need to change.

Note SourceVersion.RELEASE_14 will not cease to exist in JDK 15 - there still exists SourceVersion.RELEASE_2.

But if for example JDK15 gets released you have to add RELEASE_15 as well. So you have to add a new version every half year until nb-javac gets finally removed?

jlahoda commented 4 years ago

If nb-javac is not upgraded, and JDK 15 is released, I don't think we need to do anything special here. SourceVersion.RELEASE_14 will still exist, the method will still return true, and the pop-up still will not be shown. Do I miss something?

graben commented 4 years ago

Your right, this way it should work. :-)

neilcsmith-net commented 4 years ago

Should this logic also control the dialog at https://github.com/apache/netbeans/blob/74728a19f8906082c93163935bb7eab3bd11d610/java/java.source/src/org/netbeans/modules/java/source/JBrowseModule.java#L81 ?

In fact, should we drop the latter notification completely? Or have a preference / property to stop any of this being shown? I may have a bias in wanting that! :wink:

neilcsmith-net commented 4 years ago

Sorry, ignore previous comment - misread as calling same method on NoJavacHelper.

+1

eirikbakke commented 4 years ago

Starting NetBeans 11.3 with a clean user directory, and declining to install nbjavac, I see that the "Compile on Save" checkbox is simply disabled in the "Project Properties" dialog of a newly created maven java project. The "Learn More about Compile On Save" link that does show up in said dialog points to information that does not mention nb-javac at all.

Desired behavior: The "Compile on Save" checkbox should be enabled even if nb-javac is not installed. If it is checked, the user should be prompted to install nb-javac.

I think this needs to be fixed before we stop recommending nb-javac by default.

eirikbakke commented 4 years ago

Also note that nb-javac is still recommended/installed by the "Additional modules are recommended to run Java SE support" dialog that shows up if you try to create a Java project in a NetBeans distribution that does not have Java SE initially installed (as when you compile netbeans sources and invoke "ant tryme").

neilcsmith-net commented 4 years ago

@eirikbakke that conversation is probably better having on dev@. This is currently about a very specific issue with NB 12.0 and lack of nb-javac for JDK 14 before release date.

eirikbakke commented 4 years ago

The specific issue with this patch is that it makes an existing feature (Compile-on-Save) appear broken from the user's point of view (even though it can still be used by installing nb-javac).

Proposed trivial fix: Change CustomizerCompile.CompileOnSave to "Compile on &Save (requires nb-javac plugin)" in each of the following files:

java/java.j2semodule/src/org/netbeans/modules/java/j2semodule/ui/customizer/Bundle.properties java/java.j2seproject/src/org/netbeans/modules/java/j2seproject/ui/customizer/Bundle.properties

This UI detail is my only concern here.

eirikbakke commented 4 years ago

Hmm, hold on, the "Compile on Save" checkbox string that shows up in Project Properties is somewhere else. Trying to locate it in and building netbeans to verify that it changes in the UI...

eirikbakke commented 4 years ago

Here's the proposed change in UI wording:

--- a/java/java.j2semodule/src/org/netbeans/modules/java/j2semodule/ui/customizer/Bundle.properties
+++ b/java/java.j2semodule/src/org/netbeans/modules/java/j2semodule/ui/customizer/Bundle.properties
@@ -601,7 +601,7 @@ ACSD_librariesLocation=Folder containing location of the library definition file
 ACSD_jButtonEdit=Edit classpath items
 ACSD_BuildJarAfterCompile=Build JAR after compiling option
 LBL_CustomizeRun_Enable_Quick_Run=Enable Quick Run
-CustomizerCompile.CompileOnSave=Compile on &Save
+CustomizerCompile.CompileOnSave=Compile on &Save (requires nb-javac plugin)
 AD_CustomizerCompile.CompileOnSave=If selected, files are compiled when you save them. This option saves you time when you run or debug your application in the IDE.
 LBL_CompileOnSaveDescription=<html>If selected, files are compiled when you save them.<br>\
     This option saves you time when you run or debug your application in the IDE.<br>\
diff --git a/java/java.j2seproject/src/org/netbeans/modules/java/j2seproject/ui/customizer/Bundle.properties b/java/java.j2seproject/src/org/netbeans/modules/java/j2seproject/ui/customizer/Bundle.properties
index acf0a8a..9cb2102 100644
--- a/java/java.j2seproject/src/org/netbeans/modules/java/j2seproject/ui/customizer/Bundle.properties
+++ b/java/java.j2seproject/src/org/netbeans/modules/java/j2seproject/ui/customizer/Bundle.properties
@@ -601,7 +601,7 @@ ACSD_librariesLocation=Folder containing location of the library definition file
 ACSD_jButtonEdit=Edit classpath items
 ACSD_BuildJarAfterCompile=Build JAR after compiling option
 LBL_CustomizeRun_Enable_Quick_Run=Enable Quick Run
-CustomizerCompile.CompileOnSave=Compile on &Save
+CustomizerCompile.CompileOnSave=Compile on &Save (requires nb-javac plugin)
 AD_CustomizerCompile.CompileOnSave=If selected, files are compiled when you save them. This option saves you time when you run or debug your application in the IDE.
 LBL_CompileOnSaveDescription=<html>If selected, files are compiled when you save them.<br>\
     This option saves you time when you run or debug your application in the IDE.<br>\
diff --git a/java/maven/src/org/netbeans/modules/maven/customizer/Bundle.properties b/java/maven/src/org/netbeans/modules/maven/customizer/Bundle.properties
index 35aa7a4..311e60c 100644
--- a/java/maven/src/org/netbeans/modules/maven/customizer/Bundle.properties
+++ b/java/maven/src/org/netbeans/modules/maven/customizer/Bundle.properties
@@ -142,9 +142,9 @@ HINT_ApplicationCoS=<html>Classes compiled with the Compile on Save feature in t
 RunJarPanel.lblConfiguration.text=&Configuration:
 ActionMappings.txtPackagings.text=
 ActionMappings.lblPackagings.text=Supported Packagings:
-CompilePanel.cbCompileOnSave.text=Compile On &Save
+CompilePanel.cbCompileOnSave.text=Compile on &Save (requires nb-javac plugin)
 CompilePanel.btnLearnMore.text=<html><a href="">Learn More about Compile On Save feature in Maven projects</a></html>
 ActionMappings.jButton1.text=Show in Toolbar
 RunJarPanel.txtVMOptions.AccessibleContext.accessibleDescription=VM options
 RunJarPanel.customizeOptionsButton.text=C&ustomize...

This would show up as follows:

nbjavac-note

ebarboni commented 4 years ago

@jlahoda do you think @eirikbakke UI changes can be done ?

neilcsmith-net commented 4 years ago

Surely @eirikbakke can just put a PR in with that UI change? Seems a good thing to have in 12.0, but not sure why it needs to be in this change. (aside - disabled compile-on-save is one of the good sides of leaving nb-javac uninstalled!)

eirikbakke commented 4 years ago

The PR as it stands breaks existing functionality (CoS)--the proposed UI change was a way to mitigate that so that the PR can be safely merged.

disabled compile-on-save is one of the good sides of leaving nb-javac uninstalled!

Not sure I follow?

neilcsmith-net commented 4 years ago

@eirikbakke the PR fixes a big issue with existing functionality, at least and unless we are able to ship an update for nb-javac supporting JDK 14 in a 12.0 update. nb-javac is meant to be optional, therefore CoS has always been optional in Apache. This doesn't change that, and needs to go in. Your UI change would also be a good improvement, just wouldn't want to see this held up on it.

The joke aside was just because multiple people, including me, have a tendency to disable CoS anyway - has its own caveats.

eirikbakke commented 4 years ago

I assumed the UI change would be uncontroversial. But having stated my concerns, I'll leave it to you.

The joke aside was just because multiple people, including me, have a tendency to disable CoS anyway

Ah. In my case, I'm really dependent on CoS--disabling it makes my Edit/Compile/Run cycle go from 15 seconds to 96 seconds. And it also causes problems with "Apply Changes" in the debugger, which I use to make instant changes without restarting my application. (I have a multi-module maven-based NetBeans Platform project.)

jlahoda commented 4 years ago

I've added the UI changes (a separate PR might be better, but hopefully this is not that huge problem - just if we squash, we should mention that @eirikbakke contributed to the change).

neilcsmith-net commented 4 years ago

@jlahoda :+1: Should we have a matching UI addition in the NBM description inside the third-party UC? Warning people that nb-javac only supports editing up to Java 13? If people go to the plugin centre to install it still, they may need a warning until there is an update available?

ebarboni commented 4 years ago

how to merge that to get @eirikbakke in ?

eirikbakke commented 4 years ago

My "contribution" was meant as a trivial change... no need to mention me specifically. Or if for provenance purposes, just mention it in the commit message.

jlahoda commented 4 years ago

@ebarboni - I assume this will be squashed, so I think it would be enough to edit the comment to mention Eirik. I can do the merge, if you want and approve.

neilcsmith-net commented 4 years ago

@jlahoda description looks good, thanks!

ebarboni commented 4 years ago

@jlahoda do the merge. it's ok