Fix duplicate id attributes in diff view

 






 

 


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

 


















 

  

   Information

  





 
 

  aroben
 









 
 

  Review Board SVN (deprecated)
 









 
 

  trunk
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  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




   

   





  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    This is looking good. Thanks for doing this. A few changes though.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       /trunk/reviewboard/diffviewer/diffutils.py
       


        (Diff revision 4)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    





    
    No more need for j after the chunk['index'] change I mention.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       /trunk/reviewboard/diffviewer/diffutils.py
       


        (Diff revision 4)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    





    
    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. 



    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. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      Note that my changes to changeindex.html do keep sequential numbering of the links to the various chunks.
      
 
      

      4. 



    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.
      
 
      

      5. 


 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       /trunk/reviewboard/diffviewer/diffutils.py
       


        (Diff revision 4)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    





    
    We should be able to get rid of the interesting variable by just modifying file['changed_chunks'] directly.
    
  
    
 
    

 
    
  



 
    

    5. 




  5. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 




  

 
















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Looks great! Committing. Thanks :)
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3.