Summary

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

Review Request #6380 — Created Sept. 28, 2014 and submitted Oct. 26, 2014, 12:56 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

master

Bugs

2041

Depends On

Commit

41ef997...

Reviewers

Groups

reviewboard, students

People

Description

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.

Testing Done

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.

Files

Issues

Description	From	Last Updated
I think the thickness of the border is a little much, was 1px not visible enough?	SM smacleod	Oct. 16, 2014, 2:14 p.m.
list comprehension redefines 'file_attachment' from line 587	reviewbot	Sept. 28, 2014, 8:24 p.m.
list comprehension redefines 'file_attachment' from line 588	reviewbot	Sept. 28, 2014, 8:26 p.m.
list comprehension redefines 'file_attachment' from line 588	reviewbot	Sept. 28, 2014, 8:30 p.m.
Col: 24 E231 missing whitespace after ','	reviewbot	Sept. 28, 2014, 10:13 p.m.
list comprehension redefines 'file_attachment' from line 588	reviewbot	Sept. 28, 2014, 10:14 p.m.
list comprehension redefines 'file_attachment' from line 588	reviewbot	Sept. 29, 2014, 9:30 a.m.
list comprehension redefines 'file_attachment' from line 589	reviewbot	Oct. 2, 2014, 11:47 p.m.
list comprehension redefines 'file_attachment' from line 590	reviewbot	Oct. 3, 2014, 12:41 a.m.
'mark_safe' imported but unused	reviewbot	Oct. 14, 2014, 7:05 p.m.
'render_markdown' imported but unused	reviewbot	Oct. 14, 2014, 7:05 p.m.
Col: 25 E128 continuation line under-indented for visual indent	reviewbot	Oct. 14, 2014, 7:07 p.m.
Col: 52 E128 continuation line under-indented for visual indent	reviewbot	Oct. 14, 2014, 7:07 p.m.
Col: 52 E128 continuation line under-indented for visual indent	reviewbot	Oct. 14, 2014, 7:07 p.m.
Add a trailing comma.	david	Oct. 23, 2014, 6:02 p.m.
If you call mark_safe() here, you can avoid the \|safe in the template. This should probably also be named something …	david	Oct. 23, 2014, 6:03 p.m.
Dictionary entries in python should always end in a trailing comma.	david	Oct. 23, 2014, 6:03 p.m.
Add a trailing comma.	david	Oct. 23, 2014, 6:03 p.m.
This should probably be wrapped in a try/except in order to avoid 500s if a bogus query string is passed.	david	Oct. 23, 2014, 6:04 p.m.
Should have a trailing comma.	david	Oct. 23, 2014, 6:04 p.m.
Can we add a non-minified version of this instead? When we compile it we'll minify, but having the original source …	david	Oct. 23, 2014, 6:01 p.m.
Where do these numbers come from?	david	Oct. 23, 2014, 6:21 p.m.
Can you name this $activityIndicator?	david	Oct. 23, 2014, 6:06 p.m.
Single-line comments (other than those used as method comments) should use //	david	Oct. 23, 2014, 6:11 p.m.
All blocks should use {}	david	Oct. 23, 2014, 6:11 p.m.
We'd typically indent this as: $chunks = $button .closest('.sidebyside') .children('tbody') .not('.diff-header');	david	Oct. 23, 2014, 6:12 p.m.
Single-line comments should use //. Here and below.	david	Oct. 23, 2014, 6:12 p.m.
These should be on the same line.	david	Oct. 23, 2014, 6:12 p.m.
These should be on the same line.	david	Oct. 23, 2014, 6:12 p.m.
setActivityIndicator checks RB.ajaxOptions.enableIndicator so you shouldn't need the conditional.	david	Oct. 23, 2014, 6:13 p.m.
Should have a period at the end.	david	Oct. 23, 2014, 6:13 p.m.
{}	david	Oct. 23, 2014, 6:13 p.m.
typo: finishd	david	Oct. 23, 2014, 6:13 p.m.
You shouldn't need the conditional here because setActivityIndicator checks RB.ajaxOptions.enableIndicator.	david	Oct. 23, 2014, 6:14 p.m.
Variable definitions should happen at the top of the method.	david	Oct. 23, 2014, 6:14 p.m.
Multi-line comments should be formatted as: /* * text */	david	Oct. 23, 2014, 6:14 p.m.
Same here re: formatting. Also should end in a period.	david	Oct. 23, 2014, 6:15 p.m.
Can you revert this change?	david	Oct. 23, 2014, 6:16 p.m.
Just to be safe, how about we escape the 'text' field here? You can use format_html to create HTML and …	david	Oct. 24, 2014, 12:34 p.m.
Generally, in Python it's best to avoid lists in default arguments because any modification to that variable can end up …	david	Oct. 24, 2014, 12:36 p.m.
What happens if this URL is loaded with lines_of_context= ?	david	Oct. 24, 2014, 1:23 p.m.
This should use an === comparison.	david	Oct. 24, 2014, 12:37 p.m.
Should have a blank /* on the first line of the comment.	david	Oct. 24, 2014, 12:37 p.m.
You don't need to pass $expanded in to this--it will be available in the closure.	david	Oct. 24, 2014, 12:39 p.m.
Variable definitions should all be at the top of their method.	david	Oct. 24, 2014, 12:38 p.m.
Blocks should always use {}	david	Oct. 24, 2014, 12:38 p.m.
Col: 33 E225 missing whitespace around operator	reviewbot	Oct. 24, 2014, 1:30 p.m.
You don't need to call escape(text) because format_html will escape any arguments that aren't already safe.	david	Oct. 24, 2014, 2:18 p.m.
Can you add an "else: raise ValueError" at the end of this?	david	Oct. 24, 2014, 2:49 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py

reviewboard/reviews/views.py (Diff revision 1)
The issue has been dropped. Show all issues
```
 list comprehension redefines 'file_attachment' from line 587
```
1. brennie Sept. 28, 2014, 8:24 p.m.
  I didn't modify this line.

Change Summary:

Add extra line to comply with PEP8.

Commit:

492bf95cce10542cea668b78364205e0082bda62

da6918e7ab47e108bd0c77d9ec28e058c628fbac

Diff:

~		The `comment_diff_fragments` view now takes two optional URL get
~		parameters:
~		- `before_lines`, which indicates the number of lines before the
	~	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.
-		`start_line` that should be returned in the fragment; and
-		- `after_lines`, which indicates the number of lines after
-		`start_line`+`num_lines` should be returned in the fragment.
-		It passes these arguments to `build_diff_comment_fragments`, which now
-		takes two arguments (named the same).

~		The `build_diff_comment_fragments` function now calculates, based on
~		the comment's `first_line` and `num_lines` fields and the
~		`before_lines` and `after_lines` arguments new values for the first
~		line and number of lines to pass to `get_file_chunks_in_range`.
	~	The `build_diff_comment_fragments` function now takes a
	~	`lines_of_context` and uses that to calculate the
	~	`first_line` and `num_lines` parameters for
	~	`get_file_chunks_in_range`

		Modifying `static/rb/js/views/diffFragmentQueueView.js` to add
~		`before_lines` and `after_lines` query parameters makes the diff
	~	`lines-of-cotext` query parameter makes the diff
		fragments have the appropriate surrounding context (when available).

	+	Comment diff fragments can now be expanded by a fixed number of lines
	+	either above or below the current amount of context. Add links to the
	+	`diff_comment_fragment.html` template to facilitiate this
	+	functionality. After expanding once, the fragment may be further
	+	expanded.
	+
	+	The `DiffFragmentQueueView` now has an `_onExpandButtonClicked` event
	+	which is fired when an expansion button is clicked.
	+
	+	Add `_expandDiffFragment` to `DiffFragmentQueueView` to expand add a
	+	script tag to the current page that will update the diff fragment with
	+	the new context.
	+
	+	The `ReviewBoxListView` now initializes the `DiffFragmentQueueView`
	+	with an element (`#container`) to listen for events on.
	+
		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.

		The `build_diff_comment_fragments` function now takes a
		`lines_of_context` and uses that to calculate the
		`first_line` and `num_lines` parameters for
		`get_file_chunks_in_range`

	reviewboard/reviews/views.py
	reviewboard/static/rb/js/views/diffFragmentQueueView.js
	reviewboard/static/rb/js/views/reviewBoxListView.js
	reviewboard/templates/reviews/diff_comment_fragment.html

	reviewboard/reviews/views.py
	reviewboard/static/rb/js/utils/apiUtils.js
	reviewboard/static/rb/js/views/diffFragmentQueueView.js
	reviewboard/static/rb/js/views/reviewBoxListView.js
	reviewboard/templates/reviews/diff_comment_fragment.html
	reviewboard/templates/reviews/load_diff_comment_fragments.js

	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
	reviewboard/static/rb/js/utils/apiUtils.js
	reviewboard/static/rb/js/views/diffFragmentQueueView.js
	reviewboard/static/rb/js/views/reviewBoxListView.js
	reviewboard/templates/reviews/diff_comment_fragment.html
	reviewboard/templates/reviews/expand_link.html
	1 more

	reviewboard/staticbundles.py
	reviewboard/reviews/views.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
	reviewboard/static/rb/js/utils/apiUtils.js
	reviewboard/static/rb/js/utils/collapseButtonManager.js
	reviewboard/static/rb/js/views/diffFragmentQueueView.js
	reviewboard/static/rb/js/views/reviewBoxListView.js
	5 more

~		Comment diff fragments can now be expanded by a fixed number of lines
~		either above or below the current amount of context or to the next
~		previous header (i.e., method definition). After expanding once, the
~		fragment may be further expanded (if more of the file is available) or
~		collapsed.
~
~		The `DiffFragmentQueueView` now has an `_onExpandButtonClicked` event
~		which is fired when an expansion button is clicked.
	~	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 in that direction. If there is still room for expansion,
	~	it may be expanded again.
-
-		Add `_expandDiffFragment` to `DiffFragmentQueueView` to expand add a
-		script tag to the current page that will update the diff fragment with
-		the new context.
-
-		The `ReviewBoxListView` now initializes the `DiffFragmentQueueView`
-		with an element (`#container`) to listen for events on.

		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. It also
~		takes an optional `activity` parameter that specifies if the activity
~		indicator should be turned on (indicating asynchronous activity).
	~	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 `build_diff_comment_fragments` function now takes a
~		`lines_of_context` 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

~		The activity indicator functionality in `apiUtils.js` has been
~		refactored into another method, `RB.setActivityIndicator` so that
~		non-API calls can set the activity indicator status when they make
~		asynchronous requests.
	~	Add the `expand_fragment_link`, `expand_fragment_header_link` and
	~	`collapse_fragment_link` Django template tags. These tags create
	~	expansion and contraction links for use in diff fragments. These
	~	functions use a new template: `reviews/expand_link.html`.

~		Add the `exand_fragment_link` and `expand_fragment_header_link` Django
~		template tags. `expand_fragment_link` functions similarily to the
~		`expand_diff_link` tag, as does `expand_fragment_header_link`, except
~		`expand_fragment_header_link` is exclusively for expanding the context
~		to fragment headers.
	~	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.

~		Refactored `DiffReviewableView._updateCollapsePos` into a separate
~		mixin: `CollapseButtonManagerMixin` (in
	~	The `ReviewBoxListView` now constructs the `DiffFragmentQueueView`
	~	with an element (`#container`) on which it should listen for events.
-		`rb/js/utils/collapseButtonManager.js`). Add this path to the JS
-		bundle for the reviews views.

		Unit tests pass.

~		Expansion and contraction links work correctly on local dev server.
	~	Tested expansion and contraction in both Firefox and Chrome.

reviewboard/reviews/views.py (Diff revision 13)

reviewboard/reviews/views.py (Diff revision 13)