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

fix: fatal with invalid buffer #114

Closed
wants to merge 2 commits into from
Closed

Conversation

Parsifa1
Copy link

@Parsifa1 Parsifa1 commented Dec 1, 2023

image

Before executing buf_get_lines, add a check to see if the buf_id is valid.

Copy link
Collaborator

@willothy willothy left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

The change itself looks good to me, but have you determined what the cause of the error is (beyond just an invalid buffer) / how it can be reproduced? Ideally, we shouldn't be attempting to parse an invalid buffer in the first place, so I think this should be investigated further.

@@ -68,6 +68,10 @@ local function parse_buf(buf, lnum_end, incremental)
symbols_parsed.symbols = {}
symbols_parsed['end'] = { lnum = 0, inside_code_block = false }
end
-- if buf is invalid, return
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small nit, but I think the code is self-explanatory and a comment isn't needed.

@Bekaboo
Copy link
Owner

Bekaboo commented Dec 1, 2023

@Parsifa1 As @willothy suggests, please tell us how to reproduce this issue.

@Parsifa1
Copy link
Author

Parsifa1 commented Dec 2, 2023

It seems the real problem comes from this plugin
it provides a feature to Resume previous session. When there is a markdown buffer, it will be resumed by that plugin, then the issue appeared.
In a word, this problem does not come from the functionality of dropbar and Vanilla neovim.
but there's nothing wrong in this "buffer valid check",right?

@Bekaboo
Copy link
Owner

Bekaboo commented Dec 2, 2023

Thanks, if the function relies on the validity of buf, better to check it at the very beginning instead of in the middle of the function to avoid unnecessary computation.

@Parsifa1
Copy link
Author

Parsifa1 commented Dec 2, 2023

This is my first time making some pr stuff, maybe a little bad, ...Thank you anyway!

@Bekaboo
Copy link
Owner

Bekaboo commented Dec 2, 2023

This is my first time making some pr stuff, maybe a little bad

It is not bad at all. Thanks for reporting this issue and coming up with the PR, it helps us making dropbar.nvim more stable. Please keep goinig!

@willothy
Copy link
Collaborator

willothy commented Dec 2, 2023

This is my first time making some pr stuff, maybe a little bad

It is not bad at all. Thanks for reporting this issue and coming up with the PR, it helps us making dropbar.nvim more stable. Please keep goinig!

+1 to this, it's not bad in any way! Thank you for the PR, it is much appreciated even if it didn't end up being merged.

If you're ever interested in making another PR here I'd be happy to guide that process / answer any questions about the codebase :)

@willothy
Copy link
Collaborator

willothy commented Dec 2, 2023

Thanks, if the function relies on the validity of buf, better to check it at the very beginning instead of in the middle of the function to avoid unnecessary computation.

I'm still confused as to why this was happening in the first place though... Maybe we should make an issue for this so the root cause can be investigated, even if it's not technically an issue anymore?

I don't think the :del() method should be triggering the parsing of a buffer, right? It's likely something in that method is invoking the __index metamethod which parses the buffer. Maybe the check for self.buf? If so, using rawget could solve this issue without any kind of buffer validity check.

@Bekaboo
Copy link
Owner

Bekaboo commented Dec 2, 2023

I'm still confused as to why this was happening in the first place though... Maybe we should make an issue for this so the root cause can be investigated, even if it's not technically an issue anymore?

Yeah, but this depends on @Parsifa1 to provide minimal production steps since I haven't had the issue.

I don't think the :del() method should be triggering the parsing of a buffer, right?

You are right, :del() should not require any sources.

It's likely something in that method is invoking the __index metamethod which parses the buffer. Maybe the check for self.buf? If so, using rawget could solve this issue without any kind of buffer validity check.

I am not quite sure what is your solution here. If the __index() method of _G.dropbar.bars() creates a new dropbar instance AND (1) that instance has markdown as its source AND (2) the dropbar is attached to a markdown buffer, then the get_symbols() function of the markdown source will be called. If the buffer is unloaded the attached bar should be deleted by dropbar_t:del().

@willothy
Copy link
Collaborator

willothy commented Dec 2, 2023

I'll try to reproduce based on their description, since I already have project.nvim installed.

I am not quite sure what is your solution here.

You're right, looking back over the code I don't think I was correct. I'll try to reproduce with my config, but if that doesn't work we can just wait for repro steps.

Edit: oops, wrong project.nvim lol I use a different one with essentially the same name. I'll install it and see if I can repro anyways though.

@Parsifa1
Copy link
Author

Parsifa1 commented Dec 2, 2023

it's this plugin
https://github.com/coffebar/neovim-project

As for the minimum configuration
for neovim-project.

return {
    "coffebar/neovim-project",
    -- cmd = "Telescope",
    event = "VeryLazy",
    opts = {
        last_session_on_startup = false,
        projects = {
               -- define project roots
        },
    },
    init = function()
        -- enable saving the state of plugins in the session
        vim.opt.sessionoptions:append("globals") -- save global variables that start with an uppercase letter and contain at least one lowercase letter.
    end,
    dependencies = {
        { "nvim-lua/plenary.nvim" },
        { "nvim-telescope/telescope.nvim" },
        { "Shatur/neovim-session-manager" },
    },
}

for dropbar.nvim

-- local custom = require "custom"

return {
    "Bekaboo/dropbar.nvim",
    -- "Parsifa1/dropbar.nvim",
    dependencies = { 'nvim-telescope/telescope-fzf-native.nvim' },
    event = "VeryLazy",
    opts = {
        general = {
            enable = function(buf, win)
                return not vim.api.nvim_win_get_config(win).zindex
                    and vim.bo[buf].buftype == ""
                    and vim.api.nvim_buf_get_name(buf) ~= ""
                    and not vim.wo[win].diff
                    and vim.bo[buf].filetype ~= "toggleterm"
            end,
        },
    },
}

and the step is :

  1. add at least two project in the plugin
  2. ensure that there are at least one project with a markdown in it
  3. use :Telescope neovim-project discover, and switch some times
  4. then the error may occur

@willothy
Copy link
Collaborator

willothy commented Dec 3, 2023

Thanks @Parsifa1! What version of Neovim are you using?

@Parsifa1
Copy link
Author

Parsifa1 commented Dec 3, 2023

Thanks @Parsifa1! What version of Neovim are you using?

nightly

willothy added a commit to willothy/dropbar.nvim that referenced this pull request Dec 3, 2023
Avoids triggering lazy-loading of fields when deleting menus.

closes Bekaboo#114
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.

None yet

3 participants