Syntax highlighted diffs

[niik]

Wouldn't it be nice with some syntax highlighting in your diffs?

The Problem

Syntax highlighting is a well-understood problem with tons of options. Atom uses TextMate grammars to do theirs but since we're already using CodeMirror I took a stab at implementing ours using that.

Syntax highlighted diffs have been a much appreciated feature of GitHub.com for a long time now and one that I have missed in GitHub Desktop for a long time. Highlighting in diffs presents some added complexity over that of highlighting in a normal source file though. Pretty much all languages are contextual, in that what happened on some line "higher up" affects what's going on further down. As such you can't just pull out a line from a diff and expect it to be highlighted properly. Here's a good example

Had we just tried to highlight individual lines here we wouldn't have been able to infer that the first line was part of a multi-line comment.

Instead, we have to take the contents of the file before the change, and the contents of it after and run highlighting on both versions. Once that's done we can stitch these together to form one syntax highlighted diff.

The Approach

When we are about to perform highlighting on a diff we start out by scanning through the diff to figure out which lines we need from which file. Context lines can be pulled from either version while added/removed lines obviously need to come from a particular version. If we find that a file consists entirely of additions or entirely of deletions we can optimize further by adding a preference for one of the versions and thus getting away with loading just one file.

Once we've got that settled we load the first 256kb from both versions (256kb picked arbitrarily because I figured it should cover the majority of source files while adding a very manageable memory overhead for the feature). We then pass this content, along with which lines we want to get tokens for to one or two web workers which then run the modes. CodeMirror modes are synchronous but running them in a web worker means we can get on with other things while we're tokenizing up to half a megabyte of content in up to two different threads (threads in javascript, what have we come to). It also means that we have a real nice containment of the highlighting process and that we can terminate it should it for some reason end up taking a very long time to complete.

When we get the results from the workers we apply our own custom CodeMirror mode which takes the tokens from the language modes and applies them inside of our diff. That means that there's a small window in between when users see the diff and when highlighting gets applied. In my testing it's barely noticeable and it means we can deliver what really matters (the diff) as quickly as we've done before.

The Performance.

So far I've been blown away by how well this performs, even on my 2015 12" macbook it's real snappy. We do a couple of optimizations behind the scenes and we keep web workers alive once we've fired them up to avoid the penalty of parsing the javascript of all the modes.

Languages currently supported

We can obviously support every mode that CodeMirror supports but for now I've opted to include these until we get a sense of how well this is working.

JavaScript, JSON, TypeScript, HTML, Markdown, Yaml, XML, Objective-C, Scala, Java, C, C++, sh/bash, Go, Perl, PHP, Python, Ruby.

Feel free to clone https://github.com/niik/highlighter-tests to test some of the modes

Future improvements

• TextMate grammars seem to have a ton of traction. We might want to look at using them instead of CodeMirror modes in the future. The problem with them is that they use RegExes that aren't 100% compatible with JavaScript's RegEx engine so we'd have to bring in another tool for that. Atom uses the oniguruma library which is a C++ library. With our current implementation using web workers that's not an option for us since we can't use native modules inside of a web worker safely. There's a neat package that uses the JavaScript implementation of oniguruma but the author does call out that there are incompatibilities between the two. The most likely thing for us to do is run highlighting inside of a renderer instead.
• If we decide to stick with the CodeMirror modes and we add a bunch of other languages to the point where spinning up a worker becomes an issue we could consider loading modes on-demand in the worker.
• With the performance having improved so much since I initially went down the web worker route it's quite possible that we could move highlighting back into the renderer and manage concurrency by chunking up our tokenization work into animation frames.

Caveats

• I've spotted a fairly common occurrence bug in the typescript mode which can cause the entire file after a particular combination of tokens to be classifies as a string. This has been fixed in CodeMirror/master but not yet released.
• There's a very small race condition window in between when we get the diff from Git and when we read the file contents of a modified file in the working directory where the contents of the file could have changed. The results would be pretty benign with syntax being offset or wrong. We could add some logic to ensure that lines match up after we've tokenized them. I did that in the beginning but removed it due to the overhead it added in the token model. We could also generate diffs with infinite context and manually collapse them in code. That's a much riskier approach from a memory conservation standpoint but it would mean that we could support expanding context dynamically like dotcom does (though there are other ways to achieve that as well).

Fixes #1312

[niik]

Is there anything I can do to help out here @shiftkey?

[niik]

📼

[niik]

🍸

[shiftkey]

[xt0rted]

The release notes have #1312 as the issue/pr for the feature instead of this or #2550

[shiftkey]

@xt0rted yeah, this was generated by what-the-changelog - I think what happened here was that it chose the only linked issue (which was #1312) as we didn't link to the original issue.

[nyssance]

Need a preference to open/close the syntax, it's in a daze.

[j-f1]

@nypisces Can you open a new issue and give us a few more details about your feature request there?