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(#2654): filters.custom may be a function #2655

Merged
merged 21 commits into from
Feb 11, 2024
Merged

feat(#2654): filters.custom may be a function #2655

merged 21 commits into from
Feb 11, 2024

Conversation

dxrcy
Copy link
Contributor

@dxrcy dxrcy commented Jan 29, 2024

Fixes #2654

Related to #2654 and #2648.

  1. Adds support for Lua functions in filters.custom.
    Functions take the absolute file path (string), and return bool.

  2. Adds filters.binaries option to ignore ELF files.
    Files are read 4 bytes to check for \x7fELF header.
    No other executable/binary formats are currently supported, but can be easily added.
    Toggle hotkey is currently h as a placeholder.

Example usage:

local opts = {
    filters = {
        custom = {
            --- This:
            "*.lock",
            -- ...is equivelant to this:
            function(path)
                return path:sub(-5) == ".lock"
            end
        },
        -- New field
        binaries = true,
    },
}

@dxrcy dxrcy marked this pull request as ready for review January 29, 2024 03:29
@dxrcy dxrcy changed the title Custom filter functions and binaries field feat: custom filter functions and binaries field Jan 29, 2024
@alex-courtis
Copy link
Member

Many thanks, I'll get to this on the weekend.

@@ -69,6 +69,30 @@ local function dotfile(path)
return M.config.filter_dotfiles and utils.path_basename(path):sub(1, 1) == "."
end

---@param path string
---@return boolean
local function binary(path)
Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry, we can't include this OS specific functionality. It will also be a big performance overhead.

Fortunately we have :help executable() that is portable and fast. That is persisted on the Node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not know of this function, thanks! :)

Copy link
Contributor Author

@dxrcy dxrcy Feb 3, 2024

Choose a reason for hiding this comment

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

One downside to this option is that executable text files, such as shell scripts, will be hidden with this filter.
I think personally I will just implement the first solution as a custom function :-)

Copy link
Member

Choose a reason for hiding this comment

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

I see. That is a good use case, however as you say it is a good case for a freely defined user function.

Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

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

Looking good!

Let's get this generic.

lua/nvim-tree/keymap.lua Outdated Show resolved Hide resolved
if vim.fn.match(relpath, pat) ~= -1 or vim.fn.match(basename, pat) ~= -1 then
return true
if type(pat) == "function" then
if pat(path) then
Copy link
Member

@alex-courtis alex-courtis Feb 3, 2024

Choose a reason for hiding this comment

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

Let's give the user all the information here to allow creation of any sort of filter.

We can use lib clone_node to render the subtree to pass. That contains executable.

Yes, that's a performance hit, however only for users that use a custom filter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it possible to get the node from the path, at the stage the filter is ran?
I believe the explorer has not yet loaded, so utils.get_node_from_path returns nil.

I am not very familiar with this codebase, so apologies if I am misunderstanding something.

Copy link
Member

Choose a reason for hiding this comment

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

It will often be null; we just have to test for that and ignore it.

@alex-courtis
Copy link
Member

