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.
In the current list of classes loaded on startup the actions that "just" open a top component clearly stands out. Moreover with every new window, technology or view, the list grows up. We need to prevent this grow and potentially also replace already existing actions with something more declarative. Also we shall update API support to generate top components together with this new, declarative API.
Created attachment 63093 [details] TopComponent.openAction factory method
[JG01] It is not "normal" for a file to specify SFS.lB but for there to be no such key in the bundle; it is an error. I guess that was preexisting code but it ought to be fixed now. [JG02] SFS.lB should be of the form pkg.Bundle, not pkg/Bundle. [JG03] Please get rid of the weird changes to core.startup and the undocumented contract of "displayName" and "icon" map attributes. OpenComponentAction could just as easily read SystemFileSystem.localizingBundle and SystemFileSystem.icon attributes directly and interpret them, or just read attributes of its own devising such as "bundle", "bundleKey", and "icon". There is no particular advantage in having the instance file itself have a display name and icon. [JG04] You seem to have forgotten SFS.icon on AnalyzerAction.instance. [JG05] This whole change seems unnecessary and short-sighted. It introduces a lot of infrastructure basically to save you from typing TopComponent win = WhateverTopComponent.findInstance(); win.open(); win.requestActive(); which is too small to be considered reusable code. It does very little to make writing a singleton TopComponent more "declarative" (you still need tons of copy-pasted procedural code in the TC subclass itself). The primary goal was anyway to reduce the number of action classes loaded on startup. If that is what you want to do, then do something straightforward and general that would handle any action which is always enabled, e.g. <file name="whatever.instance"> <attr name="instanceCreate" methodvalue="org.openide.util.actions.Actions.genericAlwaysEnabledSingletonAction"/> <attr name="bundle" stringvalue="pkg.Bundle"/> <attr name="nameKey" stringvalue="whatever.name"/> <attr name="hintKey" stringvalue="whatever.short_description"/> <attr name="icon" urlvalue="nbresloc:/pkg/whatever.png"/> <attr name="class" stringvalue="pkg.WhateverTopComponent"/> <attr name="method" stringvalue="findOpenAndRequestActive"/> </> [JG06] Is there some performance measurement that indicates that avoiding the loading of a couple dozen very small Action classes will save a significant amount of startup time (worth the cost of more complex registration)?
Regarding JG05, I am not so much objecting to this change (beyond that it adds some more conceptual weight to the Window System API) so much as wondering why a more generally useful system, such as was being developed in contrib/api.dynactions (I think?), is not being put forward instead.
JG04 there was no icon for the action. JG02 OK JG01 I do not know what you mean? JG03 SFS.localizedBundle is SPI. I'd like to avoid its use in API: issue 138076 JG05 I agree a generic system would be nice. I'll investigate the possibility to place it into awt.Actions JG06 The problem is not with well written class, but with those written poorly that load a lot of related peers.
Created attachment 63337 [details] Both Actions.alwaysEnabled and TopComponent.openAction - still missing more docs and apisupport
JG01: I was referring to } catch (MissingResourceException ex) { // ignore--normal } which I think is incorrect. JG03: might be a good solution, I will add comments there.
Created attachment 64099 [details] All changes including API support modifications
Unless there are strong objections, I'll integrate by end of this week.
[It's a good idea to use [diff] git=1 or --git when preparing patches.] JG03 was not addressed. Still dislike use of private contract to core, which seems unnecessary. Anyway I thought we were using issue #138076 for the display name part, contrary to <attr name="SystemFileSystem.localizingBundle" stringvalue="org.netbeans.core.actions.Bundle"/> in the proposed core.ui patch? [JG07] <attr name="SystemFileSystem.icon" stringvalue="org/netbeans/core/resources/log-file.gif"/> is wrong. Use urlvalue="nbresloc:/..." instead. [JG08] What is going on with org-netbeans-core-actions-LogAction.instance, anyway? There is no apparent action listener ('delegate') in sight. [JG09] The new Actions.alwaysEnabled (not mentioned in this issue before) seems to use an 'iconBase' parameter, which matches neither its own Javadoc nor the core.ui patch. There is similar confusion over 'displayName' (in impl) vs. SFS.lB (in Javadoc and test). [JG10] It is not clear what the intended type of iconBase should be. Looks like Icon, Image, String, or URL are all accepted for SMALL_ICON. Yet Actions will expect it to be a String. [JG11] The description in filesystems/apichanges says "When using methodvalue in XML filesystem, one can add there expect new values 'displayName' and 'image' available to methods that accept Map." I cannot figure out what this means.
Created attachment 64204 [details] I will fix apisupport and integrate tomorrow
[JG12] The change in BinaryFS - return fo.getAttribute(key); + Object ret = fo.getAttribute(key); + if (ret != null) { + return ret; + } + return null; is a no-op. [JG13] LogAction should I suppose be changed to be a simple ActionListener, not a full action. Alternatively, enhance AlwaysEnabledAction to behave the way Eclipse declaratively registered actions do: after the first time it is invoked, if its delegate is in fact an Action and not just an ActionListener, stop using static metadata for isEnabled and getValue, and start delegating these methods to the real action too (means firing a general property change at switchover time and also delegating changes fired from the real action). This has the nice behavior that you can have a low-footprint declarative registration, but when the action starts to actually be used, it can have more complex behavior: change enablement status, update its display label, etc. (It is very unfortunate that we instanceof rather than getProperty to check if actions implement Presenter.*, since interface assignability cannot be proxied.) [JG14] core.ui layer presumably need to use 1.2 DTD, like java.hints. Same in test-layer.xml and test-layer-attribs.xml. [JG15] java.hints.analyzer/src/org/netbeans/modules/java/hints/analyzer/ui/Bundle.properties patch is a no-op. [JG16] The Javadoc <attr name="noIconInMenu" boolvalue="false"/> <!-- not needed, default behaviour --> is a bit confusing since it has a negative sense. Prefer <!-- if desired: <attr name="noIconInMenu" boolvalue="true"/> --> Same on openAction, which also has the typo "booleavalue". [JG17] "if this icon shall not have an item in menu" is backwards. [JG18] I don't believe the special treatment of Action.ACCELERATOR_KEY is needed in AlwaysEnabledAction. IIRC, something in core automatically calls put(A.A_K, ...) with the key found in Shortcuts if there is really a binding. You would have to return super.getValue(name) rather than null for this to work. [JG19] Please do not reformat openide.filesystems/apichanges.xml. (You may be suffering from issue #138951.) There do not even appear to be any interesting changes in this file. Ditto XMLMapAttr.java. [JG20] java.hints asks for openide.windows 6.23 but you are creating 6.24. [JG21] The constructor OpenComponentAction(TopComponent) looks to be unused.
Integrated: 1e9604fcc4b9 Done: JG12, JG15, JG19, JG14, JG16, JG17, JG20 Ignored: JG19, the setValue trick works very poorly with multiple instances of the same class, like clones in popup menu Ignored: JG13, but I like it very much. If you report it as a bug, I can implement it for 6.5 Ignored: JG21, I do not care.
[It was JG18 you were ignoring, not JG19.] I don't believe that's true. setValue(ACCELERATOR) should only be called on the global instance which is actually registered in Shortcuts (and which might be *.shadow'd into global menus or toolbars) - and this is what you want: context-sensitive versions are not what that accelerator would actually run. Take a look at NbKeymap.updateActionAccelerator. We already use this for all actions and AFAIK it works even for non-SystemAction's and even for actions with a context-sensitive popup variant. All I am suggesting is that the special code you introduced only for Actions.alwaysEnabled is redundant and should be deleted (while falling back to delegating to super for getValue). (Your current code also does not sort multiple keystrokes like NbK.uAA does, nor will it fire changes when the global keymap changes.) JG21 - never mind, in fact it is used from the public TopComponent.openAction overload. JG13 - OK, filed as issue #139614. Also new JG22 - the documentation for A.aE says that iconBase should be a String. Yet the implementation also accepts Icon, Image, or URL. Either the implementation should be made strict, or the documentation should specify that the layer variant of the factory can accept these other types for the parameter.
Integrated into 'main-golden', available in NB_Trunk_Production #324 build Changeset: http://hg.netbeans.org/main/rev/1e9604fcc4b9 User: Jaroslav Tulach <jtulach@netbeans.org> Log: Merge: #137709: Support for declarative actions for alwaysEnabled and TopComponent.openAction
BTW I cannot find any apichanges entry for Actions.alwaysEnabled, and for TopComponent.openAction it points to the apparently unrelated issue #136636.