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

Add code region folding to CodeEdit #74843

Merged
merged 1 commit into from
Sep 12, 2023
Merged

Conversation

jmb462
Copy link
Contributor

@jmb462 jmb462 commented Mar 12, 2023

Here is an implementation of my code region folding proposal.
Closes godotengine/godot-proposals#3366

Region code folding

While developping my game, I feel unconfortable navigating in scripts that may be quite long and contains many methods.
I miss a way to organize section of code and navigate throw it more easily.

As discuss in the proposal, I know that when misused, code folding can be consider as antipattern by some devs but I think it will help for those who need it and have a good use of it. User still have the choice to use it or not. Positive reactions under the proposal convince me to open this PR.

How does it works

Code region are defined by the use of two common region tag (can be used out of the box in VSCode).

Opening tag is #region name_of_the_region
Closing tag is #endregion
To be language agnostic, as CodeEdit can be used for any language, the first symbol depends of the comment delimiters of CodeEdit. So # will be used in Godot code editor while developping with GDScript language.

Region creation

Region can be created manualy by writing opening and closing tag around the code that we want to include in this region.

image

I've also added a context menu on text selection that do the stuff for us. We just need to modify the default name with this way.

region

Edit : Keyboard shorcut Alt + R has been added to create region.

Region highlighting

When a region is folded, its line is highlighted and a special gutter is shown.
While unfolded, there is no highlight and classic folding gutter is shown.

image

As a colorblind, the default color of line background and gutter icon is maybe not the best choice.

I choose the existing folder icon for region gutter. Of course a special icon could be designed. I don't really have Inkscape skill. I found that folder icon was not a bad temporay choice.

Clicking on the gutter icon (folder) will unfold the region.

Region color highlighting can be customized in editor settings under text_editor/theme/highlighting/folded_code_region_color.

Unit tests

I've added some unit tests in CodeEdit to check region detection, creation and folding.

Methods overview panel (EDIT : THIS PART IS DELAYED AND WILL BE ADDED IN ANOTHER PR)

The ItemList of method overview has been replaced by a Tree.

Nothing seems changed if not using regions :

image

When a region is defined in the script, it will appeared as a root element in the tree with (again) a folder icon :

image

I choose to not synchronize the folding of region in the script and in the method overview. Because i think we could want to fold script without folding the corresponding methods in the overview.

Clicking on the region name in the overview send you at the corresponding line in the script and unfold the region.
The region is also unfolded if we click on a method in that region.

Filtering works on methods and region names.

If alphabetical sort is toggled on, the root elements of the overview (region and methods that are not in a region) are sorted at first, then, inside each region, methods are also sorted.

Script order :
image

Alphabetical order :
image

Video preview

2023-03-12.22-42-12.mp4

@seppoday
Copy link

seppoday commented Mar 13, 2023

Is there a possibility to add shortcut?

@jmb462
Copy link
Contributor Author

jmb462 commented Mar 13, 2023

Edit :

Alt + F is the normal folding shortcut.
Alt + R was free and has been added to create region.

@seppoday
Copy link

Is there a possibility to add shortcut?

For creating a region ? I didn't think about it at first, but yes, as the other options in context menu, it should be possible to add a keyboard shortcut for that. I will look at it.

(It should also be possible to add shortcut to quickly go to the next / previous region.)

I mean, dunno if it will be usefull for anyone. We have shortcut for creating comments, so I figured that it might be usefull to add shortcut also here. Some people like to basically only use keyboard.

@YuriSizov YuriSizov added this to the 4.x milestone Mar 13, 2023
@xchrix
Copy link

xchrix commented Apr 10, 2023

It's great for someone like me who has to write long scripts.

@jmb462
Copy link
Contributor Author

jmb462 commented Apr 10, 2023

Updated to use CodeEdit theme cache as it was conflicting with master since YuriSizov PR merge.

Copy link
Member

@Paulb23 Paulb23 left a comment

Choose a reason for hiding this comment

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

Reviewed /scene changes.

Not sure on making the editor side quite so advanced yet as I would like to refactor that first, might be worth splitting those changes out into a separate PRs.

