Add additional options for motions #59
Conversation
doc/hop.txt
Outdated
| @@ -138,10 +138,14 @@ Most of the functions and values you need to know about are in `hop`. | |||
| Arguments:~ | |||
| {opts} Hop options. | |||
|
|
|||
| `hop.hint_char1(`{opts}`)` *hop.hint_char1* | |||
| `hop.hint_char1(`{direction}`, `{same_line}`, `{opts}`)` *hop.hint_char1* | |||
There was a problem hiding this comment.
Can you keep the anchor (right side) aligned correctly, please?
lua/hop/hint.lua
Outdated
| AFTER_CURSOR = 'after_cursor' | ||
| BEFORE_CURSOR = 'before_cursor' | ||
| ANYWHERE = 'anywhere' |
There was a problem hiding this comment.
Why do we need those constants?
plugin/hop.vim
Outdated
| @@ -12,7 +12,7 @@ command! HopWord lua require'hop'.hint_words() | |||
| command! HopPattern lua require'hop'.hint_patterns() | |||
|
|
|||
| " The jump-to-char-1 command. | |||
| command! HopChar1 lua require'hop'.hint_char1() | |||
| command! HopChar1 lua require'hop'.hint_char1(ANYWHERE, false) | |||
There was a problem hiding this comment.
I don’t think those constants are really adding value, since they are aliases to strings which value is the same as the constant name. Also, I think this line is wrong and you should be using require'hop.hint'.ANYWHERE' instead, which is not currently going to work as those constants are not added to the returned M object.
I think it’s okay to let people use literal strings here.
lua/hop/hint.lua
Outdated
| local line_too_late = hint_mode.direction == AFTER_CURSOR and line_nr + 1 < cursor_pos[1] | ||
| local line_too_early = hint_mode.direction == BEFORE_CURSOR and line_nr + 1 > cursor_pos[1] | ||
| local line_not_exact = line_nr + 1 ~= cursor_pos[1] and hint_mode.same_line | ||
| local linewise_excludes_current = line_nr + 1 == cursor_pos[1] and hint_mode.linewise | ||
| local is_invalid = line_too_early or line_too_late or line_not_exact or linewise_excludes_current |
There was a problem hiding this comment.
I don’t think the logic of the line should be handled here, since this function is going to be called for a line we know we want to hint.
Also, can you add comments explaining what these bools are for?
lua/hop/hint.lua
Outdated
| if plain_search then | ||
| pat = vim.fn.escape(pat, '\\/.$^~[]') | ||
| end | ||
| return { | ||
| direction = direction, | ||
| linewise = false, |
There was a problem hiding this comment.
I checked the rest of the PR and I don’t quite get what linewise is used for vs. same_line / one_shot.
These options include controlling which direction of the cursor motions apply to, whether they operate on entire lines, and whether the motion only applies to the current line. This branch adds the work needed to make more complicated motions possible.
Constants that are equal to their content are kind of useless in Lua so this commit removes them. Logic that checked the direction of a motion now happens earlier to better align with how the code is organized.
jhgarner
force-pushed
the
feature/new-motion-options
branch
from
406024e to
1fa77cf
Compare
|
All of the requested changes up above should be fixed in the branch now. Linewise is gone and oneshot is used instead. I also removed the constants. I thought they would be easier to debug in case the user had a typo in their config, but that doesn't seem to be true. |
There was a problem hiding this comment.
Despite the changes you made, the PR is still going in a direction I don’t think is the right one. As mentioned earlier, I think it would have been better to adapt the input instead of adapting the implementation. I will try to come up with an alternative implementation later today to compare.
| function M.by_searching(pat, plain_search) | ||
| -- pat: Vim pattern used to find matches on the screen. | ||
| -- plain_search: Whether to treat the pattern as a literal or not. | ||
| -- direction: Whether to match before the cursor, after the cursor, or |
There was a problem hiding this comment.
This is not helpful for people to use (what are the possible values?).
| @@ -53,18 +64,38 @@ end | |||
| -- | |||
| -- Used to tag words with hints. | |||
| -- M.by_word_start = M.by_searching('\\<\\w\\+') | |||
| M.by_word_start = M.by_searching('\\w\\+') | |||
| M.by_word_start = M.by_searching('\\w\\+', 'anywhere', false) | |||
There was a problem hiding this comment.
Using strings as enumerations is not a good idea. You shouldn’t have removed the constants (I was wondering what they were used for, but now I understand what you wanted to do). Instead, you should have gone an enum way.
| else | ||
| hints[i] = { | ||
| hints = {}; | ||
| length = 0 | ||
| } |
There was a problem hiding this comment.
Yeah, see, this is why I don’t like the design you suggest. You modified the already existing abstractions and implementations to bend them to do what they were not meant to be used for. This logic will be applied to lots of lines for nothing.
This first commit allows to apply the hints to only the previous part of the cursor or what comes after. This commit doesn’t currently check the column, only the line. The next commit will add support for before / after on the current line and should also add support for a way to reduce apply hints only to the current line. This is an alternative implementation of #59. Relates to #71, #91.
|
Thank you for your contribution. As discussed in the content of the PR, unfortunately, I wasn’t convinced with the implementation and was thinking of something re-using the already implemented features, adapting the inputs instead of altering too much the already existing functions. Some misunderstanding about the “constants” vs. “strings”, too, I guess here was miscommunication (using strings as a mean of variants of sum types is not something I want and I didn’t understand what you wanted to do). Superseded by #93. |
These options include controlling which direction of the cursor motions
apply to, whether they operate on entire lines, and whether the motion
only applies to the current line.
This branch adds the work needed to make more complicated motions
possible.