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] Add a way to color command-line #6364

Merged
merged 52 commits into from Aug 15, 2017

Conversation

Projects
None yet
6 participants
@ZyX-I
Contributor

ZyX-I commented Mar 26, 2017

Based on #5119 as I do not want merge conflicts.

The idea is that we can color Neovim command-line. Ultimate goal is

  1. Refactor VimL parsing so it can produce information about colors (for :/command prompts).
  2. Refactor VimL expression parsing so it can produce informaiton about colors (for =/expr prompts).
  3. Add a way for the user to specify how his input() prompts should be colored.
  4. Add a way for the user to specify how his custom commands should be colored.

This is logical to split that into the following PRs:

  1. (This) add basic prompt coloring functionality. Adds a few callbacks, including one which may be used to make pygments color Ex commands and expressions for us. Also add a way for the user to color input() prompts. If some strange problems will not occur this part is going to be easy.
  2. Remove callback which colors expressions, move expressions parsing to src/nvim/eval and split it from execution, make it produce coloring information. Obsoletes or reuses expression parser from #243. Reuses tests from #243. Resulting code is going to be rather unoptimal: expressions are going to be reparsed each time.
  3. (Many PRs) port commands parsing from #243; most likely one-by-one since #243 is not good for highlighting and is rather outdated. Execute commands with C, no translation. Again reparse things each time up until all commands are ported.
  4. Actually build AST for functions and execute it without reparsing. Though this has nothing to do with command-line coloring.
  5. Only now provide :command -color=funcname for coloring user commands.

Proposed API for input():

input(prompt :: String[, text :: String[, completion :: String]]): no additions.

input(prompt :: String, options :: Dictionary), options allow keys text, completion and highlight. No third argument allowed.

@ZyX-I ZyX-I added the ui label Mar 26, 2017

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Mar 26, 2017

PR commits start with “ex_getln: Clean up draw_cmdline a bit”.

@marvim marvim added the WIP label Mar 26, 2017

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Mar 26, 2017

Example code which will highlight using pygments:

# color_cmdline.py
def color_cmdline_setup():
    import vim
    from pygments import lex, format
    from pygments.lexers import get_lexer_by_name
    from pygments.formatter import Formatter
    from pygments.token import Token

    try:
        from __builtin__ import unicode
    except ImportError:
        unicode = str

    COLORS = {
        Token:                    'Normal',

        Token.Comment:            'Comment',
        Token.Comment.Preproc:    'PreProc',
        Token.Keyword:            'Keyword',
        Token.Keyword.Type:       'Type',
        Token.Operator:           'Operator',
        Token.Name:               'Identifier',
        Token.Name.Builtin:       'Statement',
        Token.Name.Function:      'Function',
        Token.Name.Constant:      'Number',
        Token.String:             'String',
        Token.Number:             'Number',
        Token.Punctuation:        'Operator',

        Token.Error:              'Error',
    }

    class NvimFormatter(Formatter):
        def __init__(self, **kwargs):
            super(NvimFormatter, self).__init__(**kwargs)
            self.offset = 0
            self.colorscheme = kwargs.get('colorscheme', None) or COLORS

        def format_unencoded(self, tokensource, outfile):
            for ttype, value in tokensource:
                ttype_str = str(ttype).replace(' ', '_')
                value = value.rstrip('\n')
                color = self.colorscheme.get(ttype)
                while color is None:
                    ttype = ttype[:-1]
                    color = self.colorscheme.get(ttype)
                if isinstance(value, unicode):
                    vlen = len(value.encode('utf-8'))
                else:
                    vlen = len(value)
                if vlen:
                    outfile.write('{} {} {} {}\n'.format(
                        self.offset, self.offset + vlen, color, ttype_str))
                self.offset += vlen

    def setup():
        global color_cmdline_highlight
        lexer = get_lexer_by_name('vim')

        def color_cmdline_highlight():
            cmdline = vim.eval('a:cmdline')
            add_tt = False
            if bool(int(vim.eval('get(a:000, 0)'))):
                add_tt = True
            tokens = lex(cmdline, lexer)
            fmt = format(tokens, NvimFormatter(bg=vim.options['background']))
            return [
                ((lambda s, e, g, tt: (
                    (int(s), int(e), g)
                    + ((tt,) if add_tt else ())
                ))(*line.split()))
                for line in fmt.split('\n')[:-1]
            ]

    setup()


