Skip to content

use syntect to highlight#99

Draft
light4 wants to merge 5 commits into
sigmaSd:masterfrom
light4:highlight
Draft

use syntect to highlight#99
light4 wants to merge 5 commits into
sigmaSd:masterfrom
light4:highlight

Conversation

@light4

@light4 light4 commented Nov 5, 2022

Copy link
Copy Markdown
Contributor

Try using syntect to highlight, any ideas?

@sigmaSd

sigmaSd commented Nov 5, 2022

Copy link
Copy Markdown
Owner

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

sigmaSd commented Nov 5, 2022

Copy link
Copy Markdown
Owner

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

@light4

light4 commented Nov 5, 2022

Copy link
Copy Markdown
Contributor Author

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

sigmaSd commented Nov 5, 2022

Copy link
Copy Markdown
Owner

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

sigmaSd commented Nov 5, 2022

Copy link
Copy Markdown
Owner

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

light4 commented Nov 6, 2022

Copy link
Copy Markdown
Contributor Author

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

@sigmaSd

sigmaSd commented Nov 6, 2022

Copy link
Copy Markdown
Owner

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

sigmaSd commented Nov 6, 2022

Copy link
Copy Markdown
Owner

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

sigmaSd commented Nov 6, 2022

Copy link
Copy Markdown
Owner

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

@sigmaSd

sigmaSd commented Nov 6, 2022

Copy link
Copy Markdown
Owner

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

light4 commented Nov 7, 2022

Copy link
Copy Markdown
Contributor Author

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

messed_code

@sigmaSd

sigmaSd commented Nov 7, 2022

Copy link
Copy Markdown
Owner

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.

2 participants