Bug 532832 - [code mining] Code minining leads to editor movement after refactoring
Summary: [code mining] Code minining leads to editor movement after refactoring
Status: NEW
Alias: None
Product: Platform
Classification: Eclipse Project
Component: Text (show other bugs)
Version: 3.7.1   Edit
Hardware: All All
: P3 normal with 2 votes (vote)
Target Milestone: ---   Edit
Assignee: Platform-Text-Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 547167 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-03-23 10:51 EDT by Lars Vogel CLA
Modified: 2022-01-14 03:24 EST (History)
6 users (show)

See Also:


Attachments
Demo which shows the problem of movement with codemining (237.81 KB, image/gif)
2018-03-26 05:19 EDT, Angelo ZERR CLA
no flags Details
Stack trace of first call of reset (76.50 KB, image/png)
2018-03-27 19:40 EDT, Angelo ZERR CLA
no flags Details
Stack trace of second call of reset (72.39 KB, image/png)
2018-03-27 19:41 EDT, Angelo ZERR CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Lars Vogel CLA 2018-03-23 10:51:06 EDT
If I have code mining active and refactor a method in a longer class the scroll position jumps. I have not seen that without code mining active.

Animated GIF can be found on Github: https://github.com/angelozerr/jdt-codemining/issues/7
Comment 1 Angelo ZERR CLA 2018-03-26 05:19:04 EDT
Created attachment 273293 [details]
Demo which shows the problem of movement with codemining

@Mickael, I think it's an important bug to fix. I have a usecase which causes this problem (mining must be displayed with 0 references):

1) Create class like this:

-------------------------------------------------------

public class CursorLocationProblemAfterRefactor {
	
	public void f() {
		
	}
}
-------------------------------------------------------

2) Open this class in Java Editor and resize the editor with little size to see the vertical scrollbar on the right.

3) Move the vertical scrollbar on the bottom.

4) rename f method and you will see the problem of movement.

See attached demo which reproduce thoses 4 steps.
Comment 2 Angelo ZERR CLA 2018-03-26 12:25:30 EDT
@Mickael, after debugging quickly, I suggest you to set a breakpoint in StyledText#setTopPixel(int pixel) before doing the refactoring (like the attached demo).

This method is called twice in the TextViewerState.updateViewport(). The first time it is called with a well pixel value (no movement) and the second time it is called with bad pixel value (the movement is done).

Do you think you will have time to study the problem? IMHO I think it's an important bug to fix it.
Comment 3 Angelo ZERR CLA 2018-03-27 03:23:56 EDT
@Mickael I have changed the importantce to "critical" because when you do a refactor (like the demo) or completion apply which insert "import", the move can be occurred and sometimes if you are at the end of the editor, after the "refactor", you are in the begin of the editor. IMHO I think this issue is very critical, but if you don't think so, please update the importance.
Comment 4 Eclipse Genie CLA 2018-03-27 16:20:31 EDT
New Gerrit change created: https://git.eclipse.org/r/120305
Comment 5 Angelo ZERR CLA 2018-03-27 19:39:03 EDT
@Mickael I have created a gerrit patch which fixes the problem https://git.eclipse.org/r/#/c/120305/ but I'm afaraid that it causes problem. The idea is to keep topIndex, topIndexY when reset() is called.

In case of refactoring reset is called twice. I will attach screenshot to see stack trace.

@Mickael please note it's a just a gerrit patch to start conversation. I don't know if it breaks something. Thanks for your feedback!
Comment 6 Angelo ZERR CLA 2018-03-27 19:40:09 EDT
Created attachment 273325 [details]
Stack trace of first call of reset
Comment 7 Angelo ZERR CLA 2018-03-27 19:41:01 EDT
Created attachment 273326 [details]
Stack trace of second call of reset
Comment 8 Angelo ZERR CLA 2018-03-28 02:40:53 EDT
@Lars could you tell me if my gerrit patch fixes https://github.com/angelozerr/jdt-codemining/issues/7 ?

Thanks!
Comment 9 Angelo ZERR CLA 2018-03-28 09:32:40 EDT
@Mickael, I have understood more the problem, I have created a gerrit patch but tests crashes again (not found solution for the moment).

Here the problem: when rename is done (after apply Enter), the TextViewer#enabledRedrawing is called which calls:

 1° DefaultDocumentAdapter#resumeForwardingDocumentChanges() is called which calls StyledText#reset() which calls renderer.setContent(content) which calls StyledTextRenderer.reset() which remove all styles. So at this steps there are none GlyphMetrics used to compute line height.

 2° TextViewer#ViewerState#updateViewport() which consumes getLinePixel of StyledWidget which return bad value because line height is not good (because GlyphMetrics  which set custom line height) doesn't exists at this step.

My gerrit patch is basic, it doesn't reset the styles and it work well with Java Editor, but tests are not passed because when text changed styles must be reseted.

Any help are welcome!
Comment 10 Lars Vogel CLA 2018-03-29 09:08:01 EDT
(In reply to Angelo ZERR from comment #8)
> @Lars could you tell me if my gerrit patch fixes
> https://github.com/angelozerr/jdt-codemining/issues/7 ?
> 
> Thanks!

AFAICS not, position still jumps (if I did the testing correct). Does it fix it for you?
Comment 11 Angelo ZERR CLA 2018-04-02 14:18:14 EDT
> AFAICS not, position still jumps (if I did the testing correct). Does it fix it for you?

Argh -( Yes it works for me. My gerrit patch is just an experimentation, but it breaks tests. I'm afraid it's an hard issue.

@Mickael, is there any chance that you help me about this issue. I think it's a critical bug: if you have a lot of mining and you apply completion which add import code, you could move sometimes around on the top of the editor.
Comment 12 Mickael Istria CLA 2018-04-03 04:00:17 EDT
Sorry Angelo,
I'm currently quite busy with some more critical things and am unable to estimate when I'll be able to help you with that. Then, at the end of this week, I'll be on PTO for a week... I'll try to have a quick look at it whenever I can, but not sure it can be before mid-April then.
Comment 13 Angelo ZERR CLA 2018-04-03 04:09:03 EDT
Thanks for your answer @Mickael, I will try to investigate this problem if I find time. An another codemining topic which is very important is https://bugs.eclipse.org/bugs/show_bug.cgi?id=532924#c16 If you have time just to give me an answer, it should be cool. Thanks!
Comment 14 Mickael Istria CLA 2018-04-03 09:21:16 EDT
So this happens in any text editor right?
And it's mostly caused by the fact that GlyphMetrics are removed, and then re-added too early, right?
Comment 15 Angelo ZERR CLA 2018-04-03 09:48:12 EDT
> So this happens in any text editor right?

Yes. It's the case of refactoring for instance.

> And it's mostly caused by the fact that GlyphMetrics are removed, and then re-added too early, right?

The main problem is that because StyledText#reset is called after an Enter (to apply refactor name). See my explanation at https://bugs.eclipse.org/bugs/show_bug.cgi?id=532832#c9

The StyledText#reset, reset the all line height and all GlyphMetrics. So after when int linePixel = getTextWidget().getLinePixel(stableWidgetLine)is called, the value is bad because line height doesn't take care of custom line height of line header annotation.

This behaviour is understandable, because after apply of refactoring, text can change (like add a new line for import, et). I don't know how to fix this problem?
Comment 16 Mickael Istria CLA 2019-05-16 16:59:18 EDT
*** Bug 547167 has been marked as a duplicate of this bug. ***
Comment 17 Pierre-Yves Bigourdan CLA 2019-09-23 16:32:00 EDT
This also seems to happen when formatting a piece of code.