[Help Needed] Implement provider for sticky scrolling in JAVA/JDT

[Christopher-Hermann]

What it does

The current sticky scrolling functionality is based solely on indentation. This can cause issues when the source code is formatted in unexpected ways. To address this, an extension point for providing language-specific sticky lines has been introduced in the following pull request: eclipse-platform/eclipse.platform.ui#2398.

This change represents the first proof-of-concept implementation for sticky lines in Java/JDT. As I lack experience in JDT development, the proposed approach for calculating sticky lines in Java may not be optimal.
I would greatly appreciate assistance from the community in identifying a more effective method for calculating sticky lines.

Fixes:
eclipse-platform/eclipse.platform.ui#1950
eclipse-platform/eclipse.platform.ui#2128
eclipse-platform/eclipse.platform.ui#2338

How to test

1. Check out the PR from [StickyScrolling] Add extension point for sticky lines provider eclipse-platform/eclipse.platform.ui#2398. and open plugin org.eclipse.ui.editors
2. Enable the Sticky Scrolling Feature via Preferences > General > Editors > Text Editors > Enable Sticky Scrolling
3. Open the class EnumMap, scroll to line 775 (method writeObject)

With proper Sticky Lines calculation:

With the Default Sticky Lines calculation based on indentation:

[BeckerWdf]

Is JDT interested in such a feature?
@jukzi: Could you ask one of the JDT committers to have a look at this?

[jukzi]

what kind of help do you need? build currently fails because it can not find the internal(!) API
org.eclipse.ui.internal.texteditor.stickyscroll.IStickyLine
referenced as
org.eclipse.ui.texteditor.stickyscroll.IStickyLine

[BeckerWdf]

@Christopher-Hermann asks for this:

This change represents the first proof-of-concept implementation for sticky lines in Java/JDT. As I lack experience in JDT development, the proposed approach for calculating sticky lines in Java may not be optimal.
I would greatly appreciate assistance from the community in identifying a more effective method for calculating sticky lines.

Could you or some of your JDT co-committers help him?

[jukzi]

Could you or some of your JDT co-committers help him?

I lack the skill for that. Maybe @jjohnstn could help?

[BeckerWdf]

ping @jjohnstn
Can you help here?

[jjohnstn]

@BeckerWdf I'll take a look

[jjohnstn]

Hi. I agree that indent level is insufficient information for many Java files. However, using the JavaModel is also insufficient. As you have found, you can only show Types and Methods which is less functionality than currently exists for a normally indented piece of source code (e.g. if statements,else blocks, for statements, etc... are shown).

What you need to do is parse the compilation unit and get an AST. With the AST, you can get the detail needed. For example, you may find the parent of the current statement is a block but your logic can skip over blocs and find the next parent which could be an if statement, for statement, etc... that you can choose to display. The indentation doesn't matter.

There is code in Checks.Java:

	public static CompilationUnit convertICUtoCU(ICompilationUnit compilationUnit) {
		ASTParser parser= ASTParser.newParser(IASTSharedValues.SHARED_AST_LEVEL);
		parser.setKind(ASTParser.K_COMPILATION_UNIT);
		parser.setSource(compilationUnit);
		parser.setResolveBindings(true);

		return (CompilationUnit) parser.createAST(null);
	}

as an example or parsing an ICompilationUnit to get a CompilationUnit. I do not believe you need to resolve bindings so the set call in the example should be false. From there you can find any ASTNode from its position using NodeFinder or use a custom ASTVisitor. You can define what elements you want to show (e.g. if statement, else clause, etc...) vs ignore (e.g. block, variable declaration).

[mickaelistria]

Could the sticky scrolling rely on folding ranges (when available), or the underlying strategy folding is built on, to compute which lines to keep? That would probably be sufficient in most cases; and that would provide good factorization.

[jjohnstn]

@Christopher-Hermann I have created an AST-based version of Java StickyLinesProvider in #2149 Could you try it out and let me know what you think? It looks for various Java AST constructs (e.g. for, if, try, catch, lambda expression) instead of indentation.

[Christopher-Hermann]

@Christopher-Hermann I have created an AST-based version of Java StickyLinesProvider in #2149 Could you try it out and let me know what you think? It looks for various Java AST constructs (e.g. for, if, try, catch, lambda expression) instead of indentation.

Thanks a lot, that's really awesome. I will check as soon as I have time.

[jjohnstn]

@Christopher-Hermann I have created an AST-based version of Java StickyLinesProvider in #2149 Could you try it out and let me know what you think? It looks for various Java AST constructs (e.g. for, if, try, catch, lambda expression) instead of indentation.

Thanks a lot, that's really awesome. I will check as soon as I have time.

If you like it, I think it should be optional to use. Perhaps a sticky lines provider could have a isEnabled() method so that it can check a preference in addition to the extension checking.

