[RFC] man.vim: highlight bold and underlined text #7623

keidax · 2017-11-23T01:45:18Z

I was inspired by #5847, but wanted to keep the original syntax highlighting colors:

As much as I optimized the Vimscript version it felt a little too slow. Working in Lua speeds things up nicely. My (very informal) testing with LuaJIT shows a roughly 10% overall increase in the time to start nvim and render a man page.

It doesn't look like there's any other Lua code included in the runtime yet, but I hope this can still be considered.

I also didn't handle any ANSI SGR sequences, since man on my system just uses backspaces, but if there's interest I can try to include those too.

justinmk · 2017-11-23T02:05:41Z

This is cool! We certainly aren't opposed to using Lua for all runtime code that Nvim project maintains (need to make a list of the exact files somewhere ...). And we definitely forked man.vim so forking it even more is fine :)

mhinz · 2017-11-23T11:29:03Z

Works good on macOS!

EDIT:

I think as long as there aren't any cases where SGR is used to enable bold mode (as opposed to CSI 1m), leaving it out is fine.

mhinz · 2017-11-25T23:49:15Z

Can this be merged?

nhooyr · 2017-11-25T23:52:23Z

I'd like to review it first.

justinmk · 2017-11-26T00:36:51Z

runtime/syntax/man.vim

+
+augroup man_init_highlight_groups
+  autocmd!
+  autocmd ColorScheme * call s:init_highlight_groups()


why is this needed? highlight default is a one-time thing.

I needed it in order to keep the highlighting across colorscheme changes. AFAICT highlight default links are preserved after a highlight clear, but groups (other than the builtin ones) with defined default attributes get cleared. Am I missing something?

Does this happen on startup, or only if user changes colorscheme at runtime? The latter is not important. And why wouldn't this also be needed for the other hi default's, e.g. on line 19:

highlight default link manSubHeading Function

More generally, syntax needs to be re-initialized if a colorscheme or whatever nukes it. Every plugin cannot be expected to deal with that ad-hoc.

keidax · 2017-12-06T04:38:06Z

Rebased, added escape sequence handling (for bold, italics, and underlines), plus some test cases (just for highlighting).

justinmk · 2017-12-07T09:53:20Z

runtime/autoload/man.vim

@@ -388,4 +387,14 @@ function! man#init_pager() abort
  execute 'silent file man://'.fnameescape(ref)
 endfunction

+function! man#highlight_formatted_text() abort
+  let l:modifiable = &modifiable
+  set modifiable


This could be done in the Lua code via vim.api.nvim_command() or whatever. Then we can eliminate this "public" VimL function.

nhooyr

I found this really hard to read but that may just be because I have very little experience with Lua.

Would appreciate a review from someone else more familiar with Lua.

nhooyr · 2017-12-23T22:19:41Z

runtime/lua/man.lua

