gchp / iota

A terminal-based text editor written in Rust
MIT License
1.63k stars 81 forks source link

Implementing buffer as a gap buffer #50

Closed withoutboats closed 9 years ago

withoutboats commented 9 years ago

Integration not nearly finished & no tests done. Will be travelling the next few days, please improve.

gchp commented 9 years ago

Will be travelling the next few days, please improve

Just to be sure, do you mean for others to continue with this?

gchp commented 9 years ago

I have an initial implementation of this working in the gapbuffer branch. Still quite a while to go, but its a start.

gchp commented 9 years ago

I'll update the branch with your latest changes.

withoutboats commented 9 years ago

Significant updates to this branch.

Just wanted to give a brief write up of what this pull request would do.

Buffer implementation is encapsulated.

The physical implementation of the buffer is entirely encapsulated behind the Buffer object. Currently it is implemented as a gapbuffer, but this could be easily replaced by whatever data structure benchmarks better.

All the buffer data structure needs to implement are:

I may have forgotten one, but in any event they're basic features of any container for sequential data.

Cursor and buffer object folded together.

The objects related to the cursor no longer exist. Instead, the buffer object stores a hashmap of marks, which are points of interest in the file. Marks are keyed by an enum called Mark; one mark, called the Point, is the actual position at which inserts & removes will occur. This is distinct from the cursor displayed on the screen, which is a separate mark. I have taken the terms "mark" and "point" from emacs.

The Direction enum is now in buffer, and has been changed. Up/Down/Left/Right now carry a uint indicating how far to shift in that direction (currently this feature is not used and all are set to 1). A LineStart and LineEnd direction have been added. Similar relative shifts in a mark's position can be added as values for the Direction enum.

The buffer presents these public methods right now:

The file_path field is also currently public. Ultimately this should probably be replaced with some generic way of storing different kinds of metadata about the buffer.

View rewritten (few other changes outside of buffer.rs & cursor.rs).

View has been majorly rewritten to handle the buffer refactor, but the methods presented by view have not been changed. Changes outside of buffer, cursor, and view are all minor - almost entirely updates to enums.

No tests done!

This still should not be merged until it has been thoroughly tested and whatever bugs exist have been sorted out. I'll start work on writing tests.

withoutboats commented 9 years ago

Should be ready to merge.

gchp commented 9 years ago

I'm seeing some issues when running this cargo run src/iota/view.rs.

pythonesque commented 9 years ago

May I just say, I'm loving all the deletions in this commit? :)

pythonesque commented 9 years ago

I've just reviewed your change and you removed essentially all support for characters > 1 byte. Please add it back.

withoutboats commented 9 years ago

P1start they would rrview for unicode support. I'm not very familiar with it.

Sent from my android device.

-----Original Message----- From: Joshua Yanovski notifications@github.com To: gchp/iota iota@noreply.github.com Cc: Lee Aronson lee@libertad.ucsd.edu Sent: Fri, 26 Dec 2014 9:59 AM Subject: Re: [iota] Implementing buffer as a gap buffer (#50)

I've just reviewed your change and you removed essentially all support for characters > 1 byte. Please add it back :)


Reply to this email directly or view it on GitHub: https://github.com/gchp/iota/pull/50#issuecomment-68150612

withoutboats commented 9 years ago

Ugh these sound they an off by one or something in move mark line. I'll fix ir but I can't until Saturday.

Sent from my android device.

-----Original Message----- From: Greg Chapple notifications@github.com To: gchp/iota iota@noreply.github.com Cc: Lee Aronson lee@libertad.ucsd.edu Sent: Fri, 26 Dec 2014 3:59 AM Subject: Re: [iota] Implementing buffer as a gap buffer (#50)

I'm seeing some issues when running this cargo run src/iota/view.rs.


Reply to this email directly or view it on GitHub: https://github.com/gchp/iota/pull/50#issuecomment-68138158

pythonesque commented 9 years ago

@P1start Ping, can you please give more specific suggestions on how to get Unicode working in the new model? Clearly casting char as u8 isn't going to cut it.

ftxqxd commented 9 years ago

I’ll try to have a look over it soon, but I won’t be available for the next few hours. There’s some other work I’ve been wanting to do on multi-column characters to more closely resemble other editors, so I personally wouldn’t mind if this were accepted without proper multi-byte character support. (From a cursory glance at the code, it looks like it wouldn’t require too many changes to properly support all Unicode characters anyway.)

gchp commented 9 years ago

When the issues listed above have been resolved, I'll merge it in and create a new issue for fixing unicode support. Thanks @P1start for the response!

withoutboats commented 9 years ago

Okay, should be ready to merge.

gchp commented 9 years ago

Ok, those issues are fixed. I'm seeing a pretty big performance hit with this, however.

Moving the cursor around is really sluggish all of a sudden. If you open view.rs with these changes, and hold the down arrow for a second or two (until the view is scrolling), you'll notice that when you release the key, the cursor keeps scrolling for a good while as it processes the remaining events. Compare this with the current master version, and you'll see that it is significantly faster. What are your thoughts on this?

Also, when you scroll, the cursor should always remain in view, and never go into or beneath the status bar which is what I see happening here. This is what the threshold field on View was for. It was the amount of lines from the bottom/top that the cursor would remain when scrolling.

The main thing right now though is the performance. What do you think is causing this hit?

gchp commented 9 years ago

I'm guessing its because there are a number of lookups on the entire body of text (iteration and/or slicing) for each keypress?

withoutboats commented 9 years ago

Performance hit should have been resolved on the last commit (was for me at least). Yeah, the draw methods I wrote pretty off the cuff turns out to have some bugs. In addition to the odd behavior on down, scrolling right doesn't work at all.

gchp commented 9 years ago

Don't worry too much about horizontal scrolling, it wasn't working before either :)

Once the vertical scrolling is working that's cool. If horizontal works too then great!

gchp commented 9 years ago

I fixed the vertical scrolling, and disabled horizontal scrolling for now.

This has been merged in. Thanks for the work!