color_cmdline_setup()
pyfile <sfile>:h/color_cmdline.py
function PygmentsFormat(cmdline, ...)
  return pyeval('color_cmdline_highlight()')
endfunction
let g:Nvim_color_cmdline = 'PygmentsFormat'
@bfredl

This comment has been minimized.

Member

bfredl commented Mar 26, 2017

it might be worthwhile to think of how this will interact with #6162 later. I guess GUIs can recieve a list of pairs [text, attrs] with the attrs that apply to that chunk of text, alternatively just the text and the list of[start, stop, attrs].

@justinmk justinmk added this to the 0.3 milestone Mar 26, 2017

:echo "«^ |
]])
end)
it('does the right thing when errorring', function()

This comment has been minimized.

@brcolow

brcolow Mar 27, 2017

Contributor

typo: errorring -> erroring

@ZyX-I ZyX-I force-pushed the ZyX-I:colored-cmdline branch 2 times, most recently from 8798c20 to 4c841ca Mar 29, 2017

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Apr 1, 2017

Actually it appears that input({opts}) is better then requiring prompt. What’s the point in requiring prompt if the official documentation states that you may have no prompt by supplying it an empty string, so basically prompt is not required?

@bfredl

This comment has been minimized.

Member

bfredl commented Apr 2, 2017

I would guess the purpose is to nudge plugin authors to consider adding a prompt (instead of just calling input() with no args without even checking the docs that a prompt is possible), but with a dict they will need to check the docs anyway so then it makes sense to make it optional.

Also, having user commands and input() executing a callback on every input change is going to be useful for other purposes than coloring the cmdline, for instance live feedback in a window like inccommand. So I wonder if the callback argument really should be called highlight or color, maybe instead callback to acknowledge that it has other potential uses?

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Apr 16, 2017

@bfredl One of the points of the coloring is “better have no colors then make user problems”. People using that for other purposes should be prepared that Neovim may reject calling their callback if it causes troubles: shows errors, takes too long to finish, alters some state, explicitly disabled by the user, etc. In the PR I am only going to disable it for errors, arabic shaping (not sure how to implement and how to test) and probably for some option set, but that may be extended depending on the problems I spot. If you know an existing facility to automatically interrupt after some time passed I would implement “too long to finish” as well.

@bfredl

This comment has been minimized.

Member

bfredl commented Apr 17, 2017

alters some state, explicitly disabled by the user, etc. In the PR I am only going to disable it for errors, arabic shaping (not sure how to implement and how to test) and probably for some option set,

This would be fine for other use cases as long as one still can rpcnotify() (not rpcrequest()) and start_timer(0, ...) (currently the canonical way of "do something when the main loop is ready")

If you know an existing facility to automatically interrupt

I don't know if got_int somehow can be abused for this. Checking got_int is quite common but calling os_breakcheck less so (which would be the natural place to check the time limit)

@ZyX-I ZyX-I force-pushed the ZyX-I:colored-cmdline branch from b011894 to b69a1cf May 11, 2017

@ZyX-I ZyX-I force-pushed the ZyX-I:colored-cmdline branch from b69a1cf to 311f898 May 20, 2017

@ZyX-I ZyX-I force-pushed the ZyX-I:colored-cmdline branch from 89b6be4 to 072a853 Jun 26, 2017

ZyX-I added some commits Jun 26, 2017

functests: Abstract away some ways to enter cmdline coloring mode
Reason: should actually switch to using input() coloring because other coloring 
variants are eventually going away.
functests: Make tests work with input()
There are still some issues: specifically, new “pending” test hangs busted.
@justinmk

This comment has been minimized.

justinmk commented on src/nvim/api/private/helpers.c in 4d8ff5e Aug 6, 2017

typo

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Aug 6, 2017

(Without the merge clint fails locally.)

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Aug 6, 2017

AppVeyor failure looks unrelated:

[  FAILED  ] 1 test, listed below:
[  FAILED  ] C:/projects/neovim/test/functional\core\job_spec.lua @ 69: jobs changes to given `cwd` directory
C:/projects/neovim/test/functional\core\job_spec.lua:78: Expected objects to be the same.
Passed in:
(table) {
  [1] = 'notification'
  [2] = 'stdout'
 *[3] = {
    [1] = 0
   *[2] = {
      [1] = 'C:\projects\neovim\Xtest-tmpdir\nvimdDir0n\0' } } }
Expected:
(table) {
  [1] = 'notification'
  [2] = 'stdout'
 *[3] = {
    [1] = 0
   *[2] = {
      [1] = 'C:\projects\neovim\Xtest-tmpdir\nvimdDir0n\0'
     *[2] = '' } } }
stack traceback:
	C:/projects/neovim/test/functional\core\job_spec.lua:78: in function <C:/projects/neovim/test/functional\core\job_spec.lua:69>

Travis hanged in oldtests, test49:

!echo 'q^Mq' | $NVIM_PRG -u NONE -N -es -c 'debuggreedy|set viminfo+=nviminfo' -c 'let ExtraVimBegin = 1' -c 'let ExtraVimResult = "/tmp/nvimSdwGqV/9"' -c 'breakadd func 3 G' -S /tmp/nvimSdwGqV/8
No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

Restarting.

@justinmk

This comment has been minimized.

Member

justinmk commented Aug 6, 2017

My tentative observation is that the test49 hang seems to persist if travis cache is not cleared.

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Aug 13, 2017

@justinmk This is planned to be merged after dealing with more urgent issues for the next (0.2.1) release?

@justinmk

This comment has been minimized.

Member

justinmk commented Aug 13, 2017

@ZyX-I If you think it's ready, I think we should include it in 0.2.1

@@ -265,6 +273,8 @@ Lua interface (|if_lua.txt|):
on cancel and completion respectively) via dictionary argument (replaces all
other arguments if used).
|input()| and |inputdialog()| now support user-defined cmdline highlighting.

