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

fix: use abs() so distance is always non-negative #70

Closed
wants to merge 1 commit into from

Conversation

stevenxxiu
Copy link
Contributor

Checklist

  • Used only camelCase variable names.
  • If functionality is added or modified, also made respective changes to the readme.
  • Used conventional commits keywords.

I'm not entirely sure what the existing code's condition is doing. For the following example, where I press yie, it gives a* *b, instead of b. If it just matches the pattern closest to the cursor, it works.

*a* *|b*

(| shows the cursor position.)

Using vim.fn.abs() appears to fix this. Does this make sense?

Of course, ideally a* *b would never be matched as the contents of an emphasis anyway, but I don't think Lua patterns are powerful enough for this.

@chrisgrieser
Copy link
Owner

chrisgrieser commented Jan 6, 2024

That's an approach I initially tried out, but it does not work. There is the case where you want to compare two text objects:

  1. The cursor is standing on one big textobj (= high negative distance)
  2. The cursor is right in front of a text objects (= small positive distance)

Comparing just the absolute value of the distance will in this case result in textobj 2 being selected as the closest one, even though the cursor is standing on a text object. (And the idiomatic behavior of vim is to only do forward seeking of text objects when not already standing on one.)

That one was a bit tricky to correctly figure out myself. I guess it does not hurt to add this as a comment, since it's not obvious on first look.

@stevenxxiu stevenxxiu deleted the fix/abs-distance branch January 6, 2024 16:04
@chrisgrieser
Copy link
Owner

chrisgrieser commented Jan 6, 2024

I think a* *b being matched is not an issue with the selectTextobj, but rather an issue with the patterns for the mdEmphasis object. I think the pattern matches * * as it is a string enclosed by two *.

As opposed to brackets, * does not differentiate between closing and opening character, so it's not possible to implement the textobj 100% correct without using something like treesitter's AST. If I am not mistaken, best you can do is to have a look whether tweaking the pattern of the mdEmphasis object reduces the number of false matches.

@stevenxxiu
Copy link
Contributor Author

stevenxxiu commented Jan 6, 2024

Hm what about this method?

  • If the cursor is on a textobj, then select the one whose starting point is as close to it as possible.
  • If the cursor isn't on a textobj, then select the one whose starting point is as close to it as possible.

That will fix it in my case, when Lua patterns aren't expressive enough.

Using Tree-sitter AST would be ideal. I haven't looked into that and am not sure how I'd get that working.

@chrisgrieser
Copy link
Owner

chrisgrieser commented Jan 6, 2024

If the cursor is on a textobj, then select the one whose starting point is as close to it as possible.
If the cursor isn't on a textobj, then select the one whose starting point is as close to it as possible.

That is pretty much what the current code is doing 😉

@stevenxxiu
Copy link
Contributor Author

stevenxxiu commented Jan 6, 2024

Not quite, as both of these are text objects that match:

  • *a* *|b*
  • *|b*

The second one is closer to the cursor, but the first one gets matched instead.

@chrisgrieser
Copy link
Owner

you haven't indicated a cursor position, so it's not possible to follow your example

@stevenxxiu
Copy link
Contributor Author

I've updated my comment.

@chrisgrieser
Copy link
Owner

chrisgrieser commented Jan 6, 2024

I just gave it a try with the anyQuote and anyBracket textobjects, since they are the one's most similar to the mdEmphasis object.

local a = "1 + 2222" .. "|1111 + 2"
local b = (1 + 2222) .. (|1111 + 2)

In both these cases, it matches correctly the second object. So I assume it's some issue with the patterns of mdEmphasis?

@stevenxxiu
Copy link
Contributor Author

stevenxxiu commented Jan 6, 2024

I don't know of a way to avoid matching *a* *b*. In normal regex I can just do something like (I think it covers all the cases):

/(?<!\\)\*([^\]|\\.)+\*/

But Lua doesn't offer this.

The current patterns of mdEmphasis match both *a* *b* and *b*.

The only solution I can think of is to match the one closest to the cursor.

Or of course there's also Tree-sitter which is the most ideal.

@chrisgrieser
Copy link
Owner

chrisgrieser commented Jan 6, 2024

yeah, lua patterns are somewhat limited. You could maybe have try whether maybe something with frontier patterns work, since they are lua's simplified version of lookbehinds.

The only solution I can think of is to match the one closest to the cursor.

As I said, this is pretty much what the current code is doing, considering various cases for comparing textobj standing on or not standing on an obj. And it works for me on other text objects. I want to avoid modifying the core logic if it does not solve a problem multiple textobjects have.

Or of course there's also Tree-sitter which is the most ideal.

