Closed nkgm closed 3 months ago
Hi there! Thank you for opening this feature request! I'm definitely gonna implement this.
There're some notes though:
modes = {
n = {
winhl = {
CursorLine = function()
return { bg = cNormalActive_t }
end
}
}
}
In terms of benefits you'll have dynamic values only for highlight groups you wanna apply them for and cached ones for others. But it will add some overhead for executing for sure, although it's very small.
Doing them on the group level could be overkill. What if there's 2 or more groups? Then you'd have to do the same thing times n and the overhead could come out higher, not to mention user convenience. From a quick test, the presets are run when switching windows or modes, always in response to user action. Not sure how representative this is overall, but performance impact should be negligible. Ultimately it is your decision, but my vote goes in favor of simplicity to winhl
, and you could break it down to group level at a later stage if the need arises.
Hey @nkgm! Just pushed a feature to the main branch. Feel free to test it :)
winhl
and hl
can be function now, they should always return a highlight table, but I still think of making it not required.
That was quick, I'll be testing it shortly! If you get that wiki started I'll happily contribute where I can. Thanks again!
I can use functions on the modes but not on statics (in order to set the inactive colors). I'll have to look into static
a bit more to form an opinion. Is there a particular reason of structuring like { winhl = { active = { group_name = ... } } }
instead of { active = { winhl = { group_name = ... } } }
?
config = function(_, opts)
local reactive = require("reactive")
reactive.add_preset({
name = "nord_main",
-- static = {
-- winhl = function()
-- local ret
-- if vim.bo.buftype == "terminal" then
-- ret = vim.w.nkgm_wasInsert and cInsertInactive_t or cNormalInactive_t
-- else
-- ret = vim.w.nkgm_wasInsert and cInsertInactive or cNormalInactive
-- end
-- return {
-- inactive = {
-- CursorLine = { bg = ret },
-- },
-- }
-- end,
-- },
modes = {
n = {
winhl = function()
return {
CursorLine = { bg = vim.bo.buftype == "terminal" and cNormalActive_t or cNormalActive },
}
end,
},
i = {
winhl = function()
return {
CursorLine = { bg = vim.bo.buftype == "terminal" and cInsertActive_t or cInsertActive },
}
end,
},
},
})
reactive.setup(opts)
end,
Yes, there's a reason - it's because window highlights can be applied to active and inactive windows whereas ordinary highlights (hl
) are only applied globally. So the structure looks like this:
static = {
winhl = {
active = {},
inactive = function()
return {}
end
},
hl = {
Cursor = {}
}
}
So, if you wanna use functions, you have to use them inside active/inactive fields
Thanks, that works as expected! Next comes an issue I was afraid I wouldn't be able to avoid in inactive
. The vim.bo.buftype == "terminal"
condition is not always evaluated in the ~correct~ intended window context. As a result, inactive normal windows end up getting terminal CursorLine colors. I have an idea that might also assist with caching function results, but at this point I feel I've taken enough of your time and should probably look at a PR, time allowing.
local nord_main = {
name = "nord_main",
static = {
winhl = {
active = {},
inactive = function()
local ret
if vim.bo.buftype == "terminal" then
ret = vim.w.nkgm_wasInsert and cInsertInactive_t or cNormalInactive_t
else
ret = vim.w.nkgm_wasInsert and cInsertInactive or cNormalInactive
end
return {
CursorLine = { bg = ret },
}
end,
},
},
modes = {
n = {
winhl = function()
return {
CursorLine = { bg = vim.bo.buftype == "terminal" and cNormalActive_t or cNormalActive },
}
end,
},
i = {
winhl = function()
return {
CursorLine = { bg = vim.bo.buftype == "terminal" and cInsertActive_t or cInsertActive },
}
end,
},
},
}
Looked at it and it seems to be a problem that raised up because of the latest features, it may also be a misconception of what this plugin actually does.
reactive
uses highlights per window, not per buffer. So, we have global highlights - ones that are applied for every window. Then we have window-scoped highlights divided into 2 sections.
It means that if you apply highlights for inactive windows in one preset, they will be applied for all other inactive windows as well. It doesn't matter which condition you were checking in those dynamic functions.
Now why you have that bug:
WinLeave
event) with the "terminal" buftype, you apply its highlights for all inactive windows, because a highlight group is created for it and it comes from the same preset name, meaning that highlight group name will also be the same.But to fix it I'll have to trigger inactive highlights each time a cursor is entering a window (current && active). Not that I can't do it, but it seems so redundant for me. And it's just for allowing dynamic colours for active/inactive windows. Maybe it's better to remove that functionality at all and only leave it for modes
highlights or remove it from there as well...
Although I understand that it's easier to put everything in one function and be done, but as we can see it creates a lot of confusion of what can/can't be achieved. And sometimes it's better to have a couple more lines of configuration, rather than complexity in the plugin or confusion.
I think that dynamic winhl
/hl
options can be kept for the modes
option, because they're always executed in the context of a current window and should only be used for that purpose. For now I can't imagine the situation where those functions can create any confusion or mess, can you?
That would be the best compromise for now. If you're open to discussion I still got a couple of points to argue, somewhat linked to the first issue (apologies for not replying to that yet, I still need to run some tests so I'm sure I'm not talking nonsense).
Yep, I'm pretty much always open to discuss improvements :)
You can either open a topic in the discussion section or DM me directly
@nkgm I was going through the flow and realized that the bug you encountered can actually be reproduced without using a dynamic function.
For some reason I associated highlights for non-current windows with WinLeave
event, when I should've done exactly the opposite. When WinEnter
happens, I should know which color for non-current windows is the most valid and relevant and apply it for all non-current windows.
I did it on my computer and it seems to work, it even allowed me to remove WinLeave
autocmd completely as it's not needed anymore. Although I had to refactor some of the code and will still have to do it for other parts of the plugin.
Really appreciate your contributions, they allowed me to go again through the flow and find the issues I didn't notice before. Thank you!
I'll push all of the changes as soon as they're ready.
Hi @nkgm, sorry for the long wait! I just pushed fixes to the main branch. Could you please test it and clarify if that works for you?
Thanks @rasulomaroff, just got a chance to try it.
I'm still having issues, and it would appear latest changes also broke my working "no functions" style config. Reverting back to d006300
confirms this.
I appreciate your hard work and more than happy to test further and share my config, but I would also understand should you choose to close for now.
Could you please describe what issues you're having? Maybe there's just misunderstanding how it should work :) Because I just tested everything on my machine and it seems solid
I was afraid I might have. Attaching full config and screen recordings:
Classic-style config on d006300
- working as intended:
https://github.com/rasulomaroff/reactive.nvim/assets/5631881/fa258678-d988-4222-91e0-c2ba6f9fcc97
Classic-style config on a81b123
- inactive CursorLine is always cInsertInactive_t
regardless of normal or terminal:
https://github.com/rasulomaroff/reactive.nvim/assets/5631881/712863f8-f7df-444e-bbca-16465c85630c
Function-style config on a81b123
- inactive CursorLine is always the inactive color of the active window:
https://github.com/rasulomaroff/reactive.nvim/assets/5631881/ed275728-aebf-46f7-b9cd-f25017d3a4bf
Please note I've disabled the windowpicker plugin and nkgm_wasInsert
is always nil. I have used native vim window switching in the recordings.
local cNormalActive = "#3a4355"
local cNormalInactive = "#202020"
local cInsertActive = "#344c34"
local cInsertInactive = "#012828"
local cNormalActive_t = "#3f422e"
local cNormalInactive_t = "#282a1d"
local cInsertActive_t = "#3a4355"
local cInsertInactive_t = "#888888"
local nord_preset_normal = {
name = "nord_normal",
static = {
winhl = {
inactive = {
CursorLine = { bg = cNormalInactive },
},
active = {
CursorLine = { bg = cNormalActive },
},
},
},
modes = {
n = {
winhl = {
CursorLine = { bg = cNormalActive },
},
},
i = {
winhl = {
CursorLine = { bg = cInsertActive },
},
},
},
}
local nord_preset_insert = {
name = "nord_insert",
priority = 1,
skip = function()
return not vim.w.nkgm_wasInsert
end,
static = {
winhl = {
inactive = {
CursorLine = { bg = cInsertInactive },
},
active = {
CursorLine = { bg = cNormalActive },
},
},
},
modes = {
n = {
winhl = {
CursorLine = { bg = cNormalActive },
},
},
},
}
local term_preset_normal = {
name = "term_normal",
priority = 100,
skip = function()
return vim.api.nvim_buf_get_option(0, "buftype") ~= "terminal"
end,
static = {
winhl = {
inactive = {
CursorLine = { bg = cNormalInactive_t },
},
active = {
CursorLine = { bg = cNormalActive_t },
},
},
},
modes = {
n = {
winhl = {
CursorLine = { bg = cNormalActive_t },
},
},
},
}
local term_preset_insert = {
name = "term_insert",
priority = 101,
skip = function()
return (vim.api.nvim_buf_get_option(0, "buftype") ~= "terminal" or not vim.w.nkgm_wasInsert)
end,
static = {
winhl = {
inactive = {
CursorLine = { bg = cInsertInactive_t },
},
active = {
CursorLine = { bg = cNormalActive_t },
},
},
},
modes = {
n = {
winhl = {
CursorLine = { bg = cNormalActive_t },
},
},
},
}
local nord_main = {
name = "nord_main",
static = {
winhl = {
active = {},
inactive = function()
local ret
if vim.bo.buftype == "terminal" then
ret = vim.w.nkgm_wasInsert and cInsertInactive_t or cNormalInactive_t
else
ret = vim.w.nkgm_wasInsert and cInsertInactive or cNormalInactive
end
return {
CursorLine = { bg = ret },
}
end,
},
},
modes = {
n = {
winhl = function()
return {
CursorLine = { bg = vim.bo.buftype == "terminal" and cNormalActive_t or cNormalActive },
}
end,
},
i = {
winhl = function()
return {
CursorLine = { bg = vim.bo.buftype == "terminal" and cInsertActive_t or cInsertActive },
}
end,
},
},
}
return {
"rasulomaroff/reactive.nvim",
commit = "d006300",
-- commit = "a81b123",
opts = {
builtin = {
cursorline = true,
cursor = true,
modemsg = true,
},
},
config = function(_, opts)
local reactive = require("reactive")
-- function-style
-- reactive.add_preset(nord_main)
-- classic-style: works on d006300
reactive.add_preset(nord_preset_normal)
reactive.add_preset(nord_preset_insert)
reactive.add_preset(term_preset_normal)
reactive.add_preset(term_preset_insert)
reactive.setup(opts)
end,
}
Classic-style config on
a81b123
- inactive CursorLine is alwayscInsertInactive_t
regardless of normal or terminal: rasulomaroff/reactive.nvim/assets/5631881/712863f8-f7df-444e-bbca-16465c85630cFunction-style config on
a81b123
- inactive CursorLine is always the inactive color of the active window: rasulomaroff/reactive.nvim/assets/5631881/ed275728-aebf-46f7-b9cd-f25017d3a4bf
As I said, there was just a misunderstanding how it is supposed to work and this led to a confusion. The previous behaviour was actually a bug. So, how it works:
modes
and active
section. They're always applied for a current window. Then the inactive
section is applied for all non-current windows.Another thing to note here is that all callbacks from dynamic highlights are executed in the context of the current window. It makes sense because presets have priorities and if one preset wants to highlight all inactive windows with its highlights, then it should be like that. That means that if you're going from a terminal buffer to a normal one, your buftype
option won't be terminal. And it means that inactive
section will return values for non-terminal buffers.
In simple terms, presets are formed on a global basis, not a per-window one. I do think about providing configuration options to make it possible to apply them on a per-window basis, but I have another features I'd like to introduce first, like registers and macros support.
After all these features are finished, I would look into the one I described above.
Thanks for clarifying. How about passing the winid
argument to the function callbacks and leave it up to the user to decide what they do with it? That way you don't have to make guarantees about window context functions run inside.
I see several problems implementing it in the current state.
The main problem is that the final snapshot is only formed once, regardless of how many windows you have opened. This is due to performance considerations. To pass a parameter (winid) into a function, I will have to run all of those callbacks in the context of each window, which will create unnecessary overhead (Also consider that I will have to do it only when a callback is used and not when it's just a table). Additionally, there will be inconsistency between dynamic callbacks, because those in the modes
field won't have a winid parameter just because it doesn't make sense, as mode changes are only happening within the current window.
I believe this will add unwanted complexity and there's a way to resolve it much more elegantly.
As I said, I do plan to add support for highlights on a per-window basis in the future, but I need to think it through first :)
No problem, closing for now :)
The following setup works as intended for me, but goes to show how much easier things would be if I could use a function for
winhl
.The main gist is, I want my windows to remember
i
orn
mode at the time of the switch. I'm using my fork of s1n7ax/nvim-window-picker to this end. At switch time,vim.w.nkgm_wasInsert
is set for the old window, and upon switching back, it is checked andvim.cmd("startinsert")
is executed if required. This is myreactive.nvim
configuration for assigning a different color CursorLine to each unique state.