Keep labels unchanged on first press of the repeat key

[Subjective]

Hi, I'm really liking the new repeat key functionality you implemented in 108222e, but I noticed that the labels change the first time the repeat key is pressed after "leaping". Subsequent presses of the repeat key don't change the labels.

I'm assuming this is not the intended behavior? To me, it makes more since for the labels to either always remain constant, or be updated on every repeat so that the nth leapable match can always be jumped to with with the same keystroke.

Thanks for the great plugin!

[Subjective]

Another issue I just noticed is that the repeat keys still seem to be active even when you are no longer in the context of leaping. This leads to some weird behavior when using repeat keys with flit (with multiline disabled), as leap's repeat will take over and cause the cursor to jump to the next occurrence of the previous leap pattern.

[ggandor]

I'm assuming this is not the intended behavior?

the repeat keys still seem to be active even when you are no longer in the context of leaping.

These keys repeat the last motion directly, the intended use case is doing a ;;;; streak, not s{ch}{ch};;; (for that, there is next_target/prev_target). If you press ; after s{ch}{ch}, it actually exits leap, and starts a new "repeat" call.

IMO there is not much value in s{ch}{ch};;;, that is the very purpose of showing labels (ahead of time), so that you don't need to follow the cursor jumping from match to match, or count in your head. But ;;; is a different case, then it is easier to just continue pressing ; instead of context switching.

I noticed that the labels change the first time the repeat key is pressed after "leaping". Subsequent presses of the repeat key don't change the labels.

That said, if this annoys you a lot, you can set up an autocommand that maps ;/, as next/prev-target spec. keys after LeapPatternPost (so that they won't conflict with the search pattern), and removes them on LeapLeave. (add_repeat_mappings does exactly the same, but only for repeat calls, copy that with the obvious modifications, or just ask for help if needed, I'll write it.)

This leads to some weird behavior when using repeat keys with flit (with multiline disabled), as leap's repeat will take over and cause the cursor to jump to the next occurrence of the previous leap pattern

Hmm, it shouldn't... in flit, ;/, are mapped as next_target/prev_target by default, that should take priority. It has nothing to do with multiline. Or you mean you want ;/, to function as repeat keys for Flit or Leap, depending on which one was the last motion?

[ggandor]

Off-topic: what is the colorscheme? Looks nice :)

[Subjective]

Thanks for the thorough reply, it was really insightful.

the intended use case is doing a ;;;; streak, not s{ch}{ch};;; (for that, there is next_target/prev_target).

Oh wow, I didn't even realize next_target and prev_target were a thing. Even so, my OCD was killing me so I decided to implement your suggested fix. I'm not really that experienced with autocmds, but this is what I came up.

-- edit: this somehow breaks highlighting for unlabeled phase one targets
config = function()
  local sk = (require "leap").opts.special_keys
  local saved_next_target, saved_prev_target
  local id_1, id_2
  local function set_mappings()
    pcall(vim.api.nvim_del_autocmd, id_2)
    saved_next_target = sk.next_target
    saved_prev_target = sk.prev_target
    sk.next_target = ";"
    sk.prev_target = ","
    local function remove_mappings()
      pcall(vim.api.nvim_del_autocmd, id_1)
      sk.next_target = saved_next_target
      sk.prev_target = saved_prev_target
      id_1 = vim.api.nvim_create_autocmd("User", { pattern = "LeapPatternPost", once = true, callback = set_mappings })
    end
    id_2 = vim.api.nvim_create_autocmd("User", { pattern = "LeapLeave", once = true, callback = remove_mappings })
    return nil
  end
  id_1 = vim.api.nvim_create_autocmd("User", { pattern = "LeapPatternPost", once = true, callback = set_mappings })
end,

While this does seem to work for the most part, it unfortunately overwrites the default next_target and prev_target mappings... which leads me to wonder: if repeat mappings are enabled, is there even a point in having next_target/prev_target set to their own separate mappings? I think it might actually make more sense to have both next_target/prev_target and the repeat mappings to be the same, since ; and s<cr> do pretty much the same thing.

