Bug 51690 - Two or more editor panes for the same file
Two or more editor panes for the same file
Status: RESOLVED FIXED
Product: platform
Classification: Unclassified
Component: Data Systems
4.x
All All
: P2 (vote)
: 5.x
Assigned To: Jaroslav Tulach
issues@platform
: API_REVIEW_FAST
Depends on:
Blocks: 209745
  Show dependency treegraph
 
Reported: 2004-11-19 21:17 UTC by ivan
Modified: 2012-03-19 15:04 UTC (History)
9 users (show)

See Also:
Issue Type: ENHANCEMENT
:


Attachments
Tar file of diff output + one new Java file (2.04 KB, text/plain)
2007-01-11 01:14 UTC, _ gordonp
Details
Reduced patch. Note that it does no filename-based prefiltering, it is the responsibility of the plugged in comparator. (2.33 KB, patch)
2007-01-29 12:42 UTC, Petr Nejedly
Details | Diff
Change for the openide/text module (24.70 KB, patch)
2007-01-31 16:17 UTC, Jaroslav Tulach
Details | Diff
Verification files for Jarda's patch (add-on to cnd) (14.00 KB, application/octet-stream)
2007-02-05 21:49 UTC, _ gordonp
Details

Note You need to log in before you can comment on or make changes to this bug.
Description ivan 2004-11-19 21:17:32 UTC
We have an issue with FileObjects whereas the same
file accessed through
two different paths produces distinct FileObjects
and distinct
editor tabs and so on.

Best known examples are soft links and
alternative, particularly
/net mount points. For example if I try to open
/export/home/t.c
and /net/djomolungma/export/home/t.c I get
distinct FOs.

To be more mathematical the relation between File
and FileObject is
many to one. FileUtil.toFile() fails to account
for this.

Why is this important?
Simply said in our experience with Sun Workshop
(and with dbx)
it becomes important to not present two of the
same file to the user.

    For example if your make cd's to
/net/djomolungma/export/ and
    compiles, a net relative path will appear in
the debugging
    information, while in casual use users might
not realise this and
    try to open /export/home/t.c, play with it and
then when they debug
    it the debugger instructs the GUI to open
    /net/djomolungma/export/home/t.c. The result
is two editor tabs.

    For another example, dbx has had for many
years a command called
    pathmap.  I have the following in my .dbxrc:
            pathmap /export/home
/net/djomolungma/export/home
            # maps all file paths of the for
/export/... to
            # /net/...
    so I can compile with umpunity using whatever
PWD and can access
    the files equally regardless of whether I"m
debugging on my machine
    or on a remote machine.

    The situations that we've seen people run into
are astounding.
    If you have a Sun dbx handy start it up and issue
        (dbx) help finding-files
    I repeat, most of these are not hypothetical
concerns but actual
    customer usage patterns.

In short this is important in all circumstaces
where filenames
originate from the tools as opposed to explicitly
by the users.
Presumably distributed build systems might run
into this as well.

vim and emacs have schemes to make sure that you
get one buffer for
files opened with different paths. AFAIK these
schemes are based on
leaving a backing file (in the case of vim) and
using that for
clash detection. However I tried write protecting
the directory and
vim still correctly associated a /net accessed and
a locally
accessed file.

I'd like to make this non-duplication of
FileObjects be a requirement
for 4.1.

Initially I'm more interested in a formal
specification of what exactly
this requirement should be. Then we can deleve
into implementation issues.
        For example ...
        - should we only deal with mount point
ambiguity or are
          soft links also relevant.
        - Should fileObject factories have a flag
which controls whether
          identity checks are performed?
        - Should FileObject.getPath() start
returning an array of Strings?
        - Should there be a preferred path? (for
example to display in
          an editor tab)
        - Will this work with various file views?
(It should in
          principle nodes, note Nodes, of the tree
view should be able to
          point to a shared model).
          Will the name of the FO in a view be
extracted from the FO?
          If so what happens if the same file
appears in two views?
        - I"m concerned that whatever we decide
now might become reather
          irrelevant once we start looking into
