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 53295 - Cannot override Node's inplace rename behavior
Summary: Cannot override Node's inplace rename behavior
Status: CLOSED FIXED
Alias: None
Product: platform
Classification: Unclassified
Component: Data Systems (show other bugs)
Version: 4.x
Hardware: PC Windows ME/2000
: P3 blocker (vote)
Assignee: Jan Becicka
URL:
Keywords: API, API_REVIEW_FAST
Depends on:
Blocks: 45356
  Show dependency tree
 
Reported: 2005-01-12 13:59 UTC by Jan Becicka
Modified: 2008-12-22 18:53 UTC (History)
3 users (show)

See Also:
Issue Type: DEFECT
Exception Reporter:


Attachments
Proposed patch (9.14 KB, patch)
2005-04-28 08:43 UTC, Jan Becicka
Details | Diff
Modified patch including above mentioned suggestions + tests (20.45 KB, patch)
2005-05-03 15:10 UTC, Jan Becicka
Details | Diff
hopefully final patch (18.86 KB, patch)
2005-05-03 16:13 UTC, Jan Becicka
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Becicka 2005-01-12 13:59:06 UTC
F2 on Node invokes inplace rename action.
Unfortunately there is no way how to override this
behavior. And, unfortunately, we are required to
override F2 behavior in our ui spec
 http://ui.netbeans.org/docs/hi/javamdr/refactoring_design.html
, section UI specification | Rename:

Possible ways of invocation:
    ...
    * By pressing F2 key
    
The requirement is, that F2 should not invoke
inplace rename, but refactoring rename. AFAIK it
is not possible to do it with current APIs. Can
you provide such API, please?
Comment 1 Petr Nejedly 2005-02-04 11:27:48 UTC
I don't think I can provide such API for 4.1, so setting TM to future.
If you need to fix it in the mean time, it may be possible to
intercept projects view's JTree and modify its action map, replacing
"startEditing" mapping with your own action.
Comment 2 Jan Becicka 2005-04-22 09:10:38 UTC
We appreciate some API, which allows to plugin into setName() method. Adding
Jarda to CC.
Comment 3 Jaroslav Tulach 2005-04-23 09:12:17 UTC
I guess you need to intercept renames only on DataFolder nodes. If so, let's 
add interface like HandleFolderRename { public void rename(String, 
DataFolder) } into org.openide.loader, find it in lookup and call it from 
FolderNode.setName. I guess that would solve your problems. 
 
 
Comment 4 Jan Becicka 2005-04-25 09:56:04 UTC
Yes, I do exactly the same thing in java module (JavaNode). I introduced a new
interface org.netbeans.modules.java.RenameHandler.
There will probably arise a new requirement to override F2 behavior also for
e.g. JavaBeans node in the future.
Introduction HandleFolderRename is sufficient solution for now, but I would
appreciate more general interface HandleRename, which would be applicable for
nodes in general...
Comment 5 Jan Becicka 2005-04-28 08:43:00 UTC
I introduced 3 similar interfaces RenameHandler in java module,
FolderRenameHandler in openide/loaders, and PackageRenameHandler in java/project
module.
Jardo and Tomasi, please take a look at attached diff, and if you don't have
objections, let's switch this issue to api_review mode.
Comment 6 Jan Becicka 2005-04-28 08:43:37 UTC
Created attachment 21904 [details]
Proposed patch
Comment 7 Tomas Zezula 2005-04-28 12:06:10 UTC
Several comments:
1) Why the FolderRenameHandler interface takes a DataObject? Shouldn't a
FileObject be enough? And why it is in openide.loaders package?

2) PackageViewChildren.getRenameHandlers() should listen on the Lookup.Result
(some modules can be turned on/off)

3) I don't understand to the semantics of the FolderRenameHandler. Does it the
actual renaming? If so, is it correct to allow multiple FolderRenameHandlers?
Comment 8 Jan Becicka 2005-04-28 12:29:09 UTC
> 1) Why the FolderRenameHandler interface takes a DataObject? Shouldn't a
> FileObject be enough? And why it is in openide.loaders package?

This question is for Jarda. He suggested it. But it looks natural to me, that
FolderRenameHandler is in the same folder, where DataFolder and it's node resides.

