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.

Bug 103467

Summary: Explicit control of UndoableEdit chunking
Product: editor Reporter: err <err>
Component: -- Other --Assignee: Jaroslav Tulach <jtulach>
Status: RESOLVED FIXED    
Severity: blocker CC: apireviews, dstrupl, geertjan, issues, jglick
Priority: P1 Keywords: API, API_REVIEW_FAST
Version: 6.x   
Hardware: All   
OS: All   
Issue Type: ENHANCEMENT Exception Reporter:
Bug Depends on:    
Bug Blocks: 179047, 110500    
Attachments: proposed API implementation
add javadoc to methods, tweak class description
isolated patch for this issue
nbm for interactive testing
source code for interactive test
patch
unit test
patch for comments up to Y14
updated unit tests
patch for comments up to Y22
test some additonal situations
package scope for SeparateEdit, correct mispelling
adjust unit test to corrected mispelling

Description err 2007-05-08 09:01:52 UTC
Vim has a well defined, and expected, undo/redo granularity. jVi support of this
behavior requires some platform support.

Proposed here is a trivial API extension to org.openide.awt.UndoRedo. This
extension is best described by the java doc for the new class. This new class,
UndoGroupManager, extends java's UndoManager and is used by NetBeans as:

  public static class Manager extends UndoGroupManager implements UndoRedo {...}

===== UndoGroupManager javadoc =====

UndoGroupManager extends UndoManager and allows explicit control of how
UndoableEdits are coalesced into compound edits, rather than using the rules
defined by the edits themselves. Other than the default usage, special handling
is initiated by doing beginUndoGroup().

Three use cases are supported.

   1. Default behavior is defined by {@link javax.swing.undo.UndoManager}.
   2. UnddoableEdits issued between beginUndoGroup() and endUndoGroup() are
placed into a single CompoundEdit. Thus undo() and redo() treat them atomically.
   3. Use commitUndoGroup() to place any accumulated UndoableEdits into a
CompoundEdit; an application could do this at strategic points, such as
EndOfLine entry and cursor movement.

Note that certain methods, such as undo(), automatically issue commitUndoGroup().

===== end javadoc =====

Three additional methods are available: [begin|end|commit]UndoGroup. Notice that
these methods could be empty and undo/redo behavior reverts to current.
commitUndoGroup is not used directly by jVi; it is required by the implementation.

jVi wants the seconde use-case The third use case could be used by the standard
netbeans editors to provide options for other undo/redo granularity.

I would call this interface stable, it is essentially the same used by jVi in
JBuilder 5 years ago.
Comment 1 err 2007-05-08 09:03:50 UTC
Created attachment 42210 [details]
proposed API implementation
Comment 2 err 2007-05-08 09:13:40 UTC
Attached a patch which implements this proposal. The only open issue I see is
limits handling. Currently the programmed limit applys to the number of
CompoundEdits accumulated. If necessary, the programmed "limit" on the number of
edits can be applied to the aggregated edits.
Comment 3 Vitezslav Stejskal 2007-05-09 00:13:22 UTC
VS1: I can see a lot of public methods without javadoc. They all should be
documented, especially when they have some effect on coalescing.

VS2: I am not entirely sure that changing superclass of a non-final class is
backwards compatible (it's probably binary compatible but not source
compatible). Maybe we should leave the existing Manager as it is and just add
the new GroupManager.

VS3: Could we possibly have some unit tests covering this new functionality?

VS4: If the change is accepted I think you will have to update apichanges.xml
and mark the new stuff with @since tag.
Comment 4 err 2007-05-09 06:24:50 UTC
> VS1: I can see a lot of public methods without javadoc.

I'm updating the patch with more documentation, all but a few methods are
{@inheritDoc}.

> VS2: I am not entirely sure that changing superclass of a non-final class is
> backwards compatible (it's probably binary compatible but not source
> compatible). Maybe we should leave the existing Manager as it is and just add
> the new GroupManager.

I can't see how this wouldn't be backwards compatible, could you clarify the
issue? However if that is a problem, then a whole new inheritance chain
involving CESUndoRedoManage and the functionality of Manager will have to be put
together, seems very messy. Easier would be to incorporate the functionality of
GroupManager into Manager, but not quite so clean.

> VS3: Could we possibly have some unit tests covering this new functionality?

I have not idea what's involved here. Tests for the current coalescing behavior
would be an ideal place to start. Pointers to unit tests that might be similar,
or at least related.

> VS4: If the change is accepted I think you will have to update apichanges.xml
> and mark the new stuff with @since tag.

I'll explore that.
Comment 5 err 2007-05-09 06:27:29 UTC
Created attachment 42229 [details]
add javadoc to methods, tweak class description
Comment 6 Jaroslav Tulach 2007-05-09 07:01:50 UTC
Y01 Well, I wonder why you need this API at all. Just do
CompoundEdit comp = new CompoundEdit();
getUndoRedo().undoableEditHappened(comp); 
...
comp.end();
See for example UndoRedoWrappingExampleTest. If that is not enough, you should 
really provide some unit test or modify the UndoRedoWrappingExampleTest to 
show what exactly you cannot do.

Comment 7 err 2007-05-09 07:52:27 UTC
> getUndoRedo().undoableEditHappened(comp);

