Fix duplicate id attributes in diff view

Review Request #172 — Created Oct. 22, 2007 and submitted

aroben
Review Board SVN (deprecated)
trunk
reviewboard
Not all chunks were getting an index attribute assigned, which lead to many tbody elements in the diff view having the same id attribute (e.g, "chunk0."). Now all chunks are indexed.

Also removed some unnecessary work from diffutils.add_navigation_cues while I was at it.
Verified that all chunks in a given patch now have unique id attributes. Verified that the change index at the top and bottom of the diff still functions correctly. Verified that only changed chunks are selected when using keyboard shortcuts such as j/k or n/p.
chipx86
  1. This is looking good. Thanks for doing this. A few changes though.
  2. No more need for j after the chunk['index'] change I mention.
  3. This is looking good, but this line is going to cause a problem. Only non-equal lines should have an index set. This line should probably be moved into the next conditional. Also, we need to set it to k, not j, so that the indexes in the diff viewer are like "1 2 3 4 5" rather than "1 4 6" etc.
    1. Why should only non-equal lines have an index set? That was the cause of the original bug: the id attributes are set to "chunk{{file.index}}.{{chunk.index}}", and since all equal chunks had no index, they all had the same id attribute. Will something be broken by equal chunks having an index?
    2. This is so that when you use the keyboard shortcuts (N, P, etc.) you'll be able to jump around the changed chunks.
      
      An argument could be made that you should be able to jump to Equals blocks as well, but there's some history behind this behavior. It's how our old system at VMware worked, and if we were to change it, we'd have a lot of angry people on our hands :) But even besides that, I think it's more useful to jump to the changed chunks since that's really what you're interested in when reviewing a diff.
    3. Note that my changes to changeindex.html do keep sequential numbering of the links to the various chunks.
    4. The links, yes, and it's certainly cleaner than our old code there, but keyboard shortcuts will no longer skip the equals blocks and go only to the changed chunks.
  4. We should be able to get rid of the interesting variable by just modifying file['changed_chunks'] directly.
  5. 
      
chipx86
  1. Looks great! Committing. Thanks :)
  2. 
      
Loading...