• 
      

    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?

    chipx86 chipx86

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

    chipx86 chipx86

    Each function you introduce should have a doc comment.

    chipx86 chipx86

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

    chipx86 chipx86

    Should use listenTo instead of on.

    chipx86 chipx86

    Can we put this inline in the startDragging call below?

    chipx86 chipx86

    Hmm, why not just compare against null?

    chipx86 chipx86

    Same here about inlining.

    chipx86 chipx86

    Blank line between these.

    chipx86 chipx86

    Same here about inlining.

    chipx86 chipx86

    Blank line between these.

    chipx86 chipx86

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

    chipx86 chipx86

    These need doc comments.

    chipx86 chipx86

    Blank line between these.

    chipx86 chipx86

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

    chipx86 chipx86

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

    chipx86 chipx86

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

    chipx86 chipx86

    Only one var statement per function.

    chipx86 chipx86

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

    chipx86 chipx86

    Should be inline. Same with others.

    chipx86 chipx86

    Should also compare against null directly.

    chipx86 chipx86

    Blank line between these. Same below.

    chipx86 chipx86

    Why this change?

    chipx86 chipx86

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

    chipx86 chipx86

    Missing a semicolon.

    chipx86 chipx86
    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.