Summary

Don't reload the diff viewer when changing revisions.

Review Request #4630 — Created Sept. 22, 2013 and submitted Sept. 23, 2013, 10:12 p.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Don't reload the diff viewer when changing revisions.
This change adds a new API resource called "diff-context" which can take the

same revision/interdiff parameters as the diffviewer view, but will return only

the JSON data needed to replace the page.
This is wired up to some AJAX on the client side which will take the new

revision and change out all the data on the models. A backbone router will make

sure that the URL and browser history get appropriately updated.
At the moment this can be a little annoying because our UI jumps around when

things like the current-revision label get replaced. I'll be reworking some of

these UI elements to make sure that they use up the same amount of vertical

space for different diff revisions.
The next major steps, in addition to preventing things from jumping around, are

a redesign of the revision selector widget, and a redesign of how we do

pagination.

Testing Done

Loaded a bunch of different diff and interdiff revisions. Verified that all of
the appropriate UI elements (file index, files, revision label, revision
selector, comments hint) were updated in-place. Checked that pagination still
worked.

Issues

Description	From	Last Updated
'WebAPIResponse' imported but unused	reviewbot	Sept. 22, 2013, 8:40 a.m.
'webapi_response_errors' imported but unused	reviewbot	Sept. 22, 2013, 8:40 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Sept. 22, 2013, 8:39 a.m.
Col: 80 E501 line too long (84 > 79 characters)	reviewbot	Sept. 22, 2013, 8:40 a.m.
undefined name 'Http404'	reviewbot	Sept. 22, 2013, 8:40 a.m.
Trailing comma.	chipx86	Sept. 22, 2013, 12:46 p.m.
var parts	mike_conley	Sept. 22, 2013, 12:46 p.m.
Trailing comma.	chipx86	Sept. 22, 2013, 12:47 p.m.
Can you go into more detail here, for the docs? I know it's meant for internal use, but we should …	chipx86	Sept. 22, 2013, 1:25 p.m.
Missing a period.	chipx86	Sept. 22, 2013, 12:49 p.m.
Needs to be revision, or it'll make Sphinx sad. (foo the beginning of a reference.)	chipx86	Sept. 22, 2013, 12:49 p.m.
Can you add a docstring here as well, that goes into how this is used?	chipx86	Sept. 22, 2013, 1:25 p.m.
Let's just pull this out so it doesn't have to be redefined every time.	chipx86	Sept. 22, 2013, 12:49 p.m.
Can you format this like the other resources? Also, this should have the same key as the resource: return 200, …	chipx86	Sept. 22, 2013, 12:50 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Sept. 22, 2013, 1:33 p.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	Sept. 22, 2013, 1:33 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Sept. 22, 2013, 1:33 p.m.
Col: 17 E126 continuation line over-indented for hanging indent	reviewbot	Sept. 22, 2013, 1:33 p.m.
Not sure we want to mention the JavaScript class in this comment. It doesn't really make sense in the API …	chipx86	Sept. 23, 2013, 9:27 p.m.
""" on the next line.	chipx86	Sept. 23, 2013, 9:27 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Sept. 22, 2013, 1:35 p.m.
""" on the next line.	chipx86	Sept. 23, 2013, 10:09 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Sept. 23, 2013, 9:33 p.m.
This should still probably go in the main resource doc description.	chipx86	Sept. 23, 2013, 10:10 p.m.
""" on the next line.	chipx86	Sept. 23, 2013, 10:10 p.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	Sept. 23, 2013, 10:10 p.m.

This is a review from Review Bot.
Tool: PEP8 Style Checker
Processed Files:
reviewboard/webapi/resources/diff_context.py
reviewboard/webapi/resources/review_request.py
Ignored Files:
reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/webapi/resources/diff_context.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80

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

 E501 line too long (84 > 79 characters)
```

This is a review from Review Bot.
Tool: Pyflakes
Processed Files:
reviewboard/webapi/resources/diff_context.py
reviewboard/webapi/resources/review_request.py
Ignored Files:
reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/webapi/resources/diff_context.py (Diff revision 1)
The issue has been resolved. Show all issues
```
'WebAPIResponse' imported but unused
```
reviewboard/webapi/resources/diff_context.py (Diff revision 1)
The issue has been resolved. Show all issues
```
'webapi_response_errors' imported but unused
```
reviewboard/webapi/resources/diff_context.py (Diff revision 1)
The issue has been resolved. Show all issues
```
undefined name 'Http404'
```

Diff:

Revision 2 (+109 -11)

Show changes

	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/webapi/resources/diff_context.py
	reviewboard/webapi/resources/review_request.py

reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
var parts
```

reviewboard/webapi/resources/diff_context.py (Diff revision 2)

Out of curiosity, is there an advantage to hiding this class in here, beyond making it impossible to import?

This also needs a doc file and an entry in resource-tree.txt.

reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Trailing comma.
```

reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 2)

Totally optional, but just a thought. If you don't want to use _.bind later (which is safe enough -- that'll just do the native bind() on a function on modern browsers anyway), you could pull out this.model instead of this and just reference that below.

reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Trailing comma.
```

reviewboard/webapi/resources/diff_context.py (Diff revision 2)

The issue has been resolved. Show all issues

Can you go into more detail here, for the docs? I know it's meant for internal use, but we should really flesh these out so it's clear if you don't have the source file in front of you.

reviewboard/webapi/resources/diff_context.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing a period.
```
reviewboard/webapi/resources/diff_context.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Needs to be revision, or it'll make Sphinx sad. (foo the beginning of a reference.)
```
reviewboard/webapi/resources/diff_context.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can you add a docstring here as well, that goes into how this is used?
```
reviewboard/webapi/resources/diff_context.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Let's just pull this out so it doesn't have to be redefined every time.
```

reviewboard/webapi/resources/diff_context.py (Diff revision 2)

The issue has been resolved. Show all issues

Can you format this like the other resources? Also, this should have the same key as the resource:
return 200, {
    self.item_result_key: context['diff_context'],
}

Diff:

Revision 3 (+130 -10)

Show changes

	docs/manual/webapi/2.0/resources/diff-context.txt
	docs/manual/webapi/2.0/resources/index.txt
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/webapi/resources/diff_context.py
	reviewboard/webapi/resources/review_request.py

This is a review from Review Bot.
Tool: PEP8 Style Checker
Processed Files:
reviewboard/webapi/resources/diff_context.py
reviewboard/webapi/resources/review_request.py
Ignored Files:
docs/manual/webapi/2.0/resources/index.txt
docs/manual/webapi/2.0/resources/diff-context.txt
reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/webapi/resources/diff_context.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 80

 E501 line too long (82 > 79 characters)
```
reviewboard/webapi/resources/diff_context.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 17

 E126 continuation line over-indented for hanging indent
```

This is a review from Review Bot.
Tool: Pyflakes
Processed Files:
reviewboard/webapi/resources/diff_context.py
reviewboard/webapi/resources/review_request.py
Ignored Files:
docs/manual/webapi/2.0/resources/index.txt
docs/manual/webapi/2.0/resources/diff-context.txt
reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Change Summary:

Hide the example from the diff-context documentation page.

Diff:

Revision 4 (+131 -10)

Show changes

	docs/manual/webapi/2.0/resources/diff-context.txt
	docs/manual/webapi/2.0/resources/index.txt
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/webapi/resources/diff_context.py
	reviewboard/webapi/resources/review_request.py

This is a review from Review Bot.
Tool: PEP8 Style Checker
Processed Files:
reviewboard/webapi/resources/diff_context.py
reviewboard/webapi/resources/review_request.py
Ignored Files:
docs/manual/webapi/2.0/resources/index.txt
docs/manual/webapi/2.0/resources/diff-context.txt
reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/webapi/resources/diff_context.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 80

 E501 line too long (82 > 79 characters)
```
reviewboard/webapi/resources/diff_context.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 17

 E126 continuation line over-indented for hanging indent
```

This is a review from Review Bot.
Tool: Pyflakes
Processed Files:
reviewboard/webapi/resources/diff_context.py
reviewboard/webapi/resources/review_request.py
Ignored Files:
docs/manual/webapi/2.0/resources/index.txt
docs/manual/webapi/2.0/resources/diff-context.txt
reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Change Summary:

Make ReviewBot happ(y|ier)

Diff:

Revision 5 (+131 -10)

Show changes

	docs/manual/webapi/2.0/resources/diff-context.txt
	docs/manual/webapi/2.0/resources/index.txt
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/webapi/resources/diff_context.py
	reviewboard/webapi/resources/review_request.py

This is a review from Review Bot.
Tool: PEP8 Style Checker
Processed Files:
reviewboard/webapi/resources/diff_context.py
reviewboard/webapi/resources/review_request.py
Ignored Files:
docs/manual/webapi/2.0/resources/index.txt
docs/manual/webapi/2.0/resources/diff-context.txt
reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/webapi/resources/diff_context.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 80

 E501 line too long (82 > 79 characters)
```

This is a review from Review Bot.
Tool: Pyflakes
Processed Files:
reviewboard/webapi/resources/diff_context.py
reviewboard/webapi/resources/review_request.py
Ignored Files:
docs/manual/webapi/2.0/resources/index.txt
docs/manual/webapi/2.0/resources/diff-context.txt
reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/webapi/resources/diff_context.py (Diff revision 5)

The issue has been resolved. Show all issues

Not sure we want to mention the JavaScript class in this comment. It doesn't really make sense in the API docs. It's more of an implementation detail on our end (even if this is considered more of an internally used API).
Given that this is the main description for the resource, I think we should move the bit about how this is internal into here.

reviewboard/webapi/resources/diff_context.py (Diff revision 5)
The issue has been resolved. Show all issues
```
""" on the next line.
```

Diff:

Revision 6 (+131 -10)

Show changes

	docs/manual/webapi/2.0/resources/diff-context.txt
	docs/manual/webapi/2.0/resources/index.txt
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/webapi/resources/diff_context.py
	reviewboard/webapi/resources/review_request.py

This is a review from Review Bot.
Tool: PEP8 Style Checker
Processed Files:
reviewboard/webapi/resources/diff_context.py
reviewboard/webapi/resources/review_request.py
Ignored Files:
docs/manual/webapi/2.0/resources/index.txt
docs/manual/webapi/2.0/resources/diff-context.txt
reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/webapi/resources/diff_context.py (Diff revision 6)
The issue has been dropped. Show all issues
```
Col: 80

 E501 line too long (82 > 79 characters)