This comment has been minimized.

@justinmk

justinmk Aug 13, 2017

Member

Why is only highlighting mentioned, in all of these docs? This is a handler that is called on each input character, correct? That's useful for much more than highlighting.

This comment has been minimized.

@ZyX-I

ZyX-I Aug 13, 2017

Contributor

@bfredl asked this somewhere above: it is not guaranteed to be called, if real hooks cause any problems then I would rather add code which detects those problems and stops using hook then do anything. Currently this includes stopping calling the hook in case of arabic characters present. And handler is not called on each input character, it is called on redraw, minus cases when hook output can be taken from cache. I don’t have any information regarding what caused redraw, so one will have to compute a diff: hook always receives the whole drawn command-line. And whatever other uses you have in mind, note that hook is not supposed to edit input, :echo anything or issue any errors.

This comment has been minimized.

@ZyX-I

ZyX-I Aug 13, 2017

Contributor

Though no, arabic characters fall under different category: “hard to implement, unlikely to happen and should really be solved on the different layer and not with some hacks in ex_getln.c and screen.c (it contains another incarnation of arabic shaping code)”. What falls into “causes problems” category now are hooks which throw errors.

@@ -4703,6 +4703,7 @@ input({opts})
cancelreturn "" Same as {cancelreturn} from
|inputdialog()|. Also works with
input().
highlight nothing Highlight handler: |Funcref|.

This comment has been minimized.

@justinmk

justinmk Aug 13, 2017

Member

How often is the handler called? Should mention this somewhere.

Typically we prefix VimL handlers with on_ (Vim does _cb suffix). Maybe this should be called on_input. But this is not a passive event, it's more of a hook. So maybe it should be called hl_hook (emacs has a similar foo-hook convention).

This comment has been minimized.

@ZyX-I

ZyX-I Aug 13, 2017

Contributor

on_input is not right, it is currently called on redraw. And I do not like the hook or event view of the function, in my view it is more like a syntax definition, in a form of the VimL code: this allows things like caching in case of double highlight I implemented, additionally I don’t know command-line redrawing system enough to answer questions like “how many times and for which states highlight will be called when triggering a mapping defined like cnoremap a bcd<C-\>e"efg"<CR>” without experiments.

