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

[RDY] Adds nvim_get_hl_by_name/by_id #7082

Merged
merged 4 commits into from Oct 8, 2017

Conversation

Projects
None yet
5 participants
@teto
Contributor

teto commented Jul 27, 2017

...in order to retrieve highlights.
UIs are also sent new/modified highlights.

I renamed EMPTY_ATTR to HLATTRS_INIT because it's more consistent with the rest of the code and it helps disambiguate the highlight vs attribute vs hlattr confusion. I can revert it though.

@teto teto changed the title from Adds nvim_get_hl_by_name/by_id to [RFC] Adds nvim_get_hl_by_name/by_id Jul 27, 2017

@marvim marvim added the RFC label Jul 27, 2017

@teto teto referenced this pull request Jul 28, 2017

Closed

Synchronizing colors: #33

@teto teto changed the title from [RFC] Adds nvim_get_hl_by_name/by_id to [wip] Adds nvim_get_hl_by_name/by_id Aug 7, 2017

attrs.reverse = mask & (HL_INVERSE | HL_STANDOUT);
if (use_rgb) {
if (aep->rgb_fg_color != normal_fg) {

This comment has been minimized.

@teto

teto Aug 10, 2017

Contributor

This kind of calls make the export fail for the "Normal" highlight. I'll try to replace "normal_fg" by -1 and see if htis has any adverse impact.

@@ -58,7 +58,8 @@ void set_title(String title)
FUNC_API_SINCE(3);
void set_icon(String icon)
FUNC_API_SINCE(3);
void highlight_info_set(Array highlights_changed)
FUNC_API_SINCE(3);

This comment has been minimized.

@bfredl

bfredl Aug 10, 2017

Member

Do you plan to use it in the TUI? Otherwise it should be REMOTE_ONLY and remove unnecessary boilerplate

This comment has been minimized.

@teto

teto Aug 10, 2017

Contributor

I plan to use it for TUI, to update the cursor color when the hl changes.

This comment has been minimized.

@bfredl

bfredl Aug 10, 2017

Member

Why not include the translated colors in the mode_info dict? We should avoid unnecessary indirections.

This comment has been minimized.

@teto

teto Aug 16, 2017

Contributor

I agree but let's keep that for another PR. Meanwhile I will extract the "highlight changed" into separate commit so that you can choose to include it or not (right now all UIs get notifications even when not interested).

This comment has been minimized.

@bfredl

bfredl Aug 17, 2017

Member

I'm not against the "highlight changed" event, it will be very useful for other purposes. I'm just saying whenever we implement cursor colors in TUI (in this or next PR doesn't matter) we should do it the right way directly, so there will never be a need for TUI to receive this event.

@teto teto force-pushed the teto:export_hl2 branch from c25d0dc to bdf1054 Aug 15, 2017

@teto

This comment has been minimized.

Contributor

teto commented Aug 17, 2017

Current nvim export highlight colors that are different from "Normal"'s colors, I changed that so that nvim exports initialized colors (!=-1). This requires to initialize rgb fields with -1, which is done in #6973.

@teto teto force-pushed the teto:export_hl2 branch from bdf1054 to be851c5 Aug 23, 2017

@teto

This comment has been minimized.

Contributor

teto commented Aug 24, 2017

Travis fails because of:

FAILED neovim/neovim/test/functional/autocmd/termclose_spec.lua�[0m @ �[36m47�[0m: �[1mTermClose event kills pty job trapping SIGHUP and SIGTERM�[0m
...neovim/neovim/test/functional/autocmd/termclose_spec.lua:60: Expected objects to be the same.
Passed in:
(number) 6
Expected:
(number) 4

I removed the highlight change notification code to trim down the PR and the most controversial part.

(n)vim used to conflate the 'Normal' hl with the uninitialized attribute (attribute 0).
I've removed this confusion so that an attribute can be generated for the 'Normal' hl too.

@teto teto changed the title from [wip] Adds nvim_get_hl_by_name/by_id to [rfc] Adds nvim_get_hl_by_name/by_id Aug 24, 2017

return nvim_get_attr(attrcode, err);
}
Dictionary nvim_get_attr(Integer attr_id, Error *err)

This comment has been minimized.

@justinmk

justinmk Aug 24, 2017

Member

Needs a doc. Would nvim_get_text_attr_by_id be a better name? Or nvim_get_screen_cell_attr_by_id ?

This comment has been minimized.

@justinmk

justinmk Aug 24, 2017

Member

Also there's no test for this.

This comment has been minimized.

@teto

teto Aug 24, 2017

Contributor

I am not sure how to test that one or if it should be part of the API as there is no public function AFAIK to map a hl to its attribute (though UIs can receive attribute ids). Attributes are more of a vim internal implementation detail.
What should be done is a way for the UI to combine several hl but I prefer to keep this PR short.

This comment has been minimized.

@bfredl

bfredl Aug 24, 2017

Member

I would argue any situation that needs attr combination it should be done nvim side not ui side, with possible exception of ui-managed windows/floats having separate default colors, which can be seen as extension of existing default color management and is much simpler than arbitrary combination

@@ -166,6 +166,88 @@ void ui_event(char *name, Array args)
}
}
/// @param[in] aep data to convert
/// @param[out] out structure that will be sent to UIs

This comment has been minimized.

@teto

teto Aug 24, 2017

Contributor

gotta fix the doc here 👎

PUT(hl, "reverse", BOOLEAN_OBJ(true));
}
// todo care, we send none; change order between none/invalid

This comment has been minimized.

@teto

teto Aug 24, 2017

Contributor

remove

@teto teto force-pushed the teto:export_hl2 branch from be851c5 to c4e0b03 Aug 24, 2017

@teto

This comment has been minimized.

Contributor

teto commented Aug 24, 2017

So I moved nvim_get_attr_by_id to nvim/ui.c as it's not of much use and can be confusing if made public. It's tested through its callers nvim_get_hl_by_id / nvim_get_hl_by_name

@bfredl

This comment has been minimized.

Member

bfredl commented Aug 24, 2017

Then nvim_ prefix should be removed, it is for public functions

@teto teto force-pushed the teto:export_hl2 branch 3 times, most recently from 78fc5f0 to 0d525ab Aug 24, 2017

@teto

This comment has been minimized.

Contributor

teto commented Aug 25, 2017

done

@teto teto changed the title from [rfc] Adds nvim_get_hl_by_name/by_id to [rdy] Adds nvim_get_hl_by_name/by_id Aug 26, 2017

describe('highlight api', function()
before_each(function()
clear('--cmd', 'set termguicolors')

This comment has been minimized.

@justinmk

justinmk Aug 26, 2017

Member

does it require RGB? why?

This comment has been minimized.

@teto

teto Aug 26, 2017

Contributor

I tested the most useful configuration there. I can add without termguicolors as well.

This comment has been minimized.

@justinmk

justinmk Aug 26, 2017

Member

Won't it be useful for non-rgb?

This comment has been minimized.

@teto

teto Aug 26, 2017

Contributor

The only non-RGB UIs I can think of are terminal based and even those tend to disappear as always more terminals support RGB. I hope that neovim-based UIs won't limit themselves to 16 colors. Anyway that should work even with non-RGB. I can add the tests.

/// Retrieves attribute description from its id
///
/// @param attr_id attribute id
Dictionary get_attr_by_id(Integer attr_id, Error *err)

This comment has been minimized.

@justinmk

justinmk Aug 26, 2017

Member

these new functions are in the UI module but don't have a ui_ prefix. If they really belong there then they should have that prefix. Not sure if they do belong there though, hl_ prefix might be better.

This comment has been minimized.

@teto

teto Aug 26, 2017

Contributor

I am not sure where to put it then or how to call it. If I put it in vim.c it will be autommatically picked up :/

This comment has been minimized.

@teto

teto Aug 26, 2017

Contributor

should I move it to syntax.c ? or I can merge it with nvim_get_hl_by_id but I believe the current logic separation is clearer.

/// Retrieves highlight description from its id
///
/// @param hl_id highlight id as returned by hlID()

This comment has been minimized.

@justinmk

justinmk Aug 26, 2017

Member

putting bars around tags (|hlID()|) will link the tag when the API docs are autogenerated.

it("nvim_get_hl_by_name", function()
local expected_hl = { background = Screen.colors.Yellow,
foreground = Screen.colors.Red }

This comment has been minimized.

@justinmk

justinmk Aug 26, 2017

Member

missing tests for "underline", etc.

This comment has been minimized.

@teto

teto Aug 26, 2017

Contributor

I considered it was covered by ui tests since they are relying on the same exporting function (to HlAttrs) but I can add that as well.

@teto teto force-pushed the teto:export_hl2 branch from 0d525ab to c15ec17 Aug 26, 2017

@teto

This comment has been minimized.

Contributor

teto commented Aug 26, 2017

I moved the function to syntax.c:hl_get_attr_by_id and added cterm tests.

@teto teto force-pushed the teto:export_hl2 branch from eb04bd7 to 1334e7b Sep 7, 2017

@teto teto changed the title from [rdy] Adds nvim_get_hl_by_name/by_id to [RDY] Adds nvim_get_hl_by_name/by_id Sep 7, 2017

@marvim marvim added RDY and removed RFC labels Sep 7, 2017

@teto

This comment has been minimized.

Contributor

teto commented Sep 14, 2017

I understand not to merge code before a release but if #6428 targeted is still 0.2.1 I can provide subsequent patches once this is merged, 1 of ~ 6 lines to distinguish between NONE and other colors, the other is also ~10 lines but need some input on how to properly convey hl updates.

@teto teto referenced this pull request Sep 29, 2017

Merged

[RFC] vim-patch:8.0.0142 #7335

@@ -6826,7 +6826,7 @@ int hl_combine_attr(int char_attr, int prim_attr)
// Copy all attributes from char_aep to the new entry
new_en = *char_aep;
} else {
memset(&new_en, 0, sizeof(new_en));
new_en = (attrentry_T)ATTRENTRY_INIT;

This comment has been minimized.

@bfredl

bfredl Sep 29, 2017

Member

is this not redundant, as new_en already has the correct value?

This comment has been minimized.

@teto

teto Sep 30, 2017

Contributor

seems redundant indeed

teto added some commits Jul 26, 2017

Adds nvim_get_hl_by_name/by_id
...in order to retrieve highlights.

Added test/functional/api/highlight_spec.lua
HL_NORMAL is not really a good name, since it's more like an empty attribute than the normal's one.
If one pays attention, syn_cterm_attr2entry is never called with attr=0 because it's always special cased before.
I suggest in subsequent PRs we remove the ATTR_OFF and just insert an EMPTY ATTR/RESET_ATTR/UNINITIALIZED for id 0.

@teto teto force-pushed the teto:export_hl2 branch from 1334e7b to 481e40c Sep 30, 2017

justinmk added a commit that referenced this pull request Oct 7, 2017

vim-patch:8.0.0142 (#7335)
see also #7082

Problem:    Normal colors are wrong with 'termguicolors'.
Solution:   Initialize to INVALCOLOR instead of zero. (Ben Jackson, closes vim/vim#1344)

vim/vim@0cdb72a
-- assume there is no hl with id = 30000
local err, emsg = pcall(meths.get_hl_by_id, 30000, false)
eq(false, err)
ok(string.find(emsg, 'Invalid highlight id') ~= nil)

This comment has been minimized.

@justinmk

justinmk Oct 8, 2017

Member

I changed this (and the other) to use eq() and string.match():

eq('Invalid highlight id', string.match(emsg, 'Invalid highlight id'))

that gives nicer failure messages if the test fails.

@@ -6859,6 +6857,7 @@ int hl_combine_attr(int char_attr, int prim_attr)
/// \note this function does not apply exclusively to cterm attr contrary
/// to what its name implies
/// \warn don't call it with attr 0 (i.e., the null attribute)

This comment has been minimized.

@justinmk

justinmk Oct 8, 2017

Member

@teto was there a reason we can't assert() this?

This comment has been minimized.

@teto

teto Oct 9, 2017

Contributor

I think it's safe to assert on this, but it might be even better to remove the +1/-1 all around the code, which IIRC could be done in a backwards compatible way.

@justinmk justinmk merged commit 481e40c into neovim:master Oct 8, 2017

4 checks passed

QuickBuild Build pr-7082 finished with status SUCCESSFUL
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 76.475%
Details

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

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

test: nvim_get_hl_by_name/by_id #7082
- test all properties
- test failure modes

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

@justinmk justinmk removed the RDY label Oct 8, 2017

@justinmk

This comment has been minimized.

Member

justinmk commented Oct 8, 2017

LGTM. Added some extra tests in 5251732

@teto teto deleted the teto:export_hl2 branch Oct 9, 2017

@blueyed

This comment has been minimized.

Contributor

blueyed commented Oct 9, 2017

ba7277c seems to break the "new diminactive" I am using:

    if exists('+winhighlight')
      function! s:configure_winhighlight()
        let ft = &filetype
        let bt = &buftype
        " Check white/blacklist.
        if &diff || (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()
        if exists('##OptionSet')
          " TODO: does not work with :Gdiff - diffsplit does not trigger it, too!
          au OptionSet diff call s:configure_winhighlight()
        endif
        " XXX: redraw! should not be necessary, but is?!
        " NOTE: setting cursorline fixes it?!
        " au FocusGained * hi link MyNormalWin Normal | redraw!
        " au FocusLost * hi link MyNormalWin MyInactiveWin | redraw!
        " au FocusGained * hi link MyNormalWin Normal
        " au FocusLost * hi link MyNormalWin MyInactiveWin
        " au FocusGained * echom 'FG: '.&winhighlight
        " au FocusLost * echom 'FL: '.&winhighlight | verb hi MyNormalWin
      augroup END
    endif

It now dims the active window, but not the inactive ones after that commit.

@blueyed

This comment has been minimized.

Contributor

blueyed commented Oct 9, 2017

@teto
Any idea why this might be reversed now?

@teto

This comment has been minimized.

Contributor

teto commented Oct 10, 2017

in my code, I've tried to make the "Normal" group behave like any other group. I have splitted my PR so that it gets easier to review so I wonder if the problem might be due to this hunk (that should possibly belong to one of my other PR):

// The "Normal" group doesn't need an attribute number
-  if (sgp->sg_name_u != NULL && STRCMP(sgp->sg_name_u, "NORMAL") == 0) {
-    return;
-  }

that might badly interact with winhighlight code

    if (strncmp("Normal", p, nlen) == 0) {
      w_hl_id_normal = hl_id;
    } else {
      for (hlf = 0; hlf < (int)HLF_COUNT; hlf++) {
        if (strncmp(hlf_names[hlf], p, nlen) == 0) {
          w_hl_ids[hlf] = hl_id;
          break;
        }
      }
      if (hlf == HLF_COUNT) {
        return false;
      }
    }

Maybe if you don't special case "Normal" highlight it works


      for (hlf = 0; hlf < (int)HLF_COUNT; hlf++) {
        if (strncmp(hlf_names[hlf], p, nlen) == 0) {
          w_hl_ids[hlf] = hl_id;
          break;
        }
      }
      if (hlf == HLF_COUNT) {
        return false;
      }
    }

I've got some trouble on my setup so I can't test this. I hope to be able to this weekend (unrelated but my brand new laptop screen refuses to turn on after 2 days of utilization :'( )

@bfredl

This comment has been minimized.

Member

bfredl commented Oct 11, 2017

It special cases Normal because normal is different (every other winhl needs to be combined with the Normal winhl). Your PR just made it special in a slightly different way. But I would have expected the tests to caught it, looking into it now.

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