Summary

Fix commenting on text-based file attachments.

Review Request #7092 — Created March 19, 2015 and submitted March 25, 2015, 4:56 p.m.

Information

Owner

chipx86

Repository

Review Board

Branch

release-2.5.x

Bugs

Depends On

Commit

7a9e328...

Reviewers

Groups

reviewboard

People

Description

The recent work for retrofitting text-based file attachments for diff
support triggered some bad behavior in the commenting support.
TextCommentRowSelector and comment flag placement needed to work on a
table rather than a tbody. This caused mismatches between the element's
list of rows and the rowIndexes. It also caused a reference to an
invalid variable for comment placement.

One possible solution is to update TextCommentRowSelector to work with
tbodies, and a previous attempt went this route. However, future work
for diffviewer-style diffs for text attachments will require multiple
tbodies per table, so instead, this change splits the tbodies back out
into their own tables again.

The tables, for both plain text and rendered text, are split off into
reusable templates, reducing the amount of code duplication. The
revision information has been moved into these tables, under the tabs.
This will help with future review UIs that want to render plain text,
but also want a different UI entirely under the Rendered tab (such as
SVGs).

The revision label and selector rows are now hard-coded in the template,
as well, rather than being dynamically added later.

There's also some visual fixes for the page to prevent unwanted
gradients and to fix borders at the bottom of the view.

Testing Done

Tested reviewing individual revisions and diffs of plain text and Markdown
attachments, both source and rendered.

Verified in Chrome and Firefox that the visual styles were correct.

Files

Issues

Description	From	Last Updated
You should use \|yesno here, too.	david	March 19, 2015, 9:46 p.m.
tBodies cannot be used here	WU wudi	March 20, 2015, 12:59 a.m.
Same as above, tBodies need to be removed.	WU wudi	March 20, 2015, 12:59 a.m.
This could be: lineNum = RB.MathUtils.clip(lineNum, 1, rows.length) - 1;	david	March 20, 2015, 5:48 p.m.

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/textBasedReviewableView.js
    reviewboard/templates/reviews/ui/text.html
    reviewboard/static/rb/css/pages/text-review-ui.less
    reviewboard/static/rb/js/views/textCommentRowSelector.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/textBasedReviewableView.js
    reviewboard/templates/reviews/ui/text.html
    reviewboard/static/rb/css/pages/text-review-ui.less
    reviewboard/static/rb/js/views/textCommentRowSelector.js

reviewboard/templates/reviews/ui/text.html (Diff revision 1)
The issue has been resolved. Show all issues
```
You should use |yesno here, too.
```

Change Summary:

Switched to yesno in another location.

Commit:

5bb7cf232c72248b4dbbab5dd51314e382112e99

2f85891078234eb88aba80ef25ffba48b23668fc

Diff:

Revision 2 (+42 -29)

Show changes

	reviewboard/static/rb/css/pages/text-review-ui.less
	reviewboard/static/rb/js/views/textBasedReviewableView.js
	reviewboard/static/rb/js/views/textCommentRowSelector.js
	reviewboard/templates/reviews/ui/text.html

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/textBasedReviewableView.js
    reviewboard/templates/reviews/ui/text.html
    reviewboard/static/rb/css/pages/text-review-ui.less
    reviewboard/static/rb/js/views/textCommentRowSelector.js



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/textBasedReviewableView.js
    reviewboard/templates/reviews/ui/text.html
    reviewboard/static/rb/css/pages/text-review-ui.less
    reviewboard/static/rb/js/views/textCommentRowSelector.js

reviewboard/static/rb/js/views/textBasedReviewableView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
tBodies cannot be used here
```
1. chipx86 March 20, 2015, 12:59 a.m.
  Good catch!
reviewboard/static/rb/js/views/textBasedReviewableView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
Same as above, tBodies need to be removed.
```

Change Summary:

Fixed scrolling to lines by removing the tBodies references.

Commit:

2f85891078234eb88aba80ef25ffba48b23668fc

b59a9e23ab6f05e4a7921b79dab9b713bb66c291

Diff:

Revision 3 (+44 -31)

Show changes

	reviewboard/static/rb/css/pages/text-review-ui.less
	reviewboard/static/rb/js/views/textBasedReviewableView.js
	reviewboard/static/rb/js/views/textCommentRowSelector.js
	reviewboard/templates/reviews/ui/text.html

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/textBasedReviewableView.js
    reviewboard/templates/reviews/ui/text.html
    reviewboard/static/rb/css/pages/text-review-ui.less
    reviewboard/static/rb/js/views/textCommentRowSelector.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/textBasedReviewableView.js
    reviewboard/templates/reviews/ui/text.html
    reviewboard/static/rb/css/pages/text-review-ui.less
    reviewboard/static/rb/js/views/textCommentRowSelector.js

