Open rish987 opened 1 year ago
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()
).
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.
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.
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.
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 https://github.com/ggandor/leap.nvim/issues/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.
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.
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.
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).
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?
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?)
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).
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.
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:
s<label>
in one stroke.z
) for each of the different variants of leap-to-line. I would like pressingz
to have the same effect as pressings<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 potentialsublists
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 mapselect()
tos/S
and skip callingdebeacon()
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, thenbeacon(targets)
again andselect()
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 theprepare-targets sublist
logic beyond thenot user-given-targets?
case, and thus allow the user to include thetargets.sublists
field as part of the officially supported API. Then, we would makez
map to a function that immediately callsbeacon
on all the targets from all the variations of leap-to-line, separated intosublists
. Once we get another character input to specify which variation it was, we can then callbeacon(targets)
andselect()
and finallydebeacon()
.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.