Extract operations on buffer into line buffer #71
Conversation
- Renamed insert_char -> insert_char_at - Renamed insert_str -> insert_str_at, Modified the interface to use insertion_point instead of insertion_point.offset - Added insert_char: this also modifies the insertion_point in addition to chaning the buffer - Added insert_str: this also modifies the insertion_point in addition to chaning the buffer Question: Do we really need insert_char_at and insert_str_at?
- Rename backspace -> delete_grapheme_left - Rename delete -> delete_grapheme_right
src/line_buffer.rs
Outdated
| /// Insert a single character at the given cursor postion | ||
| pub fn insert_char(&mut self, pos: InsertionPoint, c: char) { | ||
| pub fn insert_char_at(&mut self, pos: InsertionPoint, c: char) { | ||
| self.lines[pos.line].insert(pos.offset, c) | ||
| } |
There was a problem hiding this comment.
I believe we don't really have a use-case where the newly created insert_char is not sufficient, we can remove this method from the public interface and only use that.
Reasoning:
It leads to an inconsistent internal state and the user has to be cognizant about making sure that the insertion point is updated properly. I think we should only expose this sort of API if there is something we can't do with the high-level API that we have.
There was a problem hiding this comment.
If we can express everything pub with codepoint/grapheme safe moves and edits based on the current insertion point state, less weird unicode failure points 👍
For internal stuff we might still want to use it for performance reasons.
There was a problem hiding this comment.
Right now there is not really any place we are making use of this internally so, I am removing this for now. If there is a use for this we can re-add this. Is that ok?
| assert_eq!(expected_line_buffer, line_buffer); | ||
| } | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
I think we can remove this now.
There was a problem hiding this comment.
Do you want to rebase this on #62 with EditEngine or keep the other PR as a draft for a little bit longer? Would be fine to merge into main as well.
Regarding tests, in 08ff9c1 I wrote some doctests for most of the functions, but didn't end up merging that upstrream as doctests don't run on private parts and the API would change anyway. But might have some inspiration for what we might test in the future.
src/line_buffer.rs
Outdated
| /// Insert a single character at the given cursor postion | ||
| pub fn insert_char(&mut self, pos: InsertionPoint, c: char) { | ||
| pub fn insert_char_at(&mut self, pos: InsertionPoint, c: char) { | ||
| self.lines[pos.line].insert(pos.offset, c) | ||
| } |
There was a problem hiding this comment.
If we can express everything pub with codepoint/grapheme safe moves and edits based on the current insertion point state, less weird unicode failure points 👍
For internal stuff we might still want to use it for performance reasons.
I was thinking that PR to just be a sort of discussion point where we can try out a few ideas and not necessarily get it merged. If we try to get that merged it might block other contributors, which is something I would like to avoid. We can extract the bits that make sense there into smaller PRs that way we make continuous progress. Moreover, the more I think about that, the less convinced I am about the inheritance hierarchy that I have created there. |
nixypanda
force-pushed
the
extract-stuff-into-line-buffer
branch
from
10b4ea0 to
af487f9
Compare
|
@sherubthakur Ready to merge? |
Yup |
#61 (comment)
The basic idea here is to pull out the functionality of editing into
line_buffer.