Merge "Support reading VersionedMetaData from an open RevWalk"
diff --git a/gerrit-extension-api/BUCK b/gerrit-extension-api/BUCK
index 7933665..9b83c5a 100644
--- a/gerrit-extension-api/BUCK
+++ b/gerrit-extension-api/BUCK
@@ -75,6 +75,7 @@
'//lib/guice:javax-inject',
'//lib/guice:guice_library',
'//lib/guice:guice-assistedinject',
+ '//gerrit-common:annotations',
],
visibility = ['PUBLIC'],
)
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentGroup.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentGroup.java
index 20dd883..1d198ec 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentGroup.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/CommentGroup.java
@@ -31,10 +31,11 @@
*/
abstract class CommentGroup extends Composite {
+ final DisplaySide side;
+ final int line;
+
private final CommentManager manager;
private final CodeMirror cm;
- private final DisplaySide side;
- private final int line;
private final FlowPanel comments;
private LineWidget lineWidget;
private Timer resizeTimer;
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideCommentGroup.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideCommentGroup.java
index 216fbda..ae6d3c1 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideCommentGroup.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideCommentGroup.java
@@ -21,6 +21,8 @@
import net.codemirror.lib.CodeMirror;
+import java.util.PriorityQueue;
+
/**
* LineWidget attached to a CodeMirror container.
*
@@ -28,14 +30,15 @@
* The group tracks all comment boxes on that same line, and also includes an
* empty padding element to keep subsequent lines vertically aligned.
*/
-class SideBySideCommentGroup extends CommentGroup {
+class SideBySideCommentGroup extends CommentGroup
+ implements Comparable<SideBySideCommentGroup> {
static void pair(SideBySideCommentGroup a, SideBySideCommentGroup b) {
- a.peer = b;
- b.peer = a;
+ a.peers.add(b);
+ b.peers.add(a);
}
private final Element padding;
- private SideBySideCommentGroup peer;
+ private final PriorityQueue<SideBySideCommentGroup> peers;
SideBySideCommentGroup(SideBySideCommentManager manager, CodeMirror cm, DisplaySide side,
int line) {
@@ -45,29 +48,42 @@
padding.setClassName(SideBySideTable.style.padding());
SideBySideChunkManager.focusOnClick(padding, cm.side());
getElement().appendChild(padding);
+ peers = new PriorityQueue<>();
}
SideBySideCommentGroup getPeer() {
- return peer;
+ return peers.peek();
}
@Override
void remove(DraftBox box) {
super.remove(box);
- if (0 < getBoxCount() || 0 < peer.getBoxCount()) {
- resize();
- } else {
+ if (getBoxCount() == 0 && peers.size() == 1
+ && peers.peek().peers.size() > 1) {
+ SideBySideCommentGroup peer = peers.peek();
+ peer.peers.remove(this);
detach();
- peer.detach();
+ if (peer.getBoxCount() == 0 && peer.peers.size() == 1
+ && peer.peers.peek().getBoxCount() == 0) {
+ peer.detach();
+ } else {
+ peer.resize();
+ }
+ } else {
+ resize();
}
}
@Override
void init(DiffTable parent) {
- if (getLineWidget() == null && peer.getLineWidget() == null) {
- this.attach(parent);
- peer.attach(parent);
+ if (getLineWidget() == null) {
+ attach(parent);
+ }
+ for (CommentGroup peer : peers) {
+ if (peer.getLineWidget() == null) {
+ peer.attach(parent);
+ }
}
}
@@ -76,20 +92,20 @@
getLineWidget().onRedraw(new Runnable() {
@Override
public void run() {
- if (canComputeHeight() && peer.canComputeHeight()) {
+ if (canComputeHeight() && peers.peek().canComputeHeight()) {
if (getResizeTimer() != null) {
getResizeTimer().cancel();
setResizeTimer(null);
}
- adjustPadding(SideBySideCommentGroup.this, peer);
+ adjustPadding(SideBySideCommentGroup.this, peers.peek());
} else if (getResizeTimer() == null) {
setResizeTimer(new Timer() {
@Override
public void run() {
- if (canComputeHeight() && peer.canComputeHeight()) {
+ if (canComputeHeight() && peers.peek().canComputeHeight()) {
cancel();
setResizeTimer(null);
- adjustPadding(SideBySideCommentGroup.this, peer);
+ adjustPadding(SideBySideCommentGroup.this, peers.peek());
}
}
});
@@ -102,7 +118,7 @@
@Override
void resize() {
if (getLineWidget() != null) {
- adjustPadding(this, peer);
+ adjustPadding(this, peers.peek());
}
}
@@ -117,6 +133,16 @@
private static void adjustPadding(SideBySideCommentGroup a, SideBySideCommentGroup b) {
int apx = a.computeHeight();
int bpx = b.computeHeight();
+ for (SideBySideCommentGroup otherPeer : a.peers) {
+ if (otherPeer != b) {
+ bpx += otherPeer.computeHeight();
+ }
+ }
+ for (SideBySideCommentGroup otherPeer : b.peers) {
+ if (otherPeer != a) {
+ apx += otherPeer.computeHeight();
+ }
+ }
int h = Math.max(apx, bpx);
a.padding.getStyle().setHeight(Math.max(0, h - apx), Unit.PX);
b.padding.getStyle().setHeight(Math.max(0, h - bpx), Unit.PX);
@@ -125,4 +151,14 @@
a.updateSelection();
b.updateSelection();
}
+
+ @Override
+ public int compareTo(SideBySideCommentGroup o) {
+ if (side == o.side) {
+ return line - o.line;
+ } else {
+ throw new IllegalStateException(
+ "Cannot compare SideBySideCommentGroup with different sides");
+ }
+ }
}
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideCommentManager.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideCommentManager.java
index a2af3a1c..2b83b71 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideCommentManager.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/diff/SideBySideCommentManager.java
@@ -23,6 +23,7 @@
import net.codemirror.lib.TextMarker.FromTo;
import java.util.Collection;
+import java.util.Map;
import java.util.SortedMap;
/** Tracks comment widgets for {@link SideBySide}. */
@@ -94,36 +95,33 @@
@Override
CommentGroup group(DisplaySide side, int line) {
- SideBySideCommentGroup w = (SideBySideCommentGroup) map(side).get(line);
- if (w != null) {
- return w;
+ CommentGroup existing = map(side).get(line);
+ if (existing != null) {
+ return existing;
}
- int lineA;
- int lineB;
- if (line == 0) {
- lineA = lineB = 0;
- } else if (side == DisplaySide.A) {
- lineA = line;
- lineB = host.lineOnOther(side, line - 1).getLine() + 1;
+ SideBySideCommentGroup newGroup = newGroup(side, line);
+ Map<Integer, CommentGroup> map =
+ side == DisplaySide.A ? sideA : sideB;
+ Map<Integer, CommentGroup> otherMap =
+ side == DisplaySide.A ? sideB : sideA;
+ map.put(line, newGroup);
+ int otherLine = host.lineOnOther(side, line - 1).getLine() + 1;
+ existing = map(side.otherSide()).get(otherLine);
+ CommentGroup otherGroup;
+ if (existing != null) {
+ otherGroup = existing;
} else {
- lineA = host.lineOnOther(side, line - 1).getLine() + 1;
- lineB = line;
+ otherGroup = newGroup(side.otherSide(), otherLine);
+ otherMap.put(otherLine, otherGroup);
}
-
- SideBySideCommentGroup a = newGroup(DisplaySide.A, lineA);
- SideBySideCommentGroup b = newGroup(DisplaySide.B, lineB);
- SideBySideCommentGroup.pair(a, b);
-
- sideA.put(lineA, a);
- sideB.put(lineB, b);
+ SideBySideCommentGroup.pair(newGroup, (SideBySideCommentGroup) otherGroup);
if (isAttached()) {
- a.init(host.getDiffTable());
- b.handleRedraw();
+ newGroup.init(host.getDiffTable());
+ otherGroup.handleRedraw();
}
-
- return side == DisplaySide.A ? a : b;
+ return newGroup;
}
private SideBySideCommentGroup newGroup(DisplaySide side, int line) {
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,
diff --git a/polygerrit-ui/app/elements/shared/gr-storage/gr-storage.html b/polygerrit-ui/app/elements/shared/gr-storage/gr-storage.html
index e877aec..74bfcdf 100644
--- a/polygerrit-ui/app/elements/shared/gr-storage/gr-storage.html
+++ b/polygerrit-ui/app/elements/shared/gr-storage/gr-storage.html
@@ -15,6 +15,5 @@
-->
<link rel="import" href="../../../bower_components/polymer/polymer.html">
<dom-module id="gr-storage">
- <template></template>
<script src="gr-storage.js"></script>
</dom-module>
diff --git a/polygerrit-ui/app/elements/shared/gr-storage/gr-storage.js b/polygerrit-ui/app/elements/shared/gr-storage/gr-storage.js
index 79012d4..ef3a6c0 100644
--- a/polygerrit-ui/app/elements/shared/gr-storage/gr-storage.js
+++ b/polygerrit-ui/app/elements/shared/gr-storage/gr-storage.js
@@ -17,10 +17,14 @@
// Date cutoff is one day:
var DRAFT_MAX_AGE = 24*60*60*1000;
+ // Clean up old entries no more frequently than one day.
+ var CLEANUP_THROTTLE_INTERVAL = 24*60*60*1000;
+
Polymer({
is: 'gr-storage',
properties: {
+ _lastCleanup: Number,
_storage: {
type: Object,
value: function() {
@@ -29,27 +33,34 @@
},
},
- getDraft: function(changeNum, patchNum, path, line) {
+ getDraftComment: function(location) {
this._cleanupDrafts();
- return this._getObject(
- this._getDraftKey(changeNum, patchNum, path, line));
+ return this._getObject(this._getDraftKey(location));
},
- setDraft: function(changeNum, patchNum, path, line, message) {
- var key = this._getDraftKey(changeNum, patchNum, path, line);
+ setDraftComment: function(location, message) {
+ var key = this._getDraftKey(location);
this._setObject(key, {message: message, updated: Date.now()});
},
- eraseDraft: function(changeNum, patchNum, path, line) {
- var key = this._getDraftKey(changeNum, patchNum, path, line);
+ eraseDraftComment: function(location) {
+ var key = this._getDraftKey(location);
this._storage.removeItem(key);
},
- _getDraftKey: function(changeNum, patchNum, path, line) {
- return ['draft', changeNum, patchNum, path, line].join(':');
+ _getDraftKey: function(location) {
+ return ['draft', location.changeNum, location.patchNum, location.path,
+ location.line].join(':');
},
_cleanupDrafts: function() {
+ // Throttle cleanup to the throttle interval.
+ if (this._lastCleanup &&
+ Date.now() - this._lastCleanup < CLEANUP_THROTTLE_INTERVAL) {
+ return;
+ }
+ this._lastCleanup = Date.now();
+
var draft;
for (var key in this._storage) {
if (key.indexOf('draft:') === 0) {
diff --git a/polygerrit-ui/app/elements/shared/gr-storage/gr-storage_test.html b/polygerrit-ui/app/elements/shared/gr-storage/gr-storage_test.html
index d21a6c4..4e17cb6 100644
--- a/polygerrit-ui/app/elements/shared/gr-storage/gr-storage_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-storage/gr-storage_test.html
@@ -51,23 +51,29 @@
var patchNum = 5;
var path = 'my_source_file.js';
var line = 123;
+ var location = {
+ changeNum: changeNum,
+ patchNum: patchNum,
+ path: path,
+ line: line,
+ };
// The key is in the expected format.
- var key = element._getDraftKey(changeNum, patchNum, path, line);
+ var key = element._getDraftKey(location);
assert.equal(key, ['draft', changeNum, patchNum, path, line].join(':'));
// There should be no draft initially.
- var draft = element.getDraft(changeNum, patchNum, path, line);
+ var draft = element.getDraftComment(location);
assert.isNotOk(draft);
// Setting the draft stores it under the expected key.
- element.setDraft(changeNum, patchNum, path, line, 'my comment');
+ element.setDraftComment(location, 'my comment');
assert.isOk(storage.getItem(key));
assert.equal(JSON.parse(storage.getItem(key)).message, 'my comment');
assert.isOk(JSON.parse(storage.getItem(key)).updated);
// Erasing the draft removes the key.
- element.eraseDraft(changeNum, patchNum, path, line);
+ element.eraseDraftComment(location);
assert.isNotOk(storage.getItem(key));
cleanupStorage();
@@ -78,7 +84,16 @@
var patchNum = 5;
var path = 'my_source_file.js';
var line = 123;
- var key = element._getDraftKey(changeNum, patchNum, path, line);
+ var location = {
+ changeNum: changeNum,
+ patchNum: patchNum,
+ path: path,
+ line: line,
+ };
+ var key = element._getDraftKey(location);
+
+ // Make sure that the call to cleanup doesn't get throttled.
+ element._lastCleanup = 0;
var cleanupSpy = sinon.spy(element, '_cleanupDrafts');
@@ -89,7 +104,7 @@
}));
// Getting the draft should cause it to be removed.
- var draft = element.getDraft(changeNum, patchNum, path, line);
+ var draft = element.getDraftComment(location);
assert.isTrue(cleanupSpy.called);
assert.isNotOk(draft);