On adding to ACCEPTED_TYPES and setting

  filters = {
    custom = function()
      return true
    end,
Error detected while processing /tmp/nvt-dev/nvt-dev.lua:
E5113: Error while calling lua chunk: ...ck/packer/start/dxrcy/lua/nvim-tree/explorer/filters.lua:162: attempt to get length of local 'custom_filter' (a function value)
stack traceback:
        ...ck/packer/start/dxrcy/lua/nvim-tree/explorer/filters.lua:162: in function 'setup'
        .../pack/packer/start/dxrcy/lua/nvim-tree/explorer/init.lua:62: in function 'setup'
        /tmp/nvt-dev/site/pack/packer/start/dxrcy/lua/nvim-tree.lua:796: in function 'setup'
        /tmp/nvt-dev/nvt-dev.lua:85: in main chunk

Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

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

Looking good.

Once we have the plumbing in to actually execute the function we should be OK.

@@ -1241,7 +1241,7 @@ Enabling this is not useful as there is no means yet to persist bookmarks.
Custom list of vim regex for file/directory names that will not be shown.
Backslashes must be escaped e.g. "^\\.git". See |string-match|.
Toggle via |nvim-tree-api.tree.toggle_custom_filter()|, default `U`
Type: {string}, Default: `{}`
Type: {string | function(path)}, Default: `{}`
Copy link
Member

Choose a reason for hiding this comment

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

Needs to be added to ACCEPTED_TYPES

@@ -87,8 +87,14 @@ local function custom(path)
-- filter custom regexes
local relpath = utils.path_relative(path, vim.loop.cwd())
for pat, _ in pairs(M.ignore_list) do
Copy link
Member

Choose a reason for hiding this comment

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

With some hacking

function M.setup(opts)
  M.config = {
    filter_custom = opts.filters.custom,
    filter_dotfiles = opts.filters.dotfiles,
    filter_git_ignored = opts.filters.git_ignored,
    filter_git_clean = opts.filters.git_clean,
    filter_no_buffer = opts.filters.no_buffer,
    filter_no_bookmark = opts.filters.no_bookmark,
  }

  M.ignore_list = {}
  M.exclude_list = opts.filters.exclude

  log.line("dev", "filters setup M='%s'", vim.inspect(M))
  -- local custom_filter = opts.filters.custom
  -- if custom_filter and #custom_filter > 0 then
  --   for _, filter_name in pairs(custom_filter) do
  --     M.ignore_list[filter_name] = true
  --   end
  -- end
end

I was able to pass the custom function through, however it's never executed as ignore_list is null.

I reckon you'll need to directly call the custom function outside of this loop.

if vim.fn.match(relpath, pat) ~= -1 or vim.fn.match(basename, pat) ~= -1 then
return true
if type(pat) == "function" then
if pat(path) then
Copy link
Member

Choose a reason for hiding this comment

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

It will often be null; we just have to test for that and ignore it.

@dxrcy
Copy link
Contributor Author

dxrcy commented Feb 4, 2024

On adding to ACCEPTED_TYPES and setting

  filters = {
    custom = function()
      return true
    end,
Error detected while processing /tmp/nvt-dev/nvt-dev.lua:
E5113: Error while calling lua chunk: ...ck/packer/start/dxrcy/lua/nvim-tree/explorer/filters.lua:162: attempt to get length of local 'custom_filter' (a function value)
stack traceback:
        ...ck/packer/start/dxrcy/lua/nvim-tree/explorer/filters.lua:162: in function 'setup'
        .../pack/packer/start/dxrcy/lua/nvim-tree/explorer/init.lua:62: in function 'setup'
        /tmp/nvt-dev/site/pack/packer/start/dxrcy/lua/nvim-tree.lua:796: in function 'setup'
        /tmp/nvt-dev/nvt-dev.lua:85: in main chunk

To accept a function or a table of functions or strings, is the syntax custom = { { "string", "function" }, "function" } ?

@alex-courtis
Copy link
Member

To accept a function or a table of functions or strings, is the syntax custom = { { "string", "function" }, "function" } ?

Unfortunately we don't have that level of control over types. This is the best we can do:

  filters = {
    custom = { "table", "function", },
  },

Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

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

Looking good:

    custom = function(path, node)
      if node then
        if node.nodes then
          log.raw("dev", "custom path=%s node.nodes=%d\n", path, #node.nodes)
        else
          log.raw("dev", "custom path=%s node.nodes=-1\n", path)
        end
      else
        log.raw("dev", "custom path=%s node=nil\n", path)
      end
    end,

Let's flatten out for pat, _ in pairs(M.ignore_list) do and we can test in anger.

Later: we'll also need to do some optimisation, passing should_filter a node rather than a path, as retrieving / generating nodes is expensive.

@@ -146,6 +147,10 @@ function M.should_filter(path, status)
end

function M.setup(opts)
if type(opts.filters.custom) == "function" then
opts.filters.custom = { opts.filters.custom }
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to convert this to a table? We can just directly set the function as there will only ever be one:

M.config.filter_custom = opts.filters.custom,

@@ -1241,7 +1241,7 @@ Enabling this is not useful as there is no means yet to persist bookmarks.
Custom list of vim regex for file/directory names that will not be shown.
Backslashes must be escaped e.g. "^\\.git". See |string-match|.
Toggle via |nvim-tree-api.tree.toggle_custom_filter()|, default `U`
Type: {string | function(path)}, Default: `{}`
Type: {string | function(path, node)}, Default: `{}`
Copy link
Member

Choose a reason for hiding this comment

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

We can remove path here; Node.absolute_path provides what we need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about the case where node is null, though? :/

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, sorry, I did not look properly at populate_children. It doesn't hydrate the node until after the filter.

Let's see how it looks like this. If necessary we can refactor reload and populate_children to remove the premature optimisation.

@dxrcy dxrcy changed the title feat: custom filter functions and binaries field feat: custom filter functions Feb 4, 2024
@alex-courtis
Copy link
Member

This has gotten complicated. I'm the one who insisted on node, which is not strictly needed for your use case.

I'm happy if you remove node and just pass path. It matches the existing filter pattern and does allow you to use executable()

@dxrcy
Copy link
Contributor Author

dxrcy commented Feb 5, 2024

This has gotten complicated. I'm the one who insisted on node, which is not strictly needed for your use case.

I'm happy if you remove node and just pass path. It matches the existing filter pattern and does allow you to use executable()

Thanks :-)

@dxrcy
Copy link
Contributor Author

dxrcy commented Feb 5, 2024

To accept a function or a table of functions or strings, is the syntax custom = { { "string", "function" }, "function" } ?

Unfortunately we don't have that level of control over types. This is the best we can do:

  filters = {
    custom = { "table", "function", },
  },

I am getting an error when trying to set the accepted type to { "table", "function" }, and settings filters.custom to a table with a string.
Trying to pass a string as the first argument of vim.tbl_contains.
The error goes away when changing it to { "function" }, but I suppose this is not properly checking types.

@alex-courtis
Copy link
Member

The error goes away when changing it to { "function" }, but I suppose this is not properly checking types.

I see. It looks like we're not handling the table or something case.

I'll see what I can put together.

@alex-courtis
Copy link
Member

Raised #2665

We can use

  filters = {
    custom = { "function" },
  },

for now. The value will be validated as an arraylike table or function.

The only issue is the error message

[NvimTree]
Invalid option: filters.custom. Expected function, got number

That's too bad and should not hold up this change.

Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

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

Looks like it's working nicely for the initial case, however stops working after toggling.

Test case:

  • directory containing A B C
  • open tree
  • toggle twice
  • filter does not apply
---@param path string
---@return boolean
local function custom(path)
  log.line("dev", "filters.custom %s", path)
  if not M.config.filter_custom then
    log.line("dev", "filters.custom %s NOT PRESENT", path)
    return false
  end
  filters = {
    custom = function(absolute_path)
      log.line("dev", "user    custom %s", absolute_path)
      return absolute_path:match("B")
    end,
  },

Output:

[2024-02-05 16:39:33] [dev] filters.custom /home/alex/src/nvim-tree/r/2655/A
[2024-02-05 16:39:33] [dev] user    custom /home/alex/src/nvim-tree/r/2655/A
[2024-02-05 16:39:33] [dev] filters.custom /home/alex/src/nvim-tree/r/2655/B
[2024-02-05 16:39:33] [dev] user    custom /home/alex/src/nvim-tree/r/2655/B
[2024-02-05 16:39:33] [dev] filters.custom /home/alex/src/nvim-tree/r/2655/C
[2024-02-05 16:39:33] [dev] user    custom /home/alex/src/nvim-tree/r/2655/C

[2024-02-05 16:39:41] [dev] filters.custom /home/alex/src/nvim-tree/r/2655/A
[2024-02-05 16:39:41] [dev] filters.custom /home/alex/src/nvim-tree/r/2655/A NOT PRESENT
[2024-02-05 16:39:41] [dev] filters.custom /home/alex/src/nvim-tree/r/2655/B
[2024-02-05 16:39:41] [dev] filters.custom /home/alex/src/nvim-tree/r/2655/B NOT PRESENT
[2024-02-05 16:39:41] [dev] filters.custom /home/alex/src/nvim-tree/r/2655/C
[2024-02-05 16:39:41] [dev] filters.custom /home/alex/src/nvim-tree/r/2655/C NOT PRESENT

[2024-02-05 16:39:46] [dev] filters.custom /home/alex/src/nvim-tree/r/2655/A
[2024-02-05 16:39:46] [dev] filters.custom /home/alex/src/nvim-tree/r/2655/B
[2024-02-05 16:39:46] [dev] filters.custom /home/alex/src/nvim-tree/r/2655/C

Looks like M.config.filter_custom is being mutated.

@@ -1241,7 +1241,7 @@ Enabling this is not useful as there is no means yet to persist bookmarks.
Custom list of vim regex for file/directory names that will not be shown.
Backslashes must be escaped e.g. "^\\.git". See |string-match|.
Toggle via |nvim-tree-api.tree.toggle_custom_filter()|, default `U`
Type: {string}, Default: `{}`
Type: {string | function(path)} | function(path), Default: `{}`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Type: {string | function(path)} | function(path), Default: `{}`
Type: {string} | `function(absolute_path)`, Default: `{}`

We'll get to luals annotation but not today.

@dxrcy
Copy link
Contributor Author

dxrcy commented Feb 5, 2024

Looks like M.config.filter_custom is being mutated.

My bad. Should be fixed.

@@ -624,7 +624,7 @@ local ACCEPTED_TYPES = {
root_folder_label = { "function", "string", "boolean" },
},
filters = {
custom = { "table", "function" },
custom = { "function" },
Copy link
Member

Choose a reason for hiding this comment

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

Thank you, looking good!

if type(custom_filter) == "function" then
M.config.filter_custom = custom_filter
M.ignore_list[custom_filter] = true
Copy link
Member

Choose a reason for hiding this comment

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

Should we be using ignore_list at all; can't we just use M.config.custom_filter directly?

Unfortunately I'm out of time until the weekend. I'll give it a solid test then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By M.config.custom_filter, I assume you mean M.config.filter_custom?

It looks like M.config.filter_custom is a boolean, for whether all custom filters are enabled.
Adding the single custom filter function to M.ignore_list would be consistent with adding each item of the custom filter table to it, wouldn't it? As M.ignore_list is a set, with each item checked in the loop in the custom().

Perhaps I should set M.ignore_list to opts.filters.custom (a function), and check the type in custom(), if this is more intuitive or performant.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like M.config.filter_custom is a boolean, for whether all custom filters are enabled.

Right you are, please accept my apologies.

Perhaps I should set M.ignore_list to opts.filters.custom (a function), and check the type in custom(), if this is more intuitive or performant.

That is more intuitive, however it's not good practice to use a member with variable types.

Can we please go one further and explicitly separate, something like

diff --git a/lua/nvim-tree/explorer/filters.lua b/lua/nvim-tree/explorer/filters.lua
index f3beb61..24747c2 100644
--- a/lua/nvim-tree/explorer/filters.lua
+++ b/lua/nvim-tree/explorer/filters.lua
@@ -4,6 +4,7 @@ local marks = require "nvim-tree.marks"
 local M = {
   ignore_list = {},
   exclude_list = {},
+  custom_function = nil,
 }

 ---@param path string
@@ -84,18 +85,17 @@ local function custom(path)

   local basename = utils.path_basename(path)

+  -- filter by user's custom function
+  if M.custom_function and M.custom_function(path) then
+    return true
+  end
+
   -- filter custom regexes
   local relpath = utils.path_relative(path, vim.loop.cwd())
   for pat, _ in pairs(M.ignore_list) do
-    if type(pat) == "function" then
-      if pat(path) then
-        return true
-      end
-    else
       if vim.fn.match(relpath, pat) ~= -1 or vim.fn.match(basename, pat) ~= -1 then
         return true
       end
-    end
   end

   local idx = path:match ".+()%.[^.]+$"
@@ -160,7 +160,7 @@ function M.setup(opts)

   local custom_filter = opts.filters.custom
   if type(custom_filter) == "function" then
-    M.ignore_list[custom_filter] = true
+    M.custom_function = custom_filter
   else
     if custom_filter and #custom_filter > 0 then
       for _, filter_name in pairs(custom_filter) do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That looks like a better solution.
Though this won't let a user define a both a function and a pattern, but I don't think this is a problem, as a custom function can of course contain a pattern check.

custom = function(path)
    if vim.fn.match(path, ".*\\.lock") ~= -1 then -- Could be a specific function like substring
        return true
    end
    return is_binary(path)
end

Previously:

custom = {
    "*.lock",
    function(path) -- Or simply pass `is_binary`
        return is_binary(path)
    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.

Is it possible to go back to expecting a table<function | string>, instead of a function | table<string>?
A singular function would just be wrapped in a table: custom = { is_binary }.

If this is not preferable, I do not mind.

Copy link
Member

Choose a reason for hiding this comment

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

Though this won't let a user define a both a function and a pattern, but I don't think this is a problem, as a custom function can of course contain a pattern check.

Is it possible to go back to expecting a table<function | string>, instead of a function | table?
A singular function would just be wrapped in a table: custom = { is_binary }.

Sorry, I'd prefer we keep it in the style of the other similar code - one (fast) function only.

Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

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

Many thanks for all your great work and bearing with all the changes!

@alex-courtis alex-courtis changed the title feat: custom filter functions feat(#2654): filters.custom may be a function Feb 11, 2024
@alex-courtis alex-courtis merged commit 4a87b8b into nvim-tree:master Feb 11, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a binaries field to nvim-tree-opts-filters or nvim-tree-opts-sort
2 participants