In your description you have element' but it should be element's.

I'm actually going to need to redo this change. I need to go back to actually using tables, to prepare for actual diffing (since diffs need tbodies in a dedicated table).

Change Summary:

Redid the change to instead move the tbodies back into their own tables.

This will be needed in order to support a real diff view for the text files,

since diffs need to insert multiple tbodies.
Visually, the only difference is that revision labels now appear below the

tabs instead of above, for renderable text files. This is actually going to

let us do a better job of controlling the view later for fancier text review

UIs, such as for SVGs, where you want to display a text mode, but also render

with a different UI entirely.

Description:

		The recent work for retrofitting text-based file attachments for diff
		support triggered some bad behavior in the commenting support.
~		`TextCommentRowSelector` and comment flag placement assumed that they were
~		operating on an individual table, rather than a tbody within the table.
~		This caused mismatches between the element's list of rows and the
~		`rowIndexes`. It also caused a reference to an invalid variable for
	~	`TextCommentRowSelector` and comment flag placement needed to work on a
	~	table rather than a tbody. This caused mismatches between the element's
	~	list of rows and the rowIndexes. It also caused a reference to an
	~	invalid variable for comment placement.
-		comment placement.

~		A couple things were needed to fix this.
	~	One possible solution is to update `TextCommentRowSelector` to work with
	+	tbodies, and a previous attempt went this route. However, future work
	+	for diffviewer-style diffs for text attachments will require multiple
	+	tbodies per table, so instead, this change splits the tbodies back out
	+	into their own tables again.

~		First, the revision label and selector rows are now hard-coded in the
~		template, rather than being dynamically added later, which prevents some
~		of the issues with expected row indexes.
	~	The tables, for both plain text and rendered text, are split off into
	~	reusable templates, reducing the amount of code duplication. The
	~	revision information has been moved into these tables, under the tabs.
	+	This will help with future review UIs that want to render plain text,
	+	but also want a different UI entirely under the Rendered tab (such as
	+	SVGs).

~		Second, `TextCommentRowSelector` is now smart enough to compute its
~		element' offset into the list of rows, ensuring that when it fetches a
	~	The revision label and selector rows are now hard-coded in the template,
	~	as well, rather than being dynamically added later.
-		row, it's the row it expects and not something further down.
-
-		Third, the class name for the right-hand side text is now correct. It was
-		`'2'`, but it should have been `'r'`.
-
-		Fourth, comment flag placement now knows it's working with a tbody and
-		not a table, and no longer tries to access the first tbody in the
-		element.

		There's also some visual fixes for the page to prevent unwanted
		gradients and to fix borders at the bottom of the view.

Commit:

b59a9e23ab6f05e4a7921b79dab9b713bb66c291

7a9e3283bca39d0bb80e4ba82d097dae145617c2

Diff:

Revision 4 (+153 -146)

Show changes

	reviewboard/static/rb/css/pages/text-review-ui.less
	reviewboard/static/rb/js/views/textBasedReviewableView.js
	reviewboard/templates/reviews/ui/_text_rendered_table.html
	reviewboard/templates/reviews/ui/_text_table.html
	reviewboard/templates/reviews/ui/text.html

Added Files:

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/templates/reviews/ui/_text_rendered_table.html
    reviewboard/static/rb/js/views/textBasedReviewableView.js
    reviewboard/static/rb/css/pages/text-review-ui.less
    reviewboard/templates/reviews/ui/text.html
    reviewboard/templates/reviews/ui/_text_table.html



Tool: Pyflakes
Ignored Files:
    reviewboard/templates/reviews/ui/_text_rendered_table.html
    reviewboard/static/rb/js/views/textBasedReviewableView.js
    reviewboard/static/rb/css/pages/text-review-ui.less
    reviewboard/templates/reviews/ui/text.html
    reviewboard/templates/reviews/ui/_text_table.html

Ship it!

reviewboard/static/rb/js/views/textBasedReviewableView.js (Diff revision 3)

The issue has been resolved. Show all issues

This could be:

lineNum = RB.MathUtils.clip(lineNum, 1, rows.length) - 1;

Status:: Completed
Change Summary:: Pushed to release-2.5.x (da7ded9)