Summary

Add optional syntax highlighting to the diff viewer

Review Request #102 — Created June 26, 2007 and submitted

Information

Owner

chipx86

Repository

Review Board SVN (deprecated)

Branch

trunk

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Added optional syntax highlighting to the diff viewer using the Pygments library. The result is very nice syntax highlighted code, and it still works with interline diffing and everything else. The only real downside is that it may take slightly longer to generate and display the code (though it shouldn't be a significant difference), and that the resulting HTML is bigger (a 60KB HTML page becomes around 100KB). For most setups, however, the download size shouldn't be an important factor.

This feature is disabled by default and must be turned on in settings_local.py.

Testing Done

Created and ran several unit tests. Made sure that all my test diffs displayed correctly.

Screenshots

Syntax highlighting in action

/trunk/reviewboard/diffviewer/views.py (Diff revision 2)

Doing this as a setting in settings_local.py is insufficient.  I would much much much rather it be something users can toggle on and off individually (and maybe don't make it available if pygments isn't installed).

chipx86 June 26, 2007, 6:07 p.m.

That's a future improvement I do want to make, but I still think it should be in settings as well. Given the overhead (in transfer if nothing else), some installs should be able to disable it. I want to make a per-user setting on top of that that users can toggle for an install that allows it, but that'll be a future improvement.

david June 27, 2007, 4:38 a.m.

OK, keep it in settings.  Please don't commit this until you have a preference in the profile model (really, this should be so easy), 'cuz I'll want to turn it on for our internal server, and people will complain.

/trunk/reviewboard/diffviewer/templatetags/difftags.py (Diff revision 2)

Holy Big O, batman.  This is probably OK for now, but I'm thinking of an algorithm that would be roughly linear =)

chipx86 June 26, 2007, 6:08 p.m.

Yeah. Well this started out pretty reasonable until I realized we'd have to deal with the nested tags and entities issues. This is partly why I don't want this enabled by default yet :) But it's something to work off of.

chipx86 June 26, 2007, 10:10 p.m.

Funny how you can approach code a certain way that makes sense at the time but then you end up moving code around and encountering other things and then you end up in a big unoptimized mess.

I couldn't sleep so I decided to rewrite the algorithm, now that I've actually thought about the requirements after the fact instead of bumping my head along the way. Should be much better now. We make only one pass through the string.

david June 27, 2007, 4:37 a.m.
```
The new implementation looks good.
```

Ship it!

```
Just some cosmetic stuff.
```
/trunk/reviewboard/accounts/views.py (Diff revision 4)
```
Silly.
```
1. chipx86 June 27, 2007, 3:42 p.m.
  Yep. Already fixed.

/trunk/reviewboard/diffviewer/views.py (Diff revision 4)

Stick this down with the rest of the django imports.

chipx86 June 27, 2007, 3:44 p.m.

Oops. I had that there back when I had the imports for pygments (and the check for the setting) at the top of the file. I of course moved those, so no need to keep the import at the very top.

/trunk/reviewboard/diffviewer/tests.py (Diff revision 4)

Ack, that's a lot of magic.

chipx86 June 27, 2007, 3:44 p.m.

It was a patch given in one of the bug reports that triggered the same bug we hit internally at VMware. As crazy as it is, I figured it's a good test case to keep :)

Thanks!