This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 183794 - Annotations to register action implementations
Summary: Annotations to register action implementations
Status: RESOLVED FIXED
Alias: None
Product: platform
Classification: Unclassified
Component: Actions (show other bugs)
Version: 6.x
Hardware: All All
: P1 normal (vote)
Assignee: Jaroslav Tulach
URL:
Keywords: API_REVIEW_FAST
Depends on: 191236 191407 205798 206093
Blocks: 166658
  Show dependency tree
 
Reported: 2010-04-09 16:56 UTC by Jesse Glick
Modified: 2011-12-07 15:31 UTC (History)
3 users (show)

See Also:
Issue Type: ENHANCEMENT
Exception Reporter:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jesse Glick 2010-04-09 16:56:48 UTC
Bug #166658 implemented declarative context-sensitive actions but creation of corresponding annotations was deferred. Work such as declarative project context menus has made this increasingly urgent.

Registering the action itself (Actions/*/*.instance) should be relatively straightforward; either @AlwaysEnabled(displayName="#action"), or @ContextAware(type=Whatever.class, displayName="#action"), etc.

The problematic part is registering shadows. For some actions, it makes sense to register all the shadows at the same time (in the same module) you register the action itself, in which case additional annotations could easily be added. Unfortunately this is not the case for many actions; very commonly they are referred to from downstream modules, meaning annotations on the Java impl class are not a possibility for registering the references. (Annotations on package-info.java in the downstream module are possible, or continued use of XML layer registrations.)

Probably the best policy is to require an explicit String id attribute on any action registration, which would correspond to the basename of the instance file, perhaps after replacing '.' by '-'. (Most other registration annotations will generate an ID based on the defining element.) This could be made to match an existing legacy registration if needed. If annotations are introduced for references, then Java constants could be used to export the API (which has the nice effect of making Find Usages and so on work naturally); even if (hand-written) XML layers are still used for the references, exporting such IDs in API classes would be a good practice as it commits you to particular stable names that can be tracked in sigtest etc.

Using this issue to track only the action registration. Shadow registrations require more study, but I believe they can be added in a separate step so long as IDs must be explicitly specified. Even if and when registered using an annotation on the impl class, there is no need for that to be part of the action registration annotation; can be a parallel annotation which would pick up the same ID.
Comment 1 Jaroslav Tulach 2010-04-09 18:09:39 UTC
Jesse, you are a joker, aren't you? You blocked my work done on action registration because it missed handling of shadows. Now (when you feel your own pain) you are claiming that it is not important to handle shadows!

In such case, let's just backout the backout:
http://hg.netbeans.org/main-golden/rev/17122defcfe1
Comment 2 Jesse Glick 2010-04-09 19:02:32 UTC
(In reply to comment #1)
> you are claiming that it is not important to handle shadows

No, it is certainly important, but I did not yet see a satisfactory proposal (such as @ActionReference) for handling shadows, and I believe this can be added separately so long as IDs are mandatory.

> let's just backout the backout:
> http://hg.netbeans.org/main-golden/rev/17122defcfe1

Something along those lines, but with different syntax:

1. Separate annotations for distinct kinds of actions.

2. Mandatory ID.
Comment 3 Jaroslav Tulach 2010-04-09 22:35:28 UTC
Re. #1 - I don't see a reason to separate annotation for callback and context sensitive. If there is some other kind you want to separate, please specify.

Re. #2 - the ID is inherently present in each registration. It is traditionally deduced from the classname (+group). It you require id attribute, then for most of existing actions people would write @ActionRegistration(id="org-openide-actions-CopyAction") class CopyAction { } which unnecessarily duplicates the ID info and makes it subject to typos errors.
Comment 4 Jaroslav Tulach 2010-07-26 12:02:59 UTC
OK, I'd like to have this done for 6.10. I've just modified the code examples at
http://wiki.netbeans.org/DeclarativeActionRegistration#Declare_context_action_available_for_singletons
to require new @ActionId annotation. It does not infer any names, it requires category and id to be manually provided. Being an annotation it shall allow us to extend it in future, if necessary.

I expect to write the annotation processor for @ActionRegistration and @ActionId, change the wizards to use that and modify few existing actions (not convert all of them).
Comment 5 Jesse Glick 2010-07-26 15:10:46 UTC
[JG01] I still think separate annotations for different action factories (alwaysEnabled, callback, etc.) is preferable. Keeps the syntax cleaner - you can see immediately which properties are mandatory for each kind of action factory - and is more amenable to future additions to the list of standard factories (possibly from other modules).

For example, a single fixed displayName attribute makes sense for alwaysEnabled, but may not make sense for a project-sensitive action that has multiple display variants.

As another example, key="..." is needed for callback actions whereas type="..." is needed for context-sensitive actions. But you cannot syntactically state that just one should be present (only as an error in the AP), whereas this would be natural if there were separate @CallbackAction and @ContextSensitiveAction annotations.


[JG02a] @ActionId (@ActionID?) seems clumsy (we try to avoid nested annotations) compared to simply having two attributes, for category and name - or just one, for a slash-separated path. What sorts of extension to the action ID do you envision? We have had essentially the same system since NB 3.x as far as I can recall.

[JG02b] Alternately, make @ActionID be a top-level annotation with no processor. (*) As per JG02a, this could have separate category and id attributes, or a single path attribute. This would have two advantages: you would not need nested annotations (except for @ActionReference in package-info.java); and it could be read directly by an @ActionReference processor, especially useful if the registration annotation comes from a "higher" module such as projectuiapi that @ActionReference's processor has no knowledge of.

(*) Failure to specify @ActionID when some kind of @ActionRegistration is specified could be treated by the @ActionRegistration processor as an error, or a warning (default value used), or a warning if 'id' is missing so long as 'category' is specified, etc. The behavior can be a matter of policy; compared to e.g. Options dialog categories, actions are more likely to be used as implicit APIs (by referring to them from other modules), so people should be encouraged to define an ID stable against code refactorings, ideally published as a Java constant with Javadoc and @since. There could also be a static utility method (in Actions?) to get a SFS path "Actions/*/*.instance" for a given Element that may or may not have an @ActionID; this could be used by processors for @ActionRegistration variants, and processors for @ActionReference or more specific variants.
Comment 6 Jaroslav Tulach 2010-07-27 04:19:29 UTC
JG02b was my plan as well. Top level @ActionID is better. But there is "Declare callback action without default" case and it seemed to require the id() attribute. However I think I found reasonable way to eliminate this need. I'll update the wiki page. Let ActionID be top level annotation.

Re. JG01. In general it is fine to have multiple annotations to register actions. However for existing types (always, context and callback) the single annotation is enough. For example note that there is no type attribute anywhere. It is deduced from the signature of constructor or factory method. That makes difference between always and context. Plus each of these actions can be callback with fallback. Single @ActionRegistration annotation is enough.

You mentioned project-sensitive actions. They cannot be expressed in current declarative system. Project actions would probably require their own annotation and processor. The interpretation of the annotation would be completely independent, the shared point of cooperation would be mutual understanding of the @ActionID annotation (which would then allow @ActionReference to place such actions into Menu, etc.).
Comment 7 Jesse Glick 2010-07-28 14:25:32 UTC
JG01 - in case there is a simple system for distinguishing these varieties of actions, and there is overlap in their semantics, then a single annotation for them makes sense. I just do not want to overload an annotation at the expense of clarity.


(In reply to comment #6)
> project-sensitive actions [...] cannot be expressed in current
> declarative system [and] would probably require their own annotation
> and processor. The interpretation of the annotation would be completely
> independent, the shared point of cooperation would be mutual understanding of
> the @ActionID annotation

Yes, that was my intent.


[JG03] String findSystemFileSystemPath(ActionId id) will not work for elements without an @ActionId, which we may wish to support for the convenience of modules which register and link to actions they have no intention of ever exposing as APIs (and which should not be offered for customizable keybindings, which also rely on stable IDs). So consider in e.g. Actions:

public static String findPath(Element annotatedElement, Messager messager);

which would produce "Actions/System/int-action.instance" for something annotated with @ActionID(category="System", id="int.action"); something like "Actions/Hidden/my-Class-method.instance" for an unannotated ExecutableElement "method" inside a TypeElement "my.Class" (perhaps after issuing a warning); etc.


[JG04] For completeness, and integration with @ActionReference or whatever, there should be some way of using an annotation to register a traditional eagerly loaded Action, i.e. those which either use a custom Presenter.*, or which must have nontrivial enablement/display logic before first selection. While we do not wish to encourage these generally, sometimes they are appropriate (e.g. there are many in projectui), and manual layer registration is error-prone.

The currently proposed @ActionRegistration could not reliably detect these cases just by being applied to an Action (rather than an ActionListener) since it is permitted to use alwaysEnabled etc. on a full Action, with the lazy delegate semantics. Perhaps there could be a no-arg @EagerActionRegistration (accompanied as usual by @ActionID), applicable to any class/method/field assignable to Action. Its name should suffice to flag to developers that lazy loading is blocked, and its Javadoc can explain how it can be avoided in most cases.

Such an annotation could also be useful for actions used only in certain context menus. While context(...) will suffice for simple cases, some actions will want to perform more detailed enablement/visibility checks (inspecting their Lookup context for specific conditions); and lazy loading may be less important, say if the context menu is already defined only on *.php nodes or freeform project nodes etc.
Comment 8 Jesse Glick 2010-07-28 14:26:30 UTC
BTW I assume you will attach a proposed patch before concluding the review, as there might be details of specification or behavior that are not apparent from the examples in the wiki.
Comment 9 Jaroslav Tulach 2010-08-02 16:24:12 UTC
I've created action-registration-183794 branch in core-main repository and set a builder up:
http://deadlock.netbeans.org/hudson/job/prototypes-action-registration-183794/
Comment 10 Jaroslav Tulach 2010-08-03 16:47:16 UTC
First diff for review: http://hg.netbeans.org/core-main/rev/28babb2cd493
Comment 11 Jesse Glick 2010-08-03 17:18:20 UTC
JG04 still open. Could be handled in a separate issue however.


[JG05] Make Actions.* factory methods link to the new registration annotations instead of giving XML layer fragments.

(Noticed because of <description> in apichanges.xml, which is the wrong place to put primary documentation.)


[JG06] Minor, but for comfort of Find Usages etc. I recommend that @SupportedAnnotationTypes not be used; instead override getSupportedAnnotationTypes to use Anno.class.getCanonicalName(). (RFE filed for 269 to permit a Class[] rather than String[] for @SAT; also see #5f82ae35e743.)


[JG07] Javadoc for ActionID fails to mention that '.' -> '-' in id() when creating file path.


[JG08] Include the Element param to the LayerGenerationException ctor whenever possible. Otherwise error stripes in the editor cannot work correctly. (Missed on first usage.)


[JG09] Why is it required that an annotated String constant be public?


Test looks good. Might be useful to check AP error messages, especially on the tricky stuff - detection of the context-sensitive ctor.
Comment 12 Jaroslav Tulach 2010-08-05 09:10:44 UTC
New version available as
http://hg.netbeans.org/core-main/rev/334851a988fd

> JG04 still open. Could be handled in a separate issue however.

Fixed. If you implement Presenter.XYZ, your action will be registered directly. I am not advertizing this functionality in documentation however.

JG05, JG07, JG08 and JG09 fixed.
JG06 fixed in 744750f041a5

> Might be useful to check AP error messages
I've added some more, but there are still cases not covered yet.



If the API is found to be in acceptable state, I'll change the wizards and use the annotation in few cases then. I plan to integrate on 10th August, 2010.
Comment 13 Jaroslav Tulach 2010-08-09 08:29:59 UTC
About to merge tomorrow:
http://hg.netbeans.org/core-main/rev/3e0a590ff375
Comment 14 Jesse Glick 2010-08-09 21:06:19 UTC
Fix for JG04 is clever if a little obscure. There are cases where one might want to eagerly adjust display name etc. by setting Action attributes where Presenter.Menu is otherwise unnecessary, though implementing this interface is little additional work and makes it clear that you are doing special things with display.

One problem however: [JG10] the displayName attribute is unnecessary (and should be marked as an error) on such actions. Ditto for all other attrs of @ActionRegistration. Note how on RecentFileAction you are forced to provide a bundle key which is in fact unused.

Would be more natural if you followed JG01 and used a separate no-arg annotation. (@ActionRegistration would forbid implementations of Presenter.* to catch mistakes.) This would be a natural home for Javadoc explaining why lazy actions are to be preferred, and makes it explicit in source code that your action is eager.


[JG11] For type.getName().replace('$', '.') you might substitute type.getCanonicalName().


[JG12] "Only one public constructor allowed in " is not a complete sentence so should not be used as an error message. Drop the " in " suffix.


[JG13] dt.getTypeArguments().get(0) might throw an exception if someone uses List as a raw type.


[JG14] "@ActionID category() cannot contain /" might be too strict; at least Actions/Window/SelectDocumentNode/*.instance would be rejected. TBD whether these should remain in a subfolder, of course (see patch for bug #189320).
Comment 15 Jaroslav Tulach 2010-08-10 08:52:39 UTC
I have addressed JG11, JG12 and JG13 and merged to core-main#868cccdefce7

Making things less strict as JG14 can happen anytime later.

As the JG10 and JG04 go: I am keeping things unchanged. I don't want to advertize this functionality - e.g. creating separate annotation is not desirable. True, the displayName or iconBase are not used much, but they could be. For example the toolbar customization dialog could (and should) use them as a representation of the action in static context, before it is instantiated.

Anyway I don't see much value in massive rewrite of existing layer registrations to annotations (I was quite bored when I did it for the utilities module). The annotations shall be useful for new code and here it is good if they provide the right direction - e.g. lead towards declarative actions.
Comment 16 Jesse Glick 2010-08-10 13:46:04 UTC
(In reply to comment #15)
> the displayName or iconBase are not used much, but they could
> be. For example the toolbar customization dialog could (and should) use them as
> a representation of the action in static context, before it is instantiated.

There could be compatibility issues here, since existing actions expect that Action.NAME is used for presentation purposes. Possibly "displayName" could be read and NAME used as a fallback if it is unset.

> I don't see much value in massive rewrite of existing layer
> registrations to annotations

I do. Finding usages of classes and refactoring are more cumbersome with manual layer registrations, and I have had work on this kind of thing slowed by minor typos which are very hard to diagnose.

> I was quite bored when I did it for the utilities module.

I am playing with a refactoring hint for this. BTW lack of a separate annotation for conventional (eager) actions is making this harder since there are a lot of such actions in core.windows and projectui and they do not always implement presenter interfaces.
Comment 17 Quality Engineering 2010-08-11 03:12:36 UTC
Integrated into 'main-golden', will be available in build *201008110001* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/8dbca189641b
User: Jesse Glick <jglick@netbeans.org>
Log: #183794 follow-up: ability to create at least alwaysEnabled actions by factory method.
More complete fix would allow also context/fallback actions, but this will be more work in the absence of
a LayerGeneratingProcessor API enhancement to find and validate constructors and factory methods with various signatures.
Comment 18 Quality Engineering 2010-12-08 06:34:10 UTC
Integrated into 'main-golden', will be available in build *201012080001* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/14edb50d00fc
User: Jesse Glick <jglick@netbeans.org>
Log: @ActionRegistration should also be eager on anything assignable to DynamicMenuContent.
(See #183794 JG04. These hacks would not be necessary if JG01 were accepted and we had an @EagerActionRegistration.)
Comment 19 Jesse Glick 2011-12-07 15:16:44 UTC
(In reply to comment #7)
> public static String findPath(Element annotatedElement, Messager messager);

bug #205798
Comment 20 Jesse Glick 2011-12-07 15:31:39 UTC
(In reply to comment #15)
> As the JG10 and JG04 go: I am keeping things unchanged. I don't want to
> advertize this functionality - e.g. creating separate annotation is not
> desirable.

bug #206093