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

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

Closed
wants to merge 1 commit into from

Conversation

Gazareth
Copy link

@Gazareth Gazareth commented Mar 2, 2023

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:

  • Reduced repetition in defining default text objects
  • Ability to add custom text objects
  • 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)

Resolves #6 and Resolves #11

When auto-targets is toggled on, mini.ai will be used to search appropriate text object region and provide targets automatically, rather than waiting for the user to start typing
@Gazareth Gazareth changed the title Add auto-targeting (Requires mini.ai) Add auto-targeting (Requiring mini.ai) Mar 2, 2023
@ggandor
Copy link
Owner

ggandor commented Mar 3, 2023

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 #6 (comment) 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
Copy link
Author

Gazareth commented Mar 3, 2023

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
Copy link
Author

Gazareth commented Mar 3, 2023

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
Copy link
Author

Gazareth commented Mar 16, 2023

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.

@Gazareth Gazareth closed this Mar 16, 2023
@ggandor
Copy link
Owner

ggandor commented Mar 16, 2023

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 (( )).

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.

[Feature request] labels on text objects themselves [feature request] create operator like yr dr cr etc.
2 participants