Fish Trophy

yyoud88 got a fish trophy!

Make regionCommentBlock movable.

Review Request #6886 — Created Feb. 1, 2015 and discarded

yyoud88
Review Board
master
reviewboard, students

Made draft RegionCommentBlock movable, by drag-and-drop.

Currently, a RegionCommentBlock is not movable, even if it is a draft. This change makes draft RegionCommentBlocks movable by drag-and-drop. Users finish moving it by mouse-up action, and the draft RegionCommentBlock's new position is saved on server.

Passed all tests in "/js-test/".

Browser test done:
1. Go to file attachment review page, for example, http://localhost:8080/r/7/file/4/
2. Make draft comment
3. Try moving
4. Refresh the page
5. See if the bounds are updated

Edge cases tested:
1. Open file attachment review page, make move "once" and refresh. => Working well. Confirmed the location is updated.
2. Try click-and-drag non-draft commentBlocks. => Non-draft commentBlcoks are not movable. Working as expected.

Description From Last Updated

Blank line between these.

chipx86chipx86

What happens when this is fired the very first time for a new comment? Do we save that comment?

chipx86chipx86

Since this will also need to be used to decide whether a resize can be performed, I'd suggest calling this ...

chipx86chipx86

These will all need doc comments in the form used in parse() above.

chipx86chipx86

Blank line between these.

chipx86chipx86

Blank line between these.

chipx86chipx86

