Folding mechanism for while/for/if/...

[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?

[fedejeanne]

Some minor comments for the moment.

[fedejeanne]

You shouldn't need this. Try to improve the overall design of the whole configuration page and make it look more like the Content Assist page, which has Groups (with borders). You should have 2 groups:

• Initially fold ...
• Extended folding (Experimental) : This one is yours and has
  • A Label with an explanation like "Enable folding for other code blocks. Activating this feature may cause a significant performance degradation"
  • A checkbox: "Activate feature".

[jakub-suliga]

Do you want it like this?

[jakub-suliga]

Done

[fedejeanne]

Can you let the groups adapt better to the size of the dialog, just like with the Content Assist?

[iloveeclipse]

• A checkbox: "Activate feature".

Probably better to explicitly say "Enable folding for if/for/while blocks" ?

[jakub-suliga]

No, because it enables folding not only for if, for, and while blocks, but also for constructs such as try/catch blocks, lambdas et cetera.

[fedejeanne]

The test failure is unrelated. #1095

[fedejeanne]

The build failure seems to be unrelated, it's a timeout. Maybe it will work next time it runs (e.g. when you rebase and push again).

15:42:55  [ERROR] Failed to execute goal org.eclipse.tycho:tycho-apitools-plugin:4.0.11-SNAPSHOT:verify (verify) on project org.eclipse.jdt.ui: Execution verify of goal org.eclipse.tycho:tycho-apitools-plugin:4.0.11-SNAPSHOT:verify failed: Could not mirror artifact osgi.bundle,org.mortbay.jasper.apache-jsp.source,9.0.96 into the local Maven repository.See log output for details. request timed out -> [Help 1]

[fedejeanne]

The code looks much better now, thank you for the corrections @jakub-suliga.

Maybe after you fix my last #1562 (comment), a JDT committer (@iloveeclipse?) would like to review again?

I'm off for today, have a nice weekend!

[iloveeclipse]

I can't guarantee I will have time next week, I'm overloaded. The feature itself seems to be useful, so if the new test also covers existing functionality & is green, why not merge?

@jjohnstn : are you available to review this?

[jjohnstn]

@iloveeclipse Sure

[jjohnstn]

@jakub-suliga Just trying it out, it seems to work well. I tried the given test but it does not fold the switch statement or its cases. Is that expected or is that something that isn't working in my particular test?

I ran into a major bug with folding on my Linux platform which is likely Linux specific. If I fold, then hover over the plus and then click on the hover to focus it; there is no way out but Esc. This closes the hover but the flashing cursor is missing and the window pointer can get changed to the window expansion characters if I am on the edges of the hover area at the time of escape. I got similar experiences sometimes after entering via F2 to get focus in the hover window, but it seemed better and I often got the flashing cursor back. Once the cursor was gone, I could only get it back by switching into another Eclipse and its editor and then switching back. Again, likely Fedora specific, but I ask in case you have Windows/Mac and can confirm that it doesn't occur for you. It is not caused by this PR.

[jjohnstn]

@jakub-suliga Just trying it out, it seems to work well. I tried the given test but it does not fold the switch statement or its cases. Is that expected or is that something that isn't working in my particular test?

Never mind. I can see from the code that switch statements aren't covered.

[jjohnstn]

Overall, code looks good, but 2 comments to address.

[jjohnstn]

Your removal of the CommentPosition class has changed functionality. The old folding for a Javadoc comment will show you the first line after the /** after folding but your code just shows /**

/**
 * Function comment
 *
 */

Old:

* Function comment ...

New:

/**
 */

[jjohnstn]

It would be nice to have at least one test that verifies the region values are as expected as opposed to just count (e.g. verify that finally region starts at the beginning of the finally keyword and not the block