-
Notifications
You must be signed in to change notification settings - Fork 874
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
Single Java source run action provider improvements #7776
Single Java source run action provider improvements #7776
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some good changes but I think the single-source-run behavior should work analog to regular projects
- running something twice, should not cancel the first run, it should simply start a second run in another tab (e.g number added to the tab name)
- and we should add a cancel button to the output panel analog to regular projects, I had that on my TODO list but never got to it so far
the second point doesn't have to be solved here of course. I think the hook to the background processes list should already make it cancellable via the x - which is great.
For the explanation: I use NetBeans in my Java newbie class. We base on JEP 330: Launch Single-File Source-Code Programs and JEP 477: Implicitly Declared Classes and Instance Main Methods. Unfortunately from teachers perspective I could not agree with your statement "single-source-run behavior should work analog to regular projects". Here we need a user-friendly and foolproof solution (following the "Paving the on-ramp" idea), which does not turn NetBeans into a mess of instances of the same program, running each in a separate tab, and each waiting on basic user input (like for example "enter a number:"). I'm contributing this patch back to NetBeans after thorough testing in my class and with hope that I won't need to distribute patched NetBeans to my class next time. Thank you. |
BTW: another obstacle is missing central "--enable-preview" switch for all single-source-run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
single sourcecode programs can certainly be useful as first steps but they were not intended as dialect or mode specific for new programmers. I do also think you don't give newbies enough credit, the fact that a program might keep running till its stopped is fairly intuitive (or well known at this point if the CS theory class already happened) - anything else would be weird.
The tabs do also work somewhat analog to browser tabs, press play and you get a new one. The default setting for maven projects for example is to reuse tabs of finished processes - so if your program exits regularly you won't ever see more than one tab.
There are also some tasks students might have to do which would actually require starting the same program several times. "write a chat bot", "send something over a socket", (...).
Inconsistency itself causes friction while learning something.
Thank you for your feedback about JEP 477 and for your opinion about what is intuitive for Java newbies when using NetBeans. However this situation is a bit opposite, so let's speak clearly:
This is my feedback and experience of my class. |
the current implementation of this feature is rudimentary and does lack feature parity with other areas - I fully agree. I do disagree that it is unusable since I use it since it is available for various scripts. It still has some code scanning issues which needs better UX too.
thats a great lesson - a
thanks for that - good contributions are always welcome. As I said there are some good fixes in there. save-before run and working input streams etc do add parity to other areas. Singleton process by default go into the wrong direction though. It could be a setting though (e.g toggle button). The output has the entire toolbar missing, typically it would have a stop button, re-run, debug etc (this all needs to be implemented at some point for single-source scripts). |
Personally, I'd say singleton process by default is the right direction, and ideally would have been the default (but overridable) for all project actions. I'm not sure I agree with auto-cancelling the run though. The action should be disabled and require the manual termination of the first run. |
OK, let's move this topic forward. This is my complete solution proposal and I don't have much more spare time to work on it. Subsequent single-source-run executions take over a single output tab (single for all executions across the whole IDE), however it keeps running on background - that is an absence of usability (so I call it unusable). Here are the options we have:
What do you think? |
BTW: I think there is a confusion about "singleton process". I'm proposing a single process/output tab per source file, that does not mean "singleton process" (as contrast to the current singleton output tab implementation). Occupied socket by another instance of the same program cannot be solved by any if. Chat application (single app with two threads) is one of the cases I'm having the biggest problem with the current single-source-run. And split into two application is an option which also works in NetBeans only after my proposed patch, because current single-source-run re-uses singleton output tab for all programs. |
there is no need to rush anything in. The next release is still a bit away. There are other small details which likely could be improved on this PR too, for example the run action should likely save all unsaved tabs (analog to other project based run actions), not just one file since this could create an annoying loop where a student has Please leave this PR open, even if you don't have time for follow ups, It can still be closed in case other impls appear. |
Why is this code configured to use both custom IO and controllable? https://bits.netbeans.org/23/javadoc/org-netbeans-modules-extexecution/org/netbeans/api/extexecution/ExecutionDescriptor.html#controllable-boolean- According to the docs these are mutually incompatible. Requesting review from @jlahoda And is this code used in VSCode plugins too? |
It might be different now but the first two semesters were essentially tasks like: "Pass a long between two processes and implement it in 1-3 languages (or stick to java but write one for corba, sockets and RMI)." public void main() throws IOException {
Path socketFile = Path.of("/tmp/connector");
long myPid = ProcessHandle.current().pid();
println("my pid "+myPid);
UnixDomainSocketAddress address = UnixDomainSocketAddress.of(socketFile);
ByteBuffer bb = ByteBuffer.allocate(8);
if (Files.notExists(socketFile)) {
try (ServerSocketChannel server = ServerSocketChannel.open(StandardProtocolFamily.UNIX)) {
server.bind(address);
try (SocketChannel channel = server.accept()) {
bb.putLong(0, myPid);
channel.write(bb);
}
}
} else {
try (SocketChannel channel = SocketChannel.open(StandardProtocolFamily.UNIX)) {
channel.connect(address);
channel.read(bb);
println("other program pid: "+bb.getLong(0));
}
Files.delete(socketFile);
}
println("goodbye");
} It might no longer be common, but I don't think an IDE should prohibit to run things like that due to extra process-cancel magic. I think once the toolbar is there and other UX features are implemented it all will "fall into place" and make sense for newbies. The cancel-other feature might still be useful as toggle action on a toolbar - but I don't think it should be the default. |
Good point. The explicit IO is not necessary, will fix it. |
Good point, will fix it, thank you. |
...r/src/org/netbeans/modules/java/file/launcher/actions/SingleJavaSourceRunActionProvider.java
Outdated
Show resolved
Hide resolved
...r/src/org/netbeans/modules/java/file/launcher/actions/SingleJavaSourceRunActionProvider.java
Outdated
Show resolved
Hide resolved
I agree with @mbien point. The behavior of "run file" action for normal projects is that the code is started. With the start an output tab is opened and the output of the run is directed there. If a second run is started while the first is still active, a new tab is opened. A tab will be reused if the corresponding run has ended. I verified this for gradle and maven. I don't see why the single-source run should not follow that pattern. We can agree or disagree, that this is a good behavior, but at least it would be consistent. |
According to my experience in all cases I executed something twice, it was always by accident and always caused some problems. However I'm working on NetBeans and with NetBeans for about 25 years, so I used to it. For us it is a pattern, which does not make any sense, however for newbies is something they should add to their learning curve. So NO, bad patterns should not be blindly followed. When you would face it live within a group of confused people, you would definitely change your mind. And BTW I'm not asking to change behavior of the existing Maven, Ant and Gradle projects, I'm asking to fix single file execution (which never worked correctly) to make first user experience with NetBeans and Java more friendly. |
I don't see why "Run file" invoked for the same java file should result in different behavior of the IDE depending on the folder the file resides in. If it is inside the source root of the "real" project, it can be started multiple times, will get multiple tabs when that happens. If it is outside such a folder it will get different behavior. Of course that can be explained, but that will be much harder, as that is subtle. To me it sound logical, that executing a file twice will result in two instances running. And if you look at the way it is handled in the maven integration, you see a tab visually being opened. If the second run would kill the first one, that would be subtle, because I could not explain why the first run was killed. |
How many times in your life did you need to execute something twice? I would suggest at least to popup a dialog with warning about second execution of something already running, with permanent global "don't ask me again" option. And this should be standard for all executions across all project types. |
yes we understand, but this is unlikely to happen. I am sure nobody would have anything against a setting in the options or a toggle button directly on the output window toolbar, but single file programs are nothing special, they are no dialect and not exclusive for beginners - there is value in consistent behavior. I have faith in beginners to understand that every time play is pressed it will launch the program. Once the big red stop button is there it will be even more self explanatory.
Additionally as demonstrated #7776 (comment) IDEs should be flexible enough to support basic CS student assignments. I also remember having peer 2 peer demos were i pressed play 5 times to quickly produce nodes in a network. (peer to peer used to be cool at some point)
I use essentially all IDEs, as freelancer I use whatever most of the other devs use in the project to reduce friction. In projects where it doesn't matter I typically fall back to NB. Its OK that IDEs solve some problems slightly differently. |
I'm disinclined to adding the UI in this module itself. I'm a little worried about this module growing in size and dependencies. Maybe if we could make the dependency from Personally I don't have an issue with just having a boolean property on the file to allow concurrent execution, defaulted off, but possibly with the default brandable for VSCode, etc? However, if consistency is the key for others, then let's just make sure we have multiple tabs and the executions are controllable and leave it at that.
For me, it's not always by accident, but I can see a strong case for it having been off by default. eg. concurrent executions of Maven tasks in the same reactor are usually a mistake. I'm not sure it's an easy thing to retrofit, and I think consistency and transparency of behaviour is also a strong concern for end users of all skill levels. |
I'll try to make the dependency the other way ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't have time to look through this properly yet (or test it), but thanks for adding it as option.
(btw not everything has to be solved in one PR, but since the toolbar is now there and the option panel is there too, adding a settings button analog to gradle/maven/ant would be something we could do too)
@@ -16,3 +16,6 @@ | |||
# under the License. | |||
|
|||
OpenIDE-Module-Name=Java File Launcher | |||
GlobalSettingsPanel.vmLabel.text=VM Options: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what the usecase for this is, but please name it "Additional Global VM Options" to communicate that those are flags which are appended to the regular options. Otherwise users might think they have to edit this setting if they want to change the flags for a file.
(i am also a bit worried that someone might forget about this and it will break random launch configs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a solution for another problem with JEP 477 use in NetBeans. The JEP is in preview, however there is no central way to enable preview for all single source Java files.
I was experimenting to store the individual file properties together with the sources in Git, however .nbattrs are not reflected.
It is far behind acceptable usability to let every student every source file first see with errors and then either wait for hint to enable preview (which does only a half of the job by setting the preview partially also for the parent folder, so other sources in the same folder are never prompted with the hint and they fail to run). Or student can manually type --enable-preview into each single Java file properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JEP is in preview, however there is no central way to enable preview for all single source Java files.
good. This would break things very fast. Enabling preview locks the bytecode to a single JDK feature version. NB itself has only one version of javac in it and supports only one version of any preview JEP at any given release + any file can use a different JDK to launch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how intentionally user-added command line option to single source launched files could "break things very fast". Is it a sarcasm?
I'm trying to find a way for my students to use NetBeans to learn Java. What is your goal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how intentionally user-added command line option to single source launched files could "break things very fast". Is it a sarcasm?
no sarcasm. Only stating how this will cause non-obvious problems. Esp for those who might be told to quickly add a flag to this text field and then forget about it. Or those who are not aware about the impl details of nb-javac.
I'm trying to find a way for my students to use NetBeans to learn Java. What is your goal?
In this particular PR I mostly try to make sure the single file launcher fits to the rest of NB UX-wise and that it doesn't break other use cases. Basic project maintainer tasks essentially.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This conversation becomes ridiculous.
I'm trying to fix show stoppers to enable NetBeans with JDK latest features for future generation of Java developers. And you argue agains with incredible scenarios of possible problems of people trying to scratch their yesterdays head with tomorrows hand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the source launcher does not produce any (on-disk) classfiles, at least as far as I know. And --source
is no longer mandatory for source launcher, so --enable-preview
is sufficient. I think this significantly reduces possibility of bigger trouble.
And I think simple --enable-preview
should work also for nb-javac, at least to the degree preview can work with nb-javac.
Overall, I don't think the danger with having some global setting is so big. We have global settings for both Maven and Ant, and if I put something wrong there, I may get weird results.
What may be not-ideal is that one cannot easily see the effective command line (i.e. global + per-file VM options). It might make sense to add a new read-only property to the properties view showing the effective/full command line. Shouldn't be too difficult (I hope), see here:
ss.put(new JavaFileAttributeProperty(dObj, FILE_VM_OPTIONS, "runFileVMOptions", "singlefile_options")); // NOI18N |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, however that would require to move the global settings to a module accessible from both (java.source and java.file.launcher).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed the global options are persisted in context of Java Platform module
and added read-only property.
However composition of the command line is confusing, so I've added it just as a mirror of the global options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have global settings for both Maven and Ant, and if I put something wrong there, I may get weird results.
Yes of course but that is the reason I asked for the use case. The difference here is that the preview flag influences build and runtime of all project-less programs, while the maven flags for example are for the maven tooling JVM (-X etc). There isn't really a use case for enabling preview for maven tooling unless for very specific tasks like testing a plugin or something like that.
But the java launcher doesn't even understand -Xlint
, so the global args are super limited here!
I like the idea of showing the global args field in the file properties. The output could also print the exec line analog how maven support does it - but this doesn't have to be all implemented in the same PR.
java/java.file.launcher/src/org/netbeans/modules/java/file/launcher/SingleSourceFileUtil.java
Outdated
Show resolved
Hide resolved
ok. Looks like the impl stabilized. I think next week might be feature freeze for NB 24 already. To prepare integration, please squash everything to a commit and update the msg as you would like to see it in the repo history, feel free to rebase and force push so that CI and the dev build contains latest updates for testing. PRs are linked directly from the (generated) release notes, so the description would need small tweaks to reflect the PR before integration. |
|
@asotona it is purposefully disabled on this repository because it results in wrong commit information. Please squash and force push your own branch to get it into a state for merging. See also https://github.com/apache/netbeans/blob/master/.asf.yaml#L32 |
0fddd72
to
8a5ca86
Compare
8a5ca86
to
b60d6b6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks sensible to me. Please let me know in the next day or two if anyone sees any issues with this, otherwise I'll merge. Thanks!
@lahodaj I'd still prefer to see the issue in #7776 (comment) addressed before expanding this module much more, but don't have a problem with you merging this before that's resolved. |
Sure, I still plan to tackle that, just didn't get to that yet. |
Thank you! |
follow up to fix test #7887 |
Single java source run action provider improvements:
^Add meaningful description above
By opening a pull request you confirm that, unless explicitly stated otherwise, the changes -
Please make sure (eg.
git log
) that all commits have a valid name and email address for you in the Author field.If you're a first time contributor, see the Contributing guidelines for more information.
If you're a committer, please label the PR before pressing "Create pull request" so that the right test jobs can run.