Double-clicking to Add a Comment in the Diff View Discards Any Comments Made
Review Request #6340 — Created Sept. 19, 2014 and submitted
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.
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 linesAll JavaScript unit tests passed.
| 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
|
|
|
No need for the parens around the comparisons. |
chipx86
|
|
|
This is actually global state that will apply to every single instance of an AbstractReviewableView. Instead, you want to set … |
chipx86
|
|
|
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
|
|
|
The ) { must go on the previous line. |
chipx86
|
|
|
listenTo doesn't take that last parameter (this). Instead, that's implied by using listenTo. |
chipx86
|
- 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
showCommentDlgfunction, which is used for all reviewable things, like the diff viewer. You can have it keep athis._activeCommentBlockstate (set and cleared at the same times asthis.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.
-
varstatements 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
varper function, like this:var foo = 1, bar = 2, s = ''; -
-
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.
-
Tool: PEP8 Style Checker Ignored Files: 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 Tool: Pyflakes Ignored Files: reviewboard/static/rb/js/views/abstractReviewableView.js
- Change Summary:
-
Removed unneeded 'this' paramater from the listenTo call.
- Commit:
-
c48c8b762902ead95d44d5fe97360e815d2b68acb8fd0861eef92b76b9c879ca5e50d4886f8287f7
Tool: PEP8 Style Checker Ignored Files: reviewboard/static/rb/js/views/commentDialogView.js Tool: Pyflakes Ignored Files: reviewboard/static/rb/js/views/commentDialogView.js