ActionProvider and its Lookup in NbLaunchDelegate

[jtulach]

Second attempt to integrate #7011. Makes sure the FileObject (and its getLookup()) are sent even in case of COMMAND_RUN and COMMAND_DEBUG. As a result even "project oriented actions" can find out what's the currently selected file and operate on it.

My Enso projects don't have ActionProvider in their lookup. That misleads NbLaunchDelegate.java to create a Collection with null element that yields NullPointerException as soon as one tries to iterate over the collection and invoke a method on individual ActionProvider.

[matthiasblaesing]

The test failure here and in master for the "LSP tests on Linux" branch match. Before anything more is added to master, that needs to be resolved. Either by reverting the commits that introduced the problem or by adding a real fix.

[jtulach]

VSCode Extension Tests on JDK 17 were green prior 4d448c0 ... but with 4d448c0 there is a timout failure and lsp server tests fail too... maybe it is random...

[sdedic]

I am more than little nervous about introducing all FileObject's Lookup's contents - that is all services provided by the FO (editor, ...) into the (temporary) default Lookup (Lookups.executeWith()).

[jtulach]

I am more than little nervous about introducing all FileObject's Lookup's contents - that is all services provided by the FO (editor, ...) into the (temporary) default Lookup (Lookups.executeWith()).

These services are available anyway. One just needs to obj.lookup(FileObject.class).getLookup().lookup(TheService.class). I just find code that obtains file object lookup and seeks for DataObject obscure. Certainly it is not the way I intended the FileObject.getLookup() to be - it was supposed to provide all services representing the selected file object.

Of course, should there be any performance consequences, I can write the longer complex lookup code. However I doubt there is any performance slowdown. The services are registered lazily (I hope) and moreover the selected FileObject is probably opened in VSCode editor and most of its services is initialized anyway...

[sdedic]

Of course, should there be any performance consequences, I can write the longer complex lookup code. However I doubt there is any performance slowdown. The services are registered lazily (I hope) and moreover the selected FileObject is probably opened in VSCode editor and most of its services is initialized anyway...

I am not that concerned by performance, but rather by the pollution of the contextual default Lookup. I would prefer to have some boundaries, so services originally meant to be just local (i.e. bound to a file) are not broadcasted to everyone.

[jtulach]

Of course, should there be any performance consequences, I can write the longer complex lookup code. However I doubt there is any performance slowdown. The services are registered lazily (I hope) and moreover the selected FileObject is probably opened in VSCode editor and most of its services is initialized anyway...

I am not that concerned by performance, but rather by the pollution of the contextual default Lookup. I would prefer to have some boundaries, so ...

Actions in regular NetBeans UI (menu or toolbar) observe global context which at the end delegates to Node.getLookup(). For a node representing a (Java) file that lookup is equivalent to FileObject.getLookup(). As such I find using FileObject.getLookup() as context passed to ActionProvider natural.

The lookup passed to ActionProvider implementations already contained FileObject, but that was more a shortcut than a properly thoughtout design. At least that's what I got from a discussion with Martin @entlicher.

Passing different lookup to isEnabled and invokeAction would be bad - passing as similar lookup as possible is better.

... services originally meant to be just local (i.e. bound to a file) are not broadcasted to everyone.

Are you referring to my change that passes FileObject to ActionProvider.COMMAND_RUN and ActionProvider.COMMAND_DEBUG (and not only _SINGLE)?

