Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 264878

Summary: Implement groups using a linked resource with special location
Product: [Eclipse Project] e4 Reporter: John Arthorne <john.arthorne>
Component: ResourcesAssignee: John Arthorne <john.arthorne>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: jamesblackburn+eclipse, serge, sergebeauchamp
Version: unspecified   
Target Milestone: 0.9 M2   
Hardware: PC   
OS: Windows XP   
Whiteboard:
Attachments:
Description Flags
Patch none

Description John Arthorne CLA 2009-02-13 13:25:22 EST 
I finally got around to reviewing the implementation of resource groups in detail today. Most of the handling of groups seemed to be straight copies of the code for handling linked resources, with some minor variances. This isn't surprising, because a group is identical to a linked resource to a childless folder in a non-local file system. Generally this kind of duplication is bad because it results in two places to make the same fixes whenever the behaviour needs to change. More importantly, the behaviour between links and groups was not entirely consistent, and some of the special case code we have for handling links didn't have the corresponding code for groups. These are corner cases that we have discovered over the years with links, but presumably haven't stumbled over yet with groups.

There were some other surprising behaviours:

 - isLocal() was returning false for groups. The entire isLocal concept is deprecated, and was never meant to indicate "local file system" versus remote/other file system. Note that isLocal()==false indicates the resource's metadata (properties) also aren't modifiable. I think the right behaviour for isLocal() on a group is "true". 

 - IResource#move is by default a "deep" move. It manifests links in the source as concrete files in the destination. The intent here is that "deep move" converts a logical file system hierarchy containing links into a simple physical hierarchy. I think "deep" move should have the same semantics for groups as it does for links, but the current implementation is maintaining groups.

Based on these observations, I made an attempt this morning to re-implement groups using plain linked resources under the covers. There is still IFolder#createGroup and isGroup() API for clients, but the implementation just uses links with a special URI. Overall I found the result much cleaner, and I was able to delete quite a lot of code and keep the same behaviour (all tests passing), which is always a good sign. I will attach a patch here for Serge and others to take a look at.
Comment 1 John Arthorne CLA 2009-02-13 13:29:51 EST 
Created attachment 125669 [details]
Patch
Comment 2 Serge Beauchamp CLA 2009-02-13 14:44:33 EST 
Hi John, 

I think your idea is great and it certainly looks like a better implementation of the group feature than the one I provided, given it reduce significantly code duplication.
Comment 3 John Arthorne CLA 2009-02-13 15:43:54 EST 
I think it will also help with compatibility if isLinked() returns true for groups. Think of groups like a special kind of link rather than a special kind of folder... For example I noticed the CVS views seem to be broken today with groups, causing various NullPointerExceptions because the location is null. However, since they already check for and handle links, with this change they are no longer broken.
Comment 4 John Arthorne CLA 2009-02-22 23:01:20 EST 
Patch released to HEAD.
Comment 5 James Blackburn CLA 2009-03-03 10:35:37 EST 
(In reply to comment #0)
>  - IResource#move is by default a "deep" move. It manifests links in the source
> as concrete files in the destination. The intent here is that "deep move"
> converts a logical file system hierarchy containing links into a simple
> physical hierarchy. I think "deep" move should have the same semantics for
> groups as it does for links, but the current implementation is maintaining
> groups.

How does this relate to moving resources in the Navigator / Project Explorer?  When I move or rename a folder of linked resources, the linked resource within remain as links.  Similarly if I move a linked resource itself, it's the link that's moved rather than deep copying the linked destination in the destination.

This is what I intuitively expect to happen when I perform a move operation, and doesn't seem to match your description of IResource#move().  Am I missing something?
Comment 6 Serge Beauchamp CLA 2009-03-03 10:39:54 EST 
(In reply to comment #5)
> (In reply to comment #0)
> >  - IResource#move is by default a "deep" move. It manifests links in the source
> > as concrete files in the destination. The intent here is that "deep move"
> > converts a logical file system hierarchy containing links into a simple
> > physical hierarchy. I think "deep" move should have the same semantics for
> > groups as it does for links, but the current implementation is maintaining
> > groups.
> 
> How does this relate to moving resources in the Navigator / Project Explorer? 
> When I move or rename a folder of linked resources, the linked resource within
> remain as links.  Similarly if I move a linked resource itself, it's the link
> that's moved rather than deep copying the linked destination in the
> destination.
> 
> This is what I intuitively expect to happen when I perform a move operation,
> and doesn't seem to match your description of IResource#move().  Am I missing
> something?
> 

The UI code passes the IResource.SHALLOW flag when moving and copying files in the navigator, that's why it behaves like it does.
Comment 7 James Blackburn CLA 2009-03-03 10:58:49 EST 
(In reply to comment #6)
> The UI code passes the IResource.SHALLOW flag when moving and copying files in
> the navigator, that's why it behaves like it does.

Thanks Serge -- I skimmed over the overloading of 'updateFlags'! 

Since IResource#move(...) takes a workspace relative path and not a location (so the destination can never be out of the workspace),  I can't think of an instance when I would want a move to perform this kind of deep move.  Certainly moving the underlying thing pointed to by the link seems wrong.  Why would the user have chosen to use a linked resource if this was what they wanted...?  

I can see the use during a deep copy, but not for move.  Analagosly, unix cp has an option to deep copy sym links (-L) (which isn't the default) but there is no similar option for mv. It seems weird that this behaviour is default...

Is there some use case I'm missing?
Comment 8 John Arthorne CLA 2009-03-03 11:06:15 EST 
See bug 26933 for some history. The main reason for this is that when links were introduced they could only exist as direct children of a project. Thus a shallow copy/move to any other path would fail. Therefore all existing clients were forced to adapt to this situation and handle this new kind of failure. Deep copy/move always succeeds because it doesn't need to create links. Now that links can exist at any depth, this default behaviour doesn't make as much sense. Alas, we are stuck with the API we have (switching the default would certainly break all existing clients).
Comment 9 James Blackburn CLA 2009-03-03 11:12:24 EST 
(In reply to comment #8)
> See bug 26933 for some history. ...

Ah thanks for the context, I understand what's done is done! I guess I was shocked as it's something I hadn't realised and I've been using Linked Resources a fair bit ...