distributed development.
          For example, my intuition tells me that
we might want to have a
          filter analogous to pathmap installed in
MasterFS mapping
          machinery.
        - relationship to URLMapper ... this goes
way beyond my NB
          expertise.

This wasn't a big issue for us in the past because
with our own
homebrew automounting in the cpp module Tor had
manged to implement
something reasonable. Now with everything hidden
inside MasterFS the
burden falls on core.
Comment 1 ivan 2005-01-03 21:19:54 UTC
This issue has been ignored in the MasterFS rewrite, so
bubbling it up.
Comment 2 _ gordonp 2005-10-05 00:46:48 UTC
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.
Comment 3 rmatous 2006-10-16 10:50:01 UTC
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.


Comment 4 ivan 2006-10-16 20:40:16 UTC
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.
Comment 5 rmatous 2006-10-18 11:06:52 UTC
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.
Comment 6 ivan 2006-10-18 20:41:45 UTC
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.


Comment 7 _ gordonp 2006-10-19 00:35:45 UTC
> 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.
Comment 8 rmatous 2006-10-19 14:25:07 UTC
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. 



Comment 9 _ gordonp 2006-10-25 21:41:13 UTC
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.
Comment 10 rmatous 2006-10-30 10:32:50 UTC
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.

Comment 11 Petr Nejedly 2007-01-09 17:16:18 UTC
Gordon,
can you please provide the diff of your changes?
Comment 12 _ gordonp 2007-01-11 01:14:10 UTC
Created attachment 37249 [details]
Tar file of diff output + one new Java file
Comment 13 _ gordonp 2007-01-11 01:16:57 UTC
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.
Comment 14 Petr Nejedly 2007-01-18 15:40:48 UTC
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?
Comment 15 Petr Nejedly 2007-01-26 14:58:02 UTC
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).
Comment 16 Petr Nejedly 2007-01-29 12:39:53 UTC
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).
Comment 17 Petr Nejedly 2007-01-29 12:42:01 UTC
Created attachment 37771 [details]
Reduced patch. Note that it does no filename-based prefiltering, it is the responsibility of the plugged in comparator.
Comment 18 _ gordonp 2007-01-31 01:14:28 UTC
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).
Comment 19 Jaroslav Tulach 2007-01-31 16:16:08 UTC
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.
Comment 20 Jaroslav Tulach 2007-01-31 16:17:18 UTC
Created attachment 37884 [details]
Change for the openide/text module
Comment 21 Jaroslav Tulach 2007-02-04 07:31:25 UTC
When I can expect confirmation that my patch is on the right way to solve the 
problem?
Comment 22 _ gordonp 2007-02-04 19:00:08 UTC
> 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.
Comment 23 Jaroslav Tulach 2007-02-04 20:27:56 UTC
Excellent, I'll get things ready to integrate on Thursday Feb 8, 2007.
Comment 24 _ gordonp 2007-02-05 19:22:52 UTC
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).
Comment 25 _ gordonp 2007-02-05 21:49:12 UTC
Created attachment 38067 [details]
Verification files for Jarda's patch (add-on to cnd)
Comment 26 _ gordonp 2007-02-05 21:51:15 UTC
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).
Comment 27 Jaroslav Tulach 2007-02-08 14:34:51 UTC
;
/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
Comment 28 _ gordonp 2007-02-08 16:16:02 UTC
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.
Comment 29 Jaroslav Tulach 2007-02-08 16:41:35 UTC
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.
Comment 30 Thomas Preisler 2007-02-08 23:28:10 UTC
But it will be backported to 5.5.1, correct?
Comment 31 Jan Chalupa 2007-02-08 23:38:56 UTC
Yes, I've already seen a pre-integration announcement on the 
reviewers@netbeans.org alias.
Comment 32 Jaroslav Tulach 2007-02-09 11:39:27 UTC
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
Comment 33 Thomas Preisler 2007-02-13 05:09:05 UTC
Great. Thanks. Shouldn't it be marked with keyword '551_HR_FIX'?


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo