Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 541404

Summary: [code mining] Editor scrolls and caret jumps on undo/redo
Product: [Eclipse Project] Platform Reporter: Noopur Gupta <noopur_gupta>
Component: TextAssignee: Mickael Istria <mistria>
Status: RESOLVED FIXED QA Contact:
Severity: major    
Priority: P3 CC: akurtakov, azerr, daniel_megert, ericwill, Lars.Vogel, mistria, sarika.sinha
Version: 4.10   
Target Milestone: 4.12 M1   
Hardware: All   
OS: All   
See Also: https://bugs.eclipse.org/bugs/show_bug.cgi?id=542107
https://git.eclipse.org/r/#/c/138192/
https://git.eclipse.org/r/138192
https://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=dd94c4b6d3b87eb57d89f2afa3ef4dc7b96e8488
https://git.eclipse.org/r/138208
https://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=b0d63d9ce8037c9f304ee2e669621266df54820c
https://git.eclipse.org/r/138227
https://git.eclipse.org/c/platform/eclipse.platform.text.git/commit/?id=cc50b4dc1661f9bb249b77b57ff4a362304ff143
Whiteboard:
Bug Depends on: 529127    
Bug Blocks:    
Attachments:
Description Flags
Bugs
none
JDT Stack Trace when undo is done none

Description Noopur Gupta CLA 2018-11-21 09:55:23 EST
With code minings enabled, perform some editing actions/typing in an editor. Then, press Ctrl+Z multiple times to undo.

As shown in the attached animated gif, I can see two issues:
- The editor scrolls with every Ctrl+Z
- The caret jumps randomly to a previous line on each Ctrl+Z
Comment 1 Noopur Gupta CLA 2018-11-21 09:56:53 EST
Created attachment 276645 [details]
Bugs
Comment 2 Noopur Gupta CLA 2018-11-21 09:58:03 EST
Angelo, this should be fixed for 4.10.
Comment 3 Mickael Istria CLA 2018-11-21 11:29:12 EST
This could be an issue for CodeMining in general, not only JDT ones.
It should be tried against the .target file editor which now uses code-mining too.
Comment 4 Mickael Istria CLA 2018-11-23 02:54:00 EST
Resetting assignee to clarify someone else can work on this at the moment.
Comment 5 Alexander Kurtakov CLA 2018-11-23 05:47:49 EST
I can see the carret jump on clicking the mining in target editor. It goes to the line after the mining.
Comment 6 Mickael Istria CLA 2018-11-23 05:58:26 EST
I'm going to investigate and hopefully fix this one.
Comment 7 Angelo ZERR CLA 2018-11-23 07:19:44 EST
Created attachment 276672 [details]
JDT Stack Trace when undo is done

@Mickael, as it's very hard to reproduce if you don't know the context, I give you some information. I suggest you that you set a breakpoint in the StyledText#setContent(StyledTextContent content) or better in DefaultDocumentAdapter#resumeForwardingDocumentChanges() which will call StyledText#setContent(StyledTextContent content).

This problem occurs:

 * when you do a refactor of method name which have code mining header.
 * when you do an undo after having importing package with completion (case of original issue). I mean import (after completion) a package. For instance if you type OutputStre and you apply it, it will add import 'java.io'. After an undo, you will have this problem too.

Both cases, call DefaultDocumentAdapter#resumeForwardingDocumentChanges(), which call StyledText#reset() which loose the lines height, etc and I think it's a reason why cursor location is lost.

Good luck @Mickael, it's a very hard task!

See attached demo to see stacktraces.
Comment 8 Mickael Istria CLA 2018-11-23 11:40:17 EST
I am not able to reproduce it on Linux so far.
I think it's a painting issue. The caret may actually be placed at the right line in the model, but one of the code-mining redraw or the StyledText.reset() fails are redrawing it at the right location.
When you can reproduce this, can you please check how the caret behaves to the Left/Right arrow to see on which line it thinks it is?

