Migrate VFS to Data.Text.Utf16.Rope.Mixed #436
Conversation
| -- | Given a virtual file, translate a 'CodePointPosition' in that file into a 'J.Position' in that file. | ||
| -- | ||
| -- Will return 'Nothing' if the requested position is out of bounds of the document. |
There was a problem hiding this comment.
What happens now? I'm guessing we get the "clamped" result. I think both behaviours are reasonable but I'd like it to be documented.
There was a problem hiding this comment.
Yes, it's clamped.
| @@ -313,12 +313,12 @@ vspSpec = do | |||
| positionToCodePointPosition vfile (J.Position 1 2) `shouldBe` Nothing | |||
| positionToCodePointPosition vfile (J.Position 1 3) `shouldBe` Just (CodePointPosition 1 2) | |||
| positionToCodePointPosition vfile (J.Position 1 4) `shouldBe` Just (CodePointPosition 1 3) | |||
| positionToCodePointPosition vfile (J.Position 1 5) `shouldBe` Just (CodePointPosition 1 4) | |||
| positionToCodePointPosition vfile (J.Position 1 5) `shouldBe` Just (CodePointPosition 2 0) | |||
There was a problem hiding this comment.
This one looks like an actual logic change 🤔 What happened here? Was the old implementation wrong?
There was a problem hiding this comment.
I tend to think that the previous implementation was not quite correct.
vfile here contains a𐐀b\na𐐀b\n. If you take 1 line (which is a𐐀b\n) and then 4 code points, you consume the last newline (as a codepoint) as well and now your position is (2, 0).
There was a problem hiding this comment.
Okay, there's an interesting question here, namely: if you have a position (x,y) where y is greater than the number of characters in the line, does that represent a position on the next line? I would say no: the first coordinate of the position limits you to that line, and the most you can do is go to the end of it. Which I would represent as (x, maxCol+1), although arguably that's a bit weird.
There was a problem hiding this comment.
Imagine that you pressed → key x times and then → key y times. I believe in the majority of modern editors this moves your cursor to the next line x+1. Such convention also allows to make splitAtPosition total, which is a nice property.
There was a problem hiding this comment.
So the more realistic example in our case where we can end up with positions out of range is the following:
- HLS has stale file contents where line 2 only has 30 characters
- The user has locally edited line 2 so they have more than 30 characters, and so their editor sends us positions with offsets over 30
It would be more wrong in general I believe to just start wrapping onto the next line. Keeping things strictly line-based is better here. That's why I initially was returning Nothing for positions beyond the end of the line!
There was a problem hiding this comment.
I see your point, but IMHO once VFS is out of sync all bets are off anyways. I think positionToCodePointPosition is too low-level to be bothered with this kind of sanity checks, it does not edit anything per se, but I can try reverting to the previous design.
There was a problem hiding this comment.
Well, the VFS being slightly out of sync is actually the normal state when the user is editing the file!
I wish I had some clear design to point you to (the LSP spec sadly provides no guidance)... I think the clamping part is fine, but I do think we should try and keep things to the same line and use the position "just past" the end of the line when we go off the end.
| codePointPositionToPosition vfile (CodePointPosition 1 1) `shouldBe` J.Position 1 1 | ||
| codePointPositionToPosition vfile (CodePointPosition 1 2) `shouldBe` J.Position 1 3 | ||
| codePointPositionToPosition vfile (CodePointPosition 1 3) `shouldBe` J.Position 1 4 | ||
| codePointPositionToPosition vfile (CodePointPosition 1 4) `shouldBe` J.Position 2 0 |
|
Closing, I don't have interest or time to pursue this further. Feel free to reuse/rebase if needed. |
Closes #423
@michaelpj if this looks good, I'll proceed with
text-rope-0.2release.