fix: correct emphasis text objects like builtin i" & a" #695
Conversation
PriceHiller
changed the title
Resolves: nvim-orgmode#686 TODO: Tests
PriceHiller
force-pushed
the
fix/cursor-before-text-objs
branch
from
43f90f0
to
0f74b82
Compare
There was a problem hiding this comment.
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):
Line: Th|is is *bold* text
Code (using * as marker):
local cursor = vim.fn.col('.')
local line = vim.api.nvim_get_current_line()
local line_to_cursor = line:sub(1, cursor)
local line_after_cursor = line:sub(cursor + 1)
if line_to_cursor:find('%*') then
-- There is marker before the cursor, do what we do now
vim.cmd('normal! T*vt*')
else
local first_marker_after_cursor = line_after_cursor:find('%*')
if first_marker_after_cursor then
-- Go to the marker
vim.cmd('normal! f*')
-- Move cursor one column right after marker so the same command picks it up
vim.cmd('normal! l')
-- Do the same as before
vim.cmd('normal! T*vt*')
end
endThis code does not consider that cursor is at the marker, but I think you get the general idea.
I don't think we need to make it super smart, just to handle the case where you are at the beginning of the line, and having motion pick up the first one.
ftplugin/org.lua
Outdated
| 'x', | ||
| 'i' .. char, | ||
| ([[:<C-u>silent! lua require('orgmode.org.text_objects').select_nearest_token_pair('%s')<CR>]]):format(char), | ||
| { buffer = true } |
There was a problem hiding this comment.
Add silent = true here also. It prints the command without it.
Same thing for the mapping below.
| ---@param strict? boolean Should not allow reuse of end token from previous pair for new pair, default: true | ||
| ---@param init? integer Starting position to search from in the string, default: 1 | ||
| ---@return PairedTokenLocations locations The paired token locations | ||
| function TextObjects.get_paired_token_locations(str, token, strict, init) |
There was a problem hiding this comment.
This function is used only once, and strict is always true. Could we inline it?
There was a problem hiding this comment.
Sure, I separated it because I felt it was a bit easier to understand and thought it might be applicable elsewhere.
I can inline it when I get back.
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 |
It would be drastically simpler, but it adds a lot of complexity to the parser. If
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 didn't check how internals work on this. I'm judging this based solely on testing with quotes and double quotes. From the functionality standpoint, it behaves very similar to the simple implementation we have now, with the exception of finding the first marker. |
PriceHiller
force-pushed
the
fix/cursor-before-text-objs
branch
from
0f74b82
to
6d308b3
Compare
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.
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 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
force-pushed
the
fix/cursor-before-text-objs
branch
from
6d308b3
to
27c5ba6
Compare
|
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. |
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