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

use syntect to highlight #99

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

use syntect to highlight #99

wants to merge 5 commits into from

Conversation

light4
Copy link
Contributor

@light4 light4 commented Nov 5, 2022

Try using syntect to highlight, any ideas?

@sigmaSd
Copy link
Owner

sigmaSd commented Nov 5, 2022

I used to use syntect before but I replaced it with custom parsing because it's simpler even faster and have no compile time repercussion

But I like the idea but I prefer:

  • It's behind a feature flag so at least when I'm developing in other parts of the code base I don't have to be impacted by the added compile times
  • keep the old code and expose this as an option to the user for example use_syntect = bool in the config and maybe in the future change even expose the syntect theme as an option

@sigmaSd
Copy link
Owner

sigmaSd commented Nov 5, 2022

I remember the performance was a real issue (that code gets executed on each key stroke) not sure if things has improved

@light4
Copy link
Contributor Author

light4 commented Nov 5, 2022

add three features, rustc_lexer, syntect and change_highlight, rustc_lexer is the default
change_highlight support change highlight engine in config.

TODO:

  1. support theme
  2. check performance

anything else?

@sigmaSd
Copy link
Owner

sigmaSd commented Nov 5, 2022

I made a demo comparing the two: (I just copy a large text and hit shift ctrl v)

  • default
    default

  • syntect
    syntect

You can see that I can't even wait for syntect to finish xD

Admittedly the difference is less in release mode, also the pasting can be improved by using bracketed paste terminal event but that's another issue.

@sigmaSd
Copy link
Owner

sigmaSd commented Nov 5, 2022

btw I just added some new feature requests ideas https://github.com/sigmaSd/IRust/issues if you're interested.

Also feel free to add more if you want.

@light4
Copy link
Contributor Author

light4 commented Nov 6, 2022

Improved the highlight performance. If this is acceptable, I'll begin to add theme support.

@sigmaSd
Copy link
Owner

sigmaSd commented Nov 6, 2022

Nice , unfortunately now you are dealing with the very ugly parts of the codebase

Hopefully we can make it work though, I'll have to test for a bit

@sigmaSd
Copy link
Owner

sigmaSd commented Nov 6, 2022

I added a commit to actually enable bracketed-paste.

What I propose is to feature freeze this pr and try to fix the current issues and in later pr add the other features.

I have done some basic testing and here is what I see:

  • The cursor position is wrong after accepting a racer suggestion (by clicking on the right arrow)
    racer

  • The cursor position is wrong after bracketed paste
    bpaste

  • The cursor movement is laggy when using the syntect engine, it seems like I'm currently rendering the whole thing even on cursor move, but its not obvious on the default backend because its fast
    slowcursor

Like I said earlier these are the very ugly parts of the code-base, I managed to get them work somehow and I really try to not to touch them at all, before a full rewrite

@sigmaSd
Copy link
Owner

sigmaSd commented Nov 6, 2022

I added another commit which seems to fix the 2 positioning problems. But that probably need more testing

@sigmaSd
Copy link
Owner

sigmaSd commented Nov 6, 2022

I finally got around to adding benchmarks a7c0194 (I added them to my own fork)

And the number difference is huge
cargo +nightly bench -q --all-features highlight
image

@light4
Copy link
Contributor Author

light4 commented Nov 7, 2022

When EnableBracketedPaste, the codes pasted are messed up, env: macOS, iterm2/terminal.

messed_code

@sigmaSd
Copy link
Owner

sigmaSd commented Nov 7, 2022

oh well I guess that commit should be reverted and can be something to fix in a later time

I tested on linux wezterm

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.

None yet

2 participants