Summary

Add rendering of binary file attachments in the diff viewer.

Review Request #4468 — Created Aug. 20, 2013 and submitted Aug. 22, 2013, 2:13 a.m.

Information

Owner

chipx86

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Add rendering of binary file attachments in the diff viewer.

This introduces the ability to associate a FileAttachment with a
path, revision and repository. The diff viewer can make use of this to
render thumbnails for binary files.

There's no way yet of associating a file outside of modifying the
database. Later, there will be API and UI for this.

This work is based on Tina Yang's work, with some fixes and a change in
the association model. Previously, FileAttachments were associated with
FileDiffs, but have been changed in order to reduce the number of
duplicate files that exist in the database.

Testing Done

Generated a diff that introduced an image and modified another image.

Initially, I saw the standard "This is a binary file," except with the
Download links and the text in both the left-hand and right-hand sides.

I then manually associated file attachments with these files. They didn't
show up in the list of file attachments for the page, but they did show up
in the diff viewer in the appropriate places.

Files

Issues

Description	From	Last Updated
There's a lot more spacing between the second and third boxes than between the first and second.	david	Aug. 21, 2013, 4:52 p.m.
I really don't understand what this means.	david	Aug. 21, 2013, 12:01 a.m.
'DiffSet' imported but unused	reviewbot	Aug. 21, 2013, 4:48 p.m.
'DiffSetHistory' imported but unused	reviewbot	Aug. 21, 2013, 4:48 p.m.
thenew -> the new	david	Aug. 21, 2013, 4:51 p.m.
'AnonymousUser' imported but unused	reviewbot	Aug. 21, 2013, 4:48 p.m.
'Template' imported but unused	reviewbot	Aug. 21, 2013, 4:48 p.m.
'Context' imported but unused	reviewbot	Aug. 21, 2013, 4:48 p.m.
'Review' imported but unused	reviewbot	Aug. 21, 2013, 4:49 p.m.
'Tool' imported but unused	reviewbot	Aug. 21, 2013, 4:49 p.m.
Col: 80 E501 line too long (100 > 79 characters)	reviewbot	Aug. 21, 2013, 11:23 p.m.
Col: 80 E501 line too long (101 > 79 characters)	reviewbot	Aug. 21, 2013, 11:23 p.m.
Col: 80 E501 line too long (105 > 79 characters)	reviewbot	Aug. 21, 2013, 11:23 p.m.
Col: 80 E501 line too long (106 > 79 characters)	reviewbot	Aug. 21, 2013, 11:23 p.m.
Col: 80 E501 line too long (96 > 79 characters)	reviewbot	Aug. 21, 2013, 11:23 p.m.
Col: 80 E501 line too long (97 > 79 characters)	reviewbot	Aug. 21, 2013, 11:23 p.m.
local variable 'file_attachment' is assigned to but never used	reviewbot	Aug. 21, 2013, 4:49 p.m.
Col: 80 E501 line too long (101 > 79 characters)	reviewbot	Aug. 21, 2013, 11:23 p.m.
Col: 80 E501 line too long (102 > 79 characters)	reviewbot	Aug. 21, 2013, 11:23 p.m.
This comment is obsolete now, right?	david	Aug. 21, 2013, 4:52 p.m.
Col: 80 E501 line too long (100 > 79 characters)	reviewbot	Aug. 21, 2013, 11:23 p.m.
Col: 80 E501 line too long (101 > 79 characters)	reviewbot	Aug. 21, 2013, 11:23 p.m.
Col: 80 E501 line too long (105 > 79 characters)	reviewbot	Aug. 21, 2013, 11:23 p.m.
Col: 80 E501 line too long (106 > 79 characters)	reviewbot	Aug. 21, 2013, 11:23 p.m.
Col: 80 E501 line too long (96 > 79 characters)	reviewbot	Aug. 21, 2013, 11:23 p.m.
Col: 80 E501 line too long (97 > 79 characters)	reviewbot	Aug. 21, 2013, 11:23 p.m.
Col: 80 E501 line too long (101 > 79 characters)	reviewbot	Aug. 21, 2013, 11:23 p.m.
Col: 80 E501 line too long (102 > 79 characters)	reviewbot	Aug. 21, 2013, 11:23 p.m.
Col: 80 E501 line too long (100 > 79 characters)	reviewbot	Aug. 22, 2013, 12:07 a.m.
Col: 80 E501 line too long (101 > 79 characters)	reviewbot	Aug. 22, 2013, 12:07 a.m.
Col: 80 E501 line too long (105 > 79 characters)	reviewbot	Aug. 22, 2013, 12:07 a.m.
Col: 80 E501 line too long (106 > 79 characters)	reviewbot	Aug. 22, 2013, 12:07 a.m.
Col: 80 E501 line too long (96 > 79 characters)	reviewbot	Aug. 22, 2013, 12:07 a.m.
Col: 80 E501 line too long (97 > 79 characters)	reviewbot	Aug. 22, 2013, 12:07 a.m.
Col: 80 E501 line too long (101 > 79 characters)	reviewbot	Aug. 22, 2013, 12:07 a.m.
Col: 80 E501 line too long (102 > 79 characters)	reviewbot	Aug. 22, 2013, 12:07 a.m.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/attachments/managers.py
    reviewboard/attachments/models.py
    reviewboard/attachments/evolutions/file_attachment_repo_info.py
    reviewboard/attachments/admin.py
    reviewboard/scmtools/git.py
    reviewboard/attachments/tests.py
    reviewboard/attachments/templatetags/attachments.py
    reviewboard/attachments/evolutions/__init__.py
  Ignored Files:
    reviewboard/templates/diffviewer/diff_file_fragment.html
    reviewboard/static/rb/css/reviews.less
    reviewboard/static/rb/css/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/templates/reviews/reviewable_page_data.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/attachments/tests.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (100 > 79 characters)
