Fix bug and now warning will pop up if try to move to a new comment while editing some unsaved comment.
Review Request #8649 — Created Jan. 21, 2017 and discarded
Fix the bug of unsaved comment being discarded when creating/opening another comment.
For creating a new comment, the code creates a CommentBox(the little green box next to line number) first and then creates a new CommentDialog for editing. For opening another comment, the code only creates a new CommentDialog.
When creating the new CommentDialog, the "dirtiness" of existing CommentDialog is not checked, and thus the unsaved comments would be discarded.
After the fix, a check was done when creating the new CommentDialog. If the old CommentDialog is dirty, pop a warning message. If user chooses not to discard the unsaved comments, the new CommentDialog will not be created and the newly created CommentBox will be deleted in case of creating new comment. If user chooses to discard the unsaved comments, the code works the same way as before.
Test Preperation: create a comment (comment1), write something, and save it.
Test1: Create a comment (comment2), write something, and click on another line number trying to create another comment (comment3).
Test1 Result: Warning pops up.
Test1.1: Click "OK" on the warning popped up in Test1.
Test1.1 Result: comment2 is deleted, and dialog for comment3 pops up.
Test1.2: Click "cancel" on the warning popped up in Test1.
Test1.2 Result: comment3 is deleted, and dialog for comment2 stays in the screen.Test2: Create a comment (comment4), write something, and click on comment1 trying to open it.
Test2 Result: Warning pops up.
Test2.1: Click "OK" on the warning popped up in Test2.
Test2.1 Result: comment4 is deleted, and dialog for comment1 pops up.
Test2.2: Click "cancel" on the warning popped up in Test2.
Test2.2 Result: the dialog for comment4 stays in the screen, nothing happens to comment1.
| Description | From | Last Updated |
|---|---|---|
|
The summary and description don't need to include the bug number (since it's listed in the bugs field). It would … |
 david
|
|
|
This is a .es6.js file, so we should use const instead of var. |
david
|
|
|
This comment should explain in what situation RB.CommentDialogView.create would refuse to create a new dialog. There should also be a … |
david
|
|
|
This can be simplified quite a bit. We should probably also be returning an explicit null instead of just letting … |
david
|
-
-
The summary and description don't need to include the bug number (since it's listed in the bugs field).
It would be nice to separate the description a bit into a few paragraphs--one explaining what the bug was (from a user standpoint), one or more explaining a bit more about what the problem was in the code, and then one about what changes you made. Please make sure it wraps to about 70 columns, too.
Having some blank lines in the testing done section would also make it easier to read.
-
reviewboard/static/rb/js/views/abstractReviewableView.es6.js (Diff revision 1) This is a .es6.js file, so we should use
constinstead ofvar. -
reviewboard/static/rb/js/views/abstractReviewableView.es6.js (Diff revision 1) This comment should explain in what situation
RB.CommentDialogView.createwould refuse to create a new dialog. There should also be a space between "//" and the comment text, and the sentence should start with a capital letter.If the comment gets long enough to wrap onto multiple lines, use a block comment:
/* * This is a very long * comment that has to wrap. */ -
reviewboard/static/rb/js/views/commentDialogView.js (Diff revision 1) This can be simplified quite a bit. We should probably also be returning an explicit
nullinstead of just letting it beundefined.if (instance && instance.model.get('dirty') && !confirm(gettext('You have unsaved changes. Are you sure you want to exit?'))) { return null; }
JO
Change Summary:
Organizing codes and comments.
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+19 -2) |
-
Tool: PEP8 Style Checker Ignored Files: reviewboard/static/rb/js/views/abstractReviewableView.es6.js reviewboard/static/rb/js/views/commentDialogView.js Tool: Pyflakes Ignored Files: reviewboard/static/rb/js/views/abstractReviewableView.es6.js reviewboard/static/rb/js/views/commentDialogView.js
JO
Change Summary:
Removing extra spaces.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+18 -2) |
-
Tool: Pyflakes Ignored Files: reviewboard/static/rb/js/views/abstractReviewableView.es6.js reviewboard/static/rb/js/views/commentDialogView.js Tool: PEP8 Style Checker Ignored Files: reviewboard/static/rb/js/views/abstractReviewableView.es6.js reviewboard/static/rb/js/views/commentDialogView.js
Tool: PEP8 Style Checker Ignored Files: reviewboard/static/rb/js/views/abstractReviewableView.es6.js reviewboard/static/rb/js/views/commentDialogView.js Tool: Pyflakes Ignored Files: reviewboard/static/rb/js/views/abstractReviewableView.es6.js reviewboard/static/rb/js/views/commentDialogView.js