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

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

Wu Di
Review Board
master
2f1b795...
reviewboard, students

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.

Loading file attachments...

  • 6
  • 0
  • 19
  • 0
  • 25
Description From Last Updated
It's not immediately obvious to me how these are used. At the very least, there should be some documentation somewhere ... Christian Hammond Christian Hammond
This is probably a good place for that documentation I mentioned. Christian Hammond Christian Hammond
Best to pull out moveState into a local variable first, since it'll save on attribute lookups. Christian Hammond Christian Hammond
Given that we're doing this, I'd like you to look into what jquery provides, since they have all this done ... Christian Hammond Christian Hammond
A lot of this code looks the same as above. Can you factor it out into a mixin? Christian Hammond Christian Hammond
Why this change? Christian Hammond Christian Hammond
Review Bot
  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 Di
Review Bot
  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 Di
Review Bot
  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 Di
Review Bot
  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 Di
Review Bot
  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. 
      
Wang Jun Sun
  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 Di
Review Bot
  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 Di
Review Bot
  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. 
      
Christian Hammond
  1. 
      
  2. 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)
     
     
     
     
     
     
     
     
     
     

    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. Each function you introduce should have a doc comment.

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

    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. Should use listenTo instead of on.

  7. Can we put this inline in the startDragging call below?

  8. Hmm, why not just compare against null?

  9. Same here about inlining.

  10. Blank line between these.

  11. Same here about inlining.

  12. Blank line between these.

  13. 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. These need doc comments.

  15. Blank line between these.

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

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

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

  18. 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. Only one var statement per function.

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

  21. Should be inline.

    Same with others.

  22. Should also compare against null directly.

  23. Blank line between these.

    Same below.

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

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

  25. Missing a semicolon.

  26. 
      
Wu Di
Review Bot
  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 Di
Review request changed
Review Bot
  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. 
      
Loading...