+      chars[#chars + 1] = char
+    elseif escape then
+      -- Use prev_char to store the escape sequence
+      prev_char = prev_char .. char


Why do this? Why not just use char directly?

Because it's matching against the entire escape sequence, not just the last character.

nhooyr · 2017-12-23T23:22:09Z

runtime/lua/man.lua

+          add_attr_hl(match + 0) -- coerce to number
+        end
+        escape = false
+      elseif not prev_char:match("^%[[\020-\063]*$") then


Why is this needed?

Since it's iterating one character at a time, that's checking for a partial CSI sequence, and throwing away the sequence if it ends up being something else.

nhooyr · 2017-12-23T23:27:55Z

runtime/lua/man.lua

+      if sgr then
+        local match = ''
+        while sgr and #sgr > 0 do
+          match, sgr = sgr:match("^(%d*);?(.*)")


why the ? before (.*)?

It makes the semicolon optional, to match sequences like \e[4;22m as well as \e[0m

nhooyr · 2017-12-23T23:33:05Z

runtime/lua/man.lua

+  end
+
+  -- Break input into UTF8 characters
+  for char in line:gmatch("[^\128-\191][\128-\191]*") do


Why do we need that pattern? Why not just "." as the pattern?

"." will just match a single byte. But the input is encoded as UTF-8, so matching one byte at a time won't work if any characters fall outside of the ASCII range. More details here.

Side note: I've only ever seen UTF-8, but I wonder if other encodings would be possible based on terminal/locale settings? If so, would likely need some sort of Lua string library to deal with.

As long as all Ctrl chars are ASCII, which I strongly suspect in man format, segmenting UTF-8 codepoints (which not unambiguously is "chars" anyway) shouldn't be needed. The highlight API only cares about bytes anyway.

Depending on locale settings, there could be a lot of overstruck text outside of ASCII range. Handling those multibyte text characters correctly requires splitting into codepoints.

Is it well-defined somewhere that overstrike works on a codepoint at a time, and neither byte nor grapheme cluster (which is closer to user-perceived character)?

Based on my testing & digging in the grotty sources, it certainly looks that way.

Regardless of precedence, wouldn't it be more correct to handle overstrikes as applying to a grapheme and not a codepoint?

nhooyr · 2017-12-23T23:53:08Z

runtime/lua/man.lua

+        -- bullet (overstrike text '+^Ho')
+        attr = BOLD
+        char = [[·]]
+      elseif prev_char == [[·]] and char == 'o' then


why a double square bracket?

nhooyr · 2017-12-23T23:55:25Z

runtime/lua/man.lua

+    elseif escape then
+      -- Use prev_char to store the escape sequence
+      prev_char = prev_char .. char
+      local sgr = prev_char:match("^%[([\020-\063]*)m$")


Could you link http://invisible-island.net/xterm/ctlseqs/ctlseqs.html as a reference to understand the reasoning behind the structure of regular expression.

Hmm, I forget why I chose that particular range, but [\020-\063] is wrong. For a valid CSI sequence, the bytes in between [ and the final byte must be in the ASCII range 0x20-0x3f, so that should be [\032-\063]. I don't think it's mentioned in the xterm page, but this is part of the ECMA-48 standard, sect. 5.4. I'll add an explaining comment.

nhooyr · 2017-12-23T23:56:50Z

runtime/lua/man.lua

+      on = false
+    elseif code == 1 then
+      attr = BOLD
+    elseif code == 21 or code == 22 then


21 is described as a double underline at http://invisible-island.net/xterm/ctlseqs/ctlseqs.html

Good point. I was looking at the Wikipedia page which indicates "Bold off or Double Underline". I never saw 21 used anywhere, so I'll drop it.

nhooyr · 2017-12-23T23:57:21Z

runtime/lua/man.lua

+      attr = ITALIC
+    elseif code == 23 then
+      attr = ITALIC
+      on = false


Instead of on, I think end is more clear, as in whether or not to end the attribute.

nhooyr · 2017-12-24T00:13:18Z

runtime/lua/man.lua

+  local chars = {}
+  local prev_char = ''
+  local overstrike, escape = false, false
+  local hls = {} -- Store highlight groups as { attr, start, end }


This is really janky imo to not name the fields stored in that tuple but I'm not sure if there is a better way. I have very little lua experience.

Yeah, I'll name them

nhooyr · 2017-12-24T00:17:09Z

runtime/lua/man.lua

+      local last_hl = hls[#hls]
+      if char == prev_char then
+        if char == '_' and attr == UNDERLINE and last_hl and last_hl[3] == byte then
+          -- This underscore is in the middle of an underlined word


When would this occur? Could you provide some sample input to make the comment more clear?

And also, is there any precedence for this behaviour on when to underline vs bold in regards to an overstriken underscore? To me, it seems intuitive that it would always become bolded.

This doesn't occur too often, but one example would be the man page for mawk. View it with less, search for opt_expr, and you'll see an underlined word with an underscore. The behavior of checking the previous overstrike and defaulting to bold is essentially what less does too.

Very odd, I'd expect that to be a space, followed by a backspace and then an underline, not an underline, backspace and then underline. LGTM but do we need the last_hl and last_hl[3] == byte check?

bfredl · 2017-12-24T21:28:37Z

test/functional/plugin/man_spec.lua

+      {{ bold = true, foreground = Screen.colors.Blue }})
+    end
+
+    local function expect_without_highlights(string)


This is an anti-pattern. Just use screen:snapshot_util() and it will capture highlighting with not much extra work

bfredl · 2017-12-24T21:29:23Z

test/functional/plugin/man_spec.lua

+      screen:detach()
+    end)
+
+    local function expect(string)


screen:set_default_attr_ids

bfredl · 2017-12-24T21:30:20Z

test/functional/plugin/man_spec.lua

+      screen:expect(string, nil, true)
+    end
+
+    local function insert_lines(...)


helpers.insert

I added this because the text contains a lot of control characters, and I found

insert_lines("i\bi")

more readable than

helpers.insert("i<C-v><C-h>i")

Would you prefer the second form?

@keidax The second form is preferred, it's the prevailing convention in our tests and throughout Vim-land.

keidax · 2017-12-28T04:31:23Z

Thanks for the feedback! I rebased and addressed all the comments.

justinmk · 2017-12-28T12:09:26Z

test/functional/plugin/man_spec.lua

+end)
+
+describe('In autoload/man.vim', function()
+  describe('function man#highlight_formatted_text', function()


Function name needs update

justinmk · 2017-12-28T12:13:23Z

LGTM thanks for reviews!

nhooyr

Made some comments on outdated issues.

justinmk · 2018-01-09T09:11:12Z

This works well, and performance seems good. Thanks everyone!

aktau · 2018-01-09T10:17:52Z

This doesn't work for me, I'm guessing because of LUA_PATH issues I was afraid of a while ago:

E5105: Error while calling lua chunk: [string "<VimL compiled string>"]:1: module 'man' not found

Still investigating.

aktau · 2018-01-09T10:22:23Z

On second thought, it may just be because the man.lua file is not installed:

11:20:01 ~/neovim » fname lua
./share/nvim/runtime/syntax/lua.vim
./share/nvim/runtime/doc/if_lua.txt
./share/nvim/runtime/indent/lua.vim
./share/nvim/runtime/ftplugin/lua.vim

aktau · 2018-01-09T10:26:45Z

Confirmed, the following makes it work:

11:25:38 neovim/runtime git:(master) » cp -r lua $HOME/neovim/share/nvim/runtime

(My install prefix is -DCMAKE_INSTALL_PREFIX:PATH=$(HOME)/neovim).

justinmk mentioned this pull request Nov 23, 2017

[RFC] man.vim: bold and underline backspaced text #5847

Closed

marvim added the RFC label Nov 23, 2017

justinmk reviewed Nov 26, 2017

View reviewed changes

keidax force-pushed the man-highlight branch from cb1189f to cb7531e Compare December 6, 2017 04:26

justinmk reviewed Dec 7, 2017

View reviewed changes

nhooyr suggested changes Dec 24, 2017

View reviewed changes

justinmk added this to the 0.2.4 milestone Dec 24, 2017

bfredl reviewed Dec 24, 2017

View reviewed changes

keidax added 5 commits December 27, 2017 23:27

Highlight backspaced characters

0446d4d

Switch to processing in Lua

c28ce5f

Add support for escape sequences

6740c94

Add functional tests for man highlighting

134c0f0

Address PR comments

eb44519

keidax force-pushed the man-highlight branch from cb7531e to eb44519 Compare December 28, 2017 04:29

justinmk reviewed Dec 28, 2017

View reviewed changes

nhooyr reviewed Dec 28, 2017

View reviewed changes

justinmk merged commit eb44519 into neovim:master Jan 9, 2018

justinmk added a commit that referenced this pull request Jan 9, 2018

Merge #7623 'man.vim: highlight bold, underlined text'

59888b6

justinmk removed the RFC label Jan 9, 2018

justinmk mentioned this pull request Jan 9, 2018

Start newsletter #9 neovim/neovim.github.io#154

Closed

justinmk removed this from the 0.2.4 milestone Jan 9, 2018

justinmk added this to the 0.2.3 milestone Jan 9, 2018

justinmk mentioned this pull request Mar 25, 2018

man.vim: bold and underline highlighting #5846

Closed

justinmk mentioned this pull request Jun 24, 2018

less.sh, less.vim ctl seqs #6762

Closed

[RFC] man.vim: highlight bold and underlined text #7623

[RFC] man.vim: highlight bold and underlined text #7623

Conversation

keidax commented Nov 23, 2017

justinmk commented Nov 23, 2017

mhinz commented Nov 23, 2017 • edited

mhinz commented Nov 25, 2017

nhooyr commented Nov 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keidax commented Dec 6, 2017

Choose a reason for hiding this comment

nhooyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhooyr Dec 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keidax commented Dec 28, 2017

Choose a reason for hiding this comment

justinmk commented Dec 28, 2017

nhooyr left a comment

Choose a reason for hiding this comment

justinmk commented Jan 9, 2018

aktau commented Jan 9, 2018

aktau commented Jan 9, 2018

aktau commented Jan 9, 2018

mhinz commented Nov 23, 2017 •

edited

nhooyr Dec 28, 2017 •

edited