Bug 166027 - Fix and clean up editor actions
Fix and clean up editor actions
Product: editor
Classification: Unclassified
Component: Actions/Menu/Toolbar
PC All
: P1 (vote)
: 6.x
Assigned To: Miloslav Metelka
Depends on:
  Show dependency treegraph
Reported: 2009-05-26 12:56 UTC by Miloslav Metelka
Modified: 2009-10-14 17:06 UTC (History)
4 users (show)

See Also:
Issue Type: TASK

Diff of proposed changes (145.22 KB, patch)
2009-07-16 14:24 UTC, Miloslav Metelka
Details | Diff
Layer generated from new editor.actions module (4.72 KB, text/xml)
2009-07-16 14:29 UTC, Miloslav Metelka
Patch with AEA.CheckBox (172.51 KB, patch)
2009-08-24 16:46 UTC, Miloslav Metelka
Details | Diff
Actions.checkbox() patch (16.99 KB, patch)
2009-10-09 16:57 UTC, Miloslav Metelka
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Miloslav Metelka 2009-05-26 12:56:47 UTC
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).
Comment 1 Miloslav Metelka 2009-06-11 17:17:28 UTC
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
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.
Comment 2 Miloslav Metelka 2009-06-23 17:05:23 UTC
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.
Comment 3 Jesse Glick 2009-07-09 00:26:47 UTC
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.
Comment 4 Miloslav Metelka 2009-07-09 16:01:18 UTC
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.
Comment 5 Jesse Glick 2009-07-09 16:29:57 UTC
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/*.
Comment 6 Miloslav Metelka 2009-07-16 14:24:38 UTC
Created attachment 84832 [details]
Diff of proposed changes
Comment 7 Miloslav Metelka 2009-07-16 14:29:16 UTC
Created attachment 84833 [details]
Layer generated from new editor.actions module
Comment 8 Miloslav Metelka 2009-07-16 14:30:18 UTC
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)
Comment 9 Jaroslav Tulach 2009-07-22 07:07:03 UTC
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.

Comment 10 Miloslav Metelka 2009-08-11 12:41:38 UTC
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.
Comment 11 Miloslav Metelka 2009-08-24 16:45:16 UTC
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.
Comment 12 Miloslav Metelka 2009-08-24 16:46:15 UTC
Created attachment 86576 [details]
Patch with AEA.CheckBox
Comment 13 Jaroslav Tulach 2009-08-25 10:20:15 UTC
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.
Comment 14 Miloslav Metelka 2009-08-26 15:18:47 UTC
Y11 - done.
Y12 - code removed.
Y13 - typo fixed.
Thanks for review. I will possibly integrate tomorrow.
Comment 15 Miloslav Metelka 2009-09-04 10:34:14 UTC
Comment 16 Miloslav Metelka 2009-09-04 16:06:48 UTC
Fixed failing test
Comment 17 Quality Engineering 2009-09-07 17:06:00 UTC
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.
Comment 18 Jesse Glick 2009-09-09 23:47:53 UTC
[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').
Comment 19 Jaroslav Tulach 2009-09-15 10:39:31 UTC
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
Comment 20 Miloslav Metelka 2009-10-09 16:56:26 UTC
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.
Comment 21 Miloslav Metelka 2009-10-09 16:57:22 UTC
Created attachment 89210 [details]
Actions.checkbox() patch
Comment 22 Miloslav Metelka 2009-10-13 18:24:00 UTC
If nobody objects I will integrate tomorrow.
Comment 23 Miloslav Metelka 2009-10-14 17:06:17 UTC

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