Diff fragments in review comments can now be expanded and collapsed.

Review Request #6380 — Created Sept. 28, 2014 and submitted

Information

Review Board
master
41ef997...

Reviewers

Diff fragments can now be expanded in three ways:
- expanded above or below the current context by a fixed number of
lines (20);
- expanded up to the previous header (i.e., method definition); or
- expanded completely above or below.
After a diff fragment is expanded in one direction, it can be
collapsed via a floating button that follows the viewport. If there is
still more of the diff to be expanded, it may be expanded again.

The comment_diff_fragments view now takes an optional URL query
parameter, lines-of-context, which indicates the number of lines
above and below that should be included in the fragment. Likewise,
the build_diff_comment_fragments now takes a lines_of_context
parameter and uses that to calculate the first_line and
num_lines parameters for get_file_chunks_in_range.

The activity indicator functionality in RB.apiCall (in
apiUtils.js) has been refactored into another method in the same
file: RB.setActivityIndicator. Asynchronous activity that does not
involve API calls can use this method to enable or disable the
activity indicator.

Add the expand_fragment_link and expand_fragment_header_link
Django template tags. These tags create expansion links for use in
diff fragments. These functions use a new template:
reviews/expand_link.html.

Added Brian Cherne's hoverIntent jQuery plugin to the 3rd party static
bundles.

The DiffFragmentQueueView (diffFragmentQueueView.js) now handles
the expansion and contraction of diff fragments. This view monitors
for clicks on expansion and contraction buttons and dynamically adds
a script tag which will replace the contents of the appropriate diff
fragment with the same fragment, but with expanded (or contracted)
context. The activity indicator is turned on when a diff fragment is
requested and off when the requested fragment has been received.

The expansion links for diff fragments are only shown when the mouse
is hovering over the comment container for that fragment. Once the
mouse moves off the comment container, the expansion links will become
hidden. The hoverIntent jQuery plugin ensures that clumsy mouse
movements will not result in the controls toggling on and off
repeatedly.

