Folding mechanism for while/for/if/switch-case

[jakub-suliga]

Problem:
Currently, there is no folding mechanism for while/for/if/switch-case statements in Eclipse. VSCode and IntelliJ support a folding mechanism for these statements. Therefore, I have created a method that implements this folding mechanism. This is also an open issue: #1426. In addition, I deleted some commented-out code from 2007.

Before:

After:

How to test:

1. Start an Eclipse workspace and create a new class.
2. Add while/for/if/switch-case statements to this class.
3. Verify that the folding mechanism works as expected.

Here is a small program you can use to test:

private int zaehler;
private final Object sperre = new Object();


public void test() {
    if (zaehler > 0) {
        System.out.println();
    } else if (zaehler == 0) {
        System.out.println();
    } else {
        System.out.println();
    }

    while (zaehler < 10) {
        zaehler++;
    }

    for (int i = 0; i < zaehler; i++) {
        System.out.println();
    }

    do {
        zaehler--;
    } while (zaehler > 0);

    int[] zahlen = {1, 2, 3, 4, 5};
    for (int zahl : zahlen) {
        System.out.println();
    }

    switch (zaehler) {
        case 0:
            System.out.println();
            break;
        case 1:
            System.out.println();
            break;
        default:
            System.out.println();
            break;
    }

    try {
        int ergebnis = 10 / zaehler;
    } catch (ArithmeticException e) {
        System.out.println();
    } finally {
        System.out.println();
    }

    synchronized (sperre) {
        zaehler++;
    }

    Runnable runnable = () -> {
        System.out.println();
    };
    runnable.run();

    {
        System.out.println();
    }

    for (int i = 0; i < 5; i++) {
        for (int j = 0; j < 5; j++) {
            if (i * j > 6) {
            }
            System.out.println();
        }
    }

    try {
        String text = null;
        text.length();
    } catch (NullPointerException e) {
        System.out.println();
    } catch (Exception e) {
        System.out.println();
    } finally {
        System.out.println();
    }

    {
        System.out.println();
    }
    if (zaehler > 0) {
        for (int i = 0; i < zaehler; i++) {
            while (i < 5) {
                System.out.println();
                i++;
            }
        }
    }

    lambdaVerwenden();
}

public void lambdaVerwenden() {
    verarbeiteLambda(() -> {
        System.out.println();
    });
}

private void verarbeiteLambda(Runnable runnable) {
    runnable.run();
}

Bugs:

• [Bug] [Folding mechanism] Method declaration on the Same Line as the Closing Bracket #1539
• When there are multiple nested while/for/if/switch-case statements, not all foldable regions are recognized.

[jakub-suliga]

Bug:

• When you press enter or delete a line and the element is closed, the folding mechanism gets buggy

[fedejeanne]

The test failures seem to be unrelated: https://ci.eclipse.org/jdt/job/eclipse.jdt.ui-github/job/PR-1562/21/testReport/

The only test that fails with age == 1 (i.e. in this PR) is org.eclipse.jdt.text.tests.PluginsNotLoadedTest.pluginsNotLoaded but looking at its stack trace I don't see any hints that point to this PR either:

java.lang.AssertionError: 
Wrong bundles loaded:
- org.eclipse.jdt.junit
 expected:<0> but was:<24>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.eclipse.jdt.text.tests.PluginsNotLoadedTest.pluginsNotLoaded(PluginsNotLoadedTest.java:278)
...

Does anyone know the reason for this failure?

In the meantime @jakub-suliga, try with another force-push to re-trigger the checks and see if the failure persists.

[fedejeanne]

In the meantime @jakub-suliga, try with another force-push to re-trigger the checks and see if the failure persists.

On a closer look, the test failures are because of the changes introduced in this PR. Run them locally and you will see it.

Here's (part of) the console output when running org.eclipse.jdt.ui.tests.quickfix.AnnotateAssistTest1d5.testAnnotateReturn2():

