This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
In order to integrate refactoring and editor undo, we need an API to access Editor's undo queue .
Maybe BaseDocument.addUndoableEdit is the method I'm looking for. Need to investigate, how it works.
I think, that there are only 2 requirements: 1. API to plug custom undo. addUndoableEdit() is maybe sufficient, but I'm not sure what is the contract of this method 2. API to get recent UndoableEdit of specific document. I just need to know, if top undoable edit comes from refactoring (see 1.) or not. I'm attaching tests. If you make them pass, I think, that I have an API what I want :)
Created attachment 113533 [details] editor.lib/test/o.n.editor/
Created attachment 114408 [details] failing test Attached test fails, because my undoable edits are wrapped by AtomicCompoudEdit. I'd like to ask for an API which grant me access to current undoable edits. See IntegratedUndoTest.java:81 assertEquals(edit.last(), undoManager.editToBeUndone());
Created attachment 114409 [details] tests Tests are now passing using reflection to "unwrap" compound edit.
addUndoableEdit is probably not sufficient. The API should be able to wrap AtomicEdit with custom edit: BaseDocument: public static interface UndoableEditWrapper { UndoableEdit wrap(UndoableEdit ed, BaseDocument doc); } protected @Override void fireUndoableEditUpdate(UndoableEditEvent e) { UndoableEditWrapper wrapper = (UndoableEditWrapper) getProperty(UndoableEditWrapper.class); if (wrapper!=null) { e = new UndoableEditEvent(e.getSource(), wrapper.wrap(e.getEdit(), this)); } . . .
And I also need an access to editToBeUndone() thanks
I would rather add just something like /** * May be called during runAtomic() section to wrap resulting * compound edit (containing all document modifications performed * during the atomic section) into the given wrapper edit * <br/> * When runAtomic() finishes the document will call * wrapper.addEdit(allModsCompoundEdit). * <br/> * In case the atomic section will be interrupted by breakAtomicLock() * then wrapper.die() will be called without any wrapper.addEdit() call. * <br/> * This method may only be called during an ongoing runAtomic() section. * Once the runAtomic() section completes the given wrapper is no longer * used or referenced. * <br/> * Repetitive calls to this method are allowed - wrappers will be nested. * First call will provide outer wrapper (last call will provide inner wrapper). * * @param wrapper non-null wrapper undoable edit. */ void BaseDocument.addWrapperUndoableEdit(UndoableEdit wrapper) { } Regarding editToBeUndone() I've filed issue #207949.
I think, I can live with this solution. Anyway please consider also possibility of adding wrapper outside of atomic lock. Thanks
Ok, so IIUC the way you would like to use the wrapping is like this, right? RefactoringUndoableEditWrapper { UndoableEdit wrap(UndoableEdit edit, BaseDocument doc) { if (ongoingRefactoring) { // Always perform wrapping } else { return edit; } } } In this setup however the wrap() method might be called for some "intruder" inserts/removals e.g. an external document reload so you should possibly have an extra flag in your WrapperEdit like e.g. "refactoringRelated" that you set if the edit is truly generated by doing a refactoring. If the flag is unset you should just delegate to original edit. I'll make the UndoableEditWrapper in editor.lib2 spi registrable into MimeLookup by @MimeRegistration.
sounds good!
*** Bug 207949 has been marked as a duplicate of this bug. ***
I would like to solve both 207949 and this one at once to have a complete solution for the refactoring undo feature.
Created attachment 116362 [details] Requested feature implementation
I have implemented a solution as requested originally (diff attached) so the implementation on refactoring side can be started. Still I'm not happy with the fact that there can only be one wrapper. For two or more wrappers there would be no way to find the enclosed edits. Either we would have to request special wrapping edits that would be something like interface WrapEdit extends UndoableEdit { UndoableEdit getOriginalEdit(); } or (simpler) only add extra start/end edits into an existing compound edit produced by the BaseDocument. There would be an advantage that the atomic edit impl is already stable against client's Cannot(Undo/Redo)Exception thrown from undo()/redo() so the implementor would not have to care about it. However I'm not sure whether that satisfies refactoring's requirements.
Thanks for the patch. I vote for WrapEdit interface. It is more natural solution imo. Moreover I also merge several UndoableEdits from different atomic actions into big RefactoringUndoableEdit.
Draft of this API integrated. Refactoring now using impl. dependency on openide.text and editor.lib2. Once the API is made official, the impl. deps must be removed.
Integrated into 'main-golden', will be available in build *201203210400* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress) Changeset: http://hg.netbeans.org/main-golden/rev/aad4675efba2 User: Jan Becicka <jbecicka@netbeans.org> Log: Temporary solution introduces implementation dependency, which must be removed after API review. Issue #204828 - API for plugin custom Undo
API review must be finished by 7.2.
I've got one more idea regarding the final solution that would allow other UndoableEditWrapper impls to participate. Let me first ensure that the following suffices for the refactoring use: 1) A method for checking whether a RefactoringWrapEdit is an editToBeUndone()/editToBeRedone(). RWE can possibly be enclosed in another wrap edit which will delegate to it upon undo()/redo(). 2) When RefactoringWrapEdit.undo()/redo() gets invoked it may throw CannotUndoException or CannotRedoException to signal that undo/redo is not possible. For multiple UndoableEditWrapper impls e.g. UEW1 and UEW2 let's assume that the BaseDocument produces AtomicCompoundEdit (ACE) which gets wrapped by UEW1 producing wrap edit WE1 which in turn gets wrapped by UEW2 producing WE2. BaseDocument would produce class ListUndoEdit implements List<UndoableEdit> { // List contents will be { ACE , WE1 , WE2 } // All UndoableEdit's methods delegate to lastEdit = get(size() - 1); } NbDocument.getEditToBeUndoneRedoneOfType(EditorCookie ec, Class<T> type, boolean redone) will check the given edit (possibly ListUndoEdit) for being a List: if (editFromDocument instanceof List) { List<UndoableEdit> listEdit = (List<UndoableEdit>) editFromDocument; for (int i = listEdit.size() - 1; i >= 0; i--) { UndoableEdit listEdit.get(i); if (type.isInstance(edit)) { @SuppressWarnings("unchecked") T inst = (T) edit; return inst; } } } This way we can satisfy multiple UEWs presence. Moreover the WrapEdit interface (mentioned in Comment 15) is not required in this case. Also the NbDocument will require no extra "bridge" API to examine contents of the edit provided by the document. Yardo, Honzo do you agree with this solution?
> 1) A method for checking whether a RefactoringWrapEdit is an > editToBeUndone()/editToBeRedone(). RWE can possibly be enclosed in another wrap > edit which will delegate to it upon undo()/redo(). Not sure if I understand, but I need to know, if the top most undoable edit is mine (the refactoring one) or not. If the RWE will be wrapped by something, will be a chance for me to know, if this wrapped one represents refactoring controlled change? > 2) I'm afraid, that it will not work with current implementation. Again, I need to know, if the UndoableEdit is the refactoring one. If it will be wrapped by unknown wrapper I'm not sure, what happens. Refactoring undoable edit (UndoableEditDelegate) for instance does this: public boolean addEdit(UndoableEdit ue) { if (ue instanceof UndoableEditDelegate) { return inner.addEdit(((UndoableEditDelegate) ue).unwrap()); } return false; } If the following edit is also refactoring one -> merge them.
(In reply to comment #21) > > 1) A method for checking whether a RefactoringWrapEdit is an > > editToBeUndone()/editToBeRedone(). RWE can possibly be enclosed in another wrap > > edit which will delegate to it upon undo()/redo(). > > Not sure if I understand, but I need to know, if the top most undoable edit is > mine (the refactoring one) or not. If the RWE will be wrapped by something, > will be a chance for me to know, if this wrapped one represents refactoring > controlled change? Yes, like this: public class RefactoringUndoableEditWrapper { public UndoableEdit wrap(UndoableEdit ed, BaseDocument doc) { } } > > > 2) > I'm afraid, that it will not work with current implementation. Again, I need to > know, if the UndoableEdit is the refactoring one. If it will be wrapped by > unknown wrapper I'm not sure, what happens. Refactoring undoable edit > (UndoableEditDelegate) for instance does this: > > public boolean addEdit(UndoableEdit ue) { > if (ue instanceof UndoableEditDelegate) { > return inner.addEdit(((UndoableEditDelegate) ue).unwrap()); > } > return false; > } > > If the following edit is also refactoring one -> merge them.
Apologies, writing the last response again...
> > 1) A method for checking whether a RefactoringWrapEdit is an > > editToBeUndone()/editToBeRedone(). RWE can possibly be enclosed in another wrap > > edit which will delegate to it upon undo()/redo(). > > Not sure if I understand, but I need to know, if the top most undoable edit is > mine (the refactoring one) or not. If the RWE will be wrapped by something, > will be a chance for me to know, if this wrapped one represents refactoring > controlled change? Yes, this is the point of the solution that the edits from multiple wrappers will be in fact items in the resulting undo edit which will implement List<UndoableEdit> so the NbDocument.getEditToBeUndoneRedoneOfType(EditorCookie ec, Class<T> type, boolean redone) can traverse it and find your contained refactoring edit. Your undo()/redo() implementation just needs to guarantee to be "stable" for delegateUndoEdit.undo()/redo() and in case the delegates fail recover accordingly i.e. undo the effects of your own edit's undo()/redo() method (this "stability" should anyway be guaranteed by your edits regardless of whether this particular solution will be implemented or not). > > > 2) > I'm afraid, that it will not work with current implementation. Again, I need to > know, if the UndoableEdit is the refactoring one. If it will be wrapped by > unknown wrapper I'm not sure, what happens. Refactoring undoable edit > (UndoableEditDelegate) for instance does this: > > public boolean addEdit(UndoableEdit ue) { > if (ue instanceof UndoableEditDelegate) { > return inner.addEdit(((UndoableEditDelegate) ue).unwrap()); > } > return false; > } > > If the following edit is also refactoring one -> merge them. Merging of the refactoring edits was not in the original list of requirements. Anyway it can also be done, like this: public boolean addEdit(UndoableEdit ue) { List<UndoableEdit> listEdit = (List<UndoableEdit> ue; UndoableEdit topEdit = listEdit.get(listEdit.size() - 1); // Check that there's only original document's edit and the wrapping refactoring edit boolean refatoringEditOnly = listEdit.size() == 2; if (refactoringEditOnly && topEdit instanceof UndoableEditDelegate) { // Maintain a list of addEdit delegates (undo them from last to first then "this"; redo in reverse) this.addEditDelegates.add(ue); // Mark the added edit to do nothing and just deleate to its contained undoable edit ((UndoableEditDelegate)topEdit).setDoNothingJustDelegate(true); return true; } return false; } This assumes that refactoring edit that is already in UndoManager is the top one (not wrapped - otherwise its addEdit() would not be called).
Sounds good. Please attach editor patch with this new API, I'll try to use it in refactoring, Thanks!
Created attachment 118227 [details] Diff against current trunk containing mentioned ListUndoableEdit. Honzo please test it. Thanks.
Created attachment 118301 [details] refactoring update
> Merging of the refactoring edits was not in the original list of requirements. Merging of UndoableEdits is a feature of UndoableEdit, not a feature of your API... > Anyway it can also be done, like this: > > public boolean addEdit(UndoableEdit ue) { > List<UndoableEdit> listEdit = (List<UndoableEdit> ue; > UndoableEdit topEdit = listEdit.get(listEdit.size() - 1); > // Check that there's only original document's edit and the wrapping > refactoring edit > boolean refatoringEditOnly = listEdit.size() == 2; > if (refactoringEditOnly && topEdit instanceof UndoableEditDelegate) { > // Maintain a list of addEdit delegates (undo them from last to first > then "this"; redo in reverse) > this.addEditDelegates.add(ue); > // Mark the added edit to do nothing and just deleate to its contained > undoable edit > ((UndoableEditDelegate)topEdit).setDoNothingJustDelegate(true); > return true; > } > return false; > } > > This assumes that refactoring edit that is already in UndoManager is the top > one (not wrapped - otherwise its addEdit() would not be called). Well it will work in the same way as the original (I would say less complicated) version. Your new approach allows to add more wrappers, but if anyone will add a new wrapper, the merging will stop working... Anyway I created a patch and it seems to be working, since there is only one (my) wrapper.
We agreed with Honza that I will first merge my last patch together with Honza's latest patch into trunk. Then I will create a final patch (that will remove current accesors etc.) that I will submit for API review. http://hg.netbeans.org/jet-main/rev/ce3280033798
Does this API satisfy the needs of Bug 199700 ?
Integrated into 'main-golden', will be available in build *201204200400* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress) Changeset: http://hg.netbeans.org/main-golden/rev/ce3280033798 User: Miloslav Metelka <mmetelka@netbeans.org> Log: #204828 - API for plugin custom Undo - changes before creation of final patch for API review.
(In reply to comment #30) > Does this API satisfy the needs of Bug 199700 ? Yes it should.
Created attachment 118678 [details] Final patch that makes the changes in editor.lib2 and openide.text official.
Final patch attached. Kindly asking for review. Thanks.
Marking as P2 since this is not a beta blocker (functionality is already integrated and working though not through an official API).
(In reply to comment #28) > Merging of UndoableEdits is a feature of UndoableEdit, not a feature of your > API... > > > Anyway it can also be done, like this: ... > > if anyone will add a new wrapper, the merging will stop working... > Considering Bug 199700 . I'm guessing it's solution involves a new wrapper. Since refactoring can affect properties, this can break merging. What are the implications?
Refactoring wrapper merges 2 consequent refactoring edits together. It does not affect other edits. It delegates all addEdit/replaceEdit calls and should not have any effect on other edits.
(In reply to comment #37) > Refactoring wrapper merges 2 consequent refactoring edits together. It does not > affect other edits. It delegates all addEdit/replaceEdit calls and should not > have any effect on other edits. I'm confused since comment #28 says > if anyone will add a new wrapper, the merging will stop working... and the fix for Bug 199700 involves a new wrapper, IIUC. Given these two things (and that properties can be changed by refactoring) why isn't there an issue? (I'm probably missing something simple...) Hmm, is it that refactoring never involves the "properties editor"?
According to Mila it is up to wrapper implementations not to affect each other...
Yes, ideal situation is when multiple wrappers do not wrap edits both at the same time. If they do then certain things - mainly merging of edits may become tricky since two edits must be replaced by a single edit with a common processing for the two. Due to this merging of wrapped edits should either be avoided (but Honza already uses that in refactoring) or the refactoring wrapper should always be the last wrapper (by using "position" file attribute when registering the wrappers in xml FS).
http://hg.netbeans.org/jet-main/rev/e68f4f01616d
Integrated into 'main-golden', will be available in build *201205120400* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress) Changeset: http://hg.netbeans.org/main-golden/rev/e68f4f01616d User: Miloslav Metelka <mmetelka@netbeans.org> Log: #204828 - API for plugin custom Undo.
Late minor comments: [JG01] <T> should I guess be <T extends UndoableEdit>, right? [JG02] This @SuppressWarnings("unchecked") T inst = (T) edit; can be replaced by the simpler and safer T inst = type.cast(edit); [JG03] href="@TOP@/org/netbeans/spi/editor/document/UndoableEditWrapper.html" will not work. Probably you meant href="@org-netbeans-modules-editor-lib2@/org/netbeans/spi/editor/document/UndoableEditWrapper.html".
Integrated into 'main-golden', will be available in build *201205160400* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress) Changeset: http://hg.netbeans.org/main-golden/rev/e4ddfc852aaf User: Jesse Glick <jglick@netbeans.org> Log: API changes for #204828 were mangled.
Jesse, thanks for notes. http://hg.netbeans.org/jet-main/rev/fcbe4f9cd2b0
Integrated into 'main-golden', will be available in build *201205250002* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress) Changeset: http://hg.netbeans.org/main-golden/rev/fcbe4f9cd2b0 User: Miloslav Metelka <mmetelka@netbeans.org> Log: #204828 - API for plugin custom Undo - added Jesse's notes.