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 branch complete on attach #1579

Merged
merged 55 commits into from
Feb 27, 2023
Merged

Conversation

alex-courtis
Copy link
Member

@alex-courtis alex-courtis commented Sep 4, 2022

Feature branch containing all functionality to migrate from mappings to on_attach keymaps.

Resolves #1475

New

  • chore/action_cb_help_poc help displays vim mappings with their description
  • :help
  • migration notification (yes, some users will get upset) no need at this time
  • wiki
  • apply default mappings when no on_attach

Legacy

  • chore/migrate-legacy-mappings legacy migration of mappings to an on_attach function
  • chore/migrate-legacy-mappings-via-keymap remove mappings and dispatcher
  • chore/migrate-legacy-mappings-tool :NvimTreeGenerateOnAttach
  • action = "" translated to remove_keymaps
  • warn when remove_keymap or action = "" key not found as this has been consistently problematic RE case etc. silently handle removal of inexistent mappings
  • comment on removed keymap pattern
  • opts helper when mapping default and generated

@kyazdani42
Copy link
Member

@alex-courtis sorry i haven't been much here lately, i've been kind of tired to do anything related to IT to be honest.
Thank you for managing the plugin while i'm not here. I'm not sure when i'll be back to work on any of my github projects, if you feel like you don't want to maintain it alone in the meantime, i can try to find another maintainer, just tell me !

@gegoune
Copy link
Collaborator

gegoune commented Sep 28, 2022

@kyazdani42 Glad to hear you are well, burn out is a real thing, take your time!

Perhaps this is a good time to review merger with neo-tree - why duplicate work when resulting products are very similar?

Thanks for all your hard work!

@kyazdani42
Copy link
Member

:) a mixed-up feeling of burn-out and technology critique. Nevermind ^^
I think we could indeed check how to do a merger with neo-tree. I'll open an issue to discuss that with the maintainer.

@alex-courtis
Copy link
Member Author

@alex-courtis sorry i haven't been much here lately, i've been kind of tired to do anything related to IT to be honest. Thank you for managing the plugin while i'm not here.

Glad to hear you are back and not intending to stress yourself.

I'm not sure when i'll be back to work on any of my github projects, if you feel like you don't want to maintain it alone in the meantime, i can try to find another maintainer, just tell me !

Another maintainer would be great. There is a regular workload of bugs and PRs. I am able to dedicate time now, but that may change.

Perhaps we look at some PRs and see if anyone stands out that we could approach.

@gegoune : your ideas and different approaches to functionality have been most insightful. Perhaps you could be a maintainer, with a mandate of product management over development.

@gegoune
Copy link
Collaborator

gegoune commented Oct 1, 2022

I would be very happy to help to full extend of my abilities. Thank you for kind words @alex-courtis! ❤️

@alex-courtis
Copy link
Member Author

alex-courtis commented Feb 20, 2023

Testing 4ec73bb

  • actual mappings
  • help
  • :NvimTreeGenerateOnAttach
  • api.config.mappings.default, .active against master
actual help generated api
defaults OK OK defaults branch empty as expected
remove_keymaps = true empty empty empty OK
custom_only = true OK, master uses defaults, existing bug OK empty only defaults match
remove_keymaps = table OK OK defaults + removed OK
custom_only = true, list OK OK user only OK, master contains removed, existing bug
remove_keymaps = true, list OK OK user only OK
remove_keymaps = table, list OK OK defaults + removed + custom OK
list OK OK defaults + removed + custom OK
list empty OK OK defaults branch empty as expected
on_attach OK OK defaults branch empty as expected
local function print_node_path(node)
  print(node.absolute_path)
end

require("nvim-tree").setup({
  -- remove_keymaps = true,
  -- remove_keymaps = {
  --   -- inexistent
  --   "X",
  --   -- exists
  --   "Y",
  -- },
  view = {
    mappings = {
      -- custom_only = true,
      -- list = {
      --   -- new
      --   { key = "A", action = "cd" },
      --   { key = "?", action = "toggle_help" },

      --   -- override
      --   { key = "B", action = "cd" },

      --   -- remove existing
      --   { key = { "<2-RightMouse>" }, action = "" },
      --   { key = "C", action = "" },
      --   { key = { "D", "E" }, action = "" },

      --   -- remove inexistent
      --   { key = "G", action = "" },
      --   { key = { "L", "M" }, action = "" },

      --   -- custom overwrite
      --   { key = "F", action = "print_the_node_path", action_cb = print_node_path },

      --   -- custom new
      --   { key = { "N", "Q" }, action = "print_the_node_path", action_cb = print_node_path },
      -- },
    },
  },
  log = {
    enable = true,
    truncate = true,
    types = {
      dev = true,
    },
  },
})

