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: | Two or more editor panes for the same file | ||
---|---|---|---|
Product: | platform | Reporter: | ivan <ivan> |
Component: | Data Systems | Assignee: | Jaroslav Tulach <jtulach> |
Status: | RESOLVED FIXED | ||
Severity: | blocker | CC: | anebuzelsky, apireviews, gordonp, jlahoda, jtulach, pnejedly, rmatous, thp, vv159170 |
Priority: | P2 | Keywords: | API_REVIEW_FAST |
Version: | 4.x | ||
Hardware: | All | ||
OS: | All | ||
Issue Type: | ENHANCEMENT | Exception Reporter: | |
Bug Depends on: | |||
Bug Blocks: | 209745 | ||
Attachments: |
Tar file of diff output + one new Java file
Reduced patch. Note that it does no filename-based prefiltering, it is the responsibility of the plugged in comparator. Change for the openide/text module Verification files for Jarda's patch (add-on to cnd) |
Description
ivan
2004-11-19 21:17:32 UTC
This issue has been ignored in the MasterFS rewrite, so bubbling it up. Sun Studio has had both an escalted bug and a (large) lost sale because of this problem. In our Venus release, we fixed this on our private 3.5.1-based branch. In our next release, we'll need this fixed in the trunk. We realize this problem isn't as critical to the netbeans community as it is to the native language (Sun Studio) community. So our fix was to create an interface (unimplemented in netbeans), do a lookup on it, and only make the call if the lookup returned a class provided by a non-netbeans module. In our case, its a provider class which makes JNI calls where we can validate any file being opened. Our fix is currently in our 3.5.1 branch. In our next release (Mars) we'll port it to current sources and make this an official proposal for an API enhancement. Imagine structure: . |-- a.folder | `-- a.file |-- b.folder `-- b.file -> ../a.folder/a.file Asserts that must be satisfied: FileObject A.FOLDER = FileUtil.toFileObject(a.folder); FileObject A.FILE = FileUtil.toFileObject(a.file); ... assertSame(A.FOLDER.getFileObject("a.file"), A.FILE); assertNotSame(B.FOLDER, A.FOLDER); assertEquals("b.file", B.FOLDER.getFileObject("b.file").getNameExt()); assertEquals("a.file", A.FOLDER.getFileObject("a.file").getNameExt()); assertSame(B.FOLDER.getFileObject("b.file").getParent(),B.FOLDER); assertSame(A.FOLDER.getFileObject("a.file").getParent(),A.FOLDER); I don't think there is any way how to ensure these assertions if this one is also satisfied: assertSame(B.FOLDER.getFileObject("b.file"), A.FOLDER.getFileObject("a.file")); I can only imagine : assertEquals(FileUtil.toFile(A.FILE).getAbsolutePath(),FileUtil.toFile(B.FILE).getAbsolutePath()); Please, give me more information about your intentions or even better let me know how looks your private impl. These are the asserts that cause trouble: assertSame(B.FOLDER.getFileObject("b.file").getParent(),B.FOLDER); assertSame(A.FOLDER.getFileObject("a.file").getParent(),A.FOLDER); On unix if you cd to a directory and then cd .. you're not guaranteed to end up in the directory you started with. So it seems to me that getParent() applied to a file "object" is a bad idea. getParent() applied to a java.io.File, which represents a path, does make sense. Last time I raised this issue I was told that despite there being an 'Object' in FileObject it's more like a java.io.File and "path-like" and that therefore getParent() is OK. Yet, FileObject neither inherits from File nor follows it's object protoco: move/rename vs renameTo etc. It also plays a big role as the target of direct manipulation in the Explorer ... tha looks pretty object-like (as opposed to path-like) to me. As a practical use case consider what happens if you open a.folder/a.file and b.folder/b.file. Unless both map to the same editor tab you will get a situation where users might edit a bit here and edit a bit there and get mighty confused when they lose data. This has nothing to do with native development and should be considered as a generic NB editor mis-design for unix platforms. If we posit that FileObject is object-like then two different paths can be mapped to the same FO. There is some entity inside NB that is accomplishing this mapping and I had suggested, to a first approximation, that this mapping become a service. This will implicitly solve the editor problem (assuming the editor looks for existing tabs on the given FO befor opening new tabs). It will also solve the problem generally for use cases other than the editor problem. If declaring FO object-like and banishing getParent() is taboo then this can be solved in the editor tab lookup. I think this is what Gordon has implemented. It will solve the most important use case but won't be general enough. While I can't off-hand think of a use-case involving something other than editor buffers it's s ticking bomb. At least one of these can't pass: assertEquals("b.file", B.FOLDER.getFileObject("b.file").getNameExt()); assertEquals("a.file", A.FOLDER.getFileObject("a.file").getNameExt()); because you suggest: assertSame(B.FOLDER.getFileObject("b.file"), A.FOLDER.getFileObject("a.file")); There is obvious that your suggestion is not just implementation detail that can be fixed in MasterFS this change would mean incompatible changes in FS API which would cause need to change individual FS implementations, check and fix all FS client code including other fragile parts of platform. Very, very risky. Scope: huge. Let's not worry about the scope and risk yet. I raised this issue 2 years ago (note the keyword VENUS). Scope and risk issues kept us from progressing at all. Had we taken some small steps then we'd be closer to our goal now. So let's see where we want to be ... four years from now. The tradition for dealing with soft links is to has a plain filename and a follow-any-links filename. That is why java.io.File has absolute path and canonical path. FileObject doesn't seem to have this distinction. How about if you mapped getNameExt() to absolute names and introduce a getCanonicalNameExt()? You also still haven't answered me whether FileObject is supposed to be object-like or path-like. If it's object-like then all aspects of naming and connectedness (parent) should be removed and java.io.File should be used for that. If it's path-like it should strive to inherit from or implement java.io.File and a new true object-like FileObject be created. Once you have properly distinguished classes consider that the File to TrueFileObject mapping is many to one. Then for every resolved mapping one can create back-pointers from a TFO to every File that resolved to it and through delegation provide naming and parentage information associated with a TrueFileObject but these queries will now have to return lists (and defaults) and the lists will not be guaranteed to be static; they will evolve if other Files get mapped to the same TFO. Now the TFO is starting to look a lot like todays FileObject but based on a more true-to-the-world design. We took a round trip and came to the same place. We can also make this trip backward. Keep FO around for backward compatibility but introduce TFO which works with java.io.File in parallel. Then Implement FO in terms of these. Then deprecate FO. > Very, very risky. Scope: huge.
Actually, the implementation we did in our Venus release was very minimal.
I'm going to have to go digging to find it, but I'll add (or attach) it to
this IZ within the next week or so.
Basically, I added a provider API to an existing NB API. If no provider
was registered, netbeans behaved as it normally did. Then I implemented
the provider which called some JNI code which did actual file comparisons
using device and inode information (you can't do this in Java).
I think the only overhead in netbeans code was to do a lookup on my provider.
In all cases except Sun Studio, the lookup should return null and standard
behavior should prevale.
I'm ready to look at your Venus implementation first. Thinking about risk and scope can't be avoided. FileSystem is probably the most spread NB API at all. Changing it in incompatible way should be the last alternative. So, first there should be obvious: - why we actually doing it - use cases (more editor tabs may exists for one file anything else?) - is necessary to fix such low level API like FileSystems to cover use cases and if so isn't there any compatible way If a.file and b.file pathes are represented by one FileObject then I don't know why to have both methods absoluteName, canonicalName because a.file.absoluteName == b.file.absoluteName and the same for the canonical method. There are still implemetations of FS that are not based on java.io.File. So, subclassing java.io.File isn't in sipirt of FS API which is more general. I can hardly imagine how I would use more parents then probably just fo.getParents()[0] and I actually think that such method would be more or less useless. Object-like, path-like - probably mix. Here are the code differences I made to the private netbeans branch rel35Venus in support of the escalated P1 bug we had about multiple FileObjects referring to the same file. I might add that besides the escalated P1 bug, I was informed (I don't remember by whom) that Sun lost a fairly large sale to another company (ie, not the one who filed and escalated this bug). So this problem has already cost Sun $$ which is why we consider it a must-fix. ========================================================================== I diff'ed complete checkouts of 2 branches of netbeans. The checkouts were: $ (cd vulcan; cvs co -rrelease35V standard) $ (cd venus; cvs co -rrel35Venus standard Vulcan was the code name of Sun Studio 10. It used a slightly modified NetBeans 3.5.1. Venus was our code name for Sun Studio 11. It was the release with the private fix for IZ 51690. Here are the changed files: FileUtil.java: New method (copied from nb4.1, not venus code): public static String getFileDisplayName(FileObject fo); DataEditorSupport.java: Added new method (part of IZ 51690 impl): protected CloneableTopComponent getAlternateTopComponent(); Update to createStyledDocument (copied from nb4.1, not venus code): Changed a line of ode to use FileUtil.getFileDisplayName(). This change is reflected in current NetBeans. EditorSupport.java: Added new method (part of IZ 51690 impl): protected CloneableTopComponent getAlternateTopComponent(); CloneableOpenSupport.java: A few changes in openCloneableTopComponent (part of IZ 51690). A diff -w returned less than 25 lines of changes. According to your code channges seems to me that two or more editor panes for the same file is the real cause that you require to fix. Also there is obvius that no code changes in FS was made to achieve it but rather openide.text. So, reassigned to openide.text. Gordon, can you please provide the diff of your changes? Created attachment 37249 [details]
Tar file of diff output + one new Java file
I've just attached a tarball of the diffs which we used in our Venus release to fix this problem. One change (in FileUtils.java) was to add a method which was copied from NetBeans 4.1. So this change isn't needed, since its already part of the netbeans sources. Gordon, I looked at the diffs and I don't like them much. First, they don't really solve Ivan's concerns. Moreover, there will be not only two FileObjects for an aliased file, but two DataObjects, two CloneableEditorSupports and, worst of all, two independently editable Document instances. If we were about to handle the problem correctly using this approach, we'd need to cover other things as well, not only the TCs - Document, LineSet, SaveCookie and maybe the cookies generally. Better approach might be to share EditorCookie at the DataObject level, but that has an inherent problem with detecting aliases at the time of cookie creation - current logic looks for opened TCs of an aliased file[1]. Finally, I don't like the prefiltering logic hardcoded into the DataEditorSupport. If it has to be somewhere, then the PathComparisonProvider implementation is the place for it. [1]: Imagine that I multiselect the aliased pair, a/a.c and b/a.c and invoke a popup menu. None of them is opened yet, so the code doesn't recognize it needs to share the cookie. Maybe if the filter checks against live DataObjects, but there are so many of them... All in all, I don't know what to do with this issue now. Provide a small reflective hook (w/o official API?) at the EditorSupport level? Let Yarda explore the Cookie sharing at the DO level? The cookie sharing path is elegant but impossible, as every DataObject implmenetation would have to be changed to allow for the sharing. I'm currently looking into a way of reducing the API impact of the gordon's proposed change, as the original patch changes API across 3 modules (utils, windows and text). So far I have prepared a patch that reduces the API impact to loaders module only. I'll attach the patch and reassign to DS module (it never really touched text module anyway). Created attachment 37771 [details]
Reduced patch. Note that it does no filename-based prefiltering, it is the responsibility of the plugged in comparator.
The fix failed because createCloneableTopComponent() calls cloneableEditorSupport(). When I first wrote this code it was for NB 3.5.1 and DataEditorSupport and CloneableEditor were both in the same module and package. Now CE is in org-openide-text and DES is in org-openide-loaders so DES cannot access the protected cloneableEditorSupport() method. I'll try making cloneableEditorSupport() public tomorrow. That will tell me if the basic logic behind the patch still works (I'm not suggesting you take that as the fix, but it will tell me whether we're close or not). I do not like the mangling with allEditors in the previous patch. That is why I'd like to try a new approach: a redirector. I'll attach a patch, guys please do me a favor and review it and try it. It works relatively fine in the test, but I have not tried it in real IDE. Created attachment 37884 [details]
Change for the openide/text module
When I can expect confirmation that my patch is on the right way to solve the problem? > When I can expect confirmation that my patch is on the right way to
> solve the problem?
I'll give a tentative yes now. I'm 90% done with my verification and expect
to finish Monday.
Excellent, I'll get things ready to integrate on Thursday Feb 8, 2007. I'm done with my verification. However, lets call this a 90% verification because while I did verify it in NetBeans, I didn't verify it in the Sun Studio IDE (the real user of this fix). The last 10% can't be done until this patch is backported to NB 5.5.1 and tested in the Sun Studio IDE (either by Thomas Preisler or Ivan Suleimanipour). Created attachment 38067 [details]
Verification files for Jarda's patch (add-on to cnd)
I just attached the verification files I used to test Jarda's patch. I tested it in cnd but only because the real test environment (Sun Studio IDE) doesn't work with nb60 development builds. This tarball is mostly to keep a permanent copy of my verification environment (its likely to be used as the starting point of a Sun Studio fix). ; /shared/data/ccvs/repository/openide/text/apichanges.xml,v <-- apichanges.xml new revision: 1.17; previous revision: 1.16 done Checking in manifest.mf; /shared/data/ccvs/repository/openide/text/manifest.mf,v <-- manifest.mf new revision: 1.15; previous revision: 1.14 done RCS file: /shared/data/ccvs/repository/openide/text/test/unit/src/org/openide/text/CloneableEditorSupportRedirectorTest.java,v done Checking in test/unit/src/org/openide/text/CloneableEditorSupportRedirectorTest.java; /shared/data/ccvs/repository/openide/text/test/unit/src/org/openide/text/CloneableEditorSupportRedirectorTest.java,v <-- CloneableEditorSupportRedirectorTest.java initial revision: 1.1 done Checking in src/org/openide/text/CloneableEditorSupport.java; /shared/data/ccvs/repository/openide/text/src/org/openide/text/CloneableEditorSupport.java,v <-- CloneableEditorSupport.java new revision: 1.29; previous revision: 1.28 done RCS file: /shared/data/ccvs/repository/openide/text/src/org/openide/text/CloneableEditorSupportRedirector.java,v done Checking in src/org/openide/text/CloneableEditorSupportRedirector.java; /shared/data/ccvs/repository/openide/text/src/org/openide/text/CloneableEditorSupportRedirector.java,v <-- CloneableEditorSupportRedirector.java initial revision: 1.1 I'm reopening as this issue is a Mars requirement and per aggreement between my group and Honza/Tonda, it was first fixed in the trunk and will then get backported to 5.5.1. If you (Jarda) aren't the person who will do the backport, please reassign to the correct person. We can't do the final verification until we can use the fix in the Sun Studio IDE, so we need it in 5.5.1. Do not cause mess in IZ, please. The problem is fixed in trunk. That means the status is FIXED and target milestone is 6.0M7. If it will be integrated into 5.5.1, its target milestone will be changed to 5.5.1. But it will be backported to 5.5.1, correct? Yes, I've already seen a pre-integration announcement on the reviewers@netbeans.org alias. Backported to 5.5.1: /cvs/openide/text/manifest.mf,v <-- manifest.mf new revision: 1.8.4.1.2.3.6.1; previous revision: 1.8.4.1.2.3 done Checking in apichanges.xml; /cvs/openide/text/apichanges.xml,v <-- apichanges.xml new revision: 1.9.4.1.2.1.22.1; previous revision: 1.9.4.1.2.1 done Checking in src/org/openide/text/CloneableEditorSupport.java; /cvs/openide/text/src/org/openide/text/CloneableEditorSupport.java,v <-- CloneableEditorSupport.java new revision: 1.6.6.1.2.2.10.1; previous revision: 1.6.6.1.2.2 done Checking in src/org/openide/text/CloneableEditorSupportRedirector.java; /cvs/openide/text/src/org/openide/text/CloneableEditorSupportRedirector.java,v <-- CloneableEditorSupportRedirector.java new revision: 1.2.2.1; previous revision: 1.2 done Checking in test/unit/src/org/openide/text/CloneableEditorSupportRedirectorTest.java; /cvs/openide/text/test/unit/src/org/openide/text/CloneableEditorSupportRedirectorTest.java,v <-- CloneableEditorSupportRedirectorTest.java new revision: 1.1.2.1; previous revision: 1.1 done /cvs/nbbuild/javadoctools/apichanges.dtd,v <-- apichanges.dtd new revision: 1.3.36.2.30.1; previous revision: 1.3.36.2 Great. Thanks. Shouldn't it be marked with keyword '551_HR_FIX'? |