Bug 265125 - [Linked Resources] MonitorManager doesn't handle linked resources which aren't direct children of IProject
Summary: [Linked Resources] MonitorManager doesn't handle linked resources which aren'...
Status: NEW
Alias: None
Product: Platform
Classification: Eclipse Project
Component: Resources (show other bugs)
Version: 3.5   Edit
Hardware: All All
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Platform-Resources-Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-02-17 05:41 EST by James Blackburn CLA
Modified: 2019-09-06 16:04 EDT (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description James Blackburn CLA 2009-02-17 05:41:00 EST
Build ID: HEAD

Steps To Reproduce:
MonitorManager's handling of Linked Resources looks odd.  It only registers RefreshMonitors for Linked Resources directly contained in the Project. See MonitorManager#getResourcestoMonitor()

Presumably this is from the time when Linked Resources could only exist in the Project's root.

Interestingly the MonitorManager#visit(...) IResourceDeltaVisitor() registers any added linked IResource to the set of resources to be monitored.
Comment 1 Szymon Brandys CLA 2009-02-17 06:29:00 EST
(In reply to comment #0)
> Build ID: HEAD
> 
> Steps To Reproduce:
> MonitorManager's handling of Linked Resources looks odd.  It only registers
> RefreshMonitors for Linked Resources directly contained in the Project. See
> MonitorManager#getResourcestoMonitor()

I'm not following you here. What is the change that you suggest?

> Presumably this is from the time when Linked Resources could only exist in the
> Project's root.

Can linked resources exist somewhere else?

Comment 2 James Blackburn CLA 2009-02-17 06:39:29 EST
(In reply to comment #1)
> > Presumably this is from the time when Linked Resources could only exist in the
> > Project's root.
> 
> Can linked resources exist somewhere else?

Linked resources can exist at any level in the workspace tree. This was fixed in 3.2 bug73695, I believe.
Comment 3 Szymon Brandys CLA 2009-02-17 07:18:05 EST
(In reply to comment #2)
> (In reply to comment #1)
> > > Presumably this is from the time when Linked Resources could only exist in the
> > > Project's root.
> > 
> > Can linked resources exist somewhere else?
> 
> Linked resources can exist at any level in the workspace tree. This was fixed
> in 3.2 bug73695, I believe.
> 

Only projects, folders and files can be linked resources. See IResource#isLinked and WorkspaceRoot#isLinked().

Regarding MonitorManager#getResourcesToMonitor(), we add all accessible projects to resourcesToMonitor anyway, so we don't check whether they are linked or not.


Comment 4 James Blackburn CLA 2009-02-17 07:33:56 EST
(In reply to comment #3)
> Only projects, folders and files can be linked resources. See
> IResource#isLinked and WorkspaceRoot#isLinked().

Well strictly speaking only non-Project IResources will return true for IResource#isLinked()

> Regarding MonitorManager#getResourcesToMonitor(), we add all accessible
> projects to resourcesToMonitor anyway, so we don't check whether they are
> linked or not.

That's not quite true, you appear to add all direct children of the IProject if they are linked:
private List getResourcesToMonitor() {
...
for (int i = 0; i < projects.length; i++) {
	if (!projects[i].isAccessible())
		continue;
	resourcesToMonitor.add(projects[i]);
	try {
		IResource[] members = projects[i].members();
		for (int j = 0; j < members.length; j++)
			if (members[j].isLinked())
				resourcesToMonitor.add(members[j]);
	} catch (CoreException e) {
		Policy.log(IStatus.WARNING, Messages.refresh_refreshErr, e);
	}
}
...
}

I presume this is because, pre-3.2, linked resources could only exist directly under the IProject (this code seems to date from 3.0).  Linked resources which are deeper than this (i.e. children of IFolders) aren't added in this way.

I was just looking over the Refresh Monitor stuff, and noticed this inconsistency.  I would guess the right thing to do would be to #monitor(...) all linked resources in the workspace.  This may start to get expensive if users have lots of linked resources (for example by using Serge's (more-)flexible resources patches)?

Am I wrong on this?
Comment 5 Szymon Brandys CLA 2009-02-17 08:18:49 EST
(In reply to comment #4)
> Well strictly speaking only non-Project IResources will return true for
> IResource#isLinked()

Right. I was misled by the #isLink javadoc

* Returns whether this resource has been linked to
* a location other than the default location calculated by the platform. 

> I was just looking over the Refresh Monitor stuff, and noticed this
> inconsistency.  I would guess the right thing to do would be to #monitor(...)
> all linked resources in the workspace. 

If the code doesn't consider linked resources other that just direct children of projects, it can mean that it fails for those cases. James, I assume that you just inspected the code and did no other tests. Are you willing to do some extra effort and do these tests?

Comment 6 James Blackburn CLA 2009-02-17 08:38:18 EST
(In reply to comment #5)
> If the code doesn't consider linked resources other that just direct children
> of projects, it can mean that it fails for those cases. James, I assume that
> you just inspected the code and did no other tests. Are you willing to do some
> extra effort and do these tests?

Yes, I think this is just unhandled currently. [It may of course be that #isSynchronized & #refreshLocal do the right thing when they hit linked resources in which reduces the severity of this bug for users using the polling refresh monitor).

There are a bunch of Refresh / Sync issues which I've discovered, or are present in the ISV interface to core.resources which I'll try to formalize into bugs over the next day or so.  It recently came up on the CDT Dev list as the code is sprinkled with refreshLocal()s when we run external tools, this is really painful for some users -- and is especially hard for us, as while we want to keep the resource tree in sync, we don't really care that the sync happens immediately.

I'm more than happy to create tests and propose fixes if you guys can review & comment :). (I know I've got a few outstanding tests and patches for existing bugs. I'll look over those patches and perhaps you can review them too Szymon?)
Comment 7 Szymon Brandys CLA 2009-02-17 08:54:01 EST
(In reply to comment #6)
> I'm more than happy to create tests and propose fixes if you guys can review &
> comment :). (I know I've got a few outstanding tests and patches for existing
> bugs. I'll look over those patches and perhaps you can review them too Szymon?)

Sure. 

Just to mention, there are two issues raised to create refresh native support for Linux and Mac OS. Fixing them would be the best solution IMO. This is probably not 3.5 thing, but I would consider to work on it during 3.6. I'm looking for volunteers to help.

Comment 8 James Blackburn CLA 2009-02-17 09:00:07 EST
(In reply to comment #7)
> Just to mention, there are two issues raised to create refresh native support
> for Linux and Mac OS. Fixing them would be the best solution IMO. This is
> probably not 3.5 thing, but I would consider to work on it during 3.6. I'm
> looking for volunteers to help.

Indeed, thanks, those would be ideal going forward.  

As it happens we're stuck on rhel4 here which has a kernel pre-inotify (and we're also using NFS...), so it would be good to attack some of the low-hanging issues we currently see using the Polling Monitor.
Comment 9 Eclipse Webmaster CLA 2019-09-06 16:04:17 EDT
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet.

If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.