xtermjs / xterm-addon-ligatures

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

Initial implementation #1

Closed princjef closed 6 years ago

princjef commented 6 years ago

Initial implementation of the xterm-ligature-support addon. This addresses part 2 of the solution for https://github.com/xtermjs/xterm.js/issues/958 (along with https://github.com/xtermjs/xterm.js/pull/1460). It handles font loading and resolution, wrapping the font in a ligature resolver, which is used for each render call after the font is loaded.

cc: @Tyriar

Tyriar commented 6 years ago

@princjef how did you go about testing while developing this? Did you have a barebones Electron app that you used as the host?

princjef commented 6 years ago

Yeah I have an Electron app on my box with some other stuff where I've been throwing packed versions of both packages. I'll see if I can whip up a more barebones one so you can play around (and as a possible example for the repo)

princjef commented 6 years ago

Here's a version you can play with: https://github.com/princjef/xterm-electron-sample

Tyriar commented 6 years ago

😮

screen shot 2018-07-20 at 11 56 39 am
Tyriar commented 6 years ago

When I run vim in the sample it breaks, @princjef did you test an app that enter application mode? (It just could be me?)

screen shot 2018-07-20 at 6 51 11 pm
Tyriar commented 6 years ago

I'm getting this error when attempting to pull in the package via git to vscode:

❯ yarn add https://github.com/xtermjs/xterm-ligature-support.git#initial
yarn add v1.3.2
$ node build/npm/preinstall.js
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[1/5] 🔍  Validating package.json...
[2/5] 🔍  Resolving packages...
[3/5] 🚚  Fetching packages...
error An unexpected error occurred: "https://registry.yarnpkg.com/opentype.js/-/opentype.js-0.8.0.tgz: ENOENT: no such file or directory, utime'/Users/daimms/Library/Caches/Yarn/v1/npm-opentype.js-0.8.0-acabcfa1642fbe894a3e4d759e43ba694e02bd35/dist/opentype.js.map'".
info If you think this is a bug, please open a bug report with the information provided in "/Users/daimms/dev/Microsoft/vscode/yarn-error.log".
info Visit https://yarnpkg.com/en/docs/cli/add for documentation about this command.
Tyriar commented 6 years ago

This seemed to work though 😕

yarn add file:///Users/daimms/playground/xterm-electron-sample/local_node_modules/xterm-ligature-support-1.0.0.tgz
Tyriar commented 6 years ago

When trying to include in VS Code I get issues where font-finder successfully getting the list of font files, but parsing the fonts fails such that xterm-ligature-support gets an empty list of fonts back. Still investigating.

Tyriar commented 6 years ago

This is the exception that's causing the issues:

TypeError: pStream.destroy is not a function
    at parse.ts:109
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:109)

https://github.com/princjef/font-finder/blob/78e28c0f2176fa2621d7768a1b0e8172ee2e5ca5/src/parse.ts#L101

promise-stream-reader@1.0.1 is in node_modules

princjef commented 6 years ago

what version of node are you running on? stream destroy was added in node 8

Tyriar commented 6 years ago

Then I hit "Could not find font" due to fontLigatures.loadFile failing, the trace:

TypeError: util.promisify is not a function
    at Object.loadFile (index.ts:276)
    at Object.load [as default] (font.ts:23)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:109)

These setup problems go away when I run VS Code under Electron 2 (which I just merged into master).

princjef commented 6 years ago

yeah there's a requirement on node 8 right now, which came in sometime in the electron 1.8.x line. it's theoretically possible to get it down to 6, but the requirement comes from the promise-stream lib all the way at the bottom of the stack

Tyriar commented 6 years ago

Hmm, it looks like they're being drawn together but the ligatures aren't working 😕

screen shot 2018-07-20 at 8 09 45 pm
princjef commented 6 years ago

i use neovim frequently and it's working (though i'm noticing some clipping of italicized characters). i'll take a look with vim in the sample:

image

princjef commented 6 years ago

Odd that it's not rendering together. My only guess for that is that the canvas isn't detecting Fira Code. the r of fira code is pretty distinctive so that's how i usually visually confirm that the right font is being rendered

Tyriar commented 6 years ago

Got it working! Needed to remove the various things turning ligatures off from the past setting:

font-variant-ligatures: none;
font-feature-settings: "liga" 0;
Tyriar commented 6 years ago

I'm a little surprised that CSS ligature settings impact text in a canvas, but whatever 🎉

screen shot 2018-07-20 at 8 18 13 pm
princjef commented 6 years ago

i guess it saves them having to define a canvas API instead. definitely good to be aware of

princjef commented 6 years ago

Tried out vim on mac with the sample app and seems to be working alright:

image

Tyriar commented 6 years ago

@princjef vim appears to work fine when I got it working in VS Code

princjef commented 6 years ago

Updated with more useful error handling. I'll start looking at the dependencies next

princjef commented 6 years ago

Dependencies updated. Latest npm ls --production:

xterm-ligature-support@1.0.0
├─┬ font-finder@1.0.2
│ ├── get-system-fonts@2.0.0
│ └── promise-stream-reader@1.0.1
└─┬ font-ligatures@1.3.1
  ├── font-finder@1.0.2 deduped
  ├─┬ lru-cache@4.1.3
  │ ├── pseudomap@1.0.2
  │ └── yallist@2.1.2
  └─┬ opentype.js@0.8.0
    └── tiny-inflate@1.0.2

I also double checked and all remaining direct/indirect standard dependencies are MIT/ISC licensed so should be good there.