Skip to content

Conversation

keremc
Copy link
Contributor

@keremc keremc commented Sep 10, 2016

Add support for special color names:

  • If "bg" or "background" is used as a color name, use the background color of the Normal highlight group.
  • If "fg" or "foreground" is used as a color name, use the foreground color of Normal.
  • When Normal is modified, refresh the color attributes of all highlight groups.

In vim all of this already works. Special color names are converted to real colors in color_name2handle() for guibg/guifg (and in do_highlight() for ctermbg/ctermfg but this works in neovim too). When the Normal group is modified, do_highlight() calls highlight_gui_started() to refresh the highlight groups.

@keremc
Copy link
Contributor Author

keremc commented Sep 10, 2016

This should fix #4463.

@justinmk
Copy link
Member

Thanks a lot. Do you plan to add a test? See test/functional/ui/highlight_spec.lua or test/functional/terminal/highlight_spec.lua

@keremc
Copy link
Contributor Author

keremc commented Sep 10, 2016

I'll try to add some tests.

@keremc
Copy link
Contributor Author

keremc commented Sep 10, 2016

Added tests. Some will fail unfortunately.

end)

it('returns gui-color if "gui" arg is passed', function()
eq('Black', eval('synIDattr(hlID("Normal"), "bg", "gui")'))
eq('Maroon', eval('synIDattr(hlID("Keyword"), "sp", "gui")'))
eq('bg', eval('synIDattr(hlID("Visual"), "fg", "gui")'))
eq('fg', eval('synIDattr(hlID("Visual"), "bg", "gui")'))
eq('bg', eval('synIDattr(hlID("Visual"), "sp", "gui")'))
Copy link
Member

@justinmk justinmk Sep 10, 2016

Choose a reason for hiding this comment

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

Let's move these into separate it() blocks like this, for clarity:

it('returns "Normal" values for group using special fg/bg names', function()
  execute('highlight Visual ctermfg=bg ctermbg=fg guifg=bg guibg=fg guisp=bg')
  eq(eval('synIDattr(hlID("Normal"),  "fg", "gui")'), eval('synIDattr(hlID("Visual"),  "fg", "gui")'))
  eq(eval('synIDattr(hlID("Normal"),  "bg", "gui")'), eval('synIDattr(hlID("Visual"),  "bg", "gui")'))
  ...
end)

All the tests you added can live in this single test. Note the execute() setup line also lives in this it() instead of before_each since it only applies to this one test.

@justinmk
Copy link
Member

Some will fail unfortunately.

Are there more bugs? Or something wrong with the changes here?

@keremc
Copy link
Contributor Author

keremc commented Sep 10, 2016

Are there more bugs? Or something wrong with the changes here?

Yes, the tests were buggy. Since highlight Normal ctermfg=252 guifg=#ff0000 guibg=Black didn't set the ctermbg attribute of Normal, highlight Visual ctermfg=bg ctermbg=fg guifg=bg guibg=fg guisp=bg would fail to execute.

@justinmk
Copy link
Member

Since highlight Normal ... didn't set the ctermbg attribute of Normal, highlight Visual ... would fail to execute

What does that failure look like to the user (does it print an error message)?

@keremc
Copy link
Contributor Author

keremc commented Sep 10, 2016

What does that failure look like to the user (does it print an error message)?

It prints E420: BG color unknown.

name_to_color(HL_TABLE()[idx].sg_rgb_sp_name);
}
set_hl_attr(idx);
}
// If the normal group has changed, it is simpler to refresh every UI
ui_refresh();
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this is still needed now that you are calling set_hl_attr(idx) on every highlight group.

Anyways, let's put this and the block you added in a new function, highlight_groups_refresh().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is still needed. When I remove it, the tests still pass but with manual testing you can see that it doesn't work.

@keremc
Copy link
Contributor Author

keremc commented Sep 15, 2016

Is there anything else that needs to be fixed?

justinmk pushed a commit that referenced this pull request Sep 15, 2016
Refresh colors if changing Normal group
@justinmk
Copy link
Member

Merged/squashed/verified. Thanks for digging into this @keremc

@justinmk justinmk closed this Sep 15, 2016
justinmk added a commit to justinmk/neovim that referenced this pull request Oct 27, 2016
FEATURES:
0b5a7e4 neovim#4432 API: external UIs can render custom popupmenu
c6ac4f8 neovim#4934 API: call any API method from vimscript
31df051 neovim#4568 API: nvim_call_atomic(): multiple calls in a single request
b268ba3 neovim#5424 API: nvim_win_get_number(), nvim_tabpage_get_number()
e7e2844 has("nvim-1.2.3") checks for a specific Nvim version
522b885 neovim#5295, neovim#5493 `:CheckHealth` checks tmux, terminfo, performance
719dae2 neovim#5384 events: allow event processing in getchar()
f25797f neovim#5386 API: metadata: Nvim version & API level
22dfe69 neovim#5389 API: metadata: "since", "deprecated_since"
605e743 Added QuickFixLine highlight group