> 2) PackageViewChildren.getRenameHandlers() should listen on the Lookup.Result
> (some modules can be turned on/off)

Yes it should. The same applies for other Handlers.

3) I don't understand to the semantics of the FolderRenameHandler. Does it the
actual renaming? If so, is it correct to allow multiple FolderRenameHandlers?

Yes it does renaming (java and package rename handlers as well). Currently we
don't have requirement for plugging into setName() besides the one from
refactoring. If you both prefer to have only one Handler, I'll change it.
Comment 9 Tomas Zezula 2005-04-28 12:53:54 UTC
I vote for having single FolderRenameHandler in the Lookup. In case when there
are more FolderRenameHandlers it is unclear what is the result of renaming.

To the point #1. If you are sure that it will not be needed after datasystems
removal then OK.
Comment 10 Jan Becicka 2005-05-03 15:10:36 UTC
Created attachment 21956 [details]
Modified patch including above mentioned suggestions + tests
Comment 11 Jesse Glick 2005-05-03 15:24:16 UTC
Get rid of all renameHandlersCache vars; they make the code more complicated and
they are very likely useless. How often is setName called on a node? Rarely. If
and when it is, ask Lookup for the handler(s).

And do not use assertions to check that there is <= 1 instance; you do not
control the code that might be adding additional instances, so you cannot
guarantee that this will really be true. Report such an error properly using
ErrorManager.
Comment 12 Jan Becicka 2005-05-03 16:13:50 UTC
Created attachment 21961 [details]
hopefully final patch
Comment 13 Jesse Glick 2005-05-03 17:07:18 UTC
Suggest a more helpful warning message (with no stack trace, since it is not
useful in this context):

ErrorManager.getDefault().notify(ErrorManager.WARNING, "Multiple instances of
FolderRenameHandler found in Lookup; only using first one: " + handlers);

In the test,

public static junit.framework.Test suite () {
    junit.framework.TestSuite suite = new
junit.framework.TestSuite(FolderRenameHandlerTest.class);
    return suite;
}

is useless and can just be deleted.

setUp() and tearDown() should always call the super method in their body, as a
matter of style.

Use

System.setProperty ("org.openide.util.Lookup", Lkp.class.getName());

for type safety and ease of Find Usages etc.

Lkp could more cleanly be written as

public static class Lkp extends ProxyLookup {
    public Lkp() {
        super(new Lookup[0]);
    }
    public void register(Object[] instances) {
        setLookups(new Lookup[] {Lookups.fixed(instances)});
    }
}

then lookupAdded can be removed and the test methods can be written more clearly
(and not depend on order of their execution!). If necessary, you can also
include Lookups.metaInfServices(Lkp.class.getClassLoader()) among the list of
lookups to proxy to.
Comment 14 Jan Becicka 2005-05-04 07:05:46 UTC
Tests were copy/pasted from other org.openide.loaders tests. Please remove
redundant suite() methods, call super in setUp() and tearDown() methods, and use
Lkp.class.getName() instead of hardcoded Strings in openide tests. Otherwise
people like me will use the same invalid patterns.
Thanks.
Comment 15 Jan Becicka 2005-05-04 07:19:02 UTC
Jesse, there is no method
ErrorManager.getDefault().notify(int, String);
Did you mean log(int, String)?
Thanks
Comment 16 Jesse Glick 2005-05-04 15:17:38 UTC
Yes I meant log(int,String), sorry.
Comment 17 Jaroslav Tulach 2005-05-05 12:42:28 UTC
> Use 
> System.setProperty ("org.openide.util.Lookup", Lkp.class.getName()); 
 
I have a feeling that I've seen this fail as the Lkp.class may trigger 
resolution of Lookup which can called Lookup.getDefault(). that is why I use 
full string name, it is safer. 
 
> ProxyLookup vs. AbstractLookup 
 
I like AbstractLookup more, but I do not think any such preference is 
important for the tested case. 
Comment 18 Jan Becicka 2005-05-11 15:51:37 UTC
Implemented
Comment 19 Jan Becicka 2005-05-11 15:51:52 UTC
fixed.
Comment 20 Jaromir Uhrik 2005-07-14 16:20:27 UTC
Verified.