nvim-orgmode / orgmode

Orgmode clone written in Lua for Neovim 0.9+.
https://nvim-orgmode.github.io/
MIT License
2.8k stars 121 forks source link

fix: correct emphasis text objects like builtin i" & a" #695

Closed PriceHiller closed 1 month ago

PriceHiller commented 2 months ago

This PR fixes the text object motions for emphasis markers to work EXACTLY like vim's quote inner and around selections.

This PR also adds tests for those emphasis markers.

Let me know if you want me to Squash my separate test and fix commits together.

Of note, maybe it would be nicer to have the Treesitter parser pick up on these emphasis regions. The code I wrote to locate the pairs of emphasis markers was a true pain and would've been trivialized if the syntax tree TS had also included the emphasis markers as an object.

For now though, this works quite well actually.

Closes #686

PriceHiller commented 2 months ago

I tested it and it works as expected, but the code seems too complex. I still didn't go deep into it to understand it. Can't we use what we have now, with an additional step to jump to the first marker if there is some after the cursor, and none before the cursor? Example code (not tested, | is cursor position):

I agree the code is complex, if we had those emphasis regions as part of Treesitter's syntax tree it would've been much simpler to do.

The approach you detailed was the initial approach I took until I kept comparing it to how quote inner and around selections work. I then wanted to cry. It won't work perfectly.

I'm trying to figure out how to explain this without sounding positively deranged lol. I have to pop out for a while, but I'll address everything later and try to simplify the code some. If I can't then I'll write up something that hopefully doesn't make me look like this.

A quick note, it seems the textobjects builtin to vim are actually doing some tokenization of the elements like this PR. Definitely more than just t,T,f,F motions going on.

kristijanhusak commented 2 months ago

I agree the code is complex, if we had those emphasis regions as part of Treesitter's syntax tree it would've been much simpler to do.

It would be drastically simpler, but it adds a lot of complexity to the parser. If ephemeral extmarks could be retrieved, you could leverage finding extmarks on the line that are added through the markup highlighter here, but that's not the case unfortunately.

The approach you detailed was the initial approach I took until I kept comparing it to how quote inner and around selections work. I then wanted to cry. It won't work perfectly. I'm trying to figure out how to explain this without sounding positively deranged lol. I have to pop out for a while, but I'll address everything later and try to simplify the code some. If I can't then I'll write up something that hopefully doesn't make me look like this.

Don't get me wrong, I appreciate the contributions a lot, but I'm trying to simplify things as much as possible so it's easier to maintain. If we cannot go with the simpler solution, that is perfectly fine. I just want to double-check.

A quick note, it seems the textobjects builtin to vim are actually doing some tokenization of the elements like this PR. Definitely more than just t,T,f,F motions going on.

I didn't check how internals work on this. I'm judging this based solely on testing with quotes and double quotes. quotes-motion

From the functionality standpoint, it behaves very similar to the simple implementation we have now, with the exception of finding the first marker.

PriceHiller commented 2 months ago

Don't get me wrong, I appreciate the contributions a lot, but I'm trying to simplify things as much as possible so it's easier to maintain. If we cannot go with the simpler solution, that is perfectly fine. I just want to double-check.

I 100% agree. Hope I didn't come off as dismissive/defensive or something 😅. I'm always open to critique, only way to learn. I intend to support any contribution I make, so the simpler the better as it will hopefully save me time in the long run.

From the functionality standpoint, it behaves very similar to the simple implementation we have now, with the exception of finding the first marker.

I just force pushed an update to the tests. If the tests fail, then the implementation does not 100% match with how vim's built-in text objects work.

The new tests are going character by character across a line of text and running a given text object operation like da* and then comparing it (with a bit of string replacements) to the output of da".

If the two lines of text from both operations are the same, then the text object implementation is correct. The around text objects have some additional behavior with whitespace that the inner text objects do not.

I'm going take the code I have and rework it until I satisfy the above tests hopefully with some simplification along with it 😉. The current code is a bit hard to grok even for me the more attention I give to resolving this.

PriceHiller commented 1 month ago

I'm going to close this because I don't have an intention on working on this within the next few weeks and I don't want to clog up the open PRs.

However, I do want to come back to this when I have the time to do so.