macvim-dev / macvim

Vim - the text editor - for macOS
https://macvim.org
Vim License
7.49k stars 681 forks source link

Line Height Issue on latest master #977

Closed amadeus closed 4 years ago

amadeus commented 4 years ago

Describe the bug I noticed that when building the latest master of macvim, font line-heights get weirdly messed up. I am on macOS Mojave - 10.14.6

image

Normally I run off the stateful-render branch, but was trying to rebase master into it when I came across the bug. So I tried building master manually to see if it reproed, and it did indeed.

Edit Notes: I build via github, via this lil custom build script:

#!/bin/bash
make clean
make distclean
./configure --with-features=huge --enable-terminal --enable-rubyinterp --enable-python3interp --enable-perlinterp --enable-cscope --enable-rubyinterp --with-ruby-command=/usr/bin/ruby
make
rm src/MacVim/build/Release/MacVim.app/Contents/Resources/MacVim.icns
cp ./MacVim.icns src/MacVim/build/Release/MacVim.app/Contents/Resources/
open src/MacVim/build/Release

Output from reading org.vim.MacVim:

    MMAutosaveColumns = 142;
    MMAutosaveRows = 63;
    MMCurrentPreferencePane = Advanced;
    MMFullScreenFadeTime = 0;
    MMNativeFullScreen = 0;
    MMNoTitleBarWindow = 0;
    MMRenderer = 1;
    MMShowAddTabButton = 0;
    MMTitlebarAppearsTransparent = 1;
    MMTopLeftPoint = "{1279, 1417}";
    SUEnableAutomaticChecks = 0;
    SUHasLaunchedBefore = 1;
    SULastCheckTime = "2019-01-28 18:28:47 +0000";
    SUSendProfileInfo = 0;
eirnym commented 4 years ago

@amadeus thank you for an issue

You missed a few important steps of creating bug-report, so please, fill them out.

Could you tell us more about your system?

ychin commented 4 years ago

What font are you using? There was some line height issue that I think was fixed in 159 so curious if it’s related.

amadeus commented 4 years ago

@ychin my guifont settings are:

  guifont=InputMono-Regular:h15

https://input.fontbureau.com/

amadeus commented 4 years ago

@eirnym I edited my original post with the missing things you requested, however I don't think there's any way this is plugin related, unless a plugin is editing linespace which it's not - since checking the value yields 0, which my older working build had as well.

@ychin I can also confirm downloading the latest release from the releases tab in github also replicates this bug with my font settings, so could be related. Also I don't think stateful-render PR branch has 159 updates yet either, so that seems like a really good candidate.

eirnym commented 4 years ago

@ychin it seems to be same as with Menlo. Spacing between letters are higher. Fira Code Light have the same issues, but less visible

ychin commented 4 years ago

@amadeus Looking at the commits, I think the stateful-render branch actually contains a temporary bug that was introduced and fixed in between snapshot-157 and snapshot-158 (see #928). (Edit: Interestingly #928 was also complaining about the InputMono font)

Now, it's arguable whether it's a bug or not. Basically MacVim has never respected the font's line heights (try snapshot 157/158 if you don't believe me) and there was a PR which started reading font's line heights, which while sort of correct, introduced a behavior that's different from before. I fixed the issue before releasing 158 to keep it consistent with before.

If we want to support line heights, we could, but it would need to be a new option to keep it consistent with years of existing behavior.

@amadeus feel free to try older versions to see if they behave similar to snapshot-159/160.

amadeus commented 4 years ago

Interesting! I’ll experiment more tomorrow.

Out of curiosity, how do we calculate line height now? Is it simply the font size?

Ideally you guys could expose a guilineheight option to override. It looks like I could customize Input Mono to download it with a 1x line-height - but the font ends up looking VERY tight that way, so the breathing room in stateful-renderer (which i believe is using a 1.2x line height) is good.

ychin commented 4 years ago

