Make draft comments in diffviewer and text based attachments resizable and movable

Review Request #7032 — Created March 9, 2015 and updated

Information

Review Board
master
2f1b795...

Reviewers

Currently if a draft comment is made with the wrong range of lines, there is no way to correct it other than to delete it and make a new one. This fix aims to make the comment bubbles movable and resizable to update the line range.

Manual testing by dragging comment blocks.

Dragging:
Checked that the comment block can only be dragged if it is a new draft comment and not one made to an existing comment thread.

Resizing:
Checked that begin row cannot be resized to rows after end row.
Checked that end row cannot be resized to rows before begin row.
Checked that begin/end row is resized to a valid row (not headers).
Checked that dragging can be done across compressed diff headers.
Checked that dragging cannot be done across different files.

Moving:
Checked that line range stays the same when moving.
Checked that comment block can only be moved within visible lines.
Checked that line range cannot contain invalid rows.
Checked that dragging cannot be done across different files.

Triggering of comment dialog:
Checked that the comment dialog is triggered on click of the comment block.
Checked that the comment dialog is triggered on click of a row number in the diffviewer.


Description From Last Updated

For readability, can you put a space around the > on each of these in this change?

chipx86chipx86

It's not immediately obvious to me how these are used. At the very least, there should be some documentation somewhere …

chipx86chipx86

Each function you introduce should have a doc comment.

chipx86chipx86

This all feels really, really iffy. Can you explain it more? Why can't this just listen in render()?

chipx86chipx86

Should use listenTo instead of on.

chipx86chipx86

Can we put this inline in the startDragging call below?

chipx86chipx86

Hmm, why not just compare against null?

chipx86chipx86

Same here about inlining.

chipx86chipx86

Blank line between these.

chipx86chipx86

Same here about inlining.

chipx86chipx86

Blank line between these.

chipx86chipx86

The second line's $row... should align with that on the first line. Also, no need to do typeof ... to …

chipx86chipx86

These need doc comments.

chipx86chipx86

Blank line between these.

chipx86chipx86

This is probably a good place for that documentation I mentioned.

chipx86chipx86

Best to pull out moveState into a local variable first, since it'll save on attribute lookups.

chipx86chipx86

Given that we're doing this, I'd like you to look into what jquery provides, since they have all this done …

chipx86chipx86

Only one var statement per function.

chipx86chipx86

A lot of this code looks the same as above. Can you factor it out into a mixin?

chipx86chipx86

Should be inline. Same with others.

chipx86chipx86

Should also compare against null directly.

chipx86chipx86

Blank line between these. Same below.

chipx86chipx86

Why this change?

chipx86chipx86

Not necessarily a "diff" comment bubble, since this works on plain text files as well.

chipx86chipx86

Missing a semicolon.

chipx86chipx86
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffCommentBlockModel.js
        reviewboard/static/rb/js/views/textBasedCommentBlockView.js
        reviewboard/static/rb/css/pages/diffviewer.less
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffCommentBlockModel.js
        reviewboard/static/rb/js/views/textBasedCommentBlockView.js
        reviewboard/static/rb/css/pages/diffviewer.less
    
    
  2. 
      
WU
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffCommentBlockModel.js
        reviewboard/static/rb/js/diffviewer/views/diffCommentBlockView.js
        reviewboard/static/rb/js/views/textCommentRowSelector.js
        reviewboard/static/rb/css/pages/diffviewer.less
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffCommentBlockModel.js
        reviewboard/static/rb/js/diffviewer/views/diffCommentBlockView.js
        reviewboard/static/rb/js/views/textCommentRowSelector.js
        reviewboard/static/rb/css/pages/diffviewer.less
    
    
  2. 
      
WU
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffCommentBlockModel.js
        reviewboard/static/rb/js/diffviewer/views/diffCommentBlockView.js
        reviewboard/static/rb/js/views/textCommentRowSelector.js
        reviewboard/static/rb/css/pages/diffviewer.less
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffCommentBlockModel.js
        reviewboard/static/rb/js/diffviewer/views/diffCommentBlockView.js
        reviewboard/static/rb/js/views/textCommentRowSelector.js
        reviewboard/static/rb/css/pages/diffviewer.less
    
    
  2. 
      
WU
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffCommentBlockModel.js
        reviewboard/static/rb/js/diffviewer/views/diffCommentBlockView.js
        reviewboard/static/rb/js/views/textCommentRowSelector.js
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/static/rb/css/pages/diffviewer.less
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffCommentBlockModel.js
        reviewboard/static/rb/js/diffviewer/views/diffCommentBlockView.js
        reviewboard/static/rb/js/views/textCommentRowSelector.js
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/static/rb/css/pages/diffviewer.less
    
    
  2. 
      
WU
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/textBasedCommentBlockView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/static/rb/js/models/textBasedCommentBlockModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommentBlockModel.js
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/diffviewer/views/diffCommentBlockView.js
        reviewboard/static/rb/js/views/textCommentRowSelector.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/textBasedCommentBlockView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/static/rb/js/models/textBasedCommentBlockModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommentBlockModel.js
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/diffviewer/views/diffCommentBlockView.js
        reviewboard/static/rb/js/views/textCommentRowSelector.js
    
    
  2. 
      