notify is both pretty generic (in terms of option names) and yet too specific (in terms of the action that ...

chipx86chipx86

Generally-speaking, you'd want to remove this and change the below code to: if (options.notify !== false) { ... } However, ...

chipx86chipx86

Blank lines on either side of the conditional, like: this._updateTooltip(); if (options.notify) { ... } RB.DraftReview....

chipx86chipx86

Style nitpick: blank line between a statement and a block

brenniebrennie

You can use _.bind, .e.g commentBlockView.setSelectionRegionSize( _.bind(function() { ... }, this);

brenniebrennie

Blank line between these.

chipx86chipx86

Blank line between these.

chipx86chipx86

JavaScript doesn't allow a final trailing comma. It'll break IE.

chipx86chipx86

Blank line here. Both these statements are doing very different things, and much like paragraphs covering different topics, they should ...

chipx86chipx86

Hmmm, there's probably an easier way to do this. I think you want: events: _.defaults({ 'mousedown .selection-flag': '...', ..., }, ...

chipx86chipx86

Instead of these, you can do: _.bindAll(this, '_onDrag', '_onWindowMouseUp'); It'll replace those functions with bound versions.

chipx86chipx86

This doesn't need to be indented.

chipx86chipx86

Not sure these are necessary. I'd just put the logic in the calling functions.

chipx86chipx86

When possible, single quotes are preferred over double quotes in both Python and JavaScript.

chipx86chipx86

No indentation like this. Make sure to follow the doc conventions throughout the codebase. One line summary, multi-line description. This ...

chipx86chipx86

Unindent the comment relative to the *. Same below

brenniebrennie

Blank line here.

chipx86chipx86

Blank line before this. You can set multiple attributes at once by passing a dictionary.

chipx86chipx86

Our max line length is 79 so take full advantage of it for comments. Same elsewhere.

brenniebrennie

This doesn't describe what the function does.

chipx86chipx86

Make sure the blank line in the comment is removed.

chipx86chipx86

Blank line here.

chipx86chipx86

And here.

chipx86chipx86

The second line should line up with the open quote on the first line, i.e. this.model.on('foo...', this._bar, ...)

brenniebrennie

You should use this.listenTo(...) instead of this.model.on(...) when listening to events. This helps with managing circular references in signal handlers.

chipx86chipx86

Blank line between a statement and a block

brenniebrennie

All the same comments here as above.

chipx86chipx86

Blank line here.

chipx86chipx86

These lines look too long (wrap to 80 columns).

daviddavid
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/abstractCommentBlockView.js
        reviewboard/static/rb/js/resources/models/tests/baseResourceModelTests.js
        reviewboard/static/rb/js/models/regionCommentBlockModel.js
        reviewboard/static/rb/js/views/regionCommentBlockView.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/abstractCommentBlockView.js
        reviewboard/static/rb/js/resources/models/tests/baseResourceModelTests.js
        reviewboard/static/rb/js/models/regionCommentBlockModel.js
        reviewboard/static/rb/js/views/regionCommentBlockView.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
    
    
  2. 
      
brennie
  1. 
      
  2. Style nitpick: blank line between a statement and a block

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

    You can use _.bind, .e.g

    commentBlockView.setSelectionRegionSize(
        _.bind(function() {
            ...
       }, this);
    
  4. Unindent the comment relative to the *. Same below

  5. Our max line length is 79 so take full advantage of it for comments. Same elsewhere.

  6. The second line should line up with the open quote on the first line, i.e.

    this.model.on('foo...',
                  this._bar, ...)
    
  7. Blank line between a statement and a block

  8. 
      
YY
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/abstractCommentBlockView.js
        reviewboard/static/rb/js/resources/models/tests/baseResourceModelTests.js
        reviewboard/static/rb/js/models/regionCommentBlockModel.js
        reviewboard/static/rb/js/views/regionCommentBlockView.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/abstractCommentBlockView.js
        reviewboard/static/rb/js/resources/models/tests/baseResourceModelTests.js
        reviewboard/static/rb/js/models/regionCommentBlockModel.js
        reviewboard/static/rb/js/views/regionCommentBlockView.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
    
    
  2. 
      
YY
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/abstractCommentBlockView.js
        reviewboard/static/rb/js/resources/models/tests/baseResourceModelTests.js
        reviewboard/static/rb/js/models/regionCommentBlockModel.js
        reviewboard/static/rb/js/views/regionCommentBlockView.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/abstractCommentBlockView.js
        reviewboard/static/rb/js/resources/models/tests/baseResourceModelTests.js
        reviewboard/static/rb/js/models/regionCommentBlockModel.js
        reviewboard/static/rb/js/views/regionCommentBlockView.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
    
    
  2. 
      
chipx86
  1. 
      
  2. In case you were wondering, this is only available to views, and only works for DOM events.

  3. Blank line between these.

  4. What happens when this is fired the very first time for a new comment? Do we save that comment?

    1. No we don't. Event handler is going to update the underlying comment model, but doesn't save the model. End of dragging calls save.

  5. Since this will also need to be used to decide whether a resize can be performed, I'd suggest calling this canUpdateBounds.

  6. reviewboard/static/rb/js/models/regionCommentBlockModel.js (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    These will all need doc comments in the form used in parse() above.

  7. Blank line between these.

  8. Blank line between these.

  9. notify is both pretty generic (in terms of option names) and yet too specific (in terms of the action that the model is trying to block). Models should never have a concept of UI, and signal emitters should not assume how a signal will be used.

    Instead of an option name suggesting behavior, it should instead suggest what happened. So, I'd go with boundsUpdated: true. Then the view (or any future code) could make smart decisions based on the knowledge that the bounds have been updated.

  10. Generally-speaking, you'd want to remove this and change the below code to:

    if (options.notify !== false) {
        ...
    }
    

    However, with the change I suggested to the name, which also flips the value, you won't need either.

  11. Blank lines on either side of the conditional, like:

    this._updateTooltip();
    
    if (options.notify) {
        ...
    }
    
    RB.DraftReview....
    
  12. Blank line between these.

  13. Blank line between these.

  14. JavaScript doesn't allow a final trailing comma. It'll break IE.

  15. Blank line here. Both these statements are doing very different things, and much like paragraphs covering different topics, they should not be grouped together.

  16. reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1)
     
     
     
     
     
     
     

    Hmmm, there's probably an easier way to do this. I think you want:

    events: _.defaults({
        'mousedown .selection-flag': '...',
        ...,
    }, _super(this).events)
    
    1. No. _super(this).events can be a function which returns an object.

  17. Instead of these, you can do:

    _.bindAll(this, '_onDrag', '_onWindowMouseUp');
    

    It'll replace those functions with bound versions.

  18. This doesn't need to be indented.

  19. reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1)
     
     
     
     
     
     
     
     
     
     

    Not sure these are necessary. I'd just put the logic in the calling functions.

  20. When possible, single quotes are preferred over double quotes in both Python and JavaScript.

  21. No indentation like this.

    Make sure to follow the doc conventions throughout the codebase. One line summary, multi-line description.

    This applies to all comments in your change.

  22. Blank line before this.

    You can set multiple attributes at once by passing a dictionary.

  23. This doesn't describe what the function does.

  24. Make sure the blank line in the comment is removed.

  25. You should use this.listenTo(...) instead of this.model.on(...) when listening to events. This helps with managing circular references in signal handlers.

  26. reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1)
     
     
     
     
     
     
     
     

    All the same comments here as above.

  27. 
      
YY
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/abstractCommentBlockView.js
        reviewboard/static/rb/js/resources/models/tests/baseResourceModelTests.js
        reviewboard/static/rb/js/models/regionCommentBlockModel.js
        reviewboard/static/rb/js/views/regionCommentBlockView.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/abstractCommentBlockView.js
        reviewboard/static/rb/js/resources/models/tests/baseResourceModelTests.js
        reviewboard/static/rb/js/models/regionCommentBlockModel.js
        reviewboard/static/rb/js/views/regionCommentBlockView.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
    
    
  2. 
      
mike_conley
  1. Hey Stanley, could you please update the Description and Testing Done fields based on https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/ ?

    1. will do. thanks.

  2. 
      
YY
YY
david
  1. Is this change an obsolete version of /r/6919?

    1. no. 6919 is to make it resizable. 6886 is to make it movable. 6919 depends on 6886

    2. It looks like 6919 actually contains many of the same changes as this. Can you re-post and pass in the specific revisions so that it only gets the resize changes?

  2. 
      
david
  1. Please be specific about exactly what manual tests you executed in your browser. What test cases did you try? What were the results of each test? In particular, this sort of interaction often has some edge cases--which edge cases have you identified, and are they handled correctly?

  2. These lines look too long (wrap to 80 columns).

  3. 
      
YY
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/abstractCommentBlockView.js
        reviewboard/static/rb/js/resources/models/tests/baseResourceModelTests.js
        reviewboard/static/rb/js/models/regionCommentBlockModel.js
        reviewboard/static/rb/js/views/regionCommentBlockView.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/abstractCommentBlockView.js
        reviewboard/static/rb/js/resources/models/tests/baseResourceModelTests.js
        reviewboard/static/rb/js/models/regionCommentBlockModel.js
        reviewboard/static/rb/js/views/regionCommentBlockView.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/static/rb/js/resources/models/baseResourceModel.js
    
    
  2. 
      
david
  1. Still waiting on detailed testing information for this change.

  2. 
      
YY
david
  1. I just applied this patch and played with it a bit, and it seems pretty easy to get things in a state where clicking on the number (not click+drag) doesn't open the comment dialog to edit the existing comment. I think there's some state (hasMoved) that's not being reset correctly.

  2. 
      
YY
Review request changed

Status: Discarded

Loading...