Summary

Add infinite scrolling to the diff viewer

Review Request #7047 — Created March 12, 2015 and discarded Jan. 29, 2019, 1:39 p.m.

Information

Owner

mingyi

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard, students

People

Description

Add infinite scrolling to the diff viewer.
When working with long diffs, it's common for the diffs to be split up across

several pages. This can end up being a pain to review, as it means having to

deal with manual pagination.
This change replaces the pagination to scroll loading. When user scroll to the

bottom of the page, diffs will be loaded automatically without having to click

anything.
First bind a listening function to the window to check the position. And when

it detect scroll bar touch the bottom, it will active the loading function to

load next diffs. The loading function use ajax to load pagination's diffs, and

append to the current pagination. Similar to the set, when update the diffs,

properties should update one by one.
Remove the pagination and page selection, and modify the description of 'paginate

by' and 'paginate orphans'.

Testing Done

Passed all js-tests.

Manual tests with a 25 diff files review request.
Paginate by 2, and paginate orphans is 10.

When first load the pages, the page loads 2 diffs, and scroll to the bottom, it
loads 2 another difss. At last, the page loads 10 diffs.

Issues

Description	From	Last Updated
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 26, 2015, 7:48 p.m.
I believe you can just remove the else part.	SU Sunxperous	March 26, 2015, 9:22 p.m.
obejct -> object	VT VTL-Developer	March 27, 2015, 3:26 a.m.
I suppose this should be ===? I think it will be clearer if ($(document).scrollTop() + $(window).height()) == ($(document).height()) is defined …	SU Sunxperous	March 26, 2015, 9:18 p.m.
You can do this.isScrolling without the equality check.	VT VTL-Developer	March 27, 2015, 3:26 a.m.
You don't actually use attrs anywhere in this function except to feature gate emptying the items table. If this is …	brennie	April 4, 2015, 10:31 p.m.
maybe try "Append another page object into the current page so that add(parse(...)) can't be called, due to part of …	CR cristocrat	April 1, 2015, 6:59 p.m.
You're missing a space between that and add. How about "Append another set of diffs into the current page."	brennie	April 8, 2015, 6:55 p.m.
No blank line here.	brennie	April 1, 2015, 6:59 p.m.
Why are you checking this.attributes.property_name instead of attrs.property_name ?	brennie	April 1, 2015, 7:02 p.m.
All var statements should be at the top of the function.	brennie	April 1, 2015, 7:02 p.m.
Clarify what "has the next page" means.	brennie	April 1, 2015, 7:06 p.m.
This seems to be private data that doesn't leave the DiffViewerPageView. This should be _isScrolling in that case.	brennie	April 1, 2015, 7:07 p.m.
Can we rename this to _onScroll or _onWindowScroll or similar?	brennie	April 1, 2015, 7:08 p.m.
maybe try "Listens to a scroll action and check whether it reaches the bottom of the page"	CR cristocrat	April 1, 2015, 6:59 p.m.
This logic is kind of hard to grok. Can we assign the computed window top to a variable and then …	brennie	April 1, 2015, 7:13 p.m.
This is a simplified statement so these should be fine on the same line.	brennie	April 8, 2015, 6:57 p.m.
Only one var statement at the top of the function.	brennie	April 1, 2015, 7:14 p.m.
Isn't the default behaviour to fetch the first page if the page query parameter isn't provided? If that is the …	brennie	April 1, 2015, 10:14 p.m.
Won't there be no more references to the XHR object anyway? Shouldn't it get automatically garbage collected?	brennie	April 8, 2015, 6:52 p.m.
How about "Load the next page of diff content" ?	brennie	April 1, 2015, 10:14 p.m.
maybe try "Use AJAX to load the next pagination page and append..."	CR cristocrat	April 1, 2015, 6:59 p.m.
Again, why won't the GC kick in to get rid of the XHR object?	brennie	April 4, 2015, 10:31 p.m.
We usually do if (positive_case) { // ... } else { // ... } instead of ```javascript if (!positive_case) { …	brennie	April 1, 2015, 10:15 p.m.
See my previous comment about XHR objects getting GC'ed.	brennie	April 4, 2015, 10:31 p.m.
How about "before loading the diff viewer another time." ?	brennie	April 8, 2015, 6:56 p.m.
Remove trailing whitespace.	brennie	April 10, 2015, 12:18 a.m.
Blank line between these.	brennie	April 10, 2015, 12:18 a.m.
Semicolon, not comma.	brennie	April 10, 2015, 12:18 a.m.
Semicolon, not comma.	brennie	April 10, 2015, 12:19 a.m.
We're not actually setting it to null anymore so can we remove this line?	brennie	April 10, 2015, 12:18 a.m.
Doesn't look like this is used.	chipx86	May 1, 2015, 5:40 p.m.
Wrapping window and document is expensive. We should do that once, in render, storing as this._$window and this._$document, so it'll …	chipx86	May 1, 2015, 5:47 p.m.
No need for the parens around the comparison.	chipx86	May 1, 2015, 5:40 p.m.
You can do if (!attrs)	chipx86	May 1, 2015, 5:40 p.m.
Similar to Christian's comment earlier, (!attrs)?	SU Sunxperous	May 2, 2015, 6:11 a.m.
Add a blank line between the two lines, multi-line documentation should have a summary, followed by a blank line, then …	SU Sunxperous	May 2, 2015, 6:12 a.m.
There's an extra *.	SU Sunxperous	May 2, 2015, 6:12 a.m.
Not very sure here, but maybe this can be on the same line.	SU Sunxperous	May 2, 2015, 6:12 a.m.
Why not just assign $diffs = $('#diffs'); in the beginning, and then only call $diffs.empty(); in the if block?	SU Sunxperous	May 2, 2015, 6:13 a.m.
How about nextPage instead of nextpage?	SU Sunxperous	May 2, 2015, 6:14 a.m.

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Change Summary:

Fix the bugs and new bug

Commit:

56e8fe47e4f6a783145c0d6c7d30e607e64fdeba

64999804e50dc7b4dc86a82489ae1da278988a47

Diff:

Revision 2 (+111 -30)

Show changes

	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js

Change Summary:

Rebase

Commit:

64999804e50dc7b4dc86a82489ae1da278988a47

e14bc00ce4893974fba9bffd2f5234ad28aa6f31

Diff:

Revision 3 (+72 -25)

Show changes

	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js

Change Summary:

Fix load Revision bugs

Commit:

e14bc00ce4893974fba9bffd2f5234ad28aa6f31

6cccb05a69c3be3dca284efc75aa13867189b55e

Diff:

Revision 4 (+124 -56)

Show changes

	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js

Change Summary:

Modify description, rebase from master, cut down part of the memory use.

Commit:

6cccb05a69c3be3dca284efc75aa13867189b55e

161290fcc75c70790998d8f8d27b4f257dd84b3a

Diff:

Revision 5 (+115 -48)

Show changes

	reviewboard/admin/forms.py
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Tool: Pyflakes
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js

reviewboard/admin/forms.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Commit:

161290fcc75c70790998d8f8d27b4f257dd84b3a

4a3d0c11eb4c71943ea3107644e3fa917d6343f2

Groups:

students

Diff:

Revision 6 (+115 -48)

Show changes

	reviewboard/admin/forms.py
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Tool: Pyflakes
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js

Change Summary:: Change the summary, description, testing.
Summary:: [WIP] Add infinite scrolling to the diff viewer
Add infinite scrolling to the diff viewer
Description:: ~
[WIP] Scroll Down Automatically Load Next Page
~
Add infinite scrolling to the diff viewer.
+
+
When working with long diffs, it's common for the diffs to be split up across
+ several pages. This can end up being a pain to review, as it means having to
+ deal with manual pagination.
+
+
This change replaces the pagination to scroll loading. When user scroll to the
+ bottom of the page, diffs will be loaded automatically without having to click
+ anything.
+
+
First bind a listening function to the window to check the position. And when
+ it detect scroll bar touch the bottom, it will active the loading function to
+ load next diffs. The loading function use ajax to load pagination's diffs, and
+ append to the current pagination. Similar to the set, when update the diffs,
+ properties should update one by one.
+
+
Remove the pagination and page selection, and modify the description of 'paginate
+ by' and 'paginate orphans'.
Testing Done:: ~
No Test Now.
~
Passed all js-tests.
+
+
Manual tests with a 25 diff files review request.
+ Paginate by 2, and paginate orphans is 10.
+
+
When first load the pages, the page loads 2 diffs, and scroll to the bottom, it
+ loads 2 another difss. At last, the page loads 10 diffs.

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

The issue has been resolved. Show all issues

I believe you can just remove the else part.

MI

mingyi March 26, 2015, 9:22 p.m.
```
you're right~ thanks~:)
```

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

The issue has been resolved. Show all issues

I suppose this should be ===? I think it will be clearer if ($(document).scrollTop() + $(window).height()) == ($(document).height()) is defined a variable first.

MI

mingyi March 26, 2015, 9:18 p.m.
```
Thanks:) Fixed~
```

Commit:

4a3d0c11eb4c71943ea3107644e3fa917d6343f2

dcb842fa256a81fbba71f3cb249cdf05db0dffa1

Diff:

Revision 7 (+115 -48)

Show changes

	reviewboard/admin/forms.py
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js



Tool: Pyflakes
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js

Commit:

dcb842fa256a81fbba71f3cb249cdf05db0dffa1

3134a9a7ddb7a017c128b2e953b800cdfd7f28de

Diff:

Revision 8 (+113 -48)

Show changes

	reviewboard/admin/forms.py
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js

reviewboard/static/rb/js/pages/models/diffViewerPageModel.js (Diff revision 6)
The issue has been resolved. Show all issues
```
obejct -> object
```
1. MI
  
  mingyi March 26, 2015, 9:31 p.m.
  Thanks~A careless bug.~:)
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 6)
```
Is there a reason to set XHR to null?
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
You can do this.isScrolling without the equality check.
```
1. MI
  
  mingyi March 26, 2015, 9:31 p.m.
  Sorry, I don't know what you mean. You mean
  if (this.isScrolling) or delete it or anything else. :)