Out of curiosity, how do we calculate line height now? Is it simply the font size?

I believe that's the case but I actually haven't looked closely enough. I just fixed it up to make sure we didn't have a regression or incompatible change but I didn't have time to dive into the specifics of how the line height is calculated now.

Ideally you guys could expose a guilineheight option to override. It looks like I could customize Input Mono to download it with a 1x line-height - but the font ends up looking VERY tight that way, so the breathing room in stateful-renderer (which i believe is using a 1.2x line height) is good.

Yeah I agree we could expose an option. It's just that in true Vim fashion we should keep the old behavior which has been around for a while the default.

amadeus commented 4 years ago

Maybe we can convince @s4y to make it part of the stateful-render branch 😏

If I was to spec the feature, I would say - guilineheight should default to whatever it does now (for backwards compatibility), but then it could also take a floating point value to override the existing behavior which would then be a multiplier for the existing lineheight. So if the default is 1, then I could specify 1.2 which would be 1.2 * guifontsize == 1.2 * 15 == 18

ychin commented 4 years ago

Maybe we can convince @s4y to make it part of the stateful-render branch 😏

No please don't. Let's just try to merge that first before tacking on small features. I need to review the latest changes in that PR and hopefully this will get in soon.

If I was to spec the feature, I would say - guilineheight should default to whatever it does now (for backwards compatibility), but then it could also take a floating point value to override the existing behavior which would then be a multiplier for the existing lineheight. So if the default is 1, then I could specify 1.2 which would be 1.2 guifontsize == 1.2 15 == 18

Can't you just use linespace for that? I would imagine we just need some way to tell MacVim to respect the font's native line height. It could either be set as part of guifont, a new Vim settings, or just a preference set in the GUI preference panel.

amadeus commented 4 years ago

Fair enough - I always assumed linespace was to create a gap between lines, not actually change the line height.

gruvin commented 4 years ago

@ychin wrote

If we want to support line heights, we could, but it would need to be a new option to keep it consistent with years of existing behavior.

I will never understand this line of thinking. It's an editor that has always been broken. Fix it. It's not a legacy bank transaction system. It's an editor. No one is going to lose anything if their editor suddenly starts behaving more rationally. Just fix it, for goodness sake. Why not?

Please?

gruvin commented 4 years ago

Wish I knew enough to help more practically. Here's some more info that may help.

This bug never used to happen -- like for years, until a couple or three months ago or however long it's been.

Hopefully the following will at least show a way to reliably duplicate the issue on the dev's system.

