curlpipe / ox

The simple but flexible text editor
GNU General Public License v2.0
3.36k stars 110 forks source link

Add selection - part 2 #148

Closed HKalbasi closed 2 months ago

HKalbasi commented 2 months ago

This PR adds support for remove text under selection with backspace and when typing something else, and adds support for cut. It uses the undo infrastructure discussed before.

While the snapshot infrastructure is very cheap performance-wise, the overall performance story is not really great. It currently reloads the whole lines cache and highlighter, and I don't see actions to improving the current state. If we hit performance problems in future, we can do these:

All said, I don't think performance issues are currently high priority in ox, and we are good for now. (I noticed some glitches in rendering on paste, but it is unrelated to the data structures and can be fixed by some throttling).

curlpipe commented 2 months ago

Cool, thank you! I'll have a look at it a little later.

What I like about synoptic is that you can write custom syntax highlighting rules for it in the configuration file (see here), I'm not sure if the same can be done with tree-sitter.

I do like the sound of moving away from the lines and dbl_map / tab_map infrastructure and just using the rope directly with no additional data structures. We can feed synoptic lines taken directly from the rope so we wouldn't need lines. Synoptic also shouldn't take too much time if it knows which lines have changed (that's what the edited calls are throughout the codebase, they tell Synoptic that a particular line has been changed so it only needs to re-render part of the document rather than the whole thing).

By the way - I'm happy to move back to osc52-based copy - you're right in saying that it increases the build time. This would fit nicely into the scope of this PR :+1:

curlpipe commented 2 months ago

(just fixed some merge conflicts as I published a new version recently)

Just a note - I'm thinking of adjusting how the repository operates

Features can be implemented on PRs like this, which can then merge into a branch dev, and then when a new version is ready to be released, dev can be merged into master. It provides a bit of protection for the master branch and provides a place for everyone's PRs (and smaller bugfix commits) to slot together before they are all let onto the master branch.

Do you think this sounds like a good idea?

I feel like it would put less pressure on me to push full releases so quickly.

HKalbasi commented 2 months ago

What I like about synoptic is that you can write custom syntax highlighting rules for it in the configuration file (see here), I'm not sure if the same can be done with tree-sitter.

It is possible with tree sitter, but it requires more effort (it needs compiling the grammar to a .so file). It is also possible to make synoptic support fast insertions. Even if we decide to keep using synoptic, it is helpful to have a look at tree sitter. These links are helpful:

Synoptic also shouldn't take too much time if it knows which lines have changed (that's what the edited calls are throughout the codebase, they tell Synoptic that a particular line has been changed so it only needs to re-render part of the document rather than the whole thing).

The current edit api needs whole file parse in case of line break or selection delete. But with a rope like edit api I think it is possible to make it fast.

By the way - I'm happy to move back to osc52-based copy - you're right in saying that it increases the build time. This would fit nicely into the scope of this PR 👍

Good! I moved back the osc52 based copy, but for paste, I added a message to encourage users to set ctrl+v in their terminal emulator to do paste. As I said before, osc52 does have paste but it is disabled by default in most terminals, fundamentally doesn't work with tmux and zellij, and requires async communication or blocking the editor for getting the response so terminal paste is better in every way.

Just a note - I'm thinking of adjusting how the repository operates ...

I'm agree that releasing version on every change is cumbersome. The dev branch could work, but is not necessary. You can merge PRs on master, and occasionally (for example every month or after some features) do some testing and polish, then make a release commit that bumps versions in Cargo.toml, do a git tag/github release like you do now, and publish the release into crates.io (and maybe distros package managers). In README you can encourage people to install ox from crates.io (that is cargo install ox instead of cargo install --git https://github.com/curlpipe/ox ox) so they will always use the latest polished release, not the latest commit in the master. Separating dev and master branches or having release specific branches mostly help if you want to backport some commits on older releases, but I don't think you want to support old releases of ox.

curlpipe commented 2 months ago

Just reviewed the PR and I noticed around 3 bugs (I'll get to solving these as I am pretty confident where they are)

Also

curlpipe commented 2 months ago

So I'm pretty confident this is all good to go, let me know if you are happy for this to be merged

For selection part 3, we can look at double-clicking to select words and triple-clicking to select lines like other editors do.

curlpipe commented 2 months ago

I assume all is well, merging