2. VT
  
  VTL-Developer March 26, 2015, 9:40 p.m.
  Yeah, if (this.isScrolling)
3. MI
  
  mingyi March 27, 2015, 3:25 a.m.
  Yeah, it seems more clear~

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

Same question as above, is there a reason to set XHR to null?

mingyi March 26, 2015, 9:31 p.m.

Emm.. Because the XHR will still be held after it have accomplish the ajax operation. So set it to null will let gc to collect it. That's what I think~:)

VTL-Developer March 26, 2015, 9:39 p.m.

Not that familiar with javascript GC, but you can do delete variable_name

MI

mingyi March 27, 2015, 3:25 a.m.
```
Explicit seems better. Thanks~:)
```

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js



Tool: Pyflakes
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js

Commit:

3134a9a7ddb7a017c128b2e953b800cdfd7f28de

d9a8404e2dd0d761d01842d824a7bdc8403dcafd

Diff:

Revision 9 (+113 -48)

Show changes

	reviewboard/admin/forms.py
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Tool: Pyflakes
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js

Code looks good, just a few wordings suggestions!

reviewboard/static/rb/js/pages/models/diffViewerPageModel.js (Diff revision 9)

The issue has been resolved. Show all issues

maybe try "Append another page object into the current page so that add(parse(...)) can't be called, due to part of the model's property"

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

