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)
     
     
    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. I believe you can just remove the else part.

    1. you're right~ thanks~:)

  3. 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. 
      
    1. Thanks~A careless bug.~:)

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

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

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

  5. 
      
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. 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. 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. maybe try "Use AJAX to load the next pagination page and append..."

  5. 
      
brennie
  1. 
      
  2. 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. Why are you checking this.attributes.property_name instead of attrs.property_name ?

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

  5. Clarify what "has the next page" means.

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

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

    Can we rename this to _onScroll or _onWindowScroll or similar?

  8. 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) {
        // ...
    }
    
  9. Only one var statement at the top of the function.

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

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

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

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

  14. We usually do

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

    instead of

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

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

  16. 
      
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. You're missing a space between that and add.

    How about

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

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

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

    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. Remove trailing whitespace.

  3. Blank line between these.

  4. Semicolon, not comma.

  5. Semicolon, not comma.

  6. 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. Doesn't look like this is used.

  3. 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. No need for the parens around the comparison.

  5. 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. Similar to Christian's comment earlier, (!attrs)?

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

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

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

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

  6. How about nextPage instead of nextpage?

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

Loading...