Fix trailspace issues

[levouh]

Fixes two issues:

1. Highlight group can't be changed pending load order as it stands right now, use default when calling hi to handle these cases
2. Wrap the call to vim.fn.matchdelete with a pcall to avoid breaking things like Telescope if the match doesn't exist

Also changes some calls that use vim.fn to use vim.api and check valid state, etc.

---

Curious, is there a reason to defer_fn on the highlight :h autocmd call? If we do that, the call to get the "current window" might not be accurate. Wanted to check before trying to change it here.

[echasnovski]

Thank you for the very first contribution to this project! 🎉🎉🎉

Yeah, defer_fn() was a last minute change (which I might regret) as an attempt to solve the following problem. Certain common "information" buffers (like some help files, 'folke/trouble.nvim', 'folke/which-key.nvim') have trailing whitespace. All of them have something in common: they all are not modifiable. So highlight() gained check_modifiable argument which is true by default. Problem is that I couldn't find any reliable Vim events which fire after this option is set. Having defer_fn(*, 0) seems to ensure that vim.bo.modifiable contain the actual file from target buffer.

[echasnovski]

This feels like a valid change. Currently I am having trouble constructing an example where it really matters (and reading help didn't help). I can override MiniTrailspace and it works. Could you help me understand (ideally with some reproducible steps) when this can matter?

[levouh]

If I set MiniTrailspace before the plugin is loaded (lazy loading, for instance), your declaration here will overwrite whatever I set up. With this in place, it will only be defined if it is not already defined.

The key part from the :h hi-default is this line:

If highlighting has already been specified for the group the command will be ignored

which means users can define this before mini.trailspace is loaded, and the command used here won't overwrite it if they've done so.

[echasnovski]

Yep, that sounds reasonable. I am still having trouble with example. I used this 'init.lua' with only these two plugins installed and it works as expected (trailing whitespace is green):

vim.cmd([[hi MiniTrailspace guibg=#00ff00]])

require('packer').startup(function()
  use 'wbthomason/packer.nvim'
  use "echasnovski/mini.nvim"
end)

vim.cmd([[set termguicolors]])
require('mini.trailspace').setup()

[levouh]

Try linking to another highlight group rather than defining it outright, so:

vim.cmd([[hi link MiniTrailspace WarningMsg]])

...

Either way, I cannot imagine that it (this portion of the PR) will hurt to have.

[echasnovski]

This also feels like a valid change. Although if there is a use case when error is given, would you mind sharing it? I would really like to see it. Maybe it is a sign of a bigger problem in design.
I use 'teleslcope.nvim' myself and don't have any problem.

[levouh]

Agreed, it seems like it might be something a bit more "systemic", however I've never had any leftover matches after adding this (used my own fork for ~1 day now).

The error isn't super useful either, it just notes that "no match with ID ... found", seemingly as it doesn't apply to the current bufnr. This breaks anything really, but most notably Telescope (probably due to the FileType autocmd that is defined?) where an error is caught while it is forming the prompt buffer, so the entire thing ceases to work.

Not 100% sure that answers your question, but I can see if I can find a way to reproduce it with a "minimal" configuration. Mine is has a shit-load of plugins, personal libraries, and its all lazy-loaded with Packer so sometimes the issues I see don't tend to show up with "simpler" configurations. Let me know what you'd like to do.

[echasnovski]

Yeah, I've had that error when writing module and it really breaks everything. I don't have it anymore, even though I am using Telescope.
I'll wait for a day or two, maybe we'll figure something out. If not, I'll use this pcall.

[levouh]

Found the culprit on this one; it is a plugin that I have that is just calling clearmatches() (which clears all matches), when I remove that it doesn't happen anymore.

Either way at this point though, it doesn't seem like the end of the world to add a pcall wrapper to avoid breaking things (except perhaps in a scenario where we are missing an underlying bug, however I don't imagine that it'd be a feature-breaking bug to be trying to remove matches that don't exist). It's basically saying "try to delete this, and if it doesn't exist then I have nothing to delete in the first place", etc.

[levouh]

Thank you for the very first contribution to this project! tadatadatada

Yeah, defer_fn() was a last minute change (which I might regret) as an attempt to solve the following problem. Certain common "information" buffers (like some help files, 'folke/trouble.nvim', 'folke/which-key.nvim') have trailing whitespace. All of them have something in common: they all are not modifiable. So highlight() gained check_modifiable argument which is true by default. Problem is that I couldn't find any reliable Vim events which fire after this option is set. Having defer_fn(*, 0) seems to ensure that vim.bo.modifiable contain the actual file from target buffer.

I wonder if we could use vim.fn.expand("<abuf>") (which would return you the bufnr you actually care about) and then use vim.api.nvim_buf_get_option(..., "modifiable") to ensure that you aren't just querying for "the current buffer", but rather the proper target?

[echasnovski]

I wonder if we could use vim.fn.expand("<abuf>") (which would return you the bufnr you actually care about) and then use vim.api.nvim_buf_get_option(..., "modifiable") to ensure that you aren't just querying for "the current buffer", but rather the proper target?

Nope :( Having vim.fn.expand("<abuf>") in here (and removing defer_fn) doesn't seem to work.

What I believe might be happening is that modified is still false at the moment of autocommand execution. It will be changed later by plugins themselves.

[levouh]

Nope :( Having vim.fn.expand("<abuf>") in here (and removing defer_fn) doesn't seem to work.

What I believe might be happening is that modified is still false at the moment of autocommand execution. It will be changed later by plugins themselves.

Gotcha. So it seems then that one other solution would be to save off the winid while the autocmd is executing, but then perform the rest of the functionality we want later. Something like:

  vim.api.nvim_exec(
    [[augroup MiniTrailspace
        au!
        au WinEnter,BufWinEnter,InsertLeave * lua MiniTrailspace.defer_highlight()
        au WinLeave,BufWinLeave,InsertEnter * lua MiniTrailspace.unhighlight()

        au FileType TelescopePrompt let b:minitrailspace_disable=v:true
      augroup END]],
    false
  )

  -- Create highlighting
  vim.api.nvim_exec([[hi link MiniTrailspace Error]], false)
end

-- Module config
MiniTrailspace.config = {}

function MiniTrailspace.defer_highlight()
  local win_id = vim.api.nvim_get_current_win()

  vim.schedule(function()
    MiniTrailspace.highlight(nil, { win_id = win_id })
  end)
end

-- Functions to perform actions
--- Highlight trailing whitespace
---
---@param check_modifiable boolean: Whether to check |modifiable| (if it is off, don't highlight). Default: `true`.
---@param opts table: Additional options/state for highlighting, currently:
---                     - win_id: The window when the autocommand was invoked
function MiniTrailspace.highlight(check_modifiable, opts)
  check_modifiable = check_modifiable or true

  -- Highlight only in normal mode
  if H.is_disabled() or vim.fn.mode() ~= 'n' then
    MiniTrailspace.unhighlight()
    return
  end

  if check_modifiable and not vim.bo.modifiable then
    return
  end

  local win_id = opts.win_id or vim.api.nvim_get_current_win()
  if not vim.api.nvim_win_is_valid(win_id) then
    return
  end

...

would work then? Basically just call a function that saves off the "proper" window ID during the scope of the autocmd, and defers a function to be called later with that value saved off into a local scope.

By the way, do you have the "test" that you are using to find out whether or not these things do/don't work? When trying with things like ft=help buffers, I seem to get the right return value for modifiable.

[echasnovski]

Thank you very much for your patience. The reason I was so meticulous about both issues (highlighting and pcall) is that it may have been the indication about problems in other modules. And indeed they were! So you suggested a fix for other plugins too, which is double nice. I'll accept this PR as is and fix other modules myself.

About defer_fn and modifiable: I'll think about more clean solution to disable most "information" buffers. Currently using OptionSet event sounds promising.

Yeah, lack of tests is one very painful downside of this project. I hope to get myself together and write them, but not now, unfortunately.

Thank you again!

[levouh]

Absolutely, I appreciate the prodding from your end to find out why the problem was happening rather than just using pcall to start with.

Also note the problem with calling :h clearmatches was with one of my own plugins. Just making sure as you noted:

So you suggested a fix for other plugins too, which is double nice. I'll accept this PR as is and fix other modules myself

however there is nothing for you to fix in this regard, it is purely within my own config.

Thanks for the review, and the wonderful set of modules.

[echasnovski]

Just let you know that I removed defer_fn() by slightly reframing the problem it used to solve. Now instead of modifiable, tracking of buftype option is done via if-else and OptionSet autocommand. Activated with config.only_in_normal_buffers.