-
Notifications
You must be signed in to change notification settings - Fork 95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add custom folding regions #1825
base: master
Are you sure you want to change the base?
Conversation
0301e82
to
1828731
Compare
56d6c84
to
a8a21b5
Compare
@fedejeanne in the dev-call on Thursday you mentioned that you could have a look at this, does this still stand? |
@jukzi Sorry to ping you but would you mind taking a look at my PR and giving feedback? |
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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 likepublic 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, theSystem.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.
} | ||
case ITerminalSymbols.TokenNameCOMMENT_LINE: | ||
//$FALL-THROUGH$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd avoid falling through, it hurts readability.
I'd push the if (fCustomFoldingRegionsEnabled)
inside checkCustomFolding
(make it an if-!-return
) and call the method once for each case in this switch.
private String fCustomFoldingRegionBegin="#region"; //$NON-NLS-1$ | ||
private String fCustomFoldingRegionEnd="#endregion"; //$NON-NLS-1$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are overwritten later anyway.
private String fCustomFoldingRegionBegin="#region"; //$NON-NLS-1$ | |
private String fCustomFoldingRegionEnd="#endregion"; //$NON-NLS-1$ | |
private String fCustomFoldingRegionBegin; | |
private String fCustomFoldingRegionEnd; |
for(int token = scanner.getNextToken(); token != ITerminalSymbols.TokenNameEOF; token=scanner.getNextToken()) { | ||
if(isCommentToken(token)) { | ||
checkCustomFolding(ctx, regions, scanner, scanner.getCurrentTokenStartPosition(), regions.size() - 1); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated code.
Extract to method and call twice.
Should be fixed, this was because of it being the only (or last) method.
It should work whenever a comment includes the text
Interestingly, that worked for my by just adding a comment like
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 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 My current approach for automated tests is subclassing Anyway, do you have any opinions on the defaults for the |
The texts: "Text marking start of custom folding region" looks a bit unnecessary too long for my I am unsure if it's singular "region" or pluar "regions". |
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 |
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. 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 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. |
@danthe1st can you provide:
Keep in mind that generating 12.000 inner classes is an extreme case so performance will always degrade, the question is how much. |
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 |
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 ( I plan to try fixing it but doing so might require over scanning bigger parts of the source file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the text shortening.
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. |
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 useGroup
s):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.
This works even for elements of different levelsNotes / What I'm not sure about
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
)?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)?
Regarding performance, I used
String#contains
to check whether comments contain certain the region begin/end tokens (e.g.#region
/#endregion
) which requires creating aString
for each comment every time the file is edited. Should I change that to use thechar[]
returned bygetCurrentTokenSource()
or similar?I have bumped the minor version of
org.eclipse.jdt.ui
due to adding new elements inPreferenceConstants
. Is this ok that way?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?
How to test
#region
#endregion
Java
>Editor
>Folding
, it is necessary to reopen the source file.Author checklist