search

mvllow / modes.nvim

Prismatic line decorations for the adventurous vim user
567 stars 14 forks source link

refactor: use window-local highlights #30

Closed fitrh closed 2 years ago

fitrh commented 2 years ago

Putting a rough implementation here to get some feedback if anyone is interested

The current implementation of how we handle the cursor line color for each mode is to override the CursorLine highlight group, which leads to the following issues/limitations (at least for me):

  1. Assuming that CursorLine and CursorLineNr have bg Related issue: #16 https://github.com/mvllow/modes.nvim/blob/1086d94b1f7d6ba105593a1dd21e2cb02ab92fc0/lua/modes.lua#L76-L77 Depending on the color scheme, we may override the default color in NORMAL mode if the highligh group does not have bg (although we have config.set_* as a workaround)

    My LineNr is blue, once modes.nvim is activated, it turns white

    https://user-images.githubusercontent.com/55179750/172336733-d67c0a96-0530-42eb-a0a8-afa9835810b5.mp4

  2. We can not override the highlight groups on the fly for testing purposes Related issue: #18 https://github.com/mvllow/modes.nvim/blob/1086d94b1f7d6ba105593a1dd21e2cb02ab92fc0/lua/modes.lua#L40-L46 Since for every mode-switching, we redefine the highlight group using the initial config color

  3. We have a strong opinions about CursorLineNr https://github.com/mvllow/modes.nvim/blob/1086d94b1f7d6ba105593a1dd21e2cb02ab92fc0/lua/modes.lua#L41-L43 We assume that CursorLineNr the have the same definition as CursorLine

  4. If we open nvim with an initial window that set CursorLine, we end up using that definition for NORMAL mode For example, if we open nvim with -c ':Telescope find_file', NORMAL mode will use INSERT mode style since in setup() we are trying to extract color definition from CursorLine, as the result, we are using whatever the initial window defined for CursorLine

    https://user-images.githubusercontent.com/55179750/172337470-315d8834-77ea-46a9-b806-40f991ab50b2.mp4

This implementation is to address of these problems

By using winhighlight option, we have the flexibility to set CursorLine/CursorLineNr without overriding it, it is a local-window option, also we can have different definition between CursorLine/CursorLineNr so color scheme developer has more control to style those components.

We don't need to redefine highlight groups for every mode-switcing as it is already been done in define() except for ModesOperator (only built-in highlight-groups are supported by winhighlight).

In the meantime, I've removed this lines as I can't find it's usage, but I'm free to add it back

https://github.com/mvllow/modes.nvim/blob/1086d94b1f7d6ba105593a1dd21e2cb02ab92fc0/lua/modes.lua#L161-L164

This implementation provides the following highlight groups

subject to change, I'm not good at naming

Now user can define any style they want

In addition of these changes, this implementation also changes this behavior:

Opens a split window with the same buffer and view port, the VISUAL style is only applied to the focused window

Old behavior 20220607_171815_full

New behavior 20220607_171711_full

If this change is accepted, there are a few things I'd like to change in this PR (or in the future PRs):

I've been using this for a week and it seems to work, but it would be helpful if anyone could help test it.

mvllow commented 2 years ago

I invited you to be a collaborator to make contributor a little easier 😌 Thanks for all your work on this. I would suggest the following for the names:

ModeMsg* → Modes*ModeMsg (verbose but consistent with other names)
VisualVisual → ModesVisual

Other than the naming this looks great. Seems like it fixes a lot of outstanding issues. I'm unable to test at the moment but am curious if tests are passing or need updated with these changes?

fitrh commented 2 years ago

I don't have a strong opinion, so I implemented all the suggestions.

fitrh commented 2 years ago

In https://github.com/mvllow/modes.nvim/pull/30/commits/e98c8298732dd2eec96c6a01cb42374fac3f97db, I've removed ModeMsg from winhighlight, I just realized (because I always turn off showmode) that ModeMsg is not affected by winhighlight.

fitrh commented 2 years ago

VisualVisual → ModesVisual

Actually we already have ModesVisual (using config.colors.visual), so I named it ModesVisualVisual (using blended_color.visual).

fitrh commented 2 years ago

I'm unable to test at the moment but am curious if tests are passing or need updated with these changes?

Yes, the tests passed, maybe in the future we need to add more tests.

mvllow commented 2 years ago

I did need the M.define() call within the setup function for cursorline highlights to be applied when using Rosé Pine (dark mode). Otherwise, running :colorscheme rose-pine was needed to refresh the colors.

fitrh commented 2 years ago

I did need the M.define() call within the setup function for cursorline highlights to be applied when using Rosé Pine (dark mode). Otherwise, running :colorscheme rose-pine was needed to refresh the colors.

Sure, seems some lua color schemes don't trigger ColorScheme events anymore, I did not notice as I handle my color schemes manually and always trigger Colorscheme events.

mvllow commented 2 years ago

No issues after using this branch for the last few days 😌 Thanks for working on this 💜