fix(symbol): use private data to check for menu

[willothy]

Avoids triggering lazy-loading of fields when deleting menus which fixes the issue with the markdown source, but also a potentially larger issue for any menu / symbol that makes use of lazy-loading via metatables. My hope is that this will prevent this type of issue from happening in the future even in user-created sources, but there may be some other places where this behavior could occur.

Tests are passing and this seems reasonable to me, but let me know if this will cause issues I'm not aware of.

closes #114

[Bekaboo]

@willothy Hi, thanks for the PR!

fixes the issue with the markdown source

Have you managed to find out the steps to reproduce the issue in #114?

Also, I want to make sure we all know why the _ hidden field is needed. The reason why we have this field is that we need two meta tables for a symbol -- one for lazy-loading fields and one to attach table dropbar_symbol_t to the newly created symbos. The _ field should be transparent and ideally, other methods and functions should be able access symbol fields without knowing its existence:

https://github.com/Bekaboo/dropbar.nvim/blob/f54d926d67d66e226b94e3b626e5f13224bc961d/lua/dropbar/bar.lua#L49

So I wonder why accessing menu field through _ will fix the issue. Could you explain it?

[willothy]

@willothy Hi, thanks for the PR!

fixes the issue with the markdown source

Have you managed to find out the steps to reproduce the issue in #114?

Yes, I was able to repro using a minimal init (on the commit before the buf validity check). I can send you the minimal init if you'd like.

Also, I want to make sure we all know why the _ hidden field is needed. The reason why we have this field is that we need two meta tables for a symbol -- one for lazy-loading fields and one to attach table dropbar_symbol_t to the newly created symbos. The _ field should be transparent and ideally, other methods and functions should be able access symbol fields without knowing its existence:

https://github.com/Bekaboo/dropbar.nvim/blob/f54d926d67d66e226b94e3b626e5f13224bc961d/lua/dropbar/bar.lua#L49

So I wonder why accessing menu field through _ will fix the issue. Could you explain it?

Yeah, sure. My understanding is that because of the nested metatables on the symbols from the markdown source, the __index metamethod needs to be invoked to find the base class methods (in this case, dropbar_symbol_t:del()). Because of this, when the components are deleted in dropbar_t:del(), any lazy-loading is triggered by the __index method, but then all that is returned from it is the base class method dropbar_symbol_t:del(). This is where the error happens.

Since the outer table is the one which contains the lazy-loading metamethod, accessing _ (the inner data / data metatable) skips the outer metatable's __index method, and is able to access the symbol's properties directly.

There may be a better way to do this, I'm happy to rethink the implementation, but imo it would definitely be good for performance and timing issues like #114 if that extra indirection could be avoided when it's not necessary.

[Bekaboo]

Yes, I was able to repro using a minimal init (on the commit before the buf validity check). I can send you the minimal init if you'd like

Could you share the reproduce steps here? Thanks!

[willothy]

Here are the plugin specs that the OP shared:

local plugins = {
  {
    "coffebar/neovim-project",
    opts = {
      last_session_on_startup = false,
      projects = {
        "~/projects/*",
        "~/.config/*",
      },
    },
    init = function()
      vim.opt.sessionoptions:append("globals")
    end,
    dependencies = {
      { "nvim-lua/plenary.nvim" },
      { "nvim-telescope/telescope.nvim", tag = "0.1.4" },
      { "Shatur/neovim-session-manager" },
    },
    event = "VeryLazy",
  },
  {
    "Bekaboo/dropbar.nvim",
    commit = "3b7978b",
    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,
      },
    },
  },
}

1. Created minimal install with only dropbar and neovim-project (can send full script if needed)
2. Open nvim, and use :Telescope neovim-project discover to select a project
3. Open the project, and open a Markdown file in that project
4. Navigate to a different project using neovim-project again (don't close the markdown file you just opened so that it is saved with the session)
5. Navigate back to the first project, where the Markdown file was open
6. The error should appear immediately when you load the session

[Bekaboo]

For this PR I still need some time to figure out what is excatly happening and if there is a better way to fix it.

[willothy]

For this PR I still need some time to figure out what is excatly happening and if there is a better way to fix it.

Totally, there's got to be a better way. I'll keep looking into it as well.

[Bekaboo]

Here are the plugin specs that the OP shared:

local plugins = {
  {
    "coffebar/neovim-project",
    opts = {
      last_session_on_startup = false,
      projects = {
        "~/projects/*",
        "~/.config/*",
      },
    },
    init = function()
      vim.opt.sessionoptions:append("globals")
    end,
    dependencies = {
      { "nvim-lua/plenary.nvim" },
      { "nvim-telescope/telescope.nvim", tag = "0.1.4" },
      { "Shatur/neovim-session-manager" },
    },
    event = "VeryLazy",
  },
  {
    "Bekaboo/dropbar.nvim",
    commit = "3b7978b",
    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,
      },
    },
  },
}

1. Created minimal install with only dropbar and neovim-project (can send full script if needed)

2. Open nvim, and use `:Telescope neovim-project discover` to select a project

3. Open the project, and open a Markdown file in that project

4. Navigate to a different project using neovim-project again (don't close the markdown file you just opened so that it is saved with the session)

5. Navigate back to the first project, where the Markdown file was open

6. The error should appear immediately when you load the session

Emm I cannot seem to reproduce the error using lazy and spec provided. A full minimal init.lua without using a package manager might help.

[willothy]

Yeah, I'm not sure if I can anymore either so maybe worth asking the OP again for that. Feel free to close this, I agree that it's not a great solution since we still don't know what the problem actually is.