API refinement proposal

[rish987]

Having recently worked on #149 and #150, there are a couple more improvements that I would like to make there, but which don't seem possible with the current API:

1. In docs/fix: Telescope integration #149, you have to activate the mapping for the labels to show up in the telescope entry list. However, it would be of no consequence in that situation for the target labels to persist even when you aren't using a Leap mapping (it's easy to make room for them alongside the entries). Then, when you want to leap, you can look at the target label, and then type s<label> in one stroke.
2. In docs(readme): enhance leap-to-line example code #150, I created mappings (each of them starting with z) for each of the different variants of leap-to-line. I would like pressing z to have the same effect as pressing s<char> in the sense that it will immediately show me all of the targets with their labels (where there can be multiple targets with the same label if they come from mutually exclusive mappings (i.e. sublists)). This will allow the targets to be displayed one keystroke sooner.

To address (1), I propose that we separate the leap routine into three subroutines:

• beacon(targets): given a list of targets (with a potential sublists field), this will set the beacons and display them, and set some state so that we know which beacons/targets are active. If called multiple times, overrides the previous state each time.
• select(): get user input to select a target based on the current beacon state, throwing an error if a label corresponds to multiple sublists (or perhaps if there are any sublists at all).
• debeacon(): deactivate all beacons.

Now, for the Telescope extension, we can hook beacon(targets) onto each time that the entry list is updated, so the beacons will be ever-present. And we would map select() to s/S and skip calling debeacon() entirely (or only if explicitly invoked with a separate mapping).

To reconstruct the normal Leap behavior, we would call beacon(targets) after the first character input, then beacon(targets) again and select() after the second character input. We will of course have to be careful about preserving all of the current behavior, i.e. auto-jumping to and skipping the labels for the first match, but this will be part of the logic of the default leap implementation rather than that of the core API.

To address (2), we would, as suggested above in defining beacon(targets), extend the prepare-targets sublist logic beyond the not user-given-targets? case, and thus allow the user to include the targets.sublists field as part of the officially supported API. Then, we would make z map to a function that immediately calls beacon on all the targets from all the variations of leap-to-line, separated into sublists. Once we get another character input to specify which variation it was, we can then call beacon(targets) and select() and finally debeacon().

Obviously, since I'm the one proposing this refactor I am also volunteering to do it as well. But I wanted to get your thoughts on this first and perhaps learn more about your overall vision of how/whether you expect this plugin's API to change in the future.

[rish987]

Oh, and to clarify, I think that we should keep a leap() API function that works the same way it does now -- but I think we should pull out the parts specific to Leap's default behavior and move them to a different function (like leap_2char()).

[ggandor]

I'm not 100% sure we should do this, but I've been thinking about decomposing the main module along similar lines for a while, so I will definitely get back to you as soon as possible. I won't have much time for Leap in the coming weeks, unfortunately.

[ggandor]

I would like pressing z to have the same effect as pressing s in the sense that it will immediately show me all of the targets with their labels (where there can be multiple targets with the same label if they come from mutually exclusive mappings (i.e. sublists)). This will allow the targets to be displayed one keystroke sooner.

This is an interesting idea, it never occurred to me that hierarchical multi-key mappings also allow the clairvoyance trick.

[rish987]

Related to that, I think we should use different colors depending on whether we are just beaconing or actually waiting for input to select a label. It could help avoid some confusion like #148. Following this refactor, it should just be a matter of changing the extmark highlights when you call select().

I should have some time for the next couple weeks, so I'm going to try and take a shot at this refactor. I'll let you know if I encounter anything that means I have to alter some aspects of the above proposal.

[ggandor]

I think we should use different colors depending on whether we are just beaconing or actually waiting for input to select a label. It could help avoid some confusion like #148.

I don't think so, this is typical "hello world" optimization, that becomes annoying once you grok Leap. It might make the onboarding experience for some users (who don't read the fine manual) a bit easier, at the cost of (1) introducing two additional highlight groups (2) changing the beacons while typing, which should be avoided.

[rish987]

Oh, okay, having read that I agree that having the colors of a label change while you're focused on it can be distracting (you can probably tell that I'm somewhat new to Leap myself). On second thought, you're right, this applies less to the default Leap behavior, where mistyping any one of the characters in s/S<char><char> would mean a loss of the label you meant to leap to.

