Skip to content

Conversation

@danthe1st
Copy link
Contributor

@danthe1st danthe1st commented Oct 26, 2025

Fixes #2439
Fixes #2596

What it does

When extended folding was introduced, both the existing and extended folding mechanisms were changed to generally not include the last line (with a few exceptions).

If I remember correctly, this was done to prevent hiding the b() method in situations like this:

public class Test {
	void a() {
		
	}void b() {
		
	}
}

This PR fixes this regression by including the last line in the folding regions where appropriate. If there is anything additional in the same line as the end of the folding region (e.g. the b() method above), the last line won't be included.

This change applies to both the "old" and "extended" folding mechanisms but the extended folding will not include the last line in the folding regions in control structures other than lambdas (unless someone tells me when exactly these should be included).

How to test

  • Make sure folding is enabled
  • Create a Java class like the one from Folding misbehave since 2025-03 #2439 (comment)
  • If the preference Java > Editor > Folding > Include last line when folding Java elements is enabled, the } should be hidden
  • This should work with both the "old" and extended folding.

Author checklist

@fedejeanne
Copy link
Contributor

fedejeanne commented Oct 28, 2025

Looks good, I tested only with the snippet provided in #2439 (comment) and I like what I see. I would get rid of the parameter though, it's unnecessary complexity.

Thank you, @danthe1st !

@danthe1st
Copy link
Contributor Author

I would get rid of the parameter though, it's unnecessary complexity.

So you think it makes more sense to always include the last line in the folding region?

@fedejeanne
Copy link
Contributor

fedejeanne commented Oct 29, 2025