MacVim's start-up behaviour is different from any other time a font size or family is changed. At start-up, the lines are too short and font is clipped. After making any change to font however, the issues corrects itself. It's so easy to fix / work around (see below) that I just figured the bug would "go away of its own accord." (Wouldn't that be cool? :p) However, it's been months now. So, I guess the bug's presentation is not as prevalent as I have assumed.

Examples ...

% mvim --clean
:option<Cr>

screen snippet

Screen Shot 2020-05-03 at 2 31 02 PM

Increase font size +1 then immediately -1 (or any change in font, by any manner)

<cmd +><cmd ->

new screen snippet -- not that lines are "correct" (more appropriate) height now.

Screen Shot 2020-05-03 at 2 31 23 PM

Plugins were disabled for this data. Confirming evidence; I usually work on a dark background.

iMac 27" macOS 10.15.4 (19E287) Bug also present and identical on MacBook Pro, fresh install, 10.15.4 (19E287)

Screen Shot 2020-05-03 at 2 45 53 PM
:version
VIM - Vi IMproved 8.1 (2018 May 18, compiled Feb  4 2019 08:13:44)
macOS version
Included patches: 1-873
Compiled by travis@Traviss-Mac-6.local
Huge version with MacVim GUI.  Features included (+) or not (-):
+acl               +cmdline_info      +farsi             +langmap           +mouse_sgr         +profile           -tag_any_white     +visual
+arabic            +comments          +file_in_path      +libcall           -mouse_sysmouse    +python/dyn        -tcl               +visualextra
+autocmd           +conceal           +find_in_path      +linebreak         +mouse_urxvt       +python3/dyn       +termguicolors     +viminfo
+autochdir         +cryptv            +float             +lispindent        +mouse_xterm       +quickfix          +terminal          +vreplace
-autoservername    +cscope            +folding           +listcmds          +multi_byte        +reltime           +terminfo          +wildignore
+balloon_eval      +cursorbind        -footer            +localmap          +multi_lang        +rightleft         +termresponse      +wildmenu
+balloon_eval_term +cursorshape       +fork()            +lua/dyn           -mzscheme          +ruby/dyn          +textobjects       +windows
+browse            +dialog_con_gui    +fullscreen        +menu              +netbeans_intg     +scrollbind        +textprop          +writebackup
++builtin_terms    +diff              -gettext           +mksession         +num64             +signs             +timers            -X11
+byte_offset       +digraphs          -hangul_input      +modify_fname      +odbeditor         +smartindent       +title             -xfontset
+channel           +dnd               +iconv             +mouse             +packages          +startuptime       +toolbar           +xim
+cindent           -ebcdic            +insert_expand     +mouseshape        +path_extra        +statusline        +transparency      -xpm
+clientserver      +emacs_tags        +job               +mouse_dec         +perl/dyn          -sun_workshop      +user_commands     -xsmp
+clipboard         +eval              +jumplist          -mouse_gpm         +persistent_undo   +syntax            +vartabs           -xterm_clipboard
+cmdline_compl     +ex_extra          +keymap            -mouse_jsbterm     +postscript        +tag_binary        +vertsplit         -xterm_save
+cmdline_hist      +extra_search      +lambda            +mouse_netterm     +printer           +tag_old_static    +virtualedit
   system vimrc file: "$VIM/vimrc"
     user vimrc file: "$HOME/.vimrc"
 2nd user vimrc file: "~/.vim/vimrc"
      user exrc file: "$HOME/.exrc"
  system gvimrc file: "$VIM/gvimrc"
    user gvimrc file: "$HOME/.gvimrc"
2nd user gvimrc file: "~/.vim/gvimrc"
       defaults file: "$VIMRUNTIME/defaults.vim"
    system menu file: "$VIMRUNTIME/menu.vim"
  fall-back for $VIM: "/Applications/MacVim.app/Contents/Resources/vim"
Compilation: clang -c -I. -Iproto -DHAVE_CONFIG_H -DFEAT_GUI_MACVIM -Wall -Wno-unknown-pragmas -pipe  -DMACOS_X -DMACOS_X_DARWIN  -g -O2 -U_FORTIFY_SOURCE -D_FORTIFY_
SOURCE=1
Linking: clang   -L. -fstack-protector-strong -L/usr/local/lib -L/usr/local/opt/libyaml/lib -L/usr/local/opt/openssl/lib -L/usr/local/opt/readline/lib -L. -fstack-pro
tector-strong -L/usr/local/lib -L/usr/local/opt/libyaml/lib -L/usr/local/opt/openssl/lib -L/usr/local/opt/readline/lib  -L/usr/local/lib -o Vim -framework Cocoa -fram
ework Carbon       -lm  -lncurses -liconv -framework AppKit   -fstack-protector  -L/System/Library/Perl/5.18/darwin-thread-multi-2level/CORE 
s4y commented 4 years ago

@gruvin Does this happen to you in the stateful-render branch?

gruvin commented 4 years ago

@s4y I'm not set up to compile the code myself, sorry

gruvin commented 4 years ago

This issue appears to have been fixed in MacVim Version 8.2.539 (163) which came through the automatic updatimathing, for me at around 2020-05-05 10:00 GMT.

Thanks. :-)