Let's keep things simple and add org.openide.awt.ActionReference annotation.

Need to see a proposed patch incl. nontrivial uses to really evaluate, especially with subtle parts like name() and id(). Some of the issues brought up in http://wiki.netbeans.org/DeclarativeRegistrationUsingAnnotations#Problems_with_actions are handled by a separate @ActionID & @ActionRegistration and use of package annotations.


[JG01] While a generic @ActionReference is useful for extensibility to unforeseen domains such as the Projects tab context menu, the current proposal forces people to remember, and type, magic strings like "Toolbars" (capitalized differently from "JToolBar" by the way!). If you are opposed to having @ActionMenuReference etc. then at least introduce some constants into ActionReference, e.g.:

/** Main menu bar. Path will be a menu code name, possibly with submenus. */
String MENU = "Menu/";
/** Main tool bar set. Path will be a toolbar code name. */
String TOOLBARS = "Toolbars/";

so you can write e.g.

@ActionReference(path=ActionReference.TOOLBARS + "Edit", position=123)

I would suggest a special annotation for Shortcuts, e.g.

import static java.awt.event.KeyEvent.*;
import static ActionShortcutReference.Stroke.*;
import static ActionShortcutReference.Modifiers.*;
@ActionShortcutReference({@Stroke(key=VK_X, modifiers=CTRL), @Stroke(key=VK_WINDOWS)})

since the syntax is rather complex and it is easy to make typos. But if generic @ActionReference is to be used for this as well then there must be a SHORTCUTS constant whose Javadoc carefully describes the required name() syntax, {@link}ing to appropriate specifications such as KeyEvent and Utilities.stringToKeys.

The processor for a generic annotation could also hardcode syntax checks for certain well-known locations. For example, require and try to parse name() if in Shortcuts; or verify that paths under Toolbars specify no subfolder; or require a position attribute for menu items. This is not as nice as a custom annotation but better than nothing.


[JG02] Would be useful to have position-valued attributes separatorBefore() and separatorAfter(), which would require position(). These are very commonly needed for menu registrations (also possible for toolbar registrations), and specifying separators in a layer is cumbersome.

Builder is on:
http://deadlock.netbeans.org/hudson/job/prototypes-action-reference-189558/
First set of changes too:
http://hg.netbeans.org/core-main/rev/75303c50e3b0



Re. JG01 The problem without multiple difference annotations is that I don't see easy way to incorporate them into @ActionReferences(value={}). I understand that it is necessary to provide some guidance when typing the path. Rather than the tricks mentioned in JG01, I'd like to use getCompletions and provide either hardcoded or real hints.

(In reply to comment #3)
> The problem without multiple difference annotations is that I don't
> see easy way to incorporate them into @ActionReferences(value={}).

You wouldn't need to; you would have multiple top-level annotations. You only need @Somethings if you would otherwise need to have more than one @Something. (Would be nicer if JSR 175 were amended so you could repeat a given annotation, if the annotation were meta-annotated to allow this.)

> use getCompletions and provide either hardcoded or real hints

Better than nothing.

BTW String.isEmpty is easier to read than .equals("").

Is there no ActionReference.name() as in wiki, needed for shortcuts? This is why I wanted to see some representative uses in various modules.

ElementType.PACKAGE was forgotten in @Target on @ActionReference, and the processor fails to handle this case.

Delete empty @return.

Next round. I am dealing with the wizards. I have problems with properly positioning the action. I've added toInteger() method but I am not sure how to implement it:
http://hg.netbeans.org/core-main/rev/cd69522e018c

Separators are also not handled. Simplest thing is to leave them in XML file. Still todo.

Re. "ElementType.PACKAGE was forgotten". It does not make much sense to put just one @ActionReference on package, usually you want to put there more. So I left out the element intentionally.

Do not use Proxy in createActionReference. This is too complicated. Simply define a small struct class with the desired getters for use from FreeMarker.

Regarding toPosition - you would I think need to duplicate code from CreatedModifiedFiles.orderLayerEntry. Needs to find the surrounding FileObject's and pick a position between them.

Ability to add just a single @ActionReference on a package would be consistent with other package annotations which permit both the single and multiple forms. Not a major issue to omit it.

Version which imho misses only better getCompletions in the annotation processor. Shows sample usage, properly generates separators (in layer), positions the actions fine:
http://hg.netbeans.org/core-main/rev/8c42259cd002

Re. "createActionReference" - the important API is the usage from freemarker. If that is OK, I'd rather keep the Proxy. I like the symetry: one can use annotations during runtime, one can use them during compile time, so why not reference them while generating templates!?

Please notice also one additional contract between openide.awt and apisupport.project: http://hg.netbeans.org/core-main/rev/915fc0098ed5

It is not working properly yet as the IDE's code completion support needs a bit of time to settle down. Regardless of that I'd like to integrate tomorrow. We'll deal with the proper behaviour of code completion in default branch.

(In reply to comment #9)
> Please notice also one additional contract between openide.awt and
> apisupport.project: http://hg.netbeans.org/core-main/rev/915fc0098ed5

This looks like a lot more trouble than it's worth. If this level of code completion were really necessary (and I don't think it is, since new actions are usually placed by wizard) it would be better to look up the available layer positions from the classpath (probably processor path) inside ActionProcessor.

The project owner may be null, etc.

Also please do not use Thread.CCL to find a module class. It is unreliable and will not work at all in OSGI mode.

I still think specifying separators in the annotation is valuable. This is a very common requirement.

I also think that defining constants for well-known action locations, and providing warnings or errors for definite violations of semantics (such as malformed shortcut names), is a higher priority than code completion.

(In reply to comment #8)
> Re. "createActionReference" - the important API is the usage from freemarker.
> If that is OK, I'd rather keep the Proxy. I like the symetry: one can use
> annotations during runtime, one can use them during compile time, so why not
> reference them while generating templates!?

Because the code is needlessly complex and hard to track. Would be simpler and clearer to define a small struct.

As nobody questioned the fast status of the review, I am going to integrate now.

I believe the API is minimalistic, but useful as it is right now. We can certainly work on improvements (like the separator stuff; but it is not clear how to do it best. Should we mimic layer based solution or should we rather use some group="...." attribute which seems to be standard in the other IDEs? Anyway that is a matter for separate issue).

I plan to improve the live getCompletions from layers with Dušan Bálek. I believe it is going to be beneficial for the IDE support for Processor.getCompletions in general. If there are unavoidable issues with that approach, we can indeed easily turn it off.

Re. "hard to track code" - but the code is tested and I prefer consistency to easier to implement dual concepts.

Merged as core-main#d1d13a81525d

(In reply to comment #12)
> We can certainly work on improvements (like the separator stuff

Yes, it can be compatibly added. My concern is about modules which migrate to the annotation before then, and newly added actions using wizard; it may be harder to retroactively deal with their separators. This matters mainly if we release the current annotation in 6.10.

> Should we mimic layer based solution or should we rather use
> some group="...." attribute which seems to be standard in the other IDEs?

I thought about group="..." as well. But I cannot think of a way to do this at all compatibly.

(In reply to comment #13)
> it may be
> harder to retroactively deal with their separators. This matters mainly if we
> release the current annotation in 6.10.

I reported it as bug 189848