Community
Participate
Working Groups
N20050506-0010 + ZRH-plugins from 20050506_1749 Rename Type/CompilationUnit flushes the editor undo stack and thus disables the undo operation in the editor and the outline view. However, the refactoring is still undoable from the Package Explorer. Steps (smoke test scenario): - open a CU and select the main type name - 'Refactor > Rename' to something else
Dirk, can you take a look at this one?
We need to investigate whether we can reuse the undo stack of the renamed CU.
*** Bug 95170 has been marked as a duplicate of this bug. ***
Dirk and I started to work on this one. We have code ready that knows about the rename and has access to both the new and the old context. The problem we did not yet solve is the correct migration of the operations from the old to the new context. API addition will most likely be necessary in order to address this bug.
I looked into this as well and the problem I encountered is that we need a mechanism to "move" an IUndoable operation from one context to another. This can be done using remove and add for normal operations however for triggered operations remove will actually remove the children since the text operation undos only belong to one context. Susan, I think that for the move we need API additions. If this is the case I opt to defer the PR to 3.2.
Deferred unless Susan has a low-risk fix/idea for transferring the context. Susan, can I move the bug to you for investing transferring a context during 3.2?
In your prototyped fix, which code is attempting to "replace" the contexts? On the editor side, you are correct that this cannot be done without new API. On the refactoring side, I believe this can be done without API additions, if you know you are dealing with the refactoring operation. If you remove the context of a TriggeredOperations and it matches only the context of the triggering operation, it will indeed split up the operation into its child commands, thus removing them from the TriggeredOperations. This is how the "flush" and "invalid refactoring operation" scenarios are made to work. However, if you first change (by add/remove) the context of the triggering operation (the UndoableOperation2ChangeAdapter), and then change (add/remove) the context of the TriggeredOperations, you should be okay. However I would expect that code doing the "transfer" doesn't know anything in particular about the operations it is transferring. If this is so, then, yes, please transfer the bug to me and we will investigate in 3.2.
This needs to be done on the editor/text side when the editor input gets changed upon rename.
Per bug #89599, we may end up sharing an undo manager (and undo context) per document. If this is implemented, we could consider maintaining the undo context for the renamed CU and there would be no need to "transfer" the undo context.
It depends at which level the new undo manager will be and whether it knows about resources or file names and whether the document is even the same after the rename.
On a rename CU, a new document is created, so sharing undo context per document does not help in this case.
I am about to attach a series of patches that can address this problem on the text editor side. The implementation is much simpler given a shared document undo manager, so these patches depend on the shared document undo manager prototyped in bug #89599. These patches actually supercede the prototype, because the inner classes of DocumentUndoManager (the text commands) had to be promoted to visible types in order to reassign their undo manager. The basic idea is that during an element move notification, the text editor must "connect" to the old undo manager to preserve its history, create the new document undo manager, and then transfer the history from one undo manager to the other. As Dirk pointed out early on, the Operations API must also support the replacement of an undo context, so that the TriggeredOperations properly handles its children. I verified these patches by rerunning all test suites and the following scenario: - open editor A - make some local text edits - rename the type to B - make some more local text edits you can undo the entire history, including the rename. You can also do this successfully while having the same document open in two different editors. Patches forthcoming...
(In reply to comment #12) > you can undo the entire history, including the rename. You can also do this > successfully while having the same document open in two different editors. Does this depend on the file open in two different editors sharing the same document? If so, did you test the case where two different editors using the old 2.0-style API with an IDocumentProvider have the same file open? I believe in this case the content is _not_ shared and the editors may well be out of sync. Not sure what you can do about it, but thought I'd mention it.
Created attachment 26768 [details] org.eclipse.core.commands.patch introduces IContextReplacingOperation. (I will not release this set of patches until the entire set is checked by platform text.)
Created attachment 26769 [details] org.eclipse.ui.tests.patch test case for context replacing operation - to be released when the API is released
Created attachment 26770 [details] org.eclipse.text.patch refactored DocumentUndoManager with history transfer - supercedes implementation in bug #89599
Created attachment 26771 [details] org.eclipse.jface.text.patch same as bug #89599 patches - included for one-stop shopping
Created attachment 26772 [details] org.eclipse.jface.text.tests.patch same as for bug #89599
Created attachment 26773 [details] org.eclipse.ui.editors.patch same as for bug #89599
Created attachment 26774 [details] org.eclipse.ui.workbench.texteditor.patch patches for bug #89599 + the transfer of undo history during element move
Created attachment 26775 [details] org.eclipse.jdt.ui.tests.patch Updated LeakTestSuite. Note: The LeakTestSuite will fail until bug #108600 is fixed. This leak was found while adding tests for SharedUndoManager leaks, but is not caused by its presence.
re: comment #13. This particular bug is addressing the rename of a CU and preserving the undo history when a new document is created for the renamed CU. In this case a new document gets created, but the editor knows it is a moved and can transfer the undo history to the new document. My comment about multiple open editors was to point out that the scenario from bug #89599 (share undo history when the document is shared) works even when the document is regenerated, as it is still shared by both editors after the rename. I'm not familiar with the 2.0 style API, but I can say that if two editors do not share the document then I would expect the editors (and their undo history) to remain out of synch in this case.
Created attachment 26804 [details] org.eclipse.jdt.ui.tests.patch updated leak test - counts instances before the test to take into account existing JavaTextTools refs.
Just a reminder that this involves API change and must be settled during M5.
Before I dig deeper into this I have two question: 1) wouldn't the change in the triggered operation be sufficient to solve this PR? 2) I assume that even after applying the patches the following scenario won't work: 1. open editor for A.java 2. rename A to B 3. close editor ==> can undo in Package Explorer when B is selected 4. open editor on B ==> undo no longer possible I think to solve this there needs to be a way to connect the matching global undo/redo operations with the open editor. Am I right here?
1) The change to TriggeredOperations/IContextReplacingOperation is necessary, but it is not the only part of the change. The editor must drive the transfer of the undo context when the input is replaced. That said, many of the other changes here are in support of undo history shared across documents (bug #89599), as this was implemented after those changes, on the assumption that both were important and necessary. It would be additional investigation to see if a simpler transfer could be done with the changes to the operations framework and more minimal changes on the editor side. I don't recall if there were specific roadblocks tying these two together. Let me know if you are considering addressing this one without addressing the other feature. 2)You are correct in your description of the scenario. When an editor is opened, its undo history begins then. There's no attempt to look through the history and find any older operations related to the editor. This is true in general (whether there was a rename or not.) I think this is acceptable behavior, so there's no plan to address that part.
I've now looked through the patches but they seem to suffer the same defect as my simple transfer solution: as far as I can see they don't fix this bug (see comment 0): 1. open cu A.java 2. select the A in the Java editor 3. Refactor > Rename ==> Edit > Undo is grayed out Besides that the patches look good and go into the right direction. Here some comments: - we do not want to provide access to the text viewer through the editor. I think the same can be achieved to use getAdapter(Control.class) and compare to fTextViewer.getTextWidget() [in SharedDocumentUndoManager] - I'm not yet sure about the name "SharedDocumentUndoManager", maybe simply "SharedUndoManager"? - we need to find another way to get the document undo manager. I don't like to see this attached to the document. In addition the current getter is strange since it starts logging the changes only after calling it. And of course we don't want to set it up upfront for all documents. I'm currently working on finding a better place for this. - the *.jface.* package names in org.eclipse.text are due to some code refactoring that took place a while ago. New code should have the org.eclipse.text prefix. I'd suggest org.eclipse.text.undo How do we want to proceed? I can do the package renaming and find a better place to setup and keep the document undo manager. Could you investigate whether I'm right that the patches don't fix this bug and if so, look into this?
My initial idea was to use the document provider or file buffers to access the document undo manager but this doesn't fit well into the picture: it should be possible to access the undo manager without layers above JFace Text. We'd probably provide an IDocumentUndoManagerFactory with: - connect(IDocument) - disconnect(IDocument) - getDocumentUndoManager(IDocument)
Dani, this sounds good. If you will investigate the "right place" and technique for integrating this into the text framework I will look at the scenario. These patches definitely fixed the scenario back when I did them (M2). I will investigate this, perhaps something has changed since then that has caused a regression.
OK, sounds good. FYI: it works if you first type in the editor then save, then perform the scenario.
That last tip helped. It's possible I never tested the scenario when document A had no undo history to transfer, since I was so involved in making sure the history was correctly preserved. I'm attaching a new version of the org.eclipse.text patch that has the fix. I wasn't sure how to generate only the delta, so you are receiving all of the patches. If it's easy to just pull out the relevant code, see DocumentUndoManager>>transferUndoHistory(...). I verified that the simple scenario works, and that if there are many edits in the CU before it's renamed, those edits are transferred. I also verified several different renames intermixed with local edits.
Created attachment 33315 [details] org.eclipse.text.patch new DocumentUndoManager that records a rename when history is empty. (I did not do a full test pass on this patch, but can do so once you've got all the other issues sorted out.)
now that I'm familiar again with these patches, some comments on your specific points: >- we do not want to provide access to the text viewer through the editor. I > think the same can be achieved to use getAdapter(Control.class) and compare > to fTextViewer.getTextWidget() [in SharedDocumentUndoManager] yes, this should be sufficient for determining that an undo was generated in our editor. >- I'm not yet sure about the name "SharedDocumentUndoManager", maybe simply > "SharedUndoManager"? Hmmm..I'm now thinking that the word Shared was a bad choice, because in fact each source viewer configuration is creating its own instance of this class simply to hook into the document's undo manager. Since DocumentUndoManager is already taken, what about DocumentUndoManagerAdapter or DocumentBasedUndoManager or something to imply that the only job of this IUndoManager is to tap into the document-based one and to respond to it by revealing text, etc. >- we need to find another way to get the document undo manager. I don't > like to see this attached to the document. In addition the current getter is > strange since it starts logging the changes only after calling it. And of > course we don't want to set it up upfront for all documents. I'm > currently working on finding a better place for this. The factory idea seems promising. Would this factory be set into the source viewer configuration? As a side effect of this effort it would be good to make it simpler for clients to provide alternate implementations of IUndoManager without having to touch the SourceViewerConfiguration hierarchy. >- the *.jface.* package names in org.eclipse.text are due to some code > refactoring that took place a while ago. New code should have the > org.eclipse.text prefix. I'd suggest org.eclipse.text.undo sounds good to me
I'm ready to release an initial version of the patches to HEAD but before I can do so, I need you to commit the org.eclipse.core.commands patch. Let me know when this has been done. There is one remaining issue and it's unfortunately the getAdapter(...) / uiInfo story. Neither asking for the viewer nor the control will work for multi-page editors like the PDE ones and also not for simple JFace Text viewers which do not know the adapters concept at all. Remember my concerns when we introduce IAdapter into the JFace Text layer (see also Javadoc of DefaultUndoManager).
Dani - I've committed the changes to org.eclipse.core.commands. I remember the concerns about IAdaptable. I thought we concluded that the "new/minimal runtime jar" (org.eclipse.equinox.common) was small enough that this was not a big issue. org.eclipse.jface.text already uses the runtime, so are we just talking about org.eclipse.text here? I'm not sure what the reorganized version of these patches looks like, but as I see it, you will have to prereq org.eclipse.core.commands anyway, and the addition of equinox common doesn't seem unreasonable. But then, I don't live in your code, so I may not understand all the issues here. As for multi-page editors...the use of the IAdaptable is to determine that the undo/redo is happening in the originating "editor" (ie, whatever was showing when the user chose undo) so that we can select and reveal the change. If instead the DefaultUndoManager is responding to a change performed elsewhere, no select and reveal is performed. So if there is no match, there is no select and reveal. Can't the editor provide the focus control of the current page? I think this would work. If the page is a source page, and undo originated in the source page's control, then the select and reveal is performed. If there is no match, then select and reveal is not performed. This should even work for text fields in a form.
As far as I can see clients that only (i.e. no editor around it) use a text viewer and use the modified TextSourceViewerConfiguration (with new undo manager) to configure the viewer will be "broken" i.e. undo/redo would no longer select and reveal. In addition we cannot request all MPE implementors, which will now get the new undo manager, require to provide the correct adapter and Platform Text cannot (and does not want to) let TextViewer implement IAdaptable in order to work. This would be required in order to correctly select and reveal in the focus widget upon undo/redo, which is a must. Regarding the IAdaptable: the layering is one problem but I think I also raised concerns whether it would be possible to always get the correct UI info out of it. Anyway, I'll see whether we can find a solution for this. An option might be to move that code into JFace Text and merge the IDocumentUndoManager API directly into IUndoManager. Susan, I've an additional question regarding IDocumentUndoManager: why isn't it exposing undo(able)(), redo(able)() and reset() for its clients? Currently they have to be aware of the underlying implementation and go through the operation history.
Susan, I found a solution which works without using uiInfo.getAdapter() in the SharedDocumentUndoManager (now called TextViewerUndoManager).
Dani - perhaps there's something we can do on the action side of things. The OperationHistoryActionHandlers are the actual IAdaptable being passed around through the operations framework. It just so happens that if they get a request they don't recognize, they forward the request to their part. This was an easy way to extend the usefulness of the adapter since parts handle adapter requests. So we could do the following: - The existing OperationHistoryActionHandler can handle the request for Control.class, and can use existing API to handle the special case for MPE's, by answering the focus control of the MPE. There is already special case code in there for MPE issues. (See bug #103379). - For the plain text viewer case, a different kind of action is needed anyway, since OperationHistoryActionHandler works with parts. Could this action be placed in an IAdaptable-aware package so that it can handle the adapter requests and therefore retreive the text viewer's control? As for not exposing undoable, redoable, etc....it was simply a minimalist approach. Since I won't own these classes, I didn't want to create more API than needed. If you think your clients are otherwise not aware of the operation history, then I think it does make sense to expose this API.
I just posted without seeing you had a solution. At any rate, I can update the OperationHistoryActionHandler if need be.
I've released a first cut of the code with some changes to HEAD. Available in builds >= I20060124-0800. Changes are: - moved all new code in org.eclipse.text into 'org.eclipse.text.undo' package - added package.html - increased plug-in version of org.eclipse.text to 3.2.0 and updated dependent plug-ins - updated org.eclipse.platform.doc.isv plug-in - removed IAdaptable form org.eclipse.text except for the signatures that are inherited by the operations framework - updated component.xml files - renamed SharedDocumentUndoManager to TextViewerUndoManager - changed TextViewerUndoManager to no longer depend on IAdaptable and correctly select and reveal in the text viewer that originated the undo/redo - made abstract superclass for the two viewer undo manager test classes - applied our code style Remaining issue: - add more API to IDocumentUndoManager as outlined in last paragraph in comment 36 - pass over Javadoc
*** This bug has been marked as a duplicate of 89599 ***
