Recall() can succeed where colorscheme may fail

[wroyca]

As per title, Recall() can succeed where colorscheme may fail (e.g., when we invoke it to retrieve a theme that was previously used but has since been deleted).

Example:

1. Add "sainnhe/gruvbox-material" to your Neovim package manager.
2. Set colorscheme to "gruvbox-material"
3. Last-color will cache "gruvbox-material."
4. Remove "sainnhe/gruvbox-material" from your package manager.
5. Restart Neovim.

last-color will successfully fetch gruvbox-material from cache, but vim.cmd[[colorscheme %s]] will fail since gruvbox-material no longer exists.

---

A somewhat hack'ish workaround is to abuse pcall to catch instances where colorscheme fails and then fallback appropriately:

local s, _ = pcall(vim.cmd, (([[colorscheme %s]]):format(require([[last-color]]).recall()))) 

-- If neither 'gruvbox-material' nor 'habamax' are found, pcall 
-- will ignore any errors and default to Neovim's built-in scheme.

if not s then s, _ = pcall(vim.cmd, [[colorscheme gruvbox-material]])
if not s then        pcall(vim.cmd, [[colorscheme habamax]])

end
end

[raddari]

Good catch, I never considered this! There already exists a way to check if a colorscheme is valid when saving, so it should be simple enough to run this check when loading too. I'll look into it

[raddari]

I was able to reproduce this issue, and have a working fix. Can you confirm that #6 works for you too?

[wroyca]

I was able to reproduce this issue, and have a working fix. Can you confirm that #6 works for you too?

Thank for the quick response! Unfortunately, it seems now that recall() will return nil every few restarts - this seems to be nondeterministic:

Screencast from 2023-10-01 23-00-42.webm

[raddari]

Interesting, I'll see what's up

[raddari]

I can't seem to reproduce on my end, do you mind sharing the relevant parts of your config?

[wroyca]

I can't seem to reproduce on my end, do you mind sharing the relevant parts of your config?

Odd - I'll make a MRE later tonight (or tomorrow at best)

Perhaps there's something else going on here that just so happen to affect things

[wroyca]

Had a chance to free myself this morning, so here's an MRE:

-- vim:fileencoding=utf-8:foldmethod=marker

vim.g.mapleader = [[ ]]
vim.o.termguicolors = true

local lazypath = vim.fn.stdpath("data") .. "/lazy/lazy.nvim"
if not vim.loop.fs_stat(lazypath) then
  vim.fn.system({
    "git",
    "clone",
    "--filter=blob:none",
    "https://github.com/folke/lazy.nvim.git",
    "--branch=stable", -- latest stable release
    lazypath,
  })
end
vim.opt.rtp:prepend(lazypath)

require([[lazy]]).setup({
  spec = {
    {
      [[sainnhe/gruvbox-material]]
    },
    {
      [[raddari/last-color.nvim]],
      branch = [[fix/recall-removed-color]],
      config = function()
        -- We may also add: or 'habamax', but this is not important for this sample.
        local theme = require('last-color').recall()
        vim.cmd(('colorscheme %s'):format(theme))
      end
    }
  }
})

Turn out that it is a load order dependency issue. That is, last-color attempts to retrieve the colorscheme before "lazy.nvim" has a chance to register "sainnhe/gruvbox-material," even if the colorscheme is already installed. This can be mitigated by assigning a higher priority to colorscheme, but I believe that it should be handled internally?

[raddari]

Looks like you're right about plugin load order, also explains the non-deterministic aspect. I'm hesitant to handle this issue on the last-color side for a few reasons:

1. Lazy.nvim recommends raising the priority of colorschemes in their LazySpec as you mentioned, so they should theoretically be loaded first anyway.
2. last-color never touches any colorscheme commands, so users retain complete control over when to recall and set their colorscheme.
3. Determining if a recalled colorscheme can't be found because it was removed vs hasn't been loaded yet is a daunting task. I'd rather fix the former in #6 and let the user sort out the latter themselves with their config.

I'd recommend leaving the calls to last-color.recall and vim.cmd.colorscheme at the end of your init.lua, and not calling them inside of last-colors LazySpec config to ensure all colorscheme plugins have loaded first -- example of how I use it looks like this in my init.lua with no config function in the LazySpec. This should mean you won't have to adjust colorscheme priorities, but still precludes lazy loading.

For now I'll merge #6 which should at least solve the original issue you were facing. I think the non-deterministic issue is best solved in user config rather than at the plugin level for the reasons above, but I am happy to continue the discussion if you disagree :)

[wroyca]

I think the non-deterministic issue is best solved in user config rather than at the plugin level for the reasons above, but I am happy to continue the discussion if you disagree :)

I'm comfortable with either, but I'm concerned that this PR might result in user configuration breaking change. Before, order didn't matter, and last-color would consistently succeed (except for this specific issue) - So, my question is, how many users are currently following the same approach as I did, not prioritizing colorschemes and how should we proceed to let them know about this change? Finally, what would occur if new users, who are not familiar with how things work, attempt to add new themes? I suspect many issues may arise due to overlooking the priority aspect?

In any case, thank for this fix! :partying_face:

[mrs4ndman]

In my case, I was lazy loading almost 25 colorschemes, and this change did make me troubleshoot the setup until I found the update and now I need to load them on startup.

It is not ideal and I would like to leave them lazy-loading, but I have been using this plugin for over 4 months and really like it. A heads-up / breaking-change notice on the README would be nice.

Keep up the good work @raddari :+1:

[raddari]

@wroyca @mrs4ndman based on your feedback I've decided to revert the change for now. I apologise for the breaking change, I didn't test how its usage changed enough.

Overall I did some thinking and I'm not sure I'm entirely happy with the way I implemented the fix anyway. Returning nil if the colorscheme isn't found is not ideal as the user cannot lazy load colorschemes if they recall before they're loaded. I'll think about a potential fix for loading a disabled colorscheme, but for now I think not being able to lazy load is a bigger problem compared to an outdated cache.

Cheers for the feedback :)

[wroyca]

@raddari For what it's worth, my initial workaround works perfectly well in my case, and the overhead is minimal. It should be fine to just mention it in the README and close this issue :)