[RDY]:hi Normal works with -u NONE #6973

teto · 2017-07-06T17:28:03Z

The PR has 2 commits, the first one makes :hi Normal works with -u NONE works, just like vim.
This first commit also:

Makes HL_TABLE and ATTR_ENTTRY a function instead of a macro so that in can be used in gdb.
Introduces ATTRENTRY_INIT to init attrentry_tchanges, they make sense in the context of [WIP] support transparent & ctermbg cursor + minimal highlight api #6566 ; the long term goal is to be able to distinguish between NONE and an unset color.

The 2nd commit is just some documentation/styling changes (brackets) I had made to fix linting issues in another PR. I salvaged them but they are not necessary (might need to do another pass at it) so I can ditch it or send it another PR it that.

Nb: #6566 had become a mess so I am trying to split it into meaningful commits.

jamessan · 2017-07-06T19:05:12Z

src/nvim/syntax.c

+
+  // HACK we need to make sure that the first highlight is the "Normal" hl
+  if (highlight_ga.ga_len == 0) {
+    int res = syn_add_group((char_u *)"Normal");


syn_add_group() requires its argument to be an allocated string. You're passing a static string instead. Try this:

int res = syn_add_group((char_u *)xstrdup("Normal"));

jamessan · 2017-07-06T19:06:39Z

src/nvim/syntax_defs.h

@@ -73,4 +73,14 @@ typedef struct attr_entry {
  int cterm_fg_color, cterm_bg_color;
 } attrentry_T;

+#define ATTRENTRY_INIT {.rgb_ae_attr = 0, .cterm_ae_attr = 0, \


Why not put rgb_ae_attr and cterm_ae_attr on their own lines too? It looks a little odd having these two here but everything else on its own line.

jamessan · 2017-07-06T23:36:24Z

src/nvim/syntax.c

+  // HACK we need to make sure that the first highlight is the "Normal" hl
+  if (highlight_ga.ga_len == 0) {
+    int res = syn_add_group((char_u *)xstrdup("Normal"));
+    assert(res == 0);


Need to add a (void)res; line so release builds think res is used.

Indeed. I would like to move the Normal hl creation to init_highlight but that would need more thoughts/tests as I am afraid :high Normal NONE or some other command could break things.

jamessan · 2017-07-07T01:12:04Z

From test_signs.vim:
Found errors in Test_sign():
function RunTheTest[9]..Test_sign line 19: Pattern '^sign Sign1 text=x sign Sign2 icon=../../pixmaps/stock_vim_find_help.png .*text=xy linehl=Error texthl=Title$' does not match 'sign Sign1 text=x sign Sign2 icon=../../pixmaps/stock_vim_find_help.png (not supported) text=xy linehl=MatchParen texthl=VertSplit'

teto · 2017-07-07T10:54:20Z

not sure https://travis-ci.org/neovim/neovim/jobs/251093995#L2732 is related to my changes:

 [----------] Running tests from /home/travis/build/neovim/neovim/test/functional/autocmd/termclose_spec.lua

[ RUN      ] TermClose event triggers when terminal job ends: -- Output to stderr:

Vim: Caught deadly signal 'SIGHUP'

is it possible to restart the failed tests please ?

jamessan · 2017-07-07T11:03:20Z

is it possible to restart the failed tests please ?

Done.

codecov-io · 2017-08-15T17:48:57Z

Codecov Report

Merging #6973 into master will increase coverage by 0.01%.
The diff coverage is 91.89%.

@@            Coverage Diff             @@
##           master    #6973      +/-   ##
==========================================
+ Coverage   77.03%   77.05%   +0.01%     
==========================================
  Files         131      131              
  Lines      106919   106925       +6     
  Branches    25630    25628       -2     
==========================================
+ Hits        82365    82389      +24     
+ Misses      24544    24526      -18     
  Partials       10       10

Impacted Files	Coverage Δ
src/nvim/window.c	`81.98% <ø> (+0.31%)`	⬆️
src/nvim/syntax.c	`69.5% <91.89%> (+0.07%)`	⬆️
src/nvim/terminal.c	`88.81% <0%> (-1.21%)`	⬇️
src/nvim/msgpack_rpc/channel.c	`87.08% <0%> (-1.1%)`	⬇️
src/nvim/event/process.c	`94.11% <0%> (-0.54%)`	⬇️
src/nvim/ex_getln.c	`74.17% <0%> (-0.08%)`	⬇️
src/nvim/tui/tui.c	`84.74% <0%> (+0.11%)`	⬆️
src/nvim/eval.c	`81.8% <0%> (+0.18%)`	⬆️
src/nvim/os/signal.c	`81.48% <0%> (+1.85%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44dc8bb...fa8b58f. Read the comment docs.

teto · 2017-08-15T19:01:39Z

I updated the first post to better reflect the current content.

teto · 2017-08-21T21:54:54Z

Would be happy to see this merged (even if only the first commit) as it solves the :hi Normalproblem and helps me fixing #7082

justinmk · 2017-08-21T22:48:22Z

src/nvim/syntax.c


-  if (STRCMP(HL_TABLE()[idx].sg_name_u, "NORMAL") == 0)
-    is_normal_group = TRUE;
+  is_normal_group = (STRCMP(HL_TABLE()[idx].sg_name_u, "NORMAL") == 0);


before, is_normal_group kept its value if the if-block was not entered. Is this change intentional?

it's the only place that sets is_normal_group so it's equivalent.

justinmk · 2017-08-21T22:49:55Z

src/nvim/syntax.c

+/// Table with the specifications for an attribute number.
+/// Note that this table is used by ALL buffers.  This is required because the
+/// GUI can redraw at any time for any buffer.
+/// The attributes table


the last line seems a bit unnecessary

justinmk · 2017-08-21T22:51:55Z

src/nvim/syntax.c

- * Lookup a highlight group name and return it's ID.
- * If it is not found, 0 is returned.
- */
+/// Lookup a highlight group name and return it's ID.


justinmk · 2017-08-21T22:52:21Z

src/nvim/syntax.c

+/// Lookup a highlight group name and return it's ID.
+///
+/// @param highlight name e.g. 'Cursor', 'Normal'
+/// @return the highlight id, else 0 if it \p name does not exist


justinmk · 2017-08-21T22:52:31Z

src/nvim/syntax.c

- * "name" must be an allocated string, it will be consumed.
- * Return 0 for failure.
- */
+/// Add new highlight group and return it's ID.


justinmk · 2017-08-21T22:57:19Z

Great to see smaller PRs, it helps a lot :>

LGTM, one comment: #6973 (comment)

teto · 2017-08-22T10:34:05Z

The failure does not seem related to the changes:

[----------] Running tests from /home/travis/build/neovim/neovim/test/functional/autocmd/termclose_spec.lua

[ RUN      ] TermClose event triggers when fast-exiting terminal job stops: -- Output to stderr:

Vim: Caught deadly signal 'SIGHUP'

Vim: Finished.

CMake Error at /home/travis/build/neovim/neovim/cmake/RunTests.cmake:53 (message):

  Running functional tests failed with error: 1.

- :hi Normal works with -u NONE - Makes HL_TABLE and ATTR_ENTYRY a function instead of a macro so that in can be used in gdb. - Introduces ATTRENTRY_INIT to init attrentry_t

Converts some documentation to doxygen format + minor styling improvements.

teto · 2017-08-22T15:56:48Z

Thanks for the review, I rebased and pushed your fixes. Should be ok now.

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

jamessan reviewed Jul 6, 2017

View reviewed changes

teto force-pushed the normal_hl branch from 57adf4a to 4a27a1e Compare July 6, 2017 23:08

jamessan reviewed Jul 6, 2017

View reviewed changes

teto changed the title ~~:hi Normal works with -u NONE~~ [WIP]:hi Normal works with -u NONE Jul 6, 2017

teto force-pushed the normal_hl branch from 4a27a1e to b4153f3 Compare July 6, 2017 23:59

marvim added the WIP label Jul 7, 2017

teto force-pushed the normal_hl branch from b4153f3 to d0090a6 Compare July 7, 2017 00:23

teto force-pushed the normal_hl branch from d0090a6 to cf46fec Compare July 7, 2017 09:50

teto force-pushed the normal_hl branch from cf46fec to 20a2c06 Compare July 7, 2017 16:14

teto changed the title ~~[WIP]:hi Normal works with -u NONE~~ [RFC]:hi Normal works with -u NONE Jul 7, 2017

marvim added RFC and removed WIP labels Jul 7, 2017

teto force-pushed the normal_hl branch from 20a2c06 to ede6d47 Compare July 9, 2017 17:12

teto changed the title ~~[RFC]:hi Normal works with -u NONE~~ [WIP]:hi Normal works with -u NONE Jul 9, 2017

marvim added WIP and removed RFC labels Jul 9, 2017

teto force-pushed the normal_hl branch from ede6d47 to fa8b58f Compare August 15, 2017 16:11

teto changed the title ~~[WIP]:hi Normal works with -u NONE~~ [RFC]:hi Normal works with -u NONE Aug 15, 2017

marvim added RFC and removed WIP labels Aug 15, 2017

teto force-pushed the normal_hl branch from fa8b58f to 3317aaf Compare August 16, 2017 22:24

teto mentioned this pull request Aug 17, 2017

[RDY] Adds nvim_get_hl_by_name/by_id #7082

Merged

teto changed the title ~~[RFC]:hi Normal works with -u NONE~~ [RDY]:hi Normal works with -u NONE Aug 21, 2017

marvim added RDY and removed RFC labels Aug 21, 2017

teto force-pushed the normal_hl branch from 3317aaf to ab6539d Compare August 21, 2017 17:46

justinmk reviewed Aug 21, 2017

View reviewed changes

teto force-pushed the normal_hl branch from ab6539d to 0450ebb Compare August 22, 2017 09:33

justinmk approved these changes Aug 22, 2017

View reviewed changes

teto added 2 commits August 22, 2017 12:37

syntax.c: register 'Normal' highlight group

4d91369

- :hi Normal works with -u NONE - Makes HL_TABLE and ATTR_ENTYRY a function instead of a macro so that in can be used in gdb. - Introduces ATTRENTRY_INIT to init attrentry_t

syntax.c: style

5f6ad86

Converts some documentation to doxygen format + minor styling improvements.

teto force-pushed the normal_hl branch from 0450ebb to 5f6ad86 Compare August 22, 2017 10:42

justinmk merged commit 7f76986 into neovim:master Aug 22, 2017

justinmk removed the RDY label Aug 22, 2017

teto deleted the normal_hl branch August 22, 2017 22:03

justinmk mentioned this pull request Nov 8, 2017

NVIM v0.2.1 #7505

Merged

Uh oh!

[RDY]:hi Normal works with -u NONE #6973

[RDY]:hi Normal works with -u NONE #6973

Uh oh!

Conversation

teto commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamessan commented Jul 7, 2017

Uh oh!

teto commented Jul 7, 2017

Uh oh!

jamessan commented Jul 7, 2017

Uh oh!

codecov-io commented Aug 15, 2017

Codecov Report

Uh oh!

teto commented Aug 15, 2017

Uh oh!

teto commented Aug 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk commented Aug 21, 2017

Uh oh!

teto commented Aug 22, 2017

Uh oh!

teto commented Aug 22, 2017

Uh oh!

Uh oh!

teto commented Jul 6, 2017 •

edited

Loading