The issue has been resolved. Show all issues

maybe try "Listens to a scroll action and check whether it reaches the bottom of the page"

brennie April 1, 2015, 1:08 p.m.

This should be written in the imperitive mood (Listen to...)

reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 9)
The issue has been resolved. Show all issues
```
maybe try "Use AJAX to load the next pagination page and append..."
```

reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js (Diff revision 9)

The issue has been dropped. Show all issues

You don't actually use attrs anywhere in this function except to feature gate emptying the items table.
If this is a flag, we should be explicit and call it something like dontEmptyTable and explicitly check if it is not true.

mingyi April 1, 2015, 10:24 p.m.

It's a trigger. And the diffViewerPageView and diffFileIndexView will both be trigged. And the attrs are useful in diffViewerPageView, but only a flag in diffFileIndexView, I keep them as the same form. So... I think keep it is a good idea. :)

reviewboard/static/rb/js/pages/models/diffViewerPageModel.js (Diff revision 9)
The issue has been resolved. Show all issues
```
No blank line here.
```
reviewboard/static/rb/js/pages/models/diffViewerPageModel.js (Diff revision 9)
The issue has been resolved. Show all issues
```
Why are you checking this.attributes.property_name instead of attrs.property_name ?
```
reviewboard/static/rb/js/pages/models/diffViewerPageModel.js (Diff revision 9)
The issue has been resolved. Show all issues
```
All var statements should be at the top of the function.
```
reviewboard/static/rb/js/pages/models/diffViewerPageModel.js (Diff revision 9)
The issue has been resolved. Show all issues
```
Clarify what "has the next page" means.
```

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

