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

feat(highlights)!: redesign of highlighting approach #1449

Conversation

xiyaowong
Copy link
Collaborator

@xiyaowong xiyaowong commented Sep 14, 2023

This PR rewrites the approach for handling highlights. It now renders cells based on columns. It aims to maintain consistency with nvim in most daily use cases. Only a few rare edge cases in daily work may still have issues.

Some demonstration images.

  • hop & flash
  • tabs
  • emojis

emojis

  • simple benchmarks

Regular document
benchmark1

Unusual document 👀
benchmark2

Although there is noticeable lag in the second image, I still think it is sufficient.

.eslintrc.js Show resolved Hide resolved
@5h5ong
Copy link

5h5ong commented Sep 15, 2023

normal.mov
writing.mov
code.1.clojure.mov
code.2.javascript.mov

I'm using flash.nvim. It works smoothly in real usage environments. It seems that the problems i used to face have been wll resolved.

@xiyaowong
Copy link
Collaborator Author

Marked as “ready for review” to get attention and assistance with testing from others.

@xiyaowong xiyaowong marked this pull request as ready for review September 16, 2023 06:41
@theol0403
Copy link
Member

I'm extremely busy these days so might take me a bit to review this, just because it's a sensitive area and there are a lot of changes. Thanks for your work on this!
Fwiw eol highlighting is important for fake visual cursor.

@theol0403
Copy link
Member

Also, I haven't really looked at the code, but I feel like the highlight provider has the potential to reduce a lot of lines of code/simplify, if that's at all something you can work towards, as it was pretty messy before.

@xiyaowong
Copy link
Collaborator Author

xiyaowong commented Sep 17, 2023

This PR took up almost all of my time last week 😷. For the most part, it’s stable in most scenarios now, and I’m currently using the latest compiled extensions in my daily work. The remaining issue lies with multi-byte characters due to differences in handling character encoding length between positions in vim, the editor, and CSS.

But these are mostly edge cases that most people wouldn’t encounter in their daily work. For example, if you try selecting, moving, or manipulating this string: “🎉🔧🥇🎉 🔧 🥇 🤝 🎉 🔧 🥇 ❤️”, you’ll see where the issue lies. Through debugging, I’ve confirmed that the current position calculations are problematic for multi-byte characters. While I attempted to fix it, I found that it would impact other modules and introduce new issues. Therefore, I decided not to continue with the fix as I don’t want to invest too much effort in solving a rarely encountered problem. Personally, I prefer addressing such rare cases in the future.

@theol0403
Copy link
Member

Hahah it's easy to get sucked into spending a lot of time on this project 😉

@theol0403
Copy link
Member

A quick test, everything seems to be working. I'll have to review the code later.

@theol0403
Copy link
Member

Sometimes, when using leap.nvim, I get some labels that float & get stuck well past the end of the line:
image

@justinmk
Copy link
Collaborator

Sometimes ... some labels float & get stuck

I see that with the current release (CTRL-L clears it up), so I wouldn't say it's a blocker for this PR.

@xiyaowong
Copy link
Collaborator Author

@theol0403 @justinmk Solution for syntax. xiyaowong#1

* feat(highlight): rework 'syntax'

* chore: enable syntax by default

* fix: fix debouncing

avoid unnecessary refreshing

* fix: improve setting namespace of windows

* fix: should set w:_vscode_hl_ns

* fix!: fix highlight refresh

* refactor!: only clear overrides  and syntax highlights

plugins should not link to builtin groups

* chore: setup highlight before VimEnter
@xiyaowong
Copy link
Collaborator Author

xiyaowong commented Sep 24, 2023

@theol0403 I think this pr is stable enough now. It can be reviewed and considered for merging. Due to the numerous changes in this PR, it has become challenging to proceed with other refactoring works(#1471) and new features. Dealing with conflicts can be a headache.

Copy link
Member

@theol0403 theol0403 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the wait, it looks great. Just clean up a few of the commented code that isn't being used.

I don't have time to extensively test, but we can fix things as they come up.

@theol0403
Copy link
Member

Feel free to merge after you've done the final touches.

@xiyaowong xiyaowong merged commit f688d23 into vscode-neovim:master Oct 5, 2023
8 checks passed
@xiyaowong xiyaowong deleted the refactor/redesign-highlighting-approach branch October 5, 2023 06:36
@stevenmqnguyen
Copy link

Given this redesign, is flash.nvim able to do away with manually setting colors and instead use the colors of current vscode theme?

I'm not sure if I completely understand how the interaction between vscode-neovim and flash.nvim work but I tested removing the custom vscode logic in flash.nvim and it seems like in vscode the highlights aren't showing up.

@xiyaowong
Copy link
Collaborator Author

xiyaowong commented Oct 6, 2023

@stevenmqnguyen

Given this redesign, is flash.nvim able to do away with manually setting

No, plugins should not link to the builtin groups. xiyaowong#1 (comment)

There are two ways to customize colors:

  1. set colors in nvim
  2. set colors in vscode.
    References:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants