change defaults for fillchars 'vert' and 'fold' #7986

teto · 2018-02-08T16:38:51Z

follow up of #7403. On top of changing the defaults for fillchar "vert", I've made cleaner the handling of defaults (benefit might be more visible with more fillchars but that might happen):
https://github.com/neovim/neovim/pull/6073/files#diff-bfacc6d8f849b9559c1c6cd5b79a5431R3412

Current tests show that this new default might generate a pb for CJK fonts. Not sure how to deal with it.


[  ERROR   ] 2 errors, listed below:
[  ERROR   ] ...test/functional/legacy/095_regexp_multibyte_spec.lua @ 13: regex with multi-byte is working
test/functional/helpers.lua:93: Vim(set):E835: Conflicts with value of 'fillchars': ambiwidth=double

stack traceback:
	test/functional/helpers.lua:93: in function 'request'
	test/functional/helpers.lua:167: in function 'nvim_command'
	test/functional/helpers.lua:368: in function 'source'
	...test/functional/legacy/095_regexp_multibyte_spec.lua:17: in function <...test/functional/legacy/095_regexp_multibyte_spec.lua:13>

[  ERROR   ] ...eovim/test/functional/legacy/autocmd_option_spec.lua @ 309: au OptionSet with specific option being set by neovim api should trigger if a string option be set globally
test/functional/helpers.lua:93: E835: Conflicts with value of 'fillchars'

stack traceback:
	test/functional/helpers.lua:93: in function <test/functional/helpers.lua:86>
	(tail call): ?
	(tail call): ?
	...eovim/test/functional/legacy/autocmd_option_spec.lua:312: in function <...eovim/test/functional/legacy/autocmd_option_spec.lua:309>

teto · 2018-02-09T15:05:40Z

So I tried to look for a solution. both the new fillchar vert and fold have an ambiguous width: our character is
2502;BOX DRAWINGS LIGHT VERTICAL;So;0;ON;;;;;N;FORMS LIGHT VERTICAL;;;;
and in unicode/EastAsianWidth.txt,

# The format is two fields separated by a semicolon.
# Field 0: Unicode code point value or range of code point values
# Field 1: East_Asian_Width property, consisting of one of the following values:
#         "A", "F", "H", "N", "Na", "W"
2500..254B;A     # So    [76] BOX DRAWINGS LIGHT HORIZONTAL..BOX DRAWINGS HEAVY VERTICAL AND HORIZONTAL

the A stands for ambiguous and is thus governed by ambiwdith (see http://www.unicode.org/reports/tr11/ for the others, Na=> Narrow). So if we set it as default, it might cause problems for every user with set ambiwidth=2. Possible actions:

We can cheat and mark the character as singlewidth, might generate some problems though
accept to draw 2 columns split between windows :s
keep current default or find another one
proceed and break some configs
I hope I haven't missed anything.

justinmk · 2018-02-09T15:11:24Z

@teto What about setting a different fillchar when :set ambiwidth=double is invoked?

teto · 2018-02-09T15:21:52Z

It would appear as a weird sideeffect. How is the user supposed to understand why changing ambiwidth changes his fillchar value. We could convert the error into a warning since I guess in most cases it would still be visually ok but I am not a specialist of CJK fonts.

justinmk · 2018-02-09T15:24:10Z

We could convert the error into a warning since I guess in most cases it would still be visually ok but I am not a specialist of CJK fonts.

Let's special-case these 2 glyphs and skip the error entirely. If it causes visual problems we can revisit.

justinmk · 2018-02-10T18:26:39Z

runtime/doc/options.txt

 	  diff:c	'-'		deleted lines of the 'diff' option

 	Any one that is omitted will fall back to the default.  For "stl" and
 	"stlnc" the space will be used when there is highlighting, '^' or '='
 	otherwise.

+	For "vert" and "fold", the new defaults '│' and '·' being of ambiguous
+	width for CJK languages, nvim will fallback to the old defaults '|' and
+	'-' when |ambiwidth| is set to "double".


I thought we agreed not to do this. Did you find a problem when ambiwitdh is used?

I tried quickly but any way I could think of was too complex or brittle so I followed your suggestion. I fell back on reverting to old defaults when no vert/fold fillchar is specified. I believe the behavior is not perfect but reasonable: When ambiwdith=double, users who don't set fillchars won't benefit from the new defaults (the only problem is if they try another neovim on a server for instance, they might wonder why with a same neovim version they don't see the same fillchar defaults), if they set vert or fold, the usual behavior applies. Users with ambiwidth=single will see the new defaults and nothing else changes.
The TUI dictates the use of monocell characters (for now).

Most fonts should have these by now. Both are a significant visual improvement. - Vertical connecting bar `│` is used by tmux, pstree, Windows 7 cmd.exe and nvim-qt.exe. - Middle dot `·` works on Windows 7 cmd.exe, nvim-qt.exe.

removed the default set by options.lua for fillchars because nvim won't know if it's a user setting or not.

justinmk · 2018-02-20T22:28:35Z

Also for make oldtest:

diff --git a/src/nvim/testdir/setup.vim b/src/nvim/testdir/setup.vim
index 87cf1f61637c..7d6dd0c7ce77 100644
--- a/src/nvim/testdir/setup.vim
+++ b/src/nvim/testdir/setup.vim
@@ -6,6 +6,7 @@ set directory^=.
 set backspace=
 set nohidden smarttab noautoindent noautoread complete-=i noruler noshowcmd
 set listchars=eol:$
+set fillchars=vert:\|,fold:-
 " Prevent Nvim log from writing to stderr.
 let $NVIM_LOG_FILE = exists($NVIM_LOG_FILE) ? $NVIM_LOG_FILE : 'Xnvim.log'

justinmk · 2018-02-20T22:48:49Z

src/nvim/option.c

        lcs_tab1 = NUL;
-      else
-        fill_diff = '-';


~~was this removed intentionally?~~ I see it's handled by tab[i].def now.

justinmk · 2018-02-20T23:09:27Z

src/nvim/option.c

+  static struct charstab filltab[] = {
+    { &fill_stl,                  "stl"  , ' '  },
+    { &fill_stlnc,                "stlnc", ' '  },
+    { &fill_vert,                 "vert" , 9474 },  // '|'


should the comment be │ (not |)

Update tests.

justinmk · 2018-02-20T23:39:33Z

see #8035 , I removed some of the micro-management, we don't need to care about whether the user set vert: or whatever, just compare the known default value.

In either case, user can't :set ambiwidth=double without first undoing the new defaults. Need to figure that out before we can merge to master.

teto · 2018-02-21T02:41:00Z

That's why I had removed from options.lua the defaults={if_true={vi="vert:│,fold:·"}}, it then made setting ambiwidth to double painless until the user had himself set fillchars and thus would likely know how to fix the problem.

justinmk · 2018-02-21T06:11:32Z

Hmm, when I tried this PR (unmodified) that didn't work.

Update tests.

justinmk · 2018-02-22T23:08:00Z

runtime/doc/options.txt

@@ -2377,7 +2377,7 @@ A jump table for the options with a short description can be found at |Q_op|.
 	Only normal file name characters can be used, "/\*?[|<>" are illegal.

 						*'fillchars'* *'fcs'*
-'fillchars' 'fcs'	string	(default "vert:|,fold:-")
+'fillchars' 'fcs'	string	(default "vert:│,fold:·")


should be empty, I think. Will fix when merging.

justinmk · 2018-02-22T23:15:00Z

src/nvim/options.lua

@@ -812,7 +812,7 @@ return {
      vi_def=true,
      redraw={'all_windows'},
      varname='p_fcs',
-      defaults={if_true={vi="vert:|,fold:-"}}
+      defaults={if_true={vi=""}}


I see now why this was necessary. Also added a test in #8035

sry I wanted to test again to make sure I was not inducing you into errors but had this screen pb and not enough time to fix it. I am glad you figured it out it's a cool improvement

Update tests.

justinmk · 2018-02-22T23:52:08Z

Merged in #8035, thanks for following up on this !

mhinz · 2018-02-23T13:17:10Z

It was intentional to assume implicit defaults even when 'fillchars' doesn't list them? Why the magic instead of just changing default values?

teto · 2018-02-23T21:45:47Z

there are 2 levels of defaults, the hardcoded C

 static int fillchar_vsep(win_T *wp, int *attr)
 {
   *attr = win_hl_attr(wp, HLF_C);
-  if (*attr == 0 && fill_vert == ' ') {
-    return '|';
-  } else {
-    return fill_vert;
-  }

and the one from options.lua that basically call set fillchars=@defaults@. I wanted users that have set ambiwidth=double in their init.vim to not see an error when they updated neovim (which would have been triggered by the set fillchars=@defaults@ from options.lua). And as we can not distinguish between the set fillchars from options.lua from a user one, it seemed better to change the hardcoded C defaults.

justinmk · 2018-02-23T22:08:18Z

Conclusion from our discussion in chat: current behavior is fine and matches the documentation at :help 'fillchars':

'fillchars' 'fcs'       string  (default "")
...
          item          default         Used for
          stl:c         ' ' or '^'      statusline of the current window
          stlnc:c       ' ' or '='      statusline of the non-current windows
          vert:c        '│' or '|'      vertical separators :vsplit
          fold:c        '·' or '-'      filling 'foldtext'
          diff:c        '-'             deleted lines of the 'diff' option

 Any one that is omitted **will fall back to the default.**

The behavior is inconsistent with 'listchars', but consistent with other Vim flags-style options which fallback to internal defaults if a flag is not explicitly mentioned.

teto changed the title ~~change defaults for fillchars 'vert' and 'fold'~~ [WIP] change defaults for fillchars 'vert' and 'fold' Feb 8, 2018

marvim added the WIP label Feb 8, 2018

justinmk reviewed Feb 10, 2018

View reviewed changes

teto force-pushed the fillchars branch 3 times, most recently from 0365a38 to 3de742e Compare February 12, 2018 07:50

justinmk and others added 3 commits February 13, 2018 00:13

defaults: 'fillchars'

2d1476f

Most fonts should have these by now. Both are a significant visual improvement. - Vertical connecting bar `│` is used by tmux, pstree, Windows 7 cmd.exe and nvim-qt.exe. - Middle dot `·` works on Windows 7 cmd.exe, nvim-qt.exe.

added fillchars

4d41bba

Fallback iff no fillchar value set for vert/fold

7f12ec4

removed the default set by options.lua for fillchars because nvim won't know if it's a user setting or not.

teto changed the title ~~[WIP] change defaults for fillchars 'vert' and 'fold'~~ [RFC] change defaults for fillchars 'vert' and 'fold' Feb 12, 2018

fixes

3de742e

marvim added RFC and removed WIP labels Feb 12, 2018

justinmk reviewed Feb 20, 2018

View reviewed changes

justinmk pushed a commit to justinmk/neovim that referenced this pull request Feb 20, 2018

'fillchars': update defaults logic, handle ambiwidth=double neovim#7986

81460fb

Update tests.

justinmk mentioned this pull request Feb 20, 2018

defaults: 'fillchars' #8035

Merged

justinmk pushed a commit to justinmk/neovim that referenced this pull request Feb 22, 2018

'fillchars': update defaults logic, handle ambiwidth=double neovim#7986

bd183a5

Update tests.

justinmk mentioned this pull request Feb 22, 2018

defaults, part 2 #6289

Closed

20 tasks

justinmk reviewed Feb 22, 2018

View reviewed changes

justinmk pushed a commit to justinmk/neovim that referenced this pull request Feb 22, 2018

'fillchars': fix defaults logic; handle ambiwidth=double neovim#7986

384a394

Update tests.

justinmk closed this Feb 22, 2018

justinmk removed the RFC label Feb 22, 2018

teto deleted the fillchars branch February 23, 2018 02:48

ya2s mentioned this pull request Jan 27, 2021

change vertsplit bg color. Th3Whit3Wolf/one-nvim#4

Merged

justinmk changed the title ~~[RFC] change defaults for fillchars 'vert' and 'fold'~~ change defaults for fillchars 'vert' and 'fold' Aug 16, 2021

Uh oh!

change defaults for fillchars 'vert' and 'fold' #7986

change defaults for fillchars 'vert' and 'fold' #7986

Uh oh!

Conversation

teto commented Feb 8, 2018

Uh oh!

teto commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmk commented Feb 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

teto commented Feb 9, 2018

Uh oh!

justinmk commented Feb 9, 2018

Uh oh!

justinmk Feb 10, 2018

Choose a reason for hiding this comment

Uh oh!

teto Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk commented Feb 20, 2018

Uh oh!

justinmk Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teto Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

justinmk Feb 20, 2018

Choose a reason for hiding this comment

Uh oh!

teto Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

justinmk commented Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

teto commented Feb 21, 2018

Uh oh!

justinmk commented Feb 21, 2018

Uh oh!

justinmk Feb 22, 2018

Choose a reason for hiding this comment

Uh oh!

justinmk Feb 22, 2018

Choose a reason for hiding this comment

Uh oh!

teto Feb 23, 2018

Choose a reason for hiding this comment

Uh oh!

justinmk commented Feb 22, 2018

Uh oh!

mhinz commented Feb 23, 2018

Uh oh!

teto commented Feb 23, 2018

Uh oh!

justinmk commented Feb 23, 2018

Uh oh!

Uh oh!

teto commented Feb 9, 2018 •

edited

Loading

justinmk commented Feb 9, 2018 •

edited

Loading

teto Feb 12, 2018 •

edited

Loading

justinmk Feb 20, 2018 •

edited

Loading

justinmk commented Feb 20, 2018 •

edited

Loading