Community
Participate
Working Groups
On Dstore/SSH/FTP if you expand a folder under a filter and refresh the expanded folder the Filter is refreshed and selection changes to the filter. Steps to Reproduce: 1. Create a dstore connection. 2. Expand My Home. 3. Expand a folder under My Home. 4. Select the expanded folder. 5. Press F5. --> Filter is refreshed. -----------Enter bugs above this line----------- TM 3.0RC1 Testing installation : eclipse-SDK-3.4M7 RSE install : Dev Driver java.runtime : Sun 1.5.0_11-b03 os.name: : Windows XP, Service Pack 2 ------------------------------------------------
Kevin, this is a dup of a long known issue -- I'm sure you can find the bug easily.
I thought so as well, but couldn't find it. It must be in some old bug instead of being in its own separate bug.
Hm... related bugs are bug 187739 and bug 196025 but that's not quite what I was looking for... I think I had bug 191724 in mind. This one is marked as a duplicate of bug 187739, but I think that's not entirely true since it's not exactly the same issue. Anyways, thanks for filing the new bug, I agree that the issue of losing current selection on refresh is one of the major annoyances with RSE right now and we should try hard to get it fixed for 3.0
I don't think it's necessary for the filter to be expanded in order to reproduce this. One thing that happens with the refresh, is that I think we need to refresh the filter when the folder changes because (at least for non-dstore) a new fetch is needed to pick up a remote file with the changed attributes. In the case where the subject folder has been deleted on the host, we also need to refresh the filter in order to show that it's no longer there. I guess the key is to try to deal with these situations without involving the entire contents of the filter.
One thing I've noticed here is that, if the folder is expanded, the filter is refreshed AND the selection changes to the filter. If the folder is not expanded, the filter is refreshed but the selection doesn't change. Note that I'm seeing this for SSH and FTP but not for dstore right now.
Bug 191275 might be a duplicate of this
One idea that I've been having for this is, what if instead of refreshing the filter to get "folder foo" properties we did the equivalent of ls -a foo and instead of just remembering foo's children we also process the "." entry which stands for the current directory on all OS's that I know. In other words, querying the children of Foo would also query Foo itself automatically. Does such an approach scale to other kinds of resources? Is a single query that returns both Foo's children along with Foo itself also work on other kinds of subsystems?
(In reply to comment #7) > One idea that I've been having for this is, what if instead of refreshing the > filter to get "folder foo" properties we did the equivalent of > ls -a foo > and instead of just remembering foo's children we also process the "." entry > which stands for the current directory on all OS's that I know. In other words, > querying the children of Foo would also query Foo itself automatically. > Does such an approach scale to other kinds of resources? Is a single query that > returns both Foo's children along with Foo itself also work on other kinds of > subsystems? When you say "querying the children of Foo would also query Foo itself automatically", are you referring to the query at the service layer or something in the tree, itself? I believe dstore does update the subject of the query on a query of it's children automatically. For SSH and FTP, we don't reuse the IRemoteFile object (we have to fetch a new one) and that means we'd need to replace the parent tree item after the results of the query come back. I'm not sure whether that will be tricky or not but maybe we could phase this. As a first step, I'm thinking we might be best to not refresh the parent filters of a refreshed object on the refresh remote event. By not doing that, a user would have to explicitly refresh the parent filter in order to ensure the correct properties for the selected object but at least we wouldn't be messing up the selection and collapsing things.
(In reply to comment #8) > When you say "querying the children of Foo would also query Foo itself > automatically", are you referring to the query at the service layer or I basically thought about a change in API Semantics of IFileService#list() that it MUST return an IHostFile object for the parent itself, along with IHostFile objects for the children. We could force it to return the parent's IHostFile object first in the list. That way, we could ensure that we get correct Properties of the parent along with the children in a single server round-trip. It's a change in API semantics but perhaps now is the right time to do this, or we are hosed with this issue forever... Thinking about this again, if we do this for IFileService we'd likely have to do it for any kind of service, since getting children of a Process is similar... which would make the change quite pervasive... > As a first step, I'm thinking we might be best to not refresh the parent > filters of a refreshed object on the refresh remote event. Hm... odd thing is that the issue is not seen when the folder in question is collapsed... I'm afraid that when user has a folder selected and asks for Refresh (manually!) on that selection, he'd expect the selection to be refreshed... we've had such a bug, I think. But perhaps it's wrong doing a filter query in that case; perhaps we should make an IFileService query on the parent folder of the item in question... I'm afraid that just not refreshing the (parent of selection) is not the right thing to do. We should better keep the refreshes that are done now, but make sure that they don't mess up the selection and expand state.
(In reply to comment #9) > (In reply to comment #8) > > When you say "querying the children of Foo would also query Foo itself > > automatically", are you referring to the query at the service layer or > I basically thought about a change in API Semantics of IFileService#list() that > it MUST return an IHostFile object for the parent itself, along with IHostFile > objects for the children. We could force it to return the parent's IHostFile > object first in the list. That way, we could ensure that we get correct > Properties of the parent along with the children in a single server round-trip. > It's a change in API semantics but perhaps now is the right time to do this, or > we are hosed with this issue forever... > Thinking about this again, if we do this for IFileService we'd likely have to > do it for any kind of service, since getting children of a Process is > similar... which would make the change quite pervasive... That's what I'm worried about - a change like this would go well beyond just the IFileService since anything that gets children would need to change to return a parent along with children. So I'm opposed to something like that. I'd prefer an approach where the original object (i.e. IRemoteFile) would change in response to a query, such we do for LocalFile and DStoreFile. That way we're not changing the API (although we'd have to enforce this in the docs).
(In reply to comment #10) > I'd prefer an approach where the original object (i.e. IRemoteFile) would > change in response to a query, such we do for LocalFile and DStoreFile. That > way we're not changing the API (although we'd have to enforce this in the > docs). That's fine on the Subsystem Layer, but how would you get the necessary information from the Service Layer? Perform two separate queries by default, a getFile() query and a list() query? These could be optimized, by forcing service layer list() to return the parent along with the children. Or, perhaps not force it: the subsystem could be intelligent enough to check the returned children, and if the parent is in the list it doesn't need to do the separate getFile() otherwise it does it. Such an approach would likely isolate the change to the Service layer only; make it backward compatible, but allow Services to do optimizations; and allow each Service / Service type to do those optimizations on its own. How does that sound? What we need to do is (1) Change ISubSystem (and probably more) API Docs at the right places to indicate that getChildren() also needs to update the parent; and, (2) write the implementation which gets the data as needed.
(In reply to comment #11) > (In reply to comment #10) > > I'd prefer an approach where the original object (i.e. IRemoteFile) would > > change in response to a query, such we do for LocalFile and DStoreFile. That > > way we're not changing the API (although we'd have to enforce this in the > > docs). > That's fine on the Subsystem Layer, but how would you get the necessary > information from the Service Layer? For the dstore stuff, we get the information as a side-effect of the query since the underlying DataElement that is wrappered by the IRemoteFile that is the subject of the query is updated by the miner. I guess the key thing in that case, is that a service layer object referenced by IRemoteFile is updated in the service. For FTP and SSH, one approach may be to cache the original IHostFile (that is referenced by IRemoteFile) and update it accordingly after a query on it's path is made.
I'm generally against tampering with IHostFile objects, since modifying them changes their hashCode() / equals() behavior, and they are partially stored / used as index in Maps such as the ElementMap. I'd feel much more comfortable if IHostFile objects were always immutable. Just think about Threading issues. Lots of other components / Threads can be reading the "old" IHostFile while the Service is modifying it. Such behavior asks for trouble. I think there must be a clear update / notification Strategy, such that all interested parties know when an element is modified or deleted (remember the troubles that dstore IHostFile objects cause us when they are deleted). On the other hand, dstore does mutate its IHostFile objects; but, since our IFileService passes parentPath + name in as Strings, rather than using IHostFile already on this layer, doing a separate cache for this on the Service layer seems a slippery road. Finally, in terms of API Javadoc I find it very hard to explain that a method such as list() should have a particular side-effect on a particular object that is not referenced in any parameter or return value but that the Service is supposed to just "have cached".
So you are okay with modifying the IRemoteFile at the subsystem layer (i.e. replacing the underlying IHostFile with a new one) when a query comes in? For files, would we want to change RemoteFileSubSystem.internalResolveOneFilterString() or do it within FileServiceSubSystem.list(). While list() doesn't seem like the most intuitive place to update an IRemoteFile, RemoteFileSubSystem isn't assumed to have services, so it can't directly get at an IHostFile.
Created attachment 102743 [details] patch to update the parent remote file as well during a query Here is one simple way that we could go about this. Martin, what do you think of this approach?
Created attachment 102745 [details] patch to not refresh filters on refresh remote event I've added this patch so you can see what happens if we dont' refresh the filters on refresh remote events.
Ok, some comments: * The basic approach seems viable, although I'd like to see the small Performance improvement that we can get if the Service is free to return the updated parent as a side-effect of the list() call such that we can avoid the separate getFile() call. Since that implementation would conceptually remain the same but just be a Performance improvement, we could just mention the possibility in the API Docs for now but not actually implement the Performance improvement. * The actual IHostFile object should not be replaced in the IRemoteFile if it has the same properties already -- should make an equals() check before replacing it * If the IRemoteFile is stale at the time you get the new IHostFile in, shouldn't we mark it as no longer stale in order to avoid yet another query? Actually, what about this strategy: 1. Mark parent stale in SystemView 2. Perform list() query and only if stale, get new IHostFile as side-effect 3. If go the new IHostFile, mark IRemoteFile as not-stale This would keep the plain API more backward compatible. * If the IHostFile is actually replaced inside the IRemoteFile, shouldn't we fire a notification event for interested parties? Assume, for instance, that somebody wrote a Filter which shows all folders modified "today". The list query could find out that a folder's modified date changed, as side effect. Now the filter needs to be refreshed... or is this too complex? * Rather than duplicating the new logic 3 times, it should be in one common place (new method) * Your 2nd patch (SystemView) includes some LabelProvider stuff that should not be in there; and, it unnecessarily re-computes the filter references before the code that you commented out. Also, this change is global for ALL subsystems but we change only FILES to update the parent as side-effect. * IRemoteFile Javadoc does not have an @noimplement clause, so technically you are not allowed to cast to AbstractRemoteFile since extenders could contribute IRemoteFile implmenetations that do not extend AbstractRemoteFile. I'd think that IRemoteFile Javadoc MUST BE UPDATED independent of this concrete issue! --> Bug 234726
(In reply to comment #17) > Ok, some comments: > * The basic approach seems viable, although I'd like to see the small > Performance improvement that we can get if the Service is free to return > the updated parent as a side-effect of the list() call such that we can > avoid the separate getFile() call. Having the service return the updated parent in the same list as the children seems a little unintuitive to me. It makes me think that we'll have consumers that use the service treating the parent as a child of the parent. > Since that implementation would conceptually remain the same but just be a > Performance improvement, we could just mention the possibility in the API > Docs for now but not actually implement the Performance improvement. > * The actual IHostFile object should not be replaced in the IRemoteFile if it > has the same properties already -- should make an equals() check before > replacing it To do this we'll need to make sure that all implementations of IHostFile provide an equals() method that checks all the properties. > * If the IRemoteFile is stale at the time you get the new IHostFile in, > shouldn't we mark it as no longer stale in order to avoid yet another > query? > Actually, what about this strategy: > 1. Mark parent stale in SystemView > 2. Perform list() query and only if stale, get new IHostFile as side-effect > 3. If go the new IHostFile, mark IRemoteFile as not-stale > This would keep the plain API more backward compatible. This seems reasonable. > * If the IHostFile is actually replaced inside the IRemoteFile, shouldn't we > fire a notification event for interested parties? Assume, for instance, > that somebody wrote a Filter which shows all folders modified "today". > The list query could find out that a folder's modified date changed, > as side effect. Now the filter needs to be refreshed... or is this too > complex? This seems a little complex to me. What kind of event did you have in mind? Something new? > * Rather than duplicating the new logic 3 times, it should be in one common > place (new method) > * Your 2nd patch (SystemView) includes some LabelProvider stuff that should > not be in there; and, it unnecessarily re-computes the filter references > before the code that you commented out. Yeah, the label provider stuff shouldn't be in the patch. I kept the filter reference check in there because we still have an issue where an unexpanded item won't actually get refreshed if it's parent is a filter. I'm not sure how to deal with this - I was considering doing an expand on refresh but that's probably not what we want so we'll need to have some other way of indicating that unexpanded items under filters should be refreshed. > Also, this change is global for > ALL subsystems but we change only FILES to update the parent as side-effect. > * IRemoteFile Javadoc does not have an @noimplement clause, so technically > you are not allowed to cast to AbstractRemoteFile since extenders could > contribute IRemoteFile implmenetations that do not extend AbstractRemoteFile. > I'd think that IRemoteFile Javadoc MUST BE UPDATED independent of this > concrete issue! --> Bug 234726 I guess IRemoteFile would be the better place to introduce the setHostFile(IHostFile) api..since we already have a getHostFile() api there.
(In reply to comment #18) > Having the service return the updated parent in the same list as the children > seems a little unintuitive to me. It makes me think that we'll have consumers > that use the service treating the parent as a child of the parent. I'm not too concerned about this. Who is actually consuming IFileService#list() directly? It's mostly the FileServiceSubSystem, which is ours; consumers should better sit on the subsystem than on the Service. And for those who consume the Service directly, we could return the parent as a file named "." for instance -- right now, we are explicitly filtering these entries out in ssh and ftp, the client could just do the same. In the end, clients are forced to obey our API's which we are changing in 3.0 anyways, so I'm not at all concerned here. > To do this we'll need to make sure that all implementations of IHostFile > provide an equals() method that checks all the properties. I think this would make sense. What's important that we really understand the consistent overall Story of how Service and Subsystem are intended to work. In my opinion, it works like this: * On Subsystem Layer, we have IRemoteFile as "guaranteed Unique" handles. A client who obtains a handle can know that other clients, who want a handle for the same file, actually get the same handle. The Subsystem does this by its caching mechanism. Having "unique handle" makes sure that if ONE client updates a file, ALL clients benefit from the updated data. * On Service Layer, we do not want each Service to implement caching itself. Services should be simple to implement, such that the Framework (in Subsystem) can do the bulk of the work. Therefore I'm generally against caching yet again here, since the subsystem caches already. Although the APIs do allow caching since they use a "factory" pattern where the IHostFile objects area always produced by the Service... dstore makes use of this, but I still think Services should be simple and this does not involve any caching. Both strategies have advantages -- "not caching" on the Service layer means less Threading problems and simpler implementation; "caching" on the Subsystem layer means added value at the cost of caching logic and threading needs. I think it's good the way it is, with Subsystem adding value to the Service. > > * If the IHostFile is actually replaced inside the IRemoteFile, shouldn't we > > fire a notification event for interested parties? > > This seems a little complex to me. What kind of event did you have in mind? > Something new? Forget about this for now... that's a different story. Actually, if our Subsystem Story is really having ONE unique handle per remote file, we could even invent a listener mechanism where clients subscribe directly to the IRemoteFile "notify me if you change"... but, again, that's a different story and something for the future. > I guess IRemoteFile would be the better place to introduce the > setHostFile(IHostFile) api..since we already have a getHostFile() api there. NO! that would be totally wrong in my opinion, since one client could then break all other clients by setting a wrong IHostFile into the handle... no, setting the IHostFile must be an operation that's reserved for the Framework only, clients are not allowed to change their handles that way. Allowing clients change the handle would totally break my understanding of how the Subsystem works. Adding @noimplement to IRemoteFile is definitely the better solution.
(In reply to comment #19) > (In reply to comment #18) > > Having the service return the updated parent in the same list as the children > > seems a little unintuitive to me. It makes me think that we'll have consumers > > that use the service treating the parent as a child of the parent. > I'm not too concerned about this. Who is actually consuming IFileService#list() > directly? It's mostly the FileServiceSubSystem, which is ours; consumers should > better sit on the subsystem than on the Service. And for those who consume the > Service directly, we could return the parent as a file named "." for instance > -- right now, we are explicitly filtering these entries out in ssh and ftp, the > client could just do the same. I don't see what's wrong with just putting in the extra call to getFile() in the subsystem. I wouldn't think it's a huge performance hit. In FileServiceSubSystem out getFile() call guarantees that all file service implementations are taken care of. If we ask that file service returns the parent file (as ".") then everyone who implements this will need to change their service to do so - and they're not going to realize that without noticing changed API docs. It also means that each service's implementation of IHostFileToRemoteFileAdapter will need to change in order to handle the additional "." file. If they don't do this then they're going to wonder why the behaviour has changed. Beyond files, other subsystems are going to have to adapt a similar mechanism - of updating their parent object when a query for it's children comes in. I would expect it to be more straight-forward for these to update that parent object directly from the subsystem. If there are no services, then that's going to happen anyway. > > I guess IRemoteFile would be the better place to introduce the > > setHostFile(IHostFile) api..since we already have a getHostFile() api there. > NO! that would be totally wrong in my opinion, since one client could then > break all other clients by setting a wrong IHostFile into the handle... no, > setting the IHostFile must be an operation that's reserved for the Framework > only, clients are not allowed to change their handles that way. Allowing > clients change the handle would totally break my understanding of how the > Subsystem works. > Adding @noimplement to IRemoteFile is definitely the better solution. I don't think a @noimplement makes sense because IRemoteFile is implemented for each service implementation (i.e DStoreFile, FTPRemoteFile, SftpRemoteFile, etc.). If we can't put an API in IRemoteFile, then the FileServiceSubsystem is going to have to assume something about the implementation (for example, that it's an AbstractRemoteFile).
(In reply to comment #20) > I don't see what's wrong with just putting in the extra call to getFile() It's an extra client/server roundtrip and costs performance. > If we ask that file service returns the parent file (as ".") then everyone who > implements this will need to change their service to do so No. We can specify it as optional whether they want to return "." or not -- and our code in FileServiceSubSystem#list() can make your getFile() call if they don't return "." as a fallback. > and they're not going to realize that without noticing changed API docs. But of course we'd change API docs! > It also means that each service's implementation of > IHostFileToRemoteFileAdapter will need to change in order to handle No. The adapter is instantiated from the code in FileServiceSubSystem#list() which takes care of the "." and removes it before the adapter sees it. > Beyond files, other subsystems are going to have to adapt a similar mechanism Yes, and that's why I'm even more worried about Performance and a general applicable concept here. In my opinion, a concept of forcing implementers to properly return the parent along with the children is better, than forcing them to perform obscure side-effects. > If there are no services, then that's going to happen anyway. As I said, I'm ok with this strategy on the Subsystem level but not on the Service level, because in order to be able to make the side-effect, the service would need to do full caching. Or, we pass the parent's IHostFile object into the Service as an additional parameter for the Service to modify. That would be a breaking API change, (but we're breaking stuff already anyways), but a lot clearer. Could look like this, for instance: interface IFileService { public IHostFile[] list(String remoteParent, String fileFilter, int fileType, IHostFile[] parentToReturn, IProgressMonitor monitor) throws SystemMessageException; }; And clients would call it like this: IHostFile[] parentToReturn = new IHostFile[1]; fs.list("/folk/mober", "*", ALL, parentToReturn, monitor); > I don't think a @noimplement makes sense because IRemoteFile is implemented No, I'm talking about forcing clients to extend AbstractRemoteFile > then the FileServiceSubsystem is going to have to assume something about > the implementation (for example, that it's an AbstractRemoteFile). But that's exactly what your code does now, and I think it's OK to force all implementations extend AbstractRemoteFile rather than implementing IRemoteFile without extending the abstract one. No? This pattern of companion interface / abstract base class allows us to extend the interface in the future without breaking backward compatibility, and I find it very important.
(In reply to comment #21) > (In reply to comment #20) > > I don't see what's wrong with just putting in the extra call to getFile() > It's an extra client/server roundtrip and costs performance. > > If we ask that file service returns the parent file (as ".") then everyone who > > implements this will need to change their service to do so > No. We can specify it as optional whether they want to return "." or not -- and > our code in FileServiceSubSystem#list() can make your getFile() call if they > don't return "." as a fallback. Are you saying that the FileServerSubSystem would have to examine the list of files, looking for one file called "." to determine whether or not it needs to to a getFile()? > > and they're not going to realize that without noticing changed API docs. > But of course we'd change API docs! I'm not saying we wouldn't change API docs, I'm saying that those who already having IFileService implementations would not know of these changes without explicitly checking (i.e. there would be no compile errors so they'd think things are working as expected). > > It also means that each service's implementation of > > IHostFileToRemoteFileAdapter will need to change in order to handle > No. The adapter is instantiated from the code in FileServiceSubSystem#list() > which takes care of the "." and removes it before the adapter sees it. Ah, so you're saying the subsystem would augment the list of IHostFiles before passing this down to the adapter for IRemoteFile creation. I guess that could work. I don't mind so long as we handle the case were "." isn't returned (i.e. so the existing services won't be broken without changing). > > Beyond files, other subsystems are going to have to adapt a similar mechanism > Yes, and that's why I'm even more worried about Performance and a general > applicable concept here. In my opinion, a concept of forcing implementers > to properly return the parent along with the children is better, than > forcing them to perform obscure side-effects. How would we force implementers to do this? This is within their subsystem, so it's really up to them to decide how their going to do it. > > If there are no services, then that's going to happen anyway. > As I said, I'm ok with this strategy on the Subsystem level but not on the > Service level, because in order to be able to make the side-effect, the > service would need to do full caching. What I mean is if there is no service layer, then other types of subsystems wouldn't have a service (that returns an extra node for the parent) and therefore would have to provide the code that updates the parent in their subsystem as part of the query anyway. > Or, we pass the parent's IHostFile object into the Service as an additional > parameter for the Service to modify. That would be a breaking API change, > (but we're breaking stuff already anyways), but a lot clearer. Could look > like this, for instance: > interface IFileService { > public IHostFile[] list(String remoteParent, > String fileFilter, > int fileType, > IHostFile[] parentToReturn, > IProgressMonitor monitor) throws SystemMessageException; > }; > And clients would call it like this: > IHostFile[] parentToReturn = new IHostFile[1]; > fs.list("/folk/mober", "*", ALL, parentToReturn, monitor); I thought we wanted to avoid breaking API at this point. We already have future plans of changing these methods to use IHostPath so perhaps we'd be better to make a change like that at the same time. > > I don't think a @noimplement makes sense because IRemoteFile is implemented > No, I'm talking about forcing clients to extend AbstractRemoteFile > > then the FileServiceSubsystem is going to have to assume something about > > the implementation (for example, that it's an AbstractRemoteFile). > But that's exactly what your code does now, and I think it's OK to force all > implementations extend AbstractRemoteFile rather than implementing IRemoteFile > without extending the abstract one. No? Alright, would we want everyone to extend AbstractRemoteFile or RemoteFile? > This pattern of companion interface / abstract base class allows us to extend > the interface in the future without breaking backward compatibility, and I > find it very important.
(In reply to comment #22) > Are you saying that the FileServerSubSystem would have to examine the list of > files, looking for one file called "." to determine whether or not it needs to > to a getFile()? Yes > I'm not saying we wouldn't change API docs, I'm saying that those who already > having IFileService implementations would not know of these changes without > explicitly checking (i.e. there would be no compile errors so they'd think > things are working as expected). Won't hurt since the change is optional for providers; might hurt direct consumers of IFileService > How would we force implementers to do this? This is within their subsystem, > so it's really up to them to decide how their going to do it. Exactly, and they can solve it in any way they like. But if we set the example of how we do it for files (and processes) they can pick it up. As said, important point is that we have a clear overall picture. > What I mean is if there is no service layer, then other types of subsystems > wouldn't have a service (that returns an extra node for the parent) and > therefore would have to provide the code that updates the parent in their > subsystem as part of the query anyway. Yes, exactly, and I'm totally fine with this. That's the change of API Docs that I'm expecting for the subsystem layer: that we document it somewhere that during a getChildren() call, the subsystem is expected to also update the properties of the parent. > I thought we wanted to avoid breaking API at this point. Right, and that's why my original suggestion works without breaking. But you were concerned about clients not noticing change if they are not broken, so here was the alternative suggestion. I like "non breaking backward compatible optionally returning "." better anyways. Actually returning empty filename "" is even better than returning "." because on some systems, "." might be a valid file name. > Alright, would we want everyone to extend AbstractRemoteFile or RemoteFile? Good question. RemoteFile is likely better because we don't want to force implementers use the Service layer, right? But I'm also fine with AbstractRemoteFile.
> > Alright, would we want everyone to extend AbstractRemoteFile or RemoteFile? > Good question. RemoteFile is likely better because we don't want to force > implementers use the Service layer, right? But I'm also fine with > AbstractRemoteFile. RemoteFile does not have the _hostFile field. It is abstract and it leaves subclasses to implement getHostFile(). AbstractRemoteFile does have the _hostFile field so it may make sense to call setHostFile(IHostFile) in there. The thing is we'd have to force implementors to extend AbstractRemoteFile rather than RemoteFile and for some implementations the _hostFile field may not make any sense.
Created attachment 102967 [details] patch with implementation suggestion This patch provides one way this could be implemented in the subsystem. Martin, what do you think of this? One thing to note is that with listMultiple() calls we'll end up having multiple "." files in there for each parent.
Created attachment 102973 [details] patch for systemview to not refresh filters and instead refresh parent object With this patch, we don't refresh the filters on a refresh remote event but we do refresh the parent item. If it's not expanded, the expand is forced so that we can do an actual query and then we collapse it after the query in order to retrain the initial expand state.
Created attachment 103142 [details] updated patch for system view I updated the patch for the system view because there was a potential null pointer that wasn't being handled properly.
Created attachment 103152 [details] patch to handle problem with ftp user home I've added this patch since there was a problem refreshing the My Home filter for FTP. The problem was caused by how the FTPService implements getUserHome() - rather than separating the parent path from the name, the host file was created using the full path as the name and the host file was treated as a root rather than a folder.
Created attachment 103155 [details] updating ftp service patch so that we don't pass null in for parent Another problem surfaces when the user tries to expand root because root's parent is set to null. I think it should be set to "".
Created attachment 103156 [details] patch to not update root remote files I've updated the subsystem's part of the patch so that we don't update root files.
Hm... I don't understand why you do not want to update the parent if the parent is a root. On a Linux system, the timestamp of the "Root" folder can also change, and access permissions of "/" can also change (in a very important manner, actually): [root@parser ~]# ls -ald / drwxr-xr-x 105 root root 4096 May 28 05:18 / [root@parser ~]# touch / [root@parser ~]# ls -ald / drwxr-xr-x 105 root root 4096 Jun 2 21:33 / So I think it's very important to check "root" folders as well for updated properties. I've mentioned before that rather than using name "." for the special entry that denotes the parent, it might be more appropriate to use an empty name "" -- you haven't answered that suggestion, do you see any problems with using ""? For FTP, I also cannot see what's the problem of treating the "Home" folder like a "Root". Many FTP clients do not support navigating outside the home, so conceptionally it must often be treated like a root. As long as the corresponding IHostFile.isRoot() method returns true, it should not matter if its getParent() returns null. I think you should not need to apply the FTP patch, but better fix your code to deal with root IHostFiles that have a "null" parent. Your patch currently addresses the single-folder list() method only, I think it should also address the listMultiple() methods -- at least the dummy "." or "" entries need to be filtered out, or you might end up getting unwanted child entries. For the IRemoteFile / RemoteFile / AbstractRemoteFile question, I'm thinking that it's OK to allow clients to extend RemoteFile. Since the only place where we expect an AbstractRemoteFile is in the FileServiceSubSystem; clients should be allowed to make IRemoteFileSubSystem implementations that don't use IHostFile, then they can extend RemoteFile. Our assumption that every RemoteFile is actually an AbstractRemoteFile should still always hold inside FileServiceSubSystem (which is actually final), because we are the only ones who can create these instances, right?
Marking as API since the proposed new semantics of IFileService#list() introduces new backward compatible API semantics.
(In reply to comment #31) > Hm... I don't understand why you do not want to update the parent if the parent > is a root. On a Linux system, the timestamp of the "Root" folder can also > change, and access permissions of "/" can also change (in a very important > manner, actually): > [root@parser ~]# ls -ald / > drwxr-xr-x 105 root root 4096 May 28 05:18 / > [root@parser ~]# touch / > [root@parser ~]# ls -ald / > drwxr-xr-x 105 root root 4096 Jun 2 21:33 / > So I think it's very important to check "root" folders as well for updated > properties. I guess you're right that root should also be included in the update. One problem I ran into when I did use root is that the IFileService.getFile() method would not return a root object as a "root" for SSH and FTP. As a result, the icon in the view would transform into either a file or a folder. I'm not sure if this is something to address at the service layer or whether the subsystem needs to do more work here. > I've mentioned before that rather than using name "." for the > special entry that denotes the parent, it might be more appropriate to use an > empty name "" -- you haven't answered that suggestion, do you see any problems > with using ""? I personally prefer "." because it has an implied meaning and we're clearly trying to distinguish it as different from other files. For example, when I run "dir" from a DOS shell, the "." dir is clearly the current directory. What I don't like about "" as a name is that it looks like a mistake in how the IHostFile stores stuff. Right now this parent and child combination: ("/<fld1>/<fld2>", "") would look like it should have been represented as ("/<fld1>", "<fld2>"). Hmm...it just occurred to me that after we replace the IHostFile, the absolute name will change from "/<fld1>/<fold2>" to "/<fld1>/<fold2>/.". Maybe we should change the IHostFile path to match the parent when setting the IHostFile. The problem with that is we don't have any set API in our IHostFile interface. Maybe we should return the IHostFile in the same form as the parent and just check for that by comparing the absolute paths in updateRemoteFile(). Any thoughts on this? > Your patch currently addresses the single-folder list() method only, I think it > should also address the listMultiple() methods -- at least the dummy "." or "" > entries need to be filtered out, or you might end up getting unwanted child > entries. The patch does include updateRemoteFile() calls in the listMultiple() methods as well. The one thing that should probably go in is a filter at the end to take out all the "." files (if that's what we end up using to denote "same as parent"). > For the IRemoteFile / RemoteFile / AbstractRemoteFile question, I'm thinking > that it's OK to allow clients to extend RemoteFile. Since the only place where > we expect an AbstractRemoteFile is in the FileServiceSubSystem; clients should > be allowed to make IRemoteFileSubSystem implementations that don't use > IHostFile, then they can extend RemoteFile. Our assumption that every > RemoteFile is actually an AbstractRemoteFile should still always hold inside > FileServiceSubSystem (which is actually final), because we are the only ones > who can create these instances, right? FileServiceSubSystem isn't directly doing the create of the IRemoteFiles. That's delegated to IHostFileToRemoteFileAdapter which is contributed. Since the adapter is contributed it's free to construct it's own IRemoteFiles which makes it more difficult to assume AbstractRemoteFiles.
(In reply to comment #33) > I guess you're right that root should also be included in the update. One > problem I ran into when I did use root is that the IFileService.getFile() > method would not return a root object as a "root" for SSH and FTP. As a Ok, I see... getFile("/") returns an IHostFile with isRoot==false. Does DStore return an IHostFile with isRoot==true in this case? We should file a separate defect for this and get it fixed. For FTP, though, it might sometimes be problematic to know in the Service what's a root and what's not ... looking at the code, I see that RemoteFileRoot always is a root and remains a root, even if you exchange the underlying IHostFile to no longer be a root. So if you consistently use RemoteFileRoot in the subsystem, this should not be an issue, right? > I personally prefer "." because it has an implied meaning Still, I'm scared about some subsystem treating "." as a valid name. If I'm not mistaken, Windows NTFS treats devices as \\devs\\.\\Serial -- we'd likely have issues with that if we treat "." specially and start ignoring it in some places. > The problem with that is we don't have any set API in our IHostFile > interface. Maybe we should return the IHostFile in the same form as the > parent and just check for that by comparing the absolute paths in > updateRemoteFile(). Shoot, you are right! Since we don't have any set() methods for IHostFile, we must return the parent in exactly the right format, and do some markup such that the subsystem knows it's the parent and not a child. A check of parentPath.equals(hostFile.getParentPath()) on each returned node should suffice, what do you think? But then, this might be hard to do on the listFilesMultiple() call because we don't know when the new parent starts... or no, hang on, we *do* know because API requires the listMultiple() to return its results in correct order, right? > The patch does include updateRemoteFile() calls in the listMultiple() methods Uhm, but I didn't know how you know which files from listMultiple() denotes the parent(s) ? > FileServiceSubSystem isn't directly doing the create of the IRemoteFiles. > That's delegated to IHostFileToRemoteFileAdapter which is contributed. Since > the adapter is contributed it's free to construct it's own IRemoteFiles which > makes it more difficult to assume AbstractRemoteFiles. IHostFileToRemoteFileAdapter is known to have IHostFile objects. Its methods take a FileServiceSubSystem as first argument. Why don't we change this API to have it return AbstractRemoteFile ? Then clients cannot do it wrong. GIven that we adapt IHostFile objects to IRemoteFile objects it seems natural to me to make the additional restriction that the result must be AbstractRemoteFile, no? - We should file a separate [api][breaking] defect for this.
(In reply to comment #34) > (In reply to comment #33) > > I guess you're right that root should also be included in the update. One > > problem I ran into when I did use root is that the IFileService.getFile() > > method would not return a root object as a "root" for SSH and FTP. As a > Ok, I see... getFile("/") returns an IHostFile with isRoot==false. Does DStore > return an IHostFile with isRoot==true in this case? We should file a separate > defect for this and get it fixed. DStore implements DStoreHostFile.isRoot() to determine whether it's a root from the path (i.e. "/" would always be considered a root). If we file separate defects we'll want to get it fixed for the same release that all these changes go into otherwise the UI can behave very strangely. > For FTP, though, it might sometimes be problematic to know in the Service > what's a root and what's not ... looking at the code, I see that RemoteFileRoot > always is a root and remains a root, even if you exchange the underlying > IHostFile to no longer be a root. So if you consistently use RemoteFileRoot in > the subsystem, this should not be an issue, right? Looking at the code, I don't think we're actually creating RemoteFileRoot objects. > > I personally prefer "." because it has an implied meaning > Still, I'm scared about some subsystem treating "." as a valid name. If I'm not > mistaken, Windows NTFS treats devices as \\devs\\.\\Serial -- we'd likely have > issues with that if we treat "." specially and start ignoring it in some > places. > > The problem with that is we don't have any set API in our IHostFile > > interface. Maybe we should return the IHostFile in the same form as the > > parent and just check for that by comparing the absolute paths in > > updateRemoteFile(). > Shoot, you are right! Since we don't have any set() methods for IHostFile, we > must return the parent in exactly the right format, and do some markup such > that the subsystem knows it's the parent and not a child. > A check of parentPath.equals(hostFile.getParentPath()) on each returned node > should suffice, what do you think? Shouldn't this check be of parentPath.equals(hostFile.getAbsolutePath())? > But then, this might be hard to do on the > listFilesMultiple() call because we don't know when the new parent starts... or > no, hang on, we *do* know because API requires the listMultiple() to return its > results in correct order, right? For listMultiple(), here's what we do: // what files are under this one? for (int j = 0; j < farr.length; j++) { IRemoteFile child = farr[j]; String childParentPath = child.getParentPath(); if (parentPath.equals(childParentPath)) { underParent.add(child); } } ... After we've converted all host files to remote files, we go through the above loop to determine which children need to go under which parents. Then we set the appropriate contents under each parent. Going through the code, I actually just found a bug... I was setting the contents of each parent to be an array of IHostFiles when it should have been an array of IRemoteFiles. I'll update that code when I next update the patch. > > FileServiceSubSystem isn't directly doing the create of the IRemoteFiles. > > That's delegated to IHostFileToRemoteFileAdapter which is contributed. Since > > the adapter is contributed it's free to construct it's own IRemoteFiles which > > makes it more difficult to assume AbstractRemoteFiles. > IHostFileToRemoteFileAdapter is known to have IHostFile objects. Its methods > take a FileServiceSubSystem as first argument. Why don't we change this API to > have it return AbstractRemoteFile ? Then clients cannot do it wrong. GIven that > we adapt IHostFile objects to IRemoteFile objects it seems natural to me to > make the additional restriction that the result must be AbstractRemoteFile, no? > - We should file a separate [api][breaking] defect for this. Should completing this fix require that api breaking defect to be complete? If so, why not put it in here too?
(In reply to comment #35) > DStore implements DStoreHostFile.isRoot() to determine whether it's a root Ok, fixing this in FTP and SSH won't hurt, I can file a defect and fix it. > Looking at the code, I don't think we're actually creating RemoteFileRoot > objects. I think it would be a good idea if we did, in order to ensure that what's been a root once always remains a root. BTW, RemoteFileRoot should likely be a subclass of AbstractRemoteFile rather than RemoteFile > Shouldn't this check be of parentPath.equals(hostFile.getAbsolutePath())? I'd be scared of doing this, because hostFile.getAbsolutePath() includes some path processing inside the service (i.e. concatenating the parent path and the name, which it gets from the subsystem, by some magic that only the service should know). I figured that if we compare hostFile.getParentPath() then it must be the same as we put into the service, for all children but not for the parent. Looking at your code below it looks like you're doing exactly this already? > For listMultiple(), here's what we do: Looks good, thanks! I feel that we're nearing a solution of this. I really want to have this in RC3. > Should completing this fix require that api breaking defect to be complete? > If so, why not put it in here too? I prefer having api breaking defect separately for documentation and tracking purposes. Is the change required? Not necessarily. I can care for that API changing defect.
(In reply to comment #36) > > Looking at the code, I don't think we're actually creating RemoteFileRoot > > objects. > I think it would be a good idea if we did, in order to ensure that what's > been a root once always remains a root. BTW, RemoteFileRoot should likely be > a subclass of AbstractRemoteFile rather than RemoteFile For now, I'd prefer to have a separate defect for using RemoteFileRoot and defer it to 3.0.1 since we haven't used it yet and I'm not sure what the implications are. > > Shouldn't this check be of parentPath.equals(hostFile.getAbsolutePath())? > I'd be scared of doing this, because hostFile.getAbsolutePath() includes some > path processing inside the service (i.e. concatenating the parent path and > the name, which it gets from the subsystem, by some magic that only the service > should know). Maybe I'm not understanding what you're saying - I would think parentPath.equals(hostFile.getParentPath()) would always return true when a list method is called because all children of parent should have parentPath as their parent path. Aren't we calling this to figure out whether a child represents the same thing as the parent?
Created attachment 103354 [details] patch update the file service subsystem to address some issues discussed Martin, here's a patch update that addresses some of the issues we've been discussing. Do you see any problems?
(In reply to comment #37) > For now, I'd prefer to have a separate defect for using RemoteFileRoot and Ok, fine -- perhaps it isn't even needed > Maybe I'm not understanding what you're saying - I would think > parentPath.equals(hostFile.getParentPath()) would always return true when a > list method is called because all children of parent should have parentPath as For the children, yes. But not for the parent! The service should construct the parent exactly as the parent needs to be constructed, e.g. given that parentPath = "/foo/bar" int idx = parentPath.lastIndexOf('/'); if (idx > 0) { String parentOfParent = parentPath.substring(0, idx-1); String nameOfParent = parentPath.substring(idx); IHostFile hostFileOfParent = new SftpHostFile(parentOfParent, nameOfParent,...) See? If the service constructs the IHostFile that represents the parent folder properly, that IHostFile must have a different parentOfParent as parentPath. Remember that this is for an optimization so we don't necessarily have to implement this in the Services yet, but I could still take a try at Sftp if you want.
(In reply to comment #39) > See? If the service constructs the IHostFile that represents the parent folder > properly, that IHostFile must have a different parentOfParent as parentPath. > Remember that this is for an optimization so we don't necessarily have to > implement this in the Services yet, but I could still take a try at Sftp if you > want. Yeah, I realized that's what you meant after and integrated that into my last patch with the following: String parentPath = parent.getAbsolutePath(); // look for the "." file to determine whether we have an updated parent in the results for (int i = 0; i < results.length && newHostParent == null; i++){ IHostFile result = results[i]; if (!result.getParentPath().equals(parentPath)){ newHostParent = result; // we think the child is the same as the parent } }
Martin, could you review this?
Bug 235360 and bug 235363 have been filed and fixed in order to get the API and prerequisites in place for this to work.
Created attachment 103461 [details] combined patch updated to match current code from cvs I've combined all changes into one patch based on the latest changes from cvs.
I'm getting a conflict in FTPService against my changes from bug 235360... can you review my changes anytime soon?
Dave are you sure that this works in the listMultiple() case? For me, it looks like your "underParent" array will always only hold those query results that are children of the parent, but never the parent itself. Therefore, the performance optimization in updateRemoteFile() cannot work, since that one expects the underParent array to include the parent itself. Or am I missing something? In updateRemoteFile(), this code should no longer be necessary thanks to my fixes: if (parentParentPath == null){ parentParentPath = ""; //$NON-NLS-1$ } As always, I hate catching away SystemMessageException. These exceptions are allowed to fall through, and be reported to the user in the end! In updateRemoteFile(), finding the updated parent in the results and actually removing it from the results could be combined into one loop to make the code simpler, no? newHostParent.equals(oldHostParent) is bound to fail, because currently almost no IHostFile declares an equals() method -- LocalFile is the only one as per bug 168591, I found that none of the others implements it and filed bug 235489 for describing the other issues. As it stands now, I think it's best to unconditionally replace the IHostFile object and perform a refresh (if we have equals() in the futur we might be able to avoid the refresh event). Do you really need two almost identical updateRemoteFile() methods? I think these could be combined, by extracting the IHostFile[] array from the IRemoteFile[] array in a first step. The changes in SystemView I don't quite understand, I'd appreciate a little bit more documentation in the code. Thanks!
I've committed my stuff, you should be able to Team-> Update and then review your combined patch once again
Created attachment 103474 [details] another update of combined patch This update has some of your suggestions. Could you take a look?
AbstractRemoteFile#setHostFile() is missing @since in Javadoc For FileServiceSubsystem, I find it odd that you stuff in the "parent" IHostFile into the results just to parse it out of the list again. Also, you have two listMultiple() instances but add the parent only in one. Then, you have the unwanted "parent" object in the farr returned by your IHostFileToRemoteFileAdapter, but don't filter it out after updateRemoteFile. I would suggest this: 1.) Change method signature using a single method updateRemoteFile(IRemoteFile oldParent, IHostFile newParent) and when newParent==null you do the getFile() call. I think it's not really of much use trying to do the filter-out from the array as a side effect in the same function. Beter filter-out where you call updateRemoteFile. 2.) In list(), after internalList() iterate over the array to filter out the parent 3.) In the listMultiple() queries the loop can also be simplified. Because you know that results come in ordered, it should be sufficient to iterate over the result list only ONCE and re-initialize the underParent ArrayList whenever the current parent "switches". I'd also strongly suggest making one example implementation that in fact returns the parent element (e.g. SftpFileService) such that you can test your code.
Created attachment 103479 [details] updated combined patch with changes based on comment #48 I've updated the patch again to include some good optimizations suggested in comment #48. I'm still not sure what you have in mind for 3).
(In reply to comment #49) > Created an attachment (id=103479) [details] > updated combined patch with changes based on comment #48 > I've updated the patch again to include some good optimizations suggested in > comment #48. > I'm still not sure what you have in mind for 3). My patch missed one of these calls in the first listMultiple(): filteredList.add(child); I've updated my code but figure it's not worth updating the patch just yet.
Created attachment 103555 [details] updated combined patch with correction
Created attachment 103563 [details] patch updated again with tweak I updated the patch again because for listMultiple() we should do the updateRemoteFile() before the if (underParent.size() > 0) block since now it's possible for there to be no children.
I'm questioning the "fakeExpanded" logic in SystemView. If we get a refresh on a collapsed item, shouldn't we just getFile() the item and mark its children stale, rather than doing an odd fake expand? Also, I'm wondering whether the fake temporary expand is visible to the user? The logic seems to be synchronous but in the Dispatch thread (in order to be able to re-use the fakeExpanded state after querying the children). This must involve a nested event loop, which is bad. If there is no way around fakeExpanded, could this also be done asynchronously, i.e. doing the fakeExpanded stuff in a callback rather than synchronously? In FileServiceSubsystem: * In the first listMultiple() I think you should FIRST filter the IHostFile results, and THEN convert them into IRemoteFiles, in order to save work. * The parentPath.equals(child.getAbsolutePath() logic is problematic because you do not know how the Servcie computes its absolute path (what separators does it use?) -- better do grandParentPath.equals(child.getParentPath()) I think I made some more comments earlier. I need to prepare for the release review now, but I'd hope to have some time for this a bit later. Please try to answer the questions I've raised till then.
(In reply to comment #53) > I'm questioning the "fakeExpanded" logic in SystemView. If we get a refresh on > a collapsed item, shouldn't we just getFile() the item and mark its children > stale, rather than doing an odd fake expand? Also, I'm wondering whether the > fake temporary expand is visible to the user? > The logic seems to be synchronous but in the Dispatch thread (in order to be > able to re-use the fakeExpanded state after querying the children). This must > involve a nested event loop, which is bad. If there is no way around > fakeExpanded, could this also be done asynchronously, i.e. doing the > fakeExpanded stuff in a callback rather than synchronously? The tree view's mechanism to update model objects is via it's content provider. Generic content providers don't have any getFile() types of calls - instead there's just getChildren(). When the object is already expanded, a refresh via getChildren() obviously makes sense. If possible I'd prefer to keep a refresh on an unexpanded node under a filter as similar as possible to the expanded case. The artificial expand is quick but may be visible to the user - in particular via icon change (i.e. folder switches between closed folder to open folder and back). I actually like that because it shows me that something is actually happening when I do the refresh. I needed something that would indicate "this needs to really be refreshed" and expand state seems reasonable although an alternative may be to change the refreshRemoteObject() and smartRefresh() calls to take a new param to indicate that the node to refresh is special.
(In reply to comment #53) > The logic seems to be synchronous but in the Dispatch thread (in order to be > able to re-use the fakeExpanded state after querying the children). This must > involve a nested event loop, which is bad. If there is no way around > fakeExpanded, could this also be done asynchronously, i.e. doing the > fakeExpanded stuff in a callback rather than synchronously? My last reply missed this part. The refreshRemoteObject() call kicks off a query via the deferred mechanism (i.e. the query is asynchronous). We collapse the node immediately after that call and, for non-local, the "Pending..." object remains under the now-collapsed node until the results are back.
(In reply to comment #53) > In FileServiceSubsystem: > * In the first listMultiple() I think you should FIRST filter the IHostFile > results, and THEN convert them into IRemoteFiles, in order to save work. If I first filter the host file results and then convert them into remote files, I'm still going to have to loop through those results after the fact to determine which children to cache under which parent. The only savings I can think of are one less IRemoteFile created per parent.
Created attachment 103580 [details] patch with update to FileServiceSubSystem.listMultiple() to filter before creating remote files With this patch the filter of the IHostFiles is done before creating the remote files.
Created attachment 103605 [details] patch combining all
Created attachment 103630 [details] My version of patch, including Sftp impl, Javadoc and Testcase Attached is my version of the patch, including an implementation of the new optional API in SftpFileService#internalFetch() updates to all relevant Javadoc, and a testcase for new split() functionality in Sftp. I took the freedom to refine the API documentation of list() and listMultiple() in the service, such that if the optional parent IHostFile is returned, it must always be returned in order before its own children. This considerably simplifies things in FileServiceSubSystem. I also created a new default implementation for listMultiple() in RemoteFileSubSystem, such that only the "most refined" variant of the four listMultiple() variants actually needs to get implemented, thus getting rid of code duplication. I tested the fix on Sftp and it seems to work really nicely.
Naturally, I approve my own patch :-) DaveM please review it.
Created attachment 103637 [details] Additional patch with Javadocs in IRemoteFileSubSystem This additional patch augments my previous one by improving Javadoc in IRemoteFileSubsystem, covering especially the need for clients to update the parent IRemoteFile as a side-effect, but also covering things like the kinds of exceptions thrown, and what happens in case an exception interrupts a listMultiple() call prematurely. I've tried to avoid duplication in Javadocs. Please include in the Review, I was not 100% sure if the intended behavior of the file name filters is indeed to apply to the *file* names only but not the *folder* names, unless only-folders is the requested type (in this case, folders are also filtered).
Personally, I don't like assuming the service is going to put the parent in as the first time. I guess if it's documented it's okay but nevertheless I could imagine some services wanting to sort. I ran into the following exception with listMultiple() when trying this out with some junit: java.lang.IndexOutOfBoundsException: Index: 1, Size: 1 at java.util.ArrayList.RangeCheck(ArrayList.java:572) at java.util.ArrayList.get(ArrayList.java:347) at org.eclipse.rse.subsystems.files.core.servicesubsystem.FileServiceSubSystem.listMultiple(FileServiceSubSystem.java) at org.eclipse.rse.subsystems.files.core.subsystems.RemoteFileSubSystem.listMultiple(RemoteFileSubSystem.java:949) at org.eclipse.rse.subsystems.files.core.subsystems.RemoteFileSubSystem.listMultiple(RemoteFileSubSystem.java:937) at org.eclipse.rse.tests.subsystems.files.FileSubsystemConsistencyTestCase.testMultiFileQuery(FileSubsystemConsistencyTestCase.java:303) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:64) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:615) at junit.framework.TestCase.runTest(TestCase.java:164) at org.eclipse.rse.tests.core.RSECoreTestCase.runTest(RSECoreTestCase.java:310) at junit.framework.TestCase.runBare(TestCase.java:130) at org.eclipse.rse.tests.core.RSECoreTestCase.runBare(RSECoreTestCase.java:296) at junit.framework.TestResult$1.protect(TestResult.java:106) at junit.framework.TestResult.runProtected(TestResult.java:124) at junit.framework.TestResult.run(TestResult.java:109) at junit.framework.TestCase.run(TestCase.java:120) at org.eclipse.rse.tests.core.RSECoreTestCase.run(RSECoreTestCase.java:265) at junit.framework.TestSuite.runTest(TestSuite.java:230) at junit.framework.TestSuite.run(TestSuite.java:225) at org.eclipse.jdt.internal.junit.runner.junit3.JUnit3TestReference.run(JUnit3TestReference.java:130) at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:460) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:673) at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:386) at org.eclipse.pde.internal.junit.runtime.RemotePluginTestRunner.main(RemotePluginTestRunner.java:62) at org.eclipse.pde.internal.junit.runtime.UITestApplication$1.run(UITestApplication.java:118) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:130) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:3750) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3375) at org.eclipse.ui.internal.Workbench.runEventLoop(Workbench.java:2375) at org.eclipse.ui.internal.Workbench.runUI(Workbench.java:2339) at org.eclipse.ui.internal.Workbench.access$4(Workbench.java:2205) at org.eclipse.ui.internal.Workbench$4.run(Workbench.java:478) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:288) at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:473) at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:149) at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:106) at org.eclipse.pde.internal.junit.runtime.UITestApplication.start(UITestApplication.java:52) at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:193) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:110) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:79) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:362) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:175) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
I hit this problem when I tried to do a query of a folder that only had one file using dstore. The exception occurs in your code here: while (curParentPath.equals(curHostFile.getParentPath())) { resultIdx++; curHostFile = (IHostFile) hostFiles.get(resultIdx); } The hostFiles list only has one item at this point, but resultIdx is incremented from 0 to 1 and then we get an array out of bounds exception trying to access non-existant item 1. I'm not sure what you're trying to do with curHostfile here because it doesn't get used after this in the loop anyway.
Created attachment 103672 [details] My updated patch v2 Ok, I admit that my previous patch was crappy and I'm totally surprised that I didn't hit the exceptions that you did when testing. I have since added some unit tests, and fixed the main code. The unit tests are meant to complete on Local, ssh and dstore at least; currently, they don't because the subsystems work differently on the corner cases like non-existing folders or wrong types (do they throw an exception or just return empty list). Based on the new corner cases that I found, I created more unit tests and updated the Javadocs. I think the most important point now is that we agree on exact semantics of the API Docs, and finish the unit tests to capture as many scenarios as possible. In the curent form of the Javadocs (IFileService / IRemoteFileSubSystem / list and listMultipel(), I'm not too happy that we leave the implementer so many choices. I'd prefer a on-and-for-all clear specification of what they are expected to return in what situations. We can deal with the "fallbacks" if we must, but it's ugly. I need to go now, so this bug won't make it into RC3 because I really need to wrap up RC3 now. But I'd like to get more feedback on the Javadocs specifically, and potentially commit at least these.
DaveD: please review and comment on the Javadocs. DaveM: please review again.
Rado, you always have a good opinion on APIs -- I'd like your opinion on the proposed API Documentation (Javadoc) changes for IFileService#list(), IFileService#listMultiple() and their counterparts in IRemoteFileSubSystem. For the "multi" queries especially, the questions are: 1. how we best allow implementers to optionally return the updated parent nodes 2. what to do in case of "parent not found" or "parent is file" errors Currently, I don't like the fact that the Javadoc leaves so many possibilities and fallbacks. The semantics should be clearly defined. Some options include: a) ALWAYS throw exception on error -- easy to implement and clear, but not optimal for listMultiple() because it exits prematurely. Perhaps not an issue since errors shouldn't occur anyways? b) Add another bitmask flag like IFileService#LIST_OPTION_RETURN_PARENTS if not set, then clients must run in backward compatible mode, i.e. never return parent nodes; this is to allow older clients. If the flag is set, then implementers SHOULD return parent nodes. c) For the listMultiple() queries, instead of returning a List<IHostFile> like now, switch to filling the List as List<IHostFile[]> such that it's clearly known what results belong to what parent. d) Or, perhaps even better yet, Have the caller pass in an IHostFile[][n] -- An array (cardinality n == number of parents) of IHostFile arrays which are null initially but then filled by the callee. I'm personally in favor of implementing all (a)(b)(d) even if that is yet another breaking API change. Since the API was new in 3.0 I don't consider the breakage too bad. Doing (a)(b)(c) is binary compatible although semantics change (the type inside the List) so that's likely not much better than d. Doing (a)(b) only imposes some processing ambiguities, e.g. in case of listMultiple("/foo/bar", "/foo") whenn foo/bar is an empty folder, it's unclear whether the first item is the "parent node" for /foo/bar" or the first child node of "/foo" so I'm against it. Any other thoughts or ideas? The current API Doc of IFileService#listMultiple is as follows, anything I forgot? For more details please look at the latest patch. * List the contents of multiple remote folders, and optionally return * updated status information on the folders themselves as well. * <p> * By optionally placing updated status information about the parent objects * into the returned list as well, additional server round trips can be * avoided if this data is available as by-product of the query anyways. * Implementers which do not have that data available automatically, will * get following {@link #getFile(String, String, IProgressMonitor)} if * necessary, so there is no need to return that optional information. * <p> * The optional status information is especially interesting in case one of * the parent folders did not exist, or turned out to be of a wrong type * (e.g. file instead of folder). In these cases, the implementer has the * option of either returning a correct IHostFile object with * {@link IHostFile#exists()} returning false, or returning the proper type; * or, not returning any children for the element in question, in which case * a separate call needs to be made by the framework. The third option is * throwing a {@link SystemElementNotFoundException} or * {@link UnsupportedOperationException}, but that will require the * framework to perform several additional queries in order to obtain * correct results since additional queries after the first exception are no * longer processed. One important point here is, that it should be possible * to detect the difference between we should be able to tell the * intelligent such that the difference between an existing folder with no * children, and a non-existing folder. * <p> * If an error occurs during the retrieval of the contents of a folder, this * operation stops on that folder and a {@link SystemMessageException} is * thrown. Items retrieved before that folder will be returned. Items in * folders after that folder will not be retrieved. The items in the folder * on which the error occurs will not be returned. * * @param remoteParents - the names of the parent directories on the remote * file system from which to retrieve the collective child list. * The items in the list of remote parents must all be unique, * that is there must not be two same parent paths. * @param fileFilters - a set of strings that can be used to filter the * children. Only those files matching the filter corresponding * to it's remoteParent make it into the list. The interface does * not dictate where the filtering occurs. For each remoteParent, * there must be a corresponding fileFilter. * @param fileTypes - indicates whether to query files, folders, both or * some other type. For each remoteParent, there must be a * corresponding fileType. For the default list of available ile * types see {@link IFileService#FILE_TYPE_FILES} and related * constants. * @param hostFiles a list into which the found {@link IHostFile} objects * will be appended. Results are placed into this list in the * following order: * <ol> * <li>Optionally, an updated {@link IHostFile} object * representing the first remote parent in the list.</li> * <li>The list of children of the first parent, as IHostFile objects. * For all of these children, {@link IHostFile#getParentPath()} * must return the first remoteParent path as passed in. This * list of children may be empty.</li> * <li>Optionally, an updated {@link IHostFile} object * representing the second remote parent in the list.</li> * <li>The list of children of the second parent (may be * empty), same as before.</li> * <li>And so on, until all parent folders have been processed.</li> * </ol> * @param monitor the monitor for this potentially long running operation * @throws SystemMessageException if an error occurs. Typically this would * be one of those in the RemoteFileException family. * * @since org.eclipse.rse.services 3.0
As far as I understand this the whole point for making list() and listMultiple() more complicated is to safe calling getFile() for the corresponding folder. Martin, are you sure that these changes will reflect the perfomance in a significant way? I tend to agree with DaveM that some extra calls to getFile() won't hurt much. I am also not happy with leaving options for the implementers and I think that we need some benchmarks which prove that returning parent info in list()/listMultiple() is worthwhile.
In our commercial offering, we are still dealing with some targets connected by 9600 bps serial line. Every server round trip hurts extremely on those. Now I'm not really so much concerned about the file system, since we're typically doing different things with RSE on these targets - things like exploring processes, threads, kernel objects etc - so what's most important for me is the basic concept: 1.) If information from the remote is available for free, then use it without forcing an extra server round trip. It would be silly IMHO to not use this valuable, already-available extra information about the parent. 2.) When asking for children of an object, give the ability to update the object itself along the way -- just because we've seen that this information is typically needed. So, for me the question is definitely not about the concept of updating children and parent in one call; that's a very important functionality that we should really offer. The question is, what is a suitable API to support this. On the Subsystem Layer, our API is that we query the children and expect the parent to be updated automatically as a side-effect. This is possible on the subsystem layer, because the parents are always handles and because those handles are passed into the list() call. On the Service Layer, we cannot do this (a) because we're passing Strings into the Service rather than handles, and (b) because our handles should be designed to be immutable for Thread Safety reasons. So, let's not be too fearful of designing API to support this properly! I have outlined the questions and guiding principles for this. The question is simple: How can the IFileService#list*() API be enabled to return children and parent information at the same time if this information is availalble? I'm hoping that we can agree on an answer. But I admit that this question is important enough that we shouldn't rush it if we cannot agree.
Created attachment 103780 [details] patch for SystemView with additional change I've updated SystemView so that any unexpanded container that gets a refresh remote event will result in a query to itself (rather than it's parent). Prior to this we were just doing this for direct children of filters while things like folders under folders would instead have their parent refreshed. I think it makes more sense to be consistent for all cases.
Created attachment 103787 [details] patch for SystemView with additional changes Updating system view with another change so that filters are refreshed when the refresh remote event src is a non-container directly under a filter.
Martin, what do you want to do with this? Do you want to go back to a solution that doens't involve having the parent return with the children and work that out via a separate defect or do you want to keep that in whatever we commit?
Like discussed yesterday, I want to split the fix into (a) plain bugfix keeping existing API and existing API Docs; commit that plaing bugfix under this bug; (b) Create 2nd bug report asking for API Javadoc clarification and Unittests; not involving API change, but clarifying what we have now (c) Create 3rd bug report asking for Performance optimization of list() and listMultiple() by means of changing API. If you could please isolate the plain bugfix from the other performance improvement / API Javadoc stuff and attach the reduced patch here, I'd be happy to review, approve and get it committed. I'll care for the new bugs for (b) and (c).
Created attachment 103977 [details] patch for simplied solution I've created a patch to provide a simplified solution whereby no assumptions are made about the IFileService implementations of list() and listMultiple(). This code assumes that a parent is not returned when querying children in the service and therefore getFile() for the parent is always called.
bug 235360 and bug 235363 are created to deal with API docs, unit tests and API changes if we choose to alter the semantics of the IFileService.list*() APIs.
Please review the latest patch.
Patch looks good to me, thanks for workign on this. I just have 2 suggestions and 1 quetions, but even if the suggestions are not applied I'm ok with committing (no need to attach yet another patch for me): (1) In the listMultiple() methods, when I compare to HEAD, I see that explicitly casting the Object[] array returned by underParent.toArray() into an IRemoteFile[] array is not necessary - you might want to avoid that operation to be closer to what we had previously: IRemoteFile[] qresults = (IRemoteFile[])underParent.toArray(new IRemoteFile[underParent.size()]); (2) Given that IHostFileToRemoteFileAdapter now returns AbstractRemoteFile, I'd prefer working with AbstractRemoteFile as type in the code in listMultiple and updateRemoteFile -- that'll make it evident that code is allowed to replace the IHostFile without having to do a cast in updateRemoteFile, it exploits the static type checking that we can do now thanks to the API change in the adapter (3) In the updateRemoteFile code, if I'm not mistaken we *know* that DStoreFileService *does* update the IHostFile... is there a way to avoid the extra getFile call in the dstore case?
(In reply to comment #74) > bug 235360 and bug 235363 are created to deal with API docs, unit tests and API > changes if we choose to alter the semantics of the IFileService.list*() APIs. Ooops...wrong numbers: I mean to say: bug Bug 236067 and 236065 were created.
I've committed the code to cvs with some modifications as per the suggestions: (In reply to comment #76) > Patch looks good to me, thanks for workign on this. I just have 2 suggestions > and 1 quetions, but even if the suggestions are not applied I'm ok with > committing (no need to attach yet another patch for me): > (1) > In the listMultiple() methods, when I compare to HEAD, I see that explicitly > casting the Object[] array returned by underParent.toArray() into an > IRemoteFile[] array is not necessary - you might want to avoid that operation > to be closer to what we had previously: > IRemoteFile[] qresults = (IRemoteFile[])underParent.toArray(new > IRemoteFile[underParent.size()]); I've changed this part to be: Object[] qresults = underParent.toArray(); > (2) > Given that IHostFileToRemoteFileAdapter now returns AbstractRemoteFile, I'd > prefer working with AbstractRemoteFile as type in the code in listMultiple and > updateRemoteFile -- that'll make it evident that code is allowed to replace the > IHostFile without having to do a cast in updateRemoteFile, it exploits the > static type checking that we can do now thanks to the API change in the adapter I've changed the references to IRemoteFile to use AbstractRemoteFile in the list*() methods. > (3) > In the updateRemoteFile code, if I'm not mistaken we *know* that > DStoreFileService *does* update the IHostFile... is there a way to avoid the > extra getFile call in the dstore case? I'll take a look at the (3) thing later.
Looks good to me. Thanks Dave and Martin for your hard work on this issue.
