Skip to content
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 #2014

Merged
merged 1 commit into from
Feb 13, 2025

Conversation

jakub-suliga
Copy link
Contributor

@jakub-suliga jakub-suliga commented Feb 12, 2025

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).

Fixes #1992

Testing:

  1. Disabled new/experimental folding
  2. Fold a, now b should be visible
package org.example.test;

class X {
	/*
	 * a b
	 */
	void a() {

	} void b() {
	}
}

If the second method starts in the same line the first one ends then
there will still be 2 foldings.

Fixes eclipse-jdt#1992
@jakub-suliga
Copy link
Contributor Author

@fedejeanne @iloveeclipse @jjohnstn could someone of you look over the code

@fedejeanne
Copy link
Contributor

I tested the case described in #1992 and this PR makes no difference in the behavior. Can you add a test case to it and also explain how to test it manually?

@jakub-suliga
Copy link
Contributor Author

jakub-suliga commented Feb 12, 2025

I tested the case described in #1992 and this PR makes no difference in the behavior. Can you add a test case to it and also explain how to test it manually?

What happens when you fold the method? Because when I am testing it, everything works.
New:
image

Old:
image

I added how to test it and there is a test case for it, but currently it tests only when the new folding is activated. I am rewritting the test, so it tests both cases. There will be soon a PR for it.

@fedejeanne
Copy link
Contributor

Thank you @jakub-suliga, I was able to reproduce it and I can confirm that this PR fixes the bug.

I am rewritting the test, so it tests both cases. There will be soon a PR for it

Why not in this PR? If it the tests is improved so it can test this corner case then it belongs here :-)

@jakub-suliga
Copy link
Contributor Author

Why not in this PR? If it the tests is improved so it can test this corner case then it belongs here :-)

I am currently working on refactoring the test class, not just the one test case. Therefore it should be in a separate PR.

@fedejeanne
Copy link
Contributor

I am currently working on refactoring the test class, not just the one test case. Therefore it should be in a separate PR.

Finish the refactoring in another PR (merge it first if you want) and then add the test to this one. If you end up decoupling the fix from the test for that very same fix then commits can not be reverted without breaking tests, which is kind of confusing.

@danthe1st danthe1st mentioned this pull request Feb 13, 2025
3 tasks
@danthe1st
Copy link

Just a heads up:
Note that currently, the tests I added in #1825 use the current behavior from master which does not include this fix. Merging both PRs will make the build fail but fixing it should be very easy (so they should just not be merged concurrently).

@jjohnstn jjohnstn merged commit 6b4af87 into eclipse-jdt:master Feb 13, 2025
10 checks passed
@jjohnstn
Copy link
Contributor

Merged. Thanks.

fedejeanne added a commit to vi-eclipse/eclipse.jdt.ui that referenced this pull request Feb 14, 2025
For eclipse-jdt#2014. I modified
an existing test to also cover the issue that was fixed by that PR.

Contributes to eclipse-jdt#1992
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.

Code after Javadocs still folded "too much"
4 participants