The issue has been resolved. Show all issues

This seems to be private data that doesn't leave the DiffViewerPageView. This should be _isScrolling in that case.

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

The issue has been resolved. Show all issues

Can we rename this to _onScroll or _onWindowScroll or similar?

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

The issue has been resolved. Show all issues

This logic is kind of hard to grok.

Can we assign the computed window top to a variable and then do the comparison? Also when we break up an if statment we try to have each condition on a single line, e.g.:

if (really_long_condition_one &&
    really_long_condition_two) {
    // ...
}

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

The issue has been resolved. Show all issues

Only one var statement at the top of the function.

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

The issue has been resolved. Show all issues

Isn't the default behaviour to fetch the first page if the page query parameter isn't provided? If that is the case, we don't need to append &page=1 to the URL all the time.

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

The issue has been resolved. Show all issues

Won't there be no more references to the XHR object anyway? Shouldn't it get automatically garbage collected?

mingyi April 1, 2015, 10:19 p.m.

An artical said js will keep the XHR since it is a long persistent connection. So we should delete it manually.

brennie April 7, 2015, 11:49 a.m.
```
Can you link this article?
```

reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 9)
The issue has been resolved. Show all issues
```
How about "Load the next page of diff content" ?
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 9)
The issue has been dropped. Show all issues
```
Again, why won't the GC kick in to get rid of the XHR object?
```

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

The issue has been resolved. Show all issues

We usually do

if (positive_case) {
    // ...
} else {
    // ...
}


instead of
```javascript

if (!positive_case) {

    // ...

} else {

    // ...

}

reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 9)
The issue has been dropped. Show all issues
```
See my previous comment about XHR objects getting GC'ed.
```

Change Summary:

Fix issues

Commit:

d9a8404e2dd0d761d01842d824a7bdc8403dcafd

0189c5b732259b7844599f08c147e1f2b012cdaa

Diff:

Revision 10 (+117 -48)

Show changes

	reviewboard/admin/forms.py
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Tool: Pyflakes
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js

reviewboard/static/rb/js/pages/models/diffViewerPageModel.js (Diff revisions 9 - 10)
The issue has been resolved. Show all issues
```
You're missing a space between that and add.
How about
"Append another set of diffs into the current page."
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revisions 9 - 10)
The issue has been resolved. Show all issues
```
This is a simplified statement so these should be fine on the same line.
```
reviewboard/admin/forms.py (Diff revision 10)
The issue has been resolved. Show all issues
```
How about "before loading the diff viewer another time." ?
```

Change Summary:

Fix revision 10 issues

Commit:

0189c5b732259b7844599f08c147e1f2b012cdaa

27112153efc69683bc603e2591487c94a8d0e19c

Diff:

Revision 11 (+117 -48)

Show changes

	reviewboard/admin/forms.py
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js



Tool: Pyflakes
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js

Commit:

27112153efc69683bc603e2591487c94a8d0e19c

5fe724d72fc5f472f165b7ed53c422d1916733a6

Diff:

Revision 12 (+112 -47)

Show changes

	reviewboard/admin/forms.py
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Tool: Pyflakes
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js

reviewboard/static/rb/js/pages/models/diffViewerPageModel.js (Diff revision 12)
The issue has been resolved. Show all issues
```
Remove trailing whitespace.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 12)
The issue has been resolved. Show all issues
```
Blank line between these.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 12)
The issue has been resolved. Show all issues
```
Semicolon, not comma.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 12)
The issue has been resolved. Show all issues
```
Semicolon, not comma.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 12)
The issue has been resolved. Show all issues
```
We're not actually setting it to null anymore so can we remove this line?
```

Change Summary:

Fix Revision 12 Issues

Commit:

5fe724d72fc5f472f165b7ed53c422d1916733a6

2a2a19ac4733c26e5267a39e04e5ba2d09829569

Diff:

Revision 13 (+111 -47)

Show changes

	reviewboard/admin/forms.py
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js



Tool: Pyflakes
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js

Change Summary:

Remove extra whitespaces

Diff:

Revision 14 (+111 -47)

Show changes

	reviewboard/admin/forms.py
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js



Tool: Pyflakes
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js

reviewboard/static/rb/js/pages/models/diffViewerPageModel.js (Diff revision 14)
The issue has been resolved. Show all issues
```
Doesn't look like this is used.
```

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

The issue has been resolved. Show all issues

Wrapping window and document is expensive. We should do that once, in render, storing as this._$window and this._$document, so it'll be faster to handle scrolls.
Another way to improve performance is to listen for resize events and cache the window height and document height there, so tha scrolling doesn't need to recompute each time. That would make this function very fast.

reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 14)
The issue has been resolved. Show all issues
```
No need for the parens around the comparison.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 14)
The issue has been resolved. Show all issues
```
You can do if (!attrs)
```