```
reviewboard/attachments/tests.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (101 > 79 characters)
```
reviewboard/attachments/tests.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (105 > 79 characters)
```
reviewboard/attachments/tests.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (106 > 79 characters)
```
reviewboard/attachments/tests.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (96 > 79 characters)
```
reviewboard/attachments/tests.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (97 > 79 characters)
```
reviewboard/attachments/tests.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (101 > 79 characters)
```
reviewboard/attachments/tests.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (102 > 79 characters)
```

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/attachments/managers.py
    reviewboard/attachments/models.py
    reviewboard/attachments/evolutions/file_attachment_repo_info.py
    reviewboard/attachments/admin.py
    reviewboard/scmtools/git.py
    reviewboard/attachments/tests.py
    reviewboard/attachments/templatetags/attachments.py
    reviewboard/attachments/evolutions/__init__.py
  Ignored Files:
    reviewboard/templates/diffviewer/diff_file_fragment.html
    reviewboard/static/rb/css/reviews.less
    reviewboard/static/rb/css/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/templates/reviews/reviewable_page_data.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/attachments/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'DiffSet' imported but unused
```
reviewboard/attachments/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'DiffSetHistory' imported but unused
```
reviewboard/attachments/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'AnonymousUser' imported but unused
```
reviewboard/attachments/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'Template' imported but unused
```
reviewboard/attachments/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'Context' imported but unused
```
reviewboard/attachments/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'Review' imported but unused
```
reviewboard/attachments/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'Tool' imported but unused
```
reviewboard/attachments/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 local variable 'file_attachment' is assigned to but never used
```

Fix it!

binary-files.png

There's a lot more spacing between the second and third boxes than between the first and second.

The issue has been dropped. Show all issues

There's a lot more spacing between the second and third boxes than between the first and second.

chipx86 Aug. 21, 2013, 4:53 p.m.

Yep, looks like, but that wasn't introduced in this change. I'll get to it separately.

reviewboard/attachments/managers.py (Diff revision 1)

An issue was opened. Show all issues

I really don't understand what this means.

chipx86 Aug. 21, 2013, 4:53 p.m.

"... will be based on the new or modified file introduced in the diff. Otherwise, it will be based on the original file."

Is that better? Or still bad?

david Aug. 21, 2013, 5:51 p.m.

I don't understand what "based on" means.

chipx86 Aug. 21, 2013, 11:22 p.m.

