🐛 Do not modify excess whitespace when modifying indentation

[driskell]

This is a proposal for fixing #5642 and I'd welcome feedback on if this is the right approach or not?

The problem, is copying pasting PHPDoc code at an indented level, with hard tabs on, loses the asterisk alignment:

Root Cause

Seems inside Atom, in the case of encountering soft tabs in a hard tab file... it can return a floating point indentation level, such as 1.5 in this instance (tab+half-tab). When rebuilding the indentation (it calls setIndentationForBufferRow for each line), this calls @buildIndentString with 1.5 which gets converted to integer: 1. ALL leading whitespace is then replaced by this new indentation - so we lose the space that aligns the asterisk.

The tests seem to point to floating point indentation levels being allowed. Yet I can find nowhere where it is useful. Most places just Math.ceil(). And with hard tabs it doesn't even make sense.

This patch attempts to remove all the floating points (it leaves the Math.ceil calls - they're harmless and can be cleaned later if this turns out the right approach.). It then has a function for "modifying" whitespace to change its indentation level, while paying attention to straggling spacing like that in the PHPDoc comments in the image above. It also adds a couple API for handling indentation changes that can be used elsewhere.

Only thing I'm not too happy with is the overlap between my "setIndentLevelForRow" and the existing "setIndentationForBufferRow" - I'm sure mine follows correct naming convention but existing doesn't. It is the existing one that is called when pasting though.

[driskell]

Edited to try reduce my usual walls of text :(

[driskell]

Hi @50Wliu I raise this as a proposed fix for #5642 - thanks.

[winstliu]

@kevinsawicki @benogle @nathansobo?

[driskell]

Rebased onto master.

Please let me know if there is anything I can do to help with this.

[izuzak]

Recommendation: use is instead of ==. See this section of the contributing guide.

[izuzak]

@driskell Thanks for submitting this PR. 🙇 I don't review PRs normally, but wanted to give it a shot here -- a 🏫 experience for me. 😄

I think @nathansobo will have more 💭 and 💡, but just wanted to say that I think these changes make sense overall.

[driskell]

Thanks @izuzak ! Appreciate the feedback and review!

[izuzak]

I'm a bit surprised at how this method was counting tabs and spaces before -- just counting the number of spaces and tabs, regardless of where they're located. I think your approach in parseIndentation is much better. 👍

[driskell]

Yes it was surprising! I hope my method is better.

It also follows tab stops if there's spaces intermingled. I know mixed tab/space is REALLY bad anyway but it hopefully means indentLevelForLine returns nice correct numbers that match exactly the visual indentation we see, where as at moment it would be weird. For example "S\tS\t" would return 3 where visually it would actually be identical to "\t\t" which is 2 (assuming 2 space tab)

[driskell]

Again, thanks for the review @izuzak - I'll followed through and provided more info on the @modifyIndentLevelForRow logic.

[driskell]

Not sure who to check with but wondering where this sits in the pipeline, if at all?

[izuzak]

Sorry for the delay, @driskell. I believe @nathansobo wanted to look at this when he finds some time. 📟

[nathansobo]

Thanks for taking the time to do this. It served as a jumping off point for a different fix in #7563, which reduces some of the complexity that led to this issue in the first place. Please let me know if there's something you think I missed with that fix.