Skip to content

Format on type deletes valid range #3084

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

Closed
wants to merge 2 commits into from
Closed

Format on type deletes valid range #3084

wants to merge 2 commits into from

Conversation

dbaeumer
Copy link
Member

@dbaeumer dbaeumer commented May 8, 2015

Type enter on a line and the result is totally unexpected.
File (Game.ts):
///
///
///
module Mankala {
export var NoSpace = -1;
export var homeSpaces = [[0,1,2, 3, 4, 5],
[7,8,9,10,11,12]];
export var firstHomeSpace = [0,7];
export var lastHomeSpace = [5,12];
export var capturedSpaces = [12,11,10,9,8,7,NoSpace,5,4,3,2,1,0,NoSpace];
export var NoScore = 31;
export var NoMove = -1;
}
Press enter after " [7,8,9,10,11,12]];"

The result is super weird, see screenshot:
capture

The problem is in

                        else if (indentPosition < 0) {
                            edits.push({
                                span: ts.createTextSpanFromBounds(position, position - indentPosition),
                                newText: ""
                            });

IndentPosition is negative so we delete characters form the current position on forward.

The pull request fixes this plus handles the fact that existing indent uses tabs. The problem here is that indentPosition counts the column. But on removal one column doesn't equal one character to remove since \t counts for n columns.

@@ -252,8 +255,15 @@ module ts {
newLine: _os.EOL,
useCaseSensitiveFileNames: useCaseSensitiveFileNames,
write(s: string): void {
var buffer = new Buffer(s, 'utf8');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this changing as part of a formatting change?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree this needs to be in a separate change.

Copy link
Contributor

Choose a reason for hiding this comment

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

agree

@mhegazy
Copy link
Contributor

mhegazy commented May 8, 2015

@vladima can you take a look?

export function generateIndentString(n: number, editorOptions: EditorOptions): string {
if (editorOptions.ConvertTabsToSpaces) {
return generateSpaces(n);
} else {
Copy link
Member

Choose a reason for hiding this comment

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

else on the next line

@mhegazy
Copy link
Contributor

mhegazy commented May 18, 2015

@dbaeumer what is the status of this change?

@mhegazy
Copy link
Contributor

mhegazy commented Jun 23, 2015

@dbaeumer looks like the indentation issue has been fixed. we can not get it to repro on latest VS or VS Code. for the buffer issue can you please resubmit?

@mhegazy mhegazy closed this Jun 23, 2015
@dbaeumer
Copy link
Member Author

OK. I tested it and the issue seems to be fixed in the underlying formatting code. However I still believe that there are issues in the code as soon as the correction code is hit.

                        else if (indentPosition < 0) {
                            edits.push({
                                span: ts.createTextSpanFromBounds(position, position - indentPosition),
                                newText: ""
                            });
                        }

This part of the code will delete text from the position to the right (indentPosition is negative and so position - indentPosition > position.

In addition the code doesn't handle the tabs setting correctly since it always generates spaces for extra indent. I opened #3681 for this.

@dbaeumer
Copy link
Member Author

And sorry for the writeSync issue. I opened a separate pull request here:

Node System host write - need to loop since writeSync might not write all bytes in one request #3682

@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants