Add support for showing diffs of file attachments in the diff viewer.

Review Request #4481 — Created Aug. 23, 2013 and submitted

Information

Review Board
master

Reviewers

Add support for showing diffs of file attachments in the diff viewer.

Review UIs can now opt in to providing diffing functionality for file
attachments. A ReviewUI subclass that has supports_diffing=True will
have the whole width of the diff table to render in. It will be given
both the new file's fileAttachmentID, and the original's (as
diffAgainstFileAttachmentID).

FileAttachmentComment can now specify a FileAttachment that's being
diffed against. The ID for this can be set when creating a new comment
in the webapi.

FileAttachmentReviewUI will properly filter the comments when passing
down to the JavaScript to ensure only the comments matching that diff
range (or, if not using a diff, comments that are on a single
FileAttachment) are passed down.

It's up to the review UI to decide how to render the diff.
Tested with the image diff review UI change in /r/4493/.
Description From Last Updated

It seems a little weird that we ask if orig_review_ui.supports_diffing but then use the modified_review_ui to do the diff. I …

daviddavid

I think it would make more sense for this to be an else block from the previous conditional.

daviddavid

Col: 80 E501 line too long (116 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (116 > 79 characters)

reviewbotreviewbot
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/webapi/tests.py
        reviewboard/reviews/evolutions/file_attachment_comment_diff_id.py
        reviewboard/reviews/ui/base.py
      Ignored Files:
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/templates/diffviewer/diff_file_fragment.html
        reviewboard/static/rb/js/models/fileAttachmentCommentBlockModel.js
        reviewboard/static/rb/js/resources/models/reviewModel.js
        reviewboard/static/rb/js/resources/models/fileAttachmentCommentModel.js
    
    
  2. reviewboard/webapi/tests.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (116 > 79 characters)
    
  3. reviewboard/webapi/tests.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  4. 
      
reviewbot
  1. This is a review from Review Bot.
      Tool: Pyflakes
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/webapi/tests.py
        reviewboard/reviews/evolutions/file_attachment_comment_diff_id.py
        reviewboard/reviews/ui/base.py
      Ignored Files:
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/templates/diffviewer/diff_file_fragment.html
        reviewboard/static/rb/js/models/fileAttachmentCommentBlockModel.js
        reviewboard/static/rb/js/resources/models/reviewModel.js
        reviewboard/static/rb/js/resources/models/fileAttachmentCommentModel.js
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 1)
     
     
     
     
     
     
    Show all issues
    It seems a little weird that we ask if orig_review_ui.supports_diffing but then use the modified_review_ui to do the diff. I know they should be the same class, but perhaps change the check to be modified_review_ui.supports_diffing ?
  3. reviewboard/reviews/views.py (Diff revision 1)
     
     
     
    Show all issues
    I think it would make more sense for this to be an else block from the previous conditional.
  4. 
      
chipx86
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/webapi/tests.py
        reviewboard/reviews/evolutions/file_attachment_comment_diff_id.py
        reviewboard/reviews/ui/base.py
      Ignored Files:
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/templates/diffviewer/diff_file_fragment.html
        reviewboard/static/rb/js/models/fileAttachmentCommentBlockModel.js
        reviewboard/static/rb/js/resources/models/reviewModel.js
        reviewboard/static/rb/js/resources/models/fileAttachmentCommentModel.js
    
    
  2. reviewboard/webapi/tests.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (116 > 79 characters)
    
  3. 
      
reviewbot
  1. This is a review from Review Bot.
      Tool: Pyflakes
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources.py
        reviewboard/reviews/models.py
        reviewboard/reviews/evolutions/__init__.py
        reviewboard/webapi/tests.py
        reviewboard/reviews/evolutions/file_attachment_comment_diff_id.py
        reviewboard/reviews/ui/base.py
      Ignored Files:
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/templates/diffviewer/diff_file_fragment.html
        reviewboard/static/rb/js/models/fileAttachmentCommentBlockModel.js
        reviewboard/static/rb/js/resources/models/reviewModel.js
        reviewboard/static/rb/js/resources/models/fileAttachmentCommentModel.js
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
chipx86
Review request changed
Status:
Completed