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

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

Merged
merged 7 commits into from
May 2, 2020

Conversation

jlahoda
Copy link
Contributor

@jlahoda jlahoda commented Apr 28, 2020

No description provided.

@jlahoda jlahoda added the Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) label Apr 28, 2020
@jlahoda jlahoda added this to the 12.0 milestone Apr 28, 2020
@jlahoda jlahoda requested review from eirikbakke and ebarboni and removed request for eirikbakke April 28, 2020 04:49
@graben
Copy link
Contributor

graben commented Apr 28, 2020

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

@geertjanw
Copy link
Member

geertjanw commented Apr 28, 2020

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
Copy link
Contributor Author

jlahoda commented Apr 28, 2020

@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
Copy link
Contributor

graben commented Apr 28, 2020

@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
Copy link
Contributor Author

jlahoda commented Apr 28, 2020

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
Copy link
Contributor

graben commented Apr 28, 2020

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

@neilcsmith-net
Copy link
Member

Should this logic also control the dialog at

if (!NoJavacHelper.hasWorkingJavac() && !prefs.getBoolean(KEY_WARNING_SHOWN, false)) {
?

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

@neilcsmith-net
Copy link
Member

neilcsmith-net commented Apr 28, 2020

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

+1

@eirikbakke
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

@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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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

@neilcsmith-net
Copy link
Member

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
Copy link
Contributor

eirikbakke commented Apr 29, 2020

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
Copy link
Member

@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
Copy link
Contributor

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
Copy link
Contributor Author

jlahoda commented Apr 29, 2020

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
Copy link
Member

@jlahoda 👍 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
Copy link
Contributor

how to merge that to get @eirikbakke in ?

@eirikbakke
Copy link
Contributor

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
Copy link
Contributor Author

jlahoda commented Apr 30, 2020

@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
Copy link
Member

@jlahoda description looks good, thanks!

@ebarboni
Copy link
Contributor

ebarboni commented May 1, 2020

@jlahoda do the merge. it's ok

@jlahoda jlahoda merged commit 6a82cfb into apache:master May 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants