Add infinite scrolling to the diff viewer

Review Request #7047 — Created March 12, 2015 and discarded

Information

Review Board
master

Reviewers

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'.

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.

Description From Last Updated

Col: 80 E501 line too long (80 > 79 characters)

reviewbotreviewbot

I believe you can just remove the else part.

SU Sunxperous

obejct -> object

VT VTL-Developer

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

SU Sunxperous

You can do this.isScrolling without the equality check.

VT VTL-Developer

You don't actually use attrs anywhere in this function except to feature gate emptying the items table. If this is …

brenniebrennie

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

CR cristocrat

You're missing a space between that and add. How about "Append another set of diffs into the current page."

brenniebrennie

No blank line here.

brenniebrennie

Why are you checking this.attributes.property_name instead of attrs.property_name ?

brenniebrennie

All var statements should be at the top of the function.

brenniebrennie

Clarify what "has the next page" means.

brenniebrennie

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

brenniebrennie

Can we rename this to _onScroll or _onWindowScroll or similar?

brenniebrennie

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

CR cristocrat

This logic is kind of hard to grok. Can we assign the computed window top to a variable and then …

brenniebrennie

This is a simplified statement so these should be fine on the same line.

brenniebrennie

Only one var statement at the top of the function.

brenniebrennie

Isn't the default behaviour to fetch the first page if the page query parameter isn't provided? If that is the …

brenniebrennie

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

brenniebrennie

How about "Load the next page of diff content" ?

brenniebrennie

maybe try "Use AJAX to load the next pagination page and append..."

CR cristocrat

Again, why won't the GC kick in to get rid of the XHR object?

brenniebrennie

We usually do if (positive_case) { // ... } else { // ... } instead of ```javascript if (!positive_case) { …

brenniebrennie

See my previous comment about XHR objects getting GC'ed.

brenniebrennie

How about "before loading the diff viewer another time." ?

brenniebrennie

Remove trailing whitespace.

brenniebrennie

Blank line between these.

brenniebrennie

Semicolon, not comma.

brenniebrennie

Semicolon, not comma.

brenniebrennie

We're not actually setting it to null anymore so can we remove this line?

brenniebrennie

Doesn't look like this is used.

chipx86chipx86

Wrapping window and document is expensive. We should do that once, in render, storing as this._$window and this._$document, so it'll …

chipx86chipx86

No need for the parens around the comparison.

chipx86chipx86

You can do if (!attrs)

chipx86chipx86

Similar to Christian's comment earlier, (!attrs)?

SU Sunxperous

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

SU Sunxperous

There's an extra *.

SU Sunxperous

Not very sure here, but maybe this can be on the same line.

SU Sunxperous

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

SU Sunxperous

How about nextPage instead of nextpage?

SU Sunxperous
reviewbot
  1. 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
    
    
  2. 
      
MI
reviewbot
  1. 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
    
    
  2. 
      
MI
reviewbot
  1. 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
    
    
  2. 
      
MI
reviewbot
  1. 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
    
    
  2. 
      
MI
reviewbot
  1. 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
    
    
  2. reviewboard/admin/forms.py (Diff revision 5)
     
     
    Show all issues
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. 
      
MI
reviewbot
  1. 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
    
    
  2. 
      
MI
SU
  1. 
      
  2. Show all issues

    I believe you can just remove the else part.

    1. you're right~ thanks~:)

  3. 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.

  4. 
      
MI
reviewbot
  1. 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
    
    
  2. 
      
MI
VT
  1. 
      
  2. Show all issues

    obejct -> object

    1. Thanks~A careless bug.~:)

  3. Is there a reason to set XHR to null?

  4. Show all issues

    You can do this.isScrolling without the equality check.

    1. Sorry, I don't know what you mean. You mean
      if (this.isScrolling) or delete it or anything else. :)

    2. Yeah, if (this.isScrolling)

    3. Yeah, it seems more clear~

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

    1. 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~:)

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

    3. Explicit seems better. Thanks~:)

  6. 
      
reviewbot
  1. 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
    
    
  2. 
      
MI
reviewbot
  1. 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
    
    
  2. 
      
CR
  1. Code looks good, just a few wordings suggestions!

  2. 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"

  3. Show all issues

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

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

  4. Show all issues

    maybe try "Use AJAX to load the next pagination page and append..."

  5. 
      
brennie
  1. 
      
  2. 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.

    1. 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. :)

  3. Show all issues

    No blank line here.

  4. Show all issues

    Why are you checking this.attributes.property_name instead of attrs.property_name ?

  5. Show all issues

    All var statements should be at the top of the function.

  6. Show all issues

    Clarify what "has the next page" means.

  7. Show all issues

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

  8. reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 9)
     
     
     
     
     
     
     
     
     
    Show all issues

    Can we rename this to _onScroll or _onWindowScroll or similar?

  9. 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) {
        // ...
    }
    
  10. Show all issues

    Only one var statement at the top of the function.

  11. 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.

  12. Show all issues

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

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

    2. Can you link this article?

  13. Show all issues

    How about "Load the next page of diff content" ?

  14. Show all issues

    Again, why won't the GC kick in to get rid of the XHR object?

  15. Show all issues

    We usually do

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

    instead of

    ```javascript
    if (!positive_case) {
    // ...
    } else {
    // ...
    }

  16. Show all issues

    See my previous comment about XHR objects getting GC'ed.

  17. 
      
MI
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. Show all issues

    You're missing a space between that and add.

    How about

    "Append another set of diffs into the current page."

  3. Show all issues

    This is a simplified statement so these should be fine on the same line.

  4. reviewboard/admin/forms.py (Diff revision 10)
     
     
     
    Show all issues

    How about "before loading the diff viewer another time." ?

  5. 
      
MI
reviewbot
  1. 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
    
    
  2. 
      
MI
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. Show all issues

    Remove trailing whitespace.

  3. Show all issues

    Blank line between these.

  4. Show all issues

    Semicolon, not comma.

  5. Show all issues

    Semicolon, not comma.

  6. Show all issues

    We're not actually setting it to null anymore so can we remove this line?

  7. 
      
MI
reviewbot
  1. 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
    
    
  2. 
      
MI
reviewbot
  1. 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
    
    
  2. 
      
chipx86
  1. 
      
  2. Show all issues

    Doesn't look like this is used.

  3. 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.

  4. Show all issues

    No need for the parens around the comparison.

  5. Show all issues

    You can do if (!attrs)

  6. 
      
MI
reviewbot
  1. 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
    
    
  2. 
      
MI
reviewbot
  1. 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
    
    
  2. 
      
SU
  1. 
      
    1. Thanks, Sun~ :)

  2. Show all issues

    Similar to Christian's comment earlier, (!attrs)?

  3. 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.

  4. Show all issues

    There's an extra *.

  5. Show all issues

    Not very sure here, but maybe this can be on the same line.

  6. reviewboard/static/rb/js/pages/views/diffViewerPageView.js (Diff revision 16)
     
     
     
     
     
     
     
     
     
    Show all issues

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

  7. Show all issues

    How about nextPage instead of nextpage?

  8. 
      
MI
reviewbot
  1. 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
    
    
  2. 
      
david
Review request changed
Status:
Discarded