Add infinite scrolling to the diff viewer

Review Request #7047 - Created March 12, 2015 and updated

Mingyi CHEN
Review Board
master
7296819...
reviewboard, students

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.

  • 0
  • 0
  • 37
  • 4
  • 41
Description From Last Updated
Review Bot
  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. 
      
Mingyi CHEN
Review Bot
  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. 
      
Mingyi CHEN
Review Bot
  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. 
      
Mingyi CHEN
Review Bot
  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. 
      
Mingyi CHEN
Review Bot
  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. 
      
Mingyi CHEN
Review Bot
  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. 
      
Mingyi CHEN
Wang Jun Sun
  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. 
      
Mingyi CHEN
Review Bot
  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. 
      
Mingyi CHEN
Vincent Le
  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. 
      
Review Bot
  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. 
      
Mingyi CHEN
Review Bot
  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. 
      
Chris Arnold
  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. 
      
Barret Rennie
  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. 
      
Mingyi CHEN
Review Bot
  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. 
      
Barret Rennie
  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. 
      
Mingyi CHEN
Review Bot
  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. 
      
Mingyi CHEN
Review Bot
  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. 
      
Barret Rennie
  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. 
      
Mingyi CHEN
Review Bot
  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. 
      
Mingyi CHEN
Review Bot
  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. 
      
Christian Hammond
  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. 
      
Mingyi CHEN
Review Bot
  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. 
      
Mingyi CHEN
Review Bot
  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. 
      
Wang Jun Sun
  1. 
      
  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. 
      
Mingyi CHEN
Review request changed

Change Summary:

Fix revision 16 issues.

Commit:

-325a4599d82d8ba6106f0656ac3496a9b8621c5b
+72968191c6c7b9330a6786c635ea4be3e2fd26e6

Diff:

Revision 17 (+121 -48)

Show changes

Review Bot
  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. 
      
Loading...