Add optional syntax highlighting to the diff viewer

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


Review Board SVN (deprecated)


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
Created and ran several unit tests. Made sure that all my test diffs displayed correctly.
  2. /trunk/reviewboard/diffviewer/ (Diff revision 2)
    Doing this as a setting in 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).
    1. 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.
    2. 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.
  3. Holy Big O, batman.  This is probably OK for now, but I'm thinking of an algorithm that would be roughly linear =)
    1. 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.
    2. 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.
    3. The new implementation looks good.
  1. Just some cosmetic stuff.
  2. /trunk/reviewboard/accounts/ (Diff revision 4)
    1. Yep. Already fixed.
  3. /trunk/reviewboard/diffviewer/ (Diff revision 4)
    Stick this down with the rest of the django imports.
    1. 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.
  4. /trunk/reviewboard/diffviewer/ (Diff revision 4)
    Ack, that's a lot of magic.
    1. 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 :)