grapple.nvim icon indicating copy to clipboard operation
grapple.nvim copied to clipboard

feat: Statusline caching and custom functions

Open abeldekat opened this issue 1 year ago • 13 comments

This PR implements issue feature: Statusline caching and custom functions #147 and adds the following:

  1. Statusline caching.
  2. A uniform hook for the user to implement a statusline, reusing the caching and the data provided by grapple.
  3. Builtin "update" support for various statusline plugins.

Changes

  1. Added lua component grapple.statusline and corresponding tests.
  2. Added properties to grapple.settings.statusline.
  3. Api Grapple.statusline: Uses the new component. Does not return nil anymore.
  4. The lualine grapple component: Changed to use the new version of Grapple.statusline.

The second lualine example in the docs uses two api methods which are not changed:

function Grapple.exists(opts)
function Grapple.name_or_index(opts)

The same can now be achieved by using:

Grapple.setup({ statusline = { builtin_formatter = "short" }})

Considerations

The statusline is opt-in and only activates when the user invokes Grapple.statusline. As such it is a top level component.

Optimize the number of steps in the code needed to produce a line after the initial setup. As a consequence, method Grapple.statusline has no opts argument that would need to be merged.

Ultimately, only 2 steps are needed:

  • Get the statusline component: require("grapple.statusline").get()
  • Display its cached line: line:format()

Works across statusline plugins in the same manner. Currently, the second example in the docs is lualine specific.

Autodetect the statusline plugin in use and provide an on_event callback. This considerably improves the responsiveness of lualine.

Suggestions

Nice to have perhaps: Extend the default formatter by adding empty_slots, more_marks and scope_name. See test case "custom formatter" and the grappleline plugin.

Remove the second lualine example from the docs, in favor of builtin_formatter = "short".

Questions

Api

The new statusline component has a public api:

Statusline.formatters
function Statusline.get()
function Statusline:format()
function Statusline:is_current_buffer_tagged()

What's a good way to distinguish that api from all the other methods in the component?

Events

When using event GrappleUpdate, the dreaded nested error occurred:


function Statusline:subscribe_to_events()
    vim.api.nvim_create_autocmd({ "BufEnter" }, {
        group = STATUSLINE_GROUP,
        pattern = "*",
        callback = function()
            self:update()
        end,
    })
    vim.api.nvim_create_autocmd({ "User" }, { -- no scope name in event
        group = STATUSLINE_GROUP,
        pattern = "GrappleScopeChanged",
        callback = function()
            self:update()
        end,
    })
    vim.api.nvim_create_autocmd({ "User" }, -- crash
        group = STATUSLINE_GROUP,
        nested = false,
        pattern = "GrappleUpdate",
        callback = function()
            self:update()
        end,
    })
end

Perhaps it's better to not use the events provided by grapple. In my opinion, the api decoration is a good solution.

See also

Using mini.statusline in my config: config-mini.statusline config-grapple deps-editor deps-ui

Todo

Update the documentation.

abeldekat avatar Apr 12 '24 08:04 abeldekat

Hey @abeldekat! Thanks for taking the time to improve statusline integration for Grapple.

So, I decided to go ahead and do some very basic benchmarking here. Calling Grapple.statusline on a scope with 10 tags yields the following:

Without caching

Benchmark results:
  - 10000 function calls
  - 534.6490 milliseconds elapsed
  - 0.0535 milliseconds avg execution time.

With caching

Benchmark results:
  - 10000 function calls
  - 4.1180 milliseconds elapsed
  - 0.0004 milliseconds avg execution time.

While the cached result is faster, the uncached result is still negligible compared to the polling rate of 1000ms in lualine. In addition, would this be mostly (entirely?) mitigated by using a statusline plugin which natively supports an autocommand-driven statusline (e.g. heirline)?

Autodetect the statusline plugin in use and provide an on_event callback. This considerably improves the responsiveness of lualine.

I do feel that lualine can lack in "responsiveness" sometimes, which I liked that you pointed out. Would this be resolved with the following autocommand (something a user could add to their config)?

