Merge "Bazel: Tune up rbe build"
diff --git a/java/com/google/gerrit/server/restapi/change/PostReview.java b/java/com/google/gerrit/server/restapi/change/PostReview.java
index a360510..562bdf8 100644
--- a/java/com/google/gerrit/server/restapi/change/PostReview.java
+++ b/java/com/google/gerrit/server/restapi/change/PostReview.java
@@ -1272,28 +1272,27 @@
del.add(c);
update.putApproval(normName, (short) 0);
}
- } else if (c != null) {
- // Check if the label exists in the request input (the user voted again). If the user
- // hadn't voted again, there is no need to re-apply the vote.
- if (inLabels.keySet().contains(c.label())) {
- PatchSetApproval.Builder b =
- c.toBuilder()
- .value(ent.getValue())
- .granted(ctx.getWhen())
- .tag(Optional.ofNullable(in.tag));
- ctx.getUser().updateRealAccountId(b::realAccountId);
- c = b.build();
- ups.add(c);
- addLabelDelta(normName, c.value());
- oldApprovals.put(normName, previous.get(normName));
- approvals.put(normName, c.value());
- update.putApproval(normName, ent.getValue());
- } else {
- current.put(normName, c);
- oldApprovals.put(normName, null);
- approvals.put(normName, c.value());
- }
- } else {
+ // Only allow voting again if the vote is copied over from a past patch-set, or the
+ // values are different.
+ } else if (c != null
+ && (c.value() != ent.getValue() || isApprovalCopiedOver(c, ctx.getNotes()))) {
+ PatchSetApproval.Builder b =
+ c.toBuilder()
+ .value(ent.getValue())
+ .granted(ctx.getWhen())
+ .tag(Optional.ofNullable(in.tag));
+ ctx.getUser().updateRealAccountId(b::realAccountId);
+ c = b.build();
+ ups.add(c);
+ addLabelDelta(normName, c.value());
+ oldApprovals.put(normName, previous.get(normName));
+ approvals.put(normName, c.value());
+ update.putApproval(normName, ent.getValue());
+ } else if (c != null && c.value() == ent.getValue()) {
+ current.put(normName, c);
+ oldApprovals.put(normName, null);
+ approvals.put(normName, c.value());
+ } else if (c == null) {
c =
ApprovalsUtil.newApproval(psId, user, lt.getLabelId(), ent.getValue(), ctx.getWhen())
.tag(Optional.ofNullable(in.tag))
@@ -1319,6 +1318,17 @@
return !del.isEmpty() || !ups.isEmpty();
}
+ /**
+ * Approval is copied over if it doesn't exist in the approvals of the current patch-set
+ * according to change notes (which means it was computed in {@link
+ * com.google.gerrit.server.ApprovalInference})
+ */
+ private boolean isApprovalCopiedOver(
+ PatchSetApproval patchSetApproval, ChangeNotes changeNotes) {
+ return !changeNotes.getApprovals().get(changeNotes.getChange().currentPatchSetId()).stream()
+ .anyMatch(p -> p.equals(patchSetApproval));
+ }
+
private void validatePostSubmitLabels(
ChangeContext ctx,
LabelTypes labelTypes,
diff --git a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
index 0b2e0f7..c8b1715 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
@@ -63,7 +63,6 @@
import com.google.gerrit.server.restapi.change.OnPostReview;
import com.google.gerrit.server.restapi.change.PostReview;
import com.google.gerrit.server.update.CommentsRejectedException;
-import com.google.gerrit.testing.FakeEmailSender;
import com.google.gerrit.testing.TestCommentHelper;
import com.google.inject.Inject;
import com.google.inject.Module;
@@ -597,7 +596,7 @@
input = new ReviewInput().label(LabelId.CODE_REVIEW, 2);
gApi.changes().id(r.getChangeId()).current().review(input);
testOnPostReview.assertApproval(
- LabelId.CODE_REVIEW, /* expectedOldValue= */ 2, /* expectedNewValue= */ 2);
+ LabelId.CODE_REVIEW, /* expectedOldValue= */ null, /* expectedNewValue= */ 2);
// Delete the vote.
input = new ReviewInput().label(LabelId.CODE_REVIEW, 0);
@@ -627,21 +626,19 @@
assertThat(r.getChange().approvals().values()).hasSize(1);
List<ChangeMessageInfo> changeMessages = gApi.changes().id(r.getChangeId()).messages();
- // The two latest change messages are both about Code-Review+2
+ // Only the last change message is about Code-Review+2
assertThat(Iterables.getLast(changeMessages).message).isEqualTo("Patch Set 1: Code-Review+2");
changeMessages.remove(changeMessages.size() - 1);
- assertThat(Iterables.getLast(changeMessages).message).isEqualTo("Patch Set 1: Code-Review+2");
+ assertThat(Iterables.getLast(changeMessages).message)
+ .isNotEqualTo("Patch Set 1: Code-Review+2");
- // The two latest emails are about Code-Review +2.
- List<FakeEmailSender.Message> messages = sender.getMessages();
- assertThat(messages).hasSize(2);
- for (FakeEmailSender.Message message : messages) {
- assertThat(message.body()).contains("Patch Set 1: Code-Review+2");
- }
+ // Only one email is about Code-Review +2 was sent.
+ assertThat(Iterables.getOnlyElement(sender.getMessages()).body())
+ .contains("Patch Set 1: Code-Review+2");
}
@Test
- public void votingTheSameVoteSecondTimeExtendsOnPostReview() throws Exception {
+ public void votingTheSameVoteSecondTimeExtendsOnPostReviewWithOldNullValue() throws Exception {
PushOneCommit.Result r = createChange();
// Add a new vote.
@@ -656,12 +653,12 @@
gApi.changes().id(r.getChangeId()).current().review(input);
testOnPostReview.assertApproval(
- LabelId.CODE_REVIEW, /* expectedOldValue= */ 2, /* expectedNewValue= */ 2);
+ LabelId.CODE_REVIEW, /* expectedOldValue= */ null, /* expectedNewValue= */ 2);
}
}
@Test
- public void votingTheSameVoteSecondTimeFiresOnCommentAdded() throws Exception {
+ public void votingTheSameVoteSecondTimeDoesNotFireOnCommentAdded() throws Exception {
PushOneCommit.Result r = createChange();
// Add a new vote.
@@ -675,8 +672,8 @@
input = new ReviewInput().label(LabelId.CODE_REVIEW, 2);
gApi.changes().id(r.getChangeId()).current().review(input);
- assertThat(testListener.lastCommentAddedEvent.getComment())
- .isEqualTo("Patch Set 1: Code-Review+2");
+ // Event not fired.
+ assertThat(testListener.lastCommentAddedEvent).isNull();
}
}
diff --git a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
index 67e62dd..abfd7896 100644
--- a/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/revision/RevisionIT.java
@@ -176,12 +176,12 @@
assertThat(approval.postSubmit).isNull();
assertPermitted(gApi.changes().id(changeId).get(DETAILED_LABELS), LabelId.CODE_REVIEW, 1, 2);
- // Repeating the current label is allowed. Flips the postSubmit since technically this is a
- // new vote.
+ // Repeating the current label is allowed. Does not flip the postSubmit bit due to
+ // deduplication codepath.
gApi.changes().id(changeId).current().review(ReviewInput.recommend());
approval = getApproval(changeId, label);
assertThat(approval.value).isEqualTo(1);
- assertThat(approval.postSubmit).isTrue();
+ assertThat(approval.postSubmit).isNull();
// Reducing vote is not allowed.
ResourceConflictException thrown =
@@ -193,7 +193,7 @@
.isEqualTo("Cannot reduce vote on labels for closed change: Code-Review");
approval = getApproval(changeId, label);
assertThat(approval.value).isEqualTo(1);
- assertThat(approval.postSubmit).isTrue();
+ assertThat(approval.postSubmit).isNull();
// Increasing vote is allowed.
gApi.changes().id(changeId).current().review(ReviewInput.approve());
diff --git a/polygerrit-ui/app/api/annotation.ts b/polygerrit-ui/app/api/annotation.ts
index c046b4f..e58bdd5 100644
--- a/polygerrit-ui/app/api/annotation.ts
+++ b/polygerrit-ui/app/api/annotation.ts
@@ -14,18 +14,14 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-import {CoverageRange, Side} from './diff';
+import {CoverageRange, GrDiffLine, Side} from './diff';
import {StyleObject} from './styles';
-export type AddLayerFunc = (ctx: AnnotationContext) => void;
-
-export type NotifyFunc = (
- path: string,
- start: number,
- end: number,
- side: Side
-) => void;
-
+/**
+ * This is the callback object that Gerrit calls once for each diff. Gerrit
+ * is then responsible for styling the diff according the returned array of
+ * CoverageRanges.
+ */
export type CoverageProvider = (
changeNum: number,
path: string,
@@ -34,14 +30,35 @@
/**
* This is a ChangeInfo object as defined here:
* https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#change-info
- * We neither want to repeat it nor add a dependency on it here.
+ * At the moment we neither want to repeat it nor add a dependency on it here.
+ * TODO: Create a dedicated smaller object for exposing a change in the plugin
+ * API. Or allow the plugin API to depend on the entire rest API.
*/
change?: unknown
) => Promise<Array<CoverageRange>>;
+export type AnnotationCallback = (ctx: AnnotationContext) => void;
+
+/**
+ * This object is passed to the plugin from Gerrit for each line of a diff that
+ * is being rendered. The plugin can then call annotateRange() or
+ * annotateLineNumber() to apply additional styles to the diff.
+ */
export interface AnnotationContext {
+ /** Set by Gerrit and consumed by the plugin provided AddLayerFunc. */
+ readonly changeNum: number;
+ /** Set by Gerrit and consumed by the plugin provided AddLayerFunc. */
+ readonly path: string;
+ /** Set by Gerrit and consumed by the plugin provided AddLayerFunc. */
+ readonly line: GrDiffLine;
+ /** Set by Gerrit and consumed by the plugin provided AddLayerFunc. */
+ readonly contentEl: HTMLElement;
+ /** Set by Gerrit and consumed by the plugin provided AddLayerFunc. */
+ readonly lineNumberEl: HTMLElement;
+
/**
- * Method to add annotations to a content line.
+ * Can be called by the plugin to style a part of the given line of the
+ * context.
*
* @param offset The char offset where the update starts.
* @param length The number of chars that the update covers.
@@ -56,7 +73,8 @@
): void;
/**
- * Method to add a CSS class to the line number TD element.
+ * Can be called by the plugin to style a part of the given line of the
+ * context.
*
* @param styleObject The style object for the range.
* @param side The side of the update. ('left' or 'right')
@@ -66,23 +84,12 @@
export interface AnnotationPluginApi {
/**
- * Register a function to call to apply annotations. Plugins should use
- * GrAnnotationActionsContext.annotateRange and
- * GrAnnotationActionsContext.annotateLineNumber to apply a CSS class to the
- * line content or the line number.
- *
- * @param addLayerFunc The function
- * that will be called when the AnnotationLayer is ready to annotate.
+ * Registers a callback for applying annotations. Gerrit will call the
+ * callback for every line of every file that is rendered and pass the
+ * information about the file and line as an AnnotationContext, which also
+ * provides methods for the plugin to style the content.
*/
- addLayer(addLayerFunc: AddLayerFunc): AnnotationPluginApi;
-
- /**
- * The specified function will be called with a notify function for the plugin
- * to call when it has all required data for annotation. Optional.
- *
- * @param notifyFunc See doc of the notify function below to see what it does.
- */
- addNotifier(notifyFunc: (n: NotifyFunc) => void): AnnotationPluginApi;
+ setLayer(callback: AnnotationCallback): AnnotationPluginApi;
/**
* The specified function will be called when a gr-diff component is built,
@@ -117,9 +124,10 @@
): AnnotationPluginApi;
/**
- * The notify function will call the listeners of all required annotation
- * layers. Intended to be called by the plugin when all required data for
- * annotation is available.
+ * For plugins notifying Gerrit about new annotations being ready to be
+ * applied for a certain range. Gerrit will then re-render the relevant lines
+ * of the diff and call back to the layer annotation function that was
+ * registered in addLayer().
*
* @param path The file path whose listeners should be notified.
* @param start The line where the update starts.
diff --git a/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary.ts b/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary.ts
index 72ed6e6..4af51a9 100644
--- a/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary.ts
@@ -40,6 +40,7 @@
iconForCategory,
iconForStatus,
isRunning,
+ isRunningOrHasCompleted,
} from '../../../services/checks/checks-util';
import {ChangeComments} from '../../diff/gr-comment-api/gr-comment-api';
import {
@@ -288,6 +289,9 @@
:host.new-change-summary-true {
margin-bottom: var(--spacing-m);
}
+ .zeroState {
+ color: var(--primary-text-color);
+ }
td.key {
padding-right: var(--spacing-l);
padding-bottom: var(--spacing-m);
@@ -312,6 +316,11 @@
];
}
+ renderChecksZeroState() {
+ if (this.runs.some(isRunningOrHasCompleted)) return;
+ return html`<span class="font-small zeroState">No results</span>`;
+ }
+
renderChecksChipForCategory(category: Category) {
const icon = iconForCategory(category);
const runs = this.runs.filter(run => hasResultsOf(run, category));
@@ -395,7 +404,7 @@
<tr ?hidden=${!this.showChecksSummary}>
<td class="key">Checks</td>
<td class="value">
- ${this.renderChecksChipForCategory(
+ ${this.renderChecksZeroState()}${this.renderChecksChipForCategory(
Category.ERROR
)}${this.renderChecksChipForCategory(
Category.WARNING
@@ -410,13 +419,13 @@
<tr ?hidden=${!this.newChangeSummaryUiEnabled}>
<td class="key">Comments</td>
<td class="value">
- <gr-summary-chip
- styleType=${SummaryChipStyles.INFO}
+ <span
+ class="font-small zeroState"
?hidden=${!!countResolvedComments ||
!!draftCount ||
!!countUnresolvedComments}
>
- No Comments</gr-summary-chip
+ No Comments</span
><gr-summary-chip
styleType=${SummaryChipStyles.WARNING}
category=${CommentTabState.DRAFTS}
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 c33eb26..8f74f76 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
@@ -118,7 +118,7 @@
GrCommentApi,
ChangeComments,
} from '../../diff/gr-comment-api/gr-comment-api';
-import {hasOwnProperty} from '../../../utils/common-util';
+import {assertIsDefined, hasOwnProperty} from '../../../utils/common-util';
import {GrEditControls} from '../../edit/gr-edit-controls/gr-edit-controls';
import {
CommentThread,
@@ -883,7 +883,7 @@
}
_handleCommitMessageSave(e: EditableContentSaveEvent) {
- if (!this._change) throw new Error('missing required change property');
+ assertIsDefined(this._change, '_change');
if (!this._changeNum)
throw new Error('missing required changeNum property');
// Trim trailing whitespace from each line.
@@ -1424,7 +1424,7 @@
}
_handleMessageAnchorTap(e: CustomEvent<{id: string}>) {
- if (!this._change) throw new Error('missing required change property');
+ assertIsDefined(this._change, '_change');
if (!this._patchRange)
throw new Error('missing required _patchRange property');
const hash = MSG_PREFIX + e.detail.id;
@@ -1706,7 +1706,7 @@
if (this.shouldSuppressKeyboardShortcut(e)) {
return;
}
- if (!this._change) throw new Error('missing required change property');
+ assertIsDefined(this._change, '_change');
if (!this._patchRange)
throw new Error('missing required _patchRange property');
if (this._patchRange.basePatchNum === ParentPatchSetNum) {
@@ -1720,7 +1720,7 @@
if (this.shouldSuppressKeyboardShortcut(e)) {
return;
}
- if (!this._change) throw new Error('missing required change property');
+ assertIsDefined(this._change, '_change');
if (!this._patchRange)
throw new Error('missing required _patchRange property');
if (this._patchRange.basePatchNum === ParentPatchSetNum) {
@@ -1734,7 +1734,7 @@
if (this.shouldSuppressKeyboardShortcut(e)) {
return;
}
- if (!this._change) throw new Error('missing required change property');
+ assertIsDefined(this._change, '_change');
if (!this._patchRange)
throw new Error('missing required _patchRange property');
const latestPatchNum = computeLatestPatchNum(this._allPatchSets);
@@ -1753,7 +1753,7 @@
if (this.shouldSuppressKeyboardShortcut(e)) {
return;
}
- if (!this._change) throw new Error('missing required change property');
+ assertIsDefined(this._change, '_change');
const latestPatchNum = computeLatestPatchNum(this._allPatchSets);
if (!this._patchRange)
throw new Error('missing required _patchRange property');
@@ -1772,7 +1772,7 @@
if (this.shouldSuppressKeyboardShortcut(e)) {
return;
}
- if (!this._change) throw new Error('missing required change property');
+ assertIsDefined(this._change, '_change');
if (!this._patchRange)
throw new Error('missing required _patchRange property');
const latestPatchNum = computeLatestPatchNum(this._allPatchSets);
@@ -1918,7 +1918,7 @@
}
_getProjectConfig() {
- if (!this._change) throw new Error('missing required change property');
+ assertIsDefined(this._change, '_change');
return this.restApiService
.getProjectConfig(this._change.project)
.then(config => {
@@ -2314,7 +2314,7 @@
* (`this._patchRange`) being defined.
*/
_reloadPatchNumDependentResources(rightPatchNumChanged?: boolean) {
- if (!this._changeNum) throw new Error('missing changeNum');
+ assertIsDefined(this._changeNum, '_changeNum');
if (!this._patchRange?.patchNum) throw new Error('missing patchNum');
const promises = [this._getCommitInfo(), this.$.fileList.reload()];
if (rightPatchNumChanged)
@@ -2554,7 +2554,7 @@
}
this._updateCheckTimerHandle = this.async(() => {
- if (!this._change) throw new Error('missing required change property');
+ assertIsDefined(this._change, '_change');
const change = this._change;
fetchChangeUpdates(change, this.restApiService).then(result => {
let toastMessage = null;
@@ -2662,7 +2662,7 @@
GrEditControls
>('#editControls');
if (!controls) throw new Error('Missing edit controls');
- if (!this._change) throw new Error('missing required change property');
+ assertIsDefined(this._change, '_change');
if (!this._patchRange)
throw new Error('missing required _patchRange property');
const path = e.detail.path;
@@ -2697,7 +2697,7 @@
if (!this._selectedRevision) {
return;
}
- if (!this._change) throw new Error('missing required change property');
+ assertIsDefined(this._change, '_change');
let patchNum: PatchSetNum;
if (patchNumStr === 'edit') {
@@ -2745,7 +2745,7 @@
}
_handleStopEditTap() {
- if (!this._change) throw new Error('missing required change property');
+ assertIsDefined(this._change, '_change');
if (!this._patchRange)
throw new Error('missing required _patchRange property');
GerritNav.navigateToChange(this._change, this._patchRange.patchNum);
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 56c1d38..f3fb860 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
@@ -105,7 +105,7 @@
font-size: var(--font-size-mono);
line-height: var(--line-height-mono);
margin-right: var(--spacing-l);
- margin-bottom: var(--spacing-s);
+ margin-bottom: var(--spacing-m);
/* Account for border and padding and rounding errors. */
max-width: calc(72ch + 2px + 2 * var(--spacing-m) + 0.4px);
}
diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
index b4b6d31..5705c4f 100644
--- a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
@@ -451,12 +451,14 @@
?.getElementsByClassName('arrowToCurrentChange')[0]
?.nextElementSibling?.nextElementSibling?.getElementsByTagName('a')[0];
- if (!target || !currentChange) return;
+ if (!target) return;
this.reportingService.reportInteraction('related-change-click', {
sectionName,
index: sectionLinks.indexOf(target) + 1,
countChanges: sectionLinks.length,
- currentChangeIndex: sectionLinks.indexOf(currentChange) + 1,
+ currentChangeIndex: !currentChange
+ ? undefined
+ : sectionLinks.indexOf(currentChange) + 1,
});
}
}
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 36764f3..705a402 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
@@ -92,6 +92,7 @@
} from '@polymer/polymer/interfaces';
import {
areSetsEqual,
+ assertIsDefined,
assertNever,
containsAll,
} from '../../../utils/common-util';
@@ -433,7 +434,7 @@
}
open(focusTarget?: FocusTarget) {
- if (!this.change) throw new Error('missing required change property');
+ assertIsDefined(this.change, 'change');
this.knownLatestState = LatestPatchState.CHECKING;
fetchChangeUpdates(this.change, this.restApiService).then(result => {
this.knownLatestState = result.isLatest
@@ -605,7 +606,7 @@
account: AccountInfoInput | GroupInfoInput,
type: ReviewerType
) {
- if (!this.change) throw new Error('missing required change property');
+ assertIsDefined(this.change, 'change');
if (account._pendingAdd || !isAccount(account)) {
return;
}
@@ -1213,7 +1214,7 @@
}
cancel() {
- if (!this.change) throw new Error('missing required change property');
+ assertIsDefined(this.change, 'change');
if (!this._owner) throw new Error('missing required _owner property');
this.dispatchEvent(
new CustomEvent('cancel', {
@@ -1269,8 +1270,8 @@
}
_saveReview(review: ReviewInput, errFn?: ErrorCallback) {
- if (!this.change) throw new Error('missing required change property');
- if (!this.patchNum) throw new Error('missing required patchNum property');
+ assertIsDefined(this.change, 'change');
+ assertIsDefined(this.patchNum, 'patchNum');
return this.restApiService.saveChangeReview(
this.change._number,
this.patchNum,
@@ -1316,7 +1317,7 @@
}
_getStorageLocation(): StorageLocation {
- if (!this.change) throw new Error('missing required change property');
+ assertIsDefined(this.change, 'change');
return {
changeNum: this.change._number,
patchNum: '@change',
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-results.ts b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
index bc72b2a..df881f3 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-results.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
@@ -21,8 +21,10 @@
import {sharedStyles} from '../../styles/shared-styles';
import {RunResult} from '../../services/checks/checks-model';
import {
+ hasCompleted,
hasCompletedWithoutResults,
iconForCategory,
+ isRunning,
} from '../../services/checks/checks-util';
@customElement('gr-result-row')
@@ -276,6 +278,9 @@
.categoryHeader iron-icon.success {
color: var(--success-foreground);
}
+ .noCompleted {
+ margin-top: var(--spacing-l);
+ }
table.resultsTable {
width: 100%;
max-width: 1280px;
@@ -295,12 +300,21 @@
render() {
return html`
<div><h2 class="heading-2">Results</h2></div>
- ${this.renderSection(Category.ERROR)}
+ ${this.renderNoCompleted()} ${this.renderSection(Category.ERROR)}
${this.renderSection(Category.WARNING)}
${this.renderSection(Category.INFO)} ${this.renderSuccess()}
`;
}
+ renderNoCompleted() {
+ if (this.runs.some(hasCompleted)) return;
+ let text = 'No results';
+ if (this.runs.some(isRunning)) {
+ text = 'Checks are running ...';
+ }
+ return html`<div class="noCompleted">${text}</div>`;
+ }
+
renderSection(category: Category) {
const catString = category.toString().toLowerCase();
const runs = this.runs.filter(r =>
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 ff60531..47b4a1f 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
@@ -79,6 +79,7 @@
fireEvent,
} from '../../../utils/event-util';
import {getPluginLoader} from '../../shared/gr-js-api-interface/gr-plugin-loader';
+import {assertIsDefined} from '../../../utils/common-util';
const MSG_EMPTY_BLAME = 'No blame information for this diff.';
@@ -324,8 +325,8 @@
return getPluginLoader()
.awaitPluginsLoaded()
.then(() => {
- if (!this.path) throw new Error('Missing required "path" property.');
- if (!this.changeNum) throw new Error('Missing required "changeNum".');
+ assertIsDefined(this.path, 'path');
+ assertIsDefined(this.changeNum, 'changeNum');
this._layers = this._getLayers(this.path, this.changeNum);
this._coverageRanges = [];
// We kick off fetching the data here, but we don't return the promise,
@@ -341,8 +342,8 @@
*/
async reload(shouldReportMetric?: boolean) {
this.clear();
- if (!this.path) throw new Error('Missing required "path" property.');
- if (!this.changeNum) throw new Error('Missing required "changeNum" prop.');
+ assertIsDefined(this.path, 'path');
+ assertIsDefined(this.changeNum, 'changeNum');
this.diff = undefined;
this._errorMessage = null;
const whitespaceLevel = this._getIgnoreWhitespace();
@@ -420,10 +421,10 @@
}
_getCoverageData() {
- if (!this.changeNum) throw new Error('Missing required "changeNum" prop.');
- if (!this.change) throw new Error('Missing required "change" prop.');
- if (!this.path) throw new Error('Missing required "path" prop.');
- if (!this.patchRange) throw new Error('Missing required "patchRange".');
+ assertIsDefined(this.changeNum, 'changeNum');
+ assertIsDefined(this.change, 'change');
+ assertIsDefined(this.path, 'path');
+ assertIsDefined(this.patchRange, 'patchRange');
const changeNum = this.changeNum;
const change = this.change;
const path = this.path;
@@ -442,7 +443,7 @@
if (!provider) return;
provider(changeNum, path, basePatchNum, patchNum, change)
.then(coverageRanges => {
- if (!this.patchRange) throw new Error('Missing "patchRange".');
+ assertIsDefined(this.patchRange, 'patchRange');
if (
!coverageRanges ||
changeNum !== this.changeNum ||
@@ -532,9 +533,9 @@
* Load and display blame information for the base of the diff.
*/
loadBlame(): Promise<BlameInfo[]> {
- if (!this.changeNum) throw new Error('Missing required "changeNum" prop.');
- if (!this.patchRange) throw new Error('Missing required "patchRange".');
- if (!this.path) throw new Error('Missing required "path" property.');
+ assertIsDefined(this.changeNum, 'changeNum');
+ assertIsDefined(this.patchRange, 'patchRange');
+ assertIsDefined(this.path, 'path');
return this.restApiService
.getBlame(this.changeNum, this.patchRange.patchNum, this.path, true)
.then(blame => {
@@ -599,9 +600,9 @@
// Wrap the diff request in a new promise so that the error handler
// rejects the promise, allowing the error to be handled in the .catch.
return new Promise((resolve, reject) => {
- if (!this.changeNum) throw new Error('Missing required "changeNum".');
- if (!this.patchRange) throw new Error('Missing required "patchRange".');
- if (!this.path) throw new Error('Missing required "path" property.');
+ assertIsDefined(this.changeNum, 'changeNum');
+ assertIsDefined(this.patchRange, 'patchRange');
+ assertIsDefined(this.path, 'path');
this.restApiService
.getDiff(
this.changeNum,
@@ -669,7 +670,7 @@
// Report the due_to_rebase percentage in the "diff" category when
// applicable.
- if (!this.patchRange) throw new Error('Missing required "patchRange".');
+ assertIsDefined(this.patchRange, 'patchRange');
if (this.patchRange.basePatchNum === 'PARENT') {
this.reporting.reportInteraction(EVENT_AGAINST_PARENT);
} else if (percentRebaseDelta === 0) {
@@ -726,8 +727,8 @@
}
_getImages(diff: DiffInfo) {
- if (!this.changeNum) throw new Error('Missing required "changeNum" prop.');
- if (!this.patchRange) throw new Error('Missing required "patchRange".');
+ assertIsDefined(this.changeNum, 'changeNum');
+ assertIsDefined(this.patchRange, 'patchRange');
return this.restApiService.getImagesForDiff(
this.changeNum,
diff,
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
index b31333c..088d9cf 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts
@@ -97,6 +97,7 @@
import {CustomKeyboardEvent, OpenFixPreviewEvent} from '../../../types/events';
import {fireAlert, fireTitleChange} from '../../../utils/event-util';
import {GerritView} from '../../../services/router/router-model';
+import {assertIsDefined} from '../../../utils/common-util';
const ERR_REVIEW_STATUS = 'Couldn’t change file review status.';
const MSG_LOADING_BLAME = 'Loading blame...';
const MSG_LOADED_BLAME = 'Blame loaded';
@@ -370,7 +371,7 @@
}
_getChangeEdit() {
- if (!this._changeNum) throw new Error('Missing this._changeNum');
+ assertIsDefined(this._changeNum, '_changeNum');
return this.restApiService.getChangeEdit(this._changeNum);
}
@@ -980,7 +981,7 @@
leftSide = !!this.params.leftSide;
}
}
- if (!this._patchRange) throw new Error('Failed to initialize patchRange.');
+ assertIsDefined(this._patchRange, '_patchRange');
this._initLineOfInterestAndCursor(leftSide);
if (this.params?.commentId) {
@@ -1052,10 +1053,10 @@
this._initPatchRange();
this._initCommitRange();
- if (!this._path) throw new Error('path must be defined');
+ assertIsDefined(this._path, '_path');
if (!this._changeComments)
throw new Error('change comments must be defined');
- if (!this._patchRange) throw new Error('patch range must be defined');
+ assertIsDefined(this._patchRange, '_patchRange');
// TODO(dhruvsri): check if basePath should be set here
this.$.diffHost.threads = this._changeComments.getThreadsBySideForFile(
@@ -1082,9 +1083,9 @@
if (!this._diff) throw new Error('Missing this._diff');
const fileUnchanged = this._isFileUnchanged(this._diff);
if (fileUnchanged && value.commentLink) {
- if (!this._change) throw new Error('Missing this._change');
- if (!this._path) throw new Error('Missing this._path');
- if (!this._patchRange) throw new Error('Missing this._patchRange');
+ assertIsDefined(this._change, '_change');
+ assertIsDefined(this._path, '_path');
+ assertIsDefined(this._patchRange, '_patchRange');
if (this._patchRange.basePatchNum === ParentPatchSetNum) {
// file is unchanged between Base vs X
@@ -1493,7 +1494,7 @@
}
_loadComments(patchSet?: PatchSetNum) {
- if (!this._changeNum) throw new Error('Missing this._changeNum');
+ assertIsDefined(this._changeNum, '_changeNum');
return this.$.commentAPI
.loadAll(this._changeNum, patchSet)
.then(comments => {
@@ -1529,7 +1530,7 @@
}
_getDiffDrafts() {
- if (!this._changeNum) throw new Error('Missing this._changeNum');
+ assertIsDefined(this._changeNum, '_changeNum');
return this.restApiService.getDiffDrafts(this._changeNum);
}
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
index 189bac7..6974a76 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.ts
@@ -75,6 +75,7 @@
RenderPreferences,
} from '../../../api/diff';
import {isSafari} from '../../../utils/dom-util';
+import {assertIsDefined} from '../../../utils/common-util';
const NO_NEWLINE_BASE = 'No newline at end of base file.';
const NO_NEWLINE_REVISION = 'No newline at end of revision file.';
@@ -629,7 +630,7 @@
const contentEl = this.$.diffBuilder.getContentTdByLineEl(lineEl);
if (!contentEl) throw new Error('content el not found for line el');
side = side ?? this._getCommentSideByLineAndContent(lineEl, contentEl);
- if (!this.path) throw new Error('must have a path to create comments');
+ assertIsDefined(this.path, 'path');
this.dispatchEvent(
new CustomEvent<CreateCommentEventDetail>('create-comment', {
bubbles: true,
diff --git a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts
index f6f4395..1864598 100644
--- a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts
+++ b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.ts
@@ -45,6 +45,7 @@
import {fireAlert, fireTitleChange} from '../../../utils/event-util';
import {appContext} from '../../../services/app-context';
import {ErrorCallback} from '../../../api/rest';
+import {assertIsDefined} from '../../../utils/common-util';
const RESTORED_MESSAGE = 'Content restored from a previous edit.';
const SAVING_MESSAGE = 'Saving changes...';
@@ -326,7 +327,7 @@
}
_handlePublishTap() {
- if (!this._changeNum) throw new Error('missing changeNum');
+ assertIsDefined(this._changeNum, '_changeNum');
const changeNum = this._changeNum;
this._saveEdit().then(() => {
@@ -347,7 +348,7 @@
handleError
)
.then(() => {
- if (!this._change) throw new Error('missing change');
+ assertIsDefined(this._change, '_change');
GerritNav.navigateToChange(this._change);
});
});
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
index a9b910d..a5b7df7 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
@@ -60,7 +60,7 @@
import {KnownExperimentId} from '../../../services/flags/flags';
import {DiffInfo, DiffPreferencesInfo} from '../../../types/diff';
import {RenderPreferences} from '../../../api/diff';
-import {check, checkProperty} from '../../../utils/common-util';
+import {check, assertIsDefined} from '../../../utils/common-util';
const UNRESOLVED_EXPAND_COUNT = 5;
const NEWLINE_PATTERN = /\n/g;
@@ -334,8 +334,8 @@
}
_getUrlForViewDiff(comments: UIComment[]) {
- checkProperty(!!this.changeNum, 'changeNum');
- checkProperty(!!this.projectName, 'projectName');
+ assertIsDefined(this.changeNum, 'changeNum');
+ assertIsDefined(this.projectName, 'projectName');
check(comments.length > 0, 'comment not found');
return GerritNav.getUrlForComment(
this.changeNum,
@@ -633,8 +633,8 @@
}
_handleCommentDiscard(e: Event) {
- if (!this.changeNum) throw new Error('changeNum is missing');
- if (!this.patchNum) throw new Error('patchNum is missing');
+ assertIsDefined(this.changeNum, 'changeNum');
+ assertIsDefined(this.patchNum, 'patchNum');
const diffCommentEl = (dom(e) as EventApi).rootTarget as GrComment;
const comment = diffCommentEl.comment;
const idx = this._indexOf(comment, this.comments);
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
index 898aff3..bf376f6 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -61,6 +61,7 @@
import {OpenFixPreviewEventDetail} from '../../../types/events';
import {fireAlert} from '../../../utils/event-util';
import {pluralize} from '../../../utils/string-util';
+import {assertIsDefined} from '../../../utils/common-util';
const STORAGE_DEBOUNCE_INTERVAL = 400;
const TOAST_DEBOUNCE_INTERVAL = 200;
@@ -322,7 +323,7 @@
}
_handlePortedMessageClick() {
- if (!this.comment) throw new Error('comment not set');
+ assertIsDefined(this.comment, 'comment');
this.reporting.reportInteraction('navigate-to-original-comment', {
line: this.comment.line,
range: this.comment.range,
@@ -496,10 +497,8 @@
// prior to it being saved.
this.cancelDebouncer(DEBOUNCER_STORE);
- if (!this.comment?.path) throw new Error('Cannot erase Draft Comment');
- if (this.changeNum === undefined) {
- throw new Error('undefined changeNum');
- }
+ assertIsDefined(this.comment?.path, 'comment.path');
+ assertIsDefined(this.changeNum, 'changeNum');
this.storage.eraseDraftComment({
changeNum: this.changeNum,
patchNum: this._getPatchNum(),
diff --git a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account.ts b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account.ts
index 592f7903..ddae8ea 100644
--- a/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account.ts
+++ b/polygerrit-ui/app/elements/shared/gr-hovercard-account/gr-hovercard-account.ts
@@ -45,6 +45,7 @@
import {ReviewerState} from '../../../constants/constants';
import {CURRENT} from '../../../utils/patch-set-util';
import {isInvolved, isRemovableReviewer} from '../../../utils/change-util';
+import {assertIsDefined} from '../../../utils/common-util';
@customElement('gr-hovercard-account')
export class GrHovercardAccount extends GestureEventListeners(
@@ -163,7 +164,7 @@
}
_handleChangeReviewerOrCCStatus() {
- if (!this.change) throw new Error('expected change object to be present');
+ assertIsDefined(this.change, 'change');
// accountKey() throws an error if _account_id & email is not found, which
// we want to check before showing reloading toast
const _accountKey = accountKey(this.account);
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-context.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-context.ts
index 6a4da7b..9f9ba6a 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-context.ts
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-context.ts
@@ -29,14 +29,13 @@
* @param lineNumberEl The TD element of the line number to
* apply the annotation to using annotateLineNumber.
* @param line The line object.
- * @param path The file path (eg: /COMMIT_MSG').
+ * @param path The file path (eg: '/COMMIT_MSG').
* @param changeNum The Gerrit change number.
- * @param patchNum The Gerrit patch number.
*/
export class GrAnnotationActionsContext implements AnnotationContext {
- private _contentEl: HTMLElement;
+ contentEl: HTMLElement;
- private _lineNumberEl: HTMLElement;
+ lineNumberEl: HTMLElement;
line: GrDiffLine;
@@ -53,9 +52,8 @@
path: string,
changeNum: string | number
) {
- this._contentEl = contentEl;
- this._lineNumberEl = lineNumberEl;
-
+ this.contentEl = contentEl;
+ this.lineNumberEl = lineNumberEl;
this.line = line;
this.path = path;
this.changeNum = Number(changeNum);
@@ -80,12 +78,12 @@
styleObject: GrStyleObject,
side: string
) {
- if (this._contentEl?.getAttribute('data-side') === side) {
+ if (this.contentEl?.getAttribute('data-side') === side) {
GrAnnotation.annotateElement(
- this._contentEl,
+ this.contentEl,
offset,
length,
- styleObject.getClassName(this._contentEl)
+ styleObject.getClassName(this.contentEl)
);
}
}
@@ -97,8 +95,8 @@
* @param side The side of the update. ('left' or 'right')
*/
annotateLineNumber(styleObject: GrStyleObject, side: string) {
- if (this._lineNumberEl?.classList.contains(side)) {
- styleObject.apply(this._lineNumberEl);
+ if (this.lineNumberEl?.classList.contains(side)) {
+ styleObject.apply(this.lineNumberEl);
}
}
}
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api.ts
index 4abc6e1..95252cf 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api.ts
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api.ts
@@ -21,65 +21,37 @@
import {EventType, PluginApi} from '../../../api/plugin';
import {appContext} from '../../../services/app-context';
import {
- AddLayerFunc,
+ AnnotationCallback,
AnnotationPluginApi,
CoverageProvider,
- NotifyFunc,
} from '../../../api/annotation';
export class GrAnnotationActionsInterface implements AnnotationPluginApi {
- // Collect all annotation layers instantiated by getLayer. Will be used when
- // notifying their listeners in the notify function.
+ /**
+ * Collect all annotation layers instantiated by createLayer. This is only
+ * used for being able to look up the appropriate layer when notify() is
+ * being called by plugins.
+ */
private annotationLayers: AnnotationLayer[] = [];
- private coverageProvider: CoverageProvider | null = null;
+ private coverageProvider?: CoverageProvider;
- // Default impl is a no-op.
- private addLayerFunc: AddLayerFunc = () => {};
+ private annotationCallback?: AnnotationCallback;
- reporting = appContext.reportingService;
+ private readonly reporting = appContext.reportingService;
constructor(private readonly plugin: PluginApi) {
- // Return this instance when there is an annotatediff event.
plugin.on(EventType.ANNOTATE_DIFF, this);
}
- /**
- * Register a function to call to apply annotations. Plugins should use
- * GrAnnotationActionsContext.annotateRange and
- * GrAnnotationActionsContext.annotateLineNumber to apply a CSS class to the
- * line content or the line number.
- *
- * @param addLayerFunc The function
- * that will be called when the AnnotationLayer is ready to annotate.
- */
- addLayer(addLayerFunc: AddLayerFunc) {
- this.addLayerFunc = addLayerFunc;
+ setLayer(annotationCallback: AnnotationCallback) {
+ if (this.annotationCallback) {
+ console.warn('Overwriting an existing plugin annotation layer.');
+ }
+ this.annotationCallback = annotationCallback;
return this;
}
- /**
- * The specified function will be called with a notify function for the plugin
- * to call when it has all required data for annotation. Optional.
- *
- * @param notifyFunc See doc of the notify function below to see what it does.
- */
- addNotifier(notifyFunc: (n: NotifyFunc) => void) {
- notifyFunc(
- (path: string, startRange: number, endRange: number, side: Side) =>
- this.notify(path, startRange, endRange, side)
- );
- return this;
- }
-
- /**
- * The specified function will be called when a gr-diff component is built,
- * and feeds the returned coverage data into the diff. Optional.
- *
- * Be sure to call this only once and only from one plugin. Multiple coverage
- * providers are not supported. A second call will just overwrite the
- * provider of the first call.
- */
setCoverageProvider(
coverageProvider: CoverageProvider
): GrAnnotationActionsInterface {
@@ -98,23 +70,6 @@
return this.coverageProvider;
}
- /**
- * Returns a checkbox HTMLElement that can be used to toggle annotations
- * on/off. The checkbox will be initially disabled. Plugins should enable it
- * when data is ready and should add a click handler to toggle CSS on/off.
- *
- * Note1: Calling this method from multiple plugins will only work for the
- * 1st call. It will print an error message for all subsequent calls
- * and will not invoke their onAttached functions.
- * Note2: This method will be deprecated and eventually removed when
- * https://bugs.chromium.org/p/gerrit/issues/detail?id=8077 is
- * implemented.
- *
- * @param checkboxLabel Will be used as the label for the checkbox.
- * Optional. "Enable" is used if this is not specified.
- * @param onAttached The function that will be called
- * when the checkbox is attached to the page.
- */
enableToggleCheckbox(
checkboxLabel: string,
onAttached: (checkboxEl: Element | null) => void
@@ -148,16 +103,6 @@
return this;
}
- /**
- * The notify function will call the listeners of all required annotation
- * layers. Intended to be called by the plugin when all required data for
- * annotation is available.
- *
- * @param path The file path whose listeners should be notified.
- * @param start The line where the update starts.
- * @param end The line where the update ends.
- * @param side The side of the update ('left' or 'right').
- */
notify(path: string, start: number, end: number, side: Side) {
for (const annotationLayer of this.annotationLayers) {
// Notify only the annotation layer that is associated with the specified
@@ -169,24 +114,25 @@
}
/**
- * Should be called to register annotation layers by the framework. Not
- * intended to be called by plugins.
+ * Factory method called by Gerrit for creating a DiffLayer for each diff that
+ * is rendered.
*
- * Don't forget to dispose layer.
- *
- * @param path The file path (eg: /COMMIT_MSG').
- * @param changeNum The Gerrit change number.
+ * Don't forget to also call disposeLayer().
*/
- getLayer(path: string, changeNum: number) {
+ createLayer(path: string, changeNum: number) {
+ if (!this.annotationCallback) return undefined;
const annotationLayer = new AnnotationLayer(
path,
changeNum,
- this.addLayerFunc
+ this.annotationCallback
);
this.annotationLayers.push(annotationLayer);
return annotationLayer;
}
+ /**
+ * Called by Gerrit for each diff renderer that had called createLayer().
+ */
disposeLayer(path: string) {
this.annotationLayers = this.annotationLayers.filter(
annotationLayer => annotationLayer.path !== path
@@ -194,6 +140,10 @@
}
}
+/**
+ * An AnnotationLayer exists for each file that is being rendered. This class is
+ * not exposed to plugins, but being used by Gerrit's diff rendering.
+ */
export class AnnotationLayer implements DiffLayer {
private listeners: DiffLayerListener[] = [];
@@ -202,13 +152,13 @@
*
* @param path The file path (eg: /COMMIT_MSG').
* @param changeNum The Gerrit change number.
- * @param addLayerFunc The function
+ * @param annotationCallback The function
* that will be called when the AnnotationLayer is ready to annotate.
*/
constructor(
readonly path: string,
private readonly changeNum: number,
- private readonly addLayerFunc: AddLayerFunc
+ private readonly annotationCallback: AnnotationCallback
) {
this.listeners = [];
}
@@ -230,7 +180,8 @@
}
/**
- * Layer method to add annotations to a line.
+ * Called by Gerrit during diff rendering for each line. Delegates to the
+ * plugin provided callback for potentially annotating this line.
*
* @param contentEl The DIV.contentText element of the line
* content to apply the annotation to using annotateRange.
@@ -243,18 +194,20 @@
lineNumberEl: HTMLElement,
line: GrDiffLine
) {
- const annotationActionsContext = new GrAnnotationActionsContext(
+ const context = new GrAnnotationActionsContext(
contentEl,
lineNumberEl,
line,
this.path,
this.changeNum
);
- this.addLayerFunc(annotationActionsContext);
+ this.annotationCallback(context);
}
/**
- * Notify Layer listeners of changes to annotations.
+ * Notify layer listeners (which typically is just Gerrit's diff renderer) of
+ * changes to annotations after the diff rendering had already completed. This
+ * is indirectly called by plugins using the AnnotationPluginApi.notify().
*
* @param start The line where the update starts.
* @param end The line where the update ends.
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api_test.js b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api_test.js
index 7ae34cf..9811f99 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api_test.js
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api_test.js
@@ -59,9 +59,9 @@
assert.equal(context.line, line);
assert.equal(context.changeNum, changeNum);
};
- annotationActions.addLayer(testLayerFunc);
+ annotationActions.setLayer(testLayerFunc);
- const annotationLayer = annotationActions.getLayer(
+ const annotationLayer = annotationActions.createLayer(
'/dummy/path', changeNum);
const lineNumberEl = document.createElement('td');
@@ -72,27 +72,19 @@
test('add notifier', () => {
const path1 = '/dummy/path1';
const path2 = '/dummy/path2';
- const annotationLayer1 = annotationActions.getLayer(path1, 1);
- const annotationLayer2 = annotationActions.getLayer(path2, 1);
+ annotationActions.setLayer(context => {});
+ const annotationLayer1 = annotationActions.createLayer(path1, 1);
+ const annotationLayer2 = annotationActions.createLayer(path2, 1);
const layer1Spy = sinon.spy(annotationLayer1, 'notifyListeners');
const layer2Spy = sinon.spy(annotationLayer2, 'notifyListeners');
- let notify;
- let notifyFuncCalled;
- const notifyFunc = n => {
- notifyFuncCalled = true;
- notify = n;
- };
- annotationActions.addNotifier(notifyFunc);
- assert.isTrue(notifyFuncCalled);
-
// Assert that no layers are invoked with a different path.
- notify('/dummy/path3', 0, 10, 'right');
+ annotationActions.notify('/dummy/path3', 0, 10, 'right');
assert.isFalse(layer1Spy.called);
assert.isFalse(layer2Spy.called);
// Assert that only the 1st layer is invoked with path1.
- notify(path1, 0, 10, 'right');
+ annotationActions.notify(path1, 0, 10, 'right');
assert.isTrue(layer1Spy.called);
assert.isFalse(layer2Spy.called);
@@ -101,7 +93,7 @@
layer2Spy.resetHistory();
// Assert that only the 2nd layer is invoked with path2.
- notify(path2, 0, 20, 'left');
+ annotationActions.notify(path2, 0, 20, 'left');
assert.isFalse(layer1Spy.called);
assert.isTrue(layer2Spy.called);
});
@@ -143,7 +135,8 @@
});
test('layer notify listeners', () => {
- const annotationLayer = annotationActions.getLayer('/dummy/path', 1);
+ annotationActions.setLayer(context => {});
+ const annotationLayer = annotationActions.createLayer('/dummy/path', 1);
let listenerCalledTimes = 0;
const startRange = 10;
const endRange = 20;
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts
index 830fb92..8689ad2 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts
@@ -252,8 +252,8 @@
for (const cb of this._getEventCallbacks(EventType.ANNOTATE_DIFF)) {
const annotationApi = (cb as unknown) as GrAnnotationActionsInterface;
try {
- const layer = annotationApi.getLayer(path, changeNum);
- layers.push(layer);
+ const layer = annotationApi.createLayer(path, changeNum);
+ if (layer) layers.push(layer);
} catch (err) {
this.reporting.error(err);
}
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts
index f3be790..ec59ddc 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts
@@ -2234,13 +2234,13 @@
if (!basePatchNum && !patchNum && !path) {
return this._getDiffComments(changeNum, '/comments', {
'enable-context': true,
- 'context-padding': 5,
+ 'context-padding': 3,
});
}
return this._getDiffComments(
changeNum,
'/comments',
- {'enable-context': true, 'context-padding': 5},
+ {'enable-context': true, 'context-padding': 3},
basePatchNum,
patchNum,
path
diff --git a/polygerrit-ui/app/samples/coverage-plugin.js b/polygerrit-ui/app/samples/coverage-plugin.js
index 9b2b687..8d321c7 100644
--- a/polygerrit-ui/app/samples/coverage-plugin.js
+++ b/polygerrit-ui/app/samples/coverage-plugin.js
@@ -41,7 +41,7 @@
const coverageStyle = styleApi.css('background-color: #EF9B9B !important');
const emptyStyle = styleApi.css('');
- annotationApi.addLayer(context => {
+ annotationApi.setLayer(context => {
if (Object.keys(coverageData).length === 0) {
// Coverage data is not ready yet.
return;
@@ -64,19 +64,13 @@
}
}
}).enableToggleCheckbox('Display Coverage', checkbox => {
- // Checkbox is attached so now add the notifier that will be controlled
- // by the checkbox.
- // Checkbox will only be added to the file diff page, in the top right
- // section near the "Diff view".
- annotationApi.addNotifier(notifyFunc => {
- populateWithDummyData(coverageData);
- checkbox.disabled = false;
- checkbox.onclick = e => {
- displayCoverage = e.target.checked;
- Object.keys(coverageData).forEach(file => {
- notifyFunc(file, 0, coverageData[file].totalLines, 'right');
- });
- };
- });
+ populateWithDummyData(coverageData);
+ checkbox.disabled = false;
+ checkbox.onclick = e => {
+ displayCoverage = e.target.checked;
+ Object.keys(coverageData).forEach(file => {
+ annotationApi.notify(file, 0, coverageData[file].totalLines, 'right');
+ });
+ };
});
-});
\ No newline at end of file
+});
diff --git a/polygerrit-ui/app/test/common-test-setup.ts b/polygerrit-ui/app/test/common-test-setup.ts
index 0f394be..4e1662c 100644
--- a/polygerrit-ui/app/test/common-test-setup.ts
+++ b/polygerrit-ui/app/test/common-test-setup.ts
@@ -90,8 +90,10 @@
}
window.fixture = fixtureImpl;
+let testSetupTimestampMs = 0;
setup(() => {
+ testSetupTimestampMs = new Date().getTime();
window.Gerrit = {};
initGlobalVariables();
addIronOverlayBackdropStyleEl();
@@ -201,4 +203,9 @@
// `this.debounce()`. For those please be careful and cancel them using
// `this.cancelDebouncer()` in the `detached()` lifecycle hook.
flushDebouncers();
+ const testTeardownTimestampMs = new Date().getTime();
+ const elapsedMs = testTeardownTimestampMs - testSetupTimestampMs;
+ if (elapsedMs > 1000) {
+ console.warn(`ATTENTION! Test took longer than 1 second: ${elapsedMs} ms`);
+ }
});
diff --git a/polygerrit-ui/app/utils/comment-util.ts b/polygerrit-ui/app/utils/comment-util.ts
index 61456a2..de12a2a 100644
--- a/polygerrit-ui/app/utils/comment-util.ts
+++ b/polygerrit-ui/app/utils/comment-util.ts
@@ -289,6 +289,8 @@
}
export function computeDiffFromContext(context: ContextLine[], path: string) {
+ // do not render more than 20 lines of context
+ context = context.slice(0, 20);
const diff: DiffInfo = {
meta_a: {
name: '',
diff --git a/polygerrit-ui/app/utils/common-util.ts b/polygerrit-ui/app/utils/common-util.ts
index ad76b79..f95105d 100644
--- a/polygerrit-ui/app/utils/common-util.ts
+++ b/polygerrit-ui/app/utils/common-util.ts
@@ -67,6 +67,18 @@
}
/**
+ * Throws an error if the property is not defined.
+ */
+export function assertIsDefined<T>(
+ val: T,
+ variableName = 'variable'
+): asserts val is NonNullable<T> {
+ if (val === undefined || val === null) {
+ throw new Error(`${variableName} is not defined`);
+ }
+}
+
+/**
* Returns true, if both sets contain the same members.
*/
export function areSetsEqual<T>(a: Set<T>, b: Set<T>): boolean {
