Summary

Show commit history differences in interdiffs

Review Request #7219 — Created April 17, 2015 and submitted April 23, 2015, 3:17 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

dvcs

Bugs

Depends On

~~7220~~

Commit

1b90346...

Reviewers

Groups

reviewboard

People

Description

The diffviewer now shows the differences between commit histories in a
manner similar to how the change descriptions show differences in the
commit histories between revisions.

Testing Done

Ran unit tests.

Files

Issues

Description	From	Last Updated
Each case should be its own unit test, to help when things change down the road, and to keep this …	chipx86	April 20, 2015, 9:52 p.m.
Blank line between these.	chipx86	April 20, 2015, 9:53 p.m.
Let's raise a ValueError here with a suitable error, to help diagnose issues that may come up down the road.	chipx86	April 20, 2015, 9:54 p.m.
While a minor performance improvement, we should probably pull out history_entry[key] into a local variable so we don't have to …	chipx86	April 20, 2015, 9:54 p.m.
This should be imported a couple lines later.	chipx86	April 20, 2015, 10:05 p.m.
This should also raise a ValueError.	chipx86	April 20, 2015, 9:56 p.m.
We should pull out commit_dict[key] here as well.	chipx86	April 20, 2015, 9:56 p.m.
The list comprehension will be faster, since it doesn't involve a lambda call or a conversion back into a list. …	chipx86	April 20, 2015, 9:59 p.m.
Same here.	chipx86	April 20, 2015, 9:59 p.m.
There should be a blank line before each comment.	chipx86	April 20, 2015, 10 p.m.
This appears to conflict with the value. We should maybe say that if there is no interdiff, the API will …	chipx86	April 20, 2015, 10 p.m.
Space after switch.	chipx86	April 20, 2015, 10:01 p.m.
Blank line between these.	chipx86	April 20, 2015, 10:01 p.m.
Blank line between these.	chipx86	April 20, 2015, 10:01 p.m.
Col: 5 E301 expected 1 blank line, found 0	reviewbot	April 21, 2015, 6:23 a.m.
Should be indented only 4 spaces, not 30	david	April 23, 2015, 3:15 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/commitutils.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/reviews/builtin_fields.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/commitutils.py

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

Added a commit. (Revision 1)

In change descriptions, we highlight a changed commit with green (in the case of added) or red (in the case of removed).

I haven't added this to the diffviewer commits diffs because I think it will be harder to read with the diff icons AND the row backgrounds.

Thoughts?

david April 22, 2015, 12:49 p.m.

What if you surrounded the diff icon with a circle of white?

brennie April 22, 2015, 8:31 p.m.

The current screenshots reflect this change.

Depends On:

+	~~7220 - Refactor CSS for DiffCommit listings~~

reviewboard/diffviewer/tests.py (Diff revision 1)

Show all issues

Each case should be its own unit test, to help when things change down the road, and to keep this from growing longer.

reviewboard/diffviewer/views.py (Diff revision 1)
Show all issues
```
Blank line between these.
```

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

Show all issues

Let's raise a ValueError here with a suitable error, to help diagnose issues that may come up down the road.

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

Show all issues

While a minor performance improvement, we should probably pull out history_entry[key] into a local variable so we don't have to do the lookup three times.

We should do the same with history_entry['type'], since that's used as many as three times as well.

reviewboard/reviews/builtin_fields.py (Diff revision 1)
Show all issues
```
This should be imported a couple lines later.
```
reviewboard/reviews/builtin_fields.py (Diff revision 1)
Show all issues
```
This should also raise a ValueError.
```
reviewboard/reviews/builtin_fields.py (Diff revision 1)
Show all issues
```
We should pull out commit_dict[key] here as well.
```

reviewboard/reviews/builtin_fields.py (Diff revision 1)

Show all issues

The list comprehension will be faster, since it doesn't involve a lambda call or a conversion back into a list.

Also, we should pull out info['old'] into a local variable.

reviewboard/reviews/builtin_fields.py (Diff revision 1)
Show all issues
```
Same here.
```

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

Show all issues

There should be a blank line before each comment.

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

Show all issues

This appears to conflict with the value. We should maybe say that if there is no interdiff, the API will always set this to be "unmodified", or something.

reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js (Diff revision 1)
Show all issues
```
Space after switch.
```
reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js (Diff revision 1)
Show all issues
```
Blank line between these.
```
reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js (Diff revision 1)
Show all issues
```
Blank line between these.
```

Diff:

Revision 2 (+266 -50)

Show changes

	reviewboard/diffviewer/commitutils.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/views.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/commitutils.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/reviews/builtin_fields.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/commitutils.py

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

reviewboard/diffviewer/tests.py (Diff revision 2)
Show all issues
```
Col: 5
 E301 expected 1 blank line, found 0
```

Diff:

Revision 3 (+267 -50)

Show changes

	reviewboard/diffviewer/commitutils.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/views.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/commitutils.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/reviews/builtin_fields.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/commitutils.py

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

Code looks good pending resolution of the design question.

Commit:

-	f074e9b32ce906b52c8671ce75a69897caa31b1f
+	dcc36746ebf09669b1bb75a52f2d8439c05f9941

Diff:

Revision 4 (+273 -51)

Show changes

	reviewboard/diffviewer/commitutils.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/views.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
	reviewboard/static/rb/js/diffviewer/views/diffComplexityIconView.js

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/commitutils.py

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



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/commitutils.py

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

Change Summary:

Add a stroke around the diff complexity icons. Update screenshots.

Commit:

-	dcc36746ebf09669b1bb75a52f2d8439c05f9941
+	494d0cb4d43cab6c4ebc445f12cc99110519a73e

Diff:

Revision 5 (+273 -52)

Show changes

	reviewboard/diffviewer/commitutils.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/views.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
	reviewboard/static/rb/js/diffviewer/views/diffComplexityIconView.js

Added Files:

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/commitutils.py

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



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/commitutils.py

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

Added a commit. (Revision 2)

Should the stroke actually be the background of the table (which is slightly off-white) instead of white?

david April 23, 2015, 2:10 p.m.
```
Yeah, I think so.
```

Change Summary:

Stroke now matches table background and is fetched once dynamically.

Commit:

-	494d0cb4d43cab6c4ebc445f12cc99110519a73e
+	7973736873bfec6dfe3b84daf8177bd30b6147a5

Diff:

Revision 6 (+278 -52)

Show changes

	reviewboard/diffviewer/commitutils.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/views.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
	reviewboard/static/rb/js/diffviewer/views/diffComplexityIconView.js

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/commitutils.py

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



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/commitutils.py

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

Ship it!

reviewboard/static/rb/js/diffviewer/views/diffComplexityIconView.js (Diff revision 6)
Show all issues
```
Should be indented only 4 spaces, not 30
```

Commit:

-	7973736873bfec6dfe3b84daf8177bd30b6147a5
+	1b9034668818aae4c8b305a28df07eaca648f869

Diff:

Revision 7 (+279 -52)

Show changes

	reviewboard/diffviewer/commitutils.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/views.py
	reviewboard/reviews/builtin_fields.py
	reviewboard/static/rb/js/diffviewer/models/diffCommitModel.js
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
	reviewboard/static/rb/js/diffviewer/views/diffComplexityIconView.js

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/commitutils.py

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



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/builtin_fields.py
    reviewboard/diffviewer/views.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/commitutils.py

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

Status: Closed (submitted)

Change Summary:

Pushed to dvcs (8038b31)