!STACK 0
java.lang.ClassCastException: class org.eclipse.jdt.internal.core.ClassFile cannot be cast to class org.eclipse.jdt.core.ICompilationUnit (org.eclipse.jdt.internal.core.ClassFile and org.eclipse.jdt.core.ICompilationUnit are in unnamed module of loader org.eclipse.osgi.internal.loader.EquinoxClassLoader @21452f5e)
	at org.eclipse.jdt.ui.text.folding.DefaultJavaFoldingStructureProvider.computeFoldingStructure(DefaultJavaFoldingStructureProvider.java:981)
	at org.eclipse.jdt.ui.text.folding.DefaultJavaFoldingStructureProvider.update(DefaultJavaFoldingStructureProvider.java:907)
	at org.eclipse.jdt.ui.text.folding.DefaultJavaFoldingStructureProvider.initialize(DefaultJavaFoldingStructureProvider.java:852)
	at org.eclipse.jdt.ui.text.folding.DefaultJavaFoldingStructureProvider.handleProjectionEnabled(DefaultJavaFoldingStructureProvider.java:822)
	at org.eclipse.jdt.ui.text.folding.DefaultJavaFoldingStructureProvider$ProjectionListener.projectionEnabled(DefaultJavaFoldingStructureProvider.java:700)
...
	at org.eclipse.jdt.ui.tests.quickfix.AnnotateAssistTest1d5.testAnnotateReturn2(AnnotateAssistTest1d5.java:178)
...

It seems the fInput isn't always an ICompilationUnit as the new code assumes. You'll have to "fork" the execution (with an if (fPart instanceof ICompilationUnit)) and preserve the old code too (in the else block).

It would be interesting to know when exactly is fInput not and ICompilationUnit and if it's possible to turn them into ICompilationUnits (in a separate PR). By the looks of it, it might be when one opens a .class file, probably one that displays source code, like java.lang.String:

[fedejeanne]

@iloveeclipse : Jakub and I were looking into the failed tests and we found the culprit: it was a null. After some debugging and evaluating we decided that a null-check would make sense since now (in this PR) it is possible to add folding structures for things that are not IJavaElements. This led to commit ade7b52.

Do you see any possible problem with this change? We couldn't come up with any but maybe you can?

[danthe1st]

