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: add update_focused_file.exclude #2673

Merged
merged 11 commits into from
Mar 25, 2024

Conversation

ava5627
Copy link
Contributor

@ava5627 ava5627 commented Feb 15, 2024

Create option update_focus_file.exclude for files that shouldn't be auto focused like gitcommit files.
I also moved update_focused_file.ignore_list to update_focused_file.update_root.ignore_list and made update_focused_file.update_root.enable to differentiate between them and show that ignore_list is only about not updating the root.

resolves #2444

…ist to update_focused_file.update_root.ignore_list
@ava5627 ava5627 changed the title Add update_focused_file.exclude feat: Add update_focused_file.exclude Feb 15, 2024
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, we might have a bit of an issue with filetypes though:

  update_focused_file = {
    enable = true,
    ignore_list = { "config", "one", },
    update_root = {
      enable = true,
    },
    exclude = { "foo", "c", },
  },
echo "foo" > foo; echo "bar" > bar.md; echo "baz" > baz.c; echo "nil" > nil.txt

Open bar and nil then b#. No focus file change. Remove "c" and everything works as intended.

lua/nvim-tree/legacy.lua Outdated Show resolved Hide resolved
doc/nvim-tree-lua.txt Outdated Show resolved Hide resolved
@gegoune
Copy link
Collaborator

gegoune commented Feb 17, 2024

I would like to review all the options at some point. :)

@ava5627
Copy link
Contributor Author

ava5627 commented Feb 18, 2024

Open bar and nil then b#. No focus file change. Remove "c" and everything works as intended.

I'm getting focus change even with "c"

focus.mp4

@alex-courtis
Copy link
Member

alex-courtis commented Feb 18, 2024

Open bar and nil then b#. No focus file change. Remove "c" and everything works as intended.

I'm getting focus change even with "c"

Well, that looks great - working as intended.

Testing at /home/alex/src/nvim-tree/r/2673 fails, /home/alex/2673 works nicely.

Is this an issue with matching the absolute path?

exclude = { "oo", }, excludes foo and exclude = { "2673", }, excludes everything.

@alex-courtis
Copy link
Member

I would like to review all the options at some point. :)

Yes. They have gotten out of control again.

@ava5627
Copy link
Contributor Author

ava5627 commented Feb 18, 2024

Is this an issue with matching the absolute path?

Yep that was it. This should fix the issue

@ava5627
Copy link
Contributor Author

ava5627 commented Feb 18, 2024

Should partial matches also be excluded or only exact matches? I went with how ignore_list was written which does exclude partial matches

@alex-courtis
Copy link
Member

Is this an issue with matching the absolute path?

Yep that was it. This should fix the issue

Works nicely now.

@alex-courtis
Copy link
Member

Should partial matches also be excluded or only exact matches? I went with how ignore_list was written which does exclude partial matches

Well, we're in a lovely mess here. A quick audit of similar lists:

exact:

  • root_dirs
  • renderer.special_files
  • actions.expand_all.exclude

partial:

  • filters.exclude

partial and filetype:

  • update_focused_file.ignore_list
  • tab.sync.ignore

regex:

  • filters.custom
  • filesystem_watchers.ignore_dirs

What I do like is nvim-tree.actions.open_file.window_picker.exclude with its explicit filetype and buftype, although it doesn't do names/paths.

@alex-courtis
Copy link
Member

This is core functionality that will likely be used by many; I'm definitely going to take adantage of this as it is far superior to the custom filter hack.

How about we give full control to the user? Let them match a path or a name or a filetype explicitly.

They can optionally use regexes for name/path.

buftype doesn't make much sense however we could add it if a use case comes up.

Something like:

  update_focused_file = {
    enable = true,
    exclude = {
      name = {
        "gitcommit",
        "foo.*",
      },
      path = {
        "/tmp.*",
        "/bar/baz",
      },
      filetype = { "c", "markdown" },
    },
  },

What do you think @gegoune ? I hear your concerns around options...

@gegoune
Copy link
Collaborator

gegoune commented Feb 18, 2024

To be aligned with some of the other options should we also accept function returning table?

  update_focused_file = {
    enable = true,
    exclude = {
      name = fun|table,
      path = fun|table,
      filetype = fun|table,
    },
  },

Or is it too much? When would we evaluate that function? I can potentially see use case where user would like to have different behaviours based on CWD perhaps?

@gegoune
Copy link
Collaborator

gegoune commented Feb 18, 2024

Since v1 is released

I also moved update_focused_file.ignore_list to update_focused_file.update_root.ignore_list and made update_focused_file.update_root.enable to differentiate between them and show that ignore_list is only about not updating the root.

is a breaking change. Updating title accordingly.

Also, @alex-courtis perhaps when this PR is ready we should wait a bit and take this opportunity to introduce other breaking changes at the same time to avoid multiple major version bumps.

Edit: only noticed the legacy thing now. How do we want to treat those, @alex-courtis? Migrating option in the background isn't really breaking after all.

@gegoune gegoune changed the title feat: Add update_focused_file.exclude feat!: Add update_focused_file.exclude Feb 18, 2024
@alex-courtis
Copy link
Member

alex-courtis commented Feb 18, 2024

Or is it too much? When would we evaluate that function? I can potentially see use case where user would like to have different behaviours based on CWD perhaps?

There are many possible ways a user could use this exclude. A function makes sense.

edit: add bufnr

How about we only accept a function e.g.

  update_focused_file = {
    enable = true,
    exclude = function(node, bufnr)
      return node and node.name = "gitcommit" or bufnr and vim.api.nvim_buf_get_option(bufnr, "filetype") == "zig"
    end,
  },

How do we want to treat those, @alex-courtis? Migrating option in the background isn't really breaking after all.

That was the idea; I think we agreed to keep those forever. See #2679

@alex-courtis
Copy link
Member

Any updates on this one @ava5627 ?

I'm really keen to get this one as it's very useful functionality. I know I will be using it.

@ava5627
Copy link
Contributor Author

ava5627 commented Mar 24, 2024

Been more busy at work the past couple weeks and forgot about this for a bit. Exclude now is a function that takes the BufEnter event like this

update_focused_file = {
     enable = true,
     exclude = function(event)
         return vim.api.nvim_buf_get_option(event.buf, "filetype") == "gitcommit" or vim.fn.expand("%"):match("site%-packages")
     end,
},

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.

Working nicely for me:
alex-courtis/arch@3095564

Last nits...

enable = false,
ignore_list = {},
},
exclude = function()
Copy link
Member

Choose a reason for hiding this comment

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

Please use false here, rather than a function, following the convention of similar. This keeps the config small.

Please put a new "function" definiton in ACCEPTED_TYPES so that validation may occur.

You'll need to update help as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

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.

Working nicely, many thanks for your contribution!

@alex-courtis alex-courtis changed the title feat!: Add update_focused_file.exclude feat: add update_focused_file.exclude Mar 25, 2024
@alex-courtis alex-courtis merged commit e20966a into nvim-tree:master Mar 25, 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.

update_focused_file.exclude
3 participants