Hmm, it shouldn't... in flit, ;/, are mapped as next_target/prev_target by default, that should take priority. It has nothing to do with multiline.

Oh wait, I think there might be a bug with flit. When pressing , to jump to the previous occurence of a character, it's not possible to jump to an occurence before the location the cursor was currently at, which is not consistent with default vim f/F behavior. Additionally, spamming ; will keep jumping towards the next target up until the last occurence on the line, but , will still jump to the previous target once the last occurence is reached, which makes sense. Spamming ,, on the other hand, will only jump to the previous target until reaching first occurence after the starting position, at which point neither , nor ; will do anything anymore.

I also might've been confused because when there is only a single occurence of a character on a line, pressing ;/, triggers leap's repeat instead of jumping between the next and previous occurence of a character. But you were right that it has nothing to do with multiline.

Or you mean you want ;/, to function as repeat keys for Flit or Leap, depending on which one was the last motion?

I think that makes a lot of sense, and would align better with default vim behavior, since ;/, normally triggers "f/F<last found character". This would also lead to less confusion when there a single occurence of a character on a line and leap repeat is triggered, as just mentioned.

tldr; I think that it might be more intuitive for ;/, to trigger both next_target/prev_target and repeat either both leap or flit, depending on which one was used last to align more closely with default vim behavior. These would eliminate the need for <cr> and <tab>.

Off-topic: what is the colorscheme? Looks nice :)

Cappuccin with these highlight groups :)

local colors = require("catppuccin.palettes").get_palette()

LeapMatch = { fg = colors.text, bold = true, nocombine = true },
LeapLabelPrimary = { fg = colors.pink, bold = true, nocombine = true },
LeapLabelSecondary = { fg = colors.blue, bold = true, nocombine = true },

[Subjective]

I'm gonna close this issue now since I believe it will be solved by the repeat parameter hinted in #176.

[ggandor]

I believe it will be solved by the repeat parameter hinted in #176

Nope, unrelated :)

[Subjective]

Oh whoops I completely forgot what this issue was originally about, I was mainly referring to the inconsistent ;/, behavior when I commented that.

[ggandor]

While this does seem to work for the most part, it unfortunately overwrites the default next_target and prev_target mappings...

Here is a clean solution:

local set_phase2_traversal_keys
do
  -- Set a counter in this closure and generate a unique autogroup
  -- each time, so that multiple key pairs can be defined (e.g. ;/,
  -- and s/S).
  local n = 0
  set_phase2_traversal_keys = function (next_key, prev_key)
    local group = ('LeapSetPhase2TraversalKeys_' .. n)
    n = n + 1
    vim.api.nvim_create_augroup(group, {})
    vim.api.nvim_create_autocmd('User', {
      pattern = 'LeapPatternPost',
      group = group,
      callback = function ()
        local args = require('leap').state.args  -- argument table passed to `leap()`
        if args['repeat'] or args.target_windows then
          return
        end
        -- Note: `require('leap').opts` would dispatch to
        -- `opts.default` (see `init.fnl`).
        local opts = require('leap.opts').default
        local opts_cc = require('leap.opts').current_call
        if not opts_cc.special_keys then
          -- Copy the defaults - we need to keep the group switch
          -- keys - the `__index` method of `opts` will not fall
          -- back to `opts.default` for `special_keys` anymore if
          -- there is such a table in `current_call`. (And obviously
          -- deep-copy, don't overwrite the original.)
          opts_cc.special_keys = vim.deepcopy(opts.special_keys)
        end
        local cc_next = opts_cc.special_keys.next_target or {}
        local cc_prev = opts_cc.special_keys.prev_target or {}
        if (type(cc_next) == 'string') then cc_next = {cc_next} end
        if (type(cc_prev) == 'string') then cc_prev = {cc_prev} end
        table.insert(cc_next, next_key)
        table.insert(cc_prev, prev_key)
        opts_cc.special_keys.next_target = cc_next
        opts_cc.special_keys.prev_target = cc_prev
      end
    })
  end
end

set_phase2_traversal_keys(';', ',')
set_phase2_traversal_keys('s', 'S')

Actually, it might be worth including this in user.fnl.