Cache diff fragments by URL and preserve diff options while paging

 






 

 


  Review Request #2652
  —
  Created Oct. 10, 2011 and discarded 

 


















 

  

   Information

  





 
 

  bhollis
 









 
 

  Review Board
 









 
 

  
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
Thess changes take all the diff viewing options (syntax highlighting, expand/collapse) and embeds them into the URL of the diff fragment (instead of relying on cookies). This accomplishes two things - one, we now preserve diff preference as we page around the diff, and two, individual diff fragments can now be entirely cached by their URL using the @cache_page directive.

On GitHub as individual commits:

https://github.com/bhollis/reviewboard/commit/f5268174871ab5f5f30d560e592d2e53570d5780
https://github.com/bhollis/reviewboard/commit/58a2c8c6dc22b49e56e2e6cd4dc1d2367c412a93
https://github.com/bhollis/reviewboard/commit/4fc6775b62aa66b391dc9e95789b0f3891226d93


 








  


 
 

  
Used on our own RB instance

 







 









 





  








 









 






  

 








 














 



 

  

   





 BH





   

   

  

 



 















 





 

  

   




chipx86




   

   





  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    The general concept seems pretty good, and there's certainly a lot of benefits we'd get from this, but the use of cookies isn't arbitrary. We want to remember if a user last looked at some diff expanded or collapsed. Whether that's done through cookies or stored info in account preferences doesn't matter to me, but I believe people today are enabling expanded diffs and choosing to stick with that, which the cookies today give us.

I could be wrong, and maybe it's worth revisiting how expand/collapse even work today, but I don't want to give up the behavior without being sure.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      That's an interesting position - I thought it was just a bug that diffs stayed expanded. Whenever I'm using the diff view I usually want to see just the changed bits, then expand as-needed. I could see retaining the cookie if you thought people really wanted it that way, but still generate unique URLs instead of changing content based on the cookie.
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      What would you like me to do so this patch could be accepted?
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      Sorry Ben, this slipped past me.

I think we need more information. I'm not opposed to changing this behavior, but I don't really want a lot of fallout. So maybe a discussion on the mailing list, see if admins can ask their users?

If we can determine the URL partially based on the cookie, in order to keep that behavior for now, I'm satisfied.
      
 
      

      4. 


 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   





 BH





   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Discarded

  
 







 


 










  

 









     

    

   

  

  

   
   Loading...