axkirillov / hbac.nvim

Heuristic buffer auto-close
MIT License
200 stars 11 forks source link

feat: Telescope picker #5

Closed al-ce closed 1 year ago

al-ce commented 1 year ago

Copying the commit message here for convenience:


:Hbac telescope or require("hbac").telescope() spawns a Telescope picker of listed buffers with pin state indicators in the first column. Icons for the pin state and mappings for the picker's commands have defaults that can be overridden in the setup config. The following picker commands are available:

After each command, the picker is refreshed and the pin icons are updated. Pressing the default action key (typically enter) opens the selected buffer in the current window.

This commit updates the module structure in order to avoid circular imports between the command and telescope modules. The command module is now in its own directory. Both the main command module and the telescope module call command functions from command.subcommands. In order to avoid displaying notifications of Hbac events while Telescope commands are executed, the notifications have been removed from the main subcommand functions and moved into the main command module. The notifications have also been changed from print to vim.notify functions.

The refactoring and renaming of files changes code made by @axkirillov and @ditsuke.


al-ce commented 1 year ago

If this PR is too big, we can close it and start smaller! Feel free to push any changes big or small as you see fit. Thank you!

https://github.com/al-ce/hbac.nvim/assets/23170004/d9e6f03d-451b-455e-be84-caf112ff5595

axkirillov commented 1 year ago

I get image

al-ce commented 1 year ago

I get

Sorry about that, must be something from this change.

https://github.com/al-ce/hbac.nvim/blob/9ad3d209fa5f22779b45219c237a544e6d48a091/lua/hbac/telescope/init.lua#L71-L82

But I'm having a hard time figuring out what I've got in my config that's suppressing that error. Can I please see your opts in your hbac config, if you have any? Does the error show up on vim startup or when you run a specific Hbac command?

axkirillov commented 1 year ago

@al-ce I'm relying on defaults

    {
        'axkirillov/hbac.nvim',
        config = function()
            require("hbac").setup()
        end,
        branch = "test"
    }
axkirillov commented 1 year ago

I get

Sorry about that, must be something from this change.

https://github.com/al-ce/hbac.nvim/blob/9ad3d209fa5f22779b45219c237a544e6d48a091/lua/hbac/telescope/init.lua#L71-L82

But I'm having a hard time figuring out what I've got in my config that's suppressing that error. Can I please see your opts in your hbac config, if you have any? Does the error show up on vim startup or when you run a specific Hbac command?

I think the first argument to map function is supposed to be string

al-ce commented 1 year ago

I think the first argument to map function is supposed to be string

I pushed that change, and using a table there was unnecessary anyway since the function is mapping one mode at a time regardless. Any change?

axkirillov commented 1 year ago

I think the first argument to map function is supposed to be string

I pushed that change, and using a table there was unnecessary anyway since the function is mapping one mode at a time regardless. Any change?

Now it works, thanks.

One nitpick image help should have a hint for those mappings

axkirillov commented 1 year ago

https://github.com/axkirillov/hbac.nvim/assets/32141102/abb4ec26-f75f-49e2-9b2e-40fa35895e99

Using the toggle_selection resets the select position back to downmost entry. In case of multiple selection it makes sense, but if I am pinning one buffer at the top I expect the cursor / selection not to move Perhaps it make sense to have a separate mapping for single buffer pinning? This must be some telescope mechanics that we can't influence, isn't it?

al-ce commented 1 year ago

Thanks for the catch on the mapping issue!

Using the toggle_selection resets the select position back to downmost entry. In case of multiple selection it makes sense, but if I am pinning one buffer at the top I expect the cursor / selection not to move

Yeah that's been bugging me too. Also I noticed that the 'unpinned' icon's bg highlight doesn't change when an unpinned buffer is selected, I didn't notice that with my colorscheme/highlights, I'd like to fix that.

al-ce commented 1 year ago

This must be some telescope mechanics that we can't influence, isn't it?

I think the current issue is that in order to update the pin status visual cue (the icons), the refresh function redraws the entire picker, but I'm hoping there's a way to store the row position and set it back. Navigating the Telescope codebase is kinda challenging but good exercise. This maybe looks promising? set_selection Picker:set_selection(row)

My next step is to dig through these search results for picker:refresh and see what others do in this scenario.

al-ce commented 1 year ago

Summary of last three commits

axkirillov commented 1 year ago

This looks good to me. I will test it some more today. I am going on vacation this week, so you guys can merge it, or we will merge it next week then.

