Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add selection - part 2 #148

Merged
merged 9 commits into from
Sep 26, 2024
Merged

Add selection - part 2 #148

merged 9 commits into from
Sep 26, 2024

Conversation

HKalbasi
Copy link
Contributor

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:

  • Remove the lines field, together with its siblings tab_map and dbl_map: These requires O(lines) time for keeping them updated in case of line breaks and selection removes (and undo of those), and it is against the point of using rope which supports insertion and deletion in O(log length). It's a better idea to calculate them on demand from the rope itself for the current viewport.
  • Use tree-sitter instead of synpotic: synotic highlighter essentially mandates the existence of the above lines field. Tree-sitter is a more precise (it's ast-based but synoptic is regex-based) syntax highlighter that is very fast, supports incremental parsing and has a rope-friendly api (that is, it can work with discontinuous segements of memory). It is also used in other rust based editors helix and zed.

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
Copy link
Owner

curlpipe commented Sep 20, 2024

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 👍

@curlpipe
Copy link
Owner

curlpipe commented Sep 20, 2024

(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
Copy link
Contributor Author

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
Copy link
Owner

curlpipe commented Sep 21, 2024

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

  • Undoing all the way doesn't mark the file as unmodified (even though it would be the same as the version on the disk)
  • Undoing and then trying to edit the document causes an "out of range" error which prevents the edit
  • When a selection is deleted / cut, the document isn't marked as being modified and the cursor goes crazy

Also

  • Update tests to verify the new undo/redo system and ensure it isn't accidentally broken down the line

@curlpipe
Copy link
Owner

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
Copy link
Owner

I assume all is well, merging

@curlpipe curlpipe merged commit 296c063 into curlpipe:master Sep 26, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants