Fix commenting on text-based file attachments.

Review Request #7092 — Created March 19, 2015 and submitted

Information

Review Board
release-2.5.x
7a9e328...

Reviewers

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.

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.


Description From Last Updated

You should use |yesno here, too.

daviddavid

tBodies cannot be used here

WU wudi

Same as above, tBodies need to be removed.

WU wudi

This could be: lineNum = RB.MathUtils.clip(lineNum, 1, rows.length) - 1;

daviddavid
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 
      
  2. Show all issues

    You should use |yesno here, too.

  3. 
      
chipx86
reviewbot
  1. 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
    
    
  2. 
      
WU
  1. 
      
  2. Show all issues

    tBodies cannot be used here

  3. Show all issues

    Same as above, tBodies need to be removed.

  4. 
      
chipx86
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. In your description you have element' but it should be element's.

  2. 
      
chipx86
  1. 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).

  2. 
      
chipx86
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 
      
  2. Show all issues

    This could be:

    lineNum = RB.MathUtils.clip(lineNum, 1, rows.length) - 1;
    
  3. 
      
chipx86
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.5.x (da7ded9)