```

This is a review from Review Bot.
Tool: Pyflakes
Processed Files:
reviewboard/webapi/resources/diff_context.py
reviewboard/webapi/resources/review_request.py
Ignored Files:
docs/manual/webapi/2.0/resources/index.txt
docs/manual/webapi/2.0/resources/diff-context.txt
reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/webapi/resources/diff_context.py (Diff revision 6)
The issue has been resolved. Show all issues
```
""" on the next line.
```
reviewboard/webapi/resources/diff_context.py (Diff revision 6)
The issue has been resolved. Show all issues
```
This should still probably go in the main resource doc description.
```
reviewboard/webapi/resources/diff_context.py (Diff revision 6)
The issue has been resolved. Show all issues
```
""" on the next line.
```

Diff:

Revision 7 (+133 -10)

Show changes

	docs/manual/webapi/2.0/resources/diff-context.txt
	docs/manual/webapi/2.0/resources/index.txt
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js
	reviewboard/webapi/resources/diff_context.py
	reviewboard/webapi/resources/review_request.py

This is a review from Review Bot.
Tool: PEP8 Style Checker
Processed Files:
reviewboard/webapi/resources/diff_context.py
reviewboard/webapi/resources/review_request.py
Ignored Files:
docs/manual/webapi/2.0/resources/index.txt
docs/manual/webapi/2.0/resources/diff-context.txt
reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/webapi/resources/diff_context.py (Diff revision 7)
The issue has been dropped. Show all issues
```
Col: 80

 E501 line too long (82 > 79 characters)
```

This is a review from Review Bot.
Tool: Pyflakes
Processed Files:
reviewboard/webapi/resources/diff_context.py
reviewboard/webapi/resources/review_request.py
Ignored Files:
docs/manual/webapi/2.0/resources/index.txt
docs/manual/webapi/2.0/resources/diff-context.txt
reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Status:: Completed
Change Summary:: Pushed to master (ea37d8f).