Ahhhh, got it, very nice. I guess that "canUndo()" will be false while in insert
mode, which is a difference, but not a problem might even be better. I'll take a
look at the other methods.

I'll probably need some way to kick the UI to check canUndo, so the undo button
enable is correct, after exiting input mode. I'll take a look at CESUndoRedo and
see if I can figure out how the button gets enabled/disabled.
Comment 8 err 2007-05-09 23:07:03 UTC
> Y01 Well, I wonder why you need this API at all. Just do ...

Here are initial results: two code issues, a user experience issue. Handling
"code 2" requires an API tweak.

code 1 - CloneableEditorSupport injects a variety of markers into
         UndoRedo.Manager. They interfere by absorbing the CompoundEdit that
         is intended to coalesce edits.

Workaround: for beginUndoGroup, first add insignificant dummy edit, then add 
            the CompoundEdit that will coalesce the edits of interest.

code 2 - The button enable/disable state wrong after endUndoGroup, can't undo.

Workaround: change access of UndoRedo.Manager.fireChange to public, invoke 
            this after "compoundEdit.end()"

user experience - when user start input text, the undo button goes inactive
                  until exit input mode.

           This isn't serious, but is misleading.
Comment 9 Jaroslav Tulach 2007-05-10 00:39:27 UTC
> code 1 - CloneableEditorSupport injects a variety of markers into
>         UndoRedo.Manager. They interfere by absorbing the CompoundEdit that
>         is intended to coalesce edits.

Write a test showing the problem, please.
Comment 10 err 2007-05-10 01:38:22 UTC
> code 1 - CloneableEditorSupport injects a variety of markers into ...

Excuse me, I wasn't very clear about this.

During the editor first coming up, a
    CloneableEditorSupport$BeforeSaveEdit
is added to the UndoRedo.Manager. Then when jvi adds a CompoundEdit to Manager,
it is added in turn to BeforeSaveEdit, BeforeSaveEdit does
    getUndoRedo().addEdit(new BeforeModificationEdit(saveTime, anEdit));
which effectively prevents the CompoundEdit from having any edits added to it.
(It is interesting to note that the compound edit is in progress).

There may be other examples of these magic marker edits interfering, I can't
say. In the initial testing, only the first compound edit is lost. Then things
behave as expected.

If it is still needed I will need some coaching to write a test. Possibly based
on CloneableEditorSupportTest.java, how is this run? I've never used JUnitTest,
where's the driver?
Comment 11 Miloslav Metelka 2007-05-10 11:04:47 UTC
I think that this whole thing will be somewhat more complicated but I agree that
it's necessary to have a support for chunking.
1) the new chunking should definitely support some sort of nesting and multiple
independent clients. BTW there are additional requests like issue 60560 and
issue 90899 that would benefit from it.
2) CloneableEditorSupport$BeforeSaveEdit should be abandoned possibly as part of
this change. The way it works (inserting an extra edit after save) discards the
rest of possible edits in the queue. So if you make edits; then undo several
times; then save; you now cannot redo the edits. Instead there should be a
wrapping around each edit coming from the document in which the CES can set a
flag that save was performed.
3) I'm also wondering where is the best place for the API to be present -
whether in the editor or in the openide. That's not that important but IMHO we
should first clarify the clients and usecases of the API before final decission.
IMHO we should (if possible) go the way that we place the edits into the undo
manager's queue immediately and control whether they can absorb another ones or
not. This way we could possibly do the things in a more systematic way (at least
from the UndoManager's SPI point of view). That way the present Save action
could continue working as it is (without requiring an extra call to
commitUndoGroup() so that you can later undo exactly to the save point) just the
wrapper in CES would prohibit further absorbing into the lastly added edit. I'll
try to sketch something in this way.
Comment 12 Jaroslav Tulach 2007-05-11 16:58:39 UTC
> If it is still needed I will need some coaching to write a test. 

You will not be allowed to make an API change in NetBeans without test. So 
yes, it is needed.

> Possibly based
> on CloneableEditorSupportTest.java, 

Yes, or other tests in the same package.

> how is this run? I've never used JUnitTest,
> where's the driver?

Start NetBeans, open the test file, select Run File (Shift-F6). That is all.

Comment 13 err 2007-05-11 20:20:50 UTC
> > > Write a test showing the problem, please.

> > If it is still needed I will need some coaching to write a test. 
> 
> You will not be allowed to make an API change in NetBeans without test. So 
> yes, it is needed.

Some miscommunication, I was asking about whether a test was need to understand
the problem.

Thanks for the clarification and info on making contributions and how to run the
tests. Since I enjoy the details, what seems simple now will probably become
mysterious as I delve into the unit tests...
Comment 14 err 2007-05-30 21:58:53 UTC
I have limitted time over the next few months and NB6 feature freeze draws
close. For NB6, now's the time. If in NB6, all jvi users get a better
experience and I don't have to worry about supplying a patch to NB for jVi
users to install. FYI, one third of recent jvi beta downloaders also took the
NB55 BraceMatch-UndoRedo patch (essentially the same patch attached to this 
issue).

Much of the issues are beyond my understanding; I'll make some comments and
see if I'm understanding at all.

Note that if for some reason chunks aren't as big as "requested", its not
really a problem, at most its an inconvenience to the user.

> 1) chunking ... support ... nesting and multiple independent clients.

Simple model is a counter. Should always "try {begin; ...;} finally {end;}".
Any edits while counter != 0 are chunked (API could allow explicit commit).

