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

Allow keeping indentation on empty lines when using autosave and trimming whitespace options #6300

Open
blueloveTH opened this issue Feb 15, 2023 · 17 comments

Comments

@blueloveTH
Copy link

blueloveTH commented Feb 15, 2023

Godot version

4.0 Beta RC2

System information

Windows 11, Nvidia GTX 3070, Vulkan

Issue description

In theory, the new caret should be put in the same indentation level as the previous line.
However, the indentation will randomly disappear each time you press Enter.

bug.mp4

Steps to reproduce

  1. Create a new project
  2. Create a node and add a builtin GDScript
  3. Move caret to the end of the _ready function
  4. Press Enter multiple times
  5. The TABs of the previous lines disappeared (randomly)

Minimal reproduction project

You need to create a new project.

@MewPurPur
Copy link

MewPurPur commented Feb 15, 2023

Weird, after a few minutes of trying I still can't replicate it. Might be system-specific somehow?

@blueloveTH

This comment was marked as outdated.

@dalexeev
Copy link
Member

Maybe it has something to do with this editor setting:

@blueloveTH
Copy link
Author

That's right.

image

If Autosave and TrimTrailingWhiteSpaceOnSave are both checked, when the script triggers autosave event, it removes the TABs.

I think this is an unexpected behavior. If autosave happens when you are just going to write the next line (press ENTER), and TABs disappear, it leads to a bad experience.

Related code here. The trimming process does not consider a TAB may be a valid indentation.

void CodeTextEditor::trim_trailing_whitespace() {
	bool trimed_whitespace = false;
	for (int i = 0; i < text_editor->get_line_count(); i++) {
		String line = text_editor->get_line(i);
		if (line.ends_with(" ") || line.ends_with("\t")) {
			if (!trimed_whitespace) {
				text_editor->begin_complex_operation();
				trimed_whitespace = true;
			}

			int end = 0;
			for (int j = line.length() - 1; j > -1; j--) {
				if (line[j] != ' ' && line[j] != '\t') {
					end = j + 1;
					break;
				}
			}
			text_editor->set_line(i, line.substr(0, end));
		}
	}

	if (trimed_whitespace) {
		text_editor->merge_overlapping_carets();
		text_editor->end_complex_operation();
		text_editor->queue_redraw();
	}
}

@dalexeev
Copy link
Member

The trimming process does not consider a TAB may be a valid indentation.

Unfortunately, few applications make a distinction between indentation whitespace characters and whitespace characters after code/comments.

# Comment._
          ^ This is an extra space.

func f():_
         ^ This is an extra space.


--->print(1)
--->
^^^^ This is a "nice tab".

--->print(1)
--->--->
    ^^^^ This is an extra tab.


____print(1)
____
^^^^ This is "nice spaces".

____print(1)
________
    ^^^^ This is extra spaces.

____print(1)
_____
    ^ This is an extra space.

@akien-mga
Copy link
Member

akien-mga commented Feb 16, 2023

Trimming tabs on lines without code is expecting, this is part of trailing whitespace.

Personally I do want tabs removed if they're not before actual code, for example:

func _ready():
    # Do stuff

    # Do more stuff

I do want the option to strip any whitespace on the empty line between the two comments in the same indentation scope.

But indeed combined with autosave it doesn't seem too useful. Maybe the trimming option should be ignored when autosave is enabled, or only applied when doing a manual save.

@akien-mga akien-mga changed the title Losing indentation randomly when enter a newline in script editor Losing indentation when using autosave and trimming whitespace options Feb 16, 2023
@akien-mga akien-mga changed the title Losing indentation when using autosave and trimming whitespace options Allow keeping indentation on empty lines when using autosave and trimming whitespace options Feb 16, 2023
@akien-mga akien-mga transferred this issue from godotengine/godot Feb 16, 2023
@akien-mga
Copy link
Member

akien-mga commented Feb 16, 2023

Reposting my comment from the closed PR:

Another option would be to make the trimming behavior configurable as an enum:

  • No trimming
  • Trim all trailing whitespace
  • Keep indentation on empty lines

This also needs to take into account indentation with spaces and not just with tabs, so the solution would be more complex than this PR in any case.

I would also suggest checking what other popular IDEs do, because we probably shouldn't go too far out of our way if it's too implement something most users wouldn't expect.

@blueloveTH
Copy link
Author

It makes sense!

@Mickeon
Copy link

Mickeon commented Feb 16, 2023

Other IDEs... Well, err, Visual Studio, trim the indentation of an empty line away. However, they do "fake" the indentation as you're moving down the lines. As in, the line is empty until you start writing on it. At which point, it becomes a line whose indentation is dependent on context. The caret is even displayed where it's expected to be, before anything is written.

It's something that the Godot Script Editor could benefit from...

@Calinou
Copy link
Member

Calinou commented Feb 16, 2023

This is how JetBrains IDEs handle the whole situation by default.

@blueloveTH
Copy link
Author

I suggest two ideas.

  1. Do not edit the current line when autosave is enabled. It will lead to unexpected caret move.
  2. When doing trimming, use a stack to analyze the indentation instead of remove all via brute force.

https://docs.python.org/3/reference/lexical_analysis.html#indentation

For example,

func foo(a, b):
  print(1)
__ # this is an indentation we should keep. So next time the user can put caret at the right pos
  print(2)
__ # this is an indentation we can remove

func bar():
  pass

@blueloveTH
Copy link
Author

Also VS's solution mentioned by @Mickeon is clever. If it is easy to impl, it could be a better solution.

@Calinou
Copy link
Member

Calinou commented Feb 18, 2023

  1. When doing trimming, use a stack to analyze the indentation instead of remove all via brute force.

Trailing whitespace is considered invalid by a lot of external tools (including Git), no matter where it's located.

@dalexeev
Copy link
Member

I think the problem is that autosave works like the manual save: it writes changes to the edited file, performing final text transformations (depending on the settings).

In most other text editors that I know, autosave does not write changes to the target file, but creates a meta file in the same folder (or somewhere else), and the current state is saved as is, without removing extra spaces, normalizing indentation, etc. If the editor detects this file upon restart, it suggests to restore the lost changes.

@AndreevAndrei
Copy link

AndreevAndrei commented May 17, 2023

Hi, I have fix for this:

Branch 3.5:
godot/editor/code_editor.cpp (line 945)

(for 4.0 master the similar, but fixed typo trimmed_whitespace)

void CodeTextEditor::trim_trailing_whitespace() {
	bool trimed_whitespace = false;
	String spaces_below = "";
	for (int i = text_editor->get_line_count() - 1; i >= 0; i--) {
		String line = text_editor->get_line(i);
		bool empty_line = false;
		if (line.ends_with(" ") || line.ends_with("\t") || line == "") {
			if (!trimed_whitespace) {
				text_editor->begin_complex_operation();
				trimed_whitespace = true;
			}

			int end = 0;
			for (int j = line.length() - 1; j > -1; j--) {
				if (line[j] != ' ' && line[j] != '\t') {
					end = j + 1;
					break;
				}
			}
			// Lines without content should get same spaces as the line below.
			if (end == 0) {
				text_editor->set_line(i, spaces_below);
				empty_line = true;
			}
			else {
				text_editor->set_line(i, line.substr(0, end));
			}
		}

		// If the string is not empty, it will come in handy for restoring the spaces of the lines above.
		if (!empty_line) {
			for (int k = 0; k < line.length(); k++) {
				if (line[k] != ' ' && line[k] != '\t') {
					spaces_below = line.substr(0, k);
					break;
				}
			}
		}
	}

	if (trimed_whitespace) {
		text_editor->end_complex_operation();
		text_editor->update();
	}
}

@SkanerSoft
Copy link

Hi, I have fix for this:

Branch 3.5: godot/editor/code_editor.cpp (line 945) (for 4.0 master the similar, but fixed typo trimmed_whitespace)

void CodeTextEditor::trim_trailing_whitespace() {
	bool trimed_whitespace = false;
	String spaces_below = "";
	for (int i = text_editor->get_line_count() - 1; i >= 0; i--) {
		String line = text_editor->get_line(i);
		bool empty_line = false;
		if (line.ends_with(" ") || line.ends_with("\t") || line == "") {
			if (!trimed_whitespace) {
				text_editor->begin_complex_operation();
				trimed_whitespace = true;
			}

			int end = 0;
			for (int j = line.length() - 1; j > -1; j--) {
				if (line[j] != ' ' && line[j] != '\t') {
					end = j + 1;
					break;
				}
			}
			// Lines without content should get same spaces as the line below.
			if (end == 0) {
				text_editor->set_line(i, spaces_below);
				empty_line = true;
			}
			else {
				text_editor->set_line(i, line.substr(0, end));
			}
		}

		// If the string is not empty, it will come in handy for restoring the spaces of the lines above.
		if (!empty_line) {
			for (int k = 0; k < line.length(); k++) {
				if (line[k] != ' ' && line[k] != '\t') {
					spaces_below = line.substr(0, k);
					break;
				}
			}
		}
	}

	if (trimed_whitespace) {
		text_editor->end_complex_operation();
		text_editor->update();
	}
}

it's work!

@AndreevAndrei
Copy link

I've send a pull request, but don't understand how to connect to current issue (what is godot-proposals?)

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

Successfully merging a pull request may close this issue.

8 participants