Merge "Highlight the entire line in comment context for line comments"
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 3b9320c..3a5fb96 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -564,6 +564,12 @@
}
----
+Historical state of the change can be retrieved by specifying the
+`meta=SHA1` parameter. This will use a historical NoteDb snapshot to
+populate ChangeInfo. If the SHA1 is not reachable as a NoteDb state,
+status code 412 is returned.
+
+
[[get-change-detail]]
=== Get Change Detail
--
diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java
index a461b59..c7aedb6 100644
--- a/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/java/com/google/gerrit/server/change/ChangeJson.java
@@ -47,6 +47,7 @@
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Address;
import com.google.gerrit.entities.Change;
@@ -121,6 +122,7 @@
import java.util.Set;
import java.util.stream.Collectors;
import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.ObjectId;
/**
* Produces {@link ChangeInfo} (which is serialized to JSON afterwards) from {@link ChangeData}.
@@ -283,6 +285,11 @@
return format(changeDataFactory.create(change));
}
+ public ChangeInfo format(Change change, @Nullable ObjectId metaRevId) {
+ ChangeNotes notes = notesFactory.createChecked(change.getProject(), change.getId(), metaRevId);
+ return format(changeDataFactory.create(notes));
+ }
+
public ChangeInfo format(ChangeData cd) {
return format(cd, Optional.empty(), true, getPluginInfos(cd));
}
@@ -325,9 +332,13 @@
}
public ChangeInfo format(Project.NameKey project, Change.Id id) {
+ return format(project, id, null);
+ }
+
+ public ChangeInfo format(Project.NameKey project, Change.Id id, @Nullable ObjectId metaRevId) {
ChangeNotes notes;
try {
- notes = notesFactory.createChecked(project, id);
+ notes = notesFactory.createChecked(project, id, metaRevId);
} catch (StorageException e) {
if (!has(CHECK)) {
throw e;
diff --git a/java/com/google/gerrit/server/comment/CommentContextLoader.java b/java/com/google/gerrit/server/comment/CommentContextLoader.java
index 63bb8d0..c93f4b1 100644
--- a/java/com/google/gerrit/server/comment/CommentContextLoader.java
+++ b/java/com/google/gerrit/server/comment/CommentContextLoader.java
@@ -26,7 +26,6 @@
import com.google.gerrit.entities.Comment;
import com.google.gerrit.entities.CommentContext;
import com.google.gerrit.entities.Project;
-import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.extensions.common.ContextLineInfo;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.patch.ComparisonType;
@@ -158,12 +157,10 @@
private static CommentContext createContext(Text src, Range commentRange, int contextPadding) {
if (commentRange.start() < 1 || commentRange.end() - 1 > src.size()) {
- throw new StorageException(
- "Invalid comment range "
- + commentRange
- + ". Text only contains "
- + src.size()
- + " lines.");
+ // TODO(ghareeb): We should throw an exception in this case. See
+ // https://bugs.chromium.org/p/gerrit/issues/detail?id=14102 which is an example where the
+ // diff contains an extra line not in the original file.
+ return CommentContext.empty();
}
commentRange = adjustRange(commentRange, contextPadding, src.size());
ImmutableMap.Builder<Integer, String> context =
diff --git a/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java b/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
index 9b5b4d4..a7c7757 100644
--- a/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
@@ -37,6 +37,8 @@
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
/** View of contents at a single ref related to some change. * */
public abstract class AbstractChangeNotes<T> {
@@ -118,9 +120,14 @@
private ObjectId revision;
private boolean loaded;
- protected AbstractChangeNotes(Args args, Change.Id changeId) {
+ protected AbstractChangeNotes(Args args, Change.Id changeId, @Nullable ObjectId metaSha1) {
this.args = requireNonNull(args);
this.changeId = requireNonNull(changeId);
+ this.revision = metaSha1;
+ }
+
+ protected AbstractChangeNotes(Args args, Change.Id changeId) {
+ this(args, changeId, null);
}
public Change.Id getChangeId() {
@@ -144,7 +151,7 @@
Repository repo = args.repoManager.openRepository(getProjectName());
// Call openHandle even if reading is disabled, to trigger
// auto-rebuilding before this object may get passed to a ChangeUpdate.
- LoadHandle handle = openHandle(repo)) {
+ LoadHandle handle = openHandle(repo, revision)) {
revision = handle.id();
onLoad(handle);
loaded = true;
@@ -168,13 +175,17 @@
* @param repo open repository.
* @return handle for reading the entity.
* @throws NoSuchChangeException change does not exist.
+ * @throws MissingMetaObjectException specified SHA1 isn't reachable from meta branch.
* @throws IOException a repo-level error occurred.
*/
- protected LoadHandle openHandle(Repository repo) throws NoSuchChangeException, IOException {
- return openHandle(repo, readRef(repo));
- }
+ protected LoadHandle openHandle(Repository repo, @Nullable ObjectId id)
+ throws NoSuchChangeException, IOException, MissingMetaObjectException {
+ if (id == null) {
+ id = readRef(repo);
+ } else {
+ verifyMetaId(repo, id);
+ }
- protected LoadHandle openHandle(Repository repo, ObjectId id) {
return new LoadHandle(repo, id);
}
@@ -215,4 +226,20 @@
protected final T self() {
return (T) this;
}
+
+ private void verifyMetaId(Repository repo, ObjectId id)
+ throws IOException, MissingMetaObjectException {
+ try (RevWalk rw = new RevWalk(repo)) {
+ Ref ref = repo.getRefDatabase().exactRef(getRefName());
+ RevCommit tip = rw.parseCommit(ref.getObjectId());
+ rw.markStart(tip);
+ for (RevCommit rev : rw) {
+ if (id.equals(rev)) {
+ return;
+ }
+ }
+ }
+
+ throw new MissingMetaObjectException(id.getName() + " not reachable from " + getRefName());
+ }
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java
index cf09ff3..77d7bc0 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -114,9 +114,14 @@
return createChecked(c.getProject(), c.getId());
}
- public ChangeNotes createChecked(Project.NameKey project, Change.Id changeId) {
+ public ChangeNotes createChecked(
+ Project.NameKey project, Change.Id changeId, @Nullable ObjectId metaRevId) {
Change change = newChange(project, changeId);
- return new ChangeNotes(args, change, true, null).load();
+ return new ChangeNotes(args, change, true, null, metaRevId).load();
+ }
+
+ public ChangeNotes createChecked(Project.NameKey project, Change.Id changeId) {
+ return createChecked(project, changeId, null);
}
public static Change newChange(Project.NameKey project, Change.Id changeId) {
@@ -333,14 +338,23 @@
private ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals;
private ImmutableSet<Comment.Key> commentKeys;
- @VisibleForTesting
- public ChangeNotes(Args args, Change change, boolean shouldExist, @Nullable RefCache refs) {
- super(args, change.getId());
+ public ChangeNotes(
+ Args args,
+ Change change,
+ boolean shouldExist,
+ @Nullable RefCache refs,
+ @Nullable ObjectId metaSha1) {
+ super(args, change.getId(), metaSha1);
this.change = new Change(change);
this.shouldExist = shouldExist;
this.refs = refs;
}
+ @VisibleForTesting
+ public ChangeNotes(Args args, Change change, boolean shouldExist, @Nullable RefCache refs) {
+ this(args, change, shouldExist, refs, null);
+ }
+
public Change getChange() {
return change;
}
diff --git a/java/com/google/gerrit/server/notedb/MissingMetaObjectException.java b/java/com/google/gerrit/server/notedb/MissingMetaObjectException.java
new file mode 100644
index 0000000..ffe9fa8
--- /dev/null
+++ b/java/com/google/gerrit/server/notedb/MissingMetaObjectException.java
@@ -0,0 +1,22 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.notedb;
+
+/** Separate exception type to throw if requested meta SHA1 is not available. */
+public class MissingMetaObjectException extends RuntimeException {
+ MissingMetaObjectException(String msg) {
+ super(msg);
+ }
+}
diff --git a/java/com/google/gerrit/server/restapi/change/DeleteAssignee.java b/java/com/google/gerrit/server/restapi/change/DeleteAssignee.java
index 20fd675..842ed2a 100644
--- a/java/com/google/gerrit/server/restapi/change/DeleteAssignee.java
+++ b/java/com/google/gerrit/server/restapi/change/DeleteAssignee.java
@@ -93,10 +93,7 @@
}
IdentifiedUser deletedAssigneeUser = userFactory.create(currentAssigneeId);
deletedAssignee = deletedAssigneeUser.state();
- // noteDb
update.removeAssignee();
- // reviewDb
- change.setAssignee(null);
addMessage(ctx, update, deletedAssigneeUser);
return true;
}
diff --git a/java/com/google/gerrit/server/restapi/change/GetChange.java b/java/com/google/gerrit/server/restapi/change/GetChange.java
index f28d2b3..2f1c61e 100644
--- a/java/com/google/gerrit/server/restapi/change/GetChange.java
+++ b/java/com/google/gerrit/server/restapi/change/GetChange.java
@@ -16,12 +16,15 @@
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.Streams;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Change;
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.client.ListOption;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.PluginDefinedInfo;
import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.PreconditionFailedException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.server.DynamicOptions;
@@ -31,12 +34,15 @@
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.PluginDefinedAttributesFactories;
import com.google.gerrit.server.change.RevisionResource;
+import com.google.gerrit.server.notedb.MissingMetaObjectException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
import java.util.Collection;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.Map;
+import org.eclipse.jgit.errors.InvalidObjectIdException;
+import org.eclipse.jgit.lib.ObjectId;
import org.kohsuke.args4j.Option;
public class GetChange
@@ -53,6 +59,9 @@
options.add(o);
}
+ @Option(name = "--meta", usage = "NoteDb meta SHA1")
+ String metaRevId = "";
+
@Option(name = "-O", usage = "Output option flags, in hex")
void setOptionFlagsHex(String hex) {
options.addAll(ListOption.fromBits(ListChangesOption.class, Integer.parseInt(hex, 16)));
@@ -75,14 +84,35 @@
}
@Override
- public Response<ChangeInfo> apply(ChangeResource rsrc) {
- return Response.withMustRevalidate(newChangeJson().format(rsrc));
+ public Response<ChangeInfo> apply(ChangeResource rsrc)
+ throws BadRequestException, PreconditionFailedException {
+ try {
+ return Response.withMustRevalidate(newChangeJson().format(rsrc.getChange(), getMetaRevId()));
+ } catch (MissingMetaObjectException e) {
+ throw new PreconditionFailedException(e.getMessage());
+ }
}
Response<ChangeInfo> apply(RevisionResource rsrc) {
return Response.withMustRevalidate(newChangeJson().format(rsrc));
}
+ @Nullable
+ private ObjectId getMetaRevId() throws BadRequestException {
+ if (metaRevId.isEmpty()) {
+ return null;
+ }
+
+ // It might be interesting to also allow {SHA1}^^, so callers can walk back into history
+ // without having to fetch the entire /meta ref. If we do so, we have to be careful that
+ // the error messages can't be abused to fetch hidden data.
+ try {
+ return ObjectId.fromString(metaRevId);
+ } catch (InvalidObjectIdException e) {
+ throw new BadRequestException("invalid meta SHA1: " + metaRevId, e);
+ }
+ }
+
private ChangeJson newChangeJson() {
return json.create(options, this::createPluginDefinedInfos);
}
diff --git a/java/com/google/gerrit/server/restapi/change/GetDetail.java b/java/com/google/gerrit/server/restapi/change/GetDetail.java
index e31d84b..15362d5 100644
--- a/java/com/google/gerrit/server/restapi/change/GetDetail.java
+++ b/java/com/google/gerrit/server/restapi/change/GetDetail.java
@@ -16,6 +16,8 @@
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.PreconditionFailedException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.server.DynamicOptions;
@@ -58,7 +60,8 @@
}
@Override
- public Response<ChangeInfo> apply(ChangeResource rsrc) {
+ public Response<ChangeInfo> apply(ChangeResource rsrc)
+ throws BadRequestException, PreconditionFailedException {
return delegate.apply(rsrc);
}
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ChangeMetaIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ChangeMetaIT.java
index 3787a35..e025c52 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/ChangeMetaIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/ChangeMetaIT.java
@@ -20,8 +20,19 @@
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.entities.Change;
import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.PluginDefinedInfo;
+import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.server.change.ChangePluginDefinedInfoFactory;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gson.reflect.TypeToken;
+import com.google.gson.stream.JsonReader;
+import com.google.inject.AbstractModule;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
import org.eclipse.jgit.lib.Repository;
import org.junit.Test;
@@ -55,4 +66,85 @@
repo.exactRef(changeMetaRef(Change.id(before._number))).getObjectId().getName());
}
}
+
+ @Test
+ public void ChangeInfo_metaSha1_parameter() throws Exception {
+ PushOneCommit.Result result = createChange();
+ String changeId = result.getChangeId();
+ gApi.changes().id(changeId).setMessage("before\n\n" + "Change-Id: " + result.getChangeId());
+ ChangeInfo before = gApi.changes().id(changeId).get();
+ gApi.changes().id(changeId).setMessage("after\n\n" + "Change-Id: " + result.getChangeId());
+ ChangeInfo after = gApi.changes().id(changeId).get();
+ assertThat(after.metaRevId).isNotEqualTo(before.metaRevId);
+
+ RestResponse resp = adminRestSession.get("/changes/" + changeId + "/?meta=" + before.metaRevId);
+ resp.assertOK();
+
+ ChangeInfo got;
+ try (JsonReader jsonReader = new JsonReader(resp.getReader())) {
+ jsonReader.setLenient(true);
+ got = newGson().fromJson(jsonReader, ChangeInfo.class);
+ }
+ assertThat(got.subject).isEqualTo(before.subject);
+ }
+
+ @Test
+ public void metaUnreachableSha1() throws Exception {
+ PushOneCommit.Result ch1 = createChange();
+ PushOneCommit.Result ch2 = createChange();
+
+ ChangeInfo info2 = gApi.changes().id(ch2.getChangeId()).get();
+
+ RestResponse resp =
+ adminRestSession.get("/changes/" + ch1.getChangeId() + "/?meta=" + info2.metaRevId);
+
+ resp.assertStatus(412);
+ }
+
+ protected static class PluginDefinedSimpleAttributeModule extends AbstractModule {
+ static class MyMetaHash extends PluginDefinedInfo {
+ String myMetaRef;
+ };
+
+ static PluginDefinedInfo newMyMetaHash(ChangeData cd) {
+ MyMetaHash mmh = new MyMetaHash();
+ mmh.myMetaRef = cd.notes().getMetaId().name();
+ return mmh;
+ }
+
+ @Override
+ public void configure() {
+ DynamicSet.bind(binder(), ChangePluginDefinedInfoFactory.class)
+ .toInstance(
+ (cds, bp, p) -> {
+ Map<Change.Id, PluginDefinedInfo> out = new HashMap<>();
+ cds.forEach(cd -> out.put(cd.getId(), newMyMetaHash(cd)));
+ return out;
+ });
+ }
+ }
+
+ @Test
+ public void pluginDefinedAttribute() throws Exception {
+ try (AutoCloseable ignored =
+ installPlugin("my-plugin", PluginDefinedSimpleAttributeModule.class)) {
+ PushOneCommit.Result result = createChange();
+ String changeId = result.getChangeId();
+ gApi.changes().id(changeId).setMessage("before\n\n" + "Change-Id: " + result.getChangeId());
+ ChangeInfo before = gApi.changes().id(changeId).get();
+ gApi.changes().id(changeId).setMessage("after\n\n" + "Change-Id: " + result.getChangeId());
+ ChangeInfo after = gApi.changes().id(changeId).get();
+
+ RestResponse resp =
+ adminRestSession.get("/changes/" + changeId + "/?meta=" + before.metaRevId);
+ resp.assertOK();
+
+ Map<String, Object> changeInfo =
+ newGson().fromJson(resp.getReader(), new TypeToken<Map<String, Object>>() {}.getType());
+ List<Object> plugins = (List<Object>) changeInfo.get("plugins");
+ Map<String, Object> myplugin = (Map<String, Object>) plugins.get(0);
+
+ assertThat(myplugin.get("my_meta_ref")).isEqualTo(before.metaRevId);
+ }
+ }
}
diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentContextIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentContextIT.java
index adfe56d..548e3fe 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/CommentContextIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/CommentContextIT.java
@@ -19,11 +19,9 @@
import static com.google.gerrit.entities.Patch.COMMIT_MSG;
import static com.google.gerrit.entities.Patch.MERGE_LIST;
import static com.google.gerrit.entities.Patch.PATCHSET_LEVEL;
-import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.MoreCollectors;
-import com.google.common.util.concurrent.UncheckedExecutionException;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
@@ -148,12 +146,10 @@
CommentsUtil.newComment(COMMIT_MSG, Side.REVISION, 100, "comment", false);
CommentsUtil.addComments(gApi, changeId, ps1, comment);
- Throwable thrown =
- assertThrows(
- UncheckedExecutionException.class,
- () -> gApi.changes().id(changeId).commentsRequest().withContext(true).getAsList());
-
- assertThat(thrown).hasCauseThat().hasMessageThat().contains("Invalid comment range");
+ List<CommentInfo> comments =
+ gApi.changes().id(changeId).commentsRequest().withContext(true).getAsList();
+ assertThat(comments).hasSize(1);
+ assertThat(comments.get(0).contextLines).isEmpty();
}
@Test
diff --git a/polygerrit-ui/app/api/diff.ts b/polygerrit-ui/app/api/diff.ts
index 7ee68b7..bd110c8 100644
--- a/polygerrit-ui/app/api/diff.ts
+++ b/polygerrit-ui/app/api/diff.ts
@@ -162,20 +162,15 @@
export declare interface DiffPreferencesInfo {
context: number;
ignore_whitespace: IgnoreWhitespaceType;
- intraline_difference?: boolean;
line_length: number;
show_line_endings?: boolean;
show_tabs?: boolean;
show_whitespace_errors?: boolean;
- skip_uncommented?: boolean;
syntax_highlighting?: boolean;
- auto_hide_diff_table_header?: boolean;
tab_size: number;
font_size: number;
// TODO: Missing documentation
show_file_comment_button?: boolean;
- // TODO: Missing documentation
- theme?: string;
}
export declare interface RenderPreferences {
diff --git a/polygerrit-ui/app/constants/constants.ts b/polygerrit-ui/app/constants/constants.ts
index 03d5000..be502f7 100644
--- a/polygerrit-ui/app/constants/constants.ts
+++ b/polygerrit-ui/app/constants/constants.ts
@@ -403,12 +403,10 @@
// (Render mode being at least one of them).
export function createDefaultDiffPrefs(): DiffPreferencesInfo {
return {
- auto_hide_diff_table_header: true,
context: 10,
cursor_blink_rate: 0,
font_size: 12,
ignore_whitespace: 'IGNORE_NONE',
- intraline_difference: true,
line_length: 100,
line_wrapping: false,
show_line_endings: true,
@@ -416,7 +414,6 @@
show_whitespace_errors: true,
syntax_highlighting: true,
tab_size: 8,
- theme: 'DEFAULT',
};
}
diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_html.ts b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_html.ts
index cd17271..c5c73c5 100644
--- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_html.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_html.ts
@@ -115,6 +115,11 @@
commit message box. Their top border should be on the same line. */
margin-bottom: var(--spacing-s);
}
+ .show-all-button iron-icon {
+ color: inherit;
+ --iron-icon-height: 18px;
+ --iron-icon-width: 18px;
+ }
</style>
<gr-external-style id="externalStyle" name="change-metadata">
<template is="dom-if" if="[[_isNewChangeSummaryUiEnabled]]">
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 021d7c4..72ed6e6 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
@@ -55,6 +55,7 @@
import {notUndefined} from '../../../types/types';
import {uniqueDefinedAvatar} from '../../../utils/account-util';
import {PrimaryTab} from '../../../constants/constants';
+import {CommentTabState} from '../../../types/events';
export enum SummaryChipStyles {
INFO = 'info',
@@ -71,6 +72,9 @@
@property()
styleType = SummaryChipStyles.UNDEFINED;
+ @property()
+ category?: CommentTabState;
+
static get styles() {
return [
sharedStyles,
@@ -132,7 +136,9 @@
private handleClick(e: MouseEvent) {
e.stopPropagation();
e.preventDefault();
- fireShowPrimaryTab(this, PrimaryTab.COMMENT_THREADS);
+ fireShowPrimaryTab(this, PrimaryTab.COMMENT_THREADS, true, {
+ commentTab: this.category,
+ });
}
}
@@ -413,12 +419,14 @@
No Comments</gr-summary-chip
><gr-summary-chip
styleType=${SummaryChipStyles.WARNING}
+ category=${CommentTabState.DRAFTS}
icon="edit"
?hidden=${!draftCount}
>
${pluralize(draftCount, 'draft')}</gr-summary-chip
><gr-summary-chip
styleType=${SummaryChipStyles.WARNING}
+ category=${CommentTabState.UNRESOLVED}
icon="message"
?hidden=${!countUnresolvedComments}
>
@@ -433,6 +441,7 @@
${countUnresolvedComments} unresolved</gr-summary-chip
><gr-summary-chip
styleType=${SummaryChipStyles.CHECK}
+ category=${CommentTabState.SHOW_ALL}
icon="markChatRead"
?hidden=${!countResolvedComments}
>${countResolvedComments} resolved</gr-summary-chip
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 61006f9..7b80977 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
@@ -152,11 +152,13 @@
ShowAlertEventDetail,
SwitchTabEvent,
ThreadListModifiedEvent,
+ TabState,
} from '../../../types/events';
import {GrButton} from '../../shared/gr-button/gr-button';
import {GrMessagesList} from '../gr-messages-list/gr-messages-list';
import {GrThreadList} from '../gr-thread-list/gr-thread-list';
import {
+ EventType,
fireAlert,
fireEvent,
firePageError,
@@ -555,6 +557,9 @@
@property({type: Boolean})
_isNewChangeSummaryUiEnabled = false;
+ @property({type: String})
+ _tabState?: TabState;
+
restApiService = appContext.restApiService;
checksService = appContext.checksService;
@@ -694,7 +699,7 @@
this.listen(window, 'scroll', '_handleScroll');
this.listen(document, 'visibilitychange', '_handleVisibilityChange');
- this.addEventListener('show-primary-tab', e =>
+ this.addEventListener(EventType.SHOW_PRIMARY_TAB, e =>
this._setActivePrimaryTab(e)
);
this.addEventListener('show-secondary-tab', e =>
@@ -853,6 +858,7 @@
this._selectedTabPluginHeader = '';
}
}
+ this._tabState = e.detail.tabState;
}
/**
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 7fd985a..56c1d38 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
@@ -680,6 +680,7 @@
change="[[_change]]"
change-num="[[_changeNum]]"
logged-in="[[_loggedIn]]"
+ comment-tab-state="[[_tabState.commentTab]]"
only-show-robot-comments-with-human-reply=""
unresolved-only
show-comment-context
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js
index 2d38dcc..285b73f 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js
@@ -1374,12 +1374,10 @@
line_length: 100,
cursor_blink_rate: 0,
line_wrapping: false,
- intraline_difference: true,
show_line_endings: true,
show_tabs: true,
show_whitespace_errors: true,
syntax_highlighting: true,
- auto_hide_diff_table_header: true,
theme: 'DEFAULT',
ignore_whitespace: 'IGNORE_NONE',
};
diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list-experimental/gr-related-changes-list-experimental.ts b/polygerrit-ui/app/elements/change/gr-related-changes-list-experimental/gr-related-changes-list-experimental.ts
index be48038..23627ae 100644
--- a/polygerrit-ui/app/elements/change/gr-related-changes-list-experimental/gr-related-changes-list-experimental.ts
+++ b/polygerrit-ui/app/elements/change/gr-related-changes-list-experimental/gr-related-changes-list-experimental.ts
@@ -148,7 +148,7 @@
class="${classMap({
['show-when-collapsed']: showWhenCollapsedPredicate(index),
})}"
- .currentChange="${this._changesEqual(change, this.change)}"
+ .isCurrentChange="${this._changesEqual(change, this.change)}"
.change="${change}"
.href="${GerritNav.getUrlForChangeById(
change._number,
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 28bccc6..f0ec7cd 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
@@ -43,6 +43,8 @@
import {fireThreadListModifiedEvent} from '../../../utils/event-util';
import {KnownExperimentId} from '../../../services/flags/flags';
import {appContext} from '../../../services/app-context';
+import {assertNever} from '../../../utils/common-util';
+import {CommentTabState} from '../../../types/events';
interface CommentThreadWithInfo {
thread: CommentThread;
@@ -106,6 +108,9 @@
@property({type: Boolean})
_isNewChangeSummaryUiEnabled = false;
+ @property({type: Object, observer: '_commentTabStateChange'})
+ commentTabState?: CommentTabState;
+
flagsService = appContext.flagsService;
/** @override */
@@ -501,6 +506,26 @@
filterRobotThreadsWithoutHumanReply(threads?: CommentThread[]) {
return threads?.filter(t => !isRobotThread(t) || hasHumanReply(t));
}
+
+ _commentTabStateChange(
+ newValue?: CommentTabState,
+ oldValue?: CommentTabState
+ ) {
+ if (!newValue || newValue === oldValue) return;
+ switch (newValue) {
+ case CommentTabState.UNRESOLVED:
+ this._handleOnlyUnresolved();
+ break;
+ case CommentTabState.DRAFTS:
+ this._handleOnlyDrafts();
+ break;
+ case CommentTabState.SHOW_ALL:
+ this._handleAllComments();
+ break;
+ default:
+ assertNever(newValue, 'Unsupported preferred state');
+ }
+ }
}
declare global {
diff --git a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_html.ts b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_html.ts
index 1be2da1..8b1ba25 100644
--- a/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_html.ts
+++ b/polygerrit-ui/app/elements/change/gr-thread-list/gr-thread-list_html.ts
@@ -101,7 +101,7 @@
name="filterComments"
type="radio"
on-click="_handleOnlyUnresolved"
- checked$="[[unresolvedOnly]]"
+ checked="[[unresolvedOnly]]"
/>
<label for="unresolvedRadio">
Unresolved ([[_countUnresolved(threads)]])
@@ -112,7 +112,7 @@
name="filterComments"
type="radio"
on-click="_handleOnlyDrafts"
- checked$="[[_draftsOnly]]"
+ checked="[[_draftsOnly]]"
/>
<label for="draftsRadio">
Drafts ([[_countDrafts(threads)]])
@@ -123,7 +123,7 @@
name="filterComments"
type="radio"
on-click="_handleAllComments"
- checked$="[[_showAllComments(_draftsOnly, unresolvedOnly)]]"
+ checked="[[_showAllComments(_draftsOnly, unresolvedOnly)]]"
/>
<label for="all">
All ([[_countAllThreads(threads)]])
diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.js b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.js
index ec44f92..2215d5b 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff_test.js
@@ -161,12 +161,10 @@
element.patchRange = {basePatchNum: 'PARENT', patchNum: 1};
element.isImageDiff = true;
element.prefs = {
- auto_hide_diff_table_header: true,
context: 10,
cursor_blink_rate: 0,
font_size: 12,
ignore_whitespace: 'IGNORE_NONE',
- intraline_difference: true,
line_length: 100,
line_wrapping: false,
show_line_endings: true,
@@ -497,12 +495,11 @@
line_length: 100,
cursor_blink_rate: 0,
line_wrapping: false,
- intraline_difference: true,
+
show_line_endings: true,
show_tabs: true,
show_whitespace_errors: true,
syntax_highlighting: true,
- auto_hide_diff_table_header: true,
theme: 'DEFAULT',
ignore_whitespace: 'IGNORE_NONE',
};
@@ -959,11 +956,10 @@
element = basicFixture.instantiate();
element.prefs = {
ignore_whitespace: ignore_whitespace || 'IGNORE_ALL',
- auto_hide_diff_table_header: true,
context: 10,
cursor_blink_rate: 0,
font_size: 12,
- intraline_difference: true,
+
line_length: 100,
line_wrapping: false,
show_line_endings: true,
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.js
index fbd675f..7cbcaef 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.js
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.js
@@ -396,12 +396,10 @@
sinon.stub(element, 'getLoggedIn').callsFake(() => Promise.resolve(false));
return element.getDiffPreferences().then(obj => {
- assert.equal(obj.auto_hide_diff_table_header, true);
assert.equal(obj.context, 10);
assert.equal(obj.cursor_blink_rate, 0);
assert.equal(obj.font_size, 12);
assert.equal(obj.ignore_whitespace, 'IGNORE_NONE');
- assert.equal(obj.intraline_difference, true);
assert.equal(obj.line_length, 100);
assert.equal(obj.line_wrapping, false);
assert.equal(obj.show_line_endings, true);
@@ -409,7 +407,6 @@
assert.equal(obj.show_whitespace_errors, true);
assert.equal(obj.syntax_highlighting, true);
assert.equal(obj.tab_size, 8);
- assert.equal(obj.theme, 'DEFAULT');
});
});
diff --git a/polygerrit-ui/app/types/common.ts b/polygerrit-ui/app/types/common.ts
index aeacd9a..95801e7 100644
--- a/polygerrit-ui/app/types/common.ts
+++ b/polygerrit-ui/app/types/common.ts
@@ -1680,7 +1680,6 @@
context?: number;
expand_all_comments?: boolean;
ignore_whitespace: IgnoreWhitespaceType;
- intraline_difference?: boolean;
line_length?: number;
manual_review?: boolean;
retain_header?: boolean;
@@ -1688,10 +1687,8 @@
show_tabs?: boolean;
show_whitespace_errors?: boolean;
skip_deleted?: boolean;
- skip_uncommented?: boolean;
syntax_highlighting?: boolean;
hide_top_menu?: boolean;
- auto_hide_diff_table_header?: boolean;
hide_line_numbers?: boolean;
tab_size?: number;
font_size?: number;
diff --git a/polygerrit-ui/app/types/events.ts b/polygerrit-ui/app/types/events.ts
index 163c760..6b05fad 100644
--- a/polygerrit-ui/app/types/events.ts
+++ b/polygerrit-ui/app/types/events.ts
@@ -146,6 +146,18 @@
value?: number;
// scroll into the tab afterwards, from custom event
scrollIntoView?: boolean;
+ // define state of tab after opening
+ tabState?: TabState;
+}
+
+export interface TabState {
+ commentTab?: CommentTabState;
+}
+
+export enum CommentTabState {
+ UNRESOLVED = 'unresolved',
+ DRAFTS = 'drafts',
+ SHOW_ALL = 'show all',
}
export type SwitchTabEvent = CustomEvent<SwitchTabEventDetail>;
diff --git a/polygerrit-ui/app/utils/event-util.ts b/polygerrit-ui/app/utils/event-util.ts
index ee3c833..cf590e0 100644
--- a/polygerrit-ui/app/utils/event-util.ts
+++ b/polygerrit-ui/app/utils/event-util.ts
@@ -17,7 +17,7 @@
import {UrlEncodedCommentId} from '../types/common';
import {FetchRequest} from '../types/types';
-import {DialogChangeEventDetail} from '../types/events';
+import {DialogChangeEventDetail, TabState} from '../types/events';
export enum EventType {
SHOW_ALERT = 'show-alert',
@@ -27,6 +27,7 @@
TITLE_CHANGE = 'title-change',
THREAD_LIST_MODIFIED = 'thread-list-modified',
DIALOG_CHANGE = 'dialog-change',
+ SHOW_PRIMARY_TAB = 'show-primary-tab',
}
export function fireEvent(target: EventTarget, type: string) {
@@ -117,10 +118,15 @@
);
}
-export function fireShowPrimaryTab(target: EventTarget, tab: string) {
+export function fireShowPrimaryTab(
+ target: EventTarget,
+ tab: string,
+ scrollIntoView?: boolean,
+ tabState?: TabState
+) {
target.dispatchEvent(
- new CustomEvent('show-primary-tab', {
- detail: {tab},
+ new CustomEvent(EventType.SHOW_PRIMARY_TAB, {
+ detail: {tab, scrollIntoView, tabState},
composed: true,
bubbles: true,
})
