Add support for incremental diff expansion.

Review Request #3327 — Created Sept. 14, 2012 and submitted

Information

Review Board
release-1.6.x

Reviewers

Add support for incremental diff expansion.

Review Board has always had the ability to let users expand collapsed
chunks of diffs, but this can produce a lot of content when dealing with
large sections of collapsed code.

This feature introduces incremental expansion. Users can expand the
diffs by 20 lines at a time, before or after the collapsed header. They
can also expand up to the nearest function listed, and of course, can
expand te entire header.

When a diff is expanded, even partially, little floating collapse
buttons will appear near the center column. They'll stay in view,
centered amongst the visible portion of the expanded region. Clicking
them will re-collapse that segment.

This feature was sponsored by NetApp, Inc.
Tested the incremental expansion (pre/post) for chunk headers at the top,
middle, and bottom of diffs.

Tested that expanding chunks kept the chunk header in place, regardless of
which side. When fully expanding, the collapse button started where the chunk
header was.

Tested that fully expanding a chunk continues to work.

Tested that expanding to a class/function works.

Tested that collapsing works in all cases.

Tested that function/class headers update as the chunk is incrementally expanded.

Tested all this with Chrome, Firefox, and IE.
Description From Last Updated

The caller that passes this in defaults it to [None, None] too. Can we just have that be the default …

daviddavid

Alignment looks off here

daviddavid

You can just collapse this into one if statement

daviddavid

This first sentence was a little confusing at first. How about this: If we're currently expanding part of a chunk, …

daviddavid

Wanna fix this to be !== while you're in here?

daviddavid

Alignment?

daviddavid

Alignment?

daviddavid

Alignment?

daviddavid
david
  1. Haven't looked at the code yet, but this also covers issues 603 and 1231, right?
  2. 
      
david
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
     
    Show all issues
    The caller that passes this in defaults it to [None, None] too. Can we just have that be the default for the parameter instead of doing None and then having this?
    1. Nope, because in that case, last_header is basically a static variable. We modify it directly, and when you initialize to a list and then modify that list, you'll get those same modifications the next time you call.
  3. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     
     
    Show all issues
    Alignment looks off here
    1. Not here. Another instance of the bug you keep hitting I guess.
  4. reviewboard/diffviewer/views.py (Diff revision 1)
     
     
     
     
    Show all issues
    You can just collapse this into one if statement
    1. Can't collapse (the results are the same), but that second shouldn't have checked lines_of_context.
  5. reviewboard/diffviewer/views.py (Diff revision 1)
     
     
     
    Show all issues
    This first sentence was a little confusing at first. How about this:
    
    If we're currently expanding part of a chunk, we want to render the entire chunk without any lines collapsed.
  6. Show all issues
    Wanna fix this to be !== while you're in here?
  7. Show all issues
    Alignment?
    1. I don't see anything wrong.
  8. Show all issues
    Alignment?
  9. Show all issues
    Alignment?
  10. 
      
chipx86
david
  1. Ship It!
  2. 
      
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-1.6.x (754e8bf)
Loading...