But the story can change I think depending on the extension. For example, in the telescope case, I tend to fumble s/S, so it would be nice to have some visual indication of which operation I am doing (so I know to cancel if I typed the wrong thing). I think we can decide on the more "common case" (here it's probably s) and minimize any visual noise with that mapping, but have some extra indication for the less common mappings.

The change could be as subtle as making the text color go from gray to black, or could be external to the labels themselves -- a statusline indicator (like -- INSERT -- when you're in insert mode), or a symbol appearing next to the label that indicates the operation that you have selected. Of course, we should make it something that the user can opt-out of if they feel particularly confident about their typing accuracy.

[ggandor]

The change could be as subtle as making the text color go from gray to black, or could be external to the labels themselves -- a statusline indicator (like -- INSERT -- when you're in insert mode)

The suckless/obvious solution for both of these is via autocommands in userland, provided we add new events between LeapEnter/LeapLeave, like LeapPhaseTwo, etc.

[ggandor]

#65

[rish987]

The suckless/obvious solution for both of these is via autocommands in userland, provided we add new events between LeapEnter/LeapLeave, like LeapPhaseTwo, etc.

Sounds reasonable. I won't budget for it in this refactor though, will try to keep the changes minimal and we can have a more targeted discussion later.

Something else that came to mind, after ruminating a bit on the Telescope extension - I think that the beacon() function should allow for the targets to be pre-associated with their labels. This way, we could assign labels based on the filename itself (with some logic to avoid collisions), so that, once the user has learned the associated label, they don't even have to look at it again when they want to select a file. And the label wouldn't change depending on the order in the picker (which changes a lot in the case of e.g. telescope.builtin.oldfiles). To also make this consistent across different file pickers, we could, for example, order all the files in the project alphabetically and then assign the labels (Though this kind of computation may be undesirable for larger projects; I suppose we could cache the order per-session though, and only update it when we encounter a new file in a picker). (On second thought, it would probably be best to just limit this to the last #labels files in the oldfiles picker).

[rish987]

I've been thinking about the action(targets) function parameter to the current leap() API, and I think that it would make sense to additionally allow for the targets to each carry their own callback (which would override this generic callback). The current way somewhat assumes that anything that you want to do with a selected target is necessarily tied to the data that is used to display the target (i.e. window, buffer, and cursor position), so doing anything else requires that you add extra fields to the targets that you later have to extract in the generic callback.

As it is, you could of course just add a callback function as a field of the target and call that in the generic callback, but then that means extra/redundant code for every such extension. I think we should keep the generic callback however, in case you don't want to have to attach a function to every target (and you can get away easily without doing so). What do you think?

[ggandor]

allow for the targets to each carry their own callback (which would override this generic callback)

vs

you could of course just add a callback function as a field of the target and call that in the generic callback

I'm not following. What do you mean by "carry their own callback" then, if not "having a callback field"? (And for what purpose? Could you give an example?)

[rish987]

Sorry. For example, this could be a more perspicuous way of implementing #149:

local function get_telescope_targets(prompt_bufnr, action)
  local pick = action_state.get_current_picker(prompt_bufnr)
  local scroller = require "telescope.pickers.scroller"

  local wininfo = vim.fn.getwininfo(pick.results_win)

  local first = math.max(scroller.top(pick.sorting_strategy, pick.max_results, pick.manager:num_results()), wininfo[1].topline - 1)
  local last = wininfo[1].botline - 1

  local targets = {}
  for row=last,first,-1 do
    local target = {
      wininfo = wininfo[1],
      pos = {row + 1, 1},
      row = row,
    }
    target.action = function ()
      action(target, pick)
    end
    table.insert(targets, target)
  end
  return targets
end

telescope.setup {
  defaults = {
    mappings = {
      n = {
        ["S"] = function (prompt_bufnr)
          require('leap').leap {
            targets = get_telescope_targets(prompt_bufnr, function(target, picker)
                picker:set_selection(target.row)
              end),
          }
        end,
        ["s"] = function (prompt_bufnr)
          require('leap').leap {
            targets = get_telescope_targets(prompt_bufnr,
              function(target, picker)
                picker:set_selection(target.row)
                actions.select_default(prompt_bufnr)
              end),
          }
        end
      }
    }
  }
}

where we set the action callback per-target, rather than per-API call, so that we don't have to indirectly access pick through a field of the target (and we have a clear separation between leap- and extension-specific metadata).

[ggandor]

Sorry, but I still don't understand the proposal. What would be the actual change in Leap's API/code? In the above example, an action field is added to each target. You can just leap { ..., action = function (target) target.action() end } then. What's the problem here? "Extension-specific metadata" is accesed via the target.action closure.

so that we don't have to indirectly access pick through a field of the target

Technically, this is what happens now, via the action field. But since you use the same picker, I don't get it why you should inject that reference into each target in the first place, and not just use it in the (regular) action callback: leap { action = function (target) pick = ...; <do sg with target> end }. Okay, you have to reference the picker in both targets and action then, but I don't see why that's so problematic.