ggandor / leap-spooky.nvim

👻 Actions at a distance
The Unlicense
276 stars 8 forks source link

Add auto-targeting (Requiring mini.ai) #12

Closed Gazareth closed 1 year ago

Gazareth commented 1 year ago

auto_targets is a new config field. When auto-targets is set to true, mini.ai will be used to search for the appropriate text object region(s) and bring up leap targets automatically, rather than requiring the user to start typing first

Also:

Resolves #6 and Resolves #11

ggandor commented 1 year ago

This seems very interesting, thanks, I will have a deeper look at it ASAP.

A couple of remarks: Your fork doesn't have the latest fix 0bb68f0, please rebase.

This PR addresses 4 (3.5) completely different things:

Reduced repetition in defining default text objects

Over-engineering. (For 5 prefixes this refactoring would be a net gain, but here it's borderline ugly.)

Ability to add custom text objects

Valid request, but let's work on it in a separate PR.

Reorders the construction of the key mappings so that remote scope (i.e. "r", "M" etc) immediately follows the operator (so keymaps become e.g. yraw rather than yarw)

Doesn't belong to this PR, and see https://github.com/ggandor/leap-spooky.nvim/issues/6#issuecomment-1453856390 why I don't support it.

Regarding the auto-target feature, this looks promising, but there are a couple of things missing or problematic. rr is broken, for example, errors are not handled properly (e.g. if mini-ai is not installed), magic 50 for n_lines, documentation is not updated, just to name a few.

A bigger problem is that mini-ai doesn't find nested objects, only top-level ones.

Gazareth commented 1 year ago

I appreciate I've bundled too much in at once here and I'll look to create some separate PRs.

Wanted to get your opinions on the way I've gone about things before updating the docs since nothing is set in stone yet as well.

Gazareth commented 1 year ago

Over-engineering. (For 5 prefixes this refactoring would be a net gain, but here it's borderline ugly.)

I admit it's a bit clunky, but I'm also thinking of #7, which adds another whole set of the same strings of text objects.

I think the ideal situation would be if each text object was defined as a table, like so:

local default_text_objects{
  {
    base_def = "w",
    i_desc = "inner word",
    a_desc = "'a word (with white space)"
  }
...
}

Or something along these lines. This way it means the actual text object is only specified once, reducing potential for human error. The same formula could then be followed for user-defined custom text objects, which people will be writing in their own configs, again, with less chance for human error.

Gazareth commented 1 year ago

I've also discovered there are performance issues [edit: 2-3 seconds search duration] when searching for text objects like "w" and "W" (which obviously appear with great density in any portion of code). I'm having a look at mini.ai at the moment to see if there's a way around that.

For now I will close this PR as it does have a lot of issues, but I hope to return at some point as I really feel this sort of behaviour would be a differentiating feature to have in neovim.

ggandor commented 1 year ago

I've also discovered there are performance issues when searching for text objects like "w" and "W" (which obviously appear with great density in any portion of code).

There's not much sense in auto-targeting words when we have Leap's clever 2-char method instead (that is why we don't have such a feature in the first place, as opposed to EsasyMotion/Hop), so I don't really care. The big problem is blocks (( )).