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: Statusline caching and custom functions #155

Closed
wants to merge 4 commits into from
Closed

feat: Statusline caching and custom functions #155

wants to merge 4 commits into from

Conversation

abeldekat
Copy link

@abeldekat abeldekat commented Apr 12, 2024

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 abeldekat marked this pull request as ready for review April 12, 2024 08:22
@abeldekat abeldekat marked this pull request as draft April 12, 2024 08:22
@abeldekat abeldekat marked this pull request as ready for review April 12, 2024 12:58
@cbochs
Copy link
Owner

cbochs commented Apr 12, 2024

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,
})

@abeldekat
Copy link
Author

abeldekat commented Apr 12, 2024

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.

@cbochs
Copy link
Owner

cbochs commented Apr 12, 2024

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.

@abeldekat
Copy link
Author

abeldekat commented Apr 12, 2024

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
Copy link
Author

abeldekat commented Apr 12, 2024

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

@cbochs
Copy link
Owner

cbochs commented Apr 12, 2024

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

Why is the BufEnter autocommand needed here?

@abeldekat
Copy link
Author

When changing buffers the statusline needs to be updated:

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

@cbochs
Copy link
Owner

cbochs commented Apr 12, 2024

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
  • ...

@abeldekat
Copy link
Author

abeldekat commented Apr 12, 2024

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

@cbochs
Copy link
Owner

cbochs commented Apr 12, 2024

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).

@abeldekat
Copy link
Author

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

@abeldekat
Copy link
Author

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

@cbochs
Copy link
Owner

cbochs commented Apr 25, 2024

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: #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.

@abeldekat
Copy link
Author

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 abeldekat closed this Apr 26, 2024
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.

2 participants