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.
I would like to continue an effort partially done under issue 150875. Especially 1) Currently the actions in BaseKit are public and non-final which allows the descendant kits to override them. Actions registered by @EditorActionRegistration typically do not provide Action.NAME property value since the annotation defines it and AlwaysEnabledAction wrapper action propagates the value into corresponding BaseAction. Unfortunatelly if someone extends such action but registers it through kit.createActions() such action will have Action.NAME property unset. Therefore I would like to make all actions final (possibly creating a new non-public action classes to keep compatibility) and create an SPI where necessary to tweak action's behavior. 2) Remove usage of MainMenuAction where possible and just use features of o.n.m.editor.AlwaysEnabledAction (possibly extending its functionality). 3) Possibly deprecate BaseKit.createActions() and just use declarative registration. Or at least resolve the problem that all actions created by createActions() (even in child kits) are overriden by any declaratively registered actions (even those in BaseKit).
Looking at the MainMenuAction's functionality: 1) MainMenuAction (boolean forceIcon, Icon forcedIcon) - in practice it' only used to hide action's icon in menu so it could be implemented by using "noIconInMenu" property. 2) Show action's shortcut in menu/popup-menu. The current approach in AlwaysEnabledAction is if (Action.ACCELERATOR_KEY.equals(name)) { Keymap map = Lookup.getDefault().lookup(Keymap.class); if (map != null) { KeyStroke[] arr = map.getKeyStrokesForAction(action); return arr.length > 0 ? arr[0] : null; } } This will not work for editor actions. BTW the Keymap does not deal with multi-key shortcuts that are necessary e.g. for emacs keyboard profile. I'm wondering what would be the best solution for this. I guess the JMenuItem.setAccelerator(action.getValue(Action.ACCELERATOR_KEY)) needs to be replaced by something suitable for multi-key bindings e.g. ACCELERATOR_KEY_LIST which will be List<KeyStroke>. I guess that editor's key bindings will not show up when consulting Lookup.getDefault().lookup(Keymap.class). Instead the editors' actions could populate the ACCELERATOR_KEY_LIST property in the action by itself. But the AlwaysEnabledAction would have to construct the delegated action once someone asks for the ACCELERATOR_KEY_LIST property. There's also a fact that the editor's actions can be mime-type based so the actual keymap could possibly not contain the right bindings at the moment. I'll talk to Yarda in person regarding this.
Facts: F1) Editor actions can be defined by a) direct creation in BaseKit.createActions(). This would become deprecated. b) Defining action in "Editors/mime-type/Actions" folder in xml layer c) Using EditorActionRegistration which in fact generates b). This is a preferred way. F2) Each editor action needs to have its Action.NAME property filled with action's non-localized name (sort of an ID unique among the particular editor kit). (non-localized name consisting of lowercase letters and hyphens - see e.g. string constants in DefaultEditorKit). This is a Swing editors' convention. F3) Global editor actions (those for an empty mime-type "") can be overriden by mime-type specific implementations by registering an action with the same "id" for target mime-type. I would like to solve the following usecases in a developer-friendly way: U1) There should be an easy way to declare action's menu representation, popup representation and editor-toolbar representation. U2) Action's representation in menu should delegate (when invoked) to action's implementation for a particular mime-type being edited. U3) Action that is overriden for a particular mime-type should have an easy way to delegate its properties (mainly action's localized name, menu text, popup text etc.) to "global" action. Otherwise values in localization bundles would need to be duplicated in target mime-type's module. Although the localizations are mainly needed for the global actions the overriden mime-type actions can be queried for display name etc. too and they should return correct values. Proposed solution: S1) Extend @EditorActionRegistration by additional attributes: String menuPath() default ""; int menuPosition() default 0; String popupPath() default ""; int popupPosition() default 0; String toolBarIcon() default ""; int toolBarPosition() default 0; Create final EditorDelegateAction that will only contain a search for the target action to which it delegates. Annotation processor would generate EditorDelegateAction instance creation into the menu folder in xml layer. The action would implement the Presenter.* interfaces. S2) Inheritance of the properties (action's display name, menu text, popup text etc.) should probably work automatically for all the actions. There is no final solution yet but there could be a special EditorWrapperAction that would delegate to the annotated (overriding) action and defaulting to the global action's property value. Or an AlwaysEnabledAction could be extended to recognize "delegateAction" property which would be an action's instance to which the AEA would delegate in case the property is null. It could be then declared with methodValue() in the layer and the method would retrieve the global kit and find the global action's instance. I would like to ask for a review first. Thanks for opinions in advance.
API_REVIEW_FAST should be reserved for fairly straightforward changes which are not likely to be controversial and for which a patch has already been provided. A couple of comments offhand: [JG01] The handling of Action.ACCELERATOR_KEY will likely change anyway as a result of issue #152576. In particular, I plan to delete the block of code you mention, or at least move it elsewhere. You are correct that the current system does not display accelerators for multikey bindings. [JG02] position() attrs in annotations should default to Integer.MAX_INT. See Javadoc for LayerBuilder.
Yes, the patch will be non-trivial, API_REVIEW needed. I'll submit the patch during the next week. ad JG01: For @EditorActionRegistration (using AlwaysEnabledAction) I have added a methodvalue for the "AcceleratorKey" property so it should be lazy and hopefully acceptable for reviewers. ad JG02: Thanks, code updated.
To JG01 - I don't think you should do that. The accelerator for an action is defined implicitly by the action also being registered under Shortcuts or Keymaps/*.
Created attachment 84832 [details] Diff of proposed changes
Created attachment 84833 [details] Layer generated from new editor.actions module
To JG01 - The editor actions do not use the Shortcuts folder mechanism like the system actions do. Instead they use .xml files containing the keybindings according to currently use keyboard profile. E.g. editor/src/org/netbeans/modules/editor/resources/NetBeans-keybindings.xml We currently do not plan to change this mechanism since it's rather mature (we can possibly reuse a shortcut for different action in a different mimetype; also we can treat platforms (e.g. Mac) specially etc.) Attaching diff with a sketch of changes: 1) @EditorActionRegistration in editor.lib2 is extended to allow menu, popup and toolbar registrations. Also using presenterType annotation's property to allow the checkbox menu item. 2) Created editor.actions module that should contain the editor actions impls (we want to deprecate editor.lib in a future and actions editor.lib2 would require compilation tweaks to use annotation in the same module) 3) Created AbstractEditorAction in editor.lib2 as a replacement for BaseAction in editor.lib. Once I rewrite majority of editor actions I would like the AbstractEditorAction to become a SPI support class. There's also EditorUtilities.getAction() class to fast-search for an action in a kit. 4) Created GotoAction (it should host goto-declaration, goto-source etc.) in editor.actions module. It's a regular action with a menu presenter. 5) Created ToggleAction (to host toggle-toolbar and toggle-line-numbers actions). They both have checkbox style menu presenter. Since there's Action.SELECTED_KEY (from JDK1.6) I use a similar mechanism to base the checkbox's state on the action's property. For this to work however the real action needs to be instantiated in order (AlwaysEnabledAction is not used in this case) to set the SELECTED_KEY value properly. For direct instantiation the AbstractEditorAction has a constructor with "Map<String,?> attrs". If the processor detects the parameter it uses direct instantiation automatically. It would be nice if not only method but also the class instantiation could check for a constructor with "Map<String,?> attrs" parameter. 6) I have modified an order of the creation (and overriding) of the editor actions: 1. Declared "global" actions (declared in the xml layer under "Editors/Actions") 2. Declared "mime-type actions (declared in the xml layer under "Editors/content-type/Actions") 3. Result of createActions() 4. Custom actions (EditorPreferencesKeys.CUSTOM_ACTION_LIST)
Y01 I'd like to prevent direct instantiation of any implementation class. Do I understand correctly that right now only "CheckBox" presenter type is instantiated directly? Y02 I wish the presentation logic was in one common place - e.g. along AlwaysEnabledAction. I'd like to extend it to support "CheckBox" style. Here is my reasoning: Checkbox works with true/false state. NetBeans use Preferences (mostly NbPreferences) to store trivial settings values. I'd like to have org.openide.awt.Actions.checkbox(Preferences p, String key, boolean defValue, Action delegate) factory method and appropriate XML declaration. The preferences could be specified as stringvalue (could be prefixed as "global#", "user#" or "nb#" or nothing defaulting to NbPreferences) or as methodvalue (to allow editor infrastructure to provide Preferences from MimeLookup.EMPTY). The presenter would display the checkbox according to the value of p.get(key, defValue). When invoked, it would revert the value and call delegate.actionPerformed(...). Such infrastructure would be useful not only for editor, but for everyone. Y03 editor.actions module depends on editor.lib. You are known to wish to deprecated it in future. In such it would be better to remove dependency of this new module on it since beginning.
ad Y01: yes, other editor actions will still use declarative registration (in the almost all should). ad Y02: agreed, although it's sort of an implementation detail of the annotation's "CheckBox" style so it could IMHO be done afterwards. ad Y03: editor.lib dependency is only temporary until the appropriate SPIs for actions get created. Then it will be removed.
ad Y02: I've reimplemented the checkbox presentation by using the AlwaysEnabledAction and added a test for the new behavior. Jardo, please review it and I would then like to integrate finally. Thanks.
Created attachment 86576 [details] Patch with AEA.CheckBox
Y11 EditorActionNames shall have private constructor Y12 "if (Action.ACCELERATOR_KEY.equals(name))" and next ~10 lines is back, although Jesse removed that code. Maybe a bug in the merge, or do you need that code? Y13 Implementation only typo: togglePreferncesSelected() I could comment mostly just on the openide.awt part of the patch. I guess the checkbox actions is a good improvement that Geertjan can loudly announce on his blog.
Y11 - done. Y12 - code removed. Y13 - typo fixed. Thanks for review. I will possibly integrate tomorrow.
http://hg.netbeans.org/jet-main/rev/52f4cc2e85f1
Fixed failing test http://hg.netbeans.org/jet-main/rev/ceae35774591
Integrated into 'main-golden', will be available in build *200909071104* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress) Changeset: http://hg.netbeans.org/main-golden/rev/52f4cc2e85f1 User: Miloslav Metelka <mmetelka@netbeans.org> Log: #166027 - Fix and clean up editor actions.
[JG03] Not sure I understand AlwaysEnabledAction.CheckBox and why it uses a delegate. Suggest instead having a separate factory method Actions.checkbox which takes a mandatory prefs key and no ActionListener-valued delegate attr. After all, why would you need an actionPerformed in this case? Toggling the key _is_ the action. (Any code which needs to respond immediately other than the JCheckBox should simply be listening to the prefs node to begin with.) This new functionality is anyway not documented at all in Javadoc. (apichanges.xml is not an appropriate place to document details - this file should only mention the _existence_ of a new feature. BTW your apichanges entry is full of mistakes, pls. edit.) Also, PreferencesKey and PreferencesNode would conventionally be spelled preferencesKey and preferencesNode; and syntax of preferencesNode seems too complex - just make it be the node path, and if necessary (probably isn't) have a separate attr for preferencesType (default 'nb').
I think that: 1. preferencesNode without any prefix could default to nb. With prefix it shall work as it does now. 2. having documentation in javadoc is better 3. (-0.1 -+ 0.2) for new method
I have added Actions.checkbox as Jesse suggested (btw. checkbox or checkBox since there's java.awt.checkbox and javax.swing.JCheckBox)? Javadoc added. Implemented Yarda's 1. so "nb:" no longer used. Originally I used "PreferencesKey" etc. since the produced action is a Swing Action and convention is to capitalize all action's props - see javax.swing.Action e.g. "Name" etc. But I understand that existing props in Actions.alwaysEnabledAction() do not follow that convention so I now use "preferenceKey" etc. Please check the attached diff. Thanks.
Created attachment 89210 [details] Actions.checkbox() patch
If nobody objects I will integrate tomorrow.
http://hg.netbeans.org/jet-main/rev/00b039bf3b37