Summary

Double-clicking to Add a Comment in the Diff View Discards Any Comments Made

Review Request #6340 — Created Sept. 19, 2014 and submitted Sept. 20, 2014, 9:01 a.m.

Information

Owner

dkus

Repository

Review Board

Branch

master

Bugs

3437

Depends On

Commit

b8fd086...

Reviewers

Groups

reviewboard, students

People

Description

When double-clicking on a line number to add a comment in the diff view, this is registered as two seperate clicks. The comment dialog opens on the first click, and then on the second click the dialog is closed, the comment is deleted, and then the dialog is reopened (but no new comment is created). So, clicking save would not save the comment.

To solve this:
Before the comment dialog is shown, first check to see if there is already a comment dialog active. If there is, and it is a comment dialog for the same block as the one that is about to be shown, then don't show a new comment dialog (keep the old one as it displays the same comment). A double-click is registered as two clicks for the same comment block, and so the first click will show the dialog, and the second will do nothing.

Testing Done

Tried all of the following (in Firefox, Chrome):
- Adding comment by double-clicking on a line number and comment flag
- Adding comment by single-clicking on a line number and comment flag
- Adding comment by selecting a range of lines

All JavaScript unit tests passed.

Issues

Description	From	Last Updated
var statements must go to the very top of the function. While JavaScript does allow you to write them anywhere, …	chipx86	Sept. 19, 2014, 10:58 a.m.
No need for the parens around the comparisons.	chipx86	Sept. 19, 2014, 10:58 a.m.
This is actually global state that will apply to every single instance of an AbstractReviewableView. Instead, you want to set …	chipx86	Sept. 19, 2014, 10:59 a.m.
It's safe to just do a comparison without first checking if it's truthy. If it's falsy, then it'll simply fail.	chipx86	Sept. 19, 2014, 10:59 a.m.
The ) { must go on the previous line.	chipx86	Sept. 19, 2014, 10:59 a.m.
listenTo doesn't take that last parameter (this). Instead, that's implied by using listenTo.	chipx86	Sept. 20, 2014, 8:40 a.m.

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/commentDialogView.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/commentDialogView.js

Description:

~		When adding a comment to a set of lines, don't close and reopen the comment dialog if it is already open for that set of lines.
	~	When double-clicking on a line number to add a comment in the diff view, this is registered as two seperate clicks. The comment dialog opens on the first click, and then on the second click the dialog is closed, the comment is deleted, and then the dialog is reopened (but no new comment is created). So, clicking save would not save the comment.
	+
	+	To solve this:
	+	When clicking on a line number range / comment flag to add a comment, if a comment dialog is already open, then first check to see if the line number range you selected is the same as the one associated to the currently open comment. If it is, then there is no need to close / reopen the dialog. A double-click is registered as two clicks on the same line, and so the first click will open the dialog, and the second will do nothing.

Testing Done:

	+	Tried all of the following:
	+	- Adding comment by double-clicking on a line number and comment flag
	+	- Adding comment by single-clicking on a line number and comment flag
	+	- Adding comment by selecting a range of lines

This is a good start, but it'll break on non-diffs. We support commenting on all kinds of things, such as images or PDFs (if using our Power Pack extension).

What I'd suggest doing instead is updating views/abstractReviewableView.js's showCommentDlg function, which is used for all reviewable things, like the diff viewer. You can have it keep a this._activeCommentBlock state (set and cleared at the same times as this.commentDlg). If it's set and it directly matches (simple ===) the one provided to the function, it can just simply return.

I think that will work, and the nice thing is that it should fix this for all reviewable pages.

I do have some general notes on style guidelines, while I'm here.

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

The issue has been resolved. Show all issues

var statements must go to the very top of the function. While JavaScript does allow you to write them anywhere, internally it "hoists" the declarations to the top of the function, which can result in odd behavior in certain cases.
To help minify, we also only ever use one var per function, like this:
var foo = 1,
    bar = 2,
    s = '';

reviewboard/static/rb/js/views/commentDialogView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
No need for the parens around the comparisons.
```

Commit:

-	3f0e76da758d9cb1c086b0479357683d186c20f8
+	6a06608fc6bcebe68ae34c5ef7181201a1d09e5d

Diff:

Revision 2 (+11)

Show changes

reviewboard/static/rb/js/views/abstractReviewableView.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/abstractReviewableView.js



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/abstractReviewableView.js

Description:

		When double-clicking on a line number to add a comment in the diff view, this is registered as two seperate clicks. The comment dialog opens on the first click, and then on the second click the dialog is closed, the comment is deleted, and then the dialog is reopened (but no new comment is created). So, clicking save would not save the comment.

		To solve this:
~		When clicking on a line number range / comment flag to add a comment, if a comment dialog is already open, then first check to see if the line number range you selected is the same as the one associated to the currently open comment. If it is, then there is no need to close / reopen the dialog. A double-click is registered as two clicks on the same line, and so the first click will open the dialog, and the second will do nothing.
	~	Before the comment dialog is shown, first check to see if there is already a comment dialog active. If there is, and it is a comment dialog for the same block as the one that is about to be shown, then don't show a new comment dialog (keep the old one as it displays the same comment). A double-click is registered as two clicks for the same comment block, and so the first click will show the dialog, and the second will do nothing.

Testing Done:

~		Tried all of the following:
	~	Tried all of the following (in Firefox, Chrome):
		- Adding comment by double-clicking on a line number and comment flag
		- Adding comment by single-clicking on a line number and comment flag
		- Adding comment by selecting a range of lines
	+
	+	All JavaScript unit tests passed.

reviewboard/static/rb/js/views/abstractReviewableView.js (Diff revision 2)

The issue has been resolved. Show all issues

This is actually global state that will apply to every single instance of an AbstractReviewableView. Instead, you want to set this in initialize.

reviewboard/static/rb/js/views/abstractReviewableView.js (Diff revision 2)

The issue has been resolved. Show all issues

It's safe to just do a comparison without first checking if it's truthy. If it's falsy, then it'll simply fail.

reviewboard/static/rb/js/views/abstractReviewableView.js (Diff revision 2)
The issue has been resolved. Show all issues
```
The ) { must go on the previous line.
```

Commit:

-	6a06608fc6bcebe68ae34c5ef7181201a1d09e5d
+	fd64f61a2a87b54cf8909b3a255e4828fad30d52

Diff:

Revision 3 (+12 -1)

Show changes

reviewboard/static/rb/js/views/abstractReviewableView.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/abstractReviewableView.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/abstractReviewableView.js

Commit:

-	fd64f61a2a87b54cf8909b3a255e4828fad30d52
+	c48c8b762902ead95d44d5fe97360e815d2b68ac

Diff:

Revision 4 (+9 -1)

Show changes

reviewboard/static/rb/js/views/abstractReviewableView.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/abstractReviewableView.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/abstractReviewableView.js

Summary:

-	[WIP] Issue 3437: Double-clicking to Add a Comment in the Diff View Discards Any Comments Made
+	Double-clicking to Add a Comment in the Diff View Discards Any Comments Made

```
Looking good! One last thing.
```
reviewboard/static/rb/js/views/abstractReviewableView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
listenTo doesn't take that last parameter (this). Instead, that's implied by using listenTo.
```

Change Summary:

Removed unneeded 'this' paramater from the listenTo call.

Commit:

-	c48c8b762902ead95d44d5fe97360e815d2b68ac
+	b8fd0861eef92b76b9c879ca5e50d4886f8287f7

Diff:

Revision 5 (+10 -2)

Show changes

reviewboard/static/rb/js/views/abstractReviewableView.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/abstractReviewableView.js



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/abstractReviewableView.js

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (aac6bda)