Cleanup of diff comment draft saving work
- Refactors the _editDraft property of gr-diff-comment to _messageText.
- Renames the gr-storage element as instantiated in gr-diff-comment from
"localStorage" to "storage".
- The gr-storage diff comment functions are refactored to include
"comment" in their names and to accept a location object rather than 4
separate arguments.
- Added a comment describing the use of a promise in the _loadLocalDraft
method of gr-diff-comment.
- Throttled the invocation of the _cleanupDrafts method of gr-storage to
avoid potentially expensive crawls over all localStorage entries.
- Various other smaller fixes.
Bug: Issue 3787
Change-Id: Idf96051c0d56d6ce8b15f55ca2680363bd1ca805
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html
index fab0425..6e7a68a 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.html
@@ -129,7 +129,7 @@
class="editMessage"
disabled="{{disabled}}"
rows="4"
- bind-value="{{_editDraft}}"
+ bind-value="{{_messageText}}"
on-keydown="_handleTextareaKeydown"></iron-autogrow-textarea>
<gr-linked-text class="message"
pre
@@ -141,7 +141,7 @@
<gr-button class="action done" on-tap="_handleDone">Done</gr-button>
<gr-button class="action edit" on-tap="_handleEdit">Edit</gr-button>
<gr-button class="action save" on-tap="_handleSave"
- disabled$="[[_computeSaveDisabled(_editDraft)]]">Save</gr-button>
+ disabled$="[[_computeSaveDisabled(_messageText)]]">Save</gr-button>
<gr-button class="action cancel" on-tap="_handleCancel" hidden>Cancel</gr-button>
<div class="danger">
<gr-button class="action discard" on-tap="_handleDiscard">Discard</gr-button>
@@ -149,7 +149,7 @@
</div>
</div>
<gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
- <gr-storage id="localStorage"></gr-storage>
+ <gr-storage id="storage"></gr-storage>
</template>
<script src="gr-diff-comment.js"></script>
</dom-module>
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js
index c28d0ac..f7b5f01 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment.js
@@ -69,25 +69,29 @@
projectConfig: Object,
_xhrPromise: Object, // Used for testing.
- _editDraft: {
+ _messageText: {
type: String,
- observer: '_editDraftChanged',
+ observer: '_messageTextChanged',
},
},
ready: function() {
this._loadLocalDraft().then(function(loadedLocal) {
- this._editDraft = (this.comment && this.comment.message) || '';
- this.editing = !this._editDraft.length || loadedLocal;
+ this._messageText = (this.comment && this.comment.message) || '';
+ this.editing = !this._messageText.length || loadedLocal;
}.bind(this));
},
save: function() {
- this.comment.message = this._editDraft;
+ this.comment.message = this._messageText;
this.disabled = true;
- this.$.localStorage.eraseDraft(this.changeNum, this.patchNum,
- this.comment.path, this.comment.line);
+ this.$.storage.eraseDraftComment({
+ changeNum: this.changeNum,
+ patchNum: this.patchNum,
+ path: this.comment.path,
+ line: this.comment.line,
+ });
this._xhrPromise = this._saveDraft(this.comment).then(function(response) {
this.disabled = false;
@@ -147,23 +151,27 @@
}
},
- _editDraftChanged: function(newValue, oldValue) {
+ _messageTextChanged: function(newValue, oldValue) {
if (this.comment && this.comment.id) { return; }
this.debounce('store', function() {
- var message = this._editDraft;
+ var message = this._messageText;
- // If the draft has been modified to be empty, then erase the storage
- // entry.
- if ((!this._editDraft || !this._editDraft.length) && oldValue) {
- this.$.localStorage.eraseDraft(this.changeNum, this.patchNum,
- this.comment.path, this.comment.line);
- return;
+ var commentLocation = {
+ changeNum: this.changeNum,
+ patchNum: this.patchNum,
+ path: this.comment.path,
+ line: this.comment.line,
+ };
+
+ if ((!this._messageText || !this._messageText.length) && oldValue) {
+ // If the draft has been modified to be empty, then erase the storage
+ // entry.
+ this.$.storage.eraseDraftComment(commentLocation);
+ } else {
+ this.$.storage.setDraftComment(commentLocation, message);
}
-
- this.$.localStorage.setDraft(this.changeNum, this.patchNum,
- this.comment.path, this.comment.line, message);
- }.bind(this), STORAGE_DEBOUNCE_INTERVAL);
+ }, STORAGE_DEBOUNCE_INTERVAL);
},
_handleLinkTap: function(e) {
@@ -195,7 +203,7 @@
_handleEdit: function(e) {
this._preventDefaultAndBlur(e);
- this._editDraft = this.comment.message;
+ this._messageText = this.comment.message;
this.editing = true;
},
@@ -210,7 +218,7 @@
this.fire('comment-discard');
return;
}
- this._editDraft = this.comment.message;
+ this._messageText = this.comment.message;
this.editing = false;
},
@@ -252,6 +260,8 @@
},
_loadLocalDraft: function() {
+ // Use an async promise to avoid blocking render on potentially slow
+ // localStorage calls.
return new Promise(function(resolve) {
this.async(function() {
// Only apply local drafts to comments that haven't been saved
@@ -261,8 +271,12 @@
return;
}
- var draft = this.$.localStorage.getDraft(this.changeNum,
- this.patchNum, this.comment.path, this.comment.line);
+ var draft = this.$.storage.getDraftComment({
+ changeNum: this.changeNum,
+ patchNum: this.patchNum,
+ path: this.comment.path,
+ line: this.comment.line,
+ });
if (draft) {
this.comment.message = draft.message;
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html
index a333e14..8ef222e 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html
+++ b/polygerrit-ui/app/elements/diff/gr-diff-comment/gr-diff-comment_test.html
@@ -183,11 +183,11 @@
MockInteractions.tap(element.$$('.edit'));
assert.isTrue(element.editing);
- element._editDraft = '';
+ element._messageText = '';
// Save should be disabled on an empty message.
var disabled = element.$$('.save').hasAttribute('disabled');
assert.isTrue(disabled, 'save button should be disabled.');
- element._editDraft = ' ';
+ element._messageText = ' ';
disabled = element.$$('.save').hasAttribute('disabled');
assert.isTrue(disabled, 'save button should be disabled.');
@@ -206,7 +206,7 @@
test('draft saving/editing', function(done) {
element.draft = true;
MockInteractions.tap(element.$$('.edit'));
- element._editDraft = 'good news, everyone!';
+ element._messageText = 'good news, everyone!';
MockInteractions.tap(element.$$('.save'));
assert.isTrue(element.disabled,
'Element should be disabled when creating draft.');
@@ -218,7 +218,7 @@
assert.isFalse(element.editing);
}).then(function() {
MockInteractions.tap(element.$$('.edit'));
- element._editDraft = 'You’ll be delivering a package to Chapek 9, a ' +
+ element._messageText = 'You’ll be delivering a package to Chapek 9, a ' +
'world where humans are killed on sight.';
MockInteractions.tap(element.$$('.save'));
assert.isTrue(element.disabled,
