Fix auto-saving of new comment threads
<gr-diff-host> has a sophisticated `_threadsChanged()` method that is
called whenever there is a small change to the comments-model. It
carefully compares thread objects from the model with the existing DOM
elements for these, and creates, updates or removes elements as
necessary.
Since auto-saving of comments was implemented this detection logic was
missing the ability to match the new saved thread object from the model
with the existing "unsaved" DOM element that had triggered the
auto-save, because there is no rootId to match.
We are adding some logic for matching up the newly saved threads with
the unsaved elements.
We are also making sure that <gr-diff-host> never removes any unsaved
comments that are currently being edited.
Change-Id: I0a31dad3f2f55c6c18b8dccc775cb28dfd2e3b42
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
index 25a1a00..1c00c95 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
@@ -37,6 +37,7 @@
} from '../../../utils/patch-set-util';
import {
CommentThread,
+ equalLocation,
isInBaseOfPatchRange,
isInRevisionOfPatchRange,
} from '../../../utils/comment-util';
@@ -738,15 +739,36 @@
_threadsChanged(threads: CommentThread[]) {
const rootIdToThreadEl = new Map<UrlEncodedCommentId, GrCommentThread>();
+ const unsavedThreadEls: GrCommentThread[] = [];
for (const threadEl of this.getThreadEls()) {
if (threadEl.rootId) {
rootIdToThreadEl.set(threadEl.rootId, threadEl);
+ } else {
+ // Unsaved thread els must have editing:true, just being defensive here.
+ if (threadEl.editing) unsavedThreadEls.push(threadEl);
}
}
const dontRemove = new Set<GrCommentThread>();
for (const thread of threads) {
- const existingThreadEl =
+ // Let's find an existing DOM element matching the thread. Normally this
+ // is as simple as matching the rootIds.
+ let existingThreadEl =
thread.rootId && rootIdToThreadEl.get(thread.rootId);
+ // But unsaved threads don't have rootIds. The incoming thread might be
+ // the saved version of the unsaved thread element. To verify that we
+ // check that the thread only has one comment and that their location is
+ // identical.
+ // TODO(brohlfs): This matching is not perfect. You could quickly create
+ // two new threads on the same line/range. Then this code just makes a
+ // random guess.
+ if (!existingThreadEl && thread.comments?.length === 1) {
+ for (const unsavedThreadEl of unsavedThreadEls) {
+ if (equalLocation(unsavedThreadEl.thread, thread)) {
+ existingThreadEl = unsavedThreadEl;
+ break;
+ }
+ }
+ }
if (existingThreadEl) {
existingThreadEl.thread = thread;
dontRemove.add(existingThreadEl);
@@ -759,6 +781,10 @@
// Remove all threads that are no longer existing.
for (const threadEl of this.getThreadEls()) {
if (dontRemove.has(threadEl)) continue;
+ // The user may have opened a couple of comment boxes for editing. They
+ // might be unsaved and thus not be reflected in `threads` yet, so let's
+ // keep them open.
+ if (threadEl.editing && threadEl.thread?.comments.length === 0) continue;
threadEl.remove();
}
const portedThreadsCount = threads.filter(thread => thread.ported).length;
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
index 6149c82..888dce3 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
@@ -1166,6 +1166,47 @@
assert.equal(threads.length, 2);
});
+ test('unsaved thread changes to draft', async () => {
+ element.patchRange = {
+ basePatchNum: 2,
+ patchNum: 3,
+ };
+ element.file = {basePath: 'file_renamed.txt', path: element.path};
+ element.threads = [];
+ await flush();
+
+ element.dispatchEvent(new CustomEvent('create-comment', {
+ detail: {
+ side: Side.RIGHT,
+ path: element.path,
+ lineNum: 13,
+ },
+ }));
+ await flush();
+ assert.equal(element.getThreadEls().length, 1);
+ const threadEl = element.getThreadEls()[0];
+ assert.equal(threadEl.thread.line, 13);
+ assert.isDefined(threadEl.unsavedComment);
+ assert.equal(threadEl.thread.comments.length, 0);
+
+ const draftThread = createCommentThread([{
+ path: element.path,
+ patch_set: 3,
+ line: 13,
+ __draft: true,
+ }]);
+ element.threads = [draftThread];
+ await flush();
+
+ // We expect that no additional thread element was created.
+ assert.equal(element.getThreadEls().length, 1);
+ // In fact the thread element must still be the same.
+ assert.equal(element.getThreadEls()[0], threadEl);
+ // But it must have been updated from unsaved to draft:
+ assert.isUndefined(threadEl.unsavedComment);
+ assert.equal(threadEl.thread.comments.length, 1);
+ });
+
test('thread should use new file path if first created ' +
'on patch set (left) but is base', async () => {
element.patchRange = {
diff --git a/polygerrit-ui/app/utils/comment-util.ts b/polygerrit-ui/app/utils/comment-util.ts
index 966a75c..49e9674 100644
--- a/polygerrit-ui/app/utils/comment-util.ts
+++ b/polygerrit-ui/app/utils/comment-util.ts
@@ -205,6 +205,21 @@
rangeInfoLost?: boolean; // if BE was unable to determine a range for this
}
+export function equalLocation(t1?: CommentThread, t2?: CommentThread) {
+ if (t1 === t2) return true;
+ if (t1 === undefined || t2 === undefined) return false;
+ return (
+ t1.path === t2.path &&
+ t1.patchNum === t2.patchNum &&
+ t1.commentSide === t2.commentSide &&
+ t1.line === t2.line &&
+ t1.range?.start_line === t2.range?.start_line &&
+ t1.range?.start_character === t2.range?.start_character &&
+ t1.range?.end_line === t2.range?.end_line &&
+ t1.range?.end_character === t2.range?.end_character
+ );
+}
+
export function getLastComment(thread: CommentThread): CommentInfo | undefined {
const len = thread.comments.length;
return thread.comments[len - 1];
