Summary

Add complexity icons to the commits in the diffviewer.

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

Information

Owner

brennie

Repository

Review Board

Branch

dvcs

Bugs

Depends On

~~6933~~, ~~6934~~

Reviewers

Groups

reviewboard

People

Description

The DiffCommitIndexView now renders complexity icons via the
DiffComplexityIconView that the DiffIndexView also uses.

Testing Done

Diff complexity icons appear next to commits in the
diffviewer's commit list.

Issues

Description	From	Last Updated
Does prefetch_related do what you want?	david	Feb. 25, 2015, 12:03 p.m.
Comments here are a little weirdly aligned.	david	Feb. 24, 2015, 9:20 a.m.
Because this is javascript land, we actually need to do this wacky thing: for (lineType in this.attributes.lineCounts) { if (this.attributes.lineCounts.hasOwnProperty(lineType)) …	david	Feb. 24, 2015, 9:24 a.m.
We already have much of this and the one below. Let's pull those out into something reusable. The review request …	chipx86	Feb. 27, 2015, 10:27 a.m.
Can we put the comments on the lines before the attributes? Helps to allow more verbose comments and to prevent …	chipx86	Feb. 27, 2015, 10:22 a.m.
Just a thing to consider: I'm not 100% sure, but I suspect that an object doing diffcommit.get('lineCounts').deletes = 5 could …	chipx86	Feb. 27, 2015, 10:20 a.m.
I we pull out lineCounts into a local variable, we'll reduce the number of property accesses required.	chipx86	Feb. 27, 2015, 10:22 a.m.
Indentation is off here.	david	March 3, 2015, 2:23 p.m.
I think this should be defined inside the this.collection.each() callback function.	david	March 3, 2015, 2:21 p.m.

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

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
    reviewboard/static/rb/css/pages/reviews.less



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

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
    reviewboard/static/rb/css/pages/reviews.less

Change Summary:

Rebased off updated r/6933

Diff:

Revision 2 (+63 -2)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js

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

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
    reviewboard/static/rb/css/pages/reviews.less



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

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
    reviewboard/static/rb/css/pages/reviews.less

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

The issue has been resolved. Show all issues

Does prefetch_related do what you want?

brennie Feb. 24, 2015, 10:39 a.m.

Not really. As the diffset object has already been fetched, it doesn't have the ability (as far as I can see) to fetch the related objects. I believe it would have tbe done wherever the diffset is fetched.

brennie Feb. 25, 2015, 12:03 p.m.

Actually it turns out this is exactly what I want. I was just attempting to use it in the wrong way.

reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Comments here are a little weirdly aligned.
```

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

The issue has been resolved. Show all issues

Because this is javascript land, we actually need to do this wacky thing:

for (lineType in this.attributes.lineCounts) {
    if (this.attributes.lineCounts.hasOwnProperty(lineType)) {
        sum += this.attributes.lineCounts[lineType];
    }
}


Alternatively, _.each will do the right thing when iterating over object properties.

Change Summary:

Address David's issues.

Testing Done:

	+	Diff complexity icons appear next to commits in the
	+	diffviewer's commit list.

Diff:

Revision 3 (+69 -6)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js

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

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
    reviewboard/static/rb/css/pages/reviews.less



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

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
    reviewboard/static/rb/css/pages/reviews.less

reviewboard/static/rb/css/pages/reviews.less (Diff revision 3)

The issue has been resolved. Show all issues

We already have much of this and the one below. Let's pull those out into something reusable.

The review request page should be pulling in diffviewer.less, so you can just have those all in one place there.

brennie Feb. 27, 2015, 10:27 a.m.

These aren't actually used in the review request page, so I removed them and references to them. Putting complexity icons on the review request page will be another change.

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

The issue has been resolved. Show all issues

Can we put the comments on the lines before the attributes? Helps to allow more verbose comments and to prevent annoyances when we add new attributes (which may have longer names or defaults).

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

The issue has been resolved. Show all issues

Just a thing to consider:

I'm not 100% sure, but I suspect that an object doing diffcommit.get('lineCounts').deletes = 5 could end up overwriting the defaults for other instances. I don't know if Backbone makes a copy of dictionary values when populating from defaults. Just something to think about for any future changes.

brennie Feb. 27, 2015, 10:20 a.m.

As it turns out, it will not copy and can overwrite defaults for other instances.

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

The issue has been resolved. Show all issues

I we pull out lineCounts into a local variable, we'll reduce the number of property accesses required.

Diff:

Revision 4 (+72 -14)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js

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

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js



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

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js

reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Indentation is off here.
```
reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
I think this should be defined inside the this.collection.each() callback function.
```

Change Summary:

Address David's issues.

Diff:

Revision 5 (+69 -13)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js

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

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js



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

Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to dvcs (22e2cef)