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(plugins): add flash.nvim to replace clever-f.vim #926

Merged
merged 11 commits into from
Nov 1, 2023

Conversation

YuCao16
Copy link
Contributor

@YuCao16 YuCao16 commented Aug 2, 2023

#494 (comment)

  1. In core/mapping.lua, I wrote a function for users who don't have flash.nvim installed.
  2. I only bind partial keymap available in flash.nvim in keymap/editor.lua, because I use the default s quite often.

@Jint-lzxy Jint-lzxy changed the title add plugin flash.nvim feat: add flash.nvim Aug 2, 2023
@YuCao16
Copy link
Contributor Author

YuCao16 commented Aug 2, 2023

I'm not sure If I should remove cleverf, because it involves delete&replace it's keymap. I hope maintainer could offer some help and advise.

@Jint-lzxy
Copy link
Collaborator

IMO we may need to do some other cleanups as several plugins will be deprecated due to this PR, and their keymaps should bind to this new integration.

@Jint-lzxy Jint-lzxy marked this pull request as draft August 2, 2023 15:55
@Jint-lzxy Jint-lzxy changed the title feat: add flash.nvim feat(plugins): add flash.nvim Aug 2, 2023
@YuCao16
Copy link
Contributor Author

YuCao16 commented Aug 2, 2023

21748b2: remove clever-f should work, it's keymap seems equal to flash.nvim's default.

https://github.com/folke/flash.nvim/blob/967117690bd677cb7b6a87f0bc0077d2c0be3a27/lua/flash/plugins/char.lua#L117C1-L117C1

@fecet
Copy link
Contributor

fecet commented Aug 2, 2023

What's the reason to add this, it doesn't do more than hop and treehopper

@YuCao16
Copy link
Contributor Author

YuCao16 commented Aug 2, 2023

What's the reason to add this, it doesn't do more than hop and treehopper

You can check out it's README.md. In short, it combines all three plugins (include clever-f) and enhances /. Isn't it worth it? :D

@Jint-lzxy
Copy link
Collaborator

Jint-lzxy commented Aug 2, 2023

What's the reason to add this, it doesn't do more than hop and treehopper

After scanning through the introduction of this plugin, one of the most significant benefits that came to my mind was it integrates all motion commands together, which signifies a high probability of simplifying our code. But I haven't checked its implementation detail yet - looks like according to #494 (reply in thread), it also has the common issue(s) of those 'highly-integrated' plugins - being too intrusive. If this is the case, IMO we can change the purpose this PR to 'use lua alternatives for those vimscript plugins' (e.g., use leap.nvim and flit.nvim), which does a very similar thing.

@YuCao16
Copy link
Contributor Author

YuCao16 commented Aug 2, 2023

I agree, but I'm only using some of the basic features of comments.nvim. Can you explain how it conflicts with flash.nvim? Because it seems to me that I only defined S, and clever-f also defines f and F.

@fecet
Copy link
Contributor

fecet commented Aug 2, 2023

I support to use flash to replace clever-f and then users can freely choose whether to use it as a replacement for hop.

IMO hop, tree-hopper and incremental-selection covers the functionalities of flash very well and they are already in this repo. I'm also implementing folke/flash.nvim#52 on the top of TS.

And #494 (comment) cmp also enhance /

@YuCao16
Copy link
Contributor Author

YuCao16 commented Aug 2, 2023

I support to use flash to replace clever-f and then users can freely choose whether to use it as a replacement for hop.

They had the advantage over each other and I didn't remove hop.nvim, even though I use flash.nvim more often.

@Jint-lzxy
Copy link
Collaborator

I support to use flash to replace clever-f and then users can freely choose whether to use it as a replacement for hop.

Hmm FWIW this way the drawbacks of this plugin outweigh the potential benefits it may bring. I mean, why do we need to replace a single working plugin with a highly-integrated one? We can add wildfire.vim for custom textobjects and have each plugin do their respective functionality, which will also reduce maintenance pressure as no plugin will experience way faster iterations.

And #494 (comment) cmp also enhance /

Ah thanks for providing a working example! I'm working on this, trying to cover some cornercases 😄

@YuCao16
Copy link
Contributor Author

YuCao16 commented Aug 2, 2023

I support to use flash to replace clever-f and then users can freely choose whether to use it as a replacement for hop.

IMO hop, tree-hopper and incremental-selection covers the functionalities of flash very well and they are already in this repo. I'm also implementing folke/flash.nvim#52 on the top of TS.

And #494 (comment) cmp also enhance /

This is a little bit different IMO, flash.nvim also support fast jump in search mode, however, cmp and other plugins don't.

@YuCao16
Copy link
Contributor Author

YuCao16 commented Aug 2, 2023

I support to use flash to replace clever-f and then users can freely choose whether to use it as a replacement for hop.

Hmm FWIW this way the drawbacks of this plugin outweigh the potential benefits it may bring. I mean, why do we need to replace a single working plugin with a highly-integrated one? We can add wildfire.vim for custom textobjects and have each plugin do their respective functionality, which will also reduce maintenance pressure as no plugin will experience way faster iterations.

And #494 (comment) cmp also enhance /

Ah thanks for providing a working example! I'm working on this, trying to cover some cornercases smile

I've been using cmp-cmdline, because sometimes lsp integrate is needed in cmdline. but wilder.nvim's statusline is quite useful ...

@Jint-lzxy
Copy link
Collaborator

This is a little bit different IMO, flash.nvim also support fast jump in search mode, however, cmp and other plugins don't.

Maybe we can only enable this specific feature? b/c we'll probably load flash.nvim at { "CursorHold", "CursorHoldI" }, this way, its setup will unlikely to result in performance overhead.

@Jint-lzxy
Copy link
Collaborator

but wilder.nvim's statusline is quite useful ...

Yeah I'm trying to address this issue (if we can collect and import these missing items using external sources)

@YuCao16
Copy link
Contributor Author

YuCao16 commented Aug 2, 2023

Maybe we can only enable this specific feature?

You mean f, F and / or only f and F?

@Jint-lzxy
Copy link
Collaborator

Jint-lzxy commented Aug 2, 2023

You mean f, F and / or only f and F?

I mean we can only enable "fast jump in search mode". This way we can avoid introduing breaking changes - if users do want to activate some of flash's features, they can enable it themselves. To me, I tend to distribute functionalities to as many plugins as possible, as this reduces maintenance pressure by eliminating the need to frequently update a specific config (file). But this is just a matter of taste :) If a plugin can bring about better experience then, why not?

@YuCao16
Copy link
Contributor Author

YuCao16 commented Aug 2, 2023

You mean f, F and / or only f and F?

I mean we can only enable "fast jump in search mode". This way we can avoid introduing breaking changes - if users do want to activate some of flash's features, they can enable it themselves. To me, I tend to distribute functionalities to as many plugins as possible, as this reduces maintenance pressure by eliminating the need to frequently update a specific config (file). But this is just a matter of taste :) If a plugin can bring about better experience then, why not?

I agree, as if we should replace hop.nvim by flash.nvim is not my concern, everyone has their own preferences. For introducing this plugin:

  1. clever-f has some bug, sometimes highlight won't clear.
  2. flash.nvim offers fast jump in search mode.

@aarnphm
Copy link
Collaborator

aarnphm commented Aug 2, 2023

I tend to agree with @Jint-lzxy, maybe we can introduce flash.nvim gradually, first for just f, and F. For search mode I think telescope should be more than enough

@YuCao16
Copy link
Contributor Author

YuCao16 commented Aug 2, 2023

For search mode I think telescope should be more than enough

247490513-e0ac4cbc-fa54-4505-8261-43ec0505518d.png

I feel like I'm the author of 'flash.nvim' :D, but seems telescope can't do this.

@Jint-lzxy
Copy link
Collaborator

clever-f has some bug, sometimes highlight won't clear.

Hmm I haven't encountered this issue before, but clever-f does clear its highlight after some time. Is this related to ur issue?

I feel like I'm the author of 'flash.nvim' :D, but seems telescope can't do this.

Yeah that's why I said "we can only enable 'fast jump in search mode'"

@YuCao16
Copy link
Contributor Author

YuCao16 commented Aug 2, 2023

Hmm I haven't encountered this issue before, but clever-f does clear its highlight after some time. Is this related to ur issue?

I kind of forgot how to reproduce that bug, if I finish the JUMP then there is no highlight at that point. But when I press ; again (without pressing f), the highlight doesn't get cleared.

@vollowx
Copy link
Contributor

vollowx commented Aug 2, 2023

Another thing, as flash takes place of statusline, and it's prompt is configurable, we can make it look like lualine section a, maybe better.

@YuCao16
Copy link
Contributor Author

YuCao16 commented Aug 2, 2023

Another thing, as flash takes place of statusline, and it's prompt is configurable, we can make it look like lualine section a, maybe better.

Good point, I will take a look in the morning.

@CharlesChiuGit
Copy link
Collaborator

CharlesChiuGit commented Aug 7, 2023

Before I read the readme of flash.nvim, I was leaning toward Jint's side, which is distribute functionalities to each plugins, since we already had the experience of lspsaga.

But after reading the readme, I found it was THE motion plugin I ever wanted an year ago! If flash.nvim was created at that time, I think I will go all in on this.

Considering there might be lots of ppl using the repo, I think it would be better to replace one functionality/plugin at each release, as aarnphm implied.

Not sure about the status line thingy tho, I didn't look super deep into flash.nvim.

@Jint-lzxy
Copy link
Collaborator

Jint-lzxy commented Aug 7, 2023

Considering there might be lots of ppl using the repo, I think it would be better to replace one functionality/plugin at each release, as aarnphm implied.