[Christopher-Hermann]

I tested a litte bit and it looks quite promising.

I found some minor issues (The line numbers are somehow broken on my current setup, it should not be related to sticky scrolling):

1. The method name is moved into the sticky lines already for java doc.

I'm not sure if this is really an issue, I just noticed it

2. Inner classes are "overwriting" the sticky line of the main class

Regarding the isEnalbed() method: I will check in the next days if it is worth to add such a method or if we can somehow model this via the stickyLinesProviders enhancement point.

Thanks again for your help 👍

[BeckerWdf]

Perhaps a sticky lines provider could have a isEnabled() method so that it can check a preference in addition to the extension checking.

So from a user's perspective we would have the general setting in preferences to enable disable sticky scrolling completely and then have another setting so switch if a user would like to use JDT's sticky scrolling implementation of the "fallback" implementation (based on indentation). As I user would find this confusing. And I also don't see a reason why a Java developer would not like to use the Java-specific implementation.
So let's don't over-engineer this and don't provide a isEnabled() method. If we later find out we really need it we can always add it later.

[jjohnstn]

Perhaps a sticky lines provider could have a isEnabled() method so that it can check a preference in addition to the extension checking.

So from a user's perspective we would have the general setting in preferences to enable disable sticky scrolling completely and then have another setting so switch if a user would like to use JDT's sticky scrolling implementation of the "fallback" implementation (based on indentation). As I user would find this confusing. And I also don't see a reason why a Java developer would not like to use the Java-specific implementation. So let's don't over-engineer this and don't provide a isEnabled() method. If we later find out we really need it we can always add it later.

The reason I suggested enablement is that each time we enter, we need to create an AST for the entire compilation unit. Caching isn't an option if the file is writable unless we can be told that no edits have occurred or no edits before the current position have been made. For extremely large files, getting an AST each time could cause a noticeable delay and for files with errors (possibly cascading to before the current position) this will exhibit issues that the indentation version does not. I wanted to have a way for a user to have a fall-back in such cases.

A possible alternative to enablement is to make the default sticky provider accessible to the JDT version and if the file is too big, we punt to the indentation implementation. If needed, this size could be configurable for the user.

[BeckerWdf]

and for files with errors (possibly cascading to before the current position) this will exhibit issues that the indentation version does not.

Couldn't you implement the sticky scrolling provider in a way that it automatically falls back to the indentation based implementation when it detects that there are syntax errors (above the current position)?

[tobiasmelcher]

I tested this change for a large Java source with 12000 lines of code. Even after enabling the preference "Enable sticky scrolling", the sticky lines area was not visible. I have not seen any sticky lines for this large source. I checked the result values of StickyLinesProviderJava#getStickyLines in the debugger and they looked ok for me; result was not empty and I expected to see sticky lines but did not.

I then introduced following optimization which does not call javaEditor.getElementAt each time the getStickyLines method is called. Instead a simple and stupid cache is used which just remembers the javaElement from the last call (if it is a method) and checks if it can be re-used for the new requested line (see method canReuseLastElement). With this change, the sticky lines are visible and the feature works as expected. Maybe you could check if this approach might be applicable here to improve the performance?

public class StickyLinesProviderJava implements IStickyLinesProvider {

	private IJavaElement lastElement;
	private IDocument lastDocument;
	private long lastDocumentTimestamp;

	@Override
	public List<IStickyLine> getStickyLines(ISourceViewer sourceViewer, int lineNumber, StickyLinesProperties properties) {
		LinkedList<IStickyLine> stickyLines= new LinkedList<>();
		JavaEditor javaEditor= (JavaEditor) properties.editor();

		IJavaElement element= null;
		try {
			IDocument document= sourceViewer.getDocument();
			int lineOffset= document.getLineOffset(lineNumber);
			if (canReuseLastElement(document, lineOffset)) {
				element= lastElement;
			} else {
				element= javaEditor.getElementAt(lineOffset);
			}
			if (element.getElementType() == IJavaElement.METHOD) {
				lastElement= element;
				lastDocumentTimestamp= getDocumentTimestamp(document);
				lastDocument=document;
			}else {
				lastElement=null;
				lastDocumentTimestamp=0;
				lastDocument=null;
			}
		} catch (BadLocationException e) {
			// TODO Auto-generated catch block
			e.printStackTrace();
		}
		while (element != null) {
			if (element.getElementType() == IJavaElement.METHOD || element.getElementType() == IJavaElement.TYPE) {
				try {
					ISourceRange sourceRange= ((ISourceReference) element).getNameRange();
					int offset= sourceRange.getOffset();
					int stickyLineNumber= sourceViewer.getDocument().getLineOfOffset(offset);
					stickyLines.addFirst(new StickyLine(stickyLineNumber, sourceViewer));
				} catch (JavaModelException | BadLocationException e) {
					// TODO Auto-generated catch block
					e.printStackTrace();
				}
			}
			element= element.getParent();
		}
		return stickyLines;
	}


