Add the beginnings of review UI support.

 






 

 


  Review Request #3337
  —
  Created Sept. 20, 2012 and submitted 

 


















 

  

   Information

  





 
 

  chipx86
 









 
 

  Review Board
 









 
 

  master
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
Add the beginnings of review UI support.

This introduces the basic support for building review UIs. This allows
Review Board and extensions to register special UIs for reviewing
certain file attachments. An example would be a special UI for reviewing
XML files, or PDF files.

There's a base ReviewUI class that forms the base for anything that
provides review functionality for something else. It knows how to do the
basic rendering and grab the list of current comments. In time, we may
even want to move the diff viewer to this.

Then there's FileAttachmentReviewUI, which is where the magic will
really happen for us. It looks at registered handlers and tries to find
one that best matches the mimetype we're looking for. This is the same
logic as thumbnail renderer matching. (And we may want to consolidate
that logic in time.)

Review UIs are currently displayed in their own page, but they will
later be able to specify that they can review inline. In this case,
we'll just pop up a lightbox and load in the HTML for the review UI.

Right now, there's only one review UI, and that's for screenshots. This
is not a FileAttachmentReviewUI. I have one in-progress for image file
attachments. However, scanning for review UIs doesn't actually work in
practice very well, because it requires that the file defining the class
is imported. This will be fixed up for the first review UI.

Screenshot support has moved entirely over to this.

While not complete, this is a good base for the feature, and should get
us started so that students can start poking at code, and we can start
fine-tuning the support.

 








  


 
 

  
Verified that screenshot review still worked as it did before.

Tested with a branch for image review. The basics worked, except our
JavaScript really is not built to make image review outside of screenshot
classes work very well.

 







 









 





  








 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
mimeparse isn't part of the standard library, so it should be grouped with django

      		
      
       daviddavid
      
      
       
      
     




     

      
       
       
It seems a little odd that we'd show 404 for a file attachment which exists but has no review UI. …

      		
      
       daviddavid
      
      
       
      
     


    
   

  

 







 






  

 








 














 





 

  

   





 SM





   

   





  

 



 

  


   

   

    
 
smacleod


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Looking good so far! This will really open up the review possibilities with RB, especially once the extension hook stuff is in.

What could be cool as well, are DiffUIs for mimetypes. The file attachments and screenshot's aren't really versioned like the diffs, but if they were you could do the same sort of thing (Think overlayed images, side-by-sides etc.). Or you could diff against binary files in the repo (once rbtools gets the binary support of course). Just a thought I had while reviewing :D
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      That's something I fully intend to do. There was some work done for binary file attachments uploaded with diffs, and I want this integrated into there. Future project though.
      
 
      

      2. 


 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/ui/base.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    mimeparse isn't part of the standard library, so it should be grouped with django
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/ui/base.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    There's probably a really clever way to use max() here
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Maybe. This is copy/pasted from other code we have. I want to consolidate all that, so maybe revisit this particular one?
      
 
      

      2. 


 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/reviews/views.py
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    It seems a little odd that we'd show 404 for a file attachment which exists but has no review UI. Can you add some logging at the least?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      Agreed. Consider this a temporary thing while I work on the infrastructure.

I could just make it return the file, I suppose.

You'd have to go out of your way to get to this page for unreviewable file types.
      
 
      

      2. 


 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/static/rb/js/reviews.js
      


       (Diff revision 1)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    





    
    ?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      The idea is that some files could be reviewed without switching pages. So, say an image. Click the image, a lightbox appears, you do the commenting, close. No page swapping, faster reviewing.

Don't worry too much about this for now. It's coming.
      
 
      

      2. 


 
    


    

 
    

    6. 




  6. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 




  

 
















 



 

  

   




chipx86




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




chipx86




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 




 
Change Summary:

 
Pushed to master (57d405e)





 


 










  

 









     

    

   

  

  

   
   Loading...