Created attachment 73897 [details]
Patch adding org.openide.util.ContextAction and tests

I am looking for somebody to help me implement new actions. However it is more work than just having new class to 
subclass. We need declarative registrations, deferred loading, etc. Do you want to work on this task more, or your 
involvement can only be limited?

Sure, I would be interested in helping (assuming bug-fixing and work on mobility in 7.0 do not conflict.  I would like
to get this class into the API because it will allow me to delete code in a bunch of other places.  It would be pretty
simple from here to deprecate NodeAction, CookieAction and even the entire Actions API (the actions will still work,
there can just be non-SharedClassObject replacements somewhere else).


What are we missing for declarative registration?  File attributes for object type and survive-focus-change policy, or
something more?

For deferred loading, do you mean deferring loading the action, or deferring loading the object it is sensitive to in
the lookup (we did something like that in platformx here -
http://hg.netbeans.org/platformx/file/a84d84581e2c/api.objectloader/src/org/netbeans/platformx/api/objectloader/ - you
would make your action sensitive to ObjectLoader<T> in the lookup, and it could manage its enabled state without loading
the object unless the action was invoked - but you do have to handle the case that the load fails for some reason - but
this code was also partly because T might be a 300Mb serialized object, so we also had to guarantee that the loading was
not on the event thread).

Created attachment 73933 [details]
New patch

The attached patch should address all of the use-cases of CookieAction and NodeAction that I can think of or find
reflected in the sources (actually, see the patch I'm about to attach - sorry :-)).

The key method is ContextAction.createIndirectAction(Class<T extends LookupProvider>, ContextAction<?>, boolean) (I'm
starting to think these things might want to be in their own package or o.o.util.actions).  You pass in a type that is a
subclass of Lookup.Provider, such as Node, and an instance of ContextAction which is parameterized on some type that
will be in the Node's lookup.  It takes care of all of the delegation;  if you want to chain things so you just write an
action paramaterized on some object in the project's lookup, you can just call createIndirectAction() with the result of
another call to createIndirectAction(), and the action with the actual logic at the end of the chain.

So, if you want to write an action that is sensitive to ClassPathProviders in the current project, and reject any cases
where a selected node does not contain a project, or where the selected node's project does not contain a
ClassPathProvider, it's two lines of code:

ContextAction<Foo> theRealAction = new MyContextAction(); //the ClassPathProvider sensitive action
Action<Node> theGlobalAction = createIndirectAction(Node.class,
  createIndirectAction(Project.class, theRealAction));

If you do want the action enabled when some nodes don't have a project or some projects don't have a ClassPathProvider,
just use the overload of createIndirectAction that takes a boolean parameter for whether it requires all
Lookup.Providers to provide something usable.

So, Jarda, if you want to make all this declarative, you just need a ContextAction subclass that takes its values from a
file in a layer.xml (or annotation or whatever) and instantiates the real ContextAction on demand, and a spec for
specifying the chaining of types (e.g. Utilities.actionsGlobalContext() -> Node.getLookup() -> Project.getLookup() ->
type).  

And naturally, if you can specify declaratively whether the action needs to run any logic to determine its enabled
state, or if presence/absence, instance count and whether any Lookup.Providers cannot provide an instance of type is
enough, you can avoid loading the action until invocation.

Created attachment 73934 [details]
And another one (added parameter to have createIndirectAction optionally reject enablement if any Lookup.Providers don't have an object they can use)

Created attachment 73936 [details]
Convenience replacement for migrating NodeActions with fewer code changes

This is a pretty major new API. I think we need to review it carefully and make sure it covers realistic use cases (and
no others).


[JG01] A common case that occurs in the project system is that you want enablement to be sensitive to (some
characteristics of) a Project in the selection, or if there is no Project in the selection, the Project which owns the
DataObject in the selection. This lets you press F6 when selecting the project root node, or when selecting any file in
the project. There are a number of rather subtle Action base classes in projectui to handle this and other scenarios. Do
you plan to support anything like this?

No one would use your "indirect" actions in the way you suggest, because file selections do not provide a Project
directly. Even besides that, the suggested use of indirect actions is unlikely because you would not want to look for a
Node in the selection, you would directly look for a Project. (TopComponent's, hence Utilities.actionsGlobalContext,
automatically "splice" the selected Node[]s lookups into the main lookup, as well as including those Node[]s themselves.)


[JG02] Any support for setting the action label according to the selection? This is fairly common.


[JG03] One thing I think is very important is ability to hide an action, not just grey it out, when disabled. Currently
this requires an elaborate trick: a phony Action which is a ContextAwareAction, creating a phony Action which is a
Presenter.Menu (or Presenter.Popup), creating a phony JMenu which is a DynamicMenuContent, which finally creates the
actual JMenu[] you want to display (which you then have to somehow bind to one of the other actions so as get a label
and a event handler). There are a lot of long classes to do this nonsense in our codebase. It would be great to simplify
them with a factory.


[JG04] You again used '.' where you should have used '#' in a Javadoc @see tag to refer to a class member. This will not
work.


[JG05] Don't bother with the HelpCtx.Provider impl.


[JG06] Use Parameters.notNull.


[JG07] NodeContextAction Javadoc says you can override isEnabled, but it is final in ContextAction.


[JG08] checkQuantity has no Javadoc.


[JG09] @param name is wrong in actionPerformed.


[JG10] Node/CookieAction took some care to suspend listening to changes in the global selection while they were not
displayed - i.e. if removeNotify was called, meaning there were no active listeners on them. This would be true for a
menu item (with no visible toolbar button): there is no reason to update its enablement state just because the user is
moving around in the Projects tab, unless and until the user actually posts that menu. Do you plan a similar optimization?


[JG11] hashCode oddly computes the constant 47 * 7, then adds something to it. Anyway I would suggest that equals and
hashCode should _not_ be overridden. I do not see any reason for it.


[JG12] Why does SurviveSelectionChange extend Single? This seems orthogonal. In fact I think it should be a constructor
parameter of ContextAction.

Also, such actions are in fact quite common. For instance, all the project system actions (that I know of) survive focus
change. Otherwise it would be rather irritating to have the Output Window get focus - you would need to switch back to
rerun a project etc.


[JG12] All these classes should be top-level, in a proper package. Probably in a new API module.


[JG13] CookieAction correctly handled the case that a selected node dynamically added or removed a cookie. In practice
this was unusual, mainly applicable to SaveCookie (which I want deprecated anyway - filed elsewhere). Does
LookupProviderAction handle this?


[JG14] It is confusing that LookupProviderAction has enabled(Collection<? extends T>) as well as isEnabled(Collection<?
extends R>) as well as isEnabled()!

> This is a pretty major new API. I think we need to review it carefully and make sure it covers realistic use cases (and
> no others).

Fair enough.  To clarify how this all works:
The key class is ActionStub.  A ContextAction keeps its enablement state (when it keeps anyway, i.e. only between add/removeNotify), in its ActionStub - 
i.e. just a ContextAwareInstance over Utilities.actionsGlobalContext().  isEnabled(Collection) and actionPerformed(Collection) are callbacks.  So, everything 
lives in one place; the rest is delegation code.  So, replace the stub and replace the behavior - i.e . SurviveFocusChange just keeps the last-known-good 
collection of values.

IndirectAction just wraps another ContextAction, and so its context-aware-instances will be a similar wrapper over a context aware instance from the 
delegate.  An IndirectAction can do anything you can do with a LookupProviderAction;  LookupProviderAction simply is more understandable to Java 
programmers who are used to subclassing and overriding a method. 

> [JG01] A common case that occurs in the project system is that you want enablement to be sensitive to (some
> characteristics of) a Project in the selection, or if there is no Project in the selection, the Project which owns the
> DataObject in the selection. This lets you press F6 when selecting the project root node, or when selecting any file in
> the project. There are a number of rather subtle Action base classes in projectui to handle this and other scenarios. Do
> you plan to support anything like this?

I am working on this.  Basically, rather than have complicated enablement logic in one action, you implement each case as a simple ContextAction.  You 
provide one action for each special-case for enablement and generate an action that merges them with a factory method. So, for the scenario you describe, 
you would write an action that was sensitive to Projects, period.  Make two instances of it.  Then:

ContextAction<Project> caresAboutGlobalLookup = new MyProjectAction();
ContextAction<Project> caresAboutDataObject = ContextAction.indirect (DataObject.class, new MyProjectAction());
ContextAction<?> toDeploy = ContextAction.merge (caresAboutGlobalLookup, caresAboutDataObject);

The enablement logic gets a little complicated for the action that merges these, but I think there is a correct algorithm:  if any merged action is enabled, 
the merging action is enabled.  For return values of getValue(), if any actions are enabled, the first enabled one's value is returned (so the active action 
controls the name).  If all delegates are disabled, then the first non-null value, if any, is returned.

Change firing needs a bit of thought, but should be able to do accurately.

Basically, I think nobody should be writing complex enablement logic for any action except in really unusual circumstances - i.e. something much more 
complicated than testing object presence/absence;  and even in that case it shouldn't ever be something more complicated than, say, running 
FileOwnerQuery over a DataObject.  People will have fewer bugs if complex cases are separated into a couple of simple cases, and the API we provide 
handles merging two or more simple cases appropriately.  And such simpler actions will be more testable and likely to be tested thoroughly.

> No one would use your "indirect" actions in the way you suggest, because file selections do not provide a Project
> directly. 

Just write one action sensitive to Project, and one action sensitive to DataObject, which overrides enabled() and uses FileOwnerQuery to determine the 
return value, and fetches the Project the DataObject in actionPerformed().

(If one were lookup-happy, it would be fairly trivial to provide a Lookup which listens for changes in the selected DataObject and provides a Lookup that (if 
something is listening to it), sticks the result of FileOwnerQuery into it on file changes. That's kind of silly for anyone to do in their own code, but it 
wouldn't necessarily be bad for the Project UI API to provide such a global lookup - the actions people write to handle this case would end up much 
simpler.

> Even besides that, the suggested use of indirect actions is unlikely because you would not want to look for a
> Node in the selection, you would directly look for a Project. (TopComponent's, hence Utilities.actionsGlobalContext,
> automatically "splice" the selected Node[]s lookups into the main lookup, as well as including those Node[]s themselves.)

In that case.  But say you want to write an OpenAction or similar and put it in a toolbar.  Utilities.actionsGlobalContext() will just give you a pile of 
OpenCookies.  If you want the action to be disabled when one node is selected which contains no OpenCookie, you cannot use 
Utilities.actionsGlobalContext() directly;  you actually need the lookup of the Node.  IndirectAction or LookupProviderAction give you this for free if you 
pass true for the boolean constructor argument (i.e. you indicate that all lookups must contain the trigger type for the action to be enabled).

> [JG02] Any support for setting the action label according to the selection? This is fairly common.

Probably need a
protected void contentChanged(Collection<? extends T> content)
method to intercept non-enablement-changing changes in the lookup result and call setValue(NAME, ...) if necessary.

> [JG03] One thing I think is very important is ability to hide an action, not just grey it out, when disabled. Currently
> this requires an elaborate trick: a phony Action which is a ContextAwareAction, creating a phony Action which is a
> Presenter.Menu (or Presenter.Popup), creating a phony JMenu which is a DynamicMenuContent, which finally creates the
> actual JMenu[] you want to display (which you then have to somehow bind to one of the other actions so as get a label
> and a event handler). There are a lot of long classes to do this nonsense in our codebase. It would be great to simplify
> them with a factory.

Plenty of ways to express this in an action putValue ("invisibleWhenDisabled") or a constructor parameter or similar. Or "implements 
Presenter.InvisibleWhenDisabled".  Also would be easy enough to provide a factory that wraps the action in an implementation of DynamicMenuContent 
that does this + getter.  I wasn't planning to address presentation with this API (i.e. just address writing actions, not write a thing that creates UI 
components for them.  Certainly it could be done more cleanly than it is done now.

> [JG07] NodeContextAction Javadoc says you can override isEnabled, but it is final in ContextAction.
Will fix.  Not sure if NodeContextAction is necessary or not;  depends how many NodeActions that are not in our APIs are out there.  It would make it easier 
to migrate;  the question is how many things out there need migrating.

> [JG10] Node/CookieAction took some care to suspend listening to changes in the global selection while they were not
> displayed - i.e. if removeNotify was called, meaning there were no active listeners on them. This would be true for a
> menu item (with no visible toolbar button): there is no reason to update its enablement state just because the user is
> moving around in the Projects tab, unless and until the user actually posts that menu. Do you plan a similar optimization?

It's already there.  See ContextAction.addNotify().  For indirect actions and so forth, this cascades through the delegate actions, and I've got tests that it 
works correctly.

> [JG11] hashCode oddly computes the constant 47 * 7, then adds something to it. 
Auto generated crap.  Already removed.

> [JG12] Why does SurviveSelectionChange extend Single? This seems orthogonal. In fact I think it should be a constructor
> parameter of ContextAction.
Done.

> [JG12] All these classes should be top-level, in a proper package. Probably in a new API module.
I could put something in contrib, or someplace else.  Any preferences?  Guessing this should be o.n.api.actions

> [JG13] CookieAction correctly handled the case that a selected node dynamically added or removed a cookie. In practice
>this was unusual, mainly applicable to SaveCookie (which I want deprecated anyway - filed elsewhere). 

Where and why?

The only alternative that comes to mind is an equivalent which you for changes on.  That forces actions that depend on it to listen on the SaveCookie for 
changes, which means people are more likely to get that wrong than just writing a ContextAction that automatically handles the enablement logic for free.

> Does LookupProviderAction handle this?
Yes.  On addNotify, an action (and any delegates) start listening to the context they're rooted on.  They will fire changes as needed.  If enabled() is not 
overridden, then the presence/absence (and count, if overridden to be something other than > 1) determines enablement.  If enabled() is overridden it can 
do more complex things, although I don't expect this to be commonly necessary.  For any depth of delegation for IndirectAction, starting/stopping 
listening will cascade through the chain of actions.

> [JG14] It is confusing that LookupProviderAction has enabled(Collection<? extends T>) as well as isEnabled(Collection<?
> extends R>) as well as isEnabled()!

Suggestions welcome.
public void actionPerformed (Collection<? extends T>()
has the same erasure as 
public void actionPerformed (Collection<? extends R>()
so the result is uncompilable.  Same for isEnabled().

I hate all of the odd method names in our existing API (isEnabled(), enabled(), actionPerformed(), performAction(), perform()).  The only way I can think to 
handle it is probably uglier, i.e.
/* non public */ class Base<T> extends AbstractAction {
   public void _actionPerformed(Collection<T>) {}
}
and each subclass overrides actionPerformed to delegate to an actionPerformed() with appropriately parameterized().  But then we're exporting a non-
public superclass (or a useless public one), which seems more ugly.  If there's a way I'm not thinking of, let me know.

BTW, I'm aware that there are cases where it would be better to use lookup items, rather than the result of allInstances().  I'm assuming if this becomes a real 
performance issue, it's easy enough to write a wrapper collection that translates them on the fly.

when it keeps anyway -> when it keeps any :-)

Draft version now in Hg with friend module in mobility. spi.actions (I would have put it in o.n.m.spi.actions but
apparently the build is set up to fail if the folder name is not spi.actions if that's the module name - weird).

http://hg.netbeans.org/main/rev/722d860e854a

Merged actions is working.

For main and contrib modules, the dir name must be derived in a fixed manner from the module's CNB. Unfortunately this
transformation is not fully reversible, due to pressure to keep dir names short. I think you meant to use
org.netbeans.spi.actions as the CNB, not org.netbeans.modules.spi.actions.

BTW you don't need a test dep on openide.util if you already have a regular module dep on it.

Created attachment 74147 [details]
Demo module - creates an action equivalent to a project sensitive action (sensitive to main project, project in node lookup or project owning selected file)

Integrated into 'main-golden', will be available in build *200811260201* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/722d860e854a
User: tboudreau@netbeans.org
Log: #153442 - initial draft of context actions SPI

> Integrated into 'main-golden', will be available in build *200811260201*

Well, not really - I didn't add it to the module lists in nbbuild yet, since I haven't yet integrated anything that
needs to use it.

In builds as of 5e5a20ffb742, but requires client modules to be friend or impl dependency.

The foremost open code question at this point is if/whether/how to implement Presenter.* on ActionStub.  In particular,
we ought to think about how to make certain common cases simpler - for example, making an invisible-when-disabled action
currently requires implementing DynamicMenuContent.  It should be as simple as putValue ("invisibleWhenDisabled", true).

I had an epiphany about this project last night:  With Jarda's work on declarative actions (very little overlap with
this project, really), the need to implement Presenter.* or DynamicMenuContent eliminated.  If you are:
 - Implementing a submenu and need Presenter for that, your action contains no real logic anyway, so you probably are
not using this library, just a regular AbstractAction
 - Implementing a main menu or toolbar item, you will probably use Jarda's declarative stuff
 - Need a custom toolbar presenter, you are probably not really writing an action anyway, just using an action to
piggyback some component into the toolbar
 - Need menu items, you can just write multiple actions

Jarda's declarative stuff fairly handles the case of main menu actions.  However, the things this library addresses are
fairly orthagonal to that (although they could be used for it if you want): 
 - if you want to write actions that appear on a Node's popup menu or in a TopComponent that are context-sensitive and
do not want to write them as inner classes, or allow pluggability of them
 - The drill-through lookups scenario - make an action that is sensitive to Y's in the Lookup of X's in the global
selection context
 - Ease of writing both single-object and multiple-object sensitive actions

With the requirement of handling presenters removed, it appears this API has passed the fast-track-review process as
described at http://openide.netbeans.org/tutorial/review-steps.html 7 months ago.  The code has been in use in Mobility
since December w/ no problems;  and the core of it in Imagine for years before that - it is stable.  Therefore, in 24
hours, I will remove the friend restriction on spi.actions.

I'd like real review in the new context of work on 
http://deadlock.netbeans.org/hudson/job/trunk/6011/testReport/org.netbeans.modules.ide.ergonomics/AvailableJ2EEServerCheck/testGetAllJ2eeServersErgo/

I'd like real review in the new context of work on 
http://wiki.netbeans.org/DeclarativeActionRegistration
I am not convinced the overlap is marginal.

Disagree.  There are a large number of concerns that http://wiki.netbeans.org/DeclarativeActionRegistration does not
cover and that spi.actions already does.  The most important is API usability.  Complicated annotations are fine for
generated code, but for people who want to just write straightforward Java code, we need to offer them a way to do that.  

The work on spi.actions is complete and solves the usability problem;  you are offering a proposal that does solve
performance problem but does not address API usability, and which is a proposal, not a finished piece of work that has
already been in production code for a release.  spi.actions is stable and well tested code.

Declarative actions are good for static menu items and toolbar items;  if you are writing an Action that should be
sensitive to some object in the lookup of a Node, TopComponent, etc., it should not require magic annotations or
lookups, just straightforward Java code.  If I have both a TopComponent and a Node that should contain the same action,
I should not have to write two inner action classes to solve that problem;  and the amount of code required to properly
implement ContextAwareAction is too heavyweight (as evidenced by the fact that the apisupport module still gives you a
CookieAction).

If you want to block an existing, tested, working library from becoming generally available to platform users, please
specify precisely the areas in which you think there really is overlap, why overlap is a problem, and what is gained by
not making such functionality available to platform users now rather than later.

Huh?

http://deadlock.netbeans.org/hudson/job/trunk/6011/testReport/org.netbeans.modules.ide.ergonomics/AvailableJ2EEServerCheck/testGetAllJ2eeServersErgo/

This is some kind of glassfish-related failure and has nothing to do with anybody's actions APIs.  ??

Tim, I'd be more than happy to let you provide better infrastructure for Actions in 6.8 timeframe. However the current 
state of spi.actions is not good enough. It does not solve any of performance problems we are facing, it does not 
include updates to apisupport wizards and thus it is not ready for integration.

If you believe you can update your API to all requirements that will be gathered during initial inception review, and 
you have permission to implement them in 6.8 release, then start the review process.

I'd be more than happy to give up on (already approved) issue 166658, in case I gain confidence that your work will 
meet performance requirements (or any other brought up during the inception review) and that you will finish the work 
for 6.8. Until that confidence is gained I'll have to proceed with my work on issue 166658 however.

This API targets usability - ease of creating advanced action implementations. It is not an attempt to implement lazy
class loading, which is valuable for some but not all action implementations: typically those placed in prominent parts
of the global UI, which must be registered declaratively (for example in the style suggested in
DeclarativeActionRegistration), and probably cannot afford to have nontrivial display or enablement semantics.

In this sense the issues do not overlap, though one could certainly imagine in the future the lazy registrations for
context-sensitive actions being extended to work well with the Java API proposed here. For example, the lazy
registration could specify a static icon and display name, and use a limited and simplified version of enablement logic
("enable iff >=1 node with o.n.api.db.DatabaseTable is selected"); if and when first selected, the real delegate would
be loaded and be able to use the richer API to control all aspects of display and enablement henceforth.

Probably all that needs to be done in issue #166658 for this to work is to ensure that proxy actions delegate all
methods (not just actionPerformed) to the real action after it is loaded - which Actions.alwaysEnabled already does.
(One minor complication with context-sensitive actions is that the real action might consider the context passed the
_first_ time it is selected in a VM session to be inappropriate, due to a more refined enablement check. This could be
handled by the real action directly - beep, or show an error dialog - or the proxy could do this automatically.)

All this said, API_REVIEW seems more appropriate for a fairly fundamental API, especially one which is not trivial.
There should be broader usage in widely divergent modules by different authors, and development-time support.

Integrated into 'main-golden', will be available in build *200907011400* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/63118025ed38
User: Jaroslav Tulach <jtulach@netbeans.org>
Log: Sort of addresses #153442 - adding migration guide for users of old actions to switch to new Actions.* factories

> Sort of addresses #153442

"Sort of" being the point.  There is a need for a clear, simple API for writing context-aware actions using plain Java code.  

Anybody writing multiple Nodes or TopComponents that may contain an object of a type actions should be registered against needs a way to write that action 
once, simply and clearly in plain Java code.  

In that case, there is no startup overhead, because we are not talking about global action registration;  and in fact, it is more processing work (i.e. slower) to 
resolve a declaratively registered action to populate a popup menu than to run a simple constructor for say, new Single<Foo>() - for this use case, declarative 
actions are both more confusing to code and accomplish the opposite of their goal, which is to improve performance.

I think I agree. Just keep in mind these things:

1. Actions as you describe them prevent users to assign them some shortcut. I consider this a major flaw. Every 
application I use (e.g. KDE apps) define pool of actions and allows me to assign to shortcut to every action provided 
by the application.

2. There is an API for non-declarative creation of Actions.context and Actions.callback (this is the "sort of"). 
Through I am sure there can be extensions and improvements to functionality provided by this API

3. Actions as you describe them have huge troubles with multi selection. This may not be problem all the time (e.g. 
Single<T> is fine), but often is and without some global identity it is unavoidable.

> 3. Actions as you describe them have huge troubles with multi selection. 
> This may not be problem all the time (e.g. Single<T> is fine), but often is and without some global identity it is unavoidable.

Could you explain what you mean by this?  ContextAction<T> with a collection should work fine, without any need for it to be a singleton - if the system is 
handling ContextAwareAction correctly, everything should work.  So I think you must mean something else, but I don't know what.

Re shortcuts, primarily we're talking about actions that operate on some object but which are local to a Node or TopComponent.  Presumably if someone 
wants a keybinding local to their TC, they can do it in the normal way with InputMap/ActionMap for a TC, and create it on the fly for their Nodes (we don't 
really solve the problem of Node-specific keybindings for explorer views today anyway - although it could be useful to solve it), and all should be well.

I think jtulach's #3 referred to the fact that when NodeOp creates a context menu from a multiselection, it displays
only those Action's which are present on all nodes (using equals() but it is recommended for them to be ==). This is not
an issue with globally registered actions, since you create a single global Action instance which different nodes refer
to, so merged context menus work as you might expect.

Still pushing forward? Or dormant?