Show diffs for text file attachments.

Review Request #7123 — Created March 25, 2015 and submitted

Information

Review Board
release-2.5.x
6cb3753...

Reviewers

When viewing diffs of text file attachments, they're now rendered as if
they were actual diffs. We use the same diff chunk generation and
rendering logic, and commenting abilities.

This carries over to renderable text files as well. Rendered Markdown is
diffed as well. We even show the changes within lines, though only
text changes and not formatting.

Comment thumbnails are shown based on the render state. The thumbnails
show the diffed contents just like for actual diff comments.

Some of the diff-specific JavaScript code is now made more
general-purpose, and lives in TextCommentRowSelector. This is primarily
the row finding code. There's also a convenience function around it for
finding the beginning and end rows for a line range, needed for both the
diff viewer and the text attachment review UI.

Tested viewing individual revisions and revision ranges for various text
files and for Markdown files (rendered and source).

Tested commenting on lines within these, both single lines and spanning
multiple lines. I checked commenting on the first line and last lines of
the files, to test boundary issues.

Tested that the thumbnails matched the right lines and the right view mode
for plain text and Markdown, on both the review dialog and the reviews.


Description From Last Updated

Seems to be missing a left border.

daviddavid

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

reviewbotreviewbot

This could be much more nicely written as: if f: return get_chunks_in_range(f['chunks'], first_line, num_lines) else: return []

daviddavid

In this case I think makes more sense than parens.

daviddavid

This needs a var definition.

daviddavid

This should be a // comment. Same for other one-line comments below.

daviddavid

Can we use the context manager form here?

daviddavid
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/diffviewer/templatetags/difftags.py
        reviewboard/reviews/chunk_generators.py
        reviewboard/reviews/ui/text.py
        reviewboard/reviews/ui/markdownui.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/static/rb/css/pages/text-review-ui.less
        reviewboard/templates/diffviewer/diff_file_fragment.html
        reviewboard/templates/reviews/ui/text_comment_thumbnail.html
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/templates/reviews/ui/text.html
        reviewboard/templates/reviews/ui/_text_table.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/views/textCommentRowSelector.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/diffviewer/templatetags/difftags.py
        reviewboard/reviews/chunk_generators.py
        reviewboard/reviews/ui/text.py
        reviewboard/reviews/ui/markdownui.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/static/rb/css/pages/text-review-ui.less
        reviewboard/templates/diffviewer/diff_file_fragment.html
        reviewboard/templates/reviews/ui/text_comment_thumbnail.html
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/templates/reviews/ui/text.html
        reviewboard/templates/reviews/ui/_text_table.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/views/textCommentRowSelector.js
    
    
  2. reviewboard/reviews/chunk_generators.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
chipx86
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/diffviewer/templatetags/difftags.py
        reviewboard/reviews/chunk_generators.py
        reviewboard/reviews/ui/text.py
        reviewboard/reviews/ui/markdownui.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/static/rb/css/pages/text-review-ui.less
        reviewboard/templates/diffviewer/diff_file_fragment.html
        reviewboard/templates/reviews/ui/text_comment_thumbnail.html
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/templates/reviews/ui/text.html
        reviewboard/templates/reviews/ui/_text_table.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/views/textCommentRowSelector.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/diffviewer/templatetags/difftags.py
        reviewboard/reviews/chunk_generators.py
        reviewboard/reviews/ui/text.py
        reviewboard/reviews/ui/markdownui.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/static/rb/css/pages/text-review-ui.less
        reviewboard/templates/diffviewer/diff_file_fragment.html
        reviewboard/templates/reviews/ui/text_comment_thumbnail.html
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/templates/reviews/ui/text.html
        reviewboard/templates/reviews/ui/_text_table.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/views/textCommentRowSelector.js
    
    
  2. 
      
david
  1. 
      
  2. Seems to be missing a left border.

  3. reviewboard/diffviewer/diffutils.py (Diff revision 2)
     
     
     
     
     
     

    This could be much more nicely written as:

    if f:
        return get_chunks_in_range(f['chunks'], first_line, num_lines)
    else:
        return []
    
    1. get_chunks_in_range is a generator, and you can't just return those. You have to iterate and yield.

    2. That's not true:

      >>> def generator():
      ...     a = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
      ...     for item in a:
      ...         if item == 8:
      ...             raise StopIteration
      ...         yield item
      ...
      >>> def proxy():
      ...     return generator()
      ...
      >>> print list(proxy())
      [1, 2, 3, 4, 5, 6, 7]
      
    3. You're right. In my mind, I intended get_file_chunks_in_range itself to be a generator, meaning it'd have to yield from a nested generator and couldn't just return it, but that doesn't make any difference in this case. I'll change it.

    4. I still don't quite understand what you mean. In my example above, calling proxy() will return a generator object, and everything is still lazy until the list call. A function which calls a generator function, iterates it, and yields each result is useless.

  4. reviewboard/reviews/chunk_generators.py (Diff revision 2)
     
     

    In this case I think makes more sense than parens.

  5. This needs a var definition.

  6. This should be a // comment. Same for other one-line comments below.

  7. 
      
chipx86
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/ui/markdownui.py
        reviewboard/reviews/chunk_generators.py
        reviewboard/reviews/ui/text.py
        reviewboard/diffviewer/templatetags/difftags.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/static/rb/css/pages/text-review-ui.less
        reviewboard/templates/diffviewer/diff_file_fragment.html
        reviewboard/templates/reviews/ui/text_comment_thumbnail.html
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/templates/reviews/ui/text.html
        reviewboard/templates/reviews/ui/_text_table.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/views/textCommentRowSelector.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/ui/markdownui.py
        reviewboard/reviews/chunk_generators.py
        reviewboard/reviews/ui/text.py
        reviewboard/diffviewer/templatetags/difftags.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/static/rb/css/pages/text-review-ui.less
        reviewboard/templates/diffviewer/diff_file_fragment.html
        reviewboard/templates/reviews/ui/text_comment_thumbnail.html
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/templates/reviews/ui/text.html
        reviewboard/templates/reviews/ui/_text_table.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/views/textCommentRowSelector.js
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/reviews/ui/text.py (Diff revision 3)
     
     
     
     
     
     

    Can we use the context manager form here?

  3. 
      
chipx86
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/ui/markdownui.py
        reviewboard/reviews/chunk_generators.py
        reviewboard/reviews/ui/text.py
        reviewboard/diffviewer/templatetags/difftags.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/static/rb/css/pages/text-review-ui.less
        reviewboard/templates/diffviewer/diff_file_fragment.html
        reviewboard/templates/reviews/ui/text_comment_thumbnail.html
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/templates/reviews/ui/text.html
        reviewboard/templates/reviews/ui/_text_table.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/views/textCommentRowSelector.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/ui/markdownui.py
        reviewboard/reviews/chunk_generators.py
        reviewboard/reviews/ui/text.py
        reviewboard/diffviewer/templatetags/difftags.py
        reviewboard/diffviewer/diffutils.py
    
    Ignored Files:
        reviewboard/static/rb/css/pages/text-review-ui.less
        reviewboard/templates/diffviewer/diff_file_fragment.html
        reviewboard/templates/reviews/ui/text_comment_thumbnail.html
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/templates/reviews/ui/text.html
        reviewboard/templates/reviews/ui/_text_table.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/views/textCommentRowSelector.js
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (ff30e3d)
Loading...