feat: pass buffer options to hbac.telescope

al-ce commented 1 year ago

This commit adds a new module, hbac.telescope.results_opts, which filters the results of the entry picker to only show buffers that match the options passed to the picker. This commit supports the options listed in :h telescope.builtin.buffers() (except for bufnr_width, since bufnr is not displayed in the hbac picker)

Example usage:

vim.keymap.set("n", ",ht", function()
    local default_opts = {
        show_all_buffers = true,
        sort_mru = true,
        sort_lastused = true,
        ignore_current_buffer = true,
        cwd_only = false,
        cwd = nil,
    }
    require("hbac").telescope(default_opts)
end, { desc = "Hbac: pin picker", noremap = true })

addresses #9 #12

al-ce commented 1 year ago

@technicalpickles if you have time could you please give this a try and share your thoughts? Thanks!

technicalpickles commented 1 year ago

@al-ce thanks for putting this together! There are two issues I'm seeing:

the buffer list isn't immediately visible
there is an unnamed buffer in the list. if selected, it ends up just refocusing the current buffer

https://github.com/axkirillov/hbac.nvim/assets/159/468f574e-3e77-4ff2-ad4c-684d68d6703d

That said, it is ordering the way I would expect, which is great! I kinda suspect the visibility thing may be some other issue, as I saw it before trying this branch.

I'm not sure

al-ce commented 1 year ago

@technicalpickles Thanks for the feedback! I'm having a hard time replicating the unnamed buffer issue, I tried both stable and nightly nvim. I just pushed a commit that I doubt will address your issue but it does change some behavior regarding sort_lastused and ignore_current_buffer. Let me know if it still sorts as you expect it! (see my upcoming comments)

If you have time to do some investigating, I have some questions. Does the unnamed buffer still appear with ignore_current_buffer = false? If you have a session manager, could you open a new instance of Neovim with a fresh session? Does changing any of your default Telescope opts make any difference?

Could you change the lines 42-47 in hbac.telescope.makedisplay to the following:

        local bufpath = format_filepath()
        local display_filename = vim.fn.fnamemodify(bufname, ":t")
        if bufpath == "" then
            return bufnr .. " " .. display_filename
        end
        return bufnr .. " " .. display_filename .. " (" .. bufpath .. ")"

This will display the buffer number beside the results. Then, there's some things you can try:

In the Telescope prompt, enter normal mode and run :echo bufnr('%') to get the number of the prompt buffer (want to rule out whether the unnamed buffer is the picker buffer itself)
While still in normal mode with the Telescope prompt open, run :ls! to list all buffers including unlisted buffers. The Hbac telescope prompt should be something like 122u%a (u = unlisted, % = in the current window, a = active)
A command like :b5 would open buffer number 5. Where does :b{our mystery buffer number} take you?

Thanks!

al-ce commented 1 year ago

