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.
Summary: | AbstractNode.defaultAction & preferredAction interact strangely | ||
---|---|---|---|
Product: | platform | Reporter: | Jesse Glick <jglick> |
Component: | Nodes | Assignee: | Jaroslav Tulach <jtulach> |
Status: | VERIFIED FIXED | ||
Severity: | blocker | CC: | jtulach |
Priority: | P2 | Keywords: | API, REGRESSION |
Version: | 3.x | ||
Hardware: | All | ||
OS: | All | ||
Issue Type: | DEFECT | Exception Reporter: | |
Bug Depends on: | 22384, 33038 | ||
Bug Blocks: | 27868 | ||
Attachments: |
Possible patch (includes new unit test exercising all the cases I can think of)
Fix based on reflection - Jesse's test passes |
Description
Jesse Glick
2003-04-09 00:04:09 UTC
Created attachment 9789 [details]
Possible patch (includes new unit test exercising all the cases I can think of)
Marking API since the API behavior of getPreferredAction is at stake. Marking REGRESSION since in the current trunk, old nodes which used getDefaultAction and setDefaultAction do not work as expected with the new explorer views. A priority in the trunk, I think. E.g. in dev builds the preferred action for HtmlNode is always View. HtmlNode says: public SystemAction getDefaultAction() { // ... if (/* doc fs */) { return SystemAction.get(ViewAction.class); } else { return SystemAction.get(OpenAction.class); } } However DataNode says: public Action getPreferredAction() { // ... Action action = super.getPreferredAction(); if (action != null) { return action; } Action[] arr = getActions(false); if (arr != null && arr.length > 0) { return arr[0]; } return null; } and AbstractNode says: public Action getPreferredAction() { return preferredAction; } So HtmlNode.getPreferredAction is implemented in DataNode, which gets null from the super call and then chooses the first action from the context menu - View. Properly, the super call should return View or Open as defined in HtmlNode.getDefaultAction. The patch should accomplish that. I'm sure there are other things which are broken. The root of some, but not all, evil is the fact that you can both set a default action and override the getter. This is ridiculous. I am unhappy that we repeated the mistake by introducing AbstractNode.setPreferredAction (see issue #22384). Jesse's new test shall be part of the testsuite, sorry for not providing this, but AbstractNode.setPrefAction, was done after I stopped work on the actions branch. I was just notified about the change, I admit, I agree with it, but now maybe Jesse is right, we could delete it for 3.5. Anyway this does not solve the n2 example in Jesse's test. We need some hacks for it as well. I would rather use reflection instead of ThreadLocal. I'll attach an example. Created attachment 9828 [details]
Fix based on reflection - Jesse's test passes
Agreed, even without setPreferredAction you still need some tricks. Yarda's patch looks good. I was nervous about using reflection, but I think this is better - CPU speed of this method is not as important as avoiding wasted memory. Minor comments to the patch: - if preferredAction != null, can return that immediately, no need to check for getDefaultAction (IMO) - could avoid calling getClass() >1 time per call to getPreferredAction - clean up comment "we are subclass of FilterNode" BTW are there any tests confirming that FilterNode works sanely with getPreferredAction vs. getDefaultAction? AN.sPA: see issue #33038. /cvs/openide/src/org/openide/nodes/AbstractNode.java,v <-- AbstractNode.java new revision: 1.63; previous revision: 1.62 done Processing log script arguments... More commits to come... Checking in test/unit/src/org/openide/nodes/NodeTest.java; /cvs/openide/test/unit/src/org/openide/nodes/NodeTest.java,v <-- NodeTest.java new revision: 1.4; previous revision: 1.3 done Integrated to 3.5 branch /cvs/openide/src/org/openide/nodes/AbstractNode.java,v <-- AbstractNode.java new revision: 1.60.6.1; previous revision: 1.60 done Need some improvments, because of issue 33205. Once again and better. This is the new fix (only in trunk): Checking in src/org/openide/nodes/AbstractNode.java; /cvs/openide/src/org/openide/nodes/AbstractNode.java,v <-- AbstractNode.java new revision: 1.64; previous revision: 1.63 done Processing log script arguments... More commits to come... Checking in test/unit/src/org/openide/nodes/NodeTest.java; /cvs/openide/test/unit/src/org/openide/nodes/NodeTest.java,v <-- NodeTest.java new revision: 1.5; previous revision: 1.4 verfied |