vim.api.nvim_create_autocmd("User", {
    pattern = { "GrappleUpdate", "GrappleScopeChanged" },
    callback = vim.schedule_wrap(function()
        require("lualine").refresh()
    end,
})

cbochs avatar Apr 12 '24 17:04 cbochs

You're welcome! I had a lot of fun working on this PR...)

... the uncached result is still negligible compared to the polling rate of 1000ms in lualine

I currently work on a slow laptop(intel i3, 4G RAM). I regularly see "hesitations" when working withlualine and grapple. Perhaps because of that "polling". When the event occurs just after lualine polled, one second to the next poll is noticeable.

I think the benchmarking results are impressive.

I do feel that lualine can lack in "responsiveness" sometimes, which I liked that you pointed out. Would this be resolved with the following autocommand (something a user could add to their config)?

Frankly, I don't know. I could not get the new component to work with GrappleUpdate. But, in general, I think it would be best if the user does not have to add lualine code at all. Most users likely would just add the default "grapple" component and be done with it. Supporting a builtin on_event for the most-used statusline plugins seemed appropriate for an out-of-the-box grapple setup.

abeldekat avatar Apr 12 '24 17:04 abeldekat

I currently work on a slow laptop(intel i3, 4G RAM). I regularly see "hesitations" when working withlualine and grapple. Perhaps because of that "polling". When the event occurs just after lualine polled, one second to the next poll is noticeable.

I wouldn't mind digging into this a bit. Would you mind clarifying what you mean by "hesitations"? Do you find neovim slowing down when you use lualine and grapple? Is the slowness fixed by removing one (or both) of them? Or, do you just mean that lualine doesn't always immediately reflect the current state of grapple (i.e. add a tag, but it doesn't show up right away)?

Frankly, I don't know. I could not get the new component to work with GrappleUpdate.

I have an idea of what the culprit might be here: lua/grapple.lua#L675. Grapple watches for QuitPre, which does one last update before neovim quits. This means that the GrappleUpdate autocommand can be called from that final update. If there is an autocommand watching GrappleUpdate which is refreshing lualine, then it will cause neovim to crash. You should be able to reproduce with the following:

  1. Create an autocommand like:
    vim.api.nvim_create_autocmd("User", {
        group = vim.api.nvim_create_augroup("test", { clear = true })
        pattern = "GrappleUpdate",
        callback = vim.schedule_wrap(function()
            require("lualine").refresh()
        end,
    })
    
  2. Tag the current file :Grapple tag
  3. Attempt to quit :q
  4. Neovim will crash/hang

However, I don't necessarily think this is Grapple's fault. Notice, if you replace (in the above autocommand) require("lualine").refresh() with something simple like print("asdf"), neovim will not crash/hang.

cbochs avatar Apr 12 '24 18:04 cbochs

Or, do you just mean that lualine doesn't always immediately reflect the current state of grapple (i.e. add a tag, but it doesn't show up right away)?

The latter indeed. I should have been more clear by using the word "responsiveness".

I have an idea of what the culprit might be here

The error occurs on startup when first using grapple select.

stacktrace
Error executing Lua callback: ...nvim/site/pack/deps/opt/grapple.nvim/lua/grapple/tag.lua:36: BufEnter Autocommands for "*"..User Autocommands for "GrappleUpdate"..User Autoc
ommands for "GrappleUpdate"..User Autocommands for "GrappleUpdate"..User Autocommands for "GrappleUpdate"..User Autocommands for "GrappleUpdate"..User Autocommands for "Grapp
leUpdate"..User Autocommands for "GrappleUpdate"..User Autocommands for "GrappleUpdate"..User Autocommands for "GrappleUpdate": Vim(append):E218: Autocommand nesting too deep

stack traceback:
        [C]: in function 'command'
        ...nvim/site/pack/deps/opt/grapple.nvim/lua/grapple/tag.lua:36: in function 'select'
        ...are/nvim/site/pack/deps/opt/grapple.nvim/lua/grapple.lua:147: in function 'callback'
        ...e/pack/deps/opt/grapple.nvim/lua/grapple/tag_manager.lua:54: in function 'enter'
        ...nvim/site/pack/deps/opt/grapple.nvim/lua/grapple/app.lua:189: in function 'enter_without_save'
        ...are/nvim/site/pack/deps/opt/grapple.nvim/lua/grapple.lua:135: in function <...are/nvim/site/pack/deps/opt/grapple.nvim/lua/grapple.lua:129>
        ...are/nvim/site/pack/deps/opt/grapple.nvim/lua/grapple.lua:678: in function <...are/nvim/site/pack/deps/opt/grapple.nvim/lua/grapple.lua:650>

However, I don't necessarily think this is Grapple's fault.

I don't think so as well. I do think that decorating grapple's api to push events works really well. Might even be a bit faster as it does not involve Neovim's autocommand machinery. If that api changes in the future, the automated tests ensure that any breakage is found immediately.

abeldekat avatar Apr 12 '24 18:04 abeldekat

It works when using schedule_wrap for GrappleUpdate (mini.statusline)

function Statusline:subscribe_to_events()
    vim.api.nvim_create_autocmd({ "BufEnter" }, {
        group = STATUSLINE_GROUP,
        pattern = "*",
        callback = function()
            self:update()
        end,
    })
    vim.api.nvim_create_autocmd({ "User" }, {
        group = STATUSLINE_GROUP,
        pattern = "GrappleScopeChanged",
        callback = function()
            self:update()
        end,
    })
    vim.api.nvim_create_autocmd({ "User" }, {
        group = STATUSLINE_GROUP,
        nested = false,
        pattern = "GrappleUpdate",
        callback = function()
            vim.schedule_wrap(function()
                self:update()
            end)
        end,
    })
end

abeldekat avatar Apr 12 '24 18:04 abeldekat

    vim.api.nvim_create_autocmd({ "BufEnter" }, {
        group = STATUSLINE_GROUP,
        pattern = "*",
        callback = function()
            self:update()
        end,
    })

Why is the BufEnter autocommand needed here?

cbochs avatar Apr 12 '24 19:04 cbochs

When changing buffers the statusline needs to be updated:

State 1: 1 [2] 3 vim.cmd.edit("4") State 2: 1 2 3

abeldekat avatar Apr 12 '24 19:04 abeldekat

Okay, so the issue is here.

  • Grapple.select triggers a GrappleUpdate event
  • GrappleUpdate invokes the Statusline autocommands
  • Statusline autocommand calls self:update()
  • self:update() calls Grapple.find
  • Grapple.find triggers a GrappleUpdate event
  • ...

cbochs avatar Apr 12 '24 19:04 cbochs

Grapple.find triggers a GrappleUpdate event

Why? I don't expect select or find to create an update event.

Update:

I would expect the event to happen when calling the api methods I decorated in subscribe_to_api

abeldekat avatar Apr 12 '24 19:04 abeldekat

The purpose was intended that any interaction with Grapple (tag, untag, select) would cause an update (see: lua/grapple/tag_manager.lua#L43-L73). It was probably an oversight to have it trigger for API calls which only query Grapple (find, exist, name_or_index).

cbochs avatar Apr 12 '24 19:04 cbochs

That explains. What is your opinion on the current approach, using method subscribe_to_api in combination with the BufEnter event?

abeldekat avatar Apr 12 '24 19:04 abeldekat

I added one commit, improving the custom formatter example in the tests

abeldekat avatar Apr 19 '24 09:04 abeldekat

Hi @abeldekat, apologies for the delay here. Was unable to continue reviewing the other week.

I had a chance to investigate the event issue. I don't think that the decoration approach is quite the solution. I took a bit of time to refactor how the Grapple API and App work to make it so that the events now trigger in the right place. You can see the PR here: https://github.com/cbochs/grapple.nvim/pull/158 (It's 95% moving code around).

I also took another look over this PR. After some thought, I don't think this is something that I want Grapple to maintain in the long run. This is primarily due to a few things:

  1. Caching the statusline should be left up to the statusline (i.e. lualine), or other intermediate plugin. In addition, the cost of recomputing the statusline (from what we found out above) is negligible.
  2. Formatting what is displayed in the statusline should also be left up to the user. While there is a simple (default) implementation provided, anything more "custom" should be made possible with Grapple's API rather than builtin.
  3. Refreshing the statusline should also be left up to the user. Of course there should be indicators (i.e. GrappleUpdate and GrappleScopeChanged) which notify the user, but that should be the extent in which Grapple is involved.

For those reasons, I don't think I'll be able to merge this PR. However, I would be happy to add grappleline to the README for users who want that kind of flexibility with lualine as I do think there's some great value add, especially for those using lualine (and mini.statusline) which does not natively support autocommand-driven updates.

cbochs avatar Apr 25 '24 18:04 cbochs

Hello @cbochs,

I understand your choice.

The grappleline plugin was initially intended as a proof of concept. I don't think grappleline adds enough value to maintain as a plugin, as grapple.nvim already provides a statusline.

I added the code to my own config and intend to remove the plugin.

Best regards!

abeldekat avatar Apr 26 '24 08:04 abeldekat