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

SingleJavaSourceRunActionProvider fixes #7776

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

asotona
Copy link

@asotona asotona commented Sep 20, 2024

java/java.file.launcher/src/org/netbeans/modules/java/file/launcher/actions/SingleJavaSourceRunActionProvider.java is responsible for launching single Java sources living outside of any projects. In comparison to similar actions in Ant or Maven projects is SingleJavaSourceRunActionProvider very raw and does not provide user comfort.

Beside reported bug #7142 "Unable to type in the output panel with single Java file" I found following usability obstacles:

  • it also reuses single output panel without an option to run two distinct Java sources in parallel
  • it does not show name of the executed Java source
  • it does not save modified Java source before execution.

Proposed patch fixes all the above + adds a user comfort of stoping previously running instance of the same Java source. It significantly reduces distraction and confusion of Java newbies when using NetBeans.

Please review.

Thank you,
Adam


^Add meaningful description above

By opening a pull request you confirm that, unless explicitly stated otherwise, the changes -

  • are all your own work, and you have the right to contribute them.
  • are contributed solely under the terms and conditions of the Apache License 2.0 (see section 5 of the license for more information).

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.

Copy link
Member

@mbien mbien left a 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.

@mbien mbien added Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) labels Sep 20, 2024
@asotona
Copy link
Author

asotona commented Sep 23, 2024

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.

@asotona
Copy link
Author

asotona commented Sep 23, 2024

BTW: another obstacle is missing central "--enable-preview" switch for all single-source-run.

Copy link
Member

@mbien mbien left a 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.

@asotona
Copy link
Author

asotona commented Sep 23, 2024

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:

  • single-source-run is not just unintuitive, it is unusable in the current shape and has never been usable
  • I spent quite a lot of time to fix the situation and fighting the arguments like "NetBeans is too complicated to start with" and "Let's switch to Idea or VS Code".
  • My first implementation opened a new tab for each execution (as you propose), however with almost invisible x button to stop the program and with stacked progress bar - the user experience was still negative. Your example about chat bot is exactly the situation where second execution of the same program cause SocketException, completely confusing the newbie user (and requiring several actions to fix the situation for experienced NetBeans user).
  • Based on the class experience and feedback I added auto-save and auto-cancel to the single-source-run and user experience has improved significantly.

This is my feedback and experience of my class.
Based on these facts I'm investing my time and effort to improve NetBeans single-source-run user experience with this contribution and I expect it will pay back to me as NetBeans usable out of the box for my class (and nothing less).

@mbien
Copy link
Member

mbien commented Sep 23, 2024

single-source-run is not just unintuitive, it is unusable in the current shape and has never been usable

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.

Your example about chat bot is exactly the situation where second execution of the same program cause SocketException,

thats a great lesson - a if is missing somewhere most likely. If you add an artificial mechanism which forces a program into a singleton state, tasks like this aren't solvable without creating two programs.

Based on these facts I'm investing my time and effort to improve NetBeans single-source-run user experience with this contribution

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).

@neilcsmith-net
Copy link
Member

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.

@asotona
Copy link
Author

asotona commented Sep 23, 2024

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).
I think newbie class perspective is clear and any hidden switches only add management costs to the teacher.
I don't understand why all the use cases should have unified approach. Project based actions are ... well project-based and single source execution is completely different. Do you think Volvo unifies its user experience across trucks and personal cars?

Here are the options we have:

  1. I can close this PR and wait for your alternative solution. Later find some time to evaluate it and give you a feedback. All the time running my class on patched NetBeans.
  2. Or let this PR in and improve usability immediately. Later you can come with follow-up improvements, put me on CC of the PR and I can give you my and my class feedback on it.

What do you think?

@asotona
Copy link
Author

asotona commented Sep 23, 2024

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.

@mbien
Copy link
Member

mbien commented Sep 23, 2024

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 Main.java and Util.java and tries to run Main while editing Util without seeing any changes.

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.

@neilcsmith-net
Copy link
Member

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).

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?

@mbien
Copy link
Member

mbien commented Sep 23, 2024

Occupied socket by another instance of the same program cannot be solved by any if.

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.

@asotona
Copy link
Author

asotona commented Sep 23, 2024

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).

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.

Good point. The explicit IO is not necessary, will fix it.

@asotona
Copy link
Author

asotona commented Sep 23, 2024

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 Main.java and Util.java and tries to run Main while editing Util without seeing any changes.

Good point, will fix it, thank you.

@matthiasblaesing
Copy link
Contributor

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.

@asotona
Copy link
Author

asotona commented Sep 23, 2024

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.

@matthiasblaesing
Copy link
Contributor

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.

@asotona
Copy link
Author

asotona commented Sep 23, 2024

How many times in your life did you need to execute something twice?
Did you ever tried IntelliJ Idea or Visual Studio Code?

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.

@asotona
Copy link
Author

asotona commented Sep 23, 2024

This is from Idea

@mbien
Copy link
Member

mbien commented Sep 23, 2024

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

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.

How many times in your life did you need to execute something twice?

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)

Did you ever tried IntelliJ Idea or Visual Studio Code?

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.

@asotona asotona requested a review from mbien September 24, 2024 01:06
@neilcsmith-net
Copy link
Member

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 refactoring.java optional I'd be less concerned. Some platform projects have use for one without the other!

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.

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 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.

@lahodaj
Copy link
Contributor

lahodaj commented Sep 25, 2024

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 refactoring.java optional I'd be less concerned. Some platform projects have use for one without the other!

I'll try to make the dependency the other way (java.file.launcher depending on refactoring.java), so that you can use refactoring.java without traces of source file launcher.

Copy link
Member

@mbien mbien left a 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:
Copy link
Member

@mbien mbien Sep 25, 2024

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)

Copy link
Author

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.

Copy link
Member

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.

Copy link
Author

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?

Copy link
Member

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.

Copy link
Author

@asotona asotona Sep 26, 2024

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.

Copy link
Contributor

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

Copy link
Author

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).

Copy link
Author

@asotona asotona Sep 26, 2024

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.

Copy link
Member

@mbien mbien Sep 26, 2024

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.

@asotona asotona requested a review from mbien September 25, 2024 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) 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.

5 participants