yeah, this plugin isn't really intended for treesitter textobjects, since nvim-treesitter-textobj has all the logic for that. So I guess I'd also consider making a FR/PR over there.

@stevenxxiu
Copy link
Contributor Author

yeah, this plugin isn't really intended for treesitter textobjects, since nvim-treesitter-textobj has all the logic for that. So I guess I'd also consider making a FR/PR over there.

Well I did ask in nvim-treesitter/nvim-treesitter-textobjects#547 with no reply yet.

I want to avoid modifying the core logic if it does not solve a problem multiple textobjects have.

It does seem like a bug to me though, if the patterns of mdEmphasis match both *a* *b* and *b*, yet *a* *b* is selected.

@chrisgrieser
Copy link
Owner

It does seem like a bug to me though, if the patterns of mdEmphasis match both a b and b, yet a b is selected.

yes, and if the bug only occurs with mdEmphasis, that's a strong indicator that the bug lies with mdEmphasis's code?

@stevenxxiu
Copy link
Contributor Author

stevenxxiu commented Jan 6, 2024

yes, and if the bug only occurs with mdEmphasis, that's a strong indicator that the bug lies with mdEmphasis's code?

mdEmphasis includes patterns that match both *a* *b* and *b*. No matter what the patterns are, if *a* *b* ends up being selected instead of *b*, wouldn't this be a bug in selectTextObj()?

The bug only occurring in mdEmphasis could just mean that other code don't have patterns that trigger the bug.

@chrisgrieser
Copy link
Owner

chrisgrieser commented Jan 6, 2024

mdEmphasis includes patterns that match both *a* *b* and *b*

Sounds to me like an issue with the patterns. *a* *b* should not be matched to begin with, since that's not one but two emphases.

The code of selectTextobj assumes that every found textobj is a valid one. It is not intended to be used to select a textobject out of an invalid and a valid object.

@stevenxxiu
Copy link
Contributor Author

stevenxxiu commented Jan 6, 2024

Sounds to me like an issue with the patterns. *a* *b* should not be matched to begin with, since that's not one but two emphases.

You're right, but I don't think Lua has regex groups, that's required in this case.

The code of selectTextobj assumes that every found textobj is a valid one. It is not intended to be used to select a textobject out of an invalid and a valid object.

You're right as well, however given two textobjects, it's not choosing the closer one here. I believe that's what it's designed to do. If it does that, then the patterns should work out.

@chrisgrieser
Copy link
Owner

chrisgrieser commented Jan 6, 2024

yeah, selectTextobj works, because valid textobjects that overlap usually do not share the same border:

  • aa(b(c)b)aaaa has two text objects, but both their starting and end position are not the same. That is normally always the case for overlapping textobjects and therefore an assumption that selectTextobj makes to simplify its code.
  • *a* *b* has, when you factor in the "wrong" textobjects, overlapping textobjects that share the same border (*a* *b* and *b* both have the last * as end position). I think that's why the selection of the "closest" one fails.

So yes, strictly speaking, selectTextobj only selects the closest object under the assumption that overlapping candidates do not share a border. However, that is usually the case if all candidates are valid textobjs, so it's not a problematic assumption in my view.

In theory, it is possible to modify selectTextobj in such a way that it can also handle "invalid" textobjects correctly. However, with the experience I have from creating a bunch of plugins, I am generally opposed to complicating core logic to accommodate fringe cases, since that makes maintainability harder in the long run.

@stevenxxiu
Copy link
Contributor Author

I am generally opposed to complicating core logic to accommodate fringe cases, since that makes maintainability harder in the long run

That makes sense, however in the current case I can't see another easy solution. The only other solutions I can think of is to use tree-sitter, or use a proper regex engine.

I don't think handling this case would really add much code.

@chrisgrieser
Copy link
Owner

So one option would be to utilize selectTextobj as is, and make some corrections to the selection afterward, similar to the value textobj. This way the core logic is not touched.

If you can find a fix for selectTextobj that would cover your case, and it is simple and also does not break existing behavior (like the use of an absolute value would have done), I am not totally opposed to it. But I dunno, it would still introduce an extra requirement to selectTextobj to pay attention to, if I wanna improve it in the future.

Thinking about it, and considering all the edge cases that exist for mdEmphasis (having the highest number of patterns already), using Treesitter would probably be a simpler solution. You can take a look at how the pyDocstring does it, it's the only textobj from this plugin that utilizes treesitter instead of patterns.

@stevenxxiu
Copy link
Contributor Author

Thanks for the comments. I thought about the other options:

  • If I just use selectTextobj once, I don't have the ranges for every pattern, so I can't find the correct one.
  • Using Tree-sitter means I can't use this in say doc comments.

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.

2 participants