UI now supports command-line coloring. Officially only |input()| and
|inputdialog()| may be colored, temporary for testing purposes expressions
(e.g. |i_CTRL-R_=|) and regular command-line (|:|) are colored by callbacks
defined in `g:Nvim_color_expr` and `g:Nvim_color_cmdline` respectively.

This comment has been minimized.

@justinmk

justinmk Aug 13, 2017

Member

g:Nvim_ might not be a good convention because sessionoptions+=globals will try to store such variables if they are string/num types (doesn't apply here, but as a general convention ...).

Since these are temporary and deprecated, perhaps underscore prefix would make sense. g:_nvim_foo

This comment has been minimized.

@ZyX-I

ZyX-I Aug 13, 2017

Contributor

@justinmk I can’t have leading underscores, funcref variable names must start with a capital. Underscore is not one.

local screen
-- Bug in input() handling: {REDRAW} will erase the whole prompt up until

This comment has been minimized.

@justinmk

justinmk Aug 13, 2017

Member

should say :redraw! instead of {REDRAW} in this context.

} else if (colored_ccline->cmdfirstc == ':') {
try_enter(&tstate);
err_errmsg = N_(
"E5408: Unable to get Nvim_color_cmdline callback from g:: %s");

This comment has been minimized.

@justinmk

justinmk Aug 13, 2017

Member

g:: looks awkward. Suggestion:

"E5408: Unable to get g:Nvim_color_cmdline callback: %s"
@justinmk

This comment has been minimized.

Member

justinmk commented Aug 13, 2017

There's a non-trivial amount of logic (and documentation) involving getln_interrupted_highlight, prompt_id, related to concerns about performance. Just curious, why would performance be noticed here, typically commands occupy only 1 line, and AFAIK vim does not take such measures for insert-mode highlighting.

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Aug 13, 2017

Insert-mode highlighting is done in C code. I am running VimL and expect it to be slow in cases like somebody trying to implement pygments in pure VimL. And I do not see too non-trivial amount: for getln_interrupted_highlight it is just “if <C-c> happened to arrive while highlighting assume that highlighting appeared to be too slow for the user to be disturbed, so no longer execute the highlight (:try catches interrupts as exceptions, so nothing is done specially here, they go with other errors), but do not assume that user meant to interrupt the prompt (what getln_interrupted_highlight is for)”. prompt_id is there not for performance, it disables running highlight in case of errors (in order to not spam user with errors), but makes the disablement not entirely permanent, only for one prompt.

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Aug 13, 2017

Or, the other way around: getln_interrupted_highlight is what allows you to interrupt infinite cycle in highlight function, but still run what you want. Without it you could not do let g:Nvim_color_cmdline = function('Halting') and run any Ex mode command afterwards without hacks (there still is <C-r>= and execute()), including the one which will fix the problem.

prompt_id allows disabling highlighting for the duration of one prompt only, in the above case it is needed for you to not type <C-c> after typing each character of unlet g:Nvim_color_cmdline. Though I think I can remove prompt_id and just keep prev_prompt_errors and last_ccline_colors in CmdlineInfo. Behaviour will slightly change: in the example you would need to type second <C-c> if you use another prompt: :u<C-c>nlet <C-r>="g"<CR>:<C-c>Nvim_color_cmdline<CR>, and highlight function will not be recalled.

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Aug 14, 2017

CI succeeded, only left running is gcov “flaky” build which will not possibly affect green status.

@ZyX-I

This comment has been minimized.

Contributor

ZyX-I commented Aug 14, 2017

All “flaky” appeared to be green as well, it is uncommon.

@justinmk justinmk merged commit 19a2835 into neovim:master Aug 15, 2017

4 checks passed

QuickBuild Build pr-6364 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 increased (+0.06%) to 76.421%
Details

@justinmk justinmk removed the RFC label Aug 15, 2017

justinmk added a commit that referenced this pull request Aug 15, 2017

@ZyX-I ZyX-I deleted the ZyX-I:colored-cmdline branch Aug 15, 2017

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