atom-archive / text-document

MIT License
57 stars 10 forks source link

Implement position translation with clipping #3

Closed nathansobo closed 9 years ago

nathansobo commented 9 years ago

This PR demonstrates my preferred approach for dealing with clipping when translating positions. It also restructures some other things since it's early in the project and I didn't want to bother breaking things up into tiny pieces. One big change is that it replaces consume and emit* with a single function, transform. Its parameters are as follows:

Clipping is implemented in TransformLayerIterator::seek, which takes an optional second clip argument to indicate whether we should clip forward or backward to the nearest valid position when the seek lands inside a clipped region.

@as-cii Definitely not intending to step on your PR with this one, but I needed to code freely. :seedling: Would you be interested in finishing this branch out with tests for layers besides hard-tab? This branch probably conflicts textually with your PR but the concepts are pretty portable.

nathansobo commented 9 years ago

Thanks for picking this up and pushing it forward. Your changes are looking :sparkles:. I may have time later today to look into the spec failures, but the overshoot calculation is definitely a culprit so that's a good place to start investigating.

as-cii commented 9 years ago

Thanks for picking this up and pushing it forward. Your changes are looking . I may have time later today to look into the spec failures, but the overshoot calculation is definitely a culprit so that's a good place to start investigating.

Working on this stuff is just exciting and I hope to get more and more confident with the code as we progress! :blush:

I used Point to calculate the overshoot, but the problem was that we were calculating it even when sourcePosition was equal to the desired position. By the way, we should be good to go now! :hand:

There are still other specs which I copied over from TextBuffer but excluded intentionally, because clipping invalid positions hasn't been implemented yet. This last "feature" may also be implemented in terms of TransformIterator#seek: since I am not sure this is what we actually want, I sketched out a first implementation (which passes all the tests) in another PR which I am going to open soon. Code-wise it's still a bit messy, but it demonstrates the point I believe. I'll mention you there so that we can have a brief discussion (or just close it if I wrote crappy code :sweat_smile:)