I need the FileObject even in this case as I still want to execute only a single file (btw. I had to modify Enso action provider to support COMMAND_RUN and COMMAND_DEBUG - otherwise I don't get callback from NbLaunchDelegate at all. I could possibly modify the code in NbLaunchDelegate to fallback to COMMAND_DEBUG_SINGLE when the provider doesn't support COMMAND_DEBUG and then I wouldn't need to pass FileObject into the Lookup at all. But the current state seemed like a smaller change.

At the end I'd like to point that that the change to pass FileObject.getLookup() has already been approved in #7011 - so I hope at least @dbalek is still fine with it.

[jtulach]

I've just forced pushed from fb0fa22 to d07364e and changed the target to delivery to check whether my PR behaves better on JDK11.

[lahodaj]

So, trying to understand the change here. The summary only speaks about ensuring missing ActionProvider does not cause NPEs, which certainly sounds correct - it is OK for projects to not have any particular service in their Lookup. Besides that, this change appears to modify the Lookup used for isActionEnabled, but only for that, not for invokeAction. I guess my concerns so far are:

• adding the whole FileObject.getLookup() - but I understand this may match the usual content of the Lookup when running inside the whole NetBeans GUI, so this is not a huge concern.
• adding the whole Project.getLookup() - this seems a bit more troublesome. Project Lookups are not (AFAIK) required to contain the reference to the Project (so this is potentially a breaking change), and also contain various services which may interact with global services, etc. Unless the corresponding Lookup in NB GUI contains the content of the current Project, then I'd be much happier if this was dropped.
• please correct me if I am wrong, but: these changes are made only to the Lookup used for isActionEnabled ("test Lookup"), but not to the Lookup used for invokeAction ("invoke Lookup"). Sorry, but this seems clearly wrong to me. The invoke Lookup must be a (proper or improper) superset of the test Lookup, IMO. I.e. with this change, it seems to me the test Lookup contains DataObject, but the actual invoke Lookup does not(?). Not sure what is the ActionProvider that relies on DataObjects then supposed to do? (The Enso provider expects DataObject in isActionEnabled and FileObject in invokeAction - that is OK, but ActionProviders should not be forced to balance between DataObjects and FileObjects this way.)

[jtulach]

made only to the Lookup used for isActionEnabled ("test Lookup"),
but not to the Lookup used for invokeAction ("invoke Lookup")

I am pretty sure the createTargetLookup is used for both. Otherwise, as you point out, it would make no sense. Btw. the ActionProvider in question is here.

• Project Lookups are not (AFAIK) required to contain the reference to the Project (so this is potentially a breaking change),

I thought they are required to contain this. I can make sure to add Lookups.fixed(prj) if prj.getLookup().lookup(Project.class) != prj - if you want.

• adding the whole FileObject.getLookup() - but I understand this may match the usual content of the Lookup when running inside the whole NetBeans GUI, so this is not a huge concern.

I hope it matches the original spirit more. On the other hand I admit I was ready to remove the change, if it was the reason why the tests were failing (but it wasn't).

[sdedic]

• adding the whole Project.getLookup() - this seems a bit more troublesome. Project Lookups are not (AFAIK) required to contain the reference to the Project (so this is potentially a breaking change), and also contain various services which may interact with global services, etc. Unless the corresponding Lookup in NB GUI contains the content of the current Project, then I'd be much happier if this was dropped.

The corresponding GUI lookup would be probably Utilities.actionsGlobalContext(). But we are changing the default Lookup here (using Lookups.excuteWith) -- that is if I interpret the code correctly. Not only GUI actions, but everything in NB would be affected by the added data/services.

[lahodaj]

made only to the Lookup used for isActionEnabled ("test Lookup"),
but not to the Lookup used for invokeAction ("invoke Lookup")

I am pretty sure the createTargetLookup is used for both. Otherwise, as you point out, it would make no sense. Btw. the ActionProvider in question is here.

Sorry, but I don't see this in the code. I may be reading the code wrong, but what I see, the createTargetLookup is only used to produce lookup sent to findActionProvider and findNestedActionProviders, and neither of those seems to do invokeAction (that is done on a completely different level, as far as I can tell). The code is a bit messy, but I simple don't see this change affect both.

In addition to that, I've added this log:

diff --git a/java/java.file.launcher/src/org/netbeans/modules/java/file/launcher/actions/SingleJavaSourceRunActionProvider.java b/java/java.file.launcher/src/org/netbeans/modules/java/file/launcher/actions/SingleJavaSourceRunActionProvider.java
index 007764c964..61c6a38686 100644
--- a/java/java.file.launcher/src/org/netbeans/modules/java/file/launcher/actions/SingleJavaSourceRunActionProvider.java
+++ b/java/java.file.launcher/src/org/netbeans/modules/java/file/launcher/actions/SingleJavaSourceRunActionProvider.java
@@ -53,6 +53,7 @@ public final class SingleJavaSourceRunActionProvider implements ActionProvider {
     })
     @Override
     public void invokeAction(String command, Lookup context) throws IllegalArgumentException {
+        System.err.println("invokeAction context content: " + context.lookupAll(Object.class));
         FileObject fileObject = SingleSourceFileUtil.getJavaFileWithoutProjectFromLookup(context);
         if (fileObject == null) 
             return;
@@ -77,6 +78,7 @@ public final class SingleJavaSourceRunActionProvider implements ActionProvider {
 
     @Override
     public boolean isActionEnabled(String command, Lookup context) throws IllegalArgumentException {
+        System.err.println("isActionEnabled context content: " + context.lookupAll(Object.class));
         FileObject fileObject = SingleSourceFileUtil.getJavaFileWithoutProjectFromLookup(context);
         return fileObject != null;
     }

and ran a source file outside of a project, and got this output:

isActionEnabled context content: [org.netbeans.modules.java.JavaDataObject@3acf453f[/home/jlahoda/test-projects/multi-file-unnamed/test/Test.java@bd92ca97:7de40d80], CES: DocumentOpenClose: SimpleES@1970961222, documentStatus=OPENED, docRef=(NbEditorDocument@91025795), /home/jlahoda/test-projects/multi-file-unnamed/test/Test.java@bd92ca97:7de40d80, org.netbeans.modules.java.JavaNode[name=Test; displayName=Test.java][Name=Test, displayName=Test.java]]
invokeAction context content: [/home/jlahoda/test-projects/multi-file-unnamed/test/Test.java@bd92ca97:7de40d80, org.netbeans.api.extexecution.base.ExplicitProcessParameters@30be000e, org.netbeans.modules.java.lsp.server.debugging.launch.NbProcessConsole@1de77413, org.netbeans.modules.java.lsp.server.debugging.launch.NbLaunchDelegate$1@a74d360]

To me, it seems clear the isActionEnabled context contains stuff that is not in the invokeAction context.

• Project Lookups are not (AFAIK) required to contain the reference to the Project (so this is potentially a breaking change),

I thought they are required to contain this. I can make sure to add Lookups.fixed(prj) if prj.getLookup().lookup(Project.class) != prj - if you want.

I wonder what's the purpose of flattening the whole Project's Lookup into the context. Is that done anywhere else? What do we expect the ActionProviders will lookup here? Project Lookups may contain stuff that may conflict with other things, because they are normally a separate domain. IMO, at least.

And, if both Project and FileObject Lookup are flattened, what about their order? And what about duplicities?

I don't think the Project Lookup must contain anything Lookup.EMPTY is absolutely valid.

• adding the whole FileObject.getLookup() - but I understand this may match the usual content of the Lookup when running inside the whole NetBeans GUI, so this is not a huge concern.

I hope it matches the original spirit more. On the other hand I admit I was ready to remove the change, if it was the reason why the tests were failing (but it wasn't).

From my view - I'd say that FileObject.getLookup() is much more acceptable (because I think you are right that's the normal context lookup) than Project.getLookup() (which I don't think is ever flattened anywhere).

[lahodaj]

• adding the whole Project.getLookup() - this seems a bit more troublesome. Project Lookups are not (AFAIK) required to contain the reference to the Project (so this is potentially a breaking change), and also contain various services which may interact with global services, etc. Unless the corresponding Lookup in NB GUI contains the content of the current Project, then I'd be much happier if this was dropped.

The corresponding GUI lookup would be probably Utilities.actionsGlobalContext(). But we are changing the default Lookup here (using Lookups.excuteWith) -- that is if I interpret the code correctly. Not only GUI actions, but everything in NB would be affected by the added data/services.

Sorry, I don't see this in the code. I see this Lookup is only sent to ActionProvider.isActionEnabled. And (an enhanced version of) it should also be sent to ActionProvider.invokeAction. But that should be all, I think. I may be wrong, of course.

[sdedic]

Sorry, I don't see this in the code. I see this Lookup is only sent to ActionProvider.isActionEnabled. And (an enhanced version of) it should also be sent to ActionProvider.invokeAction. But that should be all, I think. I may be wrong, of course.

I stand corrected, that Lookup does not escape findTarget; I misread the diff mistakenly matched the action context Lookup and the build action Lookup.

[jtulach]

I stand corrected,

Good. I was just about to write some test. To prevent misunderstandings, can you and Lahváč state exactly what you don't like at current state of affairs? So I can address exactly what you believe is unbearable? Thank you.

[lahodaj]

I stand corrected,

Good. I was just about to write some test. To prevent misunderstandings, can you and Lahváč state exactly what you don't like at current state of affairs? So I can address exactly what you believe is unbearable? Thank you.

So, from me:

a) as far as I can tell, the current enhanced lookup is only used for isActionEnabled and not invokeAction. Please see:
#7059 (comment)

b) I would like to hear rationale for including prj.getLookup() rather than just prj. Is this done anywhere else? Seems a bit non-standard, and I would be happier if we used just prj instead of prj.getLookup().

I personally don't have so strong opinion for including file.getLookup(), so I'll leave to up to you and Svata.

[sdedic]

Please retarget the PR to the master.

To prevent misunderstandings, can you and Lahváč state exactly what you don't like

• I would second making consistent the context Lookup for isActionEnabled and invokeAction calls.
• as far as I understand the code, project actions are (usually) called with something like LastActivatedWindowLookup.INSTANCE or Utilities.globalActionLookup(). There are project instances present, but not expanded project lookups, usually (looking at ProjectSensitiveAction code)
• looking at other sources that invoke isActionEnabled ... expanding fileObject's lookup gives me still some itching, but it is what's happening when proxying selected Node lookups. For the context lookup passed as parameter it is OK/compatible

[jtulach]

• I would second making consistent the context Lookup for isActionEnabled and invokeAction calls.

Now I see where the problem is: It was consistent once: f5954a9 - but it is not consistent in the overall state of this PR. I guess I messed something up when merging or re-targeting to delivery branch.

Putting the code back with 483b658 - now the lookup sent to isActionEnabled and invokeAction shall be (almost) the same.

[jtulach]

rationale for including prj.getLookup() rather than just prj. Is this done anywhere else?

No, probably it is not done. In fact it not even a project instance is being sent in NetBeans IDE. The lookup I am seeing in isActionEnabled for run.single command on .enso file is:

lkp = (org.netbeans.modules.project.ui.actions.LookupSensitiveAction$LastActivatedWindowLookup) ProxyLookup(class=class org.netbeans.modules.project.ui.actions.LookupSensitiveAction$LastActivatedWindowLookup)->[AbstractLookup[IL[org.enso.tools.enso4igv.EnsoDataObject@4a5cf04f[
/NetBeansProjects/enso/enso/engine/runtime/src/main/scala/org/enso/interpreter/instrument/newEnsoTemplate.enso@b45c7282:377acd25], org.netbeans.modules.openide.loaders.SimpleES, 
IL[org.openide.loaders.MimeFactory@5aff7ebf, IL[org.openide.loaders.DataNode[name=newEnsoTemplate; displayName=newEnsoTemplate.enso][Name=newEnsoTemplate, displayName=newEnsoTemplate.enso], 
IL[/NetBeansProjects/enso/enso/engine/runtime/src/main/scala/org/enso/interpreter/instrument/newEnsoTemplate.enso@b45c7282:377acd25]]

E.g. it contains FileObject.getLookup(), but it doesn't seem to contain the project neither its lookup.

[jtulach]

I haven't found a way to re-target this PR to master branch. Let's continue (and hopefully finish) the review in - #7105

[neilcsmith-net]

Looks like it got automatically closed and locked when delivery was deleted. Sure that behaviour used to be different and open PRs got moved on to master.

Anyway, short answer is please don't target delivery if you're not aiming a PR for the release or it causes a bunch of issues!

[jtulach]

Anyway, short answer is please don't target delivery if you're not aiming a PR for the release or it causes a bunch of issues!

I was aiming to get the fix into a close release. I just haven't got the approvals soon enough as there was a discussion around some of my changes.

Now I need to wait (at least) next three months before I can merge enso-org/enso#9041

[neilcsmith-net]

@jtulach at no point was merging this for release discussed in review, and #7011 was merged into master rather than delivery anyway?! After release branching, if you want something in the release it needs to follow the process outlined at https://cwiki.apache.org/confluence/display/NETBEANS/Pull+requests+for+delivery