Community
Participate
Working Groups
I20110426-2000. While testing the fix for bug 303517 most of the things worked nicely. One problem though is that all clients that first check IResource.isSynchronized(int) still fail (at least on first attempt) because the method returns 'false' and only afterwards refreshes the resource. I think it should behave the same as IFile.getContents(). Test Case: 1. enable lightweight refresh 2. change a file outside Eclipse 3. try to delete it that file inside Eclipse (Package or Project Explorer) ==> out of sync dialog 4. try again ==> works now
It might be important that resource changing operations (as opposed to reading operations) are notified of an external modification -- for example when deleting a directory which contains added resources core.resources is unaware of... Why do you check #isSynchronized when the relevant core.resources API will throw CoreException on out-of-sync? There's always a window between checking isSunchronized and calling the relevant API anyway. Also changing the semantics of this API will make it impossible for API users to check the sync state when the pref is enabled. This is useful as checking sync state doesn't require the WS lock, but refreshlocal does. So breaking this API removes a potentially useful optimisation.
(In reply to comment #1) > It might be important that resource changing operations (as opposed to reading > operations) are notified of an external modification -- for example when > deleting a directory which contains added resources core.resources is unaware > of... > > Why do you check #isSynchronized when the relevant core.resources API will > throw CoreException on out-of-sync? I don't - I just filed the bug that I found while testing the new feature ;-). The reason why people use the API is probably because they see it as cleaner way than catching exceptions afterwards and because they didn't want to start the operation if the resources are out of sync (see your argument about performance). > Also changing the semantics of this API will make it impossible for API users > to check the sync state when the pref is enabled. Yes, that could indeed be a problem but wasn't the whole point of the new option that all clients get a "refreshed" view of the underlying FS? If there's really the need for an API that gets the real state then that could be added - similar to e.g. IFile.getCharset(false). The current behavior is very irritating for a user: first time I do something I get the out-of-sync but when I try again it works.
I do see your point. However I can't see any way to implement this. Consider doing isSynchronized on a resource in an operation with a resource rule held. If issync returns true (and resources are out of sync) the asynchronous refresh will not be able to refresh. So resource changing operations will still fail with out of sync. As the refresh is asynchronous, there's a window here. And if any res rules are held, e.g. during a build, the widow may be large.
Also worth noting that isSync stops when it finds the first out of sync resource. So if there's more than one, only the first will get refreshed. It's a good hint to the user that they might want to refresh the full project.
(In reply to comment #2) > The current behavior is very irritating for a user: first time I do something I > get the out-of-sync but when I try again it works. On this point, all the resource modification methods will exhibit this behaviour. In 3.8+ we could be even more radical and make PREF_LIGHTWEIGHT_REFRESH banish out-of-sync as an error condition from all methods: reading or writing. If we did this, then always return'ing true from #isSynchronized would make sense. This would make the preference enforce Martin's 'filesystem is king' semantics. What do you think?
Don't get me wrong: I see we have a dilemma here ;-). >On this point, all the resource modification methods will exhibit this >behaviour. Yes, but isSynchronized() is not a modification method. Other methods like getContentDescription() or getContents() already act like the refresh happened. Maybe we could enable it just for IFile? Currently the benefit of the new 'Refresh on access' is quite limited with this restriction.
I agree with James this has to be part of a large change, if anything. If we changed the behaviour of isSynchronized to match the behaviour of getContents, it would no longer match the behaviour of other methods, for example: if (file.isSynchronized()) //return true file.delete(); // throws out of sync While "cheating" on getContents doesn't bother me much because it is non-damaging, I do worry about playing the same tricks with other modifying operations. For example, I see a folder in the navigator that appears to be empty. I hit delete, thinking it safe to delete an empty folder. But no, the folder actually contained several files I didn't know about, and now I just deleted them without even being aware of it. While I completely understand the "file system is king" perspective, I can easily see this going wrong. For example I just pulled from git on the command line, and didn't notice that someone added those files. Now I agree the current behaviour is weird, that I just try again and it works. As a user I would prefer a prompt like, "The file has changed externally, do you want to delete it anyway?"
> Now I agree the current behaviour is weird, that I just try again and it > works. > As a user I would prefer a prompt like, "The file has changed externally, do > you want to delete it anyway?" You get that on the first try. So maybe we should remove the behind the scenes refresh from isSynchronized() again?
(In reply to comment #8) > > As a user I would prefer a prompt like, "The file has changed externally, do > > you want to delete it anyway?" > You get that on the first try. So maybe we should remove the behind the scenes > refresh from isSynchronized() again? That's not what I see. I will attach a screen shot.
Created attachment 194287 [details] Screen shot On delete, I get a dialog that says "Confirm Delete". However the text just says the file is out of sync, and hitting "Ok" does not delete it, as one might expect from the title.
(In reply to comment #8) > > As a user I would prefer a prompt like, "The file has changed externally, do > > you want to delete it anyway?" > You get that on the first try. So maybe we should remove the behind the scenes > refresh from isSynchronized() again? I don't see how that would make no difference to the flow in this case: boolean force = false; if (!file.isSynchronized()) { force = //prompt: "The file has changed externally, do you want to delete it anyway?" if (!force) // nothing to do return; } file.delete(force, true, null); The async refresh is best effort (delayed by 200ms). If a dialog is displayed there may well be time for external changes to happen between the isSync check and the delete call. The isSync check just serves to provide information that the user should be prompted for 'force'. AFAICS whether a refresh is scheduled or not shouldn't change how the API is used.
(In reply to comment #10) > Created attachment 194287 [details] > Screen shot The dialog is quite neutral. It doesn't tell the user they need to refresh (and the refresh now happens automatically), so simply retrying will work. Perhaps it would be better if it prompted as you suggested, using the outcome of the prompt as the value of 'force'.
> > You get that on the first try. So maybe we should remove the behind the scenes > > refresh from isSynchronized() again? > > I don't see how that would make no difference to the flow in this case: It would make sure that I always get the same behavior (dialog) unless I refresh. The whole point of the 'Refresh on access' was to make it work behind the scenes without clients being forced to change their code. If we now say that's needed (e.g. tweak dialogs) then we can remove all that again and force clients to check the old 'auto-refresh' preference and then act accordingly.
(In reply to comment #9) > (In reply to comment #8) > > > As a user I would prefer a prompt like, "The file has changed externally, do > > > you want to delete it anyway?" > > You get that on the first try. So maybe we should remove the behind the scenes > > refresh from isSynchronized() again? > > That's not what I see. I will attach a screen shot. I was only referring to getting a dialog. So far no dialog in the SDK has this additional button to refresh. If start to do that then ever action needs to be changed to offer it.
(In reply to comment #13) > The whole point of the 'Refresh on access' was to make it work behind the > scenes without clients being forced to change their code. If we now say that's > needed (e.g. tweak dialogs) then we can remove all that again and force clients > to check the old 'auto-refresh' preference and then act accordingly. I'd argue that if you find yourself needing to make changes now, then the existing dialog behaviour is wrong. What we have now is identical to what we have on Windows (for example) with auto-refresh enabled. i.e. resources are already asynchronously refreshed shortly after they're modified. If #isSynchronized returns false, then the IResource tree passed to #isSynchronized really needs to be refreshed, and if appropriate the user should be notified. Post 3.7 you needn't check #isSynchronized before #getContents(). #get isn't resource changing. But for modification methods, whether you check with #isSynchronized, or check by catching a CoreException from the resource modification operation, the UI flow for the user should be the same, no?
Interestingly I'm having great difficulty reproducing this on a Mac. Using I20110310-1119, with new core.resources. I can't get delete to notify me of out-of-sync with or without refresh on access on. Using Project Explorer and and an IFile: Every time I click on a file in project explorer it schedules an async decoration that does #getContentDescription: FileSystemResourceManager.asyncRefresh(IResource) line: 135 FileSystemResourceManager.isSynchronized(IResource, int) line: 681 File(Resource).isSynchronized(int) line: 1498 File.getContentDescription() line: 265 ContentTypeDecorator.decorate(Object, IDecoration) line: 47 LightweightDecoratorDefinition.decorate(Object, IDecoration) line: 269 LightweightDecoratorManager$LightweightRunnable.run() line: 81 SafeRunner.run(ISafeRunnable) line: 42 LightweightDecoratorManager.decorate(Object, DecorationBuilder, LightweightDecoratorDefinition) line: 365 DecorationScheduler.queueForDecoration(Object, Object, boolean, String, IDecorationContext) line: 140 DecorationScheduler.getResult(Object, Object, IDecorationContext) line: 212 DecorationScheduler.decorateWithOverlays(Image, Object, Object, IDecorationContext, ResourceManager) line: 179 DecoratorManager.decorateImage(Image, Object, IDecorationContext, ResourceManager) line: 584 DecoratorManager$ManagedWorkbenchLabelDecorator.decorateImage(Image, Object, IDecorationContext) line: 149 NavigatorDecoratingLabelProvider(DecoratingStyledCellLabelProvider).getImage(Object) line: 173 CommonNavigatorManager.updateStatusBar(ISelection) line: 308 CommonNavigatorManager$1.selectionChanged(SelectionChangedEvent) line: 74 With refresh on access off, force is always true, so I still don't see out-of-sync warnings: DeleteResourceChange.<init>(IPath, boolean, boolean) line: 72 DeleteResourcesProcessor.createChange(IProgressMonitor) line: 196 DeleteRefactoring(ProcessorBasedRefactoring).createChange(IProgressMonitor) line: 292 CreateChangeOperation.run(IProgressMonitor) line: 124 UIPerformChangeOperation(PerformChangeOperation).run(IProgressMonitor) line: 209 Workspace.run(IWorkspaceRunnable, ISchedulingRule, int, IProgressMonitor) line: 2326 WorkbenchRunnableAdapter.run(IProgressMonitor) line: 87 ModalContext$ModalContextThread.run() line: 121 I see the same thing deleting a Folder which has an added file which eclipse is unaware of. While delete checks isSynchronized: Folder(Resource).isSynchronized(int) line: 1498 DeleteResourcesProcessor.checkFinalConditions(IProgressMonitor, CheckConditionsContext) line: 127 DeleteRefactoring(ProcessorBasedRefactoring).checkFinalConditions(IProgressMonitor) line: 224 CheckConditionsOperation.run(IProgressMonitor) line: 85 CreateChangeOperation.run(IProgressMonitor) line: 121 UIPerformChangeOperation(PerformChangeOperation).run(IProgressMonitor) line: 209 Workspace.run(IWorkspaceRunnable, ISchedulingRule, int, IProgressMonitor) line: 2326 It does nothing with the result, and DeleteResourceChange is created with 'force' always set to true. So on my mac, with or without asyncRefresh on, I can't figure out how to get delete to notify me of out-of-sync.
My whole point is that the new feature is useless for most file (modify) scenarios as you still get the old behavior (warning dialogs). Maybe the preference should be renamed to 'Schedule refresh on access'. This lets the user better understand why the first deletion still fails but the second one works.
(In reply to comment #17) > My whole point is that the new feature is useless for most file (modify) > scenarios as you still get the old behavior (warning dialogs). > > Maybe the preference should be renamed to 'Schedule refresh on access'. This > lets the user better understand why the first deletion still fails but the > second one works. If the UI wants to care about out-of-sync (i.e. force = false) then this issue exists with or without isSynchronized() returning true. The only fix I see would be a more radical 'filesystem is king' approach from comment 5.
> The only fix I see > would be a more radical 'filesystem is king' approach from comment 5. I agree, this is definitely too late now but worth trying during 3.8.
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. If the bug is still relevant, please remove the "stalebug" whiteboard tag.
