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.

Bug 32744 - AbstractNode.defaultAction & preferredAction interact strangely
Summary: AbstractNode.defaultAction & preferredAction interact strangely
Status: VERIFIED FIXED
Alias: None
Product: platform
Classification: Unclassified
Component: Nodes (show other bugs)
Version: 3.x
Hardware: All All
: P2 blocker (vote)
Assignee: Jaroslav Tulach
URL:
Keywords: API, REGRESSION
Depends on: 22384 33038
Blocks: 27868
  Show dependency tree
 
Reported: 2003-04-09 00:04 UTC by Jesse Glick
Modified: 2008-12-22 17:13 UTC (History)
1 user (show)

See Also:
Issue Type: DEFECT
Exception Reporter:


Attachments
Possible patch (includes new unit test exercising all the cases I can think of) (7.08 KB, patch)
2003-04-09 00:05 UTC, Jesse Glick
Details | Diff
Fix based on reflection - Jesse's test passes (2.07 KB, patch)
2003-04-10 09:26 UTC, Jaroslav Tulach
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jesse Glick 2003-04-09 00:04:09 UTC
For example, overriding getDefaultAction does not
produce the intended result for
getPreferredAction. In 3.5, this is a minor
problem since no one typically calls
getPreferredAction. In the trunk, however,
Explorer views do call it, so for compatibility
with existing AbstractNode classes it is desirable
to implement it correctly. For example:

- old node overrides just getDefault -> should be
used from getPreferred

- old node is a BeanNode (which calls setDefault)
and also overrides getDefault (which used to
cancel out the effect of setDefault) -> getDefault
should be used from getPreferred

Attaching possible patch which seems to work to
keep full compatibility. Unfortunately it adds a
bit to the storage requirements for AbstractNode.
Comment 1 Jesse Glick 2003-04-09 00:05:16 UTC
Created attachment 9789 [details]
Possible patch (includes new unit test exercising all the cases I can think of)
Comment 2 Jesse Glick 2003-04-09 00:06:45 UTC
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.
Comment 3 Jesse Glick 2003-04-10 02:15:04 UTC
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).
Comment 4 Jaroslav Tulach 2003-04-10 09:04:18 UTC
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.

Comment 5 Jaroslav Tulach 2003-04-10 09:26:03 UTC
Created attachment 9828 [details]
Fix based on reflection - Jesse's test passes
Comment 6 Jesse Glick 2003-04-10 15:12:11 UTC
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?
Comment 7 Jesse Glick 2003-04-18 18:02:40 UTC
AN.sPA: see issue #33038.
Comment 8 Jaroslav Tulach 2003-04-23 10:21:21 UTC
/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
Comment 9 Jaroslav Tulach 2003-04-23 15:07:17 UTC
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
Comment 10 Jaroslav Tulach 2003-04-24 10:06:44 UTC
Need some improvments, because of issue 33205.
Comment 11 Jaroslav Tulach 2003-04-24 13:49:11 UTC
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
Comment 12 pzajac 2004-02-25 14:51:59 UTC
verfied