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

Implement trim_final_newlines setting and functionality #87099

Merged
merged 1 commit into from
May 30, 2024

Conversation

bitwise-aiden
Copy link
Contributor

@bitwise-aiden bitwise-aiden commented Jan 12, 2024

Closes: godotengine/godot-proposals#8867

Currently in a draft state as I need to figure out the best method for collapsing carets that fall outside the removed area of text.

This PR implements the trim_final_newlines setting to reduce the newlines at the end of file to the expected 1 newline. This puts the built-in script editor on parity with other editors like Visual Studio Code and helps reduce stray lines making their way into user repos.

Implementation

I've opted to remove all bar the last empty lines from the file, checking simply for length 0. It intentionally runs after the trim_trailing_whitespace as to consume lines that only contain whitespace characters as well. In the case that trim_trailing_whitespace_on_save is not enabled it will then trim all lines back to the line with whitespace, leaving that untouched with a single newline following.

This relies on the functionality added in #86978 for caret and selection support.

@bitwise-aiden bitwise-aiden marked this pull request as ready for review January 13, 2024 19:08
@bitwise-aiden bitwise-aiden requested review from a team as code owners January 13, 2024 19:08
editor/code_editor.cpp Outdated Show resolved Hide resolved
@kitbdev
Copy link
Contributor

kitbdev commented Jan 16, 2024

Yeah, the selection code is more complex than it needs to be at the moment.
Try using TextEdit::set_selection_mode(TextEdit::SelectionMode::SELECTION_MODE_NONE, check_line, 0, c) when changing the selection, so that the caret's selecting_line gets set to a not invalid line.

Or if #86978 is merged first, you wouldn't have to worry about carets or selections at all, the remove_text() would handle it.

@bitwise-aiden
Copy link
Contributor Author

Or if #86978 is merged first, you wouldn't have to worry about carets or selections at all, the remove_text() would handle it.

That's awesome context @kitbdev! Thanks :D
I don't have a great deal of bandwidth to push this forward at the moment so I'll remove the caret editing part, test against your branch, and leave it until you've pushed.

@bitwise-aiden
Copy link
Contributor Author

@kitbdev I've just tested against this change, it all looks good, the errors have gone away and any caret issues seem to be resolved too. Thank you for taking all that on 🙌🏻

@bitwise-aiden bitwise-aiden force-pushed the ba-add-trim-newlines branch 3 times, most recently from b37d294 to f4e7e18 Compare January 24, 2024 00:42
@bitwise-aiden bitwise-aiden requested a review from a team as a code owner February 11, 2024 14:56
@AThousandShips AThousandShips removed the request for review from a team February 11, 2024 15:08
@akien-mga akien-mga changed the title Implement trim_final_newlines setting and functionality Implement trim_final_newlines setting and functionality Feb 13, 2024
@bitwise-aiden bitwise-aiden force-pushed the ba-add-trim-newlines branch from f4e7e18 to 822d769 Compare May 1, 2024 13:58
@bitwise-aiden
Copy link
Contributor Author

Since @kitbdev's PR has been merged #86978, this is now ready for final review.

I've rebased and tested the cases locally where this was errorring earlier (multicaret / multi line selection) and everything seems to be working well.

@bitwise-aiden
Copy link
Contributor Author

Updated with Kitbdev's last comment. Ready for review.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 May 30, 2024
@akien-mga akien-mga merged commit 838eb5a into godotengine:master May 30, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@bitwise-aiden bitwise-aiden deleted the ba-add-trim-newlines branch May 30, 2024 13:06
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.

Add an editor setting to trim final newlines when saving a script
4 participants