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

feat: repeat indent guides on wrapped lines with 'breakindent' #810

Merged
merged 1 commit into from
Dec 26, 2023

Conversation

luukvbaal
Copy link
Contributor

@luukvbaal luukvbaal commented Dec 25, 2023

Opt-out with ibl.config.indent.repeat_linebreak = false.

Close #577

@luukvbaal
Copy link
Contributor Author

luukvbaal commented Dec 25, 2023

Depends on neovim/neovim#26625. Currently testing if it is included with pcall(nvim_buf_set_extmark, ...), may be replaced with a version check at some point I suppose.

One caveat here is that since extmarks are local to a buffer, and 'breakindent' is a window option, the indent lines may overlap buffer text when there are multiple windows with different 'breakindent' values displaying the same buffer... Doubt that is a real issue though.

@luukvbaal luukvbaal force-pushed the repeat_linebreak branch 2 times, most recently from 897c4ff to 5b3b2df Compare December 25, 2023 23:26
@luukvbaal luukvbaal marked this pull request as ready for review December 25, 2023 23:26
@lukas-reineke
Copy link
Owner

lukas-reineke commented Dec 26, 2023

Thanks, this is nice.

may be replaced with a version check at some point I suppose.

Yeah, version check would be cleaner. You can just add the version check now with 0.10. I only support latest nightly and stable.

One caveat here is that since extmarks are local to a buffer, and 'breakindent' is a window option, the indent lines may overlap buffer text when there are multiple windows with different 'breakindent' values displaying the same buffer... Doubt that is a real issue though.

This is fine, there are other option values with the same restriction already. I have this helper function to get the right window ID to use with nvim_get_option_value. Please use that as well.

---@param bufnr number
---@return number?
M.get_win = function(bufnr)
local win_list = vim.fn.win_findbuf(bufnr)
local current_tab = vim.api.nvim_get_current_tabpage()
for _, win in ipairs(win_list or {}) do
if current_tab == vim.api.nvim_win_get_tabpage(win) then
return win
end
end
return win_list and win_list[1]
end

And please add the option to the docs as well.

@luukvbaal luukvbaal force-pushed the repeat_linebreak branch 2 times, most recently from c1da3ab to d346797 Compare December 26, 2023 10:27
@luukvbaal
Copy link
Contributor Author

luukvbaal commented Dec 26, 2023

Hmm there is another caveat. Depending on the value of 'breakindentopt', we may still end up placing the indent guide virtual text over the actual buffer text. Not sure how to handle that TBH, here or in the nvim API.

Maybe we have to parse 'breakindentopt' as well, and only repeat if we can be sure the wrapped line is indented to the same column.

Copy link
Owner

@lukas-reineke lukas-reineke left a comment

Choose a reason for hiding this comment

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

Just one small comment, rest looks good now

@lukas-reineke
Copy link
Owner

I will think about how to handle breakindentopt correctly. It's pretty niche, I'm okay with just not supporting this for now.

@luukvbaal
Copy link
Contributor Author

I will think about how to handle breakindentopt correctly. It's pretty niche, I'm okay with just not supporting this for now.

That's my feeling as well. I'll address the comments/CI failure later.

Opt-out with ibl.config.indent.repeat_linebreak = false.
Copy link
Owner

@lukas-reineke lukas-reineke left a comment

Choose a reason for hiding this comment

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

thank you 🚀

@lukas-reineke lukas-reineke merged commit 9433822 into lukas-reineke:master Dec 26, 2023
@mehalter
Copy link
Contributor

Should this be on by default? It seems like having the default options require the latest Neovim nightly is a bad idea even if the user can opt out of the new feature. It seems like it should be an opt in

@luukvbaal
Copy link
Contributor Author

Whether an option requires a certain Neovim version seems irrelevant to the option's default value. I think most people will want this option enabled, that's why I suggested opt out.

I do agree that just checking for has("neovim-0.10") is more disruptive than it has to be for for people on a nightly version, but not the latest nightly. But they are using a nightly version, they can just update which I suppose was @lukas-reineke's view :)

@mehalter
Copy link
Contributor

Yeah I agree. I'll be honest, I just saw in the docs that the default is true and I didn't notice the check for 0.10. this is a completely sane default and relying on people using nightly to just update makes sense for me! People using nightly accept the chance for bugs and potentially infinite amounts of instability 😂

@lukas-reineke
Copy link
Owner

The option will simply be ignored if you are on stable.
And if you are on nightly, it's your responsibility to be up to date.

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.

Indent guides on wrapped lines
3 participants