SU
  1. 
      
  2. reviewboard/static/rb/js/views/textBasedCommentBlockView.js (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     

    Why not

    $('<div/>').addClass('...')
               .appendTo(this.$el);
    

    instead?

    1. I've followed the coding style from other parts of the codebase for consistency.

  3. 
      
WU
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/textBasedCommentBlockView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/css/pages/search.less
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/static/rb/js/models/textBasedCommentBlockModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommentBlockModel.js
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/diffviewer/views/diffCommentBlockView.js
        reviewboard/static/rb/js/views/textCommentRowSelector.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/textBasedCommentBlockView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/css/pages/search.less
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/static/rb/js/models/textBasedCommentBlockModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommentBlockModel.js
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/diffviewer/views/diffCommentBlockView.js
        reviewboard/static/rb/js/views/textCommentRowSelector.js
    
    
  2. 
      
WU
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/textBasedCommentBlockView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/static/rb/js/models/textBasedCommentBlockModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommentBlockModel.js
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/diffviewer/views/diffCommentBlockView.js
        reviewboard/static/rb/js/views/textCommentRowSelector.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/textBasedCommentBlockView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/static/rb/js/models/textBasedCommentBlockModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommentBlockModel.js
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/diffviewer/views/diffCommentBlockView.js
        reviewboard/static/rb/js/views/textCommentRowSelector.js
    
    
  2. 
      
chipx86
  1. 
      
  2. Show all issues

    For readability, can you put a space around the > on each of these in this change?

  3. reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    It's not immediately obvious to me how these are used. At the very least, there should be some documentation somewhere about these. However, I wonder if you could instead use the jquery-ui stuff for move/resize support.

    1. I've looked into it but found it unsuitable, I've listed the reasons in my notes on hackpad.

      Currently the dragging doesn't work as _findLineNumRow is moved to textCommentRowSelector and cannot be called in diffReviewableView. I was thinking of using the change in vertical pixels of the mouse movement (e.clientY) to calculate the new rows, instead of using e.target to find the new start row. This should make it more obvious how to drag them as the user can click anywhere on the comment block to move it.

  4. Show all issues

    Each function you introduce should have a doc comment.

  5. reviewboard/static/rb/js/diffviewer/views/diffCommentBlockView.js (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    This all feels really, really iffy. Can you explain it more?

    Why can't this just listen in render()?

    1. I can changed this to start listening on mousedown and stop listening on mouseup.

  6. Show all issues

    Should use listenTo instead of on.

  7. Show all issues

    Can we put this inline in the startDragging call below?

  8. Show all issues

    Hmm, why not just compare against null?

  9. Show all issues

    Same here about inlining.

  10. Show all issues

    Blank line between these.

  11. Show all issues

    Same here about inlining.

  12. Show all issues

    Blank line between these.

  13. Show all issues

    The second line's $row... should align with that on the first line.

    Also, no need to do typeof ... to compare against undefined. Just compare against undefined.

  14. Show all issues

    These need doc comments.

  15. Show all issues

    Blank line between these.

  16. reviewboard/static/rb/js/views/textBasedCommentBlockView.js (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    This is probably a good place for that documentation I mentioned.

  17. Show all issues

    Best to pull out moveState into a local variable first, since it'll save on attribute lookups.

  18. Show all issues

    Given that we're doing this, I'd like you to look into what jquery provides, since they have all this done in a way that works cross-browser and on mobile.

  19. Show all issues

    Only one var statement per function.

  20. Show all issues

    A lot of this code looks the same as above. Can you factor it out into a mixin?

  21. Show all issues

    Should be inline.

    Same with others.

  22. Show all issues

    Should also compare against null directly.

  23. Show all issues

    Blank line between these.

    Same below.

  24. Show all issues

    Why this change?

    1. .click() does not work after I defined mousedown for the commentBlockView but .dblclick() works. What would be a better solution for this?

  25. Show all issues

    Not necessarily a "diff" comment bubble, since this works on plain text files as well.

  26. Show all issues

    Missing a semicolon.

  27. 
      
WU
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/textBasedCommentBlockView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/static/rb/js/models/textBasedCommentBlockModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommentBlockModel.js
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/views/textCommentRowSelector.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/textBasedCommentBlockView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/static/rb/js/models/textBasedCommentBlockModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommentBlockModel.js
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/views/textCommentRowSelector.js
    
    
  2. 
      
WU
Review request changed
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/textBasedCommentBlockView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/static/rb/js/models/textBasedCommentBlockModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommentBlockModel.js
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/views/textCommentRowSelector.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/textBasedCommentBlockView.js
        reviewboard/static/rb/css/pages/diffviewer.less
        reviewboard/static/rb/js/diffviewer/views/diffReviewableView.js
        reviewboard/static/rb/js/models/textBasedCommentBlockModel.js
        reviewboard/static/rb/js/diffviewer/models/diffCommentBlockModel.js
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/views/textCommentRowSelector.js
    
    
  2.