Alternately, any "end" could cause a commit and if counter non-0 continue
chunking. This may have nice characteristics, for example a template operation
would split a chunk, giving 3 chunks.

> 2) CloneableEditorSupport$BeforeSaveEdit should be abandoned ...
>    make wrapping around each edit from document, CES manips wrapping flags.

Something, presume undoRedoMan, wraps each edit unless it is already wrapped.

Chunks could be delineated by a particular flag/bit. Maybe other bits for???
To undo/redo undoRedoMan checks the flags for how much to undo/redo.

> 3) Where for API, editor or openide? first clarify the clients and usecases.

jVi starts a chunk when input mode is started, ends the chunk when input mode
is exited, a JEditorPane is a convenient handle. During input mode,
there may be code completion and template expansions, an example of nesting.

> should (if possible) place the edits into the undo manager's queue
> immediately and control if they can absorb other or not; could do the things
> more systematic (at least from the UndoManager's SPI point of view). The
> present Save action could continue working as is (without requiring extra
> call to commitUndoGroup() so can later undo exactly to the save point) just
> the wrapper in CES would prohibit further absorbing into the lastly added
> edit.

I don't understand what's meant or the implications around
  "Save action ... working ... just the wrapper in CES would prohibit"
Escpecially that "wrapper in CES" phrase. Not knowing the CES certainly
contributes to my confusion. Maybe I'm on the wrong track since I'm looking at
this thing as mostly implemented in UndoRedoMan and CES knows about the
special flag/bits. The API may be exposed through the CES or somewhere else.

Question
Should it be considered an error if an inProgress edit is added to the
UndoRedo manager?

Comment 15 Miloslav Metelka 2007-06-04 17:26:33 UTC
> Question
> Should it be considered an error if an inProgress edit is added to the
> UndoRedo manager?

Yes, it can be a problem since CloneableEditorSupport.saveDocument() can come at
any time (e.g. if someone triggers compilation etc.) and if you would modify an
in-progress edit later after the save it would not be possible to reach the
at-the-save-doc-state with undo/redo actions and CES would mark/unmark the doc's
modification flag (*) at wrong points from the performed-undoable-edits-history
point of view.
For example if you would type "h" and make an in-progress UE for it and add it
to UM and then CES.saveDocument() would be called and then you'd like to join
the "h" with typed "i" then you could only undo/redo "hi" later but the saved
file would contain "h". So two edits, one before save-point and the other after
the save-point must stay split so that you can undo/redo to the save-point.
Comment 16 err 2007-06-10 20:33:57 UTC
Continuing the concept of the undo/redo chunking as a lower level feature,
here's something that appears to take care of the main problem, the "magic
marker" edits getting absorbed, without addressing issues of losing redoable
edits after a save.

Recall the original patch adds begin/endUndoGroup() and commitUndoGroup().
I'm suggesting the addition of AtomicEdit interface as follows
        public static class UndoGroupManager() extends UndoRedo {
            ....
            public interface AtomicEdit { /*no methods*/ }
            ....
        }
then any edit that should not be coalesced implements this interface.

Finally, modify CloneableEditorSupport.FilterUndoableEdit to
implement UndoGroupManager.AtomicEdit

AFAICT, this is compatible with possible solutions for fixing "losing redoable
edits".

BTW, it certainly may be that the chunking issue should be addressed in CES or
even the editor as brought up before. Without detailed study, I shouldn't
argue the merits one way or another. The idea here is to provide something
upon which a complete solution can be built.


Looking at "losing redoable edits" and using flags. Two flags are suggested 
    BeforeSave
    AfterSave
One flag may be sufficient, but with two the undo/redo operations do not need
to look at more than one edit. The flags are maintained/used like:

    When save
        clear any existing flags
        set BeforeSave on edit at indexOfNextAdd-1
        set AfterSave on edit at indexOfNextAdd,
            (if none, then add BeforeModificationEdit and set its AfterSave)
    When undo
        if AfterSave then justRevertedToNotModified = true;
    When redo
        if BeforeSave then justRevertedToNotModified = true;

A comment about wrapping to get flags if there is an UndoGroupManager:
There's an issue about when to wrap edits so they have the flags. Notice
that edits can be coalesced without being wrapped, the compound edit which
does grouping needs to have the flags. If this optimization, ie. not wrapping
coalesced edits, is worthwhile then UndoGroupManager must do the wrapping.
Comment 17 err 2007-07-26 00:54:03 UTC
Milo, I'm wondering if the lack of response to my previous comment is due more to the rush of M10 or that my analysis is
too far off target (or perhaps seemingly incoherent). I know that feature freeze has occurred, but I'm wondering if
"losing redoable edits" bug might be fixed for NB6 and the fix could include a chunking solution. If the chunking was
undocumented and available only through reflection so that a proper API and review could happen in the future that would
be ok with me.

This has suddenly escalated because of issue 110500. History... where possible, jvi uses atomicLock/Unlock to provide
undo/redo chunking; this satisfies several cases. This can't be used when user input is part of the chunk, since the
lock can not be held when the awt thread exits.

With oversight/feedback I believe I can implement what is suggested in the previous comment. Is there a way to proceed
for NB6 solution?
Comment 18 Vitezslav Stejskal 2007-07-26 14:19:54 UTC
CCing issues@openide to grab attention of people looking after openide/text, because the proposed changes also involve
this module. We in the editor are busy with bugfixing so please don't expect much input from us. If nobody else responds
I'm afraid you will have to live with what we have now, sorry.
Comment 19 err 2007-07-26 15:10:20 UTC
I understand that everyone has much to do at this time. I'm (not quite happy but) willing to put together a solution
which addresses both this issue and the CloneableEditorSupport problem of "losing redoable edits after save". But
there's no reason to push now if there's a relatively low probability of it being including in NB6. 

For reference I'm attaching the current source for the small platform7/modules/patches/org-openide-awt file I'm making
available to jVi users. It fixes the issue of hitting the save button while in the middle of jVi edit mode, and still
can undo exactly to the save point.
Comment 20 err 2007-07-26 15:12:26 UTC
Created attachment 45784 [details]
isolated patch for this issue
Comment 21 Miloslav Metelka 2007-07-26 16:57:01 UTC
Apologies, Ernie, I've returned from vacation and then accidentally skipped your last comment.
I'm not sure I understand the usages of the proposed AtomicEdit. Should CES somehow use it for savepoints? Or any other
clients?
Sorry to interrupt the discussion but I'm still trying to recall all the requirements:
jvi:
 1) chunking (aka undogroup): user goes into insert mode e.g. with "i" then it types e.g. "line1" then Enter then
