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 258798

Summary: CaretUndo.createCaretUndoEdit() needs to become an official API for actions
Product: editor Reporter: Miloslav Metelka <mmetelka>
Component: Actions/Menu/ToolbarAssignee: Miloslav Metelka <mmetelka>
Status: RESOLVED FIXED    
Severity: normal CC: issues
Priority: P1 Keywords: API, API_REVIEW_FAST
Version: 8.2   
Hardware: All   
OS: All   
Issue Type: DEFECT Exception Reporter:
Attachments: Apologies, this is an invalid diff unrelated to this change.
This is a proper diff of the change

Description Miloslav Metelka 2016-04-12 22:23:21 UTC
Any action that would want to properly restore caret state upon undo/redo will need to use the API.
Comment 1 Miloslav Metelka 2016-04-12 22:26:57 UTC
All the actions using the DocumentUtilities.setTypingModification() will likely use the new API too. I'll fix them all as part of this issue.
Comment 2 Vladimir Voskresensky 2016-04-19 13:13:11 UTC
I'm increasing priority because a lot of actions which cause document modifications misses correct caret after Undo.
recently I've faced comment line, uncomment line, delete line (Ctrl+E) and I believe any others touching document
Comment 3 Miloslav Metelka 2016-04-19 20:35:11 UTC
Created attachment 159335 [details]
Apologies, this is an invalid diff unrelated to this change.
Comment 4 Miloslav Metelka 2016-04-19 20:44:56 UTC
I'd like to ask for review of this API change which will allow actions in an arbitrary module to restore caret positions properly upon undo/redo.
The editor.document is enhanced by CustomUndoDocument interface which is implemented by BaseDocument and it allows to add custom undoable edits during atomic transactions over the document.
This API is then used by EditorUtilities.addCaretUndoableEdit() method in editor.lib2 module which is intended to be used by the actions.
I've rewritten several actions in BaseKit as part of the diff and I'll update all the remaining actions once the change becomes integrated.
Comment 5 Vladimir Voskresensky 2016-04-20 10:07:00 UTC
VV1: I do not see CustomUndoDocument in the diff. Do I miss something?
Comment 6 Miloslav Metelka 2016-04-20 11:59:23 UTC
Created attachment 159341 [details]
This is a proper diff of the change
Comment 7 Miloslav Metelka 2016-04-20 12:00:03 UTC
Vladimir, thanks, the original diff was unrelated.
Comment 8 Vladimir Voskresensky 2016-04-20 12:21:36 UTC
VV2: What if EditorUtilities.addCaretUndoableEdit() called twice? I.e. one by editor lib internals and another by the client with the same/different caret states
Comment 9 Vladimir Voskresensky 2016-04-20 12:23:52 UTC
VV3: Is it required now to update all clients what to expect to old clients? I.e. now comment/uncomment inserts removes "//" but doesn't call this method. Now on Undo caret is not restored. Would it be fixed as part of this change?
Comment 10 Miloslav Metelka 2016-04-20 14:18:24 UTC
ad VV2) It can be called as many times as desired during a single document atomic transaction. But in fact only the first snapshot will be "effective" after undo of the transaction and only the last snapshot will be effective for redo of the transaction.
 Generally if an action does some extra caret movements after text insertion/deletion it should call EditorUtilities.addCaretUndoableEdit() at the end of the transaction since that will ensure that when redoing the effect of the action the caret(s) will be at proper place(s).

 As I've already mentioned in javadoc of CaretUndo.createCaretUndoEdit() in the future we could add a code to EditorCaret that would track if any explicit caret moves were done since last call to EditorUtilities.addCaretUndoableEdit() in a single atomic transaction and if none then the CaretUndo.createCaretUndoEdit() could return null to not consume any extra space in UndoManager's queue. Both undo and even redo should still work properly just with the initial restoration at action's begining.

The original undo approach for a single caret was to navigate the caret to document modifications boundaries but that's no longer feasible with a general case of multiple modifications done by multiple carets (or at least it would be rather tricky and complex to pin the modifications to carets).

The EditorUtilities.addCaretUndoableEdit() approach will also allow us to get rid of a dependency on BaseDocumentEvent (especially isInUndo()) and work well with DefaultDocumentEvent instances in the future too. In the past it was possible to distinguish an undo of a DocumentEvent because there was removeUpdate() called on an event which had DocumentEvent.getType() == DocumentEvent.EventType.INSERT (similarly for removals). Then the Swing team changed the situation by wrapping the original event into AbstractDocument.UndoRedoDocumentEvent which however is not public so it's no longer possible to cleanly distinguish undos even for a single-caret case with DefaultDocumentEvent. Our approach should cover this too.

Proper text selection restoration is now possible too.

Ad VV3: I would definitely prefer to update the actions and I'll do it as part of this change, I just did not want to inflate the diff by these action updates.

Thanks.
Comment 11 Vladimir Voskresensky 2016-04-20 15:17:39 UTC
Re Ad VV3: My question is: what would happen with existing plugins doing modifications (comment/uncomment is like and example)? Would they all be broken in terms of undo/redo comparing to 8.1? If yes you should increase major version of editor lib to notify incompatible change introduced by multi-carets, isn't it?
Comment 12 Miloslav Metelka 2016-04-21 09:03:49 UTC
Yes, I'll attempt to extend the patch to request a compatible scrolling on modifications too.
Comment 13 Miloslav Metelka 2016-04-28 22:41:48 UTC
I have updated the patch with a compatible scrolling on modifications:
http://hg.netbeans.org/jet-main/rev/54a47a8f482a
Comment 14 Quality Engineering 2016-05-02 02:00:33 UTC
Integrated into 'main-silver', will be available in build *201605020002* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)

Changeset: http://hg.netbeans.org/main-silver/rev/54a47a8f482a
User: Miloslav Metelka <mmetelka@netbeans.org>
Log: #258798 - CaretUndo.createCaretUndoEdit() needs to become an official API for actions.
Comment 15 Miloslav Metelka 2016-05-02 08:29:05 UTC
Unfortunately the commit caused regression #259061 but it's already fixed in jet-main.