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.
DefaultTopComponentLookup could be simplified dramatically, and bugs like issue #36336 would not appear, and a bizarre null return from Lookup.Item.getInstance() would not be needed, and some other code in actions and so on would be simplified and made clearer, and performance would probably be improved, if instead of searching the TopComponent's lookup for Node.class (e.g. in NodeAction.DelegateAction), an instance of the following were placed in TopComponent lookup and queried for by anyone interested in a node selection: package org.openide.nodes; // ??? /** * Cookie representing ability to have a set of selected nodes. * May be placed in a top component's lookup, for example. Or * generally in the lookup produced by a Component which is a * Lookup.Provider in order to permit queries on the node selection. * @since XXX */ public interface NodeSelection { /** * Produce a node selection. * May be null to indicate that there are no nodes to be * selected, or empty to indicate that a selection is * possible but there are no nodes currently selected. * @return an immutable and unchanging set of selected nodes, * or null */ public Set/*<Node>*/ getSelectedNodes(); /** * Add a listener to changes in the node selection. * Call {@link #getSelectedNodes} again to get the new set, * rather than getting its items again. * @param l a listener to add */ public void addChangeListener(ChangeListener l); /** * Remove a listener to changes in the node selection. * @param l a listener to remove */ public void addChangeListener(ChangeListener l); } Using Lookup to find each node in the selection, not to mention hacking around null selections using a null-instance item with ID "none", is very ugly and it is hardly surprising the code is so complex and buggy. I can't understand why the simple solution given above was not used. (Cf. Petr Kuzel's suggestion re. an "ActivatedNode wrapper" in issue #36336.) Is it too late to change? I don't think so - this system was introduced only recently as part of openide separation, and could still be fixed.
The ContextAwareActions based on Nodes in Lookup were introduced for 3.4. The Nodes should probably stay in the selection. This class is nice, but the problem is not in nice interface, but in defining and testing the correct behaviour.
(3.5, not 3.4) Sorry, I thought this was introduced as part of openide separation, but apparently not - so we are perhaps stuck with it. (Though it is just barely documented - in TC.getLookup.) I suppose I must have reviewed this API addition at some point, but I wish I had objected before... I disagree that there is no problem with the current interface (or rather API, since no explicit interface is defined currently). Defining and testing the correct behavior is quite important, and it is unnecessarily difficult with Node's dumped directly into the lookup for the reasons I mentioned - unclear semantics when nodes contain other nodes or themselves in their lookups; abuse of Lookup.Item; etc. The code in DTCL, and the accompanying tests, are quite complex to support something that you would expect to be trivial - passing a Node[] from one place to another. Generally I feel Lookup should be used either for singletons, or first-class services for which there is a clear meaning of having one or more instances; and it should be expected that firing changes in the contents of a given Lookup should be a relatively uncommon event. Inserting a node selection into the Lookup as a list of individual items, and firing changes in the TC lookup every time the node selection is changed, seems like a real hack.
3.5. Right. The reason for having Node and its cookies in lookup is the ability to write "cookie action" without being dependent on Nodes. Right now one can only use lookupContext.lookup (SaveCookie.class) and does not need to bother with nodes anymore. This advantage was the main argument in favor of having Nodes and their cookies in context lookup. Firing changes from lookup using LookupListener plays important role in the declarative actions roadmap and should contribute to speed up of node context changes. The idea is that new cookie action will listen not on node changes but directly on the cookie changes. E.g. Project.class. With this style, the action will not be even notified if a different node in the same project is selected. The mutable lookup was a conscion choice to achieve this behaviour. See for example http://openide.netbeans.org/proposals/actions/enabled.html I am sorry for the compatibility problems and I am ready to fix them if reported to me, but I believe the Lookup containing nodes and their cookies is a good direction for future and it is worth to fix the bugs and not introduce the NodeSelection concept. Thus I'd like to not implement this issue.
Re. CookieAction - actually I think having the *cookies* of the selected nodes be gathered and placed into the lookup is nice. (Not sure how great the performance will be, but API-wise fine.) Was more complaining about having the *node instances* be in the lookup "raw", since nodes are UI artifacts, not services. If you agree with the current weird code in DTCL, and don't mind maintaining it, then I guess OK...
Originally the code was simpler, but then I received the reports about nodes that do have themselves in lookup and those that do not have themselves and FilterNodes, etc. and it got complicated. Anyway I hope that we are 90% at the best state and I am willing to implement the rest 10% when discovered rather then throwing out what works now.
I eliminated the code that manifested this problem. Assertions would be helpfull for diagnostics purposes.
Sorry, what assertions did you have in mind?
Assertion that tests implementation assumptions if any (I have not seen new impl).