Community
Participate
Working Groups
Build: R2.0 org.eclipse.ui.actions.CopyResourceAction calls IWorkspace.delete on incoming resources that collide with current resources. This should be replaced with a validateEdit and a setContents call so that VCMs that watch the filesystem can continue tracking versions of the file.
I thought there were some Core reasons for having to do it this way. DJ? Also, is it up to Core or UI to call validateEdit?
John A replies: One possible reason is that setContents fails for read-only files, but delete+copy suceeds. I think the correct pattern is validateEdit followed by setContents (whoever calls setContents should call validateEdit). In that particular UI case, I suspect it was just coded that way because delete+copy works for all resource types, whereas using setContents would involve writing a special case for IFile and extra recursive steps for folders and projects. This is based on an original bug report from JohnW: bug 20036. See also the discussion Jed had with himself in bug 21635. It looks like this is something that needs to be fixed for us to play nicely with ClearCase. It seems like there is an opportunity for core to provide better API here. Perhaps an OVERWRITE flag for IResource.copy and IResource.move would be appropriate, since it's actually non-trivial to implement. It involves recursively walking the resource tree, looking for existing IFiles that match, then using validateEdit/setContents instead of delete/create. Existing persistent properties at the destination would also have to be deleted so that they could be transferred from the source. There could be other wrinkles that I can't think of at the moment.
Session properties and markers would have to be deleted too I assume. Looks like we really need new Core API here. Or would IResource.copy with FORCE work for read-only files too?
Moving to Core for API request.
Adding TC to CC list. What is the API which you are requesting?
What we really need here is for Core and VCM to tell the UI team how we should handle this. One option is for VCM to somehow realize that a delete and re- create of the same file should keep it under management, but this is hard to do when you keep the sync info on the resource. If the right answer is to do setContents, then we need new API for clearing out existing properties.
>>One option is for VCM to somehow realize that a delete and re- >>create of the same file should keep it under management I don't see how we can do this -- the basic problem is that when you delete/create a resource its not easy for anyone listening to discover its the same resource because they appear as to two seperate unrelated operations. For example, the move/delete hook in VCM has no opportunity to know that a create is going to happen next -- the best it could do is keep around a list of things deleted, watch for adds to the same resource path, and then sort out the real deletes then the runnable completes. The 'correct' behaviour is to always overwrite rather than delete/recreate (ie. independent of asking the VCM). This isn't just a VCM issue - anyone who tracks resource lifecycle will get thrown off by delete/create since they lose identify of the resource in the interim. The "force" flag seems the right idea. To support JohnA's previous comments: for the read-only case, calling validateEdit first is exactly the right thing to do. If the resource is still read-only after that, then overwrite *should* fail. Delete/create should not be used to circumvent read-only. We need new Core API and UI needs to call it. I notice the Core PR (20036) is P2. We should decide on a milestone for it and then appropriately set this PR's priority/milestone (its P3). This work is absolutely required for 2.1.
Based on discussion, here is what DJ, Nick and KevinM conclude with respect to UI copy: 1. UI requires from Core IResource.copy() with a FORCE_OVERWRITE boolean flag. When true, if the file exists in the destination, it will receive the persistent properties from the source but will lose its session properties and markers (and existing persistent properties). This mimics the behaviour of delete+copy. 2. UI will call validateEdit() for each file it has prompted the user to overwrite. If the user says "Yes to All", then the UI will determine the remaining conflict cases and validateEdit() these all together. 3. If the user picks No then that file is not validateEdit'ed. If the user picks Cancel then no work to be done either. 4. UI will call the new IResource.copy() method instead of doing a IResource.delete() first for the overlapping cases. 5. If UI receives a ! OK from validateEdit() then that is equivalent to a Cancel -- the rest of the copy is aborted. 6. In keeping with the validateEdit() java doc, the UI will pass in a UI context (the shell) and can therefore assume that the user has been notified of the reason for ! OK. Moving this PR back to UI to remind for work to do calling the new (proposed) core API. Another one will be created for the Core.,
*** This bug has been marked as a duplicate of 27187 ***
The simple file read-only case is not clear to me. I prompt if read-only files should be overwritten. Am I supposed to continue doing the delete/copy combo instead of setContents in this case until there is a FORCE_OVERWRITE flag for copy/move? Kevin writes that "Delete/create should not be used to circumvent read-only.". This would mean you can no longer overwrite read-only files. What's the verdict on the FORCE_OVERWRITE flag? It would save me time if I could just add the flag to the copy call instead of changing code to use setContents.
FORCE_OVERWRITE will not be done for 2.1. For read-only files, if the validateEdit returns a fail status, we should abort the copy. We should not use delete/copy, just setContents.
validateEdit returns an ERROR status for read-only files so I abort the copy. The result is that you can no longer overwrite read-only files. If you try, you get an internal error dialog. This is sub-optimal to put it mildly. I would have to take out the code that prompts for overwrite. Reopening this bug because it contains most of the useful information.
validateEdit should not always return an ERROR status for read-only files. A VCM provider that implements validateEdit would ask the user if they wanted to check out the file(s), and if so, it would check them out and return OK. If not, it would return an error status. However, it's possible that, if the VCM provider does not implement validateEdit, that Core is always returning an error status (makes sense, since the read-only status won't have changed). So it looks like we need to handle this better in the UI. The user should be prompted whether they want to overwrite the read-only files (after trying validateEdit, if it comes back with an error). Note that this is a normal event. However we handle this, we should not see internal error dialogs.
Actually, it's not an internal error dialog but a multi status error dialog. The spec for validateEdit says that ERROR will be returned if a file is read- only so this works as designed. Team just has to display an error message indicating the read-only state since I pass in a context. See bug 28123. Question about delete: Moving a file using setContents requires a manual delete. The read-only flag has to be turned off somehow for this to work. Either I call validateEdit on the source or override the read-only flag manually. Please advise. Same applies to regular delete operation. Note that deleting read-only files is broken and has apparently been broken for a while (bug 28140).
>>Question about delete: >>Moving a file using setContents requires a manual delete. The read-only flag >>has to be turned off somehow for this to work. Either I call validateEdit on >>the source or override the read-only flag manually. >>Please advise. The move-delete hook will catch this and attempt to check out the file. If you try to delete it and it fails because its still read-only, then the operation should fail. If you don't have a provider, it means the user will need to change its read- only status manually. As we discussed, I believe this to be an unusual case -- readonly files not under version management. More complex solutions would involve checking if the file is under version control, but I don't think this is worth doing, not at this time anyway.
In summary/remaining work items: 1. During a move operation, when the source is read-only it should be validateEdited together with the read-only destinations. This ensures that the source can be deleted after the destination has been written. 2. The entire copy/move operation should be aborted when validateEdit fails, regardless of whether it fails for one or more files. 3. Since we provide a UI context, Team team intends to display a message if validateEdit fails. If this does not happen for 2.1 we should display a message ourselves to inform the user that the operation was aborted.
>>In summary/remaining work items: >> 1. During a move operation, Yes, but to be clear, we are only discussing the "move with collision on the destination" case. >> 2. The entire copy/move operation should be aborted when validateEdit fails, Yes >> 3. Since we provide a UI context, Team team intends to display a message We will for 2.1 so no need to prompt as well.
1. ...because in the non-collision case I do a regular move where the move hook does the right thing? I tried it out and it works. I just want to understand why/how.
Fixed the move collision case. There's another pesky problem with deleting folders. See bug 30721.
>>1. ...because in the non-collision case I do a regular move where the move hook does the right thing? Correct. You have now entered the seventh inner circle of validateEdit wisdom.
Released fix for move validation and cancel behavior. Fixed in >20030203.
