yyoud88 got a fish trophy!

Summary

Make regionCommentBlock movable.

Review Request #6886 — Created Feb. 1, 2015 and discarded Feb. 23, 2015, 3:36 p.m.

Information

Owner

yyoud88

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard, students

People

Description

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.

Testing Done

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.

Issues

Description	From	Last Updated
Blank line between these.	chipx86	Feb. 1, 2015, 6:13 p.m.
What happens when this is fired the very first time for a new comment? Do we save that comment?	chipx86	Feb. 1, 2015, 7:06 p.m.
Since this will also need to be used to decide whether a resize can be performed, I'd suggest calling this …	chipx86	Feb. 1, 2015, 6:15 p.m.
These will all need doc comments in the form used in parse() above.	chipx86	Feb. 1, 2015, 6:25 p.m.
Blank line between these.	chipx86	Feb. 1, 2015, 6:25 p.m.
Blank line between these.	chipx86	Feb. 1, 2015, 6:25 p.m.
notify is both pretty generic (in terms of option names) and yet too specific (in terms of the action that …	chipx86	Feb. 1, 2015, 7:01 p.m.
Generally-speaking, you'd want to remove this and change the below code to: if (options.notify !== false) { ... } However, …	chipx86	Feb. 1, 2015, 7:01 p.m.
Blank lines on either side of the conditional, like: this._updateTooltip(); if (options.notify) { ... } RB.DraftReview....	chipx86	Feb. 1, 2015, 7:01 p.m.
Style nitpick: blank line between a statement and a block	brennie	Feb. 1, 2015, 5:54 p.m.
You can use _.bind, .e.g commentBlockView.setSelectionRegionSize( _.bind(function() { ... }, this);	brennie	Feb. 1, 2015, 5:52 p.m.
Blank line between these.	chipx86	Feb. 1, 2015, 6:28 p.m.
Blank line between these.	chipx86	Feb. 1, 2015, 6:29 p.m.
JavaScript doesn't allow a final trailing comma. It'll break IE.	chipx86	Feb. 1, 2015, 6:29 p.m.
Blank line here. Both these statements are doing very different things, and much like paragraphs covering different topics, they should …	chipx86	Feb. 1, 2015, 6:34 p.m.
Hmmm, there's probably an easier way to do this. I think you want: events: _.defaults({ 'mousedown .selection-flag': '...', ..., }, …	chipx86	Feb. 1, 2015, 7:02 p.m.
Instead of these, you can do: _.bindAll(this, '_onDrag', '_onWindowMouseUp'); It'll replace those functions with bound versions.	chipx86	Feb. 1, 2015, 6:37 p.m.
This doesn't need to be indented.	chipx86	Feb. 1, 2015, 6:37 p.m.
Not sure these are necessary. I'd just put the logic in the calling functions.	chipx86	Feb. 1, 2015, 7:03 p.m.
When possible, single quotes are preferred over double quotes in both Python and JavaScript.	chipx86	Feb. 1, 2015, 6:38 p.m.
No indentation like this. Make sure to follow the doc conventions throughout the codebase. One line summary, multi-line description. This …	chipx86	Feb. 1, 2015, 6:38 p.m.
Unindent the comment relative to the *. Same below	brennie	Feb. 1, 2015, 5:53 p.m.
Blank line here.	chipx86	Feb. 1, 2015, 6:38 p.m.
Blank line before this. You can set multiple attributes at once by passing a dictionary.	chipx86	Feb. 1, 2015, 6:39 p.m.
Our max line length is 79 so take full advantage of it for comments. Same elsewhere.	brennie	Feb. 1, 2015, 5:54 p.m.
This doesn't describe what the function does.	chipx86	Feb. 1, 2015, 6:40 p.m.
Make sure the blank line in the comment is removed.	chipx86	Feb. 1, 2015, 6:42 p.m.
Blank line here.	chipx86	Feb. 1, 2015, 6:42 p.m.
And here.	chipx86	Feb. 1, 2015, 6:46 p.m.
The second line should line up with the open quote on the first line, i.e. this.model.on('foo...', this._bar, ...)	brennie	Feb. 1, 2015, 5:54 p.m.
You should use this.listenTo(...) instead of this.model.on(...) when listening to events. This helps with managing circular references in signal handlers.	chipx86	Feb. 1, 2015, 6:49 p.m.
Blank line between a statement and a block	brennie	Feb. 1, 2015, 5:54 p.m.
All the same comments here as above.	chipx86	Feb. 1, 2015, 6:46 p.m.
Blank line here.	chipx86	Feb. 1, 2015, 6:46 p.m.
These lines look too long (wrap to 80 columns).	david	Feb. 16, 2015, 8:09 p.m.

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

reviewboard/static/rb/js/views/abstractCommentBlockView.js (Diff revision 1)
Show all issues
```
Style nitpick: blank line between a statement and a block
```

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

Show all issues

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

reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1)
Show all issues
```
Unindent the comment relative to the *. Same below
```

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

Show all issues

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

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

Show all issues

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

this.model.on('foo...',
              this._bar, ...)

reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1)
Show all issues
```
Blank line between a statement and a block
```

