Bug 102081 - SPI for single file actions
SPI for single file actions
Status: RESOLVED FIXED
Product: projects
Classification: Unclassified
Component: Generic Infrastructure
6.x
All All
: P1 (vote)
: 6.x
Assigned To: Jesse Glick
issues@projects
: API, API_REVIEW_FAST
Depends on: 206744
Blocks: 102040 71231 91402
  Show dependency treegraph
 
Reported: 2007-04-22 07:25 UTC by Pavel Buzek
Modified: 2011-12-27 15:45 UTC (History)
7 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments
FileCommandActionProvider.java (2.99 KB, text/plain)
2007-04-22 07:40 UTC, Pavel Buzek
Details
modified FileCommandaction (5.62 KB, text/plain)
2007-04-22 07:43 UTC, Pavel Buzek
Details
diff of FileCommandAction (4.26 KB, text/plain)
2007-04-22 07:44 UTC, Pavel Buzek
Details
updated diff w/Jesse's comments addressed (5.05 KB, application/octet-stream)
2007-04-23 07:19 UTC, Pavel Buzek
Details
updated diff w/Jesse's comments addressed (5.05 KB, text/plain)
2007-04-23 07:19 UTC, Pavel Buzek
Details
Pavel's last patch, updated for current sources (4.57 KB, patch)
2009-07-16 18:38 UTC, Jesse Glick
Details | Diff
Addresses AB01, AB02, AB03 (4.14 KB, patch)
2009-07-16 19:26 UTC, Jesse Glick
Details | Diff
Sample module project using new API (4.53 KB, application/x-compressed)
2009-07-16 19:31 UTC, Jesse Glick
Details
New patch with test and API change information (9.97 KB, patch)
2009-07-16 20:08 UTC, Jesse Glick
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Buzek 2007-04-22 07:25:30 UTC
Single file action such as Run File or Debug File (i.e. those implemented by
o.n.m.prj.ui.actions.FileCommandAction) are controlled by the project type and
are only enabled and supported for files that are known to the project type. For
some files single file actions behavior could be implemented independently of
the project type, but there is no way to plug into the same UI.

Use Case 1: JavaScript provides a Run action to execute a .js file. The action
is currently exposed in editor popup menu ("Run"), but it would be more
intuitive to integrate it into the Run File action. Currently Run File action is
disabled for .js files. (Issue 102040)

Use Case 2: Ant file has a Run Target action and Debug Target action but Run
File and Debug File are disabled. They could be enabled and Run/Debug the
default target in file.
Comment 1 Pavel Buzek 2007-04-22 07:38:36 UTC
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.
Comment 2 Pavel Buzek 2007-04-22 07:40:17 UTC
Created attachment 41409 [details]
FileCommandActionProvider.java
Comment 3 Pavel Buzek 2007-04-22 07:43:40 UTC
Created attachment 41410 [details]
modified FileCommandaction
Comment 4 Pavel Buzek 2007-04-22 07:44:18 UTC
Created attachment 41411 [details]
diff of FileCommandAction
Comment 5 Pavel Buzek 2007-04-22 07:45:55 UTC
Asking for API review and comments.
Comment 6 Jesse Glick 2007-04-23 01:58:53 UTC
[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.
Comment 7 Pavel Buzek 2007-04-23 07:18:24 UTC
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
Comment 8 Pavel Buzek 2007-04-23 07:19:10 UTC
Created attachment 41414 [details]
updated diff w/Jesse's comments addressed
Comment 9 Pavel Buzek 2007-04-23 07:19:23 UTC
Created attachment 41415 [details]
updated diff w/Jesse's comments addressed
Comment 10 Milos Kleint 2007-04-23 07:58:42 UTC
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.
Comment 11 Pavel Buzek 2007-04-23 08:13:20 UTC
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.
Comment 12 Jaroslav Tulach 2007-04-23 10:44:43 UTC
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.
Comment 13 Andrei Badea 2007-04-23 12:22:17 UTC
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.
Comment 14 Jesse Glick 2007-04-23 16:55:07 UTC
"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?
Comment 15 Andrei Badea 2007-04-23 18:19:38 UTC
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).
Comment 16 Pavel Buzek 2007-05-01 20:19:38 UTC
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.
Comment 17 Pavel Buzek 2007-05-01 21:19:14 UTC
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.
Comment 18 Jesse Glick 2007-07-13 18:57:49 UTC
Obviously not in M10; status?
Comment 19 Jesse Glick 2008-06-10 00:22:37 UTC
*** Issue 136835 has been marked as a duplicate of this issue. ***
Comment 20 Jesse Glick 2009-07-16 16:32:15 UTC
This needs to be reconsidered and resubmitted. Miloši do you want to work on it, or should I?
Comment 21 Jesse Glick 2009-07-16 16:41:04 UTC
I will take it.
Comment 22 Jesse Glick 2009-07-16 18:38:58 UTC
Created attachment 84850 [details]
Pavel's last patch, updated for current sources
Comment 23 Jesse Glick 2009-07-16 19:26:49 UTC
Created attachment 84854 [details]
Addresses AB01, AB02, AB03
Comment 24 Jesse Glick 2009-07-16 19:31:49 UTC
Created attachment 84855 [details]
Sample module project using new API
Comment 25 Jesse Glick 2009-07-16 19:40:55 UTC
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.
Comment 26 Jesse Glick 2009-07-16 20:08:59 UTC
Created attachment 84856 [details]
New patch with test and API change information
Comment 27 Jesse Glick 2009-07-16 20:09:56 UTC
Please review the last patch.
Comment 28 Milos Kleint 2009-07-20 09:29:08 UTC
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..
Comment 29 Petr Jiricka 2009-07-20 09:43:20 UTC
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.
Comment 30 Jesse Glick 2009-07-23 14:08:48 UTC
core-main #925e1deec0a8


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo