Page MenuHome

Convert indentation whitespaces to user defined spaces or tabs
ClosedPublic

Authored by Bruno Boaventura Scholl (brunobbs) on Mar 14 2019, 1:31 PM.

Details

Summary

This patch fixes the problems with indentation via keyboard shortcut and with inconsistent indentation whitespaces after pasting from another source by converting all pasted indentation whitespaces into the character defined by the "use tabs as spaces" option.

Diff Detail

Repository
rB Blender

Event Timeline

Sybren A. Stüvel (sybren) requested changes to this revision.Mar 15 2019, 10:35 AM
Sybren A. Stüvel (sybren) added inline comments.
source/blender/editors/space_text/text_ops.c
70

This is not a reference but a pointer. I know C doesn't really have references, but since Blender also has a large body of code in C++ we ought to be careful in the terminology.

83

Those while statements can be replaced by for (i=0; (*buf)[i]; i++). Every iteration i++ is executed anyway, and both while statements start with i=0.

85

This if/else if construct is not very trivial and repeated twice. Maybe it's a good one to split it into a small function and call it three times?

107

This replaces a \t with tab_size spaces, which is incorrect. It should align the text with the next tab stop. " \t" should generally result in the same indentation as "\t" (unless that one space would push it to the next tab stop, in which case it's equivalent to "\t\t".

821

This boolean is only used once, so just change the condition below to if (text->flags & TXT_TABSTOSPACES == 0).

This revision now requires changes to proceed.Mar 15 2019, 10:35 AM

This doesn't fix the main bug in T60234: Shift+Tab only works with new text, which I think is that our indent/unindent operators should work with both tabs and spaces. But we can consider this as a change on its own.

Converting tabs to spaces automatically on paste makes some sense if you interpret paste as similar to typing in the code. When using the text editor for something else than Python scripting it might be unexpected though, perhaps it's an acceptable trade-off.

source/blender/editors/space_text/text_ops.c
833–835

I don't think it's right to convert all spaces to tabs when this option is off, probably it should do nothing then. When using tabs, you sometimes indent some part with spaces intentionally to ensure things align regardless of tab size.

source/blender/editors/space_text/text_ops.c
833–835

The buf_spaces_to_tabs function shouldn't change indentation; so given a tab size of 4 it should convert 7 spaces to 1 tab + 3 spaces.

Mixing tabs with spaces for indentation is invalid in Python, which brings us again to when to apply these transformations (always or only when editing Python code).

Bruno Boaventura Scholl (brunobbs) added inline comments.
source/blender/editors/space_text/text_ops.c
833–835

Given these two problems, when tabs_to_spaces == false, should I keep the pasted text original and let the user deal with it?
The user can still convert with the button.

off topic: Should I post the code as I mark the comments as done, or should I wait until every comment is solved to post?

off topic: Should I post the code as I mark the comments as done, or should I wait until every comment is solved to post?

Generally I mark them as 'done' as I do them (as to give me a reminder of what's left to do) but not submit, and submit just before I post an update to the diff.

Bruno Boaventura Scholl (brunobbs) marked 2 inline comments as done.

I have implemented the suggestions in code style and structure.
As converting from tabs could lead to unexpected behavior, I kept the text unmodified in this case.

source/blender/editors/space_text/text_ops.c
74

Rename last_state to r_last_state to indicate that it is used for returning a value.

131

The above if-block does not change line_start, so if (!line_start) should just be replaced with else.

Bruno Boaventura Scholl (brunobbs) marked 2 inline comments as done.

Changed what was suggested.

source/blender/editors/space_text/text_ops.c
812

Since buf_tabs_to_spaces is only called with true as the last argument, do we really need that parameter to be there?

Bruno Boaventura Scholl (brunobbs) marked an inline comment as done.

That flag free_src was there in case of reuse of the function, but I think there will not be many reuses, so removing it may be better.

Hi, just as a reminder of this DIff I am bumping this thread. Sorry if this is not allowed, but it has been a while since my last diff update.

source/blender/editors/space_text/text_ops.c
69

This comment seems to be outdated now. There is no line_start flag.

72

last_state should be renamed to r_last_state, and here also line_start doesn't exist, so the comment doesn't make much sense.

Bruno Boaventura Scholl (brunobbs) marked 2 inline comments as done.
Bruno Boaventura Scholl (brunobbs) set the repository for this revision to rB Blender.

Updated the function comment.

I'll commit this with some tweaks for code clarity.

Also fixing a missing reset of spaces_until_tab when starting a new line.

This revision was not accepted when it landed; it landed in state Needs Review.Thu, Apr 25, 7:35 PM
This revision was automatically updated to reflect the committed changes.