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

[Bug Fix] revert back the ctrl-p function for vscode file navigation #8835

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wswslzp
Copy link

@wswslzp wswslzp commented Jan 18, 2024

What this PR does / why we need it:
Previous PR #7261 brought in a severe bug, which is complained by issue #8574, #8581, #8587, #8588.
#7261 change hot-key ctrl-p's original function that allows for easily file navigation. So this PR is aimed to revert the #7261 change. And it does not affect the ctrl-p's vim function that move upward in suggestion.

Which issue(s) this PR fixes
#8574, #8581, #8587, #8588.

Special notes for your reviewer:
@grosssoftware can you help explain why you do this in #7261? IMO, it did not fix anything but broke an important and basic feature of VSCode.

@grosssoftware
Copy link
Contributor

So, back in the day (like pre-2000), it was very common to map ctrl-p and ctrl-n to :bp and :bn in Vim. Some of us still use those mappings. The power of Vim is in its ability to customize key bindings, but before the change in #7261, it wasn't possible to remap ctrl-p correctly.

I wouldn't say that it's broken becuase there are simple workarounds discussed in the issues you mentioned (and maybe those should be the default?), but I agree that masking a useful feature like "Go to File..." (on Windows, that is -- Mac uses cmd-p) isn't ideal or expected. If this reversion is accepted, then we still need a way to override ctrl-p. Suggestions?

@tskj
Copy link

tskj commented Jan 19, 2024

I wouldn't say that it's broken becuase there are simple workarounds discussed in the issues you mentioned

I would like to make a strong case that it is in fact broken. While I have personally adopted the simple workaround, this isn't obvious for people new to the plugin. For anyone today checking out this great plugin, be it because they get referred by a friend or simply want to get into vim, they will think the plugin is completely broken, uninstall it and probably write it off permanently in their minds as "unusable" (and we risk losing that user forever).

Personally I also thought for a long time I had messed something up in my QMK mappings before I realized this was a bug with the plugin, and it therefore took quite a long time before I even discovered there was a simple workaround. I think this applies to most users, and certainly 99% of new users - they will never find this workaround, they'll simply think this is a broken and unmaintained plugin.

So again I strongly urge that fixing this bug must be the single highest priority on this project.

@grosssoftware
Copy link
Contributor

@tskj Sorry, I guess I misspoke: I agree 100% that "Go to File..." is broken. However, I think the change made in #7261 is not broken or a bug, but... it is incomplete. The change is necessary for anyone who hopes to remap ctrl-p. It is the same change that was accepted for ctrl-n.

After looking into this issue more and playing around with some changes on BOTH Windows AND macOS, I think the most sensible approach is to add "<C-p>": false to the defaults for vim.handleKeys here, akin to the workaround suggested by others in the related issues. Ideally, we'd only make this the default for Windows because there is no issue in macOS, but I don't think that's a simple fix. Also, the README should be updated to note that overriding ctrl-p requires setting "<C-p>": true in vim.handleKeys.

Logically, this makes the most sense because that default value is saying "ctrl-p is very important to VsCode and the plugin shouldn't mess with it".

Thoughts?

@tskj
Copy link

tskj commented Jan 20, 2024

No worries, I definitely have a very poor understanding of all the moving pieces here. I don't have any particular opinion on that specific feature, except that I absolutely would be in favour of allowing people to remap ctrl-p as they please - that's a very good goal!

I think setting "<C-p>": false as a default on windows to fix this bug sounds like a very sensible course of action!

@spiffytech
Copy link

Ideally, we'd only make this the default for Windows

Linux too 🙂

@alexyarovoy
Copy link

alexyarovoy commented Jan 25, 2024

@grosssoftware @tskj
If I understand correctly, solution with "<C-p>": false will be somewhat different from what we had before the #7261.

Before the change, we had <C-p> working in code suggestions, and have <C-p> as VS Code function everywhere else. If we just disable it with "<C-p>": false won't it also disable it in code suggestions?

@tskj
Copy link

tskj commented Jan 25, 2024

I can confirm that the solution with "<C-p>": false does indeed disable it in code suggestions. I agree that this is suboptimal, but I still think it's worth it (if reverting #7261 or whatever is off the table).

I would consider the current state of affairs an emergency for the project; every day this goes unfixed is quite a lot of new users we'll lose (potentially permanently).

@black-pwq
Copy link

I strongly recommend adding the following to settings.json (and README.md in this project) instead of disabling use-ctrl-p if this commit is not accepted:

"vim.normalModeKeyBindings": [
  {
    "before": ["ctrl+p"],
    "commands": ["workbench.action.quickOpen"],
    "silent": true
  },
]

Arguments between vscode and vscodevim are unavoidable since both are editors. Personally, I prefer the vscode's way of ctrl+p. If anyone like @grosssoftware want to remap keys which aren't under vim's control, I would recommend them to try to add those in vscode keybindings.json.

@wswslzp
Copy link
Author

wswslzp commented Feb 2, 2024

I agree with @tskj.

I think we all take one principle for granted: adding new features is OK but not breaking anything that work fine before.
What exactly the new feature @grosssoftware you want to ADD is enabling ctrl-p remapping. Breaking original function "Go to File ..." of ctrl-p of VSC is the unexpected SIDE EFFECT. From plugin user point of view, just one day after they updated the VIM plugin the "Go to File ..." function is not work anymore. And they don't know what happen ( which is exactly what I experienced before). They don't even know that now CTRL-P can be remapped implicitly!

So as @grosssoftware you said, #7261 is incomplete. Either you find a nice way to add enabling ctrl-p remapping feature without breaking anything else, or you should at least find some way to notify user of this change and provide some way to close feature to keep everything unchanged just like before. If you are still not fixing this in both ways, then we should revert it. Complete your work before you make PR.

@wswslzp
Copy link
Author

wswslzp commented Feb 2, 2024

I strongly recommend adding the following to settings.json (and README.md in this project) instead of disabling use-ctrl-p if this commit is not accepted:

"vim.normalModeKeyBindings": [
  {
    "before": ["ctrl+p"],
    "commands": ["workbench.action.quickOpen"],
    "silent": true
  },
]

Arguments between vscode and vscodevim are unavoidable since both are editors. Personally, I prefer the vscode's way of ctrl+p. If anyone like @grosssoftware want to remap keys which aren't under vim's control, I would recommend them to try to add those in vscode keybindings.json.

Your workaround is really nice. I haven't update VIM plugin since #7261 checked in until seeing your WAR. Before that, I have seen many WAR in some of the issues I have mentioned in PR description, but none of them actually work fine (ctrl-p not work in suggestion).

But finding WAR is not the solution and it's not our user's responsibility to find such a WAR. It's @grosssoftware work and he should be the guy who provide fix.

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.

6 participants