I think there is no need burden the user with yet another configuration. I would simply make a decision (show it/don't show it) and ship it.

But that's just my opinion. In any case, a committer would have to make a decision about this in the end. Maybe @iloveeclipse ?

@danthe1st danthe1st force-pushed the include-last-line branch 3 times, most recently from 441a558 to 460f94c Compare November 1, 2025 18:23
@danthe1st danthe1st marked this pull request as ready for review November 1, 2025 18:23
@danthe1st
Copy link
Contributor Author

danthe1st commented Nov 1, 2025

I think there is no need burden the user with yet another configuration. I would simply make a decision (show it/don't show it) and ship it.

I removed that option with https://github.com/eclipse-jdt/eclipse.jdt.ui/compare/92b92fce70fc5864a2491444207d12b1e81e94f6..484b44fd3cfa93a22b5c15b9f804e5bae8e37f11.

I've also some more testing and fixed a remaining issue where certain statements ending with a ; were not folded because that was not considered part of the IJavaElement (so I ignored ;s). In my testing I also found some other issues (regressions that I think came from the introduction of extended folding) but I think these can be worked on independently.

@danthe1st
Copy link
Contributor Author

danthe1st commented Nov 1, 2025

In my testing I also found some other issues (regressions that I think came from the introduction of extended folding) but I think these can be worked on independently.

Nevermind, that was #2596 and I was able to fix it by by treating commas in the same way as semicolons so it makes more sense to do it in this PR.

However, one thing that still needs to be done is adapting all the tests since they expect the last line to be excluded.
image

@danthe1st danthe1st marked this pull request as draft November 1, 2025 18:40
@fedejeanne
Copy link
Contributor

Thank you for looking into this, @danthe1st 💪

@danthe1st
Copy link
Contributor Author

Note the following (change of behavior with extended folding/folding of control structures): My current approach (not fully committed yet) also folds the last line with some extended folding control structures where there is something after the region ends:

try { // not collapsed (start)
    ... // collapsed, region end
} catch(SomeException e) { // not collapsed by first region, second region starts here
    // collapsed
} // folding region end
do { // not collapsed (start)
    ... // collapsed, region end
} while(...); // Not collapsed

For methods and other control structures, the last line is included:

void something() { // not collapsed, region start
    ... // collapsed
} // collapsed, region end
while(...) { // not collapsed, region start
    ... // collapsed
} // collapsed, region end

Furthermore, anything that consists of only two lines is now folded:

void a() { // folding region here
} // this would be collapsed now because it is a region consisting of 2 lines

Also, should break; (and yield?) statements be included in the folding of case blocks or should it stop before? Currently, these are excluded. For consistency, I guess it makes sense to also collapse these?

Also, for things like while and for, it is based on indentation of the next line (there is an explicit check for that (FoldingVisitor#createFoldingRegionForStatement) - why???).
See this:

    while(...) {
        ...
    } // this is not collapsed
    // indentation ends here
    while(...) {
        ...
    } // this is collapsed???
        // indentation ends here

Should I remove that behavior as well (it should be as simple as just removing the whole if (!(node instanceof Block)) {)?

Note: With my (current) approach, comments would prevent the last line from being folded.

@fedejeanne
Copy link
Contributor

Also, should break; (and yield?) statements be included in the folding of case blocks or should it stop before? Currently, these are excluded. For consistency, I guess it makes sense to also collapse these?

Hiding these might make it look like a case in a switch uses "fall-through" logic even if it doesn't. So no, I wouldn't fold them.

Also, for things like while and for, it is based on indentation of the next line (there is an explicit check for that (FoldingVisitor#createFoldingRegionForStatement) - why???).

IIRC, it is done like that to consider "long(ish)" comments in the line of the closing bracket. But look (emphasis on the highlighted text):

image
image

@danthe1st
Copy link
Contributor Author

danthe1st commented Nov 9, 2025

IIRC, it is done like that to consider "long(ish)" comments in the line of the closing bracket. But look (emphasis on the highlighted text):

I am not sure I really understand that reasoning given that it only seems to change the line with the } for me which seems completely useless (and I have no idea what the expected behavior is supposed to be).

Also, it seems like in cases like that, the if disappears so I am not quite sure what I am supposed to do with that code:

while(i==2) {
    // ...
} if(i==2) {
		// ...
	}

@danthe1st danthe1st changed the title Add preference to include last line during folding Include last line during folding Nov 9, 2025
@danthe1st danthe1st force-pushed the include-last-line branch 2 times, most recently from d64b430 to b34833d Compare December 13, 2025 15:39
@eclipse-jdt-bot
Copy link
Contributor

eclipse-jdt-bot commented Dec 13, 2025

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

org.eclipse.jdt.text.tests/META-INF/MANIFEST.MF
org.eclipse.jdt.text.tests/pom.xml

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch
From 7f84d5e279ba91238596c3de74d07d81b8b78976 Mon Sep 17 00:00:00 2001
From: Eclipse JDT Bot <jdt-bot@eclipse.org>
Date: Sat, 13 Dec 2025 19:34:38 +0000
Subject: [PATCH] Version bump(s) for 4.39 stream


diff --git a/org.eclipse.jdt.text.tests/META-INF/MANIFEST.MF b/org.eclipse.jdt.text.tests/META-INF/MANIFEST.MF
index a74d6ce398..52b3eb4d86 100644
--- a/org.eclipse.jdt.text.tests/META-INF/MANIFEST.MF
+++ b/org.eclipse.jdt.text.tests/META-INF/MANIFEST.MF
@@ -3,7 +3,7 @@ Automatic-Module-Name: org.eclipse.jdt.text.tests
 Bundle-ManifestVersion: 2
 Bundle-Name: %Plugin.name
 Bundle-SymbolicName: org.eclipse.jdt.text.tests;singleton:=true
-Bundle-Version: 3.15.300.qualifier
+Bundle-Version: 3.15.400.qualifier
 Bundle-Activator: org.eclipse.jdt.text.tests.JdtTextTestPlugin
 Bundle-ActivationPolicy: lazy
 Bundle-Vendor: %Plugin.providerName
diff --git a/org.eclipse.jdt.text.tests/pom.xml b/org.eclipse.jdt.text.tests/pom.xml
index 909680b589..2c8b1d2a50 100644
--- a/org.eclipse.jdt.text.tests/pom.xml
+++ b/org.eclipse.jdt.text.tests/pom.xml
@@ -20,7 +20,7 @@
   </parent>
   <groupId>org.eclipse.jdt</groupId>
   <artifactId>org.eclipse.jdt.text.tests</artifactId>
-  <version>3.15.300-SNAPSHOT</version>
+  <version>3.15.400-SNAPSHOT</version>
   <packaging>eclipse-test-plugin</packaging>
 
   <properties>
-- 
2.52.0

Further information are available in Common Build Issues - Missing version increments.

@danthe1st danthe1st marked this pull request as ready for review December 13, 2025 17:10
@danthe1st
Copy link
Contributor Author

danthe1st commented Dec 13, 2025

@fedejeanne Sorry, I didn't have the time and motivation to work on this until now.

I now changed it to just not affect control structures (i.e. the things added by extended folding, e.g. if). If there's a good way to implement this for control structures as well (and someone has an idea of when the last line should be included and when it shouldn't), that can be added as well but I honestly don't know what a good implementation of that would be.

The tests should document the effects of my changes. The changes related to custom folding regions were because the changes broke what was added (as noticed by the tests) in #2282 (and that's what I wasn't able to fix last time).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"old" folding cannot expand annotated enum constants with more elements Folding misbehave since 2025-03

3 participants