The ReviewBoxListView now constructs the DiffFragmentQueueView
with an element (#container) on which it should listen for events.

The diff fragment template (diff_comment_fragment.html) has been
modified to only show 2 columns (instead of 4) when the file in the
diff is a new file. This change makes the diff comment fragment viewer
look more similar to the diffviewer.

When a diff fragment is expanded to have context, the lines that have
been flagged for review will be surrounded above and below by a gray
border.

Unit tests pass.

Verified the following in Chrome and Firefox:

  • Expansion links are only visible if the diff fragment can be
    expanded in that direction.
  • When a review request is loaded, the expansion controls are hidden.
  • Upon hovering over a diff fragment, the expansion controls expand.
  • Upon stopping hovering over a diff fragment, this is a delay before
    the expansion controls become hidden again.
  • Upon stopping hovering over a diff fragment, if the mouse hovers
    over the diff fragment again before the delay expires, the
    expansion controls do not become hidden.
  • Clicking on the "expand by 20" lines buttons expands the context by
    20 lines in the appropriate direction.
  • Click on the expand to header button expands to that header.
  • Expanding multiple times expands upon the current context.
  • The collapse button smoothly follows scrolling, staying in the
    centre of the screen (if the fragment takes up the whole view) or
    the centre of the fragment (if the fragment does not take up the
    whole view).
  • Clicking the collapse button makes all context hidden and removes
    the collapse button.
  • The lines originally flagged for review have a border around them.
  • When the file is a new file, only one line number column is
    visible.
  • If upon contracting a diff fragment the mouse is no longer above
    the fragment for a period, the diff fragment's controls will become
    hidden.

Also verified the following:

  • An invalid lines_of_context query parameter is interpreted as
    0 lines of context above and below
  • If too many lines_of_context query parameter values are passed
    (e.g., 0,0,0,0), it is interpreted as if only the first two
    values were present.
  • If the lines_of_context query parameter is passed with no value,
    it is interpreted as 0,0
  • If the lines_of_context query parameter is passed with only one
    value, it is interpreted as both the above and below context.

Description From Last Updated

I think the thickness of the border is a little much, was 1px not visible enough?

SM smacleod

list comprehension redefines 'file_attachment' from line 587

reviewbotreviewbot

list comprehension redefines 'file_attachment' from line 588

reviewbotreviewbot

list comprehension redefines 'file_attachment' from line 588

reviewbotreviewbot

Col: 24 E231 missing whitespace after ','

reviewbotreviewbot

list comprehension redefines 'file_attachment' from line 588

reviewbotreviewbot

list comprehension redefines 'file_attachment' from line 588

reviewbotreviewbot

list comprehension redefines 'file_attachment' from line 589

reviewbotreviewbot

list comprehension redefines 'file_attachment' from line 590

reviewbotreviewbot

'mark_safe' imported but unused

reviewbotreviewbot

'render_markdown' imported but unused

reviewbotreviewbot

Col: 25 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 52 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 52 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Add a trailing comma.

daviddavid

If you call mark_safe() here, you can avoid the |safe in the template. This should probably also be named something …

daviddavid

Dictionary entries in python should always end in a trailing comma.

daviddavid

Add a trailing comma.

daviddavid

This should probably be wrapped in a try/except in order to avoid 500s if a bogus query string is passed.

daviddavid

Should have a trailing comma.

daviddavid

Can we add a non-minified version of this instead? When we compile it we'll minify, but having the original source …

daviddavid

Where do these numbers come from?

daviddavid

Can you name this $activityIndicator?

daviddavid

Single-line comments (other than those used as method comments) should use //

daviddavid

All blocks should use {}

daviddavid

We'd typically indent this as: $chunks = $button .closest('.sidebyside') .children('tbody') .not('.diff-header');

daviddavid

Single-line comments should use //. Here and below.

daviddavid

These should be on the same line.

daviddavid

These should be on the same line.

daviddavid

setActivityIndicator checks RB.ajaxOptions.enableIndicator so you shouldn't need the conditional.

daviddavid

Should have a period at the end.

daviddavid

{}

daviddavid

typo: finishd

daviddavid

You shouldn't need the conditional here because setActivityIndicator checks RB.ajaxOptions.enableIndicator.

daviddavid

Variable definitions should happen at the top of the method.

daviddavid

Multi-line comments should be formatted as: /* * text */

daviddavid

Same here re: formatting. Also should end in a period.

daviddavid

Can you revert this change?

daviddavid

Just to be safe, how about we escape the 'text' field here? You can use format_html to create HTML and …

daviddavid

Generally, in Python it's best to avoid lists in default arguments because any modification to that variable can end up …

daviddavid

What happens if this URL is loaded with lines_of_context= ?

daviddavid

This should use an === comparison.

daviddavid

Should have a blank /* on the first line of the comment.

daviddavid

You don't need to pass $expanded in to this--it will be available in the closure.

daviddavid

Variable definitions should all be at the top of their method.

daviddavid

Blocks should always use {}

daviddavid

Col: 33 E225 missing whitespace around operator

reviewbotreviewbot

You don't need to call escape(text) because format_html will escape any arguments that aren't already safe.

daviddavid

Can you add an "else: raise ValueError" at the end of this?

daviddavid
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    
  2. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
     list comprehension redefines 'file_attachment' from line 587
    
    1. I didn't modify this line.

  3. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    
  2. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Show all issues
     list comprehension redefines 'file_attachment' from line 588
    
    1. Again, I did not modify this line.

  3. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    
  2. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Show all issues
     list comprehension redefines 'file_attachment' from line 588
    
  3. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    
  2. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
    Col: 24
     E231 missing whitespace after ','
    
  3. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
     list comprehension redefines 'file_attachment' from line 588
    
  4. 
      
brennie
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    
  2. reviewboard/reviews/views.py (Diff revision 5)
     
     
    Show all issues
     list comprehension redefines 'file_attachment' from line 588
    
  3. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
  2. reviewboard/reviews/views.py (Diff revision 6)
     
     
    Show all issues
     list comprehension redefines 'file_attachment' from line 589
    
  3. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
  2. reviewboard/reviews/views.py (Diff revision 7)
     
     
    Show all issues
     list comprehension redefines 'file_attachment' from line 590
    
  3. 
      
brennie
brennie
brennie
brennie
brennie
RM
  1. Ship It!

  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/templates/diffviewer/expand_link.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/templates/diffviewer/expand_link.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
  2. Show all issues
     'mark_safe' imported but unused
    
  3. Show all issues
     'render_markdown' imported but unused
    
  4. Show all issues
    Col: 25
     E128 continuation line under-indented for visual indent
    
  5. reviewboard/reviews/views.py (Diff revision 13)
     
     
    Show all issues
    Col: 52
     E128 continuation line under-indented for visual indent
    
  6. reviewboard/reviews/views.py (Diff revision 13)
     
     
    Show all issues
    Col: 52
     E128 continuation line under-indented for visual indent
    
  7. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/templates/diffviewer/expand_link.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/templates/diffviewer/expand_link.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/static/rb/js/utils/collapseButtonManager.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/lib/js/jquery.hoverIntent.min.js
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/templates/diffviewer/expand_link.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/static/rb/js/utils/collapseButtonManager.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/lib/js/jquery.hoverIntent.min.js
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/templates/diffviewer/expand_link.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
  2. 
      
brennie
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/static/rb/js/utils/collapseButtonManager.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/lib/js/jquery.hoverIntent.min.js
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/templates/diffviewer/expand_link.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/static/rb/js/utils/collapseButtonManager.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/lib/js/jquery.hoverIntent.min.js
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/templates/diffviewer/expand_link.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
  2. 
      
SM
  1. This is looking great. I haven't looked through the code yet, just thought I'd drop some quick visual feedback.

  2. Show all issues

    I think the thickness of the border is a little much, was 1px not visible enough?

  3. 
      
brennie
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/static/rb/js/utils/collapseButtonManager.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/lib/js/jquery.hoverIntent.min.js
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/templates/diffviewer/expand_link.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/static/rb/js/utils/collapseButtonManager.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/lib/js/jquery.hoverIntent.min.js
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/templates/diffviewer/expand_link.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/static/rb/js/utils/collapseButtonManager.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/lib/js/jquery.hoverIntent.min.js
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/templates/diffviewer/expand_link.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/static/rb/js/utils/collapseButtonManager.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/lib/js/jquery.hoverIntent.min.js
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/templates/diffviewer/expand_link.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/lib/js/jquery.hoverIntent.min.js
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/templates/diffviewer/expand_link.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/static/lib/js/jquery.hoverIntent.min.js
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/templates/diffviewer/expand_link.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
  2. 
      
david
  1. I'd like a lot more detail on the test cases that you ran through, especially listing out any edge cases that you've tested (for example, what about comments at the beginning of a file? The end? A comment on a file that's only a single line?)

  2. Show all issues

    Add a trailing comma.

  3. Show all issues

    If you call mark_safe() here, you can avoid the |safe in the template. This should probably also be named something like 'text_html' in order to clarify that it's HTML and not plain text.

  4. Show all issues

    Dictionary entries in python should always end in a trailing comma.

  5. reviewboard/reviews/views.py (Diff revision 19)
     
     
    Show all issues

    Add a trailing comma.

  6. reviewboard/reviews/views.py (Diff revision 19)
     
     
     
    Show all issues

    This should probably be wrapped in a try/except in order to avoid 500s if a bogus query string is passed.

  7. reviewboard/reviews/views.py (Diff revision 19)
     
     
    Show all issues

    Should have a trailing comma.

  8. Show all issues

    Can we add a non-minified version of this instead? When we compile it we'll minify, but having the original source will make it easier to debug for development.

    I'm planning on migrating more of our 3rd-party code to using non-minified sources in the future.

  9. Show all issues

    Where do these numbers come from?

    1. The width of the button is 28px. The <th> has a 1em left padding and and a 4px left padding (@linenum-padding-right). I've added a comment to the less in the latest diff and changed it to use @linenum-padding-right instead of 4px.

  10. Show all issues

    Can you name this $activityIndicator?

  11. Show all issues

    Single-line comments (other than those used as method comments) should use //

  12. Show all issues

    All blocks should use {}

  13. Show all issues

    We'd typically indent this as:

    $chunks = $button
        .closest('.sidebyside')
        .children('tbody')
        .not('.diff-header');
    
  14. Show all issues

    Single-line comments should use //. Here and below.

  15. Show all issues

    These should be on the same line.

  16. Show all issues

    These should be on the same line.

  17. Show all issues

    setActivityIndicator checks RB.ajaxOptions.enableIndicator so you shouldn't need the conditional.

  18. Show all issues

    Should have a period at the end.

  19. Show all issues

    {}

  20. Show all issues

    typo: finishd

  21. Show all issues

    You shouldn't need the conditional here because setActivityIndicator checks RB.ajaxOptions.enableIndicator.

  22. Show all issues

    Variable definitions should happen at the top of the method.

  23. Show all issues

    Multi-line comments should be formatted as:

    /*
     * text
     */
    
  24. Show all issues

    Same here re: formatting. Also should end in a period.

  25. Show all issues

    Can you revert this change?

  26. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/lib/js/jquery.hoverIntent.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/lib/js/jquery.hoverIntent.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
  2. 
      
david
  1. 
      
  2. Show all issues

    Just to be safe, how about we escape the 'text' field here? You can use format_html to create HTML and escape all in one go.

  3. reviewboard/reviews/views.py (Diff revision 20)
     
     
    Show all issues

    Generally, in Python it's best to avoid lists in default arguments because any modification to that variable can end up changing your default argument for the next call. The usual pattern is:

    def method(x=None):
        if x is None:
            x = [...]
    

    That way you can later modify x without causing any strange bugs.

  4. reviewboard/reviews/views.py (Diff revision 20)
     
     
    Show all issues

    What happens if this URL is loaded with lines_of_context= ?

    1. Ive changed this in the latest diff. I removed the if and made it always create a valid lines_of_context value

  5. Show all issues

    This should use an === comparison.

  6. Show all issues

    Should have a blank /* on the first line of the comment.

  7. Show all issues

    You don't need to pass $expanded in to this--it will be available in the closure.

  8. Show all issues

    Variable definitions should all be at the top of their method.

  9. Show all issues

    Blocks should always use {}

  10. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/lib/js/jquery.hoverIntent.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/lib/js/jquery.hoverIntent.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
  2. reviewboard/reviews/views.py (Diff revision 21)
     
     
    Show all issues
    Col: 33
     E225 missing whitespace around operator
    
  3. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/lib/js/jquery.hoverIntent.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/lib/js/jquery.hoverIntent.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
  2. 
      
david
  1. Can you add bug 2041?

    I'd still like to see a lot more detail in your testing done.

  2. Show all issues

    You don't need to call escape(text) because format_html will escape any arguments that aren't already safe.

  3. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/lib/js/jquery.hoverIntent.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/lib/js/jquery.hoverIntent.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
  2. 
      
david
  1. One last comment on the code:

  2. reviewboard/reviews/views.py (Diff revision 23)
     
     
    Show all issues

    Can you add an "else: raise ValueError" at the end of this?

    1. The else condition is that either len(lines_of_context) is 2 or 0. I've added an extra case for 0 but 2 is a valid length.

  3. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/lib/js/jquery.hoverIntent.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/lib/js/jquery.hoverIntent.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/lib/js/jquery.hoverIntent.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/templates/reviews/expand_link.html
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/lib/js/jquery.hoverIntent.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxListView.js
        reviewboard/static/rb/js/utils/apiUtils.js
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/diff_comment_fragment.html
        reviewboard/static/rb/js/views/diffFragmentQueueView.js
    
    
  2. 
      
brennie
david
  1. Ship It!

  2. 
      
brennie
Review request changed
Status:
Completed
Change Summary:
Pushed to master (ea775c5)