"line2" followed by Escape. Now all this should become a single chunk undoable with a single "u" (or Edit->Undo).
 This requires extra support since otherwise "line1" Enter and "line2" would each require extra Edit->Undo.

 2) in-group undo/redo: until group is committed the individual edits should be undoable/redoable. E.g. Edit->Undo when
"line1{Enter}lin" is typed should undo to "line1{Enter}". Is this a requirement? IMHO it makes sense at least but I
don't whether there is an intent to support this.

IDE:
 3) savepoint: Once File->Save gets pressed the savepoint is (re)defined and undo/redo before/after it must set '*"
mark. When doing several undos from savepoint and do a save again (savepoint redefined) the redo must work as expected.
Currently this is broken and it should be fixed.

Just thinking:
 4) Nested chunks?: When jvi starts a chunk can there be another client starting another chunk in the middle of the
jvi's one? IMHO yes e.g. the code templates expansion or drag&drop.
 5) chunking commit till end: When a chunk is being committed does it always extend from a certain point till the last
performed edit? Probably silly question this should always be true.

I'm looking again how things are currently done in the current CES and UndoRedo. Anyway as Vita said we are short of
time although I understand the importance of this.
Comment 22 err 2007-07-26 19:35:03 UTC
> returned from vacation and then accidentally skipped your last comment.
Sounds like the vacation was a success!

The AtomicEdit is introduced to address the third point of the comment of Thu May 10 10:04:47
> present Save action could continue working as it is (without requiring an extra call to commitUndoGroup() so that
> you can later undo exactly to the save point)
(looking back, I'm not sure I entirely understand full comment).  The code in the proposed UndoGroupManager would do
    boolean addEdit(UndoableEdit anEdit) { if(anEdit instanceof AtomicEdit) return commitAddEdit(anEdit); ... }
So "implements AtomicEdit" insures that the particular edit is never coalesced. Any "magic marker" edits in CES
would presumably be tagged with this interface, in particular CloneableEditorSupport.FilterUndoableEdit.

Reviewing the requirements. 1) is certainly a vim requirement. 2) is not a vim requirement, vim has insert mode
^W or Q comands to delete words or lines before the insertion point. Your suggestion could be done (see below).
3) is what I've been calling "losing redoable edits after save". (I lost stuff by hitting the compile button one day).
The 2nd half of the June 10 comment generally describes how I'm proposing to handle this. Let me know if unclear.

4) For nesting I was thinking of a simple counter model. Begin increments, end decrements. Should always
"try {begin; ...;} finally {end;}".  Any edits while counter != 0 are chunked (API could allow explicit commit).
With this specification, nested begin/end all get chunked together into a single undoable item.

Alternately, any "end" could cause a commit and if counter non-0 continue chunking. This may have nice characteristics,
for example a template expansion chunk would split a insert mode chunk, giving 3 chunks separately undoable.

5) Chunking commit till end. If I understand question, then always true

Implementation note and doing "2) in-group undo/redo". In what kind of use case does this come into play?
Currently, I set up a compound edit and stuff individual edits into it as they come in. Certain things cause an
implicit commit: for example an undo or receiving an AtomicEdit. This keeps things simple, there are no changes to
the undo/redo related methods. 2) could be handled by either doing  undo/redo from out of the compound edit or by
not using a compound edit and instead have begin (and possible end) group flag (there are now flags for 3) so
that parts simple). I'd rather not since, it seems that managing these flags could become messy/complicated (but
everything looks that way at first).
Comment 23 Jaroslav Tulach 2007-07-27 11:13:03 UTC
Y02 I still heard just one reason why Y01 (e.g. not to write any API at all, just use CompoundEdit) cannot work. If 
that is the case, I strongly object against any API change and once again I am asking for a unit test to show what is 
wrong with CompoundEdit and describe the actual behaviour.
Comment 24 err 2007-08-01 05:58:26 UTC
Here are the problem areas I see:
1) the CES use of absorbing edits to track modified state gives incorrect visual feedback with CompoundEdit
2) the CompoundEdit absorbs CES markers and the modified state is incorrectly displayed
3) the use case is not clearly specified, in particular nested undo group behavior

