Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Recognize existing indentation when indenting close braces #8439

Merged
merged 5 commits into from
Jul 22, 2014
Merged

Recognize existing indentation when indenting close braces #8439

merged 5 commits into from
Jul 22, 2014

Conversation

mackenza
Copy link
Contributor

this PR addresses #8395 and is a 2nd attempt. I refactored my fix based on feedback from @peterflynn and @redmunds in #8426

// before indenting so that embedded whitespace such as indents are not
// orphaned to the right of the electric char being inserted
this.setCursorPos(cursor.line, this.document.getLine(cursor.line).length);
} else if (nonWS >= cursor.ch) {
Copy link
Member

Choose a reason for hiding this comment

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

This can just be plain else since that's the only other clause in the outer if

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this 2nd conditional to handle the situation where there is actual text on the line. This will remove the whitespace between the electric char and the first non whitespace. That behaviour is contrary to our conversation in the issue, but it seemed more consistent with the prior case (all whitespace). If this is not desired, then the else condition can be removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mackenza The 1st condition seems to work great!

Can you describe a concrete case for the 2nd condition where you think this is desirable? In the 1st condition, the desire is to never leave whitespace at the end of a line after non-ws chars. But in the 2nd condition, I can't think of any case where the user intent is clear enough that we'd want to try to be smart about ws between non-ws chars.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure I see a concrete use case, so you're probably right, we should probably just leave the whitespace alone... very socialist, and I am Canadian, so this meshes well with me ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think of it more as agile development: inspect and adapt. We can always update when we find a new annoyance :)

@dangoor dangoor changed the title fix for #8395 Recognize existing indentation when autoclosing braces Jul 21, 2014
@dangoor dangoor changed the title Recognize existing indentation when autoclosing braces Recognize existing indentation when indenting close braces Jul 21, 2014
@dangoor
Copy link
Contributor

dangoor commented Jul 21, 2014

Updated the title to be more descriptive. Feel free to change if I didn't capture the intent here well enough! Thanks for the PR

@redmunds
Copy link
Contributor

Looks good. Merging.

redmunds added a commit that referenced this pull request Jul 22, 2014
Recognize existing indentation when indenting close braces
@redmunds redmunds merged commit ae17456 into adobe:master Jul 22, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants