Community
Participate
Working Groups
Now that editors can (should?) use a common document from the FileBufferManager, I think the editor's undo management should be "keyed" off of that common document (I'm guessing location?), instead of each instance of an editor. This is easiest to conceptualize by having multiple workbench windows open, with an editor opened in each on a common resource ... but even in one workbench, there's multiple ways of modifying a resource. In WTP, we've found this to be user's expecation that undo/redo be based on the resource the are modifing (not based on a particular UI widget or method of interaction. Plus, in WTP, -- to change this undo behavior -- is one of the major reasons we have to subclass SourceViewer, etc. So ... thought'd I'd open this "bug" for comment, to see if there was fundamental differences in philosophy, or just a matter of who implements what when. Thanks.
David, for me this works, i.e. if I open the same document in two windows (or as of today ;-) in the same window, type in one editor, go to the other and then undo, it undoes the last change applied to the document. Or did you think of another scenario?
Thanks Dani [same document, same window ... wow, I can't wait to see what else we need to catch up with -- or get for free!] What I'm thinking of needs one more step. If modication is made in editor A, then in editor B, you pick "undo x", then its current undo stack says "redo x" to ... well, redo the operation on the document. But editor A, to redo the modification made to the document (via undo-operation operation in B), the user needs to pick "undo x". I think conceptually its because the editors considers the two text changes as different things, but from the shared document's point of view, they are the same thing. And, for our users also the same thing (and, in admittedly more complicated scenerios) they think of it in terms of changes to the shared document/resource/object they are working on -- the history of the document, in other words, not the history of the particular UI that happened to make the change. Thanks for any insights.
I found the last comment a little hard to parse. So you're saying with two editors on the same file open in two windows I can make a change in either editor and I see the Undo option available in both editors and when I undo in either it has the same effect. But after an undo, only the editor (or window) where I performed the undo will show redo, the other editor shows undo. If this is true then I agree this seems inconsistent at best.
I think I understand the comment. He is saying it's not enough for the two editors on a single document to remain in sync, but that once a user performs "Undo typing" (let's call it Undo typing of "XYZ") in one of those editors, both of those editors should show "Redo typing" in their menus, and both of those choices should redo the typing of "XYZ". In effect, the editors share the same undo history. Is this what you mean, David? The undo framework would theoretically allow this if each editor had the same undo context, but currently each undo manager is set up with its own private (per-editor) context. If the undo context was shared between them, the behavior David describes would be possible. From the point of view of the undo framework it's a matter of what the users want/expect. Both are possible. But to be honest, I'm not sure if the current DefaultUndoManager implementation in JFace text would correctly support it. It might work for free or it might be a lot of work. Would have to try it. I could investigate this if there's consensus that it should be pursued.
Yes, Michael and Susan, I think you are both parsing my obtuse comments correctly. I just tried with "open again" in with M7 stream I-0426 (and placed opened editors side by side), and honestly, I could not even figure out how to write down an confusing example because it was so confusing. But, definitely undo in one becomes redo in other which once again becomes undo in the other ... or something. And Michael's comment of "this seems inconsistent at best" is especially accurate. At best it is confusing. At worst it is an architectural flaw that "common undo" is not so common. I apologize for not being able to investigate deeper and offer concrete suggestions, but my suspicion is there is not so much a "common undo api" as there is a "common hidden api" that every client has to know the secret of and re-implement to work as expected. [And, that's not to dimenish all the hard work that's gone on here, just to re-state that is a hard problem to solve, and you have a good start -- but this simple case of documents and editors seems, to me, to be the first apparent failure of implementing it correctly].
My comment was to explain that it has not been a goal to date to share undo stacks between editors on the same document but it is technically possible in the new undo framework. The reason it is not working as you expect is because text undo uses the common framework, but defines its own private undo context for each editor. Changing it is a matter of using the same undo context for the same document and testing out the implementation of DefaultUndoManager to see if it works for free (it might) or requires some changes. There's no hidden magic, it should be possible today but has not been set up that way or tested.
Thanks Susan, your phrase of "private undo context" is undoubtedly more accurate and less loaded than my "hidden api" phrase, and I think you've stated the issue well. The concern I was trying to voice (which probably goes beyond this specific bugzilla, which is just one visible instance of my concern) is if something as common as documents and editors have a "private undo context" then Eclipse as a platform does not have a "common undo" story or design (and that's no reflection on your framework ... mabye its a policy or larger design issue that has yet to be coordinated -- I'm not sure how "Eclipse as a platform" plans to handle "common undo"). I hope my appreciation of your work and the sincerity of my confusion comes across in these written notes ... and, I'm sure its only a lack of my understanding and imagination that can not grasp how an editor and a refactoring can "share" undo (one of your prime use caess), but two editors currently do not.
Hi, again David. I think the best place to voice concerns about the general undo strategy/policy in Eclipse is in bug #86753. There is not a coordinated story right now, as most of the time we've put into it has been getting the existing undo clients (text, refactoring) integrated. There is a lot to decide about the undo strategy overall and it looks like that is going to have to happen in the 3.2 time frame. At any rate, your comments/ideas about overall strategy, what should be undoable, etc. etc. would be much appreciated in bug #86753. We'll keep this bug open as the request to have text editors share undo when they share a document. Dani - if you want to punt this bug to me for experimentation, you can do so, but it won't be right away until other bugs are dealt with. As I've said, it might "just work" but I've just been too tied up to try it.
Basically, you can go back and forth between two editors on the same document, and keep undoing forever (which is really just cycling forward and backward through the user's real modifications). However strange, I just verified this is consistent with 3.0.1 behavior.
The goal for 3.1 was to provide the basis for an unified undo history and move existing code and functionality to it. This has been delivered by the new operation history API and has been validated by implementing text editors and refactoring on top of this new infrastructure . The missing piece i.e. sharing the undo context per document - as correctly reported in this PR - this can be implemented with the operation history API but requires new API in Platform Text that allows to share the undo context for a document. Though we could hard code this into the DefaultUndoManager I think making this explicit and let clients decide is the right approach. The shared document is managed by the File Buffers plug-in, but this isn't a good place for a shared undo context because undo is introduced by org.eclipse.jface.text in a layer that doesn't know about File Buffers. Offering shared undo context per document is something we want to provide for 3.2.
*** Bug 99642 has been marked as a duplicate of this bug. ***
*** Bug 66926 has been marked as a duplicate of this bug. ***
*** Bug 103855 has been marked as a duplicate of this bug. ***
I've done a little experimenting on this one, and it's a bit more complicated than simply sharing the undo context between undo managers. The current JFace text undo manager is assigned per viewer. It responds to document content changes by building an undoable operation (TextCommand) describing the change. But if we change things so that each editor still has its own undo manager, and they share the undo context, then each undo manager will create a separate undoable operation when it receives a document change notification. This results in duplicate operations in the shared context. There are probably ways to work around this (recognize duplicate changes) but that would be hacking around the problem. I think it makes more sense to create the undo manager per document, and assign a shared undo manager (with a shared context) to the viewer. This will also help in managing the life cycle of the undo context, since you don't want viewer-based undo managers disposing of an undo context that may be used by someone else. Dani - any comments on this? I have time to investigate further, but am curious on your thoughts regarding a shared undo manager.
(In reply to comment #14) > There are probably ways to work around this (recognize duplicate changes) but > that would be hacking around the problem. I think it makes more sense to > create the undo manager per document, and assign a shared undo manager (with a > shared context) to the viewer. Having an undo manager per model object (document) is quite certainly the right way. Probably not the right forum... but: It might be worth thinking about whether o.e.text needs a UI agnostic *editing model* besides the already existing *text model* (IDocument) - it would offer services currently managed by the viewer but really not depending on the presentation. Things that come to my mind: - compound changes (which are important not just for undo) - undo (IRewriteTarget) [- find/replace (IFindReplaceTarget)] - patch documents: recording a minimal edit script while freely modifying a document. The underlying document is not changed until the script is applied, but one can get a 'patched' view of what the document would look like if it were. I filed bug 104461 to investigate this.
I also had an undo manager per document in mind as backup. If this is the path to follow we - have to make sure the current undo manager uses the new one and works as is i.e. clients don't have to change their code and switch to the new undo manager - have to put it into org.eclipse.text so that headless plug-ins like file buffers can profit from this as well
If we go down the path of one undo manager, it means we are not giving clients a choice as to whether the undo model is shared across editors on the same document (see comment #10). However I still think it's the preferred way to implement this behavior. What I see happening is that the current undo manager would be largely a wrapper into the document's undo manager. Note that the current undo manager does use mouse down and arrow key presses to commit undo changes, so there would still have to be a way to do programmatic commits into the document's undo manager. The rest of the API seems as if it could be supported as before. There would be life cycle differences since the viewer's undo manager would be largely meaningless until the viewer it connected to had its document. (The document is null on connect in today's implementation). Side question: would moving to this strategy also help us solve bug #93955? (transferring undo context after rename). When a CU is renamed, does the document get regenerated? If the document stays the same, this bug would be fixed "for free." How do you want to proceed? I have cycles to separate out the undo manager but would feel better if someone wrote stubs for the API and classes in the text world so that things are in the right place.
>I've done a little experimenting on this one, and it's a bit more complicated >than simply sharing the undo context between undo managers. Agree, but I guess the other solution might even even more complex ;-) >If we go down the path of one undo manager, it means we are not giving clients >a choice as to whether the undo model is shared across editors on the same >document (see comment #10). That's already the case now (sort of): if two editors use a shared document then the currently different undo managers undo the changes on the shared document already i.e. they are somehow connected. If we still want to support this it could be a configuration of the current DefaultUndoManager. >Side question: would moving to this strategy also help us solve bug #93955? It depends at which level the new undo manager would be and whether it knows about resources or file names and whether the document is even the same after the rename. >How do you want to proceed? I have cycles to separate out the undo manager but >would feel better if someone wrote stubs for the API and classes in the text >world so that things are in the right place. The API has to come from the one who works on the item. It makes no sense that we provide a stub if we're (currently) not directly working on this problem. Please go ahead and make a prototype that shows the separation of the undo manager (shared one probably in o.e.text) and attach a patch to this PR or transfer the work item back to us.
I'm working on this one right now. My current approach (for initial simplicity and flexibility): - keep DefaultUndoManager as is for now. - introduce a new shared undo manager in o.e.jface.text that is largely a wrapper into the document undo manager - implement the DocumentUndoManager in o.e.text Once the shared undo manager passes the same test suites, I will attach the patch and return this bug to Platform Text. At that point you should be able to massage the API as you see fit and decide how you are deciding which undo manager to create (preference driven or switch to new model). Note that keeping both styles will introduce dual maintenance in the long run (bugs in the original DefaultUndoManager may have similar variants in the document undo manager.)
One thing about the UndoManager API: IMO, IUndoManager has a serious limitation: it relies on clients to correctly begin/end compound changes. This becomes a problem in the case where client A only detects the end of its compound change when client B starts editing the document (and has therefore already started its compound change). I believe that compound changes should be keyed, so that the undo manager knows exactly which compound change is being ended when it receives a endCompoundChange message. This would allow the manager to detect misbehaving clients and to handle the case of overlapping compound changes (which could be trivially resolved if there are no actual document modifications while the two changes overlap, which happens if client A ends its change when it receives a documentAboutToBeChanged message). An UndoManager API could either return an opaque token in beginChange that the client uses when ending an change: class UndoManager { Token beginCompoundChange(); void endCompoundChange(Token token); } Or beginChange could return a change object which the client could cancel directly: class CompoundChange { void end(); } class UndoManager { CompoundChange beginCompoundChange(); }
I am attaching patches for a prototype of a SharedUndoManager and transferring this bug back to Platform-Text for comment. The following comments are intended to be read when the patches are being examined: Functionality Differences: - when multiple editors are open on the same document, they now share undo history. For example, if the user has made several undoable changes in file A and then opens another editor on file A, the complete history is available in both. Undoing in one will cause the "Redo" for that operation to appear in both editors. I think this achieves the desired expectation - However, when an undo or redo occurs in one editor, the effect of the change is selected and revealed in BOTH editors. This is different than before, when the undo was not tied together. You could undo in one without affecting the visibility on the other. Now that undo executes in both editors, both will select and reveal the undo. API Notes - IDocumentExtension5 defines a document that supplies an undo manager, IDocumentUndoManager - IDocumentUndoManager defines an undo manager for a document, and the corresponding implementation, DocumentUndoManager is based on the 3.1 non- viewer parts of DefaultUndoManager. The intention is that this undo manager does NOT have to implement IUndoManager, but rather this is a new style of undo manager related to document models, which assumes that the client can use the undo context and operations history API to perform the undo and redo. Thus the API is leaner than IUndoManager. - The presence of IDocumentUndoManager and its implementation in the org.eclipse.text plug-in adds dependencies to this plug-in (the same dependencies introduced onto org.eclipse.jface.text when the operations history was integrated). - Re: compound changes (Tom's comment #20): If we believe the potential misuse of begin/endCompoundChange is increased by sharing an undo manager, we could choose to implement a keyed compound change in IDocumentUndoManager rather than change IUndoManager. If this problem is already arising in 3.1, then it is probably best to change it in IUndoManager and forward into IDocumentUndoManager. I did not attempt to address this issue in the prototype, as I want to demonstrate compatibility with 3.1. - SharedDocumentUndoManager is API equivalent to DefaultUndoManager. It maps its API to the underlying document undo manager. The big decision here is whether we should retain both classes (so that users can choose which style of undo to use) or replace the old DefaultUndoManager with the implementation defined in SharedDocumentUndoManager. Due to the complexity of the undo managers, my preference would be to eventually replace DefaultUndoManager with SharedDocumentUndoManager. The patches use a bogus local flag to decide which undo manager to use (left this way for testing/comparing). The JFace Text Undo tests and the JDT UI leak tests were patched to test both implementations for now.
Created attachment 26122 [details] org.eclipse.text.patch patches to eclipse text - IDocumentUndoManager and implementation classes
Created attachment 26123 [details] org.eclipse.jface.text.patch patches to jface text - SharedDocumentUndoManager which could replace DefaultUndoManager
Created attachment 26124 [details] org.eclipse.ui.workbench.texteditor.patch AbstractTextEditor - changes related to new life-cycle of undo context
Created attachment 26125 [details] org.eclipse.ui.editors.patch source viewer configuration - creates new style undo manager.
Created attachment 26126 [details] org.eclipse.jdt.ui.tests.patch updated leak test suite
Created attachment 26127 [details] org.eclipse.jface.text.tests.patch updated jface text undo manager test suite
Thanks for the patches Susan. I did not have time yet to look into them but have a first comment about the functional difference: this is not a valid option. If we have two different editors (i.e. views) on the same model and change the model through one of them then we must only updated the minimum. Changing the context i.e. current selection is not an option unless of course that selection itself is included in the change. How tight are your patches tied to the new behavior?
This behavior is not too tightly bound to the new model, it was just expedient to preserve the old undo manager behavior. To change it so that the only the originating viewer does the select/reveal, the viewer's undo manager needs to know which viewer triggered the undo/redo. The mechanism is there (the adaptable passed by the action handler to the operation history). In order for the undo manager to make a viewer-based comparison, the editor needs to provide API (or more likely an adapter) that provides its text viewer, so that the action handler can provide this info on request when the undo manager makes its comparison.
It was pretty simple to fix the select and reveal issue. You must have the org.eclipse.ui.workbench from HEAD to get the OperationHistoryActionHandler fix in bug #108144 before it will work. Attaching new patches. I had to change AbstractTextEditor to provide an adapter for its text viewer. The other changes were to ensure the IAdaptable is passed all the way through the event flow, which it should have been in the first place.
I must be missing something. Was it a problem that all viewers reset their selection, or simply an implementation problem, e.g., the undo history doesn't have the selection in terms of all possible viewers?
Created attachment 26542 [details] org.eclipse.jface.text.patch
Created attachment 26543 [details] org.eclipse.text.patch
Created attachment 26544 [details] org.eclipse.ui.workbench.texteditor.patch
re: comment #31: Bob - Dani was saying that it was a problem for all viewers to respond with a select and reveal on undo. When there are multiple open viewers/editors on the same document, typing or making changes in one is reflected in the content of the others, but the user's context (selection, cursor position, scrolling position, etc.) is never changed. My original patches were violating this. The problem was that only the viewer originating the undo should perform a select and reveal. What was missing was a way for the viewer's undo manager to know which viewer triggered the undo, so it could figure out whether it should select and reveal. That is what today's patches fixed.
Dani - please see the patches attached to bug #93955 before examining these patches in detail. The inner types of DocumentUndoManager had to be promoted to support the rename CU scenario, causing a significant bit of refactoring of these patches. I will attach all relevant patches to bug #93955 for completeness.
Comment on attachment 26125 [details] org.eclipse.ui.editors.patch see updated patches in bug #93955
Comment on attachment 26543 [details] org.eclipse.text.patch see updated patch in bug #93955
Comment on attachment 26126 [details] org.eclipse.jdt.ui.tests.patch see updated patch in bug #93955
Comment on attachment 26544 [details] org.eclipse.ui.workbench.texteditor.patch see updated patch in bug #93955
Just a reminder that this involves API change and must be settled during M5.
*** Bug 93955 has been marked as a duplicate of this bug. ***
First cut has been released to HEAD, in builds >= I20060124-0800. See bug 93955, comment 40 for a more detailed updated.
Fixed in HEAD. Available in builds >= N20060128-0010.
