Summary

Show diffs for text file attachments.

Review Request #7123 — Created March 25, 2015 and submitted April 7, 2015, 4:11 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-2.5.x

Bugs

Depends On

Commit

6cb3753...

Reviewers

Groups

reviewboard

People

Description

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.

Testing Done

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.

Files

Issues

Description	From	Last Updated
Seems to be missing a left border.	david	April 1, 2015, 2:41 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 25, 2015, 10:31 p.m.
This could be much more nicely written as: if f: return get_chunks_in_range(f['chunks'], first_line, num_lines) else: return []	david	April 4, 2015, 2:47 p.m.
In this case I think makes more sense than parens.	david	April 1, 2015, 2:43 p.m.
This needs a var definition.	david	April 1, 2015, 2:44 p.m.
This should be a // comment. Same for other one-line comments below.	david	April 1, 2015, 2:44 p.m.
Can we use the context manager form here?	david	April 4, 2015, 2:51 p.m.

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

reviewboard/reviews/chunk_generators.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Change Summary:

Fixed a line length issue.

Commit:

6abd07f2d109f9354440fb57411b8849610256d9

4e9d0f84c57cc9069398eb5d9e12798356efaaf9

Diff:

Revision 2 (+491 -272)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/templatetags/difftags.py
	reviewboard/reviews/chunk_generators.py
	reviewboard/reviews/views.py
	reviewboard/reviews/ui/markdownui.py
	reviewboard/reviews/ui/text.py
	reviewboard/static/rb/css/pages/text-review-ui.less
	reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
	6 more

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

Diff thumbnails (Revision 1)
The issue has been resolved. Show all issues
```
Seems to be missing a left border.
```

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

The issue has been resolved. Show all issues

This could be much more nicely written as:

if f:
    return get_chunks_in_range(f['chunks'], first_line, num_lines)
else:
    return []

chipx86 April 1, 2015, 2:45 p.m.

get_chunks_in_range is a generator, and you can't just return those. You have to iterate and yield.

david April 2, 2015, 2:48 p.m.

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]

chipx86 April 4, 2015, 2:48 p.m.

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.

david April 6, 2015, 2:45 p.m.

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.

reviewboard/reviews/chunk_generators.py (Diff revision 2)
The issue has been resolved. Show all issues
```
In this case I think  makes more sense than parens.
```
reviewboard/static/rb/js/views/textBasedReviewableView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
This needs a var definition.
```
reviewboard/static/rb/js/views/textCommentRowSelector.js (Diff revision 2)
The issue has been resolved. Show all issues
```
This should be a // comment. Same for other one-line comments below.
```

Change Summary:


Fixed borders on text diffs.
Cleaned up the logic around the chunk range iterator.
Switch to a \ for a line continuation.
Added a missing var.
Changed single-line comments in JavaScript code to use //.

Commit:

4e9d0f84c57cc9069398eb5d9e12798356efaaf9

8d0f95cc0bcff7bb23ff445921e7f9cc1785a498

Diff:

Revision 3 (+505 -275)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/templatetags/difftags.py
	reviewboard/reviews/chunk_generators.py
	reviewboard/reviews/ui/markdownui.py
	reviewboard/reviews/ui/text.py
	reviewboard/static/rb/css/pages/text-review-ui.less
	reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
	reviewboard/static/rb/js/views/textBasedReviewableView.js
	5 more

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

reviewboard/reviews/ui/text.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can we use the context manager form here?
```

Change Summary:


Switched to using a context manager for file closing. (Note that Django's File field doesn't do the open for you, so we still have to do that.)
Changed the body of get_file_chunks_in_range to just return the nested generator.

Commit:

8d0f95cc0bcff7bb23ff445921e7f9cc1785a498

6cb37533165f29aa4c7afd97c577be36e2ae8424

Diff:

Revision 4 (+507 -275)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/templatetags/difftags.py
	reviewboard/reviews/chunk_generators.py
	reviewboard/reviews/ui/markdownui.py
	reviewboard/reviews/ui/text.py
	reviewboard/static/rb/css/pages/text-review-ui.less
	reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
	reviewboard/static/rb/js/views/textBasedReviewableView.js
	5 more

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

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.5.x (ff30e3d)