I'm attaching a simple module to interactively examine the behavior. With the "compoundEdit" plugin active, there
is a "rainbow" icon after the redo icon on the toolbar. Click this icon and a modeless dialog comes up with two
buttons, BeginUndoGroup which adds a new CompoundEdit to the UndoRedo.Manager, and EndUndoGroup which does
CompoundEdit.end().

Open a file in the editor, click BeginUndoGroup, add a few line of text, click EndUndoGroup . Notice that the file
is marked as modified, but the undo button is inactive. Add another line, now the undo button is operative, and
some undo clicks show that the first inputs are in fact chunked as requested.

BeginUndoGroup, enter "aaa\n", do SaveFile, enter "bbb\n", EndUndoGroup. (note that the CES save file marker is in
the middle of the group) Enter "ccc". Click undo until the "aaa\nbbb\n" is undone and observe that the file is show
as not modified; this is incorrect, the file was saved after the "aaa\n" was entered.

A) SaveFile, Enter: "aaa", SaveFile, BeginUndoGroup, EndUndoGroup, Enter: "bbb".
B) Click undo - file "aaa"    state modified (incorrect)
C) Click undo - file ""       state not modified (incorrect)
D) Click redo - file "aaa"    state not modified (correct, but state stayed not modified and characters are added)
E) Click redo - file "aaabbb" stat modified (correct) in same state as after step A), can repeat from step B).


Finally there's the situation where there might be nested compound edits, without defining the target semantics,
the behavior can't be analyzed.

Two advantages to a chunking API: much simpler (less bug prone) than trying to maintain proper state with
CompoundEdit (note that should assert if inProgress edit is added). Secondly, simple to define useful semantics for
nested behavior.
Comment 25 err 2007-08-01 06:00:20 UTC
Created attachment 45969 [details]
nbm for interactive testing
Comment 26 err 2007-08-01 06:01:32 UTC
Created attachment 45970 [details]
source code for interactive test
Comment 27 err 2007-08-08 18:14:11 UTC
This issue is becoming critical for jVi.
JTtulach, is there further discussion on using CompoundEdit for chunking in the NetBeans environment?
Milo, did comments of "Thu Jul 26 18:35:03" adequately address your issues?

jVi uses two undo chunking techniques: atomicLock and an optional NB patch (see Nb60M10UndoRedo02.patch) related to the
discussion of this issue. atomicLock is used when a change does not involve user typing into the document. In addition,
jvi may use NB actions in response to user commands, for example reformat and gotoBraceMatch.

Recent changes for NB6 in NetBeans' editor infrastructure are making the use of atomicLock for undo chunking difficult,
if not impossible. For example, there is a lock ordering requirement of Formatter lock before atom lock and
gotoBraceMatch deadlocks if invoked while atomicLock is held. As NB develops to take further advantage of threading, I
can only believe that there will be increased restrictions on the document state when invoking editor actions.

It seem that jvi should stop using atomicLock in this way.
Comment 28 err 2010-03-29 02:11:04 UTC
Created attachment 96122 [details]
patch

Attached patch against the current development tree.

I believe I've addressed all the issues raised (that was over two years ago)

I assume its too late for 6.9 (though the patch has been in continuous use since NB5.5) consideration.

