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

[RFC] allow window specific ui highlighting (currently only window background) #6597

Merged
merged 4 commits into from May 8, 2017

Conversation

Projects
None yet
8 participants
@bfredl
Member

bfredl commented Apr 26, 2017

While doing stupid experiments with floating windows, I realized they need to be in a different background color. But this might be useful in general. (It was discussed, issue #5302).

set winbg=HighlightGroup

Though it is not currently restricted to background color, any attribute can be set. So maybe the option should have a different name.

@marvim marvim added the WIP label Apr 26, 2017

@fmoralesc

This comment has been minimized.

Contributor

fmoralesc commented Apr 28, 2017

Nice! However, couldn't this be implemented as some sort of flag to the :hi command, something like hi local link Normal HighlightGroup? Then, it would be fully general, no?

@bfredl

This comment has been minimized.

Member

bfredl commented Apr 28, 2017

Yes, but that would be a lot more complicated. For ui highlighting there is the HLF_ caching that assumes ui highlights are global, and for syntax, well there is :ownsyntax as a workaround. It might be a worthwhile project, but I don't think it should block this rather simple PR.

The only addition I would consider is support two values for active/inactive window as I guess that is a common usecase. Or maybe people should just use WinEnter/WinLeave.

@fmoralesc

This comment has been minimized.

Contributor

fmoralesc commented Apr 28, 2017

Oh, for sure. It was just a thought (main concern is that you are introducing a setting which might become obsolete by b some mechanism like above). Maybe the setting could be extensible so it is similar to the way guicursor works (taking a list of items and their attributes).

@bfredl

This comment has been minimized.

Member

bfredl commented Apr 28, 2017

Maybe the setting could be extensible so it is similar to the way guicursor works (taking a list of items and their attributes).

That's a good suggestion, then we can make it clear a few specific groups are overridable, unlike hi local link which would seem to support everything.

@bfredl

This comment has been minimized.

Member

bfredl commented Apr 28, 2017

I'm unable to get buffer-local behavior for window option as described in local-options

It's possible to set a local window option specifically for a type of buffer.
When you edit another buffer in the same window, you don't want to keep
using these local window options.  Therefore Vim keeps a global value of the
local window options, which is used when editing another buffer.  Each window
has its own copy of these values.  Thus these are local to the window, but
global to all buffers in the window.  With this you can do: >
	:e one
	:set list
	:e two
Now the 'list' option will also be set in "two", since with the ":set list"
command you have also set the global value. >
	:set nolist
	:e one
	:setlocal list
	:e two
Now the 'list' option is not set, because ":set nolist" resets the global
value, ":setlocal list" only changes the local value and ":e two" gets the
global value.  Note that if you do this next: >
	:e one
You will get back the 'list' value as it was the last time you edited "one".
The options local to a window are remembered for each buffer.  This also
happens when the buffer is not loaded, but they are lost when the buffer is
wiped out |:bwipe|.

Is there a checklist somewhere for all the places a new window option, there are quite a few....

@fmoralesc

This comment has been minimized.

Contributor

fmoralesc commented Apr 28, 2017

Indeed. For example, what would hi local link Statusline Group do? So I agree, that is not a good idea.

@justinmk

This comment has been minimized.

Member

justinmk commented Apr 28, 2017

I'm unable to get buffer-local behavior for window option as described in local-options

Why not make it buffer-local? See 7bc37ff for how 'scrollback' was converted from "global" to buffer-local.

Is there a checklist somewhere for all the places a new window option, there are quite a few...

Do you mean in the C code? Top of option.c is mostly sufficient.

If it's any help, see #6337 (comment)
(tl;dr global-local is different than buffer-local; buffer-local actually has "global default" capability).

@bfredl

This comment has been minimized.

Member

bfredl commented Apr 29, 2017

Thanks for the pointers. The top of option.c is not complete, doesn't mention check_winopt and clear_winopt. An option with sideeffects also in get_winopts and win_init. But briopt is for some reason in win_copy_options; this only gets called by win_init but not by buffer_enter so it does not work. I will update the head of option.c and maybe factor out didset_window_options.

This is now set winhl=normal:TheGroup,morestuff:AnotherGroup, but for the moment only normal, maybe should add inactive for background of inactive window.

@bfredl bfredl changed the title from [WIP] allow different backgrounds in windows to [WIP] allow window specific ui highlighting (currently only window background) Apr 29, 2017

@bfredl

This comment has been minimized.

Member

bfredl commented Apr 29, 2017

Added inactive variant, so now fixes #5302 .

@bfredl

This comment has been minimized.

Member

bfredl commented Apr 29, 2017

marking RFC.

@bfredl bfredl changed the title from [WIP] allow window specific ui highlighting (currently only window background) to [RFC] allow window specific ui highlighting (currently only window background) Apr 29, 2017

@marvim marvim added RFC and removed WIP labels Apr 29, 2017

@justinmk

This comment has been minimized.

Member

justinmk commented Apr 29, 2017

would it be much trouble to make matchadd()/matchaddpos() support changing the bgcolor?
Should we just accept that "Normal bg is special"?
#5302 (comment)

@bfredl

This comment has been minimized.

Member

bfredl commented Apr 29, 2017

No. Background is not special, it is just the intended usecase. You can override fg color and attributes. It would seem like a big stretch to let matchadd() add highlights where there is no text, both after end of line, and EOB. matchadd() also can't be buffer-specific.

@justinmk

This comment has been minimized.

Member

justinmk commented Apr 29, 2017

Right, matchadd() is the wrong mechanism because one cannot "match" non-text. I guess what I'm looking for is a way to decorate cells by XY coordinates. Anyways, that can wait until later.

@bfredl

This comment has been minimized.

Member

bfredl commented Apr 29, 2017

and bufhl can't be window-global, though it could make sense to allow matchadd or bufhl to highlight the gap between end of a logical line and the physical line.

But the main point is that this will be needed for (hopefully future) builtin functionality (floating windows), and such should have a builtin mechanism.

@justinmk

This comment has been minimized.

Member

justinmk commented Apr 29, 2017

segfault in win_enter_ext:

https://gist.github.com/2ec9f8d3a442560ca38515dd0927b1aa

Steps:

nvim -u NONE
:tabnew
:tabclose

bfredl added a commit to bfredl/neovim that referenced this pull request Apr 29, 2017

@bfredl

This comment has been minimized.

Member

bfredl commented Apr 29, 2017

Thanks, please try latest commit.

to set window specific background color.
inactive: Highlight applied to the window when inactive. If
not set uses same value as `normal`.

This comment has been minimized.

@blueyed

blueyed Apr 30, 2017

Contributor

Maybe this could have an example, e.g. :set winhl=inactive:ColorColumn?!

@blueyed

This comment has been minimized.

Contributor

blueyed commented Apr 30, 2017

Very nice (it could finally make https://github.com/blueyed/vim-diminactive obsolete).

I've wondered if it could be applied to all existing windows already when setting it?! - or should it be considered as a normal window setting in that sense?
Then it is more difficult to change it in the sense of a scheme (when you have to do :windo in all tabs to set/change it).

@bfredl

This comment has been minimized.

Member

bfredl commented May 1, 2017

It is a window option, so "global" means the window (preserved across splits etc) and "local" means the buffer in the window. But what could make sense is a global default for inactive like WindowInactive, so it can be changed for all "normal" windows with hi WindowInactive guibg=... (it should default to hi link Normal). But set winbg=normal:MySpecialWindow should take precedence.

@bfredl

This comment has been minimized.

Member

bfredl commented May 1, 2017

Now one can hi InactiveWin guibg=...globally.

Note: I am breaking the convention "HLF constants must have super obscure names".

/* time limit is set at the toplevel, for all windows */
// time limit is set at the toplevel, for all windows
// determine window speciic background set in 'winhighlight'

This comment has been minimized.

@romgrk

romgrk May 1, 2017

Contributor

*specific

This comment has been minimized.

@bfredl

bfredl May 2, 2017

Member

👍

@justinmk justinmk added this to the 0.2.1 milestone May 1, 2017

@justinmk justinmk referenced this pull request May 2, 2017

Merged

news: version 0.2 #151

@blueyed

This comment has been minimized.

Contributor

blueyed commented May 5, 2017

I am using the following now to replace my plugin:

if exists('+winhighlight')
  function! s:configure_winhighlights(...)
    let winnr = a:0 ? a:1 : winnr()
    let force = a:0 > 1 ? a:2 : 0
    if !force
      if a:0
        let ft = getbufvar(winbufnr(winnr), '&filetype')
        let bt = getbufvar(winbufnr(winnr), '&buftype')
      else
        let ft = &filetype
        let bt = &buftype
      endif
      " Check white/blacklist.
      if index(['dirvish'], ft) == -1
            \ && (index(['nofile', 'nowrite', 'acwrite', 'quickfix', 'help'], bt) != -1
            \     || index(['startify'], ft) == -1)
        call setwinvar(winnr, '&winhighlight', 'inactive:normal')
        return
      endif
    endif
    call setwinvar(winnr, '&winhighlight', 'inactive:MyInactiveWin')
  endfunction
  augroup inactive_win
    au!
    au ColorScheme * hi MyInactiveWin ctermbg=18
    au FileType,WinNew * call s:configure_winhighlights()
    au FocusLost * for w in range(1, winnr('$')) | call s:configure_winhighlights(w, 1) | endfor | set winhighlight+=,normal:MyInactiveWin
    au FocusGained * for w in range(1, winnr('$')) | call s:configure_winhighlights(w) | endfor | set winhighlight-=,normal:MyInactiveWin
  augroup END
else
  Plug 'blueyed/vim-diminactive'
endif

Not sure if the usage of +=,normal:MyInactiveWin and -=,normal:MyInactiveWin is supported intentionally?!

@blueyed

This comment has been minimized.

Contributor

blueyed commented May 5, 2017

Using hi link works for Focus{Gained,Lost}, thanks!

if exists('+winhighlight')
  function! s:configure_winhighlight()
    let ft = &filetype
    let bt = &buftype
    " Check white/blacklist.
    if index(['dirvish'], ft) == -1
          \ && (index(['nofile', 'nowrite', 'acwrite', 'quickfix', 'help'], bt) != -1
          \     || index(['startify'], ft) != -1)
      set winhighlight=Normal:MyNormalWin,NormalNC:MyNormalWin
      " echom "normal" winnr() &winhighlight 'ft:'.&ft 'bt:'.&bt
    else
      set winhighlight=Normal:MyNormalWin,NormalNC:MyInactiveWin
      " echom "inactive" winnr() &winhighlight 'ft:'.&ft 'bt:'.&bt
    endif
  endfunction
  augroup inactive_win
    au!
    au ColorScheme * hi link MyInactiveWin ColorColumn | hi link MyNormalWin Normal
    au FileType,BufWinEnter * call s:configure_winhighlight()
    au FocusGained * hi link MyNormalWin Normal
    au FocusLost * hi link MyNormalWin MyInactiveWin
  augroup END
else
  Plug 'blueyed/vim-diminactive'
endif
@blueyed

This comment has been minimized.

Contributor

blueyed commented May 6, 2017

It does not cover the region used by 'breakindent'.

I am using:

let &showbreak = '' 
if exists('+breakindent')
  set breakindent
  set breakindentopt=min:20,shift:0,sbr
endif

2017-05-06-180743_365x200_scrot

(the same applies to the 'colorcolumn' hack (from my plugin).

@justinmk

This comment has been minimized.

Member

justinmk commented May 6, 2017

Is that the Whitespace or SpecialChar highlight taking priority?

@bfredl

This comment has been minimized.

Member

bfredl commented May 6, 2017

No, it happens even if NonText doesn't have bg, so it is a bug...

@bfredl

This comment has been minimized.

Member

bfredl commented May 7, 2017

should work correctly with non-text highlights now.

@bfredl

This comment has been minimized.

Member

bfredl commented May 7, 2017

@justinmk Should we merge this? There might be more bad interactions, but the best way to find that out is let more people try it. Test errors look quite unrelated.

@blueyed

This comment has been minimized.

Contributor

blueyed commented May 7, 2017

I found another case where it is not applied: the extends from 'listchars':

2017-05-07-180529_230x152_scrot

@bfredl

This comment has been minimized.

Member

bfredl commented May 8, 2017

lots of ad-hoc logic... For some reason the terminal has its own beyond-end-of-line loop, instead of reusing the existing one. Someone disabled colorcolumn in terminal instead of bothering fixing it proberly, but you can the same breakage using cursorcolumn...

@bfredl

This comment has been minimized.

Member

bfredl commented May 8, 2017

should work with precedes and extends now.

@@ -930,6 +932,10 @@ struct window_S {
synblock_T *w_s; /* for :ownsyntax */
int w_hl_id; ///< 'winhighlight' id
int w_hl_id_inactive; ///< 'winhighlight' id for inactive window

This comment has been minimized.

@justinmk

justinmk May 8, 2017

Member

To support all builtin hl groups in the future, I suppose this will be some sort of list?

This comment has been minimized.

@bfredl

bfredl May 8, 2017

Member

Actually, if we do 'highlight' reform, this should just mimic the global hl_attr() list. Then we can have hl_attr_w(wp, HLF_XX), support a lot of stuff by doing :%s/hl_attr(/hl_attr_w(wp,/gc in screen.c and as a bonus remove a lot of the ad-hoc logic this PR currently needed to deal with vim's ad-hoc logic.

This comment has been minimized.

@justinmk

justinmk May 8, 2017

Member

FWIW I think we shouldn't hesitate to do "'highlight' reform".

This comment has been minimized.

@bfredl

bfredl May 8, 2017

Member

I'm not hesitating ,I'm working on it!

end)
it('works local to the buffer', function()

This comment has been minimized.

@justinmk

justinmk May 8, 2017

Member

it's window-local, not buffer-local, right?

This comment has been minimized.

@fmoralesc

fmoralesc May 8, 2017

Contributor

Wasn't it decided that when setlocal was used, it would be buffer-local?

This comment has been minimized.

@fmoralesc

This comment has been minimized.

@bfredl

bfredl May 8, 2017

Member

That was decided by bram long ago. "window option" means global=per window, local=per buffer.

This comment has been minimized.

@justinmk

justinmk May 8, 2017

Member

Hmm.. after testing, it does seem buffer-specific. Option rules continue to mystify.

This comment has been minimized.

@bfredl

bfredl May 8, 2017

Member

and as windows proliferate by division, setting it in vimrc will set it for all windows, unless changed later. But there is no true global-global value.

@justinmk justinmk merged commit 188bae5 into neovim:master May 8, 2017

1 of 3 checks passed

QuickBuild Build pr-6597 finished with status FAILED
Details
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

justinmk added a commit that referenced this pull request May 8, 2017

@justinmk justinmk removed the RFC label May 8, 2017

@rafaeln

This comment has been minimized.

rafaeln commented May 10, 2017

Hey, @blueyed, I was a happy user of vim-diminactive before, and now I'm an even happier user of the snipped you supplied above. Have you noticed, though, that it won't change the background if there isn't a file open, that is to say, if you open vim without supplying a filename? Incidentally, if you do that, open an empty split, then move between them, the original split all of a sudden starts changing background, but the new one doesn't.

@blueyed

This comment has been minimized.

Contributor

blueyed commented May 12, 2017

@rafaeln
Thanks!
I am using a modified version locally already, and plan to integrate it into vim-diminactive, so that it uses Neovim's method if it is available instead of the colorcolumn hack. Not sure if I'll finish it this soonish though.
I've updated the snippet above (#6597 (comment)), which should also fix your issue if I understand it correctly.

@rafaeln

This comment has been minimized.

rafaeln commented Sep 16, 2017

Hi, @blueyed, is the snippet still working for you? It was maybe after I upgraded neovim today that it stopped working for me. Specifically, it still works between vim splits, but stopped working between a vim pane and other tmux panes. I tried to understand better what was going on and so far I found that Focus{Gained,Lost} events are still being detected.

@rafaeln

This comment has been minimized.

rafaeln commented Oct 10, 2017

Now the snippet got even weirder...

@blueyed

This comment has been minimized.

Contributor

blueyed commented Oct 10, 2017

@rafaeln
It worked until recently for me.
This broke it: #7082 (comment)

justinmk added a commit to justinmk/neovim that referenced this pull request Nov 8, 2017

NVIM v0.2.1
FEATURES:
0e873a3 Lua(Jit) built-in neovim#4411
5b32bce Windows: `:terminal` neovim#7007
7b0ceb3 UI/API: externalize cmdline neovim#7173
b67f58b UI/API: externalize wildmenu neovim#7454
b23aa1c UI: 'winhighlight' neovim#6597
17531ed UI: command-line coloring (`:help input()-highlight`) neovim#6364
244a1f9 API: execute lua directly from the remote api neovim#6704
45626de API: `get_keymap()` neovim#6236
db99982 API: `nvim_get_hl_by_name()`, `nvim_get_hl_by_id()` neovim#7082
dc68538 menu_get() function neovim#6322
9db42d4 :cquit : take an error code argument neovim#7336
9cc185d job-control: serverstart(): support ipv6 neovim#6680
1b7a9bf job-control: sockopen() neovim#6594
6efe84a clipboard: fallback to tmux clipboard neovim#6894
6016ac2 clipboard: customize clipboard with `g:clipboard` neovim#6030
3a86dd5 ruby: override ruby host via `g:ruby_host_prog` neovim#6841
16cce1a debug: $NVIM_LOG_FILE neovim#6827
0cba3da `:checkhealth` built-in, validates $VIMRUNTIME neovim#7399

FIXES:
105d680 TUI: more terminals, improve scroll/resize neovim#6816
cb912a3 :terminal : handle F1-F12, other keys neovim#7241
619838f inccommand: improve performance neovim#6949
04b3c32 inccommand: Fix matches for zero-width neovim#7487
60b1e8a inccommand: multiline, other fixes neovim#7315
f1f7f3b inccommand: Ignore leading modifiers in the command neovim#6967
1551f71 inccommand: fix 'gdefault' lockup neovim#7262
6338199 API: bufhl: support creating new groups neovim#7414
541dde3 API: allow K_EVENT during operator-pending
8c732f7 terminal: adjust for 'number' neovim#7440
5bec946 UI: preserve wildmenu during jobs/events neovim#7110
c349083 UI: disable 'lazyredraw' during ui_refresh. neovim#6259
51808a2 send FocusGained/FocusLost event instead of pseudokey neovim#7221
133f8bc shada: preserve unnamed register on restart neovim#4700
1b70a1d shada: avoid assertion on corrupt shada file neovim#6958
9f534f3 mksession: Restore tab-local working directory neovim#6859
de1084f fix buf_write() crash neovim#7140
7f76986 syntax: register 'Normal' highlight group neovim#6973
6e7a8c3 RPC: close channel if stream was closed neovim#7081
85f3084 clipboard: disallow recursion; show hint only once neovim#7203
8d1ccb6 clipboard: performance, avoid weird edge-cases neovim#7193
01487d4 'titleold' neovim#7358
01e53a5 Windows: better path-handling, separator (slash) hygiene neovim#7349
0f2873c Windows: multibyte startup arguments neovim#7060

CHANGES:
9ff0cc7 :terminal : start in normal-mode neovim#6808
032b088 lower priority of 'cursorcolumn', 'colorcolumn' neovim#7364
2a3bcd1 RPC: Don't delay notifications when request is pending neovim#6544
023f67c :terminal : Do not change 'number', 'relativenumber' neovim#6796
1ef2d76 socket.c: Disable Nagle's algorithm on TCP sockets neovim#6915
6720fe2 help: `K` tries Vim help instead of manpage neovim#3104
7068370 help, man.vim: change "outline" map to `gO` neovim#7405

@justinmk justinmk referenced this pull request Nov 8, 2017

Merged

NVIM v0.2.1 #7505

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment