search

atom-archive / text-document

MIT License
57 stars 10 forks source link

Basic Position Translation #2

Closed as-cii closed 9 years ago

as-cii commented 9 years ago

Based on the much appreciated task list in README.md I started off a branch, which hopefully will be a starting point for me for future and more complicated contributions :blush:

Please note that this is far from being finished, as I assume we should include clipping as well (which I believe should be implemented at TransformIterator layer?). However, I wanted to have some early feedback from you (/cc: @nathansobo @maxbrunsfeld) to understand if we're on the right track :footprints:

API-wise I thought that we could just have to simple methods to traverse layers' positions back and forth:

Which, roughly, would correspond respectively to screenPositionForBufferPosition and bufferPositionForScreenPosition. This is different from what we had on text-transform repository, as it adopts a different naming and allows only for end-to-end traversal (which should be what we actually want).

Please, bear with me if there are any evident issues with my proposed changes :pray:

Thanks!

nathansobo commented 9 years ago

@as-cii I was thinking that each layer would have a fromPositionInLayer and toPositionInLayer method like the original text-transform attempt. These methods would take a point and a layer lower in the stack. We would then recursively translate the position through successive layers until we finally bottomed out in the same layer as the second argument. That way we have a basic primitive we can use to translate between any two layers.

as-cii commented 9 years ago

I was thinking that each layer would have a fromPositionInLayer and toPositionInLayer method like the original text-transform attempt. These methods would take a point and a layer lower in the stack. We would then recursively translate the position through successive layers until we finally bottomed out in the same layer as the second argument. That way we have a basic primitive we can use to translate between any two layers.

@nathansobo: yeah, I thought to structure things that way too, but I couldn't justify why we would need to stop at certain layers while translating points back and forth. I'll add that soon anyway, but could you explain what's your thinking behind allowing such flexibility, please?

nathansobo commented 9 years ago

We'll need the ability to translate positions in the lines layer to character offsets in the file to support scanning. This is currently implemented in TextBuffer with a span-skip-list.

maxbrunsfeld commented 9 years ago

@as-cii I was thinking that each layer would have a fromPositionInLayer and toPositionInLayer method like the original text-transform attempt. These methods would take a point and a layer lower in the stack.

@nathansobo @as-cii what do you think about TransformLayer having the methods sourcePositionForPosition(point) and positionForSourcePosition(point), rather than fromPositionInLayer(point, layer) and toPositionInLayer(point, layer). The functionality of iterating through the transform stack could be implemented in TextDisplayDocument in terms of these methods. The advantage in my mind is that the TransformLayer's contract is a little simpler; it doesn't need to know that it is in a stack of layers. It just knows that it has a source layer. Meanwhile, the TextDisplayDocument already needs to be aware of the transform stack, so shifting this job to that class doesn't add to its responsibilities.

nathansobo commented 9 years ago

@maxbrunsfeld That sounds good. I'm really thinking things might be clearer if we used the word target to contrast with source in our naming. Are you still strongly opposed to that? It's all going to be internal APIs and I want to emphasize the narrative of two corresponding coordinate systems.

as-cii commented 9 years ago

I'm really thinking things might be clearer if we used the word target to contrast with source in our naming. Are you still strongly opposed to that? It's all going to be internal APIs and I want to emphasize the narrative of two corresponding coordinate systems.

Visually speaking, I think it would definitely be more symmetric. However, I wouldn't enforce this naming everywhere. In layers, for example, there isn't really a target: as a result, I find Layer#positionForSourcePosition much more expressive compared to Layer#targetPositionForSourcePosition.

Iterators, on the other hand, may be a good place where we could apply this naming, since, if I understand correctly, they already have the concept of a source and a target.

@nathansobo @maxbrunsfeld: my main goal is to help you :ship:ping things faster, but I still need to wrap my head around the code, and I understand that I may slow you down. Therefore, please, feel free to work on this branch and to bring it forward if you need to. Thanks :smiley_cat:

nathansobo commented 9 years ago

However, I wouldn't enforce this naming everywhere. In layers, for example, there isn't really a target: as a result, I find Layer#positionForSourcePosition much more expressive compared to Layer#targetPositionForSourcePosition.

I agree. I'm over that idea. In my new PR I'm using the names fromSourcePosition and toSourcePosition to keep things a bit more brief.

my main goal is to help you :ship:ping things faster, but I still need to wrap my head around the code, and I understand that I may slow you down.

It's awesome that you're digging into this and it's the best way to wrap your head around it. Even if we don't merge a given PR the best way to express ideas is in code and the discussion is valuable. If you could help with the PR I just opened that would be huge... I just want to get translate tests on all the layers.

Another thing you could do is start fleshing out the public classes in terms of our layer primitives and porting over tests. Feel free to take your own direction, too, just thought I'd offer some suggestions to ensure we work effectively in close quarters.

nathansobo commented 9 years ago

Another more exciting thing to do would be to split a MutationLayer out of BufferLayer. BufferLayer is currently dealing with caching and mutations, and there's no real reason to handle both of these there. The code will be cleaner if they each operate on their own layer.

as-cii commented 9 years ago

@nathansobo: they all sound like great ideas! I'll start by writing specs for your newly created PR and basically port TextBuffer's position translation public API (along with its related spec suite) so that we can merge it ASAP.

Before extracting the mutation layer, however, I think I need to have a look and start addressing https://github.com/atom/atom/issues/6055, as it is currently blocking us from shipping https://github.com/atom/atom/pull/6027. Hopefully, I can fix it by Monday (I wish days had 36 hours :joy:) and go on with what's left on this repo.

Great job on #3 by the way :raised_hands: Let's close this one! :wink:

nathansobo commented 9 years ago

Thanks so much. I really appreciate your help and input.

nathansobo commented 9 years ago

@as-cii As for atom/atom#6055, we might be able to put it off if @zcbenz can maintain a patch libchromiumcontent for a while. That said, we really should fix that some time in the near future.

as-cii commented 9 years ago

As for atom/atom#6055, we might be able to put it off if @zcbenz can maintain a patch libchromiumcontent for a while. That said, we really should fix that some time in the near future.

@nathansobo: nice to hear that. If it's not so pressing I'll concentrate my attention into this repo :metal: (however, I feel like it's a problem we need to solve sooner or later and I'll put some thinking into it anyway :thought_balloon:)