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.
Following snippet: >> Node original = <create some node>; FilterNode filter = new FilterNode(original); System.err.println(filter.equals(original)); // prints true System.err.println(original.equals(filter)); // prints false!! << That breaks equals symmetry, which leads, to unexpected results. E.g. when trying to get value from map which keys are FilterNodes, but when tried to use for get(..) method the original node, it is returned null.
Added failing test: openide/test/unit/../FilterNodeTest#testEquals
I'd be interested in hearing of any suggestion how to fix this... node equality, generally, is a messy issue. Before making any changes, be *very careful* to check every node in the IDE with MoveUpAction, MoveDownAction, or ReorderAction, to see if it still works. Also look for unexplained NPEs from Children. :-( The basic problem is that org.openide.nodes.Index works on nodes, not their cookies (data models), yet it itself is a cookie. This gives rise to a lot of different problems and can be very confusing. Take a look at the weird code in FilterNode for example; just by subclassing FilterNode (without overriding any methods) you change its semantics w.r.t. equality. Personally I doubt this issue can be solved without throwing out Index and fixing the API. Just a guess though. You may find some strange workaround.
I thought at least also Node.equals has to be rewritten, to manage it. I don't remember whole stuff about equality (from Effective programming), whether it is even possible to achieve the equality contract in the case of Node - FilterNode and their subclasses. Anyway I think the current state is wrong. Decreasing the priority, since it is unknown what to do.
Yeah, you would have to change Node.equals. But this is all wrong: Node is the API, FilterNode is *an* SPI, so Node should not refer to FilterNode in this way. What if you made org.pzavadsky.nbutil.MyFilterNode, a copy of org.openide.nodes.FilterNode? Then equality would not work on that. Probably no Node subclass should override equals or hashCode at all. Then some stuff relating to Index would need to be patched somehow.
Marking as future. Fix wound be trivial if possible at all.
Created attachment 11108 [details] Providing trivial fix
I guess I can change the milestone to 4.0. The fix is simple and in my opinion pretty safe. It keeps the old behaviour and makes it symetric as requested. Nobody shall be hurt by it. Maybe we could delete the API keyword?
As I wrote on 2002-10-24, I consider this patch a workaround for some common instances of the bug, not a solution. Using equals/hashCode on Node at all is probably inherently wrong. (BTW if you must override Node.equals(), don't give it Javadoc.) Nodes are presentation devices and as such should never be tested be equality. org.openide.nodes.Index is the root of the evil; it should IMHO be deprecated and redesigned. In issue #29192 I offer a suggestion how this could work properly using Looks, that would not require any strange hacks like this. Also someone should check the performance of the patch when e.g. comparing two nodes which have each been filtered five times, which is quite common in NB, especially in the Options window.
BTW: s/equaliness/equality/ Perf: FN^5(A).equals(FN^5(B)) = false will take "only" 2^(6+6) equals() calls. Note this is way more than comparing each node on the left side with each node on the right side (6x6). Of course it is possible to easily create an algorithm to do such equals in O(N) with ~1 equals() call.
Index cookie maybe the root evil, but this bug is about Node and FilterNode.equals. If we do not want to solve it, we may make it wontfix. "Nodes are presentation devices" - ok, but maybe for that reasons somebody ;-) tried really hard to make two Nodes that will always appear the same equal to each other. FilterNode.isDefault () == true will aways be the same as its original. I have found line "return original.equals (o) || o.equals (original);" in FilterNode.equals and it seems to slow things down a lot. The meaning of this is probably that FilterNode is equal to another node if the node is (one of its originals) or the filter node is itself one of originals of "o". I believe it could be shorten to "return o.equals (original)" that will give O(numberOfFilterNodes) but to be honest I have to write some tests first. I agree that general fix is desired, but my primary impuls for investigating this issue is the continuesly failing unit test FilterNodeTest.testEquals. I want it to stop failing, whether by deleting it or by fixing it. But right now I feel we are closer to fixing it.
Created attachment 11159 [details] In order to test current behaviour of FilterNode.equals here are some additional tests
Re. What is the complexity of comparing: Big, FilterNode.equals does some strange || at the end. Maybe unnecessary, if we do not rely on compatibility that much. Re. Too complex in Tools/Options: Not true, the equals is not complex for subclasses of FilterNode, it is quity simple than and that is the case for Tools/Options.
Created attachment 11160 [details] Tests extended by another one
My point re. Index cookie is simply that this terrible API is the *only* reason you would ever need to call Node.equals at all. If we replace Index, then we can delete all overrides of equals and hashCode from Node and its subclasses, leaving them to compare pointers only, and this problem disappears forever. Re. "the equals is not complex for subclasses of FilterNode, it is quity simple than and that is the case for Tools/Options" - not so simple to me; I spent a lot of time debugging and fixing it. Issue #10186, issue #17920, issue #8552, e.g. in ToolbarFolderNode.ToolbarItemNode: /** Make Move Up and Move Down actions be enabled. */ public boolean equals (Object o) { if (o == null) return false; return this == o || getOriginal ().equals (o) || o.equals (getOriginal ()); }
Taking over the node bugs from phrebejk.
Fine, I suggest you to apply the fix here or remove the only failing test in nodes package.
I've just faced this bug in /cvs/contrib/favourites/src/org/netbeans/modules/favourites/Favourites.java,v <-- Favourites.java new revision: 1.3; may I ask for fixing it especially if the solution is known.
Note that the attached fix is O(2^(m+n) for case when FN^m(A).equals(FN^m(B)) == false I can't apply such fix, I need another one...
Created attachment 15130 [details] Different patch that has O(n+m) complexity, but different semantics. Is it OK?
Probably OK, since the actual semantics of FilterNode.equals is undefined and probably undefinable...
OK, integrated. openide/src/org/openide/nodes/Node.java,v1.84 openide/src/org/openide/nodes/FilterNode.java,v1.91 openide/test/cfg-unit.xml,v1.111 openide/test/unit/src/org/openide/nodes/FilterNodeTest.java,v1.14
Seems symetric which is probably the only necessary semantic it should have.