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 for Visual Bug in Code Folding #1939

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jakub-suliga
Copy link

@jakub-suliga jakub-suliga commented Jan 15, 2025

Fix folding of 2 consecutive methods

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

Fixes #1539

How to test

  1. Create a class with two or more methods. Ensure that the second method begins immediately after the first method ends. For example:
public class b {
	void a() {
		
	}void b() {
		
	}
}
  1. Fold the first method. The second method should remain visible.

Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test (you should create the class FoldingTest from #1562 in this PR)

Change the commit text to:

Fix folding of 2 consecutive methods

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

Fixes https://github.com/eclipse-jdt/eclipse.jdt.ui/issues/1539

The last line is important so GH recognizes the issue and closes it automatically when you PR is merged ;-)

@jakub-suliga jakub-suliga force-pushed the bug/folding branch 3 times, most recently from eba082c to 630f26f Compare January 17, 2025 05:56
Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code does what it should and the test is appreciated.

I only have some minor comments regarding code formatting.

Maybe a committer (@jukzi?) would like to approve this PR and let it run the tests right after you process my comments ?

@jakub-suliga jakub-suliga force-pushed the bug/folding branch 2 times, most recently from f1c28dd to bb317c4 Compare January 17, 2025 08:55
@jakub-suliga jakub-suliga requested a review from jukzi January 17, 2025 09:22
@jukzi
Copy link
Contributor

jukzi commented Jan 17, 2025

i am not familiar with this code, please ask someone else for review.

@fedejeanne
Copy link
Contributor

Maybe @noopur2507 / @carstenartur / @vogella ? Those are the only names I could recognize from the History view of this class.

The good thing is that this PR has a proper regression test and the issue is clearly defined so manual testing seems easy enough :-) .

@fedejeanne
Copy link
Contributor

I tested manually and this PR produces the intended result.

This is on master (broken ❌ )

folding_master


This one is after applying this PR (Fixed ✔️ )

folding_fix

@vogella
Copy link
Contributor

vogella commented Jan 17, 2025

I'm not a jdt committer

@jakub-suliga
Copy link
Author

@iloveeclipse could your review this too?

Copy link
Member

@iloveeclipse iloveeclipse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This introduces (at least one) regression.
The imports section folding lost last import, so if you have N imports, it should be folded by default to 1 (first) line, but it misses to fold last line now and we have first and last line shown. Also folding indicator (bracket line shown on mouse over) on the left side is shown for only N-1 lines.

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

Fixes eclipse-jdt#1539
@iloveeclipse
Copy link
Member

iloveeclipse commented Jan 17, 2025

I saw in a second PR (#1562) you had lot of folding tests added.

I believe what would be good if you could have a dedicated PR that adds FoldingTest alone (no production code changes) that provides basic folding tests for existing folding regions / functionality.

In two other PR's you could then add dedicated tests for new or fixed functionality.

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.

[Bug] [Folding mechanism] Method declaration on the Same Line as the Closing Bracket
7 participants