-
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
Fix folding of 2 consecutive methods #1993
base: master
Are you sure you want to change the base?
Conversation
org.eclipse.jdt.text.tests/src/org/eclipse/jdt/text/tests/folding/FoldingTestNew.java
Show resolved
Hide resolved
While you are at it, I noticed that local classes (defined in methods) are folded with the old folding but not with your new folding. class Outer {
void a() {
class Inner2{
}
}
} Is this intentional? Apart from that, I want to inform you that there is also the idea of using folding annotations for sticky scrolling. See #1851 (comment) and eclipse-platform/eclipse.platform.ui#2756 (comment) for details on that. I just want to make sure you know about that discussion. Given that (at least if this is done), it may even be worth considering to add a folding annotatation for |
org.eclipse.jdt.text.tests/src/org/eclipse/jdt/text/tests/folding/FoldingTest.java
Outdated
Show resolved
Hide resolved
Another change that I think is probably intentional (in contrast to the previous one) is that multiline comments are folded within methods: public class Test {
void a() {
/*
* This multiline comment is folded with the AST-based folding but not the old one.
*/
}
} To be honest, I would probably consider that to be a good thing. (These are part of one of the tests I'm using in my PR which is why I noticed that since I'm running mostly the same tests on both folding strategies.) I am pointing this out to ensure that all changes you made are intentional. |
This is intentional, I want that every comment can be folded. |
ce725e6
to
dffa505
Compare
Fixed and added a test case for it.
The plan is for outer to also receive a folding annotation. When I initially tried to implement this, I encountered some issues and decided to put it on hold at the time. |
If the second method starts in the same line the first one ends then there will still be 2 foldings. Fixes eclipse-jdt#1992
@fedejeanne @jjohnstn could you look over the code? |
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 haven't looked deep into the code but I'd like for you to please:
- Only solve 1 issue in this PR: the folding of 2 consecutive methods
- Create another PR to rename/move the tests and split testing the "old" folding and the "new" folding (if even necessary).
A hint for the other PR (the one about the tests): you don't need to create a new test class, you can parameterize the tests to use the "old" and/or the "new" folding: the deciding part is probably store.setValue(PreferenceConstants.EDITOR_NEW_FOLDING_ENABLED, x);
with x
being the parameter for the tests. With that, you can simply write all the tests, skip some if necessary (i.e. if the "old" folding doesn't work for a given structure) and adapt the assertions. That way you will probably avoid having duplicated test code (e.g. testinnerClassinMethod
) and you'll make sure that no one forgets to add tests for any of the foldings in the future since all the test methods are in the same class.
@@ -71,6 +72,7 @@ | |||
JavaElementPrefixPatternMatcherTest.class, | |||
CodeMiningTriggerTest.class, | |||
ParameterNamesCodeMiningTest.class, | |||
FoldingTestNew.class, |
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.
Hopefully, you will not need another class and you can parameterize FoldingTest
(see my comment in the review). In any case, the name FoldingNewTest
would have been better. But please let's move this discussion to another PR.
If the second method starts on the same line where the first one ends, there will still be two foldings. (I accidentally set includeLastLine to true a second time, which caused the issue).
Additionally, I have separated the tests: FoldingTest verifies the old behavior, while FoldingTestNew tests my feature (#1562).
Fixes #1992