Hi @jakub-suliga
I have been working on changes to DefaultJavaFoldingStructureProvider aside you (#1825) and I checked for merge conflicts and ran my tests to check for similar issues (out of curiosity) by rebasing your changes on top of mine.

While there were (unsurprisingly) some merge conflicts, these were all trivial to resolve.

My tests noticed the following changes to folding to the current folding when using your code (see danthe1st@d0c8ba8 where I adapted my tests - if your PR is merged after mine, you might have to do something similar):

If there is a single import, your implementation adds a folding region for that import.

package a;

import java.util.List;

If a class contains fields, your implementation adds a folding region for every field of the class. In the following example, int a; is folded. With multiple fields, each field gets its own folding region.

package a;

public class JakobTest2 {
	int a;
}

These are changes in behavior from the current master (these also occur without my custom folding changes) introduced by your PR if your "New Folding" option is disabled. I want to inform you about this changes since they seem to be unintentional (at least the second one with the fields).

Another change you made that interacted with my test is your removal of CommentPosition. This changes where exactly comment regions "end" (it essentially removes some aligning done by CommentPosition which my test relies on) but that shouldn't have any impact to users because the end of the region should still be in the same line (I think) but it would be necessary to relax the checking of the tests (danthe1st@d0c8ba8#diff-e8836e7c6985ef3dc58865becd219f6708db0854c28f22c7b29843456c704652R379) which may be useful for you to know if my PR gets merged before yours.

You can see the result of me rebasing your changes on top of mine here: https://github.com/danthe1st/eclipse.jdt.ui/tree/jakob-folding-test

(I am not recommending to base your changes on mine (on PR can be rebased whenever the other is merged), I just did it as an experiment/to see what happens/how our changes interact with each other.)

[jakub-suliga]

Hi @danthe1st,
thank you for reviewing my PR! I fixed the bug with the single import, but I couldn’t reproduce the second bug.

After disabling my option, you need to restart the IDE. I’ll add a message to inform users that a restart is required.
I also removed CommentPosition because I didn’t need it, but feel free to add it back if you still need it.

[danthe1st]

Hi @danthe1st, thank you for reviewing my PR! I fixed the bug with the single import, but I couldn’t reproduce the second bug.

For me, this issue also happens even when starting Eclipse without your "new folding" enabled (and without enabling it).
Here is one test for it (the test is for my changes and it's adapted to "allow" for your behavior so you'd have to change the assertEquals(3, projectionRanges.size()); to assertEquals(1, projectionRanges.size()); and the following assertions to remove all but assertContainsRegionUsingStartAndEndLine(projectionRanges, str, 6, 8);//void b()):

Here is the modified test code that should reproduce the issue (put it somewhere in org.eclipse.jdt.ui.tests/ui and run it as a "JUnit Plug-In test").

//TODO add your package declaration here

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assertions.fail;

import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;

import org.junit.After;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;

import org.eclipse.jdt.testplugin.JavaProjectHelper;

import org.eclipse.core.runtime.CoreException;

import org.eclipse.jface.text.IRegion;
import org.eclipse.jface.text.Position;
import org.eclipse.jface.text.Region;
import org.eclipse.jface.text.source.Annotation;
import org.eclipse.jface.text.source.projection.ProjectionAnnotation;
import org.eclipse.jface.text.source.projection.ProjectionAnnotationModel;

import org.eclipse.ui.PartInitException;

import org.eclipse.jdt.core.ICompilationUnit;
import org.eclipse.jdt.core.IJavaProject;
import org.eclipse.jdt.core.IPackageFragment;
import org.eclipse.jdt.core.IPackageFragmentRoot;
import org.eclipse.jdt.core.JavaModelException;

import org.eclipse.jdt.ui.tests.core.rules.ProjectTestSetup;

import org.eclipse.jdt.internal.ui.javaeditor.EditorUtility;
import org.eclipse.jdt.internal.ui.javaeditor.JavaEditor;

public class FoldingTest {
	@Rule
	public ProjectTestSetup projectSetup= new ProjectTestSetup();

	private IJavaProject fJProject1;

	private IPackageFragmentRoot fSourceFolder;

	private IPackageFragment fPackageFragment;

	@Before
	public void setUp() throws CoreException {
		fJProject1= projectSetup.getProject();
		fSourceFolder= JavaProjectHelper.addSourceContainer(fJProject1, "src");
		fPackageFragment= fSourceFolder.createPackageFragment("org.example.test", false, null);
	}


	@After
	public void tearDown() throws CoreException {
		JavaProjectHelper.delete(fJProject1);
	}
	@Test
	public void testCustomRegionsAroundFieldAndMethod() throws JavaModelException, PartInitException {
		String str= """
				package org.example.test;

				public class Test {
					// #region
					int a;

					void b(){

					}
					// #endregion
				}
				""";
		List<IRegion> projectionRanges= getProjectionRangesOfFile(str);
		assertEquals(1, projectionRanges.size());//needs to be 2 to pass with your code
		assertContainsRegionUsingStartAndEndLine(projectionRanges, str, 6, 8);//void b()
		//assertContainsRegionUsingStartAndEndLine(projectionRanges, str, 4, 5);//int a;//would unexpectedly pass with your code
	}
	private void assertContainsRegionUsingStartAndEndLine(List<IRegion> projectionRanges, String input, int startLine, int endLine) {
		assertTrue(startLine <= endLine, "start line must be smaller or equal to end line");
		int startLineBegin= findLineStartIndex(input, startLine);

		int endLineBegin= findLineStartIndex(input, endLine);
		int endLineEnd= findNextLineStart(input, endLineBegin);
		endLineEnd= getLengthIfNotFound(input, endLineEnd);

		for (IRegion region : projectionRanges) {
			if (region.getOffset() == startLineBegin + 1 && region.getOffset() + region.getLength() >= endLineBegin && region.getOffset() + region.getLength() <= endLineEnd) {
				return;
			}
		}

		fail(
				"missing region from line " + startLine + " (index " + (startLineBegin + 1) + ") " +
						"to line " + endLine + " (index " + (endLineEnd) + ", length "+(endLineEnd - (startLineBegin + 1)) + ")" +
						", actual regions: " + projectionRanges
		);
	}


	private int getLengthIfNotFound(String input, int startLineEnd) {
		if (startLineEnd == -1) {
			startLineEnd= input.length();
		}
		return startLineEnd;
	}


	private int findLineStartIndex(String input, int lineNumber) {
		int currentInputIndex= 0;
		for (int i= 0; i < lineNumber; i++) {
			currentInputIndex= findNextLineStart(input, currentInputIndex);
			if (currentInputIndex == -1) {
				fail("line number is greater than the total number of lines");
			}
		}
		return currentInputIndex;
	}


	private int findNextLineStart(String input, int currentInputIndex) {
		return input.indexOf('\n', currentInputIndex + 1);
	}

	private List<IRegion> getProjectionRangesOfFile(String str) throws JavaModelException, PartInitException {

		ICompilationUnit compilationUnit= fPackageFragment.createCompilationUnit("Test.java", str, false, null);
		JavaEditor editor= (JavaEditor) EditorUtility.openInEditor(compilationUnit);
		ProjectionAnnotationModel model= editor.getAdapter(ProjectionAnnotationModel.class);
		List<IRegion> regions= new ArrayList<>();
		for (Iterator<Annotation> it= model.getAnnotationIterator(); it.hasNext();) {
			Annotation annotation= it.next();
			if (annotation instanceof ProjectionAnnotation projectionAnnotation) {
				Position position= model.getPosition(projectionAnnotation);
				regions.add(new Region(position.getOffset(), position.getLength()));
			}
		}
		return regions;
	}

}

I am developing on Ubuntu 24.10 if that changes anything - but I don't really see a reason why it should.

Disclaimer: I am not a committer or otherwise in the position of giving a proper review (and I didn't even try to properly review anything in your PR, I just made a few experiments and shared my results with you).

After disabling my option, you need to restart the IDE. I’ll add a message to inform users that a restart is required.

Is this absolutely necessary? Is there any reason it shouldn't work by just reopening the source file (and can you get around that - that seems to be quite a limitation)?

I also removed CommentPosition because I didn’t need it, but feel free to add it back if you still need it.

My changes don't really need it but the tests I implemented rely on it being aligned that way (and your changes modify that). I just wanted to give you a heads-up for what you might need to change in my tests if your changes get merged after mine (if it happens the other way round, I can perform the necessary changes).

[jakub-suliga]

Is this absolutely necessary? Is there any reason it shouldn't work by just reopening the source file (and can you get around that - that seems to be quite a limitation)?

I added a ChangeListener, now you dont need to restart your IDE. I will have a look on your test.

[danthe1st]

I added a ChangeListener, now you dont need to restart your IDE.

Oh I see what you meant. I think that without that listener, it should still apply the changes but only after closing and reopening the source file (at least that's what happened in my experience).
I don't think a restart of the IDE is needed in either way.

What your change with the added listener is doing (I think) is making it work without reopening the editor (i.e. that change causes updates to the preferences take effect in already open editors).

[jakub-suliga]

I don't think a restart of the IDE is needed in either way.

I didnt test it with closing the file, probably it works too.

[jakub-suliga]

@danthe1st I looked into your test and found the bug, now it should work.

[danthe1st]

I looked into your test and found the bug, now it should work.

I can confirm that this works now.
I want to note another difference I found with your current change without your new folding enabled but I actually think that might be a good thing.

Let's say we have a class like this:

public class SomeTest {
	void b(){

	}
}

Here, the b() method is folded both with and without your change but there is a slight difference in the way it's folded.
In the current master, the } is included in the folding:


However, in your version, it isn't:

While I personally prefer the way it's done with your version, this is a change in the behavior of the folding without your option being enabled so I think it's worth noting.

[jakub-suliga]

I added it because it fixes this #1539.

For example you have this:

public class Test {
	void b(){

	} void c(){

	}
}

and you want to fold the top method, the lower method is folded too (the old version):

With the change, you can see both methods (my version):