CHANGES:
4af6ec7 neovim#5253 perf: Disable clipboard in do_cmdline()
6e9f329 neovim#5299 perf: Skip foldUpdate() in insert-mode.
9d4fcec neovim#5426 perf: Do not auto-update folds for some foldmethods.
eeec0ca neovim#5419 tui: Default to normal-mode cursor shape.

FIXES:
e838452 neovim#5436 tui: Fix "weird characters" / "bleeding termcodes"
10a54ad neovim#5243 signal_init: Always unblock SIGCHLD.
bccb49b neovim#5316 eval.c: Fix memory leak for detached pty job
626065d neovim#5227 tchdir: New tab should inherit CWD.
cd321b7 neovim#5292 getcwd(): Return empty string if CWD is invalid.
6127eae shada: Fix non-writeable ShaDa directory handling
ca65514 neovim#2789 system(): Respect shellxescape, shellxquote
2daf54e neovim#4874 Restore vim-like tab dragging
0c536b5 neovim#5319 syntax.c: Support bg/fg special color-names.
3c53371 neovim#4972 from justinmk/schedule-ui_refresh
68bcb32 neovim#4789 tui.c: Do not wait for tui loop on teardown.
c8b6ec2 neovim#5409 v:count broken in command-line window
6bc3bce neovim#5461 fix emoji display
51937e1 neovim#5470 fix :terminal with :argadd, :argu
79d77da neovim#5481 external UIs: opening multiple files from command-line
657ba62 neovim#5501 rplugin: resolve paths in manifest file
6a6f188 neovim#5502 system('foo &', 'bar'): Show error, don't crash.
1ff162c neovim#5515 os_nodetype: open fd with O_NONBLOCK
2a6c5bb neovim#5450 modeline: Handle version number overflow.
0ade1bb neovim#5225 CI tests now run against Windows!
justinmk added a commit to justinmk/neovim that referenced this pull request Oct 28, 2016
FEATURES:
0b5a7e4 neovim#4432 API: external UIs can render custom popupmenu
c6ac4f8 neovim#4934 API: call any API method from vimscript
31df051 neovim#4568 API: nvim_call_atomic(): multiple calls in a single request
b268ba3 neovim#5424 API: nvim_win_get_number(), nvim_tabpage_get_number()
e7e2844 has("nvim-1.2.3") checks for a specific Nvim version
522b885 neovim#5295, neovim#5493 `:CheckHealth` checks tmux, terminfo, performance
719dae2 neovim#5384 events: allow event processing in getchar()
f25797f neovim#5386 API: metadata: Nvim version & API level
22dfe69 neovim#5389 API: metadata: "since", "deprecated_since"
605e743 Added QuickFixLine highlight group

CHANGES:
4af6ec7 neovim#5253 perf: Disable clipboard in do_cmdline()
6e9f329 neovim#5299 perf: Skip foldUpdate() in insert-mode.
9d4fcec neovim#5426 perf: Do not auto-update folds for some foldmethods.
eeec0ca neovim#5419 tui: Default to normal-mode cursor shape.

FIXES:
e838452 neovim#5436 tui: Fix "weird characters" / "bleeding termcodes"
10a54ad neovim#5243 signal_init: Always unblock SIGCHLD.
bccb49b neovim#5316 eval.c: Fix memory leak for detached pty job
626065d neovim#5227 tchdir: New tab should inherit CWD.
cd321b7 neovim#5292 getcwd(): Return empty string if CWD is invalid.
6127eae shada: Fix non-writeable ShaDa directory handling
ca65514 neovim#2789 system(): Respect shellxescape, shellxquote
2daf54e neovim#4874 Restore vim-like tab dragging
0c536b5 neovim#5319 syntax.c: Support bg/fg special color-names.
3c53371 neovim#4972 from justinmk/schedule-ui_refresh
68bcb32 neovim#4789 tui.c: Do not wait for tui loop on teardown.
c8b6ec2 neovim#5409 v:count broken in command-line window
6bc3bce neovim#5461 fix emoji display
51937e1 neovim#5470 fix :terminal with :argadd, :argu
79d77da neovim#5481 external UIs: opening multiple files from command-line
657ba62 neovim#5501 rplugin: resolve paths in manifest file
6a6f188 neovim#5502 system('foo &', 'bar'): Show error, don't crash.
1ff162c neovim#5515 os_nodetype: open fd with O_NONBLOCK
2a6c5bb neovim#5450 modeline: Handle version number overflow.
0ade1bb neovim#5225 CI tests now run against Windows!
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.

2 participants