Fix observing comment threads in gr-diff
Observing the DOM nodes of <gr-diff> should not be tied to the element
being connected or not. If the rendered diff is supposed to be kept
around, then the observers also have to be kept around. Node observing
is used for detecting commen thread elements being added to <gr-diff>.
The main goal is to add <slot> elements on-the-fly, so that comment
threads end up in the correct location in the diff.
Start observing when diff rendering has completed.
Stop observing when the diff is cleared or freshly rendered.
Otherwise don't do anything to the node observers.
It is unclear why we have two node observers instead of one. Added a
TODO.
Generally we think that diff components do too much in
`(dis)connectedCallback()`. They should maybe not worry so much about
their connected state.
Also we believe that re-using of <gr-diff-host> and <gr-diff> across
files is probably not a great idea. This is likely much more trouble
than it is worth. We may run into inconsistent state (threads for old
file, diff content for new file). And cancelling+initializing is much
more error prone than just throwing away and re-creating a component.
Release-Notes: skip
Change-Id: Id863110451f7c5af505f724fc92d2167917e3c77
diff --git a/plugins/delete-project b/plugins/delete-project
index ae4ee2a..b183ee5 160000
--- a/plugins/delete-project
+++ b/plugins/delete-project
@@ -1 +1 @@
-Subproject commit ae4ee2acf4a72c508d079ccd9666a64e8bcf6dae
+Subproject commit b183ee5230273670f3235cc5b3cf32562ccfb7ee
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
index efa39a2..8257adc 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff.ts
@@ -298,15 +298,12 @@
override connectedCallback() {
super.connectedCallback();
- this._observeNodes();
this.isAttached = true;
}
override disconnectedCallback() {
this.isAttached = false;
this.renderDiffTableTask?.cancel();
- this._unobserveIncrementalNodes();
- this._unobserveNodes();
this.diffSelection.cleanup();
this.highlights.cleanup();
this.diffBuilder.cancel();
@@ -376,7 +373,10 @@
: document.getSelection();
}
+ // TODO: Merge _nodeObserver and _incrementalNodeObserver. It is not clear why
+ // we have two separate observers.
_observeNodes() {
+ this._unobserveNodes();
this._nodeObserver = (dom(this) as PolymerDomWrapper).observeNodes(info => {
const addedThreadEls = info.addedNodes.filter(isThreadEl);
const removedThreadEls = info.removedNodes.filter(isThreadEl);
@@ -831,6 +831,8 @@
}
_renderDiffTable() {
+ this._unobserveNodes();
+ this._unobserveIncrementalNodes();
if (!this.prefs) {
fireEvent(this, 'render');
return;
@@ -874,10 +876,15 @@
element.remove()
);
this._setLoading(false);
- this._unobserveIncrementalNodes();
+ this._observeNodes();
+ this._observeIncrementalNodes();
// We are just converting 'render-content' into 'render' here. Maybe we
// should retire the 'render' event in favor of 'render-content'?
fireEvent(this, 'render');
+ }
+
+ private _observeIncrementalNodes() {
+ this._unobserveIncrementalNodes();
this._incrementalNodeObserver = (
dom(this) as PolymerDomWrapper
).observeNodes(info => {
@@ -972,12 +979,14 @@
(dom(this) as PolymerDomWrapper).unobserveNodes(
this._incrementalNodeObserver
);
+ this._incrementalNodeObserver = undefined;
}
}
_unobserveNodes() {
if (this._nodeObserver) {
(dom(this) as PolymerDomWrapper).unobserveNodes(this._nodeObserver);
+ this._nodeObserver = undefined;
}
}
@@ -992,6 +1001,7 @@
}
clearDiffContent() {
+ this._unobserveNodes();
this._unobserveIncrementalNodes();
while (this.$.diffTable.hasChildNodes()) {
this.$.diffTable.removeChild(this.$.diffTable.lastChild!);