local api = require("nvim-tree.api")
local log = require("nvim-tree.log")

local default = api.config.mappings.default()
for _, a in ipairs(default) do
  log.raw("dev", "D %s %s\n", vim.inspect(a.key), a.action)
end

local active = api.config.mappings.active()
for _, a in ipairs(active) do
  log.raw("dev", "A %s %s\n", vim.inspect(a.key), a.action)
end

@alex-courtis
Copy link
Member Author

Add [NvimTree] "No view.mappings.list for on_attach generation." message when using defaults / no legacy mappings.

@alex-courtis
Copy link
Member Author

I think we are finally ready to release this:

  • on_attach is generated from existing view.mappings - silent migration
  • new HELP
  • :help updated
  • :NvimTreeGenerateOnAttach added
  • wiki Migrating To on_attach

@gegoune I would be most grateful if you could review the docs, help, wiki etc and sanity test the migration wiki steps with your configuration.


Single right / middle mouse mappings will requre changes to |mousemodel| or |mouse|.

DEFAULT MAPPINGS *nvim-tree-default-mappings*
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed these as they are problematic to generate; DEFAULT_ON_ATTACH is quite readable.

@gegoune
Copy link
Collaborator

gegoune commented Feb 21, 2023

Of course.

Cases on all PR's branch:

  • no changes to configuration: everything works as before, no warning
  • with generated on_attach: all works fine!

We could turn last argument (opts) into some helper function like

  local opts = function(desc)
    return { desc = 'nvim-tree: ' .. desc, buffer = bufnr, noremap = true, silent = true, nowait = true }
  end

this will also add nice prefix automatically. Users can change it easily, but will avoid a lot of repeatition.

Also, mappings with action_cb did not get their code copied over. I can see that you added some skeleton there so it's good enough for me, probably quite hard to parse it out of configuration file.

Will make couple edits to wiki page in a bit, will also look through docs and make individual comments if I find anything.

Exceptional work on that PR Alex, well done!

@gegoune
Copy link
Collaborator

gegoune commented Feb 21, 2023

Noticed that

  vim.keymap.set('n', 'D', '', { buffer = bufnr })
  vim.keymap.del('n', 'D', { buffer = bufnr })

Is it easy enough to detect and remove that pair altogether? If not mentioning it on wiki might be enough.

Copy link
Collaborator

@gegoune gegoune left a comment

Choose a reason for hiding this comment

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

Excellent!

@alex-courtis
Copy link
Member Author

Is it easy enough to detect and remove that pair altogether? If not mentioning it on wiki might be enough.

That one is annoying. I failed at reliably detecting existing mappings so just gave up and mapped/unmapped. I'll add a comment in the generated file.

@alex-courtis
Copy link
Member Author

We could turn last argument (opts) into some helper function like

Heh... I have a helper like that in my config.

Yes. That will make the default and generated much easier to read.

@alex-courtis
Copy link
Member Author

Also, mappings with action_cb did not get their code copied over. I can see that you added some skeleton there so it's good enough for me, probably quite hard to parse it out of configuration file.

Yeah... we tried a few tricks but gave up... even getting the function name reliably is an issue.

@alex-courtis
Copy link
Member Author

Thank you so much for reviewing... this one is horrible.

I'll try and get the updates in tomorrow.

@gegoune
Copy link
Collaborator

gegoune commented Feb 26, 2023

There could be a comment on top of generated function saying something along those lines: "Attention: body of this function was automatically generated. Although care was taken to ensure (?)its correctness and completeness(/?) user's review is required. Please check for following issues in auto generated content: ".

I have been using that branch entire week. It just works.

@alex-courtis
Copy link
Member Author

I have been using that branch entire week. It just works.

This is great news.

I was tempted to try and revert some legacy bits to current, now that you've made on_attach much simpler, however working code should be used.

Finishing last little tasks and releasing...

@alex-courtis alex-courtis merged commit 7495975 into master Feb 27, 2023
@alex-courtis alex-courtis deleted the feat-branch-complete-on-attach branch February 27, 2023 03:19
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.

[Followup] full migration to on_attach API mappings
3 participants