al-ce commented 1 year ago

This looks good to me. I will test it some more today. I am going on vacation this week, so you guys can merge it, or we will merge it next week then.

Thanks for the opportunity to work with you again, and @ditsuke too! I appreciate the learning experience. Enjoy your vacation. I'll squash down to fewer commits at some point.

ditsuke commented 1 year ago

Thanks for working on this @al-ce. I'm busy because of exams right now so it'll take me around a week to add the picker to my own configuration, lets keep merging on hold for a while but everything LGTM from back when I reviewed and tested it last.

axkirillov commented 1 year ago

Trying to delete a buffer results in a following error

Never mind, I was using an old mapping

al-ce commented 1 year ago

Are either of you having issues with the main hbac branch after upgrading to neovim 0.9.1? Sorry I didn't test this properly, I'm noticing right before I run out for the day. The autoclose thershold isn't being respected and the close_unpinned command gives me this error:

Error executing lua callback: Vim:E315: ml_get: invalid lnum: 1
stack traceback:
    [C]: in function 'bufload'
    vim/_editor.lua: in function 'region'
    ...ar/neovim/0.9.1/share/nvim/runtime/lua/vim/highlight.lua:35: in function 'range'
    ...share/nvim/lazy/nvim-ts-rainbow/lua/rainbow/internal.lua:86: in function 'update_range'
    ...share/nvim/lazy/nvim-ts-rainbow/lua/rainbow/internal.lua:194: in function 'cb'
    ...1/share/nvim/runtime/lua/vim/treesitter/languagetree.lua:736: in function '_do_callback'
    ...1/share/nvim/runtime/lua/vim/treesitter/languagetree.lua:176: in function 'invalidate'
    ...1/share/nvim/runtime/lua/vim/treesitter/languagetree.lua:877: in function '_on_detach'
    ...r/neovim/0.9.1/share/nvim/runtime/lua/vim/treesitter.lua:73: in function <...r/neovim/0.9.1/share/nvim/runtime/lua/vim/treesitter.lua:69>
    [C]: in function 'nvim_buf_delete'
    ...rani/.local/share/nvim/lazy/hbac.nvim/lua/hbac/setup.lua:9: in function 'close_command'
    ...ni/.local/share/nvim/lazy/hbac.nvim/lua/hbac/command.lua:13: in function <...ni/.local/share/nvim/lazy/hbac.nvim/lua/hbac/command.lua:8>
    ...ni/.local/share/nvim/lazy/hbac.nvim/lua/hbac/command.lua:40: in function 'vim_cmd_func'
    ...rani/.local/share/nvim/lazy/hbac.nvim/lua/hbac/setup.lua:41: in function <...rani/.local/share/nvim/lazy/hbac.nvim/lua/hbac/setup.lua:40>

I'll check in a minimal Neovim config later and open an issue if it seems like it's not a personal issue. I'm just mentioning this here so we don't move forward with anything Telescope related in case we need to refocus our priorities. Thanks

axkirillov commented 1 year ago

@al-ce I havent updated yet

axkirillov commented 1 year ago

Hey, looking at the stack traceback I am very confused. There is no command.lua file in your branch. setup.lua:9 is also not a call to close_command. Are you sure you are on correct branch? Sorry, should have noticed earlier that you are talking about main branch. Have you tried this one though?

axkirillov commented 1 year ago

I think we can still merge the pr. We should add it to readme that it doesnt work on 9.1. My intuition is that most people will not upgrade right away. We can then tackle this issue separately in a separate issue.

al-ce commented 1 year ago

I think we can still merge the pr. We should add it to readme that it doesnt work on 9.1. My intuition is that most people will not upgrade right away. We can then tackle this issue separately in a separate issue.

Sounds good, sorry for the panicky message above, I didn't have time to check if I had everything set up correctly.

al-ce commented 1 year ago

The issue with 0.9.1 was with conflicting plugins that I didn't disable, working fine with an as-minimal-as-possible config. Sorry for the confusion. (so there's nothing to add to the readme)

@ditsuke thanks for testing. I agree about using sort_mru, that would be a good default option. This was my first time implementing a telescope picker so there's a lot of sensible options I may have missed. I can wait to make the PR for the commands/subcommands modules refactoring until after you make one for that (I'm also not attached to the current default icons).

Thanks everyone!

axkirillov commented 1 year ago

@ditsuke im also indifferent to which icons, so feel free to make yours the default 😁

ditsuke commented 1 year ago

Changed the default icons. We're ready for a merge