Skip to content
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

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions doc/table_structures/FUNCTIONS.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,27 @@ local functions = {
}
```

## 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:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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):


```lua
local functions = {
{
mode = 'x', description = 'My function',
function() print('only runs in charwise-visual and selection mode!') end
},
{
description = 'Buffer: git: stage selected hunk'
mode = { 'x', 'V', 'v' },
function()
gitsigns.stage_hunk({ vim.fn.line('.'), vim.fn.line('v') })
end
}
}
```

## Options

You can also pass options via the `opts` property:
Expand Down
59 changes: 59 additions & 0 deletions lua/legendary/data/function.lua
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,24 @@ local class = require('legendary.vendor.middleclass')
local util = require('legendary.util')
local Filters = require('legendary.data.filters')

---Check if modes is an array of strings or itself a string
---@param modes table
---@return boolean
local is_list_of_strings_or_string = function(modes)
if modes == nil or type(modes) == 'string' then
return true
end
for _, mode in ipairs(modes) do
if type(mode) ~= 'string' then
return false
end
end
return true
end

---@class Function
---@field implementation function
---@field mode_mappings string[]
Copy link
Owner

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).

Copy link
Contributor Author

@hinell hinell Oct 20, 2023

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.

function Function:parse(tbl) -- luacheck: no unused
vim.validate({
['1'] = { tbl[1], { 'function' } },
description = { util.get_desc(tbl), { 'string' } },
opts = { tbl.opts, { 'table' }, true },
})

Copy link
Owner

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 Functions, then they're going to work the same as they do for Keymaps.

Copy link
Contributor Author

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

---@field description string
---@field opts table
---@field filters (function[])|nil
Expand All @@ -15,6 +31,11 @@ Function:include(Filters) ---@diagnostic disable-line
function Function:parse(tbl) -- luacheck: no unused
vim.validate({
['1'] = { tbl[1], { 'function' } },
mode = {
tbl.mode,
is_list_of_strings_or_string,
'item.mode should contain only strings of modes: n, i, v etc.',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'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.',

},
description = { util.get_desc(tbl), { 'string' } },
opts = { tbl.opts, { 'table' }, true },
})
Expand All @@ -26,6 +47,29 @@ function Function:parse(tbl) -- luacheck: no unused
instance.opts = tbl.opts or {}
instance:parse_filters(tbl.filters)

-- By default, function can be run in all modes, so mode_mapping is empty
instance.mode_mappings = vim.tbl_islist(tbl.mode) and tbl.mode or {}

-- If tbl = { fn, mode = "n" }
if type(tbl.mode) == 'string' then
table.insert(instance.mode_mappings, tbl.mode)
end
-- only tbl = { fn, mode = { 'n' } }
-- Reassing implementation with a current-mode hanlder
if not vim.tbl_isempty(instance.mode_mappings) then
local impl = instance.implementation
instance.implementation = function()
local modeCurrent = vim.fn.mode()
for _, modeInstance in ipairs(instance.mode_mappings) do
if modeCurrent == modeInstance then
impl()
end
end
Comment on lines +62 to +67
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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()

end
end
-- 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)
Comment on lines +70 to +72
Copy link
Owner

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.

Suggested change
-- 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)

Copy link
Contributor Author

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

Copy link
Owner

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.

Copy link
Contributor Author

@hinell hinell Oct 20, 2023

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.

Copy link
Owner

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.

return instance
end

Expand All @@ -43,4 +87,19 @@ function Function:frecency_id()
return string.format('<function> %s', self.description)
end

--- Intended to be used by UI filters
function Function:modeSwitched()
return self._mode_switched
end

Comment on lines +90 to +94
Copy link
Owner

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.

Suggested change
--- Intended to be used by UI filters
function Function:modeSwitched()
return self._mode_switched
end

Copy link
Contributor Author

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

Copy link
Owner

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.

Copy link
Contributor Author

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

function Function:modes()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function Function:modes()
---@return string[] | nil
function Function:modes()

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
Comment on lines +96 to +102
Copy link
Owner

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.

Suggested change
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

Copy link
Contributor Author

@hinell hinell Oct 20, 2023

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disagree.

Copy link
Contributor Author

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

end

return Function
9 changes: 5 additions & 4 deletions lua/legendary/filters.lua
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@ local M = {}
---@return LegendaryItemFilter
function M.mode(mode)
return function(item)
-- include everything that isn't a keymap since they aren't tied to a mode
if not Toolbox.is_keymap(item) then
-- allow only keymaps and functions
-- TODO: include commands
if not (Toolbox.is_keymap(item) or Toolbox.is_function(item)) then
return true
end

Expand All @@ -26,9 +27,9 @@ function M.mode(mode)
end

-- filter where any filter_modes match any item:modes()
return #vim.tbl_filter(function(keymap_mode)
return #vim.tbl_filter(function(item_mode)
return #vim.tbl_filter(function(filter_mode)
return filter_mode == keymap_mode
return filter_mode == item_mode
end, filter_modes) > 0
end, item:modes()) > 0
end
Expand Down
6 changes: 5 additions & 1 deletion lua/legendary/ui/format.lua
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,15 @@ function M.default_format(item)
item.description,
}
elseif Toolbox.is_function(item) then
-- stylua: ignore start
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this here?

Copy link
Contributor Author

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

Copy link
Owner

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.

return {
Config.icons.fn,
item--[[@as keymap ]]:modeSwitched()
and table.concat(item--[[@as Keymap]]:modes(), ', ')
or Config.icons.fn,
Comment on lines +43 to +45
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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 }, ', '),

Copy link
Contributor Author

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.

Copy link
Owner

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.

'<function>',
item.description,
}
-- stylua: ignore end
Copy link
Owner

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.

elseif Toolbox.is_itemgroup(item) then
return {
item.icon or Config.icons.itemgroup,
Expand Down
Loading