Update thread in file view if user updates it in other places
Currently user can interact with the comment threads in the change
view through:
1. reply dialog
2. change log
3. comments tab
4. individually expanded diff in file list
When user creates or updates a draft in views 1,2 or 3 they send
an event to change-view(comment-refresh) which requests new drafts
and updates the ChangeComments object.
The comment threads in diffs are however not updated if they were
already expanded.
This leads to inconsistent state where the thread is updated in
one view but not in another view.
This inconsistency also leads to other inconsistent states such as
creating two different drafts on the same thread in two different
views. (https://imgur.com/a/ul1X23P)
gr-file-list already has the method reloadCommentsForThreadWithRootId
which updates a thread with given rootId if it's updated in other
places.
gr-file-list does not render the gr-thread-list element, therefore
changing the comment thread in file view itself does not fire the
thread-list-modified event and cause re-rendering of the thread.
Change-Id: I77f5fea9083f75a11aa3ffc1530e3e061d74aa75
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
index 950fbf3..684891f 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -145,6 +145,7 @@
EditableContentSaveEvent,
OpenFixPreviewEvent,
SwitchTabEvent,
+ ThreadListModifiedEvent,
} from '../../../types/events';
import {GrButton} from '../../shared/gr-button/gr-button';
import {GrMessagesList} from '../gr-messages-list/gr-messages-list';
@@ -600,6 +601,11 @@
this._handleReloadCommentThreads()
);
+ this.addEventListener(
+ 'thread-list-modified',
+ (e: ThreadListModifiedEvent) => this._handleReloadDiffComments(e)
+ );
+
this.addEventListener('open-reply-dialog', () => this._openReplyDialog());
}
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts
index cb8ed6a..4fcc9fe 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts
@@ -646,7 +646,6 @@
change-num="[[_changeNum]]"
logged-in="[[_loggedIn]]"
only-show-robot-comments-with-human-reply=""
- on-thread-list-modified="_handleReloadDiffComments"
unresolved-only
></gr-thread-list>
</template>
@@ -674,7 +673,6 @@
logged-in="[[_loggedIn]]"
hide-toggle-buttons
empty-thread-msg="[[_messages.NO_ROBOT_COMMENTS_THREADS_MSG]]"
- on-thread-list-modified="_handleReloadDiffComments"
>
</gr-thread-list>
<template is="dom-if" if="[[_showRobotCommentsButton]]">
diff --git a/polygerrit-ui/app/elements/change/gr-message/gr-message.ts b/polygerrit-ui/app/elements/change/gr-message/gr-message.ts
index a25ff20..fb20f0c 100644
--- a/polygerrit-ui/app/elements/change/gr-message/gr-message.ts
+++ b/polygerrit-ui/app/elements/change/gr-message/gr-message.ts
@@ -45,7 +45,6 @@
import {hasOwnProperty} from '../../../utils/common-util';
import {appContext} from '../../../services/app-context';
import {pluralize} from '../../../utils/string-util';
-import {fireEvent} from '../../../utils/event-util';
import {GerritNav} from '../../core/gr-navigation/gr-navigation';
const PATCH_SET_PREFIX_PATTERN = /^(?:Uploaded\s*)?(?:P|p)atch (?:S|s)et \d+:\s*(.*)/;
@@ -226,15 +225,6 @@
return pluralize(threadsLength, 'comment');
}
- _onThreadListModified() {
- // TODO(taoalpha): this won't propagate the changes to the files
- // should consider replacing this with either top level events
- // or gerrit level events
-
- // emit the event so change-view can also get updated with latest changes
- fireEvent(this, 'comment-refresh');
- }
-
_computeMessageContentExpanded(content?: string, tag?: ReviewInputTag) {
return this._computeMessageContent(true, content, tag);
}
diff --git a/polygerrit-ui/app/elements/change/gr-message/gr-message_html.ts b/polygerrit-ui/app/elements/change/gr-message/gr-message_html.ts
index 815d8bb..57beacf 100644
--- a/polygerrit-ui/app/elements/change/gr-message/gr-message_html.ts
+++ b/polygerrit-ui/app/elements/change/gr-message/gr-message_html.ts
@@ -256,7 +256,6 @@
change-num="[[changeNum]]"
logged-in="[[_loggedIn]]"
hide-toggle-buttons
- on-thread-list-modified="_onThreadListModified"
>
</gr-thread-list>
</template>
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
index f4d698ea..8801150 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
@@ -1463,15 +1463,6 @@
return provider;
}
- _onThreadListModified() {
- // TODO(taoalpha): this won't propogate the changes to the files
- // should consider replacing this with either top level events
- // or gerrit level events
-
- // emit the event so change-view can also get updated with latest changes
- fireEvent(this, 'comment-refresh');
- }
-
reportAttentionSetChanges(
modified: boolean,
addedSet?: AttentionSetInput[],
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_html.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_html.ts
index 0948c1e..ca9795ea 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_html.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_html.ts
@@ -357,7 +357,6 @@
change-num="[[change._number]]"
logged-in="true"
hide-toggle-buttons=""
- on-thread-list-modified="_onThreadListModified"
>
</gr-thread-list>
<span
diff --git a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts
index ff30452..4976503 100644
--- a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list.ts
@@ -33,6 +33,7 @@
import {ChangeInfo} from '../../../types/common';
import {CommentThread, isDraft, UIRobot} from '../../../utils/comment-util';
import {pluralize} from '../../../utils/string-util';
+import {fireThreadListModifiedEvent} from '../../../utils/event-util';
interface CommentThreadWithInfo {
thread: CommentThread;
@@ -421,11 +422,7 @@
}
_handleCommentsChanged(e: CustomEvent) {
- this.dispatchEvent(
- new CustomEvent('thread-list-modified', {
- detail: {rootId: e.detail.rootId, path: e.detail.path},
- })
- );
+ fireThreadListModifiedEvent(this, e.detail.rootId, e.detail.path);
}
_isOnParent(side?: CommentSide) {
diff --git a/polygerrit-ui/app/types/events.ts b/polygerrit-ui/app/types/events.ts
index 1553187..2398a66 100644
--- a/polygerrit-ui/app/types/events.ts
+++ b/polygerrit-ui/app/types/events.ts
@@ -15,7 +15,7 @@
* limitations under the License.
*/
import {EventApi} from '@polymer/polymer/lib/legacy/polymer.dom';
-import {PatchSetNum} from './common';
+import {PatchSetNum, UrlEncodedCommentId} from './common';
import {UIComment} from '../utils/comment-util';
import {Side} from '../constants/constants';
import {LineNumber} from '../elements/diff/gr-diff/gr-diff-line';
@@ -214,3 +214,16 @@
readonly keyCode: number;
readonly repeat: boolean;
}
+
+export interface ThreadListModifiedDetail {
+ rootId: UrlEncodedCommentId;
+ path: string;
+}
+
+export type ThreadListModifiedEvent = CustomEvent<ThreadListModifiedDetail>;
+
+declare global {
+ interface HTMLElementEventMap {
+ 'thread-list-modified': ThreadListModifiedEvent;
+ }
+}
diff --git a/polygerrit-ui/app/utils/event-util.ts b/polygerrit-ui/app/utils/event-util.ts
index 3dd1b9e..5e1ff44 100644
--- a/polygerrit-ui/app/utils/event-util.ts
+++ b/polygerrit-ui/app/utils/event-util.ts
@@ -15,6 +15,7 @@
* limitations under the License.
*/
+import {UrlEncodedCommentId} from '../types/common';
import {FetchRequest} from '../types/types';
export enum EventType {
@@ -23,6 +24,7 @@
SERVER_ERROR = 'server-error',
NETWORK_ERROR = 'network-error',
TITLE_CHANGE = 'title-change',
+ THREAD_LIST_MODIFIED = 'thread-list-modified',
}
export function fireEvent(target: EventTarget, type: string) {
@@ -83,3 +85,17 @@
})
);
}
+
+export function fireThreadListModifiedEvent(
+ target: EventTarget,
+ rootId: UrlEncodedCommentId,
+ path: string
+) {
+ target.dispatchEvent(
+ new CustomEvent(EventType.THREAD_LIST_MODIFIED, {
+ detail: {rootId, path},
+ composed: true,
+ bubbles: true,
+ })
+ );
+}