(In reply to Alexander Kurtakov from comment #5)
> I can see the carret jump on clicking the mining in target editor. It goes
> to the line after the mining.

I don't think it's related to this one.
Comment 9 Mickael Istria CLA 2018-11-23 13:54:58 EST
I'm unassigning myself as I cannot reproduce it locally.
Who want to give a try?
Comment 10 Angelo ZERR CLA 2018-11-23 14:09:38 EST
> I'm unassigning myself as I cannot reproduce it locally.
Who want to give a try?

Have you set a breakpoint in DefaultDocumentAdapter#resumeForwardingDocumentChanges() to check you ar ein the usecase (try do a refactor on method which have a line header code mining below)?
Comment 11 Sarika Sinha CLA 2018-11-27 00:16:34 EST
(In reply to Alexander Kurtakov from comment #5)
> I can see the carret jump on clicking the mining in target editor. It goes
> to the line after the mining.

I was trying out on target editor, but a strange behavior is visible, see Bug 541575. 

On a normal Java file, trying to undo does moves the caret here there, not at all predictable.
Caret jump was reproducible on Windows and Mac but not on Linux.

Scrolling up and down is reproducible on Linux also and that should be fixed.

@Michael,
You are not able to reproduce scroll up and down on Linux?
Try it on TextViewer.java, it may not be reproducible on a new Java Project.
Comment 12 Mickael Istria CLA 2018-11-27 03:18:24 EST
(In reply to Sarika Sinha from comment #11)
> @Michael,
> You are not able to reproduce scroll up and down on Linux?
> Try it on TextViewer.java, it may not be reproducible on a new Java Project.

I'll try.
Can we share some exact steps to reproduce to make sure we're testing the same thing and can refer to the same steps? Ideally putting those steps a JUnit test case?
Comment 13 Dani Megert CLA 2018-12-04 11:24:22 EST
Ping! One day left for 4.10.
Comment 14 Mickael Istria CLA 2018-12-04 14:19:30 EST
(In reply to Dani Megert from comment #13)
> Ping! One day left for 4.10.

I personally didn't manage to reproduce the issue. I experienced it once in regular usage, kind of randomly, and tried to reproduce it and never managed to do it.
Comment 15 Eric Williams CLA 2018-12-04 15:42:23 EST
I was able to reproduce the cursor hopping issue that Alex describes in comment 5. I filed a bug for this in SWT, see bug 542107.
Comment 16 Lars Vogel CLA 2019-02-19 03:32:32 EST
Mass change, please reset target if you still planning to fix this for 4.11.
Comment 17 Mickael Istria CLA 2019-03-05 10:09:10 EST
I could now reproduce, at least in the case of JDT with the `System.err.println("a")` line. Most other cases don't show an error.
Comment 18 Mickael Istria CLA 2019-03-06 08:04:23 EST
When the issue happens on an Undo, then next Redo seems to also trigger the issue.
What I'm looking at now is that StyledText.setStyleRange(...) receive a different result from getPartialTopIndex() when this issue happens from when it doesn't.
Comment 19 Mickael Istria CLA 2019-03-06 08:18:56 EST
A deeper cause seems to be that under some circumstances StyledText.handleTextChanged(...) isn't called -causing bug.
Comment 20 Mickael Istria CLA 2019-03-06 08:41:55 EST
TextViewerUndoManager.DocumentUndoListener.documentUndoNotification sometimes invoke `setRedraw(true)` which causes necessary StyledText.handleTextChanged() listener to never be invoked, so topIndex and others not computed as expected.
Comment 21 Mickael Istria CLA 2019-03-06 11:47:37 EST
https://git.eclipse.org/r/#/c/138192/ is an optimization/workaround: it makes the undo/redo process a simpler flow when there is only 1 change in the "Compound" one. This makes the StyledText go through a non-bugged state.
I believe that while this change isn't an exhaustive fix, it's a good optimization that can have good impact on other parts of the IDE.
Comment 22 Eclipse Genie CLA 2019-03-06 13:55:31 EST
New Gerrit change created: https://git.eclipse.org/r/138192
Comment 24 Eclipse Genie CLA 2019-03-06 13:56:11 EST
New Gerrit change created: https://git.eclipse.org/r/138208
Comment 26 Eclipse Genie CLA 2019-03-06 17:12:19 EST
New Gerrit change created: https://git.eclipse.org/r/138227
Comment 28 Mickael Istria CLA 2019-03-11 08:49:29 EDT
Merged patch seems enough to fix the erroneous case.
Thanks Noopur for the report and review!