scene/gui/code_edit.cpp Outdated Show resolved Hide resolved
scene/gui/code_edit.cpp Outdated Show resolved Hide resolved
scene/gui/code_edit.cpp Outdated Show resolved Hide resolved
scene/gui/code_edit.cpp Show resolved Hide resolved
scene/gui/code_edit.h Outdated Show resolved Hide resolved
scene/gui/code_edit.h Outdated Show resolved Hide resolved
scene/gui/code_edit.h Show resolved Hide resolved
scene/gui/code_edit.cpp Outdated Show resolved Hide resolved
@jmb462 jmb462 force-pushed the code_region branch 2 times, most recently from a0017f9 to e71455c Compare April 20, 2023 18:44
@jmb462 jmb462 marked this pull request as draft April 20, 2023 20:05
@jmb462 jmb462 force-pushed the code_region branch 6 times, most recently from 4ec92eb to cbb0ad3 Compare April 22, 2023 11:33
@jmb462
Copy link
Contributor Author

jmb462 commented Apr 22, 2023

@Paulb23 I've updated the PR thanks to your review.

1°) Methods overview has been reverted to ItemList instead of Tree. It will not show anymore the code region. I will propose a separate PR if this one is merged.

2°) Code region tag (start / end) are now property. They are only updated when comment delimiters are modified (added, removed or cleared)

3°) Code region methods have been grouped with code folding methods in the header files.

4°) opening / closing vocabulary has been replaced with start / end to match comment delimiters vocabulary.

5°) Each time comments delimiters are modified, the region tags are updated to select the first available delimiters that is line_only without end delimiter. For now, it doesn't support langage with no one line comment. As I said in an other comment, it's seems good enough because it seems that it's a common have of all languages.

6°) Disable create code region via shortcut if several carrets / selections are active to avoid unwanted behaviour. In master, right click on mouse while having multiple selection will unselected all, so we can already not create region by menu when multiple selections are actives.

7°) Now support nested regions. Now early returns false in fold_line() only if start region tag has no end tag with same level. Still want to early return in that case because we never want to fold a bad nested region.

8°) Updated unit tests with checks for good update of region tags when modifying comment delimiters and check correct behavior of nested region.

@jmb462 jmb462 marked this pull request as ready for review April 22, 2023 11:53
@MewPurPur
Copy link
Contributor

My updated SVGs are no longer translucent, just purr #fff

@jmb462
Copy link
Contributor Author

jmb462 commented Aug 30, 2023

@Calinou Checking with a color picker, it's the same color as the line highlight. Seems to be an optical illusion.

Should we choose another color (or boost icon opacity to counteract this feeling ?
image
image

@Calinou
Copy link
Member

Calinou commented Aug 30, 2023

Should we choose another color (or boost icon opacity to counteract this feeling ?

Yes, I think we can increase the icon opacity.

@jmb462
Copy link
Contributor Author

jmb462 commented Aug 30, 2023

Yes, I think we can increase the icon opacity.

Is it ok with a minimum alpha of 0.4 ?

image
image

@Calinou
Copy link
Member

Calinou commented Aug 30, 2023

Is it ok with a minimum alpha of 0.4 ?

Looks good 🙂

tests/scene/test_code_edit.h Outdated Show resolved Hide resolved
tests/scene/test_code_edit.h Outdated Show resolved Hide resolved
@YuriSizov
Copy link
Contributor

@Calinou Was the icon the only thing missing, or is there anything else you'd like to have addressed?

@Calinou
Copy link
Member

Calinou commented Sep 9, 2023

@Calinou Was the icon the only thing missing, or is there anything else you'd like to have addressed?

Everything in the PR looks good on my end now.

@jmb462
Copy link
Contributor Author

jmb462 commented Sep 10, 2023

New git conflict fixed (EditorStringNames singleton)

Comment on lines 128 to 129
String code_region_start_string = "";
String code_region_end_string = "";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
String code_region_start_string = "";
String code_region_end_string = "";
String code_region_start_string;
String code_region_end_string;

These already default to this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there harm in being explicit?

Copy link
Member

Choose a reason for hiding this comment

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

I mean not as such, but it's unnecessary, and we usually suggest it elsewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nearly all Strings are explicitly set to "" in this file, that's why I chose to keep the same style.
But ok, I'm doing the modification for my new Strings.

@akien-mga
Copy link
Member

Needs another rebase, and handling the minor style comment from @AThousandShips. Then this should be ready to merge :)

@jmb462
Copy link
Contributor Author

jmb462 commented Sep 11, 2023

Needs another rebase, and handling the minor style comment from @AThousandShips. Then this should be ready to merge :)

Done.

@akien-mga akien-mga merged commit 2c2ca3d into godotengine:master Sep 12, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@markofjohnson
Copy link

Great, thank you! Looking forward to the methods overview panel part down the road.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement custom folding regions in the script editor