	private long getDocumentTimestamp(IDocument document) {
		if (document instanceof AbstractDocument ad) {
			return ad.getModificationStamp();
		}
		return 0;
	}


	private boolean canReuseLastElement(IDocument document, int offset) {
		if (document.equals(lastDocument)==false) {
			return false;
		}
		if (lastElement instanceof ISourceReference r) {
			long current= getDocumentTimestamp(document);
			if (lastDocumentTimestamp != current) {
				return false;
			}
			try {
				ISourceRange range= r.getSourceRange();
				if (offset >= range.getOffset() && offset <= range.getOffset() + range.getLength()) {
					return true;
				}
			} catch (JavaModelException e) {
				e.printStackTrace();
			}
		}
		return false;
	}
}

[jjohnstn]

and for files with errors (possibly cascading to before the current position) this will exhibit issues that the indentation version does not.

Couldn't you implement the sticky scrolling provider in a way that it automatically falls back to the indentation based implementation when it detects that there are syntax errors (above the current position)?

That was my suggestion at the end of my comment. If the default sticky scrolling provider could be extended, I could use a super call when needed. It might lead to some weirdness because the origin of this requirement is that some files are indented with braces on the next line which causes braces to be shown instead of meaningful statements. So, when errors appear, we might back off into braces being shown - users might understand this though.

Having the ability to extend the default sticky provider using indentation also provides the ability to add a separate enablement in JDT UI, if desired.

[jjohnstn]

I tested this change for a large Java source with 12000 lines of code. Even after enabling the preference "Enable sticky scrolling", the sticky lines area was not visible. I have not seen any sticky lines for this large source. I checked the result values of StickyLinesProviderJava#getStickyLines in the debugger and they looked ok for me; result was not empty and I expected to see sticky lines but did not.

I then introduced following optimization which does not call javaEditor.getElementAt each time the getStickyLines method is called. Instead a simple and stupid cache is used which just remembers the javaElement from the last call (if it is a method) and checks if it can be re-used for the new requested line (see method canReuseLastElement). With this change, the sticky lines are visible and the feature works as expected. Maybe you could check if this approach might be applicable here to improve the performance?

I think that caching/reuse in the last file is a good idea. I was also going to try caching class file asts to prevent creating them if the file has already been edited. I will put something together today and update the patch.

[jjohnstn]

Ok, I have added caching of Compilation Units and time stamps to my patch. It is based on the type root. Works considerably better for my tests using JDK HashMap and EnumMap class files. At the moment the caches are never cleared. I think a listener for editor update/close is in order to prevent bloat.

I am currently still determining the sticky lines from scratch (i.e. get the current line node then use node parentage), but as mentioned the editor and sticky feature is running much better without any reuse of previous sticky line info. A problem existed with regular for loops that had an empty expression and I have fixed that after noticing it pop up in HashMap.

Occasionally I am seeing a model modification exception thrown when I try to get the working copy, but I am doing no changes. Not sure if there is an error in my logic or there is a platform error, or whether my target platform is causing this. I was getting this error before when I tried discarding the working copy I got for a class file, but I added a check to prevent this.

[Christopher-Hermann]

Having the ability to extend the default sticky provider using indentation also provides the ability to add a separate enablement in JDT UI, if desired.

Yes, I think this is a good idea. If there are no concerns I will move the Default provider in the public package as well.

[BeckerWdf]

Having the ability to extend the default sticky provider using indentation also provides the ability to add a separate enablement in JDT UI, if desired.

Yes, I think this is a good idea. If there are no concerns I will move the Default provider in the public package as well.

So that other providers can inherit from the default provider implementation? Inheriting does create a very hard coupling and might it harder to change the default provider later on (because methods are part of the API).

[Christopher-Hermann]

So that other providers can inherit from the default provider implementation?

Yes exactly.

Another option would be to make the DefaultStickyLinesProvider a final class so that other implementations can fall back by calling DefaultStickyLinesProvider#getStickyLines. But you are right, even this approach is introducing some ugly dependency which makes it harder to change behavior in the DefaultStickyLinesProvider.

[BeckerWdf]

So that other providers can inherit from the default provider implementation?

Yes exactly.

Another option would be to make the DefaultStickyLinesProvider a final class so that other implementations can fall back by calling DefaultStickyLinesProvider#getStickyLines. But you are right, even this approach is introducing some ugly dependency which makes it harder to change behavior in the DefaultStickyLinesProvider.

What about adding a simply API that implementers can call that wraps this "calling DefaultStickyLinesProvider#getStickyLines"? Would that be better / more flexible or is that engineered?

[jjohnstn]

@Christopher-Hermann Could you try the latest version of the patch? I made a few fixes and made it faster.