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: handle visual highlight using ModeChanged event #38

Merged
merged 5 commits into from
Oct 10, 2022

Conversation

fitrh
Copy link
Collaborator

@fitrh fitrh commented Sep 7, 2022

cb54e80 introduce changes to check visual mode highlight using
current_mode, this changes causes a bug with Shift + v (visual
line mode) where the cursor highlight still uses the default Visual
highlight until pressing the second key.

Checking visual mode highlights Using key (and checking it inside
current_mode == 'n') is more reliable than using current_mode.

Update

We can completely avoid checking key or current_mode for visual highlight by using autocmd on these events:

  • ModeChanged *:[vV\x16] for set highlight
  • ModeChanged [vV\x16]:n for reset

cb54e80 introduce changes to check visual mode highlight using
`current_mode`, this changes causes a bug with `Shift + v` (visual
line mode) where the cursor highlight still uses the default `Visual`
highlight until pressing the second key.

Checking visual mode highlights Using `key` (and checking it inside
`current_mode == 'n'`) is more reliable than using `current_mode`.
@fitrh fitrh requested a review from mvllow September 7, 2022 06:47
@fitrh
Copy link
Collaborator Author

fitrh commented Sep 7, 2022

@TheBlob42 can you confirm that this doesn't break your use case?

@TheBlob42
Copy link
Contributor

I'm currently only on my phone, will test it tomorrow

Copy link
Owner

@mvllow mvllow left a comment

Choose a reason for hiding this comment

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

This looks fine to me. I did notice this odd behaviour that isn't necessarily because of this PR but happens one keystroke sooner now. Hard to explain, so here's a screenshot before pressing v...

Screenshot 2022-09-07 at 16 17 00

...and after:

Screenshot 2022-09-07 at 16 17 07

Notice the sign column is highlighted immediately after pressing v. This happened before this PR, although not until moving the visual selection by one first.

This shouldn't be a blocker and probably warrants a new issue but I don't find it bothersome enough to make that issue.

@fitrh
Copy link
Collaborator Author

fitrh commented Sep 8, 2022

Notice the sign column is highlighted immediately after pressing v. This happened before this PR, although not until moving the visual selection by one first.

@mvllow isn't that something we introduced in #32?

@mvllow
Copy link
Owner

mvllow commented Sep 8, 2022

Yea, I think with visual mode it's a little strange (since your current line isn't always highlighted like the other modes) but not a big deal. More of a feature request if anything but also would add more complexity.

Fix behaviour when creating new split windows using `CTRL-W_v`
where modes.nvim treats `v` keypress as visual mode.
lua/modes.lua Outdated
end
end

if key == utils.replace_termcodes('<esc>') or key == current_mode then
Copy link
Contributor

@TheBlob42 TheBlob42 Sep 8, 2022

Choose a reason for hiding this comment

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

I guess you introduced key == current_mode here in case the user presses v in visual mode to exit? But unfortunately this breaks other modes and their highlighting:
For example in insert mode if I press the character i or if I press the character r in replace mode (if using R, see :h Replace) the highlighting is reset, but I'm still in the respective mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

My proposal would be to limit this special exit condition (key == current_mode) to visual mode only as I don't know any other mode that can be exited by pressing the same "key" again (if I'm forgetting one please correct me). The general check for <esc> however makes perfect sense to me 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For example in insert mode if I press the character i or if I press the character r in replace mode (if using R, see :h Replace) the highlighting is reset, but I'm still in the respective mode.

You are right, I never realized it since I disabled the cursor line in insert mode

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, I think I got a more robust solution

@TheBlob42
Copy link
Contributor

@TheBlob42 can you confirm that this doesn't break your use case?

Works fine with vim-cutlass or any other similar mappings for me 👍

@fitrh fitrh requested a review from mvllow September 8, 2022 06:49
@fitrh fitrh changed the title fix: use key to check visual mode highlight feat: handle visual highlight using ModeChanged event Sep 8, 2022
@fitrh
Copy link
Collaborator Author

fitrh commented Sep 8, 2022

@mvllow @TheBlob42 using autocmd on ModeChanged event seems more reliable than using key or current_mode variable

@fitrh
Copy link
Collaborator Author

fitrh commented Sep 8, 2022

The autocmd filter is taken from the example in :h ModeChanged, *:[vV\x16] means every time you enter VISUAL{LINE,BLOCK} mode

@TheBlob42
Copy link
Contributor

Now that you mention the ModeChanged event: Is there anything speaking against moving ALL logic from the vim.on_key function to appropriate "ModeChange autocommands" instead? 🤔

This handles the case where leaving visual mode without `<Esc>` or
`vim.on_key` doesn't catch mode switching like toggling comments
in visual mode.
@fitrh
Copy link
Collaborator Author

fitrh commented Sep 8, 2022

Is there anything speaking against moving ALL logic from the vim.on_key function to appropriate "ModeChange autocommands" instead?

@TheBlob42 you can take a look into https://github.com/fitrh/modes.nvim/tree/feat/modechanged-autocmd, maybe we can discuss this in another issue

@fitrh
Copy link
Collaborator Author

fitrh commented Oct 10, 2022

I'm going to merge this since I haven't had any issues using it until now

@fitrh fitrh merged commit 73791bf into main Oct 10, 2022
@fitrh fitrh deleted the fix/visual-line-mode branch October 10, 2022 05:58
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.

3 participants