Initial implementation by princjef · Pull Request #1 · xtermjs/xterm-addon-ligatures

princjef · 2018-07-11T16:09:01Z

Initial implementation of the xterm-ligature-support addon. This addresses part 2 of the solution for xtermjs/xterm.js#958 (along with xtermjs/xterm.js#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

I'll try find time soon to have a deep look at this, thanks for all the hard work @princjef 😃

Also not sure if you've been following this but we're planning on formalizing how addons are built which should make everything nicer. Would love any feedback on this issue if you have any since you've built an addon in the current style xtermjs/xterm.js#1128

Tyriar · 2018-07-11T20:57:08Z

+  before(() => {
+    sinon.stub(fontFinder, 'list').returns(Promise.resolve({
+      'Fira Code': [{
+        path: path.join(__dirname, '../fonts/firaCode.otf'),


There might be some license issue with including these in the repo, it might also be a good idea to download them as part of npm install?

Both are licensed OFL, which is the MIT of the font world. I'll add a note about the fonts with links to their licenses at the bottom of the readme. Fetching them in a postinstall step is a good idea either way, so I'll add that too.

Tyriar · 2018-07-18T18:35:31Z

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

princjef · 2018-07-19T05:17:24Z

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 · 2018-07-19T06:28:03Z

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

Tyriar · 2018-07-20T18:59:33Z

😮

Tyriar · 2018-07-21T01:53:02Z

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

Tyriar · 2018-07-20T19:20:41Z

+          range => [range[0], range[1]]
+        );
+      } catch {
+        return [];


I tried to pull this into VS Code by copying over node modules and this catch make it fail silently where it would be better to fail hard, I think the actual error is that the modules were compiled on a different version of Electron to VS Codes.

makes sense. i can play around with it to return empty results for error cases that are normal (such as font not found) and throw actual errors for unexpected ones)

Tyriar · 2018-07-20T19:25:06Z

+    "terminal"
+  ],
+  "license": "MIT",
+  "dependencies": {


There are quite a lot of dependencies across these 3 packages, any ones functions we can easily inline would be good.

└─┬ xterm-ligature-support@1.0.0 ├─┬ font-finder@1.0.1 │ ├─┬ get-system-fonts@1.0.0 │ │ └── fast-glob@2.2.2 deduped │ └── promise-stream-reader@1.0.1 ├─┬ font-ligatures@1.3.0 │ ├─┬ debug@3.1.0 │ │ └── ms@2.0.0 deduped │ ├── font-finder@1.0.1 deduped │ ├── lodash.clonedeep@4.5.0 │ ├─┬ lru-cache@4.1.3 │ │ ├── pseudomap@1.0.2 │ │ └── yallist@2.1.2 │ └─┬ opentype.js@0.8.0 │ └── tiny-inflate@1.0.2 └── postcss-value-parser@3.3.0

I'm not sure we'd want to ship this many with VS Code as each one adds overhead (size, loaded code, etc.), also I haven't looked over their licenses yet.

We have our own LRU map which could maybe be shared somehow (not sure on what the best way to do this is just yet) https://github.com/xtermjs/xterm.js/blob/8f22d8dda4f282e6c10943576ed5797317ad626f/src/renderer/atlas/LRUMap.ts

From a dependency perspective, I'd bucket them into the following categories:

Core functionality written for this package (hard to remove):

font-finder

font-ligatures

promise-stream-reader

get-system-fonts

Important to how the library works but parts may be inlineable:

lru-cache

opentype.js

postcss-value-parser

lodash.clonedeep

Unnecessary fluff:

debug

fast-glob

What are the primary considerations w.r.t. dependencies? count, size, or depth? I wrote all of the packages in the first bucket so i can go over them with a fine-tooth comb to make sure nothing unnecessary creeps into the installed version (there's not a ton of code in any of them). The main one that adds a lot of weight is opentype.js, though a decent chunk of it is needed for handling the font file

What are the primary considerations w.r.t. dependencies? count, size, or depth?

All of the above, count/depth increases complexity and the likely hood of security issues, size increases the payload (it would be good to get opentype.js down some but luckily it's mostly JS so should be fairly small gzipped, not sure if it's worth the effort there).

Given your categorization I'd say we should look into the following:

Think about sharing xterm.js' LRU map in the future (I think we were planning on simplifying this a lot when we drop some browser support)

Inline postcss-value-parser, lodash.clonedeep

Remove debug, fast-glob

Tyriar · 2018-07-21T02:15:01Z

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 · 2018-07-21T02:19:51Z

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 · 2018-07-21T02:33:13Z

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 · 2018-07-21T02:42:31Z

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 · 2018-07-21T02:47:59Z

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

Tyriar · 2018-07-21T03:01:24Z

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 · 2018-07-21T03:09:04Z

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 · 2018-07-21T03:11:13Z

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

princjef · 2018-07-21T03:14:33Z

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:

princjef · 2018-07-21T03:17:04Z

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 · 2018-07-21T03:17:23Z

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 · 2018-07-21T03:18:22Z

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

princjef · 2018-07-21T03:20:35Z

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

princjef · 2018-07-21T05:04:19Z

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

Tyriar · 2018-07-21T17:30:57Z

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

princjef · 2018-07-24T02:43:47Z

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

princjef · 2018-08-07T04:49:08Z

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.

Tyriar

Let's do this! We can make iterative improvements on it going forward and pushing this will help unblock you.

Tyriar reviewed Jul 11, 2018

View reviewed changes

princjef force-pushed the initial branch 2 times, most recently from f23d2bc to 6bb1eb3 Compare July 12, 2018 15:13

Tyriar reviewed Jul 21, 2018

View reviewed changes

egamma mentioned this pull request Jul 23, 2018

Iteration Plan for July 2018 microsoft/vscode#53850

Closed

49 tasks

princjef force-pushed the initial branch from 6bb1eb3 to b1817a9 Compare July 24, 2018 02:38

kieferrm mentioned this pull request Aug 7, 2018

Iteration Plan for August 2018 microsoft/vscode#55927

Closed

39 tasks

Initial implementation

e01ccdc

princjef force-pushed the initial branch from b1817a9 to e01ccdc Compare August 7, 2018 04:36

Tyriar approved these changes Aug 8, 2018

View reviewed changes

Tyriar merged commit 8a0ed83 into master Aug 8, 2018

Tyriar deleted the initial branch August 8, 2018 21:35

Conversation

princjef commented Jul 11, 2018

Uh oh!

Tyriar left a comment

Choose a reason for hiding this comment

Uh oh!

Tyriar Jul 11, 2018

Choose a reason for hiding this comment

Uh oh!

princjef Jul 12, 2018

Choose a reason for hiding this comment

Uh oh!

Tyriar commented Jul 18, 2018

Uh oh!

princjef commented Jul 19, 2018

Uh oh!

princjef commented Jul 19, 2018

Uh oh!

Tyriar commented Jul 20, 2018

Uh oh!

Tyriar commented Jul 21, 2018

Uh oh!

Tyriar Jul 20, 2018

Choose a reason for hiding this comment

Uh oh!

princjef Jul 21, 2018

Choose a reason for hiding this comment

Uh oh!

Tyriar Jul 20, 2018

Choose a reason for hiding this comment

Uh oh!

princjef Jul 21, 2018

Choose a reason for hiding this comment

Uh oh!

Tyriar Jul 21, 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!

Tyriar commented Jul 21, 2018

Uh oh!

Tyriar commented Jul 21, 2018

Uh oh!

Tyriar commented Jul 21, 2018

Uh oh!

Tyriar commented Jul 21, 2018

Uh oh!

princjef commented Jul 21, 2018

Uh oh!

Tyriar commented Jul 21, 2018

Uh oh!

princjef commented Jul 21, 2018

Uh oh!

Tyriar commented Jul 21, 2018

Uh oh!

princjef commented Jul 21, 2018

Uh oh!

princjef commented Jul 21, 2018

Uh oh!

Tyriar commented Jul 21, 2018

Uh oh!

Tyriar commented Jul 21, 2018

Uh oh!

princjef commented Jul 21, 2018

Uh oh!

princjef commented Jul 21, 2018

Uh oh!

Tyriar commented Jul 21, 2018

Uh oh!

princjef commented Jul 24, 2018

Uh oh!

princjef commented Aug 7, 2018

Uh oh!

Tyriar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Tyriar Jul 21, 2018 •

edited

Loading