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.
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.
Created attachment 42210 [details] proposed API implementation
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.
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.
> 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.
Created attachment 42229 [details] add javadoc to methods, tweak class description
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.
> 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.
> 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.
> 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.
> 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?
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.
> 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.
> > > 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...
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?
> 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.
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.
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?
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.
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.
Created attachment 45784 [details] isolated patch for this issue
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.
> 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).
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.
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.
Created attachment 45969 [details] nbm for interactive testing
Created attachment 45970 [details] source code for interactive test
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.
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.
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.
> 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.
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)
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...)
(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?
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?
> 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.
(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).
(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.
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;}
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)
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
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.
> 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.
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
Created attachment 103151 [details] test some additonal situations test cases mentioned in Y22
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.
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.
Created attachment 103193 [details] adjust unit test to corrected mispelling
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.
Thanks for your contribution to NetBeans APIs! It was not easy for you
... 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.
{@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!
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.
I cannot integrate. Two test cases from the new test set do not pass! Fix it please.
I've commented out the two tests and integrated as core-main#3b4c4e6862b6, please fix the tests as a separate issue.
(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) {...} }
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