xtermjs / xterm-addon-ligatures

An xterm.js addon that provides font ligature support
MIT License
14 stars 4 forks source link

Exception when integrating into VS Code #16

Closed Tyriar closed 6 years ago

Tyriar commented 6 years ago

Using xterm-addon-ligatures@0.1.0-beta-2

screen shot 2018-08-10 at 8 27 00 pm
Tyriar commented 6 years ago
screen shot 2018-08-10 at 8 43 41 pm
princjef commented 6 years ago

Interesting. I had pulled back on some of the error swallowing down at those levels which is probably what's causing this to pop up. I'm betting there's a font installed on your system that it doesn't like.

It's maps back here, which only throws if the font has no OS/2 table, which is required per the OpenType spec:

https://github.com/princjef/font-finder/blob/56f21b22a1524cabcec99fd009c414d2887ac9e9/src/tables/os2.ts#L51

princjef commented 6 years ago

There are probably lots of little issues like this that will crop up when fonts are structured a little bit differently than expected/specified. I'm not sure of a good way to guarantee safety without just ignoring fonts that error out for any reason. Changing that would cause the stream.destroy() error from before to get swallowed again.

princjef commented 6 years ago

@Tyriar if you can drop a breakpoint in the js output for this line you should be able to at least see what file is causing the problem

princjef commented 6 years ago

Switched over to mac and I'm seeing it now. Looking into it

princjef commented 6 years ago

Yep there seem to be a few fonts in there that don't have any OS/2 table. I'll have to dig into why and what to do in that case, but for now I'll just have it skip those fonts because they're failing anyway. It'll take a couple publishes to plumb it but I'll have a PR soon.

An example of one of the fonts on my mac that triggers it: /Library/Fonts/AppleGothic.ttf. It only has these tables:

princjef commented 6 years ago

FYI currently looking into how to surface the correct information for these fonts via https://github.com/princjef/font-finder/issues/5. Once I have it I'll plumb it through separately.

These mac-specific fonts aren't a huge concern for this package because to my knowledge there aren't any platform-specific fonts supporting programming ligatures at the moment.

Tyriar commented 6 years ago

@princjef this seemed to be happening on all fonts on macOS (though it worked fine in the past). I tried "Hack" (no ligatures), "Hasklig" and "Fira Code"

princjef commented 6 years ago

@Tyriar yeah that's because to find the font that's being used I have to scan all of the system fonts until I find it. The font family name and the file name don't necessarily have to be correlated, so I have to parse parts of the font file to extract the name from it.

The issue popped up when I stopped swallowing certain types font loading errors. I had previously tested the font loading on mac, but hadn't noticed that a couple of the fonts were being skipped due to missing OS/2 tables. I had tested the newer code without exception swallowing on Windows and Linux (via WSL), but Mac has some different font rules it follows that caused the issue.

The deeper problem is that passing up errors opens up a certain level of risk to stability. Because I have to load all fonts to find the one being used, any font that is sufficiently malformed might cause the entire call to fail.

princjef commented 6 years ago

I see two approaches to this:

  1. Try to add more hardening code to font-finder to handle specific problems with font files
  2. Add an option to ignore errors when loading certain fonts. The files could potentially be surfaced off to the side for debugging purposes or a debug param could be plumbed through so people can figure out why their font isn't working.