Also quite in favor of this (as per #926 (comment)) - we can gradually complete such transition like what we did in #920 and #921. Cause from the user's perspective, those individual PRs only adds a few files, along with several trivial (and "conflict-friendly") changes that are easy to adopt. This can also effectively avoid breaking changes (which can be quite annoying in some cases).

@ayamir
Copy link
Owner

ayamir commented Sep 30, 2023

Maybe it's time to promote this PR @YuCao16 ?

@YuCao16
Copy link
Contributor Author

YuCao16 commented Oct 2, 2023

Maybe it's time to promote this PR @YuCao16 ?

Yes, give me sometime.

@YuCao16
Copy link
Contributor Author

YuCao16 commented Oct 21, 2023

Sorry for the long delay, what other changes do I need to make before merge? @Jint-lzxy @ayamir

@ayamir
Copy link
Owner

ayamir commented Oct 22, 2023

It's undoubted that flash.nvim's f/F/t/T function can replace clever-f.vim. But its other functions should be discussed if we want to replace more existing plugins.
IMO the jump function is enough to replace HopWord. But the search and jump function can't replace wilder.nvim because it doesn't support file level. Its remote function is wired for me and I can't understand its value.

@YuCao16
Copy link
Contributor Author

YuCao16 commented Oct 23, 2023

It's undoubted that flash.nvim's f/F/t/T function can replace clever-f.vim. But its other functions should be discussed if we want to replace more existing plugins.
IMO the jump function is enough to replace HopWord. But the search and jump function can't replace wilder.nvim because it doesn't support file level. Its remote function is wired for me and I can't understand its value.

My use case is that flash.nvim is a perfect replacement for clever-f, can be used with wilder, but replacing hop requires a bit of a learning curve. I don't even use remote, we don't need to configure all the features of a plugin.

Signed-off-by: ayamir <lgt986452565@gmail.com>
@ayamir
Copy link
Owner

ayamir commented Oct 24, 2023

IMO it's better to leave the decision to users. It looks good to me now.

@ayamir ayamir marked this pull request as ready for review October 24, 2023 02:53
lua/core/mapping.lua Outdated Show resolved Hide resolved
Copy link
Collaborator

@Jint-lzxy Jint-lzxy left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. This PR LGTM now 😸 I explicitly disabled flash.nvim's search mode integration (but kept the related keymap to facilitate user customization), b/c IMHO this may not be the point that some users expect to change, and there're compatibility issues with wilder.nvim (correct me if I'm wrong :D)

@Jint-lzxy Jint-lzxy changed the title feat(plugins): add flash.nvim feat(plugins): add flash.nvim to replace clever-f.nvim Oct 30, 2023
@Jint-lzxy Jint-lzxy changed the title feat(plugins): add flash.nvim to replace clever-f.nvim feat(plugins): add flash.nvim to replace clever-f.vim Oct 30, 2023
@YuCao16
Copy link
Contributor Author

YuCao16 commented Oct 30, 2023

Sorry for the delay. This PR LGTM now 😸 I explicitly disabled flash.nvim's search mode integration (but kept the related keymap to facilitate user customization), b/c IMHO this may not be the point that some users expect to change, and there're compatibility issues with wilder.nvim (correct me if I'm wrong :D)

I agree, LGTM, thanks for your help!

@ayamir ayamir merged commit e6a2d0a into ayamir:main Nov 1, 2023
2 checks passed
singlemancombat pushed a commit to singlemancombat/nvim-config that referenced this pull request Nov 3, 2023
* disable enforce_regular_tabs to fix unique name not working

* add flash.nvim

* disable multi_window

* fixup!: remove comment lines in keymap, add NOTEs for users.

Signed-off-by: ayamir <lgt986452565@gmail.com>

* move flash_ecs to keymap/helpers.lua and rename to _flash_esc

* feat(flash): support user-override

* pref: explicitly disable `flash.search` by default

* chore: sort keymap helpers

* fix typo

* fixup! fix typo

---------

Signed-off-by: ayamir <lgt986452565@gmail.com>
Co-authored-by: ayamir <lgt986452565@gmail.com>
Co-authored-by: Jint-lzxy <50296129+Jint-lzxy@users.noreply.github.com>
ttbug pushed a commit to ttbug/nvimconf that referenced this pull request Nov 9, 2023
* disable enforce_regular_tabs to fix unique name not working

* add flash.nvim

* disable multi_window

* fixup!: remove comment lines in keymap, add NOTEs for users.

Signed-off-by: ayamir <lgt986452565@gmail.com>

* move flash_ecs to keymap/helpers.lua and rename to _flash_esc

* feat(flash): support user-override

* pref: explicitly disable `flash.search` by default

* chore: sort keymap helpers

* fix typo

* fixup! fix typo

---------

Signed-off-by: ayamir <lgt986452565@gmail.com>
Co-authored-by: ayamir <lgt986452565@gmail.com>
Co-authored-by: Jint-lzxy <50296129+Jint-lzxy@users.noreply.github.com>
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.

7 participants