Community
Participate
Working Groups
Imagine you're editing some file in Eclipse but when you want to save it Eclipse says the file has changed on disk. For example, another person edited and saved the file while you were typing. In this situation, Eclipse asks if you want to override the file or cancel your modifications. If you don't take care, some work may be lost. I think a better behaviour is giving the developer a comparison between the edited text in memory and the conflicting text on the disk, using the 'Compare With' infra-structure. This way you can compare what changes are between your version in memory and the one on disk, and decide better what to do, including a merge. I think it wouldn't be hard to implement, it's just about integrating existing components. This enhancement is similar to CVS conflicts, but this is not about Eclipse donig CVS jobs. This enhancement would be nice because the current behaviour in my opinion is bad.
I think the place to change is the method org.eclipse.ui.texteditor.AbstractTextEditor.handleExceptionOnSave, from CVS module /cvsroot/eclipse/org.eclipse.ui.workbench.texteditor. This method would then rely on org.eclipse.compare to launch the diff view. The problem is that org.eclipse.ui.workbench.texteditor is already a dependency of org.eclipse.compare, and I don't know how to deal with that.
I think a possible solution is ui.workbench.texteditor calling an observer like IOutOfSyncOnSaveHandler or so, and let org.eclipse.compare register itself as such observer on plugin startup. This way texteditor won't need to reference compare.
(In reply to comment #2) "On startup" is a no-go. A solution would be to define an extension point in the text editor plug-in and contribute to it from the compare plug-in.
If it's only about showing the diff, we could use the RangeDifferencer from compare.core and visualize it.
I think it should work like this: when out of sync is detected, instead of choosing to override or not, you'd choose what to do from a list of possible actions, which would include at least one implementation for the extension point and also the default option to override directly. Something like this: The file has changed on file system. What do you want to do? (o) Compare file system and editor versions ( ) Override file anyway [ Ok ] [ Cancel ] If there's no implementation, then the default current dialog would be used. If compare is selected, then compare plug-in would open a diff view (and maybe close the editor). The file system version would be read-only, so that you choose which changes to import to the editor. If you want to discard editor changes, you'd just delete the lines. When saving changes in diff view, the file would be overwritten but the old disk version would be kept in memory or temp file for further comparison. The end of the merge would be only when the user close the view with saved changes. If he/she didn't save anything, it would be asked about overwrite or not. When the view is closed the editor comes back with the resulting version: from merge, editor or file system.
(In reply to comment #5) > When saving changes in diff view, the file would be overwritten but the old > disk version would be kept in memory or temp file for further comparison. The > end of the merge would be only when the user close the view with saved changes. Or simply confirm save operation (no need of temp files / memory copy).
Created attachment 127096 [details] Partial implementation Extension point implemented. When out-of-sync happens, the available contributors are retrieved from extension registry, and a new dialog is used for asking user on what to do. For each available handler, this dialog will add a new item in the action list. I think that dialog font could be bigger, and items could have an icon, or either another kind of dialog should be used (with radio buttons for example). A default out-of-sync handler was created in-line, doing the same as answering "yes" on the old dialog, as well as a proptotype handler in org.eclipse.compare, just showing a simple message. The extension point interface (actually, an abstract class) needs to have the handler method parameters defined, so that the handler will have access to both current and the file system versions. I didn't work on that.
Created attachment 127098 [details] Partial implementation Added possible parameters for handle method: IDocumentProvider and IEditorInput. Improved compare handler prototype so that now it shows the current editor version and the file system version. To retrieve this last one, I had to require core.filesystem and change some code in ui.editors. I couldn't do this in ui.workbench.texteditor because it's already a dependency of ui.editors. I think developers more experienced with the code might improve this behavior so that a direct file reference can be passed to the handler method, instead of requiring the implementation to retrieve it by itself (as in compare prototype). The problem is that IDocumentProvider and other stuff seems not give access to the file system version (as ui.editors does, but can't be used to avoid a cycle).
Also, I noted that the compare implementation would be pretty similar to "resource > compare with > local history > most recent". I think it might be just about reusing the code.
Created attachment 127104 [details] Proposed extension point implementation and compare contributor prototype The dialog now has a new style, using radio buttons and a warning icon.
Created attachment 127105 [details] Proposed extension point implementation and compare contributor prototype
Created attachment 127106 [details] Proposed extension point implementation and compare contributor prototype
Let's take a closer look at this during 3.6. Just a heads-up: making any of the org.eclipse.ui.editors.text.TextFileDocumentProvider.getFile* methods public is a no-go.
(In reply to comment #13) > Let's take a closer look at this during 3.6. Just a heads-up: making any of the > org.eclipse.ui.editors.text.TextFileDocumentProvider.getFile* methods public is > a no-go. > I think this modification was just for the compare prototype, maybe it won't be necessary in the actual handler.
The prototype patch needs to be finalized before we can continue on it. Especially, it has to - also handle the case when switching from a non-Eclipse editor where a change was made to the Eclipse editor - show the compare dialog which allows to copy/merge the changes (we could even open the compare editor instead of showing the dialog) Also, we don't need to allow to select the handler. The extension point should specify that the last one wins (see org.eclipse.ui.texteditor.ConfigurationElementSorter). Some other initial comments to the patch: - the schema definition is incomplete - answer / getAnswer is not clear - OutOfSyncHandler doesn't provide any real implementation and hence that should be internal and the API that needs to be implemented specified via an interface - TFDP.getFileStore(Object) is not needed: instead, simply get the document using IDocumentProvider.getDocument(editorInput) - CompareOutOfSyncHandler must not be API - java.lang.StringBuilder cannot be used since compare must run on 1.4 - the exception handling is bad: - catching 'Exception' is a no go - printing to the console (e1.printStackTrace()) is a no go - strings in plugin.xml need to be NLSed - missing since tags - all new files should have * Copyright (c) 2010 YOUR NAME and others. - copyright date of existing files needs to be update 2010
(In reply to comment #15) http://pastie.org/806068.txt
Renato, please put your comments/questions directly into this bug report so that we can related the response(s) to them. Also, the other link might disappear in the future.
(In reply to comment #17) I would just like to chat with you and clarify things, so that we don't write too many comments here, specially those for misunderstandings. But ok here we go. The patch as a whole is not a prototype, it defines the basic infrastructure upon which this feature would be implemented: plugins can contribute handlers to the out-of-sync situation, when Eclipse will ask the user what to do. Each possible answer represents one handler (that's why it needs a getAnswer(), got it?). By default there would be two handlers: the default, inline "overwrite anyway" handler, and the compare handler which would be contributed by the compare plugin. I was thinking that we in the future, or some external plugin, may want to add new kinds of handling. Example: imagine one creates a plugin for semantic diffs. You may want to know whether two CSS files have the same functionality, that is, whether they are semantically equal. You don't care about the order of the lines as in default diffs, you just want to know if they'll produce the same result. Well, such plugin would probably want to contribute its smart comparison to the out-of-sync event, so that you can be like "oh, I just moved this stuff to top and the smart diff is telling me it doesn't change the functionality at all", rather than "hmm let me see all these diff lines and check if the change is relevant". Dani wrote: - also handle the case when switching from a non-Eclipse editor where a change was made to the Eclipse editor - show the compare dialog which allows to copy/merge the changes (we could even open the compare editor instead of showing the dialog) Well, as in the patch title: Proposed extension point implementation and **compare contributor prototype**, so yes, someone needs to complete it. I didn't because I had/have no expertise/time. About the dialog, it would work as described above and in the patch. First, one dialog would be opened to ask the user what to do: compare, overwrite anyway, [other handlers], cancel. After selection, the underlying handler would be activated. I don't think showing the compare view directly is a good idea, because the user may want to overwrite anyway, or cancel. Besides, it would break the out-of-sync-open-handling idea (it may not be a problem, although would require more patch rewriting). Besides, aren't compare editor and compare dialog the same thing? Dani wrote: Also, we don't need to allow to select the handler. The extension point should specify that the last one wins (see org.eclipse.ui.texteditor.ConfigurationElementSorter). I don't think this applies here. Each contributor should be listed in the what-to-do dialog. No handler should replace any other. Dani wrote: - OutOfSyncHandler doesn't provide any real implementation and hence that should be internal and the API that needs to be implemented specified via an interface - CompareOutOfSyncHandler must not be API OutOfSyncHandler is the base, abstract class. Contributors extend that class to provide an actual handler, for example CompareOutOfSyncHandler (which currently is just a prototype), but the class itself is not a handler. I think the code can be refactored to use an interface and move getContributors() to somewhere else, but is there really any special reason for this? "Should be internal" and "CompareOutOfSyncHandler must not be API" means just package naming/hiding? Dani wrote: - TFDP.getFileStore(Object) is not needed: instead, simply get the document using IDocumentProvider.getDocument(editorInput) Are they really the same thing? Iirc, the former is to get the version from disk, and the latter is to get the editor's content. Dani wrote: - the schema definition is incomplete - strings in plugin.xml need to be NLSed - missing since tags - the exception handling is bad: - catching 'Exception' is a no go - printing to the console (e1.printStackTrace()) is a no go - java.lang.StringBuilder cannot be used since compare must run on 1.4 - all new files should have * Copyright (c) 2010 YOUR NAME and others. - copyright date of existing files needs to be update 2010 Ok. I think a summarized, clear todo list can be created for people interested in completing the patch. By the way, it would be amazing if you guys moved to some DVCS like Bazaar or Git, raw patches are a PITA :)
Let me first start by saying that we like the feature in general, that's why I reviewed it in the first place. However, our resources are limited, hence we can only accept a complete polished good quality patch and as a rule in the Eclipse SDK we do not just release a framework without a concrete consumer. > The patch as a whole is not a prototype, it defines the basic infrastructure Yes, I know but even that part is not finished: just look at the schema documentation for example. >upon which this feature would be implemented: plugins can contribute handlers >to the out-of-sync situation, when Eclipse will ask the user what to do. Each >possible answer represents one handler (that's why it needs a getAnswer(), got >it?). ... >I don't think this applies here. Each contributor should be listed in the >what-to-do dialog. No handler should replace any other. I don't want to show a selection dialog to the user. This is overkill for an out of sync corner case: the compare dialog (or editor) is good enough and if not, there's still room for a product to override this. >Besides, aren't compare editor and compare dialog the same thing? One major difference is that the dialog is modal. >I don't think showing the >compare view directly is a good idea, because the user may want to overwrite >anyway, or cancel. Yes, of course the handler must offer those options. >I think the code >can be refactored to use an interface and move getContributors() to somewhere >else, but is there really any special reason for this? Yes, if the implementer doesn't get any value (e.g. default implementations) from the abstract class then we use an interface. > "Should be internal" and >"CompareOutOfSyncHandler must not be API" means just package naming/hiding? It means the class is not intended to be used by clients hence not API and hence must either be package visible in the API package or moved to a *.internal.* package. >Dani wrote: >- TFDP.getFileStore(Object) is not needed: instead, simply get the document >using IDocumentProvider.getDocument(editorInput) > >Are they really the same thing? Iirc, the former is to get the version from >disk, and the latter is to get the editor's content. Correct, but if you only want to read the file then you can do that via editor input and hence the addition of the new API is also useless. Also note that the final implementation of the compare out of sync handler will need the document since that one will be compared with the file contents. >so yes, someone needs to complete it. I >didn't because I had/have no expertise/time. Would you be willing to finish and polish the framework part as outline by my comments if we find a buddy for the compare side?
(In reply to comment #19) > Let me first start by saying that we like the feature in general, that's why I > reviewed it in the first place. However, our resources are limited, hence we > can only accept a complete polished good quality patch and as a rule in the > Eclipse SDK we do not just release a framework without a concrete consumer. > I don't want to show a selection dialog to the user. This is overkill for an > out of sync corner case: the compare dialog (or editor) is good enough and if > not, there's still room for a product to override this. I believe opening the compare view directly without telling the user why you're doing that may cause confusion, specially in the cases of shared editing. For example, imagine that while you're typing, the compare view simply pops up in your face, and so you ask wft. All this because someone else edited the file in the meanwhile, but you have no idea of this. Hence I believe Eclipse should first tell what's happening, offering the radio button options like overwrite/compare/[other...]/cancel. > >I don't think showing the > >compare view directly is a good idea, because the user may want to overwrite > >anyway, or cancel. > Yes, of course the handler must offer those options. But these options would be shared between all handlers. Even if there can be only one "winning" handler, every plugin developer would need to implement these options in his view. Even if I can't give you a nice example (the mentioned semantic diffs would be better off integrated into the existing compare view), I'm still inclined to believe that allowing multiple handlers and presenting them as radio button options on a selection dialog is a better approach. If you really want to just use the compare plugin, then I would try to find a way to no other plugin be able to fulfill the extension point. This way other plugins won't interfere in the out-of-sync handling. However, as I said above, I still think an explanatory dialog with at least three buttons overwrite/compare/cancel. > >Dani wrote: > >- TFDP.getFileStore(Object) is not needed: instead, simply get the document > >using IDocumentProvider.getDocument(editorInput) > > > >Are they really the same thing? Iirc, the former is to get the version from > >disk, and the latter is to get the editor's content. > Correct, but if you only want to read the file then you can do that via editor > input and hence the addition of the new API is also useless. Also note that > the final implementation of the compare out of sync handler will need the > document since that one will be compared with the file contents. Iirc at the time I wrote that patch the document object had no API to retireve the content from disk, maybe there's a new API or I missed something. But ok, one more todo. > >so yes, someone needs to complete it. I > >didn't because I had/have no expertise/time. > Would you be willing to finish and polish the framework part as outline by my > comments if we find a buddy for the compare side? Yes.
Correction: I still think an explanatory dialog with at least three buttons overwrite/compare/cancel **is needed**.
Rephrasing: However, as I said above, I still think an explanatory dialog is needed (in this case with three regular, static buttons overwrite/compare/cancel).
>Rephrasing: However, as I said above, I still think an explanatory dialog is >needed (in this case with three regular, static buttons >overwrite/compare/cancel). Yes I agree, we definitely need to tell the user what's going on i.e. we can't just open the compare dialog or editor without additional info. Currently the extension point is quite open i.e. it's just an out of sync handler. We could change that to 'compareOpener' and the dialog would allow to cancel, overwrite and open comparison (if an opener is contributed). The benefit of such an opener is that we could use it for other things too, e.g. a better fix for bug 144422. >Iirc at the time I wrote that patch the document object had no API to retireve >the content from disk, That's still true but from the editor input you can get the file or URI and with that you can of course fetch it from disk (easiest via file buffers). Tomasz, can you help us with the compare aspect of this bug (3.7 time frame)? What parameters are needed so that the compareOpener can open the compare dialog or editor (would be a parameter as well) and fill it with the necessary input? Keep in mind that Text does not know about Compare which means we can't use CompareEditorInput for example.
(In reply to comment #18) > Example: imagine one creates a plugin for semantic diffs. You may want to know > whether two CSS files have the same functionality, that is, whether they are > semantically equal. [..] You don't need a separate OutOfSyncHandler for this if you use the compare framework. There, you already have extension points org.eclipse.compare.contentMergeViewers and structureMergeViewers, which implement semantic diffs (you e.g. get them for free for .java files).
(In reply to comment #24) > You don't need a separate OutOfSyncHandler for this if you use the compare > framework. There, you already have extension points > org.eclipse.compare.contentMergeViewers and structureMergeViewers, which > implement semantic diffs (you e.g. get them for free for .java files). Yes, as I said above: "the mentioned semantic diffs would be better off integrated into the existing compare view". BTW, the semantic Java diff rocks!
(In reply to comment #23) > >Iirc at the time I wrote that patch the document object had no API to retireve > >the content from disk, > That's still true but from the editor input you can get the file or URI and > with that you can of course fetch it from disk (easiest via file buffers). > Iirc I tried to get the File/path from the editor input, but IEditorInput did not have an API for that. I have checked now, I would need to cast IEditorInput to call FileEditorInput.getFile() or so. However, isn't this bad code? What if the editor input is not a FileEditorInput?
(In reply to comment #23) > Currently the extension point is quite open i.e. it's just an out of sync > handler. We could change that to 'compareOpener' and the dialog would allow to > cancel, overwrite and open comparison (if an opener is contributed). The > benefit of such an opener is that we could use it for other things too, e.g. a > better fix for bug 144422. So you want compare plugin to contribute its compare view to 'compareOpener', so that the text editor can use it in these two situations (encoding, out-of-sync)?
(In reply to comment #27) > (In reply to comment #23) > > Currently the extension point is quite open i.e. it's just an out of sync > > handler. We could change that to 'compareOpener' and the dialog would allow to > > cancel, overwrite and open comparison (if an opener is contributed). The > > benefit of such an opener is that we could use it for other things too, e.g. a > > better fix for bug 144422. > > So you want compare plugin to contribute its compare view to 'compareOpener', > so that the text editor can use it in these two situations (encoding, > out-of-sync)? If I understood correctly, then yes, I like the idea. However, I think opening a search view would be better than compare for that bug: https://bugs.eclipse.org/bugs/show_bug.cgi?id=144422#c23.
> I would need to cast IEditorInput >to call FileEditorInput.getFile() or so. However, isn't this bad code? What if >the editor input is not a FileEditorInput? That code would be used in the opener which is driven by Text and hence we know which editor inputs are possible (i.e. those supported by textual editors). Of course we'd do 'instanceof' checks there and not just hard-cast. >So you want compare plugin to contribute its compare view to 'compareOpener', >so that the text editor can use it in these two situations (encoding, >out-of-sync)? I guess it would handle the whole opening and not just return the viewer/editor but I'm open here.
(In reply to comment #23) > Tomasz, can you help us with the compare aspect of this bug (3.7 time frame)? I would definitely like to help, I think this would be a nice feature to have. However, before I go through all the comments/patches I would like to tick off more items I have scheduled for 3.6. 3.7 sounds feasible to me.
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.