I've never contributed more than a few lines to fix a bug, so I really don't understand the process. I'd appreciate some feedback to get this moving.
Comment 29 Jaroslav Tulach 2010-03-31 08:18:18 UTC
I have to repeat Y01. I want to see a unit test showing why CompoundEdit does not work. If I see the sample use of CompountEdit does not achieve desired results, then I can find out what to fix to align the steps and result. Please write a unit test (see openide.text/test/unit/src/** for examples) to demonstrate why CompoundEdit does not work.
Comment 30 err 2010-03-31 14:19:25 UTC
> I want to see a unit test showing why CompoundEdit does not work.

I thought the modules I attached showing how
    CloneableEditorSupport$FilterUndoableEdit
based classes got "lost" in the compound edit and screwed up the save tracking would be sufficient. 

Thanks for the pointer, I thought I'd recently looked for UndoRedoWrappingExampleTest through the whole tree... must have had a misspelling.
Comment 31 err 2010-04-02 03:28:40 UTC
This issue has had 3 primary threads of discussion.
  A) behavioral requirements and specification
  B) issues in CES state tracking
  C) getting a unit test.

There is general agreement that the proposed feature should be supported. Question whether an API change is required.

=== Requirements and Specification

Proposed patch adds a beginUndoGroup() and endUndoGroup() to the UndoManager API.  Originally proposed a "commit" method, it is currently private and used internally. Not needed, may be convenient.

1) nesting and multiple clients
   Current patch doesn't support this, any endUndoGroup() stops chunking until a beginUndoGroup is encountered. Should be easy to implement as described in earlier comments.
   
   Suggest creating separate chunks delineated by calls to begin/end (as opposed to all nested begin/end put into single chunk). In this way, individual clients will not have their changes combined with other clients.

2) Until a group is commited, the individual edits should be undoable/redoable.
   Current patch doesn't support this. An undo while a chunk is open goes back to the most recent beginUndoGroup. 

I'm uncertain if this is required, more discussion...


=== issues

Most questions of API and current issues are comment #11, comment #21

1) CloneableEditorSupport$BeforeSaveEdit should be abandoned

2) Loosing redoable edits after save

3) Where to put API (assuming there is an API change to support this feature)
Comment 32 err 2010-04-02 03:38:38 UTC
Created attachment 96566 [details]
unit test

Unit test based on scaffolding of openide.text/test/unit/src/org/openide/text/UndoRedoCooperationTest.java

Test uses beginChunk()/endChunk(). Currently implemented to use Compound edit. Commented out implementation using the proposed patch. Both implementation pass testTrivialChunk. Both fail testNestedChunks. The rest pass with the patch and fail with compound edit. One assert is commented out in testSaveDocumentWhileActiveChunk, I believe this is a CES bug.

(I understand that now is a busy period...)
Comment 33 err 2010-11-04 16:48:48 UTC
(In reply to comment #29)
> I have to repeat Y01. I want to see a unit test showing why CompoundEdit does
> not work. If I see the sample use of CompountEdit does not achieve desired
> results, then I can find out what to fix to align the steps and result. Please
> write a unit test (see openide.text/test/unit/src/** for examples) to
> demonstrate why CompoundEdit does not work.

Is the unit test attached with comment 32 sufficient to demonstrate the problems with using CompoundEdit?
Comment 34 Jaroslav Tulach 2010-11-04 19:02:47 UTC
Thanks for the unit tests. Now we have something material to reason about!

I still don't believe you need an API change at all. You claim that the tests are failing and without an API change they will continue to fail. I don't think so. I tried:

    CompoundEdit beginChunk(Document d) {
        CompoundEdit ce = new SequenceEdit();
        support.getUndoRedo().undoableEditHappened
                (new UndoableEditEvent(d, ce));
        return ce;
    }

    class SequenceEdit extends CompoundEdit {
        @Override
        public boolean canUndo() {
            if (isInProgress()) {
                return !edits.isEmpty();
            } else {
                return super.canUndo();
            }
        }
    }

and after that the testSingleChunk test succeeded. I don't have time to play with other operations (undo, redo, canRedo), but I believe they can be implemented in the SequenceEdit to satisfy behavior of your test too. Don't you want to give the SequenceEdit a try?
Comment 35 err 2010-11-04 20:09:14 UTC
> I don't have time to play with other operations (undo, redo, canRedo)

Sadly, I don't either. That's why I haven't pinged in the half year since I attached the unit test.

I'm taking just enough time at this point to get jVi working with the wrap features of the new view hierarchy.

Maybe by NB7.1.
Comment 36 Jaroslav Tulach 2010-11-06 09:38:36 UTC
(In reply to comment #35)
> > I don't have time to play with other operations (undo, redo, canRedo)

Actually I got some spare time to play with your unit tests. I can now understand what are the obsticles one has to face when implementing requested behavior. Simulating this with (subclassed) CompoundEdit is not easy.

I think that resolves Y01, yes you need some API. Looking at your API. You introduce two new methods and an inteface now:

Y11 Is it possible, instead of defining new class to define just 

public static final UndoableEdit BEGIN_COMMIT_GROUP;
public static final UndoableEdit END_COMMIT_GROUP;

or some factory methods to create these two? Everyone could then create these special edits and send them to the manager via UndoableEditListener, e.g. one's editor could just emit these edits, would not need to seek for the proper UndoRedo class that implements them.

Y12 The question is whether your behavior is generic enough or is tight directly to CloneableEditorSupport? The fact that one edit in CloneableEditorSupport has to implement the SeparableEdit interface and also that there are variables called saveBuildUndoGroup makes me believe that the whole support is directly connected with current behavior of CloneableEditorSupport. If so, make the new fields from Y11 part of ClonebleEditorSupport.

Y13 After Y12, the whole code in UndoGroupManager can be moved to not public class in org.openide.text.

Y14 After Y12 SeparableEdit no longer needs to be a public class (can be removed or make package private to org.openide.text).
Comment 37 err 2010-11-07 01:59:44 UTC
(In reply to comment #36)

Y11 - "... UndoableEdit BEGIN_COMMIT_GROUP ..."

Yes, these seem equivelent to the two methods. I wonder if user needs to hold Document's read or write lock while delivering these edits through all the AbstractDocument::getUndoableEditListeners()? Note that AbstractDocument::fireUndoableEdit is protected.

My first inclination is to handle these in "undoableEditHappened()" and call beginUndoGroup/endUndoGroup as appropriate (and those methods are made private). The *_COMMIT_GROUP undoableEdit is discarded. Comment?

Y12 - "generic enough or is tight directly to CloneableEditorSupport?"

The saveBuildUndoGroup variable is needed because super.addEdit is recursive in some situations. At lease that's what a comment says and it's been years since I put together this patch and I don't remember the details. In any event this seems implementation issue, not API.

A generic undo manager which provides chunking needs a way for a manager, like CES, to specify that a particular edit can not be coalesced. SeparateEdit interface is a convenient way to provide this feature. An alternative is to make the method "commitAddEdit(anEdit)" protected instead of private and rename it to addSeparateEdit() or somesuch, but its usage seems more cumbersome. SeparateEdit interface could also be protected.

I don't see a requirement for SeparateEdit for a client, like jVi or CodeCompletion.

There is a way for an undo manager to acheive this without either SeparateEdit interface or addSeparateEdit().
    class FancyManager extends UndoGroupManager {
        // synchronized, so atomic
        synchronized boolean addSeparateEdit(anEdit) {
            commitUndoGroup();
            addEdit(anEdit);
            commitUndoGroup();
        }
    }
Then the FancyManager's magic edits are either top level or the first and only edit in a top level compound edit. This requires commitUndoGroup be made protected.

Also possible is, the method doing
            addEdit(BEGIN_COMMIT_GROUP);
            addEdit(anEdit);
            addEdit(END_COMMIT_GROUP);
with the same trick needed to check if an edit is magic. This depends on proper nesting of BEGIN/END_COMMIT_GROUP.

BTW, I notice that commitUndoGroup is currently public. I was debating this when I made the original patch, but it can be made private or possibly protected.


Y13 - "UndoGroupManager moved to not public class in org.openide.text"
Y14 - SeparableEdit no longer needs to be a public class

Depends on Y12, if treat as specific to CES, then agree.


Historical comment.
The optional NB patch for this currently available with jVi does not patch ClonableEditorSupport; instead in awt/UndoRedo it checks "isMagic(anEdit)" (in addition to "anEdit instanceof SeparateEdit") to determine if anEdit can be coalesced, where isMagic does
    anEdit.getClass().getName().startsWith(
        "org.openide.text.CloneableEditorSupport$")
This hack has allowed the patch to be extremely stable since awt/UndoRedo.java rarely changes, the same binary patch that was used in NB5.5 still works.

Also the use of an additional class, rather than incorporating into awt's Manager class, has allowed the use of UndoGroupManager (without isMagic) in other applications.
Comment 38 err 2010-11-07 21:29:58 UTC
A behavioral issue was brought up in comment 11 by Miloslav Metelka
(that comment also iluminates the problem with BeforeSaveEdit and how it looses undoableEdits)

> 1) chunking ... support ... nesting and multiple independent clients.

Consider jVi and template, suppose the following sequence:
    - jVi   BEGIN
    - templ BEGIN
    - templ END
    - jVi   END
Two possible behaviors for this: single undoable chunk, three undoable chunks. I prefer the later.

Current behavior produces 2 chunks, and tiny undoableEdits
    chunk1: jVi BEGIN - templ BEGIN
    chunk2: templ BEGIN - templ END
modifications between templ END and jVi END are individual edits, there is no coalescing for them.

Proper nesting beavior is simple to implement. It requires user care in balancing BEGIN/END like:
    BEGIN; try {...;} finally {END;}
Comment 39 err 2010-11-14 18:32:43 UTC
Created attachment 102962 [details]
patch for comments up to Y14

This patch
- edits are used to trigger begin/end of undo groups
- only public API is BEGIN_COMMIT_GROUP and END_COMIT_GROUP
- moves UndoGroupManager to a private class in CES
- Enables nesting (nesting test now passes)
Comment 40 err 2010-11-14 18:42:40 UTC
Created attachment 102963 [details]
updated unit tests

In addition to modifying unit tests to work with latest patch, three unit tests are added. 

Two of these tests fail and have nothing to do with coalescing edits. They could be moved to UndoRedoCooperationTest.java, I'm not sure on policy of adding tests that fail...

- testSaveDocumentErrorCase
  demonstrates how undo past save point has incorrect "isModified" state.
- testSaveDocument
  This passes; identical to previous test without failing assert
- testRedoAfterSave
  demonstrates how save loses edits
Comment 41 Jaroslav Tulach 2010-11-15 08:46:29 UTC
Y21 The javadoc on UndoGroupManager will no longer be visible. We need to find a better place for it. Probably one of the newly added public class members could absorb it.

Y22 I am not really proud of "must always be paired" warning. At least people shall know what happens if they are not paired. Will undo/redo be broken? I guess the requests can be nested. What if there is more END_COMMIT_GROUP requests than begin ones? These cases should be documented and tested.

Y23 An alternative to Y22 is to have factory method like: 
public static UndoableEdit[] createCommitGroup(String name?)
that would always return two element array. Zero element to begin the group, next one to end it. I am not sure if that would be cleaner API however, whether it could check more constraints and guarantee more consistency than the two public fields.
Comment 42 err 2010-11-15 13:51:26 UTC
> Y21 The javadoc on UndoGroupManager will no longer be visible.

The patch adds a short paragraph of javadoc to CES and the new public members. It seems at a consistent level of verbosity.

I will add a few more sentences covering the issues raised in Y22.

I left the private doc as reference for anyone working inside CES, or who was curious of the details.

> Y22 I am not really proud of "must always be paired" warning.

Agreed.

> At least people
> shall know what happens if they are not paired.
> Will undo/redo be broken?

No. While coalescing, use of undo/redo/save commit the inprogress chunk and start a new chunk. There is an implicit END/BEGIN.

> I guess the requests can be nested.

Yes, that was an explicitly asked for use case.

> What if there is more END_COMMIT_GROUP
> requests than begin ones?

They are ignored.

> These cases should be documented and tested.

The unit test has "testUndoWhileActiveChunk", I will expand this test and add an additional test for multiple END. There is a nesting test.

> 
> Y23 An alternative to Y22 is to have factory method like: 
> public static UndoableEdit[] createCommitGroup(String name?)
> that would always return two element array. Zero element to begin the group,
> next one to end it. I am not sure if that would be cleaner API however, whether
> it could check more constraints and guarantee more consistency than the two
> public fields.

With multiple clients, there are no ordering constraints between them, not much to check. I guess there could be a check for using END#1 before BEGIN#1; but I don't think that's much better than checking any END without a BEGIN.


About Logging. How does the following sound?

- add WARNING if END while no BEGIN active.
- add INFO if undo/redo/save while coalescing.
- add FINE for BEGIN/END
- use the logger "ERR" that CES already defines.
Comment 43 err 2010-11-21 01:15:51 UTC
Created attachment 103150 [details]
patch for comments up to Y22

(see comment 42 for discussion of Y23)

log INFO with stacktrace if END without BEGIN
log FINE with nesting depth for BEGIN/END
Comment 44 err 2010-11-21 01:17:25 UTC
Created attachment 103151 [details]
test some additonal situations

test cases mentioned in Y22
Comment 45 Jaroslav Tulach 2010-11-22 14:45:57 UTC
I guess the SeparateEdit class shall not be public, but package private, otherwise it might be accessible and visible in javadoc. It is not good to have inner classes that are visible while outclasses are hidden...

Other than that I am fine with the changes. If Míla Metelka agrees, I think we can integrate.
Comment 46 err 2010-11-22 16:00:55 UTC
Created attachment 103192 [details]
package scope for SeparateEdit, correct mispelling

> SeparateEdit class shall not be public, but package private,
> otherwise it might be accessible and visible in javadoc.

Ah, I didn't think it could leak out. Corrected to package scope.

I just noticed that *_COMMIT_* is mispelled, only had one "M". Also corrected.
Comment 47 err 2010-11-22 16:03:05 UTC
Created attachment 103193 [details]
adjust unit test to corrected mispelling
Comment 48 Miloslav Metelka 2010-11-23 14:54:13 UTC
I agree with Ernie's patch.

BTW Yardo I think that we should make

    /** Create an undo/redo manager.
    * This manager is then attached to the document, and listens to
    * all changes made in it.
    * <P>
    * The default implementation uses improved <code>UndoRedo.Manager</code>.
    *
    * @return the undo/redo manager
    */
    protected UndoRedo.Manager createUndoRedoManager() {
        return new CESUndoRedoManager(this);
    }

in CES final since if anyone would really want to override it then important features like bolding modified filename and also this new feature would be lost.
Comment 49 Jaroslav Tulach 2010-11-23 16:30:28 UTC
Thanks for your contribution to NetBeans APIs! It was not easy for you
Comment 50 Jaroslav Tulach 2010-11-23 16:33:55 UTC
... to make all of this happen, but I think we are almost there. If there are no objections I'll integrate tomorrow.

@Míla: I cannot make the method final, but I'll change the javadoc to describe that overriding is almost impossible without calling super impl.
Comment 51 Jesse Glick 2010-11-23 17:29:57 UTC
{@BEGIN_COMMIT_GROUP} in Javadoc should be {@link #BEGIN_COMMIT_GROUP} I guess, and ditto for {@END_COMMIT_GROUP}. {@link END_COMMIT_GROUP} is also wrong. Suggest building Javadoc and checking for new warnings.

Also don't bother with

 /** {@inheritDoc} */

as such lines can simply be deleted to make source code shorter without any effect on Javadoc output. Especially in private classes which the module harness does not document to begin with!
Comment 52 err 2010-11-24 04:54:48 UTC
Shall I submit another patch with the Javadoc fixes?

I'll keep all this in mind, there may be more patches in there...
And I hope this is generally useful.

> Javadoc should be {@link #BEGIN_COMMIT_GROUP} 
Yep, it's done better in the private inner class where no one can see it.
Last minute change without full retest, bad idea. Yet I still do it.

> Also don't bother with /** {@inheritDoc} */
Funny, that got added in response to a comment on first patch over three years ago; when the class was public in a different module.
Comment 53 Jaroslav Tulach 2010-11-24 10:30:05 UTC
I cannot integrate. Two test cases from the new test set do not pass! Fix it please.
Comment 54 Jaroslav Tulach 2010-11-24 12:33:11 UTC
I've commented out the two tests and integrated as core-main#3b4c4e6862b6, please fix the tests as a separate issue.
Comment 55 Jesse Glick 2010-11-24 15:25:09 UTC
(In reply to comment #52)
>> don't bother with /** {@inheritDoc} */
>
> that got added in response to a comment on first patch over three years ago

All visible methods should indeed be documented. But when a method merely implements a method from a supertype without adding any nonobvious semantics, there is no need to give it a Javadoc comment, since the output will automatically refer to the supertype's documentation. @inheritDoc may be used in specialized cases such as

public final class Coordinate implements Comparable<Coordinate> {
  // ...
  /**
   * Note: nonzero return values will in fact be the magnitude of the distance.
   * @inheritDoc
   */
  public @Override int compareTo(Coordinate other) {...}
}
Comment 56 Quality Engineering 2010-11-25 06:18:33 UTC
Integrated into 'main-golden', will be available in build *201011250001* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/3b4c4e6862b6
User: Jaroslav Tulach <jtulach@netbeans.org>
Log: #103467: Explicit control of UndoableEdit chunking