I just pushed a commit that rechecks the finder opts after every refresh which is relevant for keeping the current buffer hidden in the results if ignore_current_buffer == true. I also did some refactoring that doesn't use Telescope's 'flag' check to check the current buffer for sort_lastused since the active (%) and alternate (#) buffer aren't flagged the same as they are before a Telescope picker loads, so we'll just check a list of bufs sorted by recent use. This is needed because of the picker refresh.

I did run into another issue that I couldn't find a way around. The sorting function Telescope uses for sort_mru (which I copied for this pr) is:

  if opts.sort_mru then
    table.sort(bufnrs, function(a, b)
      return vim.fn.getbufinfo(a)[1].lastused > vim.fn.getbufinfo(b)[1].lastused
    end)
  end

However, lastused checks the timestamp in seconds. So if we switch buffers and open up a buffer picker (like Hbac's) in the same timestamp-second, or if we move around buffers too quickly, sort_mru won't look like one might expect. This happens in the builtin Telescope buffer picker as well. This probably isn't a real issue when we're doing actual work, but if we're just testing things manually we need to go slow so the timestamps can register.

axkirillov commented 1 year ago

Works great for me, thank you! We need to update the readme for this, and also the users of the Hbac command will have to switch to calling a lua function to have this behavior, which is not a big deal in itself, but, on the other hand, I would love the sorting (and excluding current buffer) to be the default behavior of the picker, what do you think?

al-ce commented 1 year ago

@axkirillov Thanks for the review! Yes I agree that we could pass some defaults to the Hbac telescope command. Should we add a default to the config table that the user can change easily? e.g. in hbac.config.values

    telescope = {
        opts = {
            sort_mru = true,
            ignore_current_buffer = true,
        },
        mappings = {
            n = {
                close_unpinned = "<M-c>",
              -- ...etc...

then in hbac.command.subcommands.telescope

M.telescope = function(opts)
    local hbac_telescope = require("hbac.telescope")
    if not hbac_telescope then
        return
    end
    local telescope_opts = require("hbac.config").values.telescope.opts
    opts = opts or telescope_opts
    hbac_telescope.pin_picker(opts)
end

So that any function call with an opts table would override the defaults. But with the opts table in the setup config, the user can still configure the Hbac telescope command. What do you think? Also, did I interpret correctly that the default should be ignore_current_buffer=true vs. sort_lastused=true true? If I implemented the options correctly, this would hide the current buffer (and its pinned state) from the picker, whereas the latter would set the selection cursor to the most recent selection (# or alternate) in the second row.

axkirillov commented 1 year ago

@axkirillov Thanks for the review! Yes I agree that we could pass some defaults to the Hbac telescope command. Should we add a default to the config table that the user can change easily? e.g. in hbac.config.values
  telescope = {
      opts = {
          sort_mru = true,
          ignore_current_buffer = true,
      },
      mappings = {
          n = {
              close_unpinned = "<M-c>",
              -- ...etc...

Yes, this is the perfect place 👍🏻

then in hbac.command.subcommands.telescope
M.telescope = function(opts)
  local hbac_telescope = require("hbac.telescope")
  if not hbac_telescope then
      return
  end
  local telescope_opts = require("hbac.config").values.telescope.opts
  opts = opts or telescope_opts
  hbac_telescope.pin_picker(opts)
end
So that any function call with an opts table would override the defaults. But with the opts table in the setup config, the user can still configure the Hbac telescope command. What do you think? Also, did I interpret correctly that the default should be ignore_current_buffer=true vs. sort_lastused=true true? If I implemented the options correctly, this would hide the current buffer (and its pinned state) from the picker, whereas the latter would set the selection cursor to the most recent selection (# or alternate) in the second row.

Ah, right, I didn't think about this. To me it seems that since the pinned states are part of unique functionality of this picker, hiding current buffer and its pinned state kinda works against it. So, probably the second option (just setting the cursor to the most recent) is a better default. But I think as the author of the picker you can decide whatever works best for you.

al-ce commented 1 year ago

Would you be ok with wrapping this PR up and waiting to push it to main until we resolve #11? I think it makes sense to push breaking changes to the mappings structure in the same update that we introduce the picker opts, since mappings is a Telescope opt anyway. This would let us combine the telescope opts table into a single one that matches the standard Telescope structure, e.g.

telescope = {
    sort_mru = true,
    sort_lastused = true,
    selection_strategy = "row",
    mappings = {
      n = {
        ["<M-c>"] = actions.close_unpinned,
        ["<M-x>"] = actions.delete_buffer,
        ["<M-a>"] = actions.pin_all,
        ["<M-u>"] = actions.unpin_all,
        ["<M-y>"] = actions.toggle_pin,
      },
      i = {
        -- etc.
      },
  },
  pin_icons = {
    pinned = { "󰐃 ", hl = "DiagnosticOk" },
    unpinned = { "󰤱 ", hl = "DiagnosticError" },
  },
},

These opts could be overridden in the user's setup config, and then the user config could be futher overridden by the opts in a keymap as suggested above. I can also bring those changes over into this PR if you want to do it all in one go. I think we'd be reducing the overall complexity and save ourselves and users some whiplash. Introducing standard Telescope opts is opening up a lot of simplification.

axkirillov commented 1 year ago

Would you be ok with wrapping this PR up and waiting to push it to main until we resolve #11? I think it makes sense to push breaking changes to the mappings structure in the same update that we introduce the picker opts, since mappings is a Telescope opt anyway. This would let us combine the telescope opts table into a single one that matches the standard Telescope structure, e.g.
telescope = {
    sort_mru = true,
    sort_lastused = true,
    selection_strategy = "row",
    mappings = {
      n = {
        ["<M-c>"] = actions.close_unpinned,
        ["<M-x>"] = actions.delete_buffer,
        ["<M-a>"] = actions.pin_all,
        ["<M-u>"] = actions.unpin_all,
        ["<M-y>"] = actions.toggle_pin,
      },
      i = {
        -- etc.
      },
  },
  pin_icons = {
    pinned = { "󰐃 ", hl = "DiagnosticOk" },
    unpinned = { "󰤱 ", hl = "DiagnosticError" },
  },
},
These opts could be overridden in the user's setup config, and then the user config could be futher overridden by the opts in a keymap as suggested above. I can also bring those changes over into this PR if you want to do it all in one go. I think we'd be reducing the overall complexity and save ourselves and users some whiplash. Introducing standard Telescope opts is opening up a lot of simplification.

Sure, we can also create a develop branch and merge it there

technicalpickles commented 1 year ago

I just tested against the latest on this branch, and it is working as expected without any other changes 🎉

    {
        "al-ce/hbac.nvim",
        branch = "picker_opts",
        opts = {
            -- threshold = 40,
            telescope = {
                pin_icons = {
                    unpinned = { "" },
                    pinned = { "󰐃", hl = "DiagnosticError" },
                },
            },
        },
        cmd = "Hbac",
        event = "VeryLazy",
        keys = {
            {
                "<D-k>",
                function()
                    local default_opts = {
                        show_all_buffers = true,
                        sort_mru = true,
                        sort_lastused = true,
                        ignore_current_buffer = true,
                        cwd_only = false,
                        cwd = nil,
                    }
                    require("hbac").telescope(default_opts)
                end,
            },
        },
    },

al-ce commented 1 year ago

I rebased and undid the commit that integrated the telescope.opts table that would have set defaults for the Hbac telescope command. I'll merge this into develop and open a new PR for the mapping restructure and the default opts, hope I've understood correctly about the workflow!

axkirillov / hbac.nvim

feat: pass buffer options to hbac.telescope #13