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

vH and vL is not work as expected when $ and ^ is remapped to L and H respectively. #4991

Closed
w93163red opened this issue Jul 5, 2020 · 8 comments · Fixed by #4735 or #5000
Closed

Comments

@w93163red
Copy link

Describe the bug
When I remapped the $ and ^ to L and H on visual mode, it will not behave as Vim does.
In Vim, if I want to select to end of line, I did vL,
but in vsvim, I need to vlL.
To Reproduce
Steps to reproduce the behavior:

  1. remap $, ^ to L, H.
  2. type vL.

Expected behavior
should select to line end.
Screenshots
If applicable, add screenshots to help explain your problem.

image

Environment (please complete the following information):

  • Extension (VsCodeVim) version:1.14.5
    Version: 1.46.1
    Commit: cd9ea6488829f560dc949a8b2fb789f3cdc05f5d
    Date: 2020-06-17T21:17:14.222Z (2 wks ago)
    Electron: 7.3.1
    Chrome: 78.0.3904.130
    Node.js: 12.8.1
    V8: 7.8.279.23-electron.0
    OS: Darwin x64 19.5.0
@berknam berknam mentioned this issue Jul 5, 2020
10 tasks
@sql-koala
Copy link
Contributor

This should work / be possible.
Can you post your exact mapping config?

@berknam
Copy link
Contributor

berknam commented Jul 7, 2020

Currently there is an issue in visual mode that if you type a key right after going into visual mode it will send that key to the remapper along with the key used to go into visual mode. Example: in this issue case if you press vL without waiting the timeout between both keys it will check for a visual remap of vL instead of L. If you wait for timeout between those keys then it will work normally. This will be fixed with PR #4735. Now as a quick and dirty workaround you can either wait for timeout or you can repeat the same remaps with 'v' before, in your case you can do:

"vim.visualModeKeyBindings": [
    {
        "before": ["H"],
        "after": ["^"],
    },
    {
        "before": ["v", "H"],
        "after": ["^"],
    },
    {
        "before": ["L"],
        "after": ["$"],
    },
    {
        "before": ["v", "L"],
        "after": ["$"],
    },
]

@J-Fields I don't know what is the ETA to merge PR #4735? If you think it will still take some time and if it is worth it I can try to fix the current remapper just for this issue.

@J-Fields
Copy link
Member

J-Fields commented Jul 7, 2020

@J-Fields I don't know what is the ETA to merge PR #4735? If you think it will still take some time and if it is worth it I can try to fix the current remapper just for this issue.

Hard to say, it's become such a thick PR 🙃 I think I can do another review soon, currently trying to decide whether to put it off til 1.16.0 or delay 1.15.0 to include it.

@J-Fields
Copy link
Member

J-Fields commented Jul 7, 2020

If it's easy enough to peel this fix off of that PR, I'd say go for it - that would certainly make everything a bit more manageable. If it's not trivial, though, probably no sense

@w93163red
Copy link
Author

w93163red commented Jul 7, 2020 via email

@sql-koala
Copy link
Contributor

Currently there is an issue in visual mode that if you type a key right after going into visual mode it will send that key to the remapper along with the key used to go into visual mode.

Ah, I see. I am not 100% sure, but I think ran into that bug, too. In my case I found a fix by accident:

nnoremap v v

Then first motion after that works as expected.

@berknam
Copy link
Contributor

berknam commented Jul 7, 2020

Ah, I see. I am not 100% sure, but I think ran into that bug, too. In my case I found a fix by accident:

nnoremap v v

Then first motion after that works as expected.

Yep that also fixes it.

If it's easy enough to peel this fix off of that PR, I'd say go for it - that would certainly make everything a bit more manageable. If it's not trivial, though, probably no sense

Its not really a peel off, because this issue is created by a part of code that no longer exists in my PR but is still a quick fix. So I will push it in a moment. Although the fix will be just for the specific situation of this issue, all the other problems with the remapper will still be there.

Hard to say, it's become such a thick PR 🙃 I think I can do another review soon, currently trying to decide whether to put it off til 1.16.0 or delay 1.15.0 to include it.

Yeah I know. I'm sorry! 😅 That's why I put #4995 into another PR. I really think this refactor will improve the experience for a lot of people!

I know it's quite a lot to review but any questions you have feel free to tell me and I'll do my best to help. I tried commenting every piece of code that wasn't obvious. I think I might even have gone overboard with too many comments! 😆 But I think its best to have too much comments than the alternative, specially since there are some pieces of code that might seem simple to understand now I'm working on them but I now that in the future if the comments weren't there it would take a while to figure it out again. I've been using and testing that PR, and some of the latest commits have been some small bugfixes that I already found and fix. I know that after a lot more people start using it there will be more issues coming up that I can't even imagine right now but I'm here to help fix those too when they come.

@J-Fields
Copy link
Member

J-Fields commented Jul 7, 2020

No worries at all, sorry I've taken so long to get to it! I'm super excited to actually get it merged, I think the endless list of issues it'll fix speaks for itself lmao.

I think I might even have gone overboard with too many comments!

This is always better than the alternative imo

I know that after a lot more people start using it there will be more issues coming up that I can't even imagine right now but I'm here to help fix those too when they come.

Yeah this is pretty inevitable with such a large refactor. I'm sure it'll fix more than it causes, though. Since nobody dies or loses large sums of money when a bug is introduced to this project, my attitude is generally "fix the known issues, and wait for people to complain" 🙃

J-Fields pushed a commit that referenced this issue Jul 9, 2020
When going to visual mode with key 'v' since it is not a complete action the modeHandler wasn't resetting the recordedState.commandList which meant that if the next key came before timeout had passed it would add that key to the commandList that still had the 'v'. Then it would send that list to the remappers.

If you had a remap in visual mode like L -> $ after pressing vL it would send ["v", "L"] to the remappers so it wouldn't find any remap. Only after timeout ended would it send only the last key of commandList and in that case the remappers would find the remap.

Fixes #4991
J-Fields pushed a commit that referenced this issue Aug 16, 2020
This is a pretty massive change; see pull request #4735 for full details

Most notably:
- Support for operator-pending mode, including remaps and a half-cursor decoration
- Correct handling of ambiguous remaps with timeout
- Correct handling of recursive special case when the RHS starts with the LHS
- Correct handling of multi-key remaps in insert mode
- Failed movements that occur partway through a remap stop & discard the rest of the remap
- Implement `unmap` and `mapclear` in .vimrc

Refs #463, refs #4908
Fixes #1261, fixes #1398, fixes #1579, fixes #1821, fixes #1835
Fixes #1870, fixes #1883, fixes #2041, fixes #2234, fixes #2466
Fixes #2897, fixes #2955, fixes #2975, fixes #3082, fixes #3086
Fixes #3171, fixes #3373, fixes #3413, fixes #3742, fixes #3768
Fixes #3988, fixes #4057, fixes #4118, fixes #4236, fixes #4353
Fixes #4464, fixes #4530, fixes #4532, fixes #4563, fixes #4674
Fixes #4756, fixes #4883, fixes #4928, fixes #4991, fixes #5016
Fixes #5057, fixes #5067, fixes #5084, fixes #5125
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 a pull request may close this issue.

4 participants