Change Summary:

Fix revision 14 issues

Commit:

2a2a19ac4733c26e5267a39e04e5ba2d09829569

03542e057beb31d9adb7473804d6e148f889d1da

Diff:

Revision 15 (+120 -47)

Show changes

	reviewboard/admin/forms.py
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Tool: Pyflakes
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js

Change Summary:

Rebase from master

Commit:

03542e057beb31d9adb7473804d6e148f889d1da

325a4599d82d8ba6106f0656ac3496a9b8621c5b

Diff:

Revision 16 (+120 -47)

Show changes

	reviewboard/admin/forms.py
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Tool: Pyflakes
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js

1. MI
  
  mingyi May 2, 2015, 8:19 a.m.
  Thanks, Sun~ :)
reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js (Diff revision 16)
The issue has been resolved. Show all issues
```
Similar to Christian's comment earlier, (!attrs)?
```

reviewboard/static/rb/js/pages/models/diffViewerPageModel.js (Diff revision 16)

The issue has been resolved. Show all issues

Add a blank line between the two lines, multi-line documentation should have a summary, followed by a blank line, then the description.

reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 16)
The issue has been resolved. Show all issues
```
There's an extra *.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 16)
The issue has been dropped. Show all issues
```
Not very sure here, but maybe this can be on the same line.
```

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

The issue has been resolved. Show all issues

Why not just assign $diffs = $('#diffs'); in the beginning, and then only call $diffs.empty(); in the if block?

reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 16)
The issue has been resolved. Show all issues
```
How about nextPage instead of nextpage?
```

Change Summary:

Fix revision 16 issues.

Commit:

325a4599d82d8ba6106f0656ac3496a9b8621c5b

72968191c6c7b9330a6786c635ea4be3e2fd26e6

Diff:

Revision 17 (+121 -48)

Show changes

	reviewboard/admin/forms.py
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
	reviewboard/static/rb/js/pages/models/diffViewerPageModel.js
	reviewboard/static/rb/js/pages/views/diffViewerPageView.js

Tool: Pyflakes
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/forms.py

Ignored Files:
    reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.js
    reviewboard/static/rb/js/pages/models/diffViewerPageModel.js

Status:: Discarded

~		[WIP] Scroll Down Automatically Load Next Page
	~	Add infinite scrolling to the diff viewer.
	+
	+	When working with long diffs, it's common for the diffs to be split up across
	+	several pages. This can end up being a pain to review, as it means having to
	+	deal with manual pagination.
	+
	+	This change replaces the pagination to scroll loading. When user scroll to the
	+	bottom of the page, diffs will be loaded automatically without having to click
	+	anything.
	+
	+	First bind a listening function to the window to check the position. And when
	+	it detect scroll bar touch the bottom, it will active the loading function to
	+	load next diffs. The loading function use ajax to load pagination's diffs, and
	+	append to the current pagination. Similar to the set, when update the diffs,
	+	properties should update one by one.
	+
	+	Remove the pagination and page selection, and modify the description of 'paginate
	+	by' and 'paginate orphans'.

~		No Test Now.
	~	Passed all js-tests.
	+
	+	Manual tests with a 25 diff files review request.
	+	Paginate by 2, and paginate orphans is 10.
	+
	+	When first load the pages, the page loads 2 diffs, and scroll to the bottom, it
	+	loads 2 another difss. At last, the page loads 10 diffs.