The details of the FileAttachment being constructed will be [based on/come from/I don't know what word is best] one of the two revisions that a FileDiff stores.

Each side-by-side diff in the diff viewer is backed by a FileDiff, and has a left side and a right side. The FileAttachment being created can be based on either the original file (left side) or modified file (right side).

david Aug. 21, 2013, 11:44 p.m.

How about something like this:

FileAttachments created from a FileDiff are used to represent changes to binary files which would otherwise not be displayed with the diff. An individual FileAttachment can represent either the original or modified copy of the file. If 'from_modified' is True, then this FileAttachment will be created using the information (filename, revision, etc.) for the modified version. If it is False, this FileAttachment will be created using the information for the original version.

reviewboard/attachments/templatetags/attachments.py (Diff revision 1)
The issue has been resolved. Show all issues
```
thenew -> the new
```
reviewboard/scmtools/git.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This comment is obsolete now, right?
```

Change Summary:

* Removed a bunch of unused imports.
* Fixed some documentation issues.

Diff:

Revision 2 (+723 -139)

Show changes

	reviewboard/attachments/admin.py
	reviewboard/attachments/managers.py
	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/attachments/evolutions/__init__.py
	reviewboard/attachments/evolutions/file_attachment_repo_info.py
	reviewboard/attachments/templatetags/attachments.py
	reviewboard/reviews/views.py
	8 more

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/attachments/managers.py
    reviewboard/attachments/models.py
    reviewboard/attachments/evolutions/file_attachment_repo_info.py
    reviewboard/attachments/admin.py
    reviewboard/scmtools/git.py
    reviewboard/attachments/tests.py
    reviewboard/attachments/templatetags/attachments.py
    reviewboard/attachments/evolutions/__init__.py
  Ignored Files:
    reviewboard/templates/diffviewer/diff_file_fragment.html
    reviewboard/static/rb/css/reviews.less
    reviewboard/static/rb/css/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/templates/reviews/reviewable_page_data.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/attachments/tests.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (100 > 79 characters)
```
reviewboard/attachments/tests.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (101 > 79 characters)
```
reviewboard/attachments/tests.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (105 > 79 characters)
```
reviewboard/attachments/tests.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (106 > 79 characters)
```
reviewboard/attachments/tests.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (96 > 79 characters)
```
reviewboard/attachments/tests.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (97 > 79 characters)
```
reviewboard/attachments/tests.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (101 > 79 characters)
```
reviewboard/attachments/tests.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (102 > 79 characters)
```

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/attachments/managers.py
    reviewboard/attachments/models.py
    reviewboard/attachments/evolutions/file_attachment_repo_info.py
    reviewboard/attachments/admin.py
    reviewboard/scmtools/git.py
    reviewboard/attachments/tests.py
    reviewboard/attachments/templatetags/attachments.py
    reviewboard/attachments/evolutions/__init__.py
  Ignored Files:
    reviewboard/templates/diffviewer/diff_file_fragment.html
    reviewboard/static/rb/css/reviews.less
    reviewboard/static/rb/css/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/templates/reviews/reviewable_page_data.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Change Summary:

Changed the documentation for create_from_filediff.

Diff:

Revision 3 (+726 -139)

Show changes

	reviewboard/attachments/admin.py
	reviewboard/attachments/managers.py
	reviewboard/attachments/models.py
	reviewboard/attachments/tests.py
	reviewboard/attachments/evolutions/__init__.py
	reviewboard/attachments/evolutions/file_attachment_repo_info.py
	reviewboard/attachments/templatetags/attachments.py
	reviewboard/reviews/views.py
	8 more

Fix it!

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/attachments/managers.py
    reviewboard/attachments/models.py
    reviewboard/attachments/evolutions/file_attachment_repo_info.py
    reviewboard/attachments/admin.py
    reviewboard/scmtools/git.py
    reviewboard/attachments/tests.py
    reviewboard/attachments/templatetags/attachments.py
    reviewboard/attachments/evolutions/__init__.py
  Ignored Files:
    reviewboard/templates/diffviewer/diff_file_fragment.html
    reviewboard/static/rb/css/reviews.less
    reviewboard/static/rb/css/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/templates/reviews/reviewable_page_data.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/attachments/tests.py (Diff revision 3)
An issue was opened. Show all issues
```
Col: 80
 E501 line too long (100 > 79 characters)
```
reviewboard/attachments/tests.py (Diff revision 3)
An issue was opened. Show all issues
```
Col: 80
 E501 line too long (101 > 79 characters)
```
reviewboard/attachments/tests.py (Diff revision 3)
An issue was opened. Show all issues
```
Col: 80
 E501 line too long (105 > 79 characters)
```
reviewboard/attachments/tests.py (Diff revision 3)
An issue was opened. Show all issues
```
Col: 80
 E501 line too long (106 > 79 characters)
```
reviewboard/attachments/tests.py (Diff revision 3)
An issue was opened. Show all issues
```
Col: 80
 E501 line too long (96 > 79 characters)
```
reviewboard/attachments/tests.py (Diff revision 3)
An issue was opened. Show all issues
```
Col: 80
 E501 line too long (97 > 79 characters)
```
reviewboard/attachments/tests.py (Diff revision 3)
An issue was opened. Show all issues
```
Col: 80
 E501 line too long (101 > 79 characters)
```
reviewboard/attachments/tests.py (Diff revision 3)
An issue was opened. Show all issues
```
Col: 80
 E501 line too long (102 > 79 characters)
```

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    reviewboard/reviews/views.py
    reviewboard/attachments/managers.py
    reviewboard/attachments/models.py
    reviewboard/attachments/evolutions/file_attachment_repo_info.py
    reviewboard/attachments/admin.py
    reviewboard/scmtools/git.py
    reviewboard/attachments/tests.py
    reviewboard/attachments/templatetags/attachments.py
    reviewboard/attachments/evolutions/__init__.py
  Ignored Files:
    reviewboard/templates/diffviewer/diff_file_fragment.html
    reviewboard/static/rb/css/reviews.less
    reviewboard/static/rb/css/diffviewer.less
    reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/templates/reviews/reviewable_page_data.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Ship it!

```
Ship It!
```

Status:: Completed