Merge "Fix auto-saving of new comment threads"
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];
