-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Implementing a leap plugin #8197
base: master
Are you sure you want to change the base?
Conversation
The settings need to be contributed to the |
thx your tips, it's fixed now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I would love to see this PR get merge. Already built you version into my custom VSCodeVim build I'm using for my everyday work.
In the meantime, I made a review.
My main complaint is that you use !
too freely for my taste 🙃
src/actions/plugins/leap/Marker.ts
Outdated
private marker!: Marker; | ||
private textEditorDecorationType: vscode.TextEditorDecorationType; | ||
|
||
private static backgroundColors = ['#ccff88', '#99ccff']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably need to be configurable (or theme dependent)
src/configuration/configuration.ts
Outdated
leap = false; | ||
leapShowMarkerPosition = 'after'; | ||
leapLabels = 'sklyuiopnm,qwertzxcvbahdgjf;'; | ||
leapCaseSensitive = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably should be added to IConfiguration
as well
src/actions/plugins/leap/match.ts
Outdated
let pattern = ''; | ||
if (secondChar === ' ') { | ||
pattern = firstChar + '\\s' + '|' + firstChar + '$'; | ||
} else { | ||
pattern = firstChar + secondChar; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let pattern = ''; | |
if (secondChar === ' ') { | |
pattern = firstChar + '\\s' + '|' + firstChar + '$'; | |
} else { | |
pattern = firstChar + secondChar; | |
} | |
const pattern = | |
secondChar === ' ' | |
? `${firstChar}\\s|${firstChar}$' | |
: `${firstChar}{secondChar}`; |
for me, this is clearer. maybe only for me, tough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha I forget to refactor this code, Thank you for reminding me
src/actions/plugins/leap/match.ts
Outdated
return new RegExp(getPattern(searchString), getFlags()); | ||
} | ||
|
||
const MATCH_LINE_COUNT = 50; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be possible to get visible range of the document, instead of hard-coding it.
https://code.visualstudio.com/api/references/vscode-api#TextEditor
src/actions/plugins/leap/leap.ts
Outdated
previousMode!: Mode; | ||
isRepeatLastSearch: boolean = false; | ||
direction?: LeapSearchDirection; | ||
leapAction?: LeapAction; | ||
firstSearchString?: string; | ||
|
||
constructor(vimState: VimState) { | ||
this.vimState = vimState; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previousMode!: Mode; | |
isRepeatLastSearch: boolean = false; | |
direction?: LeapSearchDirection; | |
leapAction?: LeapAction; | |
firstSearchString?: string; | |
constructor(vimState: VimState) { | |
this.vimState = vimState; | |
} | |
previousMode: Mode; | |
isRepeatLastSearch: boolean = false; | |
direction?: LeapSearchDirection; | |
leapAction?: LeapAction; | |
firstSearchString?: string; | |
constructor(vimState: VimState) { | |
this.vimState = vimState; | |
this.previousMode = vimState.currentMode; | |
} |
How about this?
in visual mode |
@kabouzeid I just found this detail The x in visualization mode does not contain the match, while the s in visualization mode contains both characters of the match |
fixed the bug that in visual mode x and X don't jump to the right character
|
@cuixiaorui leap.nvim can be set to support two-way search, do you plan to support it later? |
Is there a description document for two-way search? I'll look into it. |
@cuixiaorui |
Haha |
|
I finally finished implementing this function yesterday set |
Would love to see this merged. |
package.json
Outdated
@@ -621,6 +621,31 @@ | |||
"markdownDescription": "Enable the [CamelCaseMotion](https://github.com/bkad/CamelCaseMotion) plugin for Vim.", | |||
"default": false | |||
}, | |||
"vim.leap": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this vim.leap.enabled
and change others to vim.leap.showMarkerPosition
, for instance
package.json
Outdated
@@ -1190,6 +1215,7 @@ | |||
"ts-loader": "9.4.2", | |||
"tslint": "6.1.3", | |||
"typescript": "4.9.5", | |||
"vitest": "^0.29.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is unintended?
Hi @J-Fields @elazarcoh
Please feel free to contact me if you have any questions. |
I have a suggestion, make the first match always be "j", or have a setting where user can choose what character they want to jumo to the first match point. (you know they did similar stuff in neovim ,in neovim they support command , but that's whole another thing though) |
You know what maybe make the first match the last command character |
You can set |
This PR wasn't merged yet? |
@cuixiaorui, thanks a lot for the plugin implementation. Screen.Recording.2023-06-28.at.22.48.02.mp4It's an easy fix, we just need to check for |
@cuixiaorui, the leap action is dot-repeatable, which is not desirable. Here's how we can exclude it from being marked as dot-repeatable archilkarchava@12da1ca. |
@cuixiaorui, also, I've discovered that the |
I've been daily driving this plugin for a couple of days and discovered a significant performance degradation over time when it comes to displaying the markers. Here's a demo: Before the fixvscode-vim-leap-before-the-fix.mp4After the fixvscode-vim-after-the-fix.mp4 |
I want to express my sincere gratitude for bringing these issues to my attention. Your feedback has been incredibly useful. I have addressed and resolved the problems you pointed out. The code has been updated accordingly. I truly appreciate your help in improving the quality of this project. Please feel free to check the updated code and let me know if you encounter any other issues. Your continued support and feedback are always welcome. Thank you once again for your valuable contribution. @archilkarchava |
@J-Fields is this PR okay to merge? |
Love it! For further reference and inspiration, if required, there is yet another alternative https://github.com/folke/flash.nvim, that also features remote actions, which I find rather ingenious. Remote actions work like that:
|
Soon I will implement flash-like functionality in vscodeVim! |
@cuixiaorui, thanks for your kind words. I've been using this plugin ever since and it's been a major improvement to my workflow. We can fix the issue by not assigning a label in case a search string only has a single corresponding match: archilkarchava@318fd31. Before the fixScreen.Recording.2023-10-07.at.00.08.55.mp4After the fixScreen.Recording.2023-10-07.at.00.04.07.mp4 |
Why this PR not be merged? This is a realy used plugin for vim user. |
I'd love to see the PR merged, leap is a great plugin. |
Any updates on this PR? It's been so long, and it's a much-needed plugin. |
+1 to wanting to see this merged, leap is by far my favorite nvim plugin |
Waiting for this PR merged we need leap I'm vscodevim... For now I'll use the edited extension that I forked |
Can we get this merged? This extension could really benefit from something more than easymotion. |
What this PR does / why we need it:
hi @J-Fields
I have implemented a leap functionality plugin in vscodevim
leap is currently a very popular mobile plugin in the neovim community, and it is really efficient. I believe there are many vscodevim users waiting for its arrival #8144
Functional Features
You can see how it behaves in vscodevim
Docs
You can read leap documentation for more detailed information
Compare the functional differences between leap.nvim
Because the implementation of vscodevim is different from neovim. So in some functions it is not the same as leap.nvim
Does not automatically jump to the first marker point
When matching multiple tag points, it does not automatically jump to the first tag point position.
In leap.nvim you can recognize that when you press a character that is the tag of a marker point then it will automatically jump to the marker point, otherwise it will execute the character as if it was a command in normal mode, but unfortunately this is not possible in vscodevim.
Another problem is that this feature can be confusing. Sometimes it jumps automatically and sometimes it doesn't (for example when there are multiple jump points, or you set the corresponding configuration). So I simply killed this feature in my implementation.
Instead of jumping to the first match point by default, the implementation now requires you to press an s. This s can be modified by the configuration
Of course, if only one marker is matched, then it will automatically jump over
Multiple markers are selected in the following way
In leap.nvim, if there are too many matching tokens, then the selection method is to select the corresponding group by /.The problem with this method is that if three groups are matched, then needs to be pressed twice to select them. (Of course, this is a very extreme case, so you will not encounter it)
In my implementation, I refer to easymotion's jump implementation, which starts with one letter and uses two letters if it goes beyond that, so the advantage is that no matter how many matches are made, the two letters will directly locate the marker point.Of course, another reason is that this implementation is unified with easymotion. Because some mobile scenes use easymotion. So it would be difficult to have two sets of selections. (I believe many users of vscodeVim also use easymotion, so this behavior may be more preferable for them, but I'm not sure... haha... what do you think?)
Use with operators is not supported
Unfortunately, this feature is also not supported, again because of the underlying implementation of vscodevim. Currently there is no way to do this
But this feature scenario is not used very often
Can I map other keys if I don't want to use s S?
You can refer to the following configuration
Replace s with f
How should I experience this feature before this pr merge?
You can install the experience locally based on my code branch
From time to time, I will merge new code from vscodevim into my branch until this pr is merged.
Here's how to do it:
vim.leap.enable = true
on setting.jsonThere is another easy way to download my compiled files directly
download vim-1.25.2.vsix
Issues
Conflict of S keys in visualization mode
the s/S in visualization mode contains both characters of the match
But now the problem is that S is used by the surround plugin.
How can I solve this key conflict problem? Does anyone have any good ideas?
Update
Support for using leap in visual mode
Use x / X instead of s / S on visual mode, because s/S is already taken by the surround plugin.
Support bidirectional search
set
vim.leap.bidirectionalSearch": true
enable the featureBecause I feel a little uncomfortable using ’S’. I need to reflect between ’s’ and ’S’ and ’S’ makes me press an extra ’shift’.
It also supports ’s’ and ’x’ in visual mode