custom folding regions

[danthe1st]

Relevant issues: https://bugs.eclipse.org/bugs/show_bug.cgi?id=173796 (and there are also similar ones like https://bugs.eclipse.org/bugs/show_bug.cgi?id=258965 for CDT).

There is also a Stack Overflow post asking for how to do that with quite a few upvotes so I'd say it seems like many people are interested in such a feature.

What it does

This PR allows to add custom folding regions using comments.

I added new options to the preferences in Java > Editor > Folding (not that I changed this preference page to use Groups):

If a comment containing the start region and a comment containing the end region are present in the same source file, a folding region is added allowing to collapse that section.

It would be possible to change the defaults from region/endregion to #region/#endregion or similar. I have decided for // region, // endregion as that seems less arbitrary. I don't really have an opinion about the best choice of defaults here.

This functionality is added both to the normal folding and the new folding based on the AST introduced in #1562. The folding using the AST uses existing folding annotations to compute the new ones.

This works even for elements of different levels This is intentionally restricted since someone folding things from different elements is likely a mistake of the user (they probably don't want to fold something in the bodies of different methods or similar) as discussed in a devcall.

This PR also allows configuring folding per-project as requested in this comment. This has been moved to #1979

How to test

• Create a Java source file
• Create a comment starting with the text region
• Create a comment starting with the text endregion after the region comment (but within the same block (e.g. method/class))
• Test similar things with the source file
• After editing the corresponding changes in Java > Editor > Folding, it is necessary to reopen the source file. the changes are automatically applied to open editors (and will also be used when opening a new editor).
  • It is also possible to change these preferences in the project properties. This also requires reopening the source file.

Author checklist

• I have thoroughly tested my changes
  • performance testing: custom folding regions #1825 (comment)
• The change is following the coding conventions
• I have signed the Eclipse Contributor Agreement (ECA)

[HannesWell]

@fedejeanne in the dev-call on Thursday you mentioned that you could have a look at this, does this still stand?
Eventueally we need a JDT committer to submit this anyways.

[danthe1st]

@jukzi Sorry to ping you but would you mind taking a look at my PR and giving feedback?

[jukzi]

At a rough view the PR's is a legit feature request. The code looks simple enough to be reviewed and tested. There are no obvious flaws. Such a contribution should come with a Junit Test, that tests both the added API and the effect of the change. I am sure that similar tests already exists but i don't know where. You may look in the git history of the changed files or the call hierarchy to find related tests.
Since i am personally not interested in this particular change i can not offer much time for review/testing/adding junit. If someone else (@fedejeanne ?) likes to help with that i can offer to merge once necessary steps done.

[fedejeanne]

Hi. It's on my list but I am currently focusing on issues regarding the Edge browser, which have top priority until the end of the year.
I haven't forgotten about you @danthe1st though, I am just swamped 😓

[fedejeanne]

I did a quick review and made some comments regarding coding style.

I also tested a bit and this is what I found:

• It doesn't work if I define a region completely inside the public static void main(String... args) method but it does work on other methods like public boolean equals(Object obj)
• I can define regions if the comment starts with the custom text e.g. // #region2, if that is intended then the text in the preferences should say so. I would add some extra label explaining how custom regions work, right before the 2 check-boxes
• If I fold the // #region2 in this snippet, the System.out.println() is folded too:

@Override
public boolean equals(Object obj) {
	// #region1
	// #region2
	if (this == obj)
		return true;
	if (obj == null || getClass() != obj.getClass())
		return false;
	Cat cat = (Cat) obj;
	// #endregion2
	System.out.println();
	return size == cat.size;
	// #endregion1
}

• Moving regions around works without reopening the source file but creating regions doesn't. I would very much like it to work too, it doesn't feel natural to have to close the editor to add regions.

Your questions

Custom folding regions are disabled if either of the custom texts/preferences are empty or if one contains the other. Should I add some information about that to the preferences? Would it be better to disable it by default (currently, it defaults to #region/#endregion)?

Some info about it is supposed to work is good but I'd keep it short, 2-3 lines in a label. If it gets too long you could consider adding some help page like the Readme example does.
I'd disable (gray out) the group "Custom folding regions" in the preferences if the checkbox "Custom folding regions" is not selected.

I haven't written any tests so far. Are there existing tests for folding in JDT-UI? What about performance tests (my implementation requires scanning more parts of the source file which could affect performance)?

To test performance you'll have to do it manually. Alter this generator (credits to Andrey Loskutov) and let it generate several internal classes with custom folding regions: GenerateJava.zip. Then sample with VisualVM and share the results.

I don't know of any functional tests, maybe someone else does?

Regarding performance, I used String#contains to check whether comments contain certain the region begin/end tokens (e.g. #region/#endregion) which requires creating a String for each comment every time the file is edited. Should I change that to use the char[] returned by getCurrentTokenSource() or similar?

You'll have to sample with VisualVM and see if this is a performance bottle-neck.

I have bumped the minor version of org.eclipse.jdt.ui due to adding new elements in PreferenceConstants. Is this ok that way?

It's fine and this commit did the same so you're safe: d0563dc. Such bumps are no longer necessary since they will be done automatically by GH, as was the case for the mentioned commit.

I guess this should be added to the "New and noteworthy" list (assuming this change gets accepted and merged). If so, should I add it and is this the right place?

Correct, but let's cross that bridge when we get there :-) . Usually a committer adds the "noteworthy" tag to your PR / Issue right after it was merged/solved and that's your queue to do the PR for the N&N entry.

[danthe1st]

It doesn't work if I define a region completely inside the public static void main(String... args) method but it does work on other methods like public boolean equals(Object obj)

Should be fixed, this was because of it being the only (or last) method.

I can define regions if the comment starts with the custom text e.g. // #region2

It should work whenever a comment includes the text #region so it could be // Hello #region World or anything like that. If you would (intuitively) expect something else, I'd be happy to hear that and possibly change my implementation. Currently, it treats all comments the same way but I'm open to change that if other people prefer it differently.

Moving regions around works without reopening the source file but creating regions doesn't. I would very much like it to work too, it doesn't feel natural to have to close the editor to add regions.

Interestingly, that worked for my by just adding a comment like // #region or whatever. What doesn't work is it being automatically updated when changing the preferences related to custom folding regions.
Maybe you wrote the #region commend but not the #endregion comment?

I'd disable (gray out) the group "Custom folding regions" in the preferences if the checkbox "Custom folding regions" is not selected.

The checkbox is about initially folding/collapsing these elements. If the checkbox is disabled, the elements should still be folded but not collapsed by default.

I don't know of any functional tests, maybe someone else does?

I didn't find any existing tests related to folding so I'll try making my own tests for custom folding regions in a new directory org.eclipse.jdt.ui.tests/ui/org/eclipse/jdt/ui/tests/folding (I currently don't have a good way to access the folding regions/annotations added to the editor and I don't want to add API just for tests but I might find a good way to do it at some point ()). If anyone still finds tests after that, it shouldn't be hard to move them (once I wrote the tests).

My current approach for automated tests is subclassing DefaultJavaFoldingStructureProvider and overriding computeFoldingStructure and getProjectionRanges to access the created IRegion[]s.

---

Anyway, do you have any opinions on the defaults for the #region/#endregion marker texts? Do you think using a different text would be better? I think one alternatively I have seen in other IDEs was // region where it must be at the start of the comment but I felt it being at any position was more flexible.

[danthe1st]

I might still rework that label in the future.

[BeckerWdf]

The texts: "Text marking start of custom folding region" looks a bit unnecessary too long for my
Why not simply saying "Text marking start of region" and "Text marking end of region"
due to the box around you don't need to repeat that it's a "custom folding region".

I am unsure if it's singular "region" or pluar "regions".

[danthe1st]

As discussed in today's devcall, I changed the PR to disallow regions between an element and it's child.

class X{
	// #region
	void a() {
		// #endregion this is not folded with the outer region because it is at a different level
		
		// #region However, this is folded because loops are not considered children elements and I don't think it's a good idea to parse all of that for folding
		for(int i=0; i<10;i++) {
			// #endregion
		}
	}
	// #endregion this will be folded with the outer region
}

Another suggestion during the devcall was to restrict this to comments starting with #region (or whatever is configured) instead of allowing it anywhere (// Hello #region World).
Since the token scanner seems to treat the whole comment as a token (including the prefix like //, /*, /** or ///), I decided to not change this for now. If it is wanted, I can implement something stripping away the prefix of the comment token (skipping up to two slashes/stars and any whitespaces at the beginning) but I am unsure whether this is actually the preferred way of doing this as this might be a maintainance burden if new comment types (or similar) are added to Java.

[danthe1st]

Preliminary profiling results:

I ran the generator mentioned by @fedejeanne with 12_000 inner classes (the default) and added a custom region both outside and inside each of these classes.

I loaded the class in Eclipse, started CPU sampling with VisualVM, added some newlines at the top, bottom and in the middle of the file. I also added a single-line comment in the middle of the file.
This is what VisualVM shows:

Similar with CPU profiling:

As it seems, the custom folding logic does have an impact on performance.

Loading all the folding annotation takes a bit of time in general (especially with the profiler active) but it does not block the UI.

CPU profiling with similar operations with custom folding regions disabled (loading the folding regions still takes time with in this case):

It seems like the biggest issue is that custom folding regions require the scanner to process more tokens (it needs to actually go over method bodies etc which is otherwise not done for methods without any children). I could get rid of the scanner.getCurrentTokenSource() call in every comment but I don't think it would change much here.

I don't know whether this is a problem. The generated file has 156 005 lines. If the performance is a problem, we can disable this feature by default.

[fedejeanne]

@danthe1st can you provide:

1. The simplified version of the generated file (generate it with 1 inner class)
2. Sampling/performance analysis for the same scenarios on master (for comparison)?

Keep in mind that generating 12.000 inner classes is an extreme case so performance will always degrade, the question is how much.

[akurtakov]

As a reference for people interested in similar functionality for other editors - Generic editor provides extension point for that https://github.com/eclipse-platform/eclipse.platform.ui/blob/master/bundles/org.eclipse.ui.genericeditor/schema/foldingReconcilers.exsd

[danthe1st]

profiling with merge base (not current master but the master I based my PR on):

sampling:

Generated code with one inner class:

package a.generated;

public class TestClass {

    // #region outside
    public static class InnerClass0 {
        // #region inside
        public int x = 42; // no error
        
        public void helloWorld() {
                System.out.println("Hello World!"); // no error
                System.out.println("In class " + 0 + ", x=" + x); // expected error
        }
        // #endregion inside
    } // end of InnerClass0
    // #endregion

}

(I changed the code to not have any compilation errors - some regions may be missing in that case (probably because of the token scanner behaving differently))

Notably, this doesn't test long comments (the current implementation converts all comment tokens to a String on which it performs the contains() check. As mentioned above, I could change this to only check the beginning of the comment (and use the char[] provided by scanner.getSource() or similar) to only check the first characters (filtering out things like //, ///, /*, /**). If wanted, I can also make a comparison with that.

[danthe1st]

Other experiment with 6_000 inner classes and each class having a multiline comment with 20 lines each containing the character a 100 times:

// #region outside
    /*
    * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    */
    public static class InnerClass0 {
        // #region inside
        public int x = 42; // no error
        
        public void helloWorld() {
                System.out.println("Hello World!"); // no error
                System.out.println("In class " + 0 + ", x=" + x); // expected error
        }
        // #endregion inside
    } // end of InnerClass0
    // #endregion

Custom regions disabled, sampling:

Custom regions disabled, profiling (I had to disable automated collapsing of normal/header comments to be able to do this without Eclipse not responding):

Custom regions enabled, sampling:

Custom regions enabled, profiling (automated collapsing of comments enabled as well):

[danthe1st]

I realized that I can use the information of what the current token is from the scanner to know where I need to start looking for characters (so I don't need to check for things like //). With this change, it is now required that region markers (#region/#endregion) are at the start of the comment so // Hello #region World is now prevented as discussed in the devcall.
I used this chance to change the implementation to use char[]s and not process the entire comment.

Sampling with that change and custom folding regions enabled:

Profiling:

Note that all VisualVM screenshots I have included so far are about modifying source files after opening them and not about initially loading the source files (this isn't included in any sampling/profiling I've done until now).

[danthe1st]

EDIT: This issue is fixed now.

I noticed a bug in my implementation where the following is not detected:

class Z{
	void a() {
		// #region this is not detected because there is an inner class (considered as a child) in the same scope and there are statements/non-comment tokens between the comment with the region marker and the inner class
		int x;
		
		// #endregion this marker can be detected without issues
		class Inner{
			
		}
	}
}

This seems to only occur with local classes (declared within methods). The "old" folding implementation (master) generally ignores multi-line comments written in positions like that (it only folds comments before classes/methods/etc).

I plan to try fixing it but doing so might require over scanning bigger parts of the source file.

[BeckerWdf]

I might still rework that label in the future.

I am not a big fan of big explanatory text directly in the preference page. Can't we simply put this into the help document related to that preference page?

[BeckerWdf]

Thanks for the text shortening.

[danthe1st]

I am not a big fan of big explanatory text directly in the preference page. Can't we simply put this into the help document related to that preference page?

I guess I can remove the label and write a PR changing https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/blob/master/eclipse.platform.common/bundles/org.eclipse.jdt.doc.user/reference/preferences/java/editor/ref-preferences-folding.htm once this is done.

[jjohnstn]

The strings above are for the description of the text fields for the region markers (not the text inside the fields).

[danthe1st]

I think that having the text fields in the top group right by the Custom folding checkbox is clearer.

Is there any reason for this? The top group is for initially folding elements so I don't think adding extra configuration for custom folding regions there makes sense. The only thing these checkboxes are changing is whether these entries are collapsed or expanded by default. The name EDITOR_FOLDING_CUSTOM_REGIONS should probably be EDITOR_FOLDING_COLLAPSE_CUSTOM_REGIONS or similar (I went with a name corresponding to the existing similar names).

I was thinking something along the lines of the following (Note that there are two different checkboxes in this screenshot):

In case you want to see a diff of changing that (I didn't commit it since I'd like to read about your opinion first):

diff --git a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/folding/DefaultJavaFoldingPreferenceBlock.java b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/folding/DefaultJavaFoldingPreferenceBlock.java
index f7c1c34da0..d55bdfff65 100644
--- a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/folding/DefaultJavaFoldingPreferenceBlock.java
+++ b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/folding/DefaultJavaFoldingPreferenceBlock.java
@@ -84,7 +84,8 @@ public class DefaultJavaFoldingPreferenceBlock implements IJavaFoldingPreference
 		overlayKeys.add(new OverlayPreferenceStore.OverlayKey(OverlayPreferenceStore.BOOLEAN, PreferenceConstants.EDITOR_FOLDING_IMPORTS));
 		overlayKeys.add(new OverlayPreferenceStore.OverlayKey(OverlayPreferenceStore.BOOLEAN, PreferenceConstants.EDITOR_FOLDING_HEADERS));
 		overlayKeys.add(new OverlayPreferenceStore.OverlayKey(OverlayPreferenceStore.BOOLEAN, PreferenceConstants.EDITOR_NEW_FOLDING_ENABLED));
-		overlayKeys.add(new OverlayPreferenceStore.OverlayKey(OverlayPreferenceStore.BOOLEAN, PreferenceConstants.EDITOR_FOLDING_CUSTOM_REGIONS));
+		overlayKeys.add(new OverlayPreferenceStore.OverlayKey(OverlayPreferenceStore.BOOLEAN, PreferenceConstants.EDITOR_FOLDING_COLLAPSE_CUSTOM_REGIONS));
+		overlayKeys.add(new OverlayPreferenceStore.OverlayKey(OverlayPreferenceStore.BOOLEAN, PreferenceConstants.EDITOR_FOLDING_CUSTOM_REGIONS_ENABLED));
 		overlayKeys.add(new OverlayPreferenceStore.OverlayKey(OverlayPreferenceStore.STRING, PreferenceConstants.EDITOR_FOLDING_CUSTOM_REGION_START));
 		overlayKeys.add(new OverlayPreferenceStore.OverlayKey(OverlayPreferenceStore.STRING, PreferenceConstants.EDITOR_FOLDING_CUSTOM_REGION_END));
 		return overlayKeys.toArray(new OverlayKey[overlayKeys.size()]);
@@ -117,7 +118,7 @@ public class DefaultJavaFoldingPreferenceBlock implements IJavaFoldingPreference
 		addCheckBox(initialFoldingGroup, FoldingMessages.DefaultJavaFoldingPreferenceBlock_innerTypes, PreferenceConstants.EDITOR_FOLDING_INNERTYPES, 0);
 		addCheckBox(initialFoldingGroup, FoldingMessages.DefaultJavaFoldingPreferenceBlock_methods, PreferenceConstants.EDITOR_FOLDING_METHODS, 0);
 		addCheckBox(initialFoldingGroup, FoldingMessages.DefaultJavaFoldingPreferenceBlock_imports, PreferenceConstants.EDITOR_FOLDING_IMPORTS, 0);
-		addCheckBox(initialFoldingGroup, FoldingMessages.DefaultJavaFoldingPreferenceBlock_customRegions, PreferenceConstants.EDITOR_FOLDING_CUSTOM_REGIONS, 0);
+		addCheckBox(initialFoldingGroup, FoldingMessages.DefaultJavaFoldingPreferenceBlock_customRegions, PreferenceConstants.EDITOR_FOLDING_CUSTOM_REGIONS_ENABLED, 0);
 
 		Group extendedFoldingGroup= new Group(outer, SWT.NONE);
 		GridLayout extendedFoldingLayout= new GridLayout(1, false);
@@ -139,19 +140,24 @@ public class DefaultJavaFoldingPreferenceBlock implements IJavaFoldingPreference
 		GridLayout customRegionLayout= new GridLayout(2, false);
 		customRegionGroup.setLayout(customRegionLayout);
 		customRegionGroup.setText(FoldingMessages.DefaultJavaFoldingPreferenceBlock_custom_region_title);
-		addStringInput(customRegionGroup, FoldingMessages.DefaultJavaFoldingPreferenceBlock_CustomRegionStart, PreferenceConstants.EDITOR_FOLDING_CUSTOM_REGION_START);
-		addStringInput(customRegionGroup, FoldingMessages.DefaultJavaFoldingPreferenceBlock_CustomRegionEnd, PreferenceConstants.EDITOR_FOLDING_CUSTOM_REGION_END);
+
+		addCheckBox(customRegionGroup, FoldingMessages.DefaultJavaFoldingPreferenceBlock_customRegionsEnabled, PreferenceConstants.EDITOR_FOLDING_CUSTOM_REGIONS_ENABLED, 0, 2);
+		addStringInput(customRegionGroup, FoldingMessages.DefaultJavaFoldingPreferenceBlock_customRegionStart, PreferenceConstants.EDITOR_FOLDING_CUSTOM_REGION_START);
+		addStringInput(customRegionGroup, FoldingMessages.defaultJavaFoldingPreferenceBlock_customRegionEnd, PreferenceConstants.EDITOR_FOLDING_CUSTOM_REGION_END);
 
 		return outer;
 	}
 
 	private Button addCheckBox(Composite parent, String label, String key, int indentation) {
+		return addCheckBox(parent, label, key, indentation, 1);
+	}
+	private Button addCheckBox(Composite parent, String label, String key, int indentation, int horizontalSpan) {
 		Button checkBox= new Button(parent, SWT.CHECK);
 		checkBox.setText(label);
 
 		GridData gd= new GridData(GridData.HORIZONTAL_ALIGN_BEGINNING);
 		gd.horizontalIndent= indentation;
-		gd.horizontalSpan= 1;
+		gd.horizontalSpan= horizontalSpan;
 		gd.grabExcessVerticalSpace= false;
 		checkBox.setLayoutData(gd);
 		checkBox.addSelectionListener(fCheckBoxListener);
diff --git a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/folding/FoldingMessages.java b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/folding/FoldingMessages.java
index a3c7fc254a..adf6b75056 100644
--- a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/folding/FoldingMessages.java
+++ b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/folding/FoldingMessages.java
@@ -36,8 +36,9 @@ final class FoldingMessages extends NLS {
 	public static String DefaultJavaFoldingPreferenceBlock_customRegions;
 
 	public static String DefaultJavaFoldingPreferenceBlock_custom_region_title;
-	public static String DefaultJavaFoldingPreferenceBlock_CustomRegionStart;
-	public static String DefaultJavaFoldingPreferenceBlock_CustomRegionEnd;
+	public static String DefaultJavaFoldingPreferenceBlock_customRegionsEnabled;
+	public static String DefaultJavaFoldingPreferenceBlock_customRegionStart;
+	public static String defaultJavaFoldingPreferenceBlock_customRegionEnd;
 	public static String EmptyJavaFoldingPreferenceBlock_emptyCaption;
 	public static String JavaFoldingStructureProviderRegistry_warning_providerNotFound_resetToDefault;
 	public static String DefaultJavaFoldingPreferenceBlock_New_Setting_Title;
diff --git a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/folding/FoldingMessages.properties b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/folding/FoldingMessages.properties
index b4ddfbbd9c..56a19bd5d6 100644
--- a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/folding/FoldingMessages.properties
+++ b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/text/folding/FoldingMessages.properties
@@ -22,8 +22,9 @@ DefaultJavaFoldingPreferenceBlock_headers= &Header Comments
 DefaultJavaFoldingPreferenceBlock_customRegions= Custom folding regions
 
 DefaultJavaFoldingPreferenceBlock_custom_region_title= Custom folding regions
-DefaultJavaFoldingPreferenceBlock_CustomRegionStart= Region start comment text
-DefaultJavaFoldingPreferenceBlock_CustomRegionEnd= Region end comment text
+DefaultJavaFoldingPreferenceBlock_customRegionsEnabled=Enable folding of custom regions
+DefaultJavaFoldingPreferenceBlock_customRegionStart=Region start comment text
+defaultJavaFoldingPreferenceBlock_customRegionEnd=Region end comment text
 
 DefaultJavaFoldingPreferenceBlock_New = &Activate feature
 DefaultJavaFoldingPreferenceBlock_New_Setting_Title = &Extended folding (Experimental)
diff --git a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/ui/PreferenceConstants.java b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/ui/PreferenceConstants.java
index e4f9ba6667..d7a495f086 100644
--- a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/ui/PreferenceConstants.java
+++ b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/ui/PreferenceConstants.java
@@ -3490,7 +3490,17 @@ public class PreferenceConstants {
 	 *
 	 * @since 3.34
 	 */
-	public static final String EDITOR_FOLDING_CUSTOM_REGIONS= "editor_folding_custom_regions"; //$NON-NLS-1$
+	public static final String EDITOR_FOLDING_COLLAPSE_CUSTOM_REGIONS= "editor_folding_default_custom_regions"; //$NON-NLS-1$
+
+	/**
+	 * A named preference that stores the value for enabling custom region folding for the default folding provider.
+	 * <p>
+	 * Value is of type <code>Boolean</code>.
+	 * </p>
+	 *
+	 * @since 3.34
+	 */
+	public static final String EDITOR_FOLDING_CUSTOM_REGIONS_ENABLED= "editor_folding_custom_regions"; //$NON-NLS-1$
 
 	/**
 	 * A named preference that stores the value for the start indicator of custom folding regions for the default folding provider.
@@ -4340,7 +4350,9 @@ public class PreferenceConstants {
 		store.setDefault(PreferenceConstants.EDITOR_FOLDING_METHODS, false);
 		store.setDefault(PreferenceConstants.EDITOR_FOLDING_IMPORTS, true);
 		store.setDefault(PreferenceConstants.EDITOR_FOLDING_HEADERS, true);
-		store.setDefault(PreferenceConstants.EDITOR_FOLDING_CUSTOM_REGIONS, false);
+		store.setDefault(PreferenceConstants.EDITOR_FOLDING_COLLAPSE_CUSTOM_REGIONS, false);
+
+		store.setDefault(PreferenceConstants.EDITOR_FOLDING_CUSTOM_REGIONS_ENABLED, true);
 		store.setDefault(PreferenceConstants.EDITOR_FOLDING_CUSTOM_REGION_START, "region"); //$NON-NLS-1$
 		store.setDefault(PreferenceConstants.EDITOR_FOLDING_CUSTOM_REGION_END, "endregion"); //$NON-NLS-1$
 
diff --git a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/ui/text/folding/DefaultJavaFoldingStructureProvider.java b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/ui/text/folding/DefaultJavaFoldingStructureProvider.java
index 451bbf41f4..94551fa1b4 100755
--- a/org.eclipse.jdt.ui/ui/org/eclipse/jdt/ui/text/folding/DefaultJavaFoldingStructureProvider.java
+++ b/org.eclipse.jdt.ui/ui/org/eclipse/jdt/ui/text/folding/DefaultJavaFoldingStructureProvider.java
@@ -1215,7 +1215,8 @@ public class DefaultJavaFoldingStructureProvider implements IJavaFoldingStructur
 		String customFoldingRegionEnd= store.getString(PreferenceConstants.EDITOR_FOLDING_CUSTOM_REGION_END);
 		fCustomFoldingRegionBegin=customFoldingRegionBegin.toCharArray();
 		fCustomFoldingRegionEnd=customFoldingRegionEnd.toCharArray();
-		fCustomFoldingRegionsEnabled = !customFoldingRegionBegin.isEmpty() && !customFoldingRegionEnd.isEmpty() &&
+		fCustomFoldingRegionsEnabled = store.getBoolean(PreferenceConstants.EDITOR_FOLDING_CUSTOM_REGIONS_ENABLED) &&
+				!customFoldingRegionBegin.isEmpty() && !customFoldingRegionEnd.isEmpty() &&
 				!customFoldingRegionBegin.startsWith(customFoldingRegionEnd) && !customFoldingRegionEnd.startsWith(customFoldingRegionBegin);
 		fNewFolding = store.getBoolean(PreferenceConstants.EDITOR_NEW_FOLDING_ENABLED);
 	}

I want the strings to read "Region start comment text" and "Region end comment text" which tells the user it is a comment.

Done

[jjohnstn]

I think that having the text fields in the top group right by the Custom folding checkbox is clearer.

Is there any reason for this? The top group is for initially folding elements so I don't think adding extra configuration for custom folding regions there makes sense. The only thing these checkboxes are changing is whether these entries are collapsed or expanded by default. The name EDITOR_FOLDING_CUSTOM_REGIONS should probably be EDITOR_FOLDING_COLLAPSE_CUSTOM_REGIONS or similar (I went with a name corresponding to the existing similar names).

It is much cleaner. - one less group in the dialog. The settings are only relevant to the custom folding checkbox so putting them there (indented) doesn't require the user to look elsewhere. I am not a fan of the added group for the extended folding and eventually this should be merged above unless the individual items are broken out.

[danthe1st]

Also note that currently, my tests use the current behavior from master which does not include the fixes from #2014 and #2015. Merging this PR will make #2014 and #2015 fail but fixing it should be very easy (they should just not be merged concurrently).

[danthe1st]

It is much cleaner. - one less group in the dialog. The settings are only relevant to the custom folding checkbox so putting them there (indented) doesn't require the user to look elsewhere. I am not a fan of the added group for the extended folding and eventually this should be merged above unless the individual items are broken out.

In that case, I guess it would be best to rename the existing group since the name "Initially fold these elements" would no longer be fitting (and custom folding regions also make sense to be used if that checkbox is disabled).

That being said, I could imagine not using groups for custom folding regions at all but the preferences I'm adding here for custom folding don't really belong in any of the existing groups if they aren't renamed.

When I originally implemented it, I went with groups since other preference screens do something similar (e.g. Java > Editor > Content Assist or Java > Editor > Typing).

[danthe1st]

Also, do you have any opinion on adding an additional checkbox for enabling/disabling folding as opposed to just letting users disabling it by clearing out the Region start comment text/Region end comment text?

[jjohnstn]

It is much cleaner. - one less group in the dialog. The settings are only relevant to the custom folding checkbox so putting them there (indented) doesn't require the user to look elsewhere. I am not a fan of the added group for the extended folding and eventually this should be merged above unless the individual items are broken out.

In that case, I guess it would be best to rename the existing group since the name "Initially fold these elements" would no longer be fitting (and custom folding regions also make sense to be used if that checkbox is disabled).

That being said, I could imagine not using groups for custom folding regions at all.

Not sure what you mean about the group name being inappropriate. Are you are referring to the fact that the custom folding is regenerated if the text fields change so not truly initially at startup? BTW: I merged those PRs you mentioned #2014 and #2015 as they were bug fixes that should have already been merged so thanks for the reminder about them.

[danthe1st]

Not sure what you mean about the group name being inappropriate.

The current preference screen looks like this:

I am assuming you are talking about the "Initially fold these elements" group. which contains a "Custom region folding" checkbox.

None of the checkboxes in that group enable or disable the addition of folding annotations at any point. The only difference these checkboxes make is whether these are collapsed by default or not (so they change the "default state" of the folding annotations).

If I enable all checkboxes in that group, it looks like this:

If I disable all of them, I get the following:

As you can see, the folding regions are added/processed either way. It's just that enabling these checkboxes collapses some parts when a file is opened. By default, this is the case for imports and header comments since once typically doesn't want to see those.

I then also suggested to add another checkbox that configures whether custom folding regions are enabled:

[danthe1st]

BTW: I merged those PRs you mentioned #2014 and #2015 as they were bug fixes that should have already been merged so thanks for the reminder about them.

I rebased my changes and adapted my tests to no longer use the special cases/behavior that should be fixed by these PRs.

[jjohnstn]

Ok, I wasn't thinking clearly, Yes, I see what you mean now. I think that adding the enablement button makes sense.

[danthe1st]

I think that adding the enablement button makes sense.

I added it.

[jjohnstn]

@danthe1st Works great. One more change is needed - the default for the enablement should be false. Any new feature should do this.

[danthe1st]

@danthe1st Works great. One more change is needed - the default for the enablement should be false. Any new feature should do this.

Done

[jjohnstn]

Thanks @danthe1st

[danthe1st]

Looking at #2019, I used the same approach for the tests introduced with this PR ensuring I only need one test class.

@fedejeanne Should I add you in the copyright notice? According to your GitHub profile, you work for Vector so I guess I would add "Vector Informatik GmbH" to the contributor list of the test class? Before doing that, I want to make sure that is correct.

That being said, I don't know whether the FoldingTestSuite class I introduced is necessary any more since there are now only two test classes again.

[jjohnstn]

Hi @danthe1st I will try and merge the patch as soon as 4.36 M1 opens up. RC1 is next week and RC2 the following week after that.

[fedejeanne]

Looking at #2019, I used the same approach for the tests introduced with this PR ensuring I only need one test class.

@fedejeanne Should I add you in the copyright notice?

No need, but thank you for asking :-)

[danthe1st]

I noticed that the line containing void test(){} would be folded in the following code:

package org.example.test;

public class Test {
	// region

	/* endregion */ void test(){}
}

I changed it so that the last line is not included if there's other text in that line.