Change Summary:

improve styles.

Commit:

-	3ce32bc39735c43e42a44a35992e9618ba4940b4
+	89a08c16b6db43d7cf789d5721e3598520a979de

Diff:

Revision 2 (+262 -21)

Show changes

	reviewboard/static/rb/js/models/regionCommentBlockModel.js
	reviewboard/static/rb/js/resources/models/baseResourceModel.js
	reviewboard/static/rb/js/resources/models/tests/baseResourceModelTests.js
	reviewboard/static/rb/js/views/abstractCommentBlockView.js
	reviewboard/static/rb/js/views/imageReviewableView.js
	reviewboard/static/rb/js/views/regionCommentBlockView.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



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

Change Summary:

fixed style issues

Commit:

-	89a08c16b6db43d7cf789d5721e3598520a979de
+	5b78dd6c9dab603d8836d118e24056c73cc73488

Diff:

Revision 3 (+262 -21)

Show changes

	reviewboard/static/rb/js/models/regionCommentBlockModel.js
	reviewboard/static/rb/js/resources/models/baseResourceModel.js
	reviewboard/static/rb/js/resources/models/tests/baseResourceModelTests.js
	reviewboard/static/rb/js/views/abstractCommentBlockView.js
	reviewboard/static/rb/js/views/imageReviewableView.js
	reviewboard/static/rb/js/views/regionCommentBlockView.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



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

1. YY
  
  yyoud88 Feb. 1, 2015, 7:09 p.m.
  deleted

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

In case you were wondering, this is only available to views, and only works for DOM events.

reviewboard/static/rb/js/models/regionCommentBlockModel.js (Diff revision 1)
Show all issues
```
Blank line between these.
```

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

Show all issues

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

yyoud88 Feb. 1, 2015, 7:09 p.m.

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.

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

Show all issues

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

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

Show all issues

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

reviewboard/static/rb/js/models/regionCommentBlockModel.js (Diff revision 1)
Show all issues
```
Blank line between these.
```
reviewboard/static/rb/js/models/regionCommentBlockModel.js (Diff revision 1)
Show all issues
```
Blank line between these.
```

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

Show all issues

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.

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

Show all issues

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.

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

Show all issues

Blank lines on either side of the conditional, like:

this._updateTooltip();

if (options.notify) {
    ...
}

RB.DraftReview....

reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 1)
Show all issues
```
Blank line between these.
```
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 1)
Show all issues
```
Blank line between these.
```
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 1)
Show all issues
```
JavaScript doesn't allow a final trailing comma. It'll break IE.
```

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

Show all issues

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

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

Show all issues

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

events: _.defaults({
    'mousedown .selection-flag': '...',
    ...,
}, _super(this).events)

yyoud88 Feb. 1, 2015, 7:09 p.m.

No. _super(this).events can be a function which returns an object.

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

Show all issues

Instead of these, you can do:

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

It'll replace those functions with bound versions.

reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1)
Show all issues
```
This doesn't need to be indented.
```

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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.

reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1)
Show all issues
```
Blank line here.
```

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

Show all issues

Blank line before this.

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

reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1)
Show all issues
```
This doesn't describe what the function does.
```
reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1)
Show all issues
```
Make sure the blank line in the comment is removed.
```
reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1)
Show all issues
```
Blank line here.
```
reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1)
Show all issues
```
And here.
```

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

Show all issues

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

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

Show all issues

All the same comments here as above.

reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1)
Show all issues
```
Blank line here.
```

Change Summary:

solved issues opened.
improved styles, method names, keys, ...

Commit:

-	5b78dd6c9dab603d8836d118e24056c73cc73488
+	784c8ead1e8e5343c2a933c372222fe228d6e297

Diff:

Revision 4 (+259 -21)

Show changes

	reviewboard/static/rb/js/models/regionCommentBlockModel.js
	reviewboard/static/rb/js/resources/models/baseResourceModel.js
	reviewboard/static/rb/js/resources/models/tests/baseResourceModelTests.js
	reviewboard/static/rb/js/views/abstractCommentBlockView.js
	reviewboard/static/rb/js/views/imageReviewableView.js
	reviewboard/static/rb/js/views/regionCommentBlockView.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



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

Hey Stanley, could you please update the Description and Testing Done fields based on https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/ ?

YY

yyoud88 Feb. 1, 2015, 7:42 p.m.
```
will do. thanks.
```

Change Summary:

updated "description" and "testing done" part.

Summary:

-	[WIP] Make regionCommentBlock movable.
+	Make regionCommentBlock movable.

Description:

~		Allow draft regionCommentBlock to be moved.
	~	Made draft RegionCommentBlock movable, by drag-and-drop.
	+	A user finishes moving it by mouse-up, and it will update new position on server.

Testing Done:

~		passed all tests in http://localhost:8080/js-tests/
	~	Passed all tests in http://localhost:8080/js-tests/
	+	Tested on my local machine on browsers.

Description:

~		Made draft RegionCommentBlock movable, by drag-and-drop.
~		A user finishes moving it by mouse-up, and it will update new position on server.
	~	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.

Is this change an obsolete version of /r/6919?

yyoud88 Feb. 13, 2015, 2:17 p.m.

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

david Feb. 13, 2015, 2:20 p.m.

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?

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?

reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 4)
Show all issues
```
These lines look too long (wrap to 80 columns).
```

Change Summary:

fixed style issue

Commit:

-	784c8ead1e8e5343c2a933c372222fe228d6e297
+	9fcb612dc59c1fdf140d4e6b4a0d652315391db9

Diff:

Revision 5 (+264 -21)

Show changes

	reviewboard/static/rb/js/models/regionCommentBlockModel.js
	reviewboard/static/rb/js/resources/models/baseResourceModel.js
	reviewboard/static/rb/js/resources/models/tests/baseResourceModelTests.js
	reviewboard/static/rb/js/views/abstractCommentBlockView.js
	reviewboard/static/rb/js/views/imageReviewableView.js
	reviewboard/static/rb/js/views/regionCommentBlockView.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



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

Still waiting on detailed testing information for this change.

Testing Done:

~		Passed all tests in http://localhost:8080/js-tests/
~		Tested on my local machine on browsers.
	~	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.

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.

Review Board 6.0 beta 4 (dev)

yyoud88 got a fish trophy!

Make regionCommentBlock movable.

Information

Reviewers

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Summary:

Description:

Testing Done:

Description:

Change Summary:

Commit:

Diff:

Testing Done:

Status: Discarded