Summary

Display commits in the diffviewer based on the selected revision

Review Request #6933 — Created Feb. 11, 2015 and submitted March 3, 2015, 5:37 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

dvcs

Bugs

Depends On

~~6816~~, ~~6815~~

Blocks

6943

Commit

9c6d138...

Reviewers

Groups

reviewboard

People

chipx86, mike_conley, smacleod

Description

The DiffViewerView now provides information about the DiffCommits on
the selected revision. The DiffViewerPageView and associated models
have been updated to parse this information. The information on
DiffCommits is shown in the diffviewer now via the DiffCommitIndexView,
which renders a table similar to the DiffCommitListField on a review
request.

When the diff revision slider is moved to select a diff revision, the
DiffCommitListField will automatically update and populate the table
with information from the individual diff commits pertaining to the
selected revision. If the selected diff revision does not have any
commits, then the table will be hidden until a revision is selected that
does have associated commits.

Testing Done

Manually verified the following:

Selecting the base and a diff revision that had commits displayed the
commit table.
Selecting the base and a diff revision that did not have commits did
not display the table.
Switching from one diff revision to another with different commits
changed the table to display the new commits.

Files

Issues

Description	From	Last Updated
We can reduce the work needed with: diff_context['diff_commits'] = \ diffset.diff_commits.values('commit_id', 'summary', 'author_name')	chipx86	Feb. 12, 2015, 3:02 p.m.
Can you sort these alphabetically, with comments?	chipx86	Feb. 12, 2015, 3:02 p.m.
If you append this after building all the children, you'll reduce the work performed by the browser.	chipx86	Feb. 12, 2015, 3:03 p.m.
No space after function.	chipx86	Feb. 12, 2015, 3:03 p.m.
Can this be one line?	chipx86	Feb. 12, 2015, 3:04 p.m.
You can use this.$el for this.	chipx86	Feb. 12, 2015, 3:04 p.m.
Should be inserted alphabetically.	chipx86	Feb. 12, 2015, 3:05 p.m.
Can we put the comments before the attributes, like in other models? That helps to prevent having to modify other …	chipx86	Feb. 27, 2015, 9:49 a.m.
What if this is empty?	david	Feb. 23, 2015, 9:08 a.m.
These need to be localized. Given that, we should just use _.template for this.	chipx86	Feb. 27, 2015, 9:55 a.m.
Can we use underscores in the ID to match the other diff ones?	chipx86	Feb. 27, 2015, 9:57 a.m.
Did you test this? I don't think <%- %> can work this way (since it pulls values out of the …	david	Feb. 27, 2015, 5:14 p.m.
This line is useless because you set it again in update()	david	March 3, 2015, 2:17 p.m.
These two lines can be combined.	david	March 3, 2015, 4:47 p.m.
Now that you've moved the creation into here, you don't need the .empty() call.	david	March 3, 2015, 4:58 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Change Summary:

Add screenshots and a screen capture.

Added Files:

Change Summary:

Update dependencies

Depends On:

+	~~6816 - Add initial web API for commit histories.~~
+	~~6815 - Add DiffCommit model for review requests with commit histories.~~

Diff:

Revision 2 (+142 -2)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/views.py
	reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/templates/diffviewer/view_diff.html

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

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

The issue has been resolved. Show all issues

We can reduce the work needed with:

diff_context['diff_commits'] = \
    diffset.diff_commits.values('commit_id', 'summary', 'author_name')

reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Can you sort these alphabetically, with comments?
```
reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
If you append this after building all the children, you'll reduce the work performed by the browser.
```
reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
No space after function.
```
reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Can this be one line?
```
reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
You can use this.$el for this.
```
reviewboard/static/rb/js/pages/models/diffViewerPageModel.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Should be inserted alphabetically.
```

Change Summary:

master -> dvcs

Branch:

-	master
+	dvcs

Change Summary:

Fix Christian's issues.

Diff:

Revision 3 (+144)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/views.py
	reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/templates/diffviewer/view_diff.html

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Change Summary:

JSHint

Diff:

Revision 4 (+144)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/views.py
	reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/templates/diffviewer/view_diff.html

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js (Diff revision 4)
The issue has been dropped. Show all issues
```
What if this is empty?
```
1. brennie Feb. 23, 2015, 9:09 a.m.
  It will return the empty string because ''.split('\n') returns [''].

reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js (Diff revision 4)

The issue has been resolved. Show all issues

Can we put the comments before the attributes, like in other models? That helps to prevent having to modify other lines when we add things, in order to line things back up again.

reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
These need to be localized.
Given that, we should just use _.template for this.
```
reviewboard/templates/diffviewer/view_diff.html (Diff revision 4)
The issue has been resolved. Show all issues
```
Can we use underscores in the ID to match the other diff ones?
```

Diff:

Revision 5 (+148)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/views.py
	reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/templates/diffviewer/view_diff.html

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js (Diff revision 5)

The issue has been dropped. Show all issues

Did you test this? I don't think <%- %> can work this way (since it pulls values out of the object passed in to the template). Usually we use names here and then call the template like this:

_tableHeader({
    summaryTitle: gettext('Summary'),
    authorTitle: gettext('Author')
});

brennie Feb. 27, 2015, 5:14 p.m.

Yep, I tested it and it works. It apparently will evaluate whatever is in there and return the result:
>>> _.template('<%- gettext("Author") %>')()
"Author"
>>> _.template('<%- 1 + 1 %>')()
"2"

david Feb. 27, 2015, 5:17 p.m.
```
You learn something new every day.
```

reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
This line is useless because you set it again in update()
```

Commit:

+	9c6d138b3f015eb1745930bb7aa18b1d17a127a2

Diff:

Revision 6 (+143)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/views.py
	reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/templates/diffviewer/view_diff.html

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js (Diff revision 6)
The issue has been resolved. Show all issues
```
These two lines can be combined.
```

reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js (Diff revision 6)

Just to double check, you want it to still take up room on the page? If not, this should use .show()/.hide()

brennie March 3, 2015, 4:53 p.m.

I've changed it to use show() and hide().

Summary:

-	Display commits in the diffviewer based on the selected revision.
+	Display commits in the diffviewer based on the selected revision

Diff:

Revision 7 (+133)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/views.py
	reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/templates/diffviewer/view_diff.html

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js (Diff revisions 6 - 7)
The issue has been resolved. Show all issues
```
Now that you've moved the creation into here, you don't need the .empty() call.
```

Diff:

Revision 8 (+141)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/views.py
	reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/templates/diffviewer/view_diff.html

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Diff:

Revision 9 (+141)

Show changes

	reviewboard/staticbundles.py
	reviewboard/diffviewer/views.py
	reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/templates/diffviewer/view_diff.html

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/views.py
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/templates/diffviewer/view_diff.html
    reviewboard/static/rb/js/diffviewer/collections/diffCommitCollection.js
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to dvcs (ba84d9d)

Display commits in the diffviewer based on the selected revision

Information

Reviewers

Change Summary:

Added Files:

Change Summary:

Depends On:

Diff:

Change Summary:

Branch:

Change Summary:

Diff:

Change Summary:

Diff:

Diff:

Commit:

Diff:

Summary:

Diff:

Diff:

Diff:

Status: Closed (submitted)

Change Summary: