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

TextDocument.positionAt and offsetAt incorrectly include line endings #1285

Closed
mroch opened this issue Aug 8, 2023 · 3 comments
Closed

TextDocument.positionAt and offsetAt incorrectly include line endings #1285

mroch opened this issue Aug 8, 2023 · 3 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug
Milestone

Comments

@mroch
Copy link
Contributor

mroch commented Aug 8, 2023

The Position docs say that you can't specify a position between an \r and \n, or after a line ending:

* Positions are line end character agnostic. So you can not specify a position that
* denotes `\r|\n` or `\n|` where `|` represents the character offset.

And that if the character is too big it defaults to the line length:

* If the character value is greater than the line length it defaults back
* to the line length. This property is implementation specific.

But these cases do include the line endings:

const document = newDocument('A\nB\rC\r\nD');
assert.equal(document.offsetAt(Positions.create(0, 10)), 2);
assert.equal(document.offsetAt(Positions.create(1, 10)), 4);
assert.equal(document.offsetAt(Positions.create(2, 2)), 6); // between \r and \n
assert.equal(document.offsetAt(Positions.create(2, 3)), 7);
assert.equal(document.offsetAt(Positions.create(2, 10)), 7);
assert.equal(document.offsetAt(Positions.create(3, 10)), 8);

// this should be (2, 1), I think
assert.deepEqual(document.positionAt(6), Positions.create(2, 2)); // between \r and \n

I think the line length of each line is 1, so the invalid positions should be treated as (line, 1), and the correct offsets should be 1, 3, 5, 5, 5, 8 respectively

@dbaeumer
Copy link
Member

dbaeumer commented Aug 16, 2023

@aeschli since you are back from vacation can you please have a look since you originally wrote the code. There is also a PR with test cases.

@aeschli aeschli self-assigned this Aug 16, 2023
@dbaeumer dbaeumer added the bug Issue identified by VS Code Team member as probable bug label Dec 5, 2023
@dbaeumer dbaeumer added this to the Next milestone Dec 5, 2023
@dbaeumer
Copy link
Member

dbaeumer commented Jul 8, 2024

@aeschli could you please have a look at this.

@dbaeumer
Copy link
Member

dbaeumer commented Aug 5, 2024

PR got merged and released in new next version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue identified by VS Code Team member as probable bug
Projects
None yet
Development

No branches or pull requests

3 participants