Bug 102081

Proposed solution is to provide an interface FileCommandActionProvider similar
to o.n.spi.project.ActionProvider that would be implemented by modules that want
to provide single action semantics and placed into default lookup. 

public interface FileCommandActionProvider {
    // Expected only COMMAND_..._SINGLE
    String[] getSupportedActions();
    void invokeAction(String command, Lookup context) throws IAE;
    boolean isActionEnabled(String command, Lookup context) throws IAE;
}

Then the FileActionCommand would be modified. In case the project type
ActionProvider does not enable the given action for a given context
FileActionCommand would look for FileCommandActionProvider instances to check if
any of them enables the action for given context.

Created attachment 41409 [details]
FileCommandActionProvider.java

Created attachment 41410 [details]
modified FileCommandaction

Created attachment 41411 [details]
diff of FileCommandAction

Asking for API review and comments.

[JG01] Why invent another interface? IMHO we could just look for ActionProvider
in default lookup.


[JG02] Do not use HashSet. Preserve order.


[JG03] Do not log anything at INFO.


[JG04] The code

for (String action : p.getSupportedActions()) {
  if (action.equals(getCommand())) {
    providers.add(p);
  }
}

could be simplified to

if (Arrays.asList(p.getSupportedActions()).contains(command)) {
  providers.add(p);
}


[JG05] The actionPerformed method is wrong since it could perform an action
multiple times. After any call to invokeAction, return from the method.


BTW (as I mentioned to Honza Pokorsky) the new file
FileCommandActionProvider.java has the wrong license header. For now you have to
customize cddl-license.txt in every userdir.

Jesse, thanks for API/code review. I agree with all your comments:

JG01 - I will just add javadoc comment into ActionProvider and not add FCAP.
JG02 - changed to ArrayList
JG03 - The logging was just for my testing, sorry I left it in there.
JG04 - cool, done
JG05 - thanks for catching that

Created attachment 41414 [details]
updated diff w/Jesse's comments addressed

Created attachment 41415 [details]
updated diff w/Jesse's comments addressed

MK01: I assume the actions provided in this way will not rely on build script or
any internal for ant projects, correct? I think this is ought to work with maven
based projects as well.

MK01 Yes, this is my assumption as well, at least for the cases I had in mind. I
am not sure it has to be mentioned explicitly in javadoc, though.

Y01 I somehow feel the test coverage is not really big, could you spend a 
little bit of time and make it a bit greater? I guess it is one simple use of 
MockServices...

Y02 I can see the new diff is much smaller, but still we need to increase the 
spec version of the module and have an apichange entry. That way people will 
be able to depend on the version of the module that does provide this new API.

In issue 102040 you mention that Run File is enabled for Java files, but
disabled for JavaScript files. Thus I assume you are talking about files inside
a project. Then an alternative solution could be a LookupMerger<ActionProvider>
and registering your action provider in the project's lookup. That would have
the advantage that your ActionProvider can be project-dependent. The
disadvantage is that your ActionProvider would be project-dependent (so for JS
files you need to register it in the lookups of all projects) and that it
wouldn't work for files not owned by any project. The former could that be
solved by a Projects/Lookup folder included in the lookups of all projects
(which would be nice to have anyway), and the latter is larger (see e.g. issue
71231, which Pavel's API change does not address -- unless we agree that db can
depend on projects/projectapi of course).

Anyway, if we go on with the proposed change:

AB01: I would say better not to call the default lookup ActionProvider's if
project.length > 1; do it only when == 0.

AB02: I guess you want the action for JS files in the main menu to be named "Run
<file>", as it is for Java files. This doesn't seem to be allowed by the patch.

AB03: FileCommandAction is used in the main menu, thus it can be long-lived,
thus ActionProvider's can be added and removed to/from the default lookup during
the action's lifetime, thus you probably need to listen on the default lookup
for ActionProvider's. Haven't tried your patch, just guessing, so sorry if it's
a false alarm.

"the advantage that your ActionProvider can be project-dependent" is probably no
real advantage, since you can just register a global AP which inspects the
project owning the file for various conditions and conditionally enables the action.


AB03 is probably a real problem (for the global menu item, not for context menu
items). Is it necessary to precompute the set of ActionProvider's? Is there
really a performance issue with just asking Lookup each time?

Re. project-dependent ActionProvider's: I was thinking that e.g.  registering an
AP in the Projects/o-n-m-web-project/Lookup folder inherently enables that AP
only in the context of a web project, and that actions really want to do that.
But you are right, actions are probably sensitive to the services provided by
the project, not to the project type (in the example above it would e.g. try to
find a WebModule for the current file).

I do not have time to work on it now. When the comments are addressed it should
be submitted for another review.

BTW, one other use case might be Run SQL in .sql file.

I did not mean to task Milos with this, I just set it to default owner. If I
have time I will reassign back to myself.

Obviously not in M10; status?

*** Issue 136835 has been marked as a duplicate of this issue. ***

This needs to be reconsidered and resubmitted. Miloši do you want to work on it, or should I?

I will take it.

Created attachment 84850 [details]
Pavel's last patch, updated for current sources

Created attachment 84854 [details]
Addresses AB01, AB02, AB03

Created attachment 84855 [details]
Sample module project using new API

Attached a sample module which allows you to F9 a *.pdf by running evince on it. Seems to work fine.

Currently it looks like there are going to be maybe three impls, all RUN_SINGLE on a single file of specific type. If
there prove to be a lot of impls, it would be undesirable to iterate them all in global lookup (too many classes
loaded). In that case it would be straightforward to create an annotation-based registration which lazily loads impls by
command and file type, e.g.

public class RunJavaScript {
  @GlobalFileSensitiveActionRegistration(command=ActionProvider.COMMAND_RUN_SINGLE, mimeType="text/x-javascript")
  public static void run(FileObject file) {...}
}

Right now that seems premature, so I would leave it to a future API request if the need arises.

Created attachment 84856 [details]
New patch with test and API change information

Please review the last patch.

code looks ok, I'm a bit worried generally about the actual implementations though. I don't see how one can write an
action provider without the ties to the project type, so my worry is that some impls might opt for ant based support
only. But let's see..

Milos, if you look at the issues that are currently blocked by this (running Ruby, JavaScript and SQL), they are all
pretty independent of Ant and I don't see why any of them would want to be tied to Ant. So I don't share your worries.
This enhancement is really useful, and will help NetBeans become more competitive with lightweight text editors. Jesse,
thanks many times for taking it on.

core-main #925e1deec0a8