Skip to content

Fix trailspace issues #1

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 15, 2021
Merged

Fix trailspace issues #1

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions lua/mini/trailspace.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ function MiniTrailspace.setup(config)
)

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

@echasnovski echasnovski Oct 14, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@levouh levouh Oct 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@echasnovski echasnovski Oct 14, 2021

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@levouh levouh Oct 15, 2021

Choose a reason for hiding this comment

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

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.

end

-- Module config
Expand All @@ -86,21 +86,27 @@ function MiniTrailspace.highlight(check_modifiable)
return
end

local win_id = vim.fn.win_getid()
local win_match = H.window_matches[win_id]
local win_id = vim.api.nvim_get_current_win()
if not vim.api.nvim_win_is_valid(win_id) then
return
end

-- Don't add match id on top of existing one
if win_match == nil then
if H.window_matches[win_id] == nil then
H.window_matches[win_id] = vim.fn.matchadd('MiniTrailspace', [[\s\+$]])
end
end

--- Unhighlight trailing whitespace
function MiniTrailspace.unhighlight()
local win_id = vim.fn.win_getid()
local win_id = vim.api.nvim_get_current_win()
if not vim.api.nvim_win_is_valid(win_id) then
return
end

local win_match = H.window_matches[win_id]
if win_match ~= nil then
vim.fn.matchdelete(win_match)
pcall(vim.fn.matchdelete, win_match)
Copy link
Member

@echasnovski echasnovski Oct 14, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@levouh levouh Oct 14, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member

@echasnovski echasnovski Oct 14, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@levouh levouh Oct 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

H.window_matches[win_id] = nil
end
end
Expand Down