annotate-snippets-rs icon indicating copy to clipboard operation
annotate-snippets-rs copied to clipboard

Fix origin position computation

Open KarelPeeters opened this issue 1 year ago • 1 comments

This PR fixes an issue in the computation of the position in the origin file. When a highlighted section starts at the beginning of the line, the origin position used to point to the end of the previous line, instead of the start of the current line. This resulted in outputs like this:

error: title
 --> origin.txt:2:4
  |
1 | aaa
2 | bbb
3 | ccc
  | ^^^ annotation
4 | ddd
  |

Here origin.txt:2:4 is wrong, it should be origin.txt:3:1, which actually matches the highlighted setting.

As a drive-by fix, 21645ad032dd9117d811db57a5740b6a6406f94a fixes EndLine, which was used incorrectly in multiple places that happened to cancel out.

KarelPeeters avatar Oct 13 '24 22:10 KarelPeeters

Ah, I missed those failing test cases, since they're disabled on windows: https://github.com/rust-lang/annotate-snippets-rs/blob/c84e388949f0e06f4529de060c195e12e6531fbd/tests/fixtures/main.rs#L9-L14 Is there any reason for that, maybe CRLF issues? They seem to behave as expected for me when I just remove that #[cfg(not(windows))] line!

The failing test are about cases that try to point one byte past the end of the line. Previously that was indeed possible, but only in very limited situations, eg. the highlighting only worked on EOF cases anyway. See https://github.com/rust-lang/annotate-snippets-rs/issues/108#issuecomment-2290855597 for vaguely relevant discussion about this.

I'm not sure what the best way to proceed here is. Is the "one past the end of the line" use case already adequately supported by just allowing users to point at the newline characters themselves? Ideally there would be some out-of-band way to signal this case, since only using byte indices for this is tricky.

KarelPeeters avatar Oct 13 '24 23:10 KarelPeeters

Is there any reason for that, maybe CRLF issues? They seem to behave as expected for me when I just remove that #[cfg(not(windows))] line!

I believe we disabled the test harness on Windows because there were issues with the test harness running on windows.

Muscraft avatar Oct 15 '24 21:10 Muscraft

I think I've addressed all of the comments, and I have rebased this PR on the current master. Let me know if I can do anything else to improve this PR!

KarelPeeters avatar Oct 20 '24 12:10 KarelPeeters