-
-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(functions): Mode aware functions #415
Conversation
## Specifying mode | ||
|
||
By default, the funciton is shown and run in `*` (all) modes. | ||
You can use `mode` property to narrow function's scope, so it always run in specified mode: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use `mode` property to narrow function's scope, so it always run in specified mode: | |
You can use `mode` property to narrow function's scope, so it only appears in the specified mode(s): |
mode = { | ||
tbl.mode, | ||
is_list_of_strings_or_string, | ||
'item.mode should contain only strings of modes: n, i, v etc.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'item.mode should contain only strings of modes: n, i, v etc.', | |
'item.mode must be a string or list of strings specifying modes: n, i, v etc.', |
local modeCurrent = vim.fn.mode() | ||
for _, modeInstance in ipairs(instance.mode_mappings) do | ||
if modeCurrent == modeInstance then | ||
impl() | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local modeCurrent = vim.fn.mode() | |
for _, modeInstance in ipairs(instance.mode_mappings) do | |
if modeCurrent == modeInstance then | |
impl() | |
end | |
end | |
local current_mode = vim.fn.mode() | |
if not vim.tbl_contains(instance.mode_mappings, current_mode) then | |
return | |
end | |
impl() |
-- mode_mapping is going to remain static during legendary runtime | ||
-- so we can cache current mapping state | ||
instance._mode_switched = vim.tbl_islist(instance.mode_mappings) and not vim.tbl_isempty(instance.mode_mappings) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't feel necessary to me to cache this.
-- mode_mapping is going to remain static during legendary runtime | |
-- so we can cache current mapping state | |
instance._mode_switched = vim.tbl_islist(instance.mode_mappings) and not vim.tbl_isempty(instance.mode_mappings) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used for displaying: 29513d1#r1366849251
; It's essential for both performance reason and readability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't think it will have a noticeable impact on performance, and I don't see how it affects readability either, personally.
If you can show some benchmarks proving otherwise I'm all ears.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrjones2014 It's cheap to check once and leave, then checking whether the list is empty all the time when the list doesn't change. This is obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s an extremely inexpensive check. It doesn’t need to be cached.
--- Intended to be used by UI filters | ||
function Function:modeSwitched() | ||
return self._mode_switched | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the diff it looks like this function isn't needed either.
--- Intended to be used by UI filters | |
function Function:modeSwitched() | |
return self._mode_switched | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's used for displaying: 29513d1#r1366849251
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you don't have to do it that way though -- see the other comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to refactor it after a merge
if self:modeSwitched() then | ||
return self.mode_mappings | ||
else | ||
-- Just use all modes for UI filtering | ||
-- it's half-assed because UI filtering is ugly ¯\_(ツ)_/¯ | ||
return { 'n', 'V', 'v', 'x', 's', 'o', '' } | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's keep it consistent with how the mode_mappings
API works for other items.
if self:modeSwitched() then | |
return self.mode_mappings | |
else | |
-- Just use all modes for UI filtering | |
-- it's half-assed because UI filtering is ugly ¯\_(ツ)_/¯ | |
return { 'n', 'V', 'v', 'x', 's', 'o', '�' } | |
end | |
if vim.tbl_islist(self.mode_mappings) then | |
return self.mode_mappings | |
end | |
local modes = {} | |
for mode, mapping in pairs(self.mode_mappings) do | |
if mapping then | |
table.insert(modes, mode) | |
end | |
end | |
if #modes == 0 then | |
return { 'n', 'V', 'v', 'x', 's', 'o', '�' } | |
end | |
return modes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will require a lot of changes in Function's constructor where mode_mappings
is built . See my reply below.
let's keep it consistent
Well, mode-map is not consistent across keymaps, commands, autocommands, and functions.
This PR lays a groundwork for further extension; I personally see no reason for complicated mode-map. Again, see my reply below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disagree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not an intent of this PR; I'm fine if you refactor it further after merging it;
there is a lof of space for improvement in this project
@@ -38,11 +38,15 @@ function M.default_format(item) | |||
item.description, | |||
} | |||
elseif Toolbox.is_function(item) then | |||
-- stylua: ignore start |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stylua makes lines in-between ugly otherwise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather just have the formatter format it than stray from the style of everything else in the entire codebase.
'<function>', | ||
item.description, | ||
} | ||
-- stylua: ignore end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be ignoring formatting unless there's a good reason.
item--[[@as keymap ]]:modeSwitched() | ||
and table.concat(item--[[@as Keymap]]:modes(), ', ') | ||
or Config.icons.fn, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
item--[[@as keymap ]]:modeSwitched() | |
and table.concat(item--[[@as Keymap]]:modes(), ', ') | |
or Config.icons.fn, | |
table.concat(item--[[@as Function]]:modes() or { Config.icons.fn }, ', '), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, look carefully what you suggest; this introduces a bug; item:modeSwitched()
is essential here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't introduce a bug if you also tweak the implementation of modes()
as I suggested.
---@class Function | ||
---@field implementation function | ||
---@field mode_mappings string[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't have the mode_mappings
be a different type for functions than for everything else.
Take a look at how Keymap
handles it. The type should be something similar to ModeKeymap
, but you'll just need to modify it slightly to take different options (the options for a Function
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite a lot for an overnight PR which I made without a prior knowledge of this project, isn't it?
I didn't intend to implement fully keymap
-compatible mode-map, cuz:
- Nobody is going to drop funcs items into
legendary.keymap({ ... })
- Even if they do, current modemap type is fully forward compatible with
legendary.keymap({ ... })
; - Zero use cases
- No need for complicated ways of mode-map declaration
Ambiguities
you need just slightly
Only to make things more complicated: validation (see below) requires that item's [1]
-field is provided with a function, so introduction of keymaps
-like node-map will likely break something - you have to figure you what to do with that function; it's subject for further discussion.
I suggest you take care of map enhancement further after merging this PR.
legendary.nvim/lua/legendary/data/function.lua
Lines 15 to 20 in 6ea46c2
function Function:parse(tbl) -- luacheck: no unused | |
vim.validate({ | |
['1'] = { tbl[1], { 'function' } }, | |
description = { util.get_desc(tbl), { 'string' } }, | |
opts = { tbl.opts, { 'table' }, true }, | |
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite a lot for an overnight PR which I made without a prior knowledge of the project, isn't it?
Not sure what you mean. There's no deadline on the PR.
Regarding your 3 bullets, addressing in-order:
- That's not what I said and it's not what my comment is meant to support
- See above
- The use case is to support the alternate declaration syntax (2nd code block here)
Only to make things more complicated: validation (see below) requires that item's [1]-field is provided with a function, so introduction of keymaps-like node-map will likely break something - you have to figure you what to do with that function; it's subject of further discussion.
It's not really more complicated. You can copy the validator from keymap.lua
only for the options, validate that they're Function
options instead of Keymap
options. Then just update executor.lua
to pull the right implementation out of the item based on the mode.
It we're going to introduce mode_mappings
for Function
s, then they're going to work the same as they do for Keymap
s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(2nd code block here)
then they're going to work the same as they do for Keymaps.
As I said, this PR is about functions, not keymaps; if you want to unify mode-map apis for both, I would suggest you to take care of further mode-map improvement yourself taking the laid groundwork as basis; for me, it's more than enough to have mode = { 'n', 'v', 'x' }
per one function; for more - i would just create more items with different mode
; cheap and quick
And as I said, I see no reason to introduce that complicated approach, cause there are going to be ambiguities you will have to resolve as project owner
Also, please update the PR title per CONTRIBUTING.md |
Since it seems like you're totally unwilling to work with me to address the issues in the PR, I'm just going to close it. Feel free to reopen or open a new PR if you decide you're willing to follow the project guidelines and code reviews from project maintainers. |
If you want more features - go ahead, I'm fine; My point is: I suggest the laid ground work for futher improvement, it's obviously beneficial to have half of the job done, then doing it yourself It's also faster to merge and improve, than bogging down this discussion in very vague, or obviously incorrect suggestions It's sad that I have to maintain a separate fork of this project; anyway |
Sure, you're definitely right about that. That doesn't mean I'm going to merge the feature in its current half-baked state.
Yes it's faster, that doesn't mean I'm okay with merging half-baked features that don't align with existing features for other item types.
Please explain to me how you, who by your own admission doesn't have any prior knowledge of the project, would be able to identify that better than the project maintainer, and point me to a suggestion I made that is "obviously incorrect".
I'm more than willing to work with you to get the PR into a mergeable state, however you don't seem willing to accept any of the feedback or address any of the problems with the PR. If you'd rather maintain your own fork than actually collaborate, then go ahead, but open source collaboration doesn't mean merging incomplete PRs just because you say so. Maybe you could try being a little less rude next time 🙂 |
Deleted a bunch of useless vile rudeness comments. |
functions-demo-switched-mode.mov
Summary:
mode_mappings
Note
I request to squash-rebase
How to Test
New
funcs = { ... mode = 'n' }
field for functions - its type should be eitherstring
ortable<string>
; that's itTesting for Regressions
I have tested the following:
legendary.nvim
in all modes (normal, insert, visual)legendary.nvim
, then triggering via the keymap in all modes (normal, insert, visual)legendary.nvim
in all modes (normal, insert, visual)legendary.nvim
, then running the command manually from the command lineaugroup
/autocmd
s created throughlegendary.nvim
work correctly