Bug 204828 - API for plugin custom Undo
API for plugin custom Undo
Status: RESOLVED FIXED
Product: platform
Classification: Unclassified
Component: Text
7.1
All All
: P2 (vote)
: 7.2
Assigned To: apireviews
issues@editor
:
: 207949 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-08 12:36 UTC by Jan Becicka
Modified: 2012-05-25 05:48 UTC (History)
6 users (show)

See Also:
Issue Type: DEFECT
:


Attachments
editor.lib/test/o.n.editor/ (5.99 KB, application/octet-stream)
2011-11-25 11:27 UTC, Jan Becicka
Details
failing test (9.81 KB, patch)
2011-12-22 14:10 UTC, Jan Becicka
Details | Diff
tests (10.75 KB, patch)
2011-12-22 14:59 UTC, Jan Becicka
Details | Diff
Requested feature implementation (27.18 KB, patch)
2012-03-05 16:30 UTC, Miloslav Metelka
Details | Diff
Diff against current trunk containing mentioned ListUndoableEdit. Honzo please test it. Thanks. (19.89 KB, patch)
2012-04-13 09:45 UTC, Miloslav Metelka
Details | Diff
refactoring update (1.80 KB, patch)
2012-04-16 07:24 UTC, Jan Becicka
Details | Diff
Final patch that makes the changes in editor.lib2 and openide.text official. (17.17 KB, patch)
2012-04-24 09:43 UTC, Miloslav Metelka
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Becicka 2011-11-08 12:36:05 UTC
In order to integrate refactoring and editor undo, we need an API to access Editor's undo queue .
Comment 1 Jan Becicka 2011-11-18 13:07:30 UTC
Maybe BaseDocument.addUndoableEdit is the method I'm looking for. Need to investigate, how it works.
Comment 2 Jan Becicka 2011-11-25 11:22:56 UTC
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 :)
Comment 3 Jan Becicka 2011-11-25 11:27:18 UTC
Created attachment 113533 [details]
editor.lib/test/o.n.editor/
Comment 4 Jan Becicka 2011-12-22 14:10:05 UTC
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());
Comment 5 Jan Becicka 2011-12-22 14:59:34 UTC
Created attachment 114409 [details]
tests

Tests are now passing using reflection to "unwrap" compound edit.
Comment 6 Jan Becicka 2012-01-12 12:34:43 UTC
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));
        }
.
.
.
Comment 7 Jan Becicka 2012-01-12 12:43:59 UTC
And I also need an  access to editToBeUndone()
thanks
Comment 8 Miloslav Metelka 2012-01-31 12:53:09 UTC
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.
Comment 9 Jan Becicka 2012-02-02 13:39:18 UTC
I think, I can live with this solution. Anyway please consider also possibility of adding wrapper outside of atomic lock. Thanks
Comment 10 Miloslav Metelka 2012-02-02 15:06:57 UTC
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.
Comment 11 Jan Becicka 2012-02-03 09:37:28 UTC
sounds good!
Comment 12 Miloslav Metelka 2012-03-05 15:37:29 UTC
*** Bug 207949 has been marked as a duplicate of this bug. ***
Comment 13 Miloslav Metelka 2012-03-05 16:19:06 UTC
I would like to solve both 207949 and this one at once to have a complete solution for the refactoring undo feature.
Comment 14 Miloslav Metelka 2012-03-05 16:30:26 UTC
Created attachment 116362 [details]
Requested feature implementation
Comment 15 Miloslav Metelka 2012-03-05 16:31:03 UTC
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.
Comment 16 Jan Becicka 2012-03-09 15:40:38 UTC
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.
Comment 17 Jan Becicka 2012-03-20 10:02:46 UTC
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.
Comment 18 Quality Engineering 2012-03-21 12:06:20 UTC
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
Comment 19 Jaroslav Tulach 2012-03-26 19:46:19 UTC
API review must be finished by 7.2.
Comment 20 Miloslav Metelka 2012-03-27 14:58:02 UTC
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?
Comment 21 Jan Becicka 2012-03-30 10:27:13 UTC
> 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.
Comment 22 Miloslav Metelka 2012-03-30 11:27:12 UTC
(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.
Comment 23 Miloslav Metelka 2012-03-30 11:28:10 UTC
Apologies, writing the last response again...
Comment 24 Miloslav Metelka 2012-03-30 12:22:38 UTC
> > 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).
Comment 25 Jan Becicka 2012-03-30 12:49:43 UTC
Sounds good. Please attach editor patch with this new API, I'll try to use it in refactoring, Thanks!
Comment 26 Miloslav Metelka 2012-04-13 09:45:53 UTC
Created attachment 118227 [details]
Diff against current trunk containing mentioned ListUndoableEdit. Honzo please test it. Thanks.
Comment 27 Jan Becicka 2012-04-16 07:24:45 UTC
Created attachment 118301 [details]
refactoring update
Comment 28 Jan Becicka 2012-04-16 07:28:42 UTC
> 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.
Comment 29 Miloslav Metelka 2012-04-19 13:12:42 UTC
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
Comment 30 err 2012-04-19 13:32:33 UTC
Does this API satisfy the needs of Bug 199700 ?
Comment 31 Quality Engineering 2012-04-20 10:14:18 UTC
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.
Comment 32 Miloslav Metelka 2012-04-24 09:14:25 UTC
(In reply to comment #30)
> Does this API satisfy the needs of Bug 199700 ?

Yes it should.
Comment 33 Miloslav Metelka 2012-04-24 09:43:05 UTC
Created attachment 118678 [details]
Final patch that makes the changes in editor.lib2 and openide.text official.
Comment 34 Miloslav Metelka 2012-04-24 09:44:04 UTC
Final patch attached. Kindly asking for review. Thanks.
Comment 35 Miloslav Metelka 2012-04-26 05:55:58 UTC
Marking as P2 since this is not a beta blocker (functionality is already integrated and working though not through an official API).
Comment 36 err 2012-04-26 14:24:07 UTC
(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?
Comment 37 Jan Becicka 2012-04-27 06:46:37 UTC
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.
Comment 38 err 2012-05-02 15:14:05 UTC
(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"?
Comment 39 Jan Becicka 2012-05-03 06:27:02 UTC
According to Mila it is up to wrapper implementations not to affect each other...
Comment 40 Miloslav Metelka 2012-05-03 09:13:12 UTC
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).
Comment 41 Miloslav Metelka 2012-05-10 13:18:19 UTC
http://hg.netbeans.org/jet-main/rev/e68f4f01616d
Comment 42 Quality Engineering 2012-05-12 10:00:07 UTC
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.
Comment 43 Jesse Glick 2012-05-14 19:38:38 UTC
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".
Comment 44 Quality Engineering 2012-05-16 11:26:24 UTC
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.
Comment 45 Miloslav Metelka 2012-05-24 12:07:27 UTC
Jesse, thanks for notes.
http://hg.netbeans.org/jet-main/rev/fcbe4f9cd2b0
Comment 46 Quality Engineering 2012-05-25 05:48:32 UTC
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.


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo