Merge "Add reviewers to attention set when replying to WIP changes"
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 265014e..9876b53 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -2056,6 +2056,10 @@
will contain a list of link:#context-line[ContextLine] containing the lines of
the source file where the comment was written.
+The `context-padding` request parameter can be used to specify an extra number
+of context lines to be added before and after the comment range. This parameter
+only works if `enable-context` is set to true.
+
.Request
----
GET /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/comments HTTP/1.0
diff --git a/java/com/google/gerrit/entities/Change.java b/java/com/google/gerrit/entities/Change.java
index 1fa099e..ca13db9 100644
--- a/java/com/google/gerrit/entities/Change.java
+++ b/java/com/google/gerrit/entities/Change.java
@@ -456,20 +456,14 @@
*/
protected Timestamp lastUpdatedOn;
- // DELETED: id = 6 (sortkey)
-
protected Account.Id owner;
/** The branch (and project) this change merges into. */
protected BranchNameKey dest;
- // DELETED: id = 9 (open)
-
/** Current state code; see {@link Status}. */
protected char status;
- // DELETED: id = 11 (nbrPatchSets)
-
/** The current patch set. */
protected int currentPatchSetId;
@@ -479,9 +473,6 @@
/** Topic name assigned by the user, if any. */
@Nullable protected String topic;
- // DELETED: id = 15 (lastSha1MergeTested)
- // DELETED: id = 16 (mergeable)
-
/**
* First line of first patch set's commit message.
*
@@ -553,12 +544,12 @@
cherryPickOf = other.cherryPickOf;
}
- /** Legacy 32 bit integer identity for a change. */
+ /** 32 bit integer identity for a change. */
public Change.Id getId() {
return changeId;
}
- /** Legacy 32 bit integer identity for a change. */
+ /** 32 bit integer identity for a change. */
public int getChangeId() {
return changeId.get();
}
diff --git a/java/com/google/gerrit/extensions/api/changes/ChangeApi.java b/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
index 3364fc1..6c15c0c 100644
--- a/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
+++ b/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
@@ -413,6 +413,7 @@
abstract class CommentsRequest {
private boolean enableContext;
+ private int contextPadding;
/**
* Get all published comments on a change.
@@ -436,6 +437,11 @@
return this;
}
+ public CommentsRequest contextPadding(int contextPadding) {
+ this.contextPadding = contextPadding;
+ return this;
+ }
+
public CommentsRequest withContext() {
this.enableContext = true;
return this;
@@ -444,6 +450,10 @@
public boolean getContext() {
return enableContext;
}
+
+ public int getContextPadding() {
+ return contextPadding;
+ }
}
abstract class SuggestedReviewersRequest {
diff --git a/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java b/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
index 8893039..8047e0e 100644
--- a/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
+++ b/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
@@ -612,6 +612,7 @@
try {
ListChangeComments listComments = listCommentsProvider.get();
listComments.setContext(this.getContext());
+ listComments.setContextPadding(this.getContextPadding());
return listComments.apply(change).value();
} catch (Exception e) {
throw asRestApiException("Cannot get comments", e);
@@ -623,6 +624,7 @@
try {
ListChangeComments listComments = listCommentsProvider.get();
listComments.setContext(this.getContext());
+ listComments.setContextPadding(this.getContextPadding());
return listComments.getComments(change);
} catch (Exception e) {
throw asRestApiException("Cannot get comments", e);
diff --git a/java/com/google/gerrit/server/comment/CommentContextCacheImpl.java b/java/com/google/gerrit/server/comment/CommentContextCacheImpl.java
index a0ab398..3d75349 100644
--- a/java/com/google/gerrit/server/comment/CommentContextCacheImpl.java
+++ b/java/com/google/gerrit/server/comment/CommentContextCacheImpl.java
@@ -16,6 +16,7 @@
import static java.nio.charset.StandardCharsets.UTF_8;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.cache.CacheLoader;
import com.google.common.cache.LoadingCache;
import com.google.common.cache.Weigher;
@@ -23,6 +24,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.common.collect.Streams;
+import com.google.common.flogger.FluentLogger;
import com.google.common.hash.Hashing;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Comment;
@@ -36,6 +38,7 @@
import com.google.gerrit.server.cache.proto.Cache.AllCommentContextProto;
import com.google.gerrit.server.cache.proto.Cache.AllCommentContextProto.CommentContextProto;
import com.google.gerrit.server.cache.serialize.CacheSerializer;
+import com.google.gerrit.server.comment.CommentContextLoader.ContextInput;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.inject.Inject;
import com.google.inject.Module;
@@ -50,14 +53,22 @@
/** Implementation of {@link CommentContextCache}. */
public class CommentContextCacheImpl implements CommentContextCache {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
private static final String CACHE_NAME = "comment_context";
+ /**
+ * Comment context is expected to contain just few lines of code to be displayed beside the
+ * comment. Setting an upper bound of 100 for padding.
+ */
+ @VisibleForTesting public static final int MAX_CONTEXT_PADDING = 50;
+
public static Module module() {
return new CacheModule() {
@Override
protected void configure() {
persist(CACHE_NAME, CommentContextKey.class, CommentContext.class)
- .version(1)
+ .version(2)
.diskLimit(1 << 30) // limit the total cache size to 1 GB
.maximumWeight(1 << 23) // Limit the size of the in-memory cache to 8 MB
.weigher(CommentContextWeigher.class)
@@ -88,9 +99,14 @@
Iterable<CommentContextKey> inputKeys) {
ImmutableMap.Builder<CommentContextKey, CommentContext> result = ImmutableMap.builder();
+ List<CommentContextKey> adjustedKeys =
+ Streams.stream(inputKeys)
+ .map(CommentContextCacheImpl::adjustMaxContextPadding)
+ .collect(ImmutableList.toImmutableList());
+
// Convert the input keys to the same keys but with their file paths hashed
Map<CommentContextKey, CommentContextKey> keysToCacheKeys =
- Streams.stream(inputKeys)
+ adjustedKeys.stream()
.collect(
Collectors.toMap(
Function.identity(),
@@ -101,7 +117,7 @@
contextCache.getAll(keysToCacheKeys.values());
for (CommentContextKey inputKey : inputKeys) {
- CommentContextKey cacheKey = keysToCacheKeys.get(inputKey);
+ CommentContextKey cacheKey = keysToCacheKeys.get(adjustMaxContextPadding(inputKey));
result.put(inputKey, allContext.get(cacheKey));
}
return result.build();
@@ -110,6 +126,23 @@
}
}
+ private static CommentContextKey adjustMaxContextPadding(CommentContextKey key) {
+ if (key.contextPadding() < 0) {
+ logger.atWarning().log(
+ "Cannot set context padding to a negative number %d. Adjusting the number to 0",
+ key.contextPadding());
+ return key.toBuilder().contextPadding(0).build();
+ }
+ if (key.contextPadding() > MAX_CONTEXT_PADDING) {
+ logger.atWarning().log(
+ "Number of requested context lines is %d and exceeding the configured maximum of %d."
+ + " Adjusting the number to the maximum.",
+ key.contextPadding(), MAX_CONTEXT_PADDING);
+ return key.toBuilder().contextPadding(MAX_CONTEXT_PADDING).build();
+ }
+ return key;
+ }
+
public enum CommentContextSerializer implements CacheSerializer<CommentContext> {
INSTANCE;
@@ -216,11 +249,12 @@
ChangeNotes notes = notesFactory.createChecked(project, changeId);
List<HumanComment> humanComments = commentsUtil.publishedHumanCommentsByChange(notes);
CommentContextLoader loader = factory.create(project);
- Map<Comment, CommentContextKey> commentsToKeys = new HashMap<>();
+ Map<ContextInput, CommentContextKey> commentsToKeys = new HashMap<>();
for (CommentContextKey key : keys) {
- commentsToKeys.put(getCommentForKey(humanComments, key), key);
+ Comment comment = getCommentForKey(humanComments, key);
+ commentsToKeys.put(ContextInput.fromComment(comment, key.contextPadding()), key);
}
- Map<Comment, CommentContext> allContext = loader.getContext(commentsToKeys.keySet());
+ Map<ContextInput, CommentContext> allContext = loader.getContext(commentsToKeys.keySet());
return allContext.entrySet().stream()
.collect(Collectors.toMap(e -> commentsToKeys.get(e.getKey()), Map.Entry::getValue));
}
diff --git a/java/com/google/gerrit/server/comment/CommentContextKey.java b/java/com/google/gerrit/server/comment/CommentContextKey.java
index ccd50b7..af2ae92 100644
--- a/java/com/google/gerrit/server/comment/CommentContextKey.java
+++ b/java/com/google/gerrit/server/comment/CommentContextKey.java
@@ -28,6 +28,9 @@
abstract Integer patchset();
+ /** Number of extra lines of context that should be added before and after the comment range. */
+ abstract int contextPadding();
+
abstract Builder toBuilder();
public static Builder builder() {
@@ -47,6 +50,8 @@
public abstract Builder patchset(Integer patchset);
+ public abstract Builder contextPadding(Integer numLines);
+
public abstract CommentContextKey build();
}
@@ -62,6 +67,7 @@
.setPatchset(key.patchset())
.setPathHash(key.path())
.setCommentId(key.id())
+ .setContextPadding(key.contextPadding())
.build());
}
@@ -75,6 +81,7 @@
.patchset(proto.getPatchset())
.id(proto.getCommentId())
.path(proto.getPathHash())
+ .contextPadding(proto.getContextPadding())
.build();
}
}
diff --git a/java/com/google/gerrit/server/comment/CommentContextLoader.java b/java/com/google/gerrit/server/comment/CommentContextLoader.java
index f642438..63bb8d0 100644
--- a/java/com/google/gerrit/server/comment/CommentContextLoader.java
+++ b/java/com/google/gerrit/server/comment/CommentContextLoader.java
@@ -21,8 +21,8 @@
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
-import com.google.common.collect.Streams;
import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Comment;
import com.google.gerrit.entities.CommentContext;
import com.google.gerrit.entities.Project;
@@ -34,6 +34,7 @@
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import java.io.IOException;
+import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.Optional;
@@ -66,43 +67,51 @@
}
/**
- * Load the comment context for multiple comments at once. This method will open the repository
- * and read the source files for all necessary comments' file paths.
+ * Load the comment context for multiple contextInputs at once. This method will open the
+ * repository and read the source files for all necessary contextInputs' file paths.
*
- * @param comments a list of comments.
- * @return a Map where all entries consist of the input comments and the values are their
+ * @param contextInputs a list of contextInputs.
+ * @return a Map where all entries consist of the input contextInputs and the values are their
* corresponding {@link CommentContext}.
*/
- public Map<Comment, CommentContext> getContext(Iterable<Comment> comments) throws IOException {
- ImmutableMap.Builder<Comment, CommentContext> result =
- ImmutableMap.builderWithExpectedSize(Iterables.size(comments));
+ public Map<ContextInput, CommentContext> getContext(Collection<ContextInput> contextInputs)
+ throws IOException {
+ ImmutableMap.Builder<ContextInput, CommentContext> result =
+ ImmutableMap.builderWithExpectedSize(Iterables.size(contextInputs));
- // Group comments by commit ID so that each commit is parsed only once
- Map<ObjectId, List<Comment>> commentsByCommitId =
- Streams.stream(comments).collect(groupingBy(Comment::getCommitId));
+ // Group contextInputs by commit ID so that each commit is parsed only once
+ Map<ObjectId, List<ContextInput>> commentsByCommitId =
+ contextInputs.stream().collect(groupingBy(ContextInput::commitId));
try (Repository repo = repoManager.openRepository(project);
RevWalk rw = new RevWalk(repo)) {
for (ObjectId commitId : commentsByCommitId.keySet()) {
RevCommit commit = rw.parseCommit(commitId);
- for (Comment comment : commentsByCommitId.get(commitId)) {
- Optional<Range> range = getStartAndEndLines(comment);
+ for (ContextInput contextInput : commentsByCommitId.get(commitId)) {
+ Optional<Range> range = getStartAndEndLines(contextInput);
if (!range.isPresent()) {
- result.put(comment, CommentContext.empty());
+ result.put(contextInput, CommentContext.empty());
continue;
}
- String filePath = comment.key.filename;
+ String filePath = contextInput.filePath();
switch (filePath) {
case COMMIT_MSG:
result.put(
- comment, getContextForCommitMessage(rw.getObjectReader(), commit, range.get()));
+ contextInput,
+ getContextForCommitMessage(
+ rw.getObjectReader(), commit, range.get(), contextInput.contextPadding()));
break;
case MERGE_LIST:
result.put(
- comment, getContextForMergeList(rw.getObjectReader(), commit, range.get()));
+ contextInput,
+ getContextForMergeList(
+ rw.getObjectReader(), commit, range.get(), contextInput.contextPadding()));
break;
default:
- result.put(comment, getContextForFilePath(repo, rw, commit, filePath, range.get()));
+ result.put(
+ contextInput,
+ getContextForFilePath(
+ repo, rw, commit, filePath, range.get(), contextInput.contextPadding()));
}
}
}
@@ -111,20 +120,27 @@
}
private CommentContext getContextForCommitMessage(
- ObjectReader reader, RevCommit commit, Range range) throws IOException {
+ ObjectReader reader, RevCommit commit, Range commentRange, int contextPadding)
+ throws IOException {
Text text = Text.forCommit(reader, commit);
- return createContext(text, range);
+ return createContext(text, commentRange, contextPadding);
}
- private CommentContext getContextForMergeList(ObjectReader reader, RevCommit commit, Range range)
+ private CommentContext getContextForMergeList(
+ ObjectReader reader, RevCommit commit, Range commentRange, int contextPadding)
throws IOException {
ComparisonType cmp = ComparisonType.againstParent(1);
Text text = Text.forMergeList(cmp, reader, commit);
- return createContext(text, range);
+ return createContext(text, commentRange, contextPadding);
}
private CommentContext getContextForFilePath(
- Repository repo, RevWalk rw, RevCommit commit, String filePath, Range range)
+ Repository repo,
+ RevWalk rw,
+ RevCommit commit,
+ String filePath,
+ Range commentRange,
+ int contextPadding)
throws IOException {
// TODO(ghareeb): We can further group the comments by file paths to avoid opening
// the same file multiple times.
@@ -136,28 +152,43 @@
}
ObjectId id = tw.getObjectId(0);
Text src = new Text(repo.open(id, Constants.OBJ_BLOB));
- return createContext(src, range);
+ return createContext(src, commentRange, contextPadding);
}
}
- private static CommentContext createContext(Text src, Range range) {
- if (range.start() < 1 || range.end() > src.size()) {
+ 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 " + range + ". Text only contains " + src.size() + " lines.");
+ "Invalid comment range "
+ + commentRange
+ + ". Text only contains "
+ + src.size()
+ + " lines.");
}
+ commentRange = adjustRange(commentRange, contextPadding, src.size());
ImmutableMap.Builder<Integer, String> context =
- ImmutableMap.builderWithExpectedSize(range.end() - range.start());
- for (int i = range.start(); i < range.end(); i++) {
+ ImmutableMap.builderWithExpectedSize(commentRange.end() - commentRange.start());
+ for (int i = commentRange.start(); i < commentRange.end(); i++) {
context.put(i, src.getString(i - 1));
}
return CommentContext.create(context.build());
}
- private static Optional<Range> getStartAndEndLines(Comment comment) {
- if (comment.range != null) {
- return Optional.of(Range.create(comment.range.startLine, comment.range.endLine + 1));
- } else if (comment.lineNbr > 0) {
- return Optional.of(Range.create(comment.lineNbr, comment.lineNbr + 1));
+ /**
+ * Adjust the {@code commentRange} parameter by adding {@code contextPadding} lines before and
+ * after the comment range.
+ */
+ private static Range adjustRange(Range commentRange, int contextPadding, int fileLines) {
+ int newStartLine = commentRange.start() - contextPadding;
+ int newEndLine = commentRange.end() + contextPadding;
+ return Range.create(Math.max(1, newStartLine), Math.min(fileLines + 1, newEndLine));
+ }
+
+ private static Optional<Range> getStartAndEndLines(ContextInput comment) {
+ if (comment.range() != null) {
+ return Optional.of(Range.create(comment.range().startLine, comment.range().endLine + 1));
+ } else if (comment.lineNumber() > 0) {
+ return Optional.of(Range.create(comment.lineNumber(), comment.lineNumber() + 1));
}
return Optional.empty();
}
@@ -173,5 +204,62 @@
/** End line of the comment (exclusive). */
abstract int end();
+
+ /** Number of lines covered by this range. */
+ int size() {
+ return end() - start();
+ }
+ }
+
+ /** This entity only contains comment fields needed to load the comment context. */
+ @AutoValue
+ abstract static class ContextInput {
+ static ContextInput fromComment(Comment comment, int contextPadding) {
+ return new AutoValue_CommentContextLoader_ContextInput.Builder()
+ .commitId(comment.getCommitId())
+ .filePath(comment.key.filename)
+ .range(comment.range)
+ .lineNumber(comment.lineNbr)
+ .contextPadding(contextPadding)
+ .build();
+ }
+
+ /** 20 bytes SHA-1 of the patchset commit containing the file where the comment is written. */
+ abstract ObjectId commitId();
+
+ /** File path where the comment is written. */
+ abstract String filePath();
+
+ /**
+ * Position of the comment in the file (start line, start char, end line, end char). This field
+ * can be null if the range is not available for this comment.
+ */
+ @Nullable
+ abstract Comment.Range range();
+
+ /**
+ * The 1-based line number where the comment is written. A value 0 means that the line number is
+ * not available for this comment.
+ */
+ abstract Integer lineNumber();
+
+ /** Number of extra lines of context that should be added before and after the comment range. */
+ abstract Integer contextPadding();
+
+ @AutoValue.Builder
+ public abstract static class Builder {
+
+ public abstract Builder commitId(ObjectId commitId);
+
+ public abstract Builder filePath(String filePath);
+
+ public abstract Builder range(@Nullable Comment.Range range);
+
+ public abstract Builder lineNumber(Integer lineNumber);
+
+ public abstract Builder contextPadding(Integer contextPadding);
+
+ public abstract ContextInput build();
+ }
}
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesState.java b/java/com/google/gerrit/server/notedb/ChangeNotesState.java
index af8c8c8..33bc039 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesState.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesState.java
@@ -47,7 +47,6 @@
import com.google.gerrit.entities.converter.ChangeMessageProtoConverter;
import com.google.gerrit.entities.converter.PatchSetApprovalProtoConverter;
import com.google.gerrit.entities.converter.PatchSetProtoConverter;
-import com.google.gerrit.entities.converter.ProtoConverter;
import com.google.gerrit.json.OutputFormat;
import com.google.gerrit.proto.Protos;
import com.google.gerrit.server.AssigneeStatusUpdate;
@@ -65,8 +64,6 @@
import com.google.gerrit.server.cache.serialize.ObjectIdConverter;
import com.google.gerrit.server.index.change.ChangeField.StoredSubmitRecord;
import com.google.gson.Gson;
-import com.google.protobuf.ByteString;
-import com.google.protobuf.MessageLite;
import java.sql.Timestamp;
import java.time.Instant;
import java.util.List;
@@ -455,6 +452,9 @@
abstract ChangeNotesState build();
}
+ /**
+ * Convert ChangeNotesState (which is AutoValue based) to byte[] and back, using protocol buffers.
+ */
enum Serializer implements CacheSerializer<ChangeNotesState> {
INSTANCE;
@@ -482,13 +482,11 @@
object.hashtags().forEach(b::addHashtag);
object
.patchSets()
- .forEach(e -> b.addPatchSet(toByteString(e.getValue(), PatchSetProtoConverter.INSTANCE)));
+ .forEach(e -> b.addPatchSet(PatchSetProtoConverter.INSTANCE.toProto(e.getValue())));
object
.approvals()
.forEach(
- e ->
- b.addApproval(
- toByteString(e.getValue(), PatchSetApprovalProtoConverter.INSTANCE)));
+ e -> b.addApproval(PatchSetApprovalProtoConverter.INSTANCE.toProto(e.getValue())));
object.reviewers().asTable().cellSet().forEach(c -> b.addReviewer(toReviewerSetEntry(c)));
object
@@ -519,7 +517,7 @@
.forEach(r -> b.addSubmitRecord(GSON.toJson(new StoredSubmitRecord(r))));
object
.changeMessages()
- .forEach(m -> b.addChangeMessage(toByteString(m, ChangeMessageProtoConverter.INSTANCE)));
+ .forEach(m -> b.addChangeMessage(ChangeMessageProtoConverter.INSTANCE.toProto(m)));
object.publishedComments().values().forEach(c -> b.addPublishedComment(GSON.toJson(c)));
b.setUpdateCount(object.updateCount());
if (object.mergedOn() != null) {
@@ -530,12 +528,6 @@
return Protos.toByteArray(b.build());
}
- @VisibleForTesting
- static <T> ByteString toByteString(T object, ProtoConverter<?, T> converter) {
- MessageLite message = converter.toProto(object);
- return Protos.toByteString(message);
- }
-
private static ChangeColumnsProto toChangeColumnsProto(ChangeColumns cols) {
ChangeColumnsProto.Builder b =
ChangeColumnsProto.newBuilder()
@@ -635,12 +627,12 @@
.hashtags(proto.getHashtagList())
.patchSets(
proto.getPatchSetList().stream()
- .map(bytes -> parseProtoFrom(PatchSetProtoConverter.INSTANCE, bytes))
+ .map(msg -> PatchSetProtoConverter.INSTANCE.fromProto(msg))
.map(ps -> Maps.immutableEntry(ps.id(), ps))
.collect(toImmutableList()))
.approvals(
proto.getApprovalList().stream()
- .map(bytes -> parseProtoFrom(PatchSetApprovalProtoConverter.INSTANCE, bytes))
+ .map(msg -> PatchSetApprovalProtoConverter.INSTANCE.fromProto(msg))
.map(a -> Maps.immutableEntry(a.patchSetId(), a))
.collect(toImmutableList()))
.reviewers(toReviewerSet(proto.getReviewerList()))
@@ -660,7 +652,7 @@
.collect(toImmutableList()))
.changeMessages(
proto.getChangeMessageList().stream()
- .map(bytes -> parseProtoFrom(ChangeMessageProtoConverter.INSTANCE, bytes))
+ .map(msg -> ChangeMessageProtoConverter.INSTANCE.fromProto(msg))
.collect(toImmutableList()))
.publishedComments(
proto.getPublishedCommentList().stream()
@@ -671,12 +663,6 @@
return b.build();
}
- private static <P extends MessageLite, T> T parseProtoFrom(
- ProtoConverter<P, T> converter, ByteString byteString) {
- P message = Protos.parseUnchecked(converter.getParser(), byteString);
- return converter.fromProto(message);
- }
-
private static ChangeColumns toChangeColumns(Change.Id changeId, ChangeColumnsProto proto) {
ChangeColumns.Builder b =
ChangeColumns.builder()
diff --git a/java/com/google/gerrit/server/restapi/change/CommentJson.java b/java/com/google/gerrit/server/restapi/change/CommentJson.java
index 02f39ab..77b58c6 100644
--- a/java/com/google/gerrit/server/restapi/change/CommentJson.java
+++ b/java/com/google/gerrit/server/restapi/change/CommentJson.java
@@ -61,6 +61,7 @@
private boolean fillAccounts = true;
private boolean fillPatchSet;
private boolean fillCommentContext;
+ private int contextPadding;
@Inject
CommentJson(AccountLoader.Factory accountLoaderFactory, CommentContextCache commentContextCache) {
@@ -83,6 +84,11 @@
return this;
}
+ CommentJson setContextPadding(int contextPadding) {
+ this.contextPadding = contextPadding;
+ return this;
+ }
+
CommentJson setProjectKey(Project.NameKey project) {
this.project = project;
return this;
@@ -184,6 +190,7 @@
.id(r.id)
.path(r.path)
.patchset(r.patchSet)
+ .contextPadding(contextPadding)
.build();
}
diff --git a/java/com/google/gerrit/server/restapi/change/ListChangeComments.java b/java/com/google/gerrit/server/restapi/change/ListChangeComments.java
index fa7c1f5..c90e4fc 100644
--- a/java/com/google/gerrit/server/restapi/change/ListChangeComments.java
+++ b/java/com/google/gerrit/server/restapi/change/ListChangeComments.java
@@ -42,6 +42,7 @@
private final CommentsUtil commentsUtil;
private boolean includeContext;
+ private int contextPadding;
/**
* Optional parameter. If set, the contextLines field of the {@link ContextLineInfo} of the
@@ -54,6 +55,16 @@
this.includeContext = context;
}
+ /**
+ * Optional parameter. Works only if {@link #includeContext} is set to true. If {@link
+ * #contextPadding} is set, the context lines in the response will be padded with {@link
+ * #contextPadding} extra lines before and after the comment range.
+ */
+ @Option(name = "--context-padding")
+ public void setContextPadding(int contextPadding) {
+ this.contextPadding = contextPadding;
+ }
+
@Inject
ListChangeComments(
ChangeData.Factory changeDataFactory,
@@ -105,6 +116,7 @@
.setFillAccounts(true)
.setFillPatchSet(true)
.setFillCommentContext(includeContext)
+ .setContextPadding(contextPadding)
.setProjectKey(rsrc.getProject())
.setChangeId(rsrc.getId())
.newHumanCommentFormatter();
diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentContextIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentContextIT.java
index 8fa80ce..adfe56d 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/CommentContextIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/CommentContextIT.java
@@ -37,6 +37,7 @@
import java.util.ArrayList;
import java.util.List;
import java.util.stream.Collectors;
+import java.util.stream.IntStream;
import org.junit.Before;
import org.junit.Test;
@@ -286,6 +287,90 @@
assertThat(comments.get(0).contextLines).isEmpty();
}
+ @Test
+ public void commentContextWithZeroPadding() throws Exception {
+ String changeId = createChangeWithComment(3, 4);
+ assertContextLines(changeId, /* contextPadding= */ 0, ImmutableList.of(3, 4));
+ }
+
+ @Test
+ public void commentContextWithSmallPadding() throws Exception {
+ String changeId = createChangeWithComment(3, 4);
+ assertContextLines(changeId, /* contextPadding= */ 1, ImmutableList.of(2, 3, 4, 5));
+ }
+
+ @Test
+ public void commentContextWithSmallPaddingAtTheBeginningOfFile() throws Exception {
+ String changeId = createChangeWithComment(1, 2);
+ assertContextLines(changeId, /* contextPadding= */ 2, ImmutableList.of(1, 2, 3, 4));
+ }
+
+ @Test
+ public void commentContextWithPaddingLargerThanFileSize() throws Exception {
+ String changeId = createChangeWithComment(3, 3);
+ assertContextLines(
+ changeId,
+ /* contextPadding= */ 20,
+ ImmutableList.of(1, 2, 3, 4, 5, 6)); // file only contains six lines.
+ }
+
+ @Test
+ public void commentContextWithLargePaddingReturnsAdjustedMaximumPadding() throws Exception {
+ String changeId = createChangeWithCommentLarge(250, 250);
+ assertContextLines(
+ changeId,
+ /* contextPadding= */ 300,
+ IntStream.range(200, 301).boxed().collect(ImmutableList.toImmutableList()));
+ }
+
+ private String createChangeWithComment(int startLine, int endLine) throws Exception {
+ PushOneCommit.Result result =
+ createChange(testRepo, "master", SUBJECT, FILE_NAME, FILE_CONTENT, "topic");
+ String changeId = result.getChangeId();
+ String ps1 = result.getCommit().name();
+
+ Comment.Range commentRange = createCommentRange(startLine, endLine);
+ CommentInput comment =
+ CommentsUtil.newComment(FILE_NAME, Side.REVISION, commentRange, "comment", false);
+ CommentsUtil.addComments(gApi, changeId, ps1, comment);
+ return changeId;
+ }
+
+ private String createChangeWithCommentLarge(int startLine, int endLine) throws Exception {
+ StringBuilder largeContent = new StringBuilder();
+ for (int i = 0; i < 1000; i++) {
+ largeContent.append("line " + i + "\n");
+ }
+ PushOneCommit.Result result =
+ createChange(testRepo, "master", SUBJECT, FILE_NAME, largeContent.toString(), "topic");
+ String changeId = result.getChangeId();
+ String ps1 = result.getCommit().name();
+
+ Comment.Range commentRange = createCommentRange(startLine, endLine);
+ CommentInput comment =
+ CommentsUtil.newComment(FILE_NAME, Side.REVISION, commentRange, "comment", false);
+ CommentsUtil.addComments(gApi, changeId, ps1, comment);
+ return changeId;
+ }
+
+ private void assertContextLines(
+ String changeId, int contextPadding, ImmutableList<Integer> expectedLines) throws Exception {
+ List<CommentInfo> comments =
+ gApi.changes()
+ .id(changeId)
+ .commentsRequest()
+ .withContext(true)
+ .contextPadding(contextPadding)
+ .getAsList();
+
+ assertThat(comments).hasSize(1);
+ assertThat(
+ comments.get(0).contextLines.stream()
+ .map(c -> c.lineNumber)
+ .collect(Collectors.toList()))
+ .containsExactlyElementsIn(expectedLines);
+ }
+
private Comment.Range createCommentRange(int startLine, int endLine) {
Comment.Range range = new Comment.Range();
range.startLine = startLine;
diff --git a/javatests/com/google/gerrit/server/BUILD b/javatests/com/google/gerrit/server/BUILD
index 248c7d1..9a6b82b 100644
--- a/javatests/com/google/gerrit/server/BUILD
+++ b/javatests/com/google/gerrit/server/BUILD
@@ -49,6 +49,7 @@
"//java/com/google/gerrit/lifecycle",
"//java/com/google/gerrit/mail",
"//java/com/google/gerrit/metrics",
+ "//java/com/google/gerrit/proto",
"//java/com/google/gerrit/proto/testing",
"//java/com/google/gerrit/server",
"//java/com/google/gerrit/server/account/externalids/testing",
@@ -82,5 +83,6 @@
"//lib/truth:truth-java8-extension",
"//lib/truth:truth-proto-extension",
"//proto:cache_java_proto",
+ "//proto:entities_java_proto",
],
)
diff --git a/javatests/com/google/gerrit/server/cache/serialize/CommentContextSerializerTest.java b/javatests/com/google/gerrit/server/cache/serialize/CommentContextSerializerTest.java
index 84f290c..643c7b7 100644
--- a/javatests/com/google/gerrit/server/cache/serialize/CommentContextSerializerTest.java
+++ b/javatests/com/google/gerrit/server/cache/serialize/CommentContextSerializerTest.java
@@ -34,6 +34,7 @@
.id("commentId")
.path("pathHash")
.patchset(1)
+ .contextPadding(3)
.build();
byte[] serialized = CommentContextKey.Serializer.INSTANCE.serialize(k);
assertThat(k).isEqualTo(CommentContextKey.Serializer.INSTANCE.deserialize(serialized));
diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
index a1a1ca3..67181b7 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
@@ -17,7 +17,6 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat;
import static com.google.gerrit.proto.testing.SerializedClassSubject.assertThatSerializedClass;
-import static com.google.gerrit.server.notedb.ChangeNotesState.Serializer.toByteString;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
@@ -40,6 +39,8 @@
import com.google.gerrit.entities.converter.ChangeMessageProtoConverter;
import com.google.gerrit.entities.converter.PatchSetApprovalProtoConverter;
import com.google.gerrit.entities.converter.PatchSetProtoConverter;
+import com.google.gerrit.proto.Entities;
+import com.google.gerrit.proto.Protos;
import com.google.gerrit.server.AssigneeStatusUpdate;
import com.google.gerrit.server.ReviewerByEmailSet;
import com.google.gerrit.server.ReviewerSet;
@@ -332,7 +333,8 @@
.uploader(Account.id(2000))
.createdOn(cols.createdOn())
.build();
- ByteString ps1Bytes = toByteString(ps1, PatchSetProtoConverter.INSTANCE);
+ Entities.PatchSet ps1Proto = PatchSetProtoConverter.INSTANCE.toProto(ps1);
+ ByteString ps1Bytes = Protos.toByteString(ps1Proto);
assertThat(ps1Bytes.size()).isEqualTo(66);
PatchSet ps2 =
@@ -342,7 +344,8 @@
.uploader(Account.id(3000))
.createdOn(cols.lastUpdatedOn())
.build();
- ByteString ps2Bytes = toByteString(ps2, PatchSetProtoConverter.INSTANCE);
+ Entities.PatchSet ps2Proto = PatchSetProtoConverter.INSTANCE.toProto(ps2);
+ ByteString ps2Bytes = Protos.toByteString(ps2Proto);
assertThat(ps2Bytes.size()).isEqualTo(66);
assertThat(ps2Bytes).isNotEqualTo(ps1Bytes);
@@ -352,8 +355,8 @@
.setMetaId(SHA_BYTES)
.setChangeId(ID.get())
.setColumns(colsProto)
- .addPatchSet(ps2Bytes)
- .addPatchSet(ps1Bytes)
+ .addPatchSet(ps2Proto)
+ .addPatchSet(ps1Proto)
.build());
}
@@ -367,8 +370,8 @@
.value(1)
.granted(new Timestamp(1212L))
.build();
- ByteString a1Bytes = toByteString(a1, PatchSetApprovalProtoConverter.INSTANCE);
- assertThat(a1Bytes.size()).isEqualTo(43);
+ Entities.PatchSetApproval psa1 = PatchSetApprovalProtoConverter.INSTANCE.toProto(a1);
+ ByteString a1Bytes = Protos.toByteString(psa1);
PatchSetApproval a2 =
PatchSetApproval.builder()
@@ -378,7 +381,8 @@
.value(-1)
.granted(new Timestamp(3434L))
.build();
- ByteString a2Bytes = toByteString(a2, PatchSetApprovalProtoConverter.INSTANCE);
+ Entities.PatchSetApproval psa2 = PatchSetApprovalProtoConverter.INSTANCE.toProto(a2);
+ ByteString a2Bytes = Protos.toByteString(psa2);
assertThat(a2Bytes.size()).isEqualTo(49);
assertThat(a2Bytes).isNotEqualTo(a1Bytes);
@@ -390,8 +394,8 @@
.setMetaId(SHA_BYTES)
.setChangeId(ID.get())
.setColumns(colsProto)
- .addApproval(a2Bytes)
- .addApproval(a1Bytes)
+ .addApproval(psa2)
+ .addApproval(psa1)
.build());
}
@@ -722,7 +726,8 @@
Account.id(1000),
new Timestamp(1212L),
PatchSet.id(ID, 1));
- ByteString m1Bytes = toByteString(m1, ChangeMessageProtoConverter.INSTANCE);
+ Entities.ChangeMessage m1Proto = ChangeMessageProtoConverter.INSTANCE.toProto(m1);
+ ByteString m1Bytes = Protos.toByteString(m1Proto);
assertThat(m1Bytes.size()).isEqualTo(35);
ChangeMessage m2 =
@@ -731,7 +736,8 @@
Account.id(2000),
new Timestamp(3434L),
PatchSet.id(ID, 2));
- ByteString m2Bytes = toByteString(m2, ChangeMessageProtoConverter.INSTANCE);
+ Entities.ChangeMessage m2Proto = ChangeMessageProtoConverter.INSTANCE.toProto(m2);
+ ByteString m2Bytes = Protos.toByteString(m2Proto);
assertThat(m2Bytes.size()).isEqualTo(35);
assertThat(m2Bytes).isNotEqualTo(m1Bytes);
@@ -741,8 +747,8 @@
.setMetaId(SHA_BYTES)
.setChangeId(ID.get())
.setColumns(colsProto)
- .addChangeMessage(m2Bytes)
- .addChangeMessage(m1Bytes)
+ .addChangeMessage(m2Proto)
+ .addChangeMessage(m1Proto)
.build());
}
@@ -1007,6 +1013,60 @@
.build());
}
+ /* Transitional test. Remove once follow-up change is live without accidents. */
+ @Test
+ public void binaryCompatibility() throws Exception {
+ ChangeNotesState.Builder builder = newBuilder();
+ PatchSet ps1 =
+ PatchSet.builder()
+ .id(PatchSet.id(ID, 1))
+ .commitId(ObjectId.fromString("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"))
+ .uploader(Account.id(2000))
+ .createdOn(cols.createdOn())
+ .build();
+ PatchSetApproval a1 =
+ PatchSetApproval.builder()
+ .key(
+ PatchSetApproval.key(
+ ps1.id(), Account.id(2001), LabelId.create(LabelId.CODE_REVIEW)))
+ .value(1)
+ .granted(new Timestamp(1212L))
+ .build();
+
+ ChangeMessage m1 =
+ new ChangeMessage(
+ ChangeMessage.key(ID, "uuid1"),
+ Account.id(1000),
+ new Timestamp(1212L),
+ PatchSet.id(ID, 1));
+ ChangeNotesState state =
+ builder
+ .approvals(ImmutableMap.of(PatchSet.id(ID, 1), a1).entrySet())
+ .patchSets(ImmutableMap.of(ps1.id(), ps1).entrySet())
+ .changeMessages(ImmutableList.of(m1))
+ .build();
+
+ byte got[] = ChangeNotesState.Serializer.INSTANCE.serialize(state);
+ byte want[] =
+ new byte[] {
+ 10, 20, 18, 52, 86, 120, 18, 52, 86, 120, 18, 52, 86, 120, 18, 52, 86, 120, 18, 52, 86,
+ 120, 16, 123, 26, 89, 10, 41, 73, 97, 98, 99, 100, 97, 98, 99, 100, 97, 98, 99, 100, 97,
+ 98, 99, 100, 97, 98, 99, 100, 97, 98, 99, 100, 97, 98, 99, 100, 97, 98, 99, 100, 97, 98,
+ 99, 100, 97, 98, 99, 100, 16, -64, -60, 7, 24, -57, -88, 14, 32, -24, 7, 42, 17, 114, 101,
+ 102, 115, 47, 104, 101, 97, 100, 115, 47, 109, 97, 115, 116, 101, 114, 66, 11, 84, 101,
+ 115, 116, 32, 99, 104, 97, 110, 103, 101, -88, 1, 1, 50, 66, 10, 6, 10, 2, 8, 123, 16, 1,
+ 18, 42, 10, 40, 97, 97, 97, 97, 97, 97, 97, 97, 97, 97, 97, 97, 97, 97, 97, 97, 97, 97,
+ 97, 97, 97, 97, 97, 97, 97, 97, 97, 97, 97, 97, 97, 97, 97, 97, 97, 97, 97, 97, 97, 97,
+ 26, 3, 8, -48, 15, 33, 64, -30, 1, 0, 0, 0, 0, 0, 58, 43, 10, 28, 10, 6, 10, 2, 8, 123,
+ 16, 1, 18, 3, 8, -47, 15, 26, 13, 10, 11, 67, 111, 100, 101, 45, 82, 101, 118, 105, 101,
+ 119, 16, 1, 25, -68, 4, 0, 0, 0, 0, 0, 0, 64, 0, 122, 35, 10, 11, 10, 2, 8, 123, 18, 5,
+ 117, 117, 105, 100, 49, 18, 3, 8, -24, 7, 25, -68, 4, 0, 0, 0, 0, 0, 0, 42, 6, 10, 2, 8,
+ 123, 16, 1
+ };
+
+ assertThat(got).isEqualTo(want);
+ }
+
@Test
public void commentFields() throws Exception {
assertThatSerializedClass(Comment.Key.class)
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 86b714c..6e99a7d 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
@@ -52,6 +52,7 @@
import {AccountInfo} from '../../../types/common';
import {notUndefined} from '../../../types/types';
import {uniqueDefinedAvatar} from '../../../utils/account-util';
+import {PrimaryTab} from '../../../constants/constants';
export enum SummaryChipStyles {
INFO = 'info',
@@ -115,11 +116,21 @@
render() {
const chipClass = `summaryChip font-small ${this.styleType}`;
const grIcon = this.icon ? `gr-icons:${this.icon}` : '';
- return html`<div class="${chipClass}" role="button">
+ return html`<div
+ class="${chipClass}"
+ role="button"
+ @click="${this.handleClick}"
+ >
${this.icon && html`<iron-icon icon="${grIcon}"></iron-icon>`}
<slot></slot>
</div>`;
}
+
+ private handleClick(e: MouseEvent) {
+ e.stopPropagation();
+ e.preventDefault();
+ fireShowPrimaryTab(this, PrimaryTab.COMMENT_THREADS);
+ }
}
@customElement('gr-checks-chip')
@@ -219,16 +230,13 @@
private handleClick(e: MouseEvent) {
e.stopPropagation();
e.preventDefault();
- fireShowPrimaryTab(this, 'checks');
+ fireShowPrimaryTab(this, PrimaryTab.CHECKS);
}
}
/** What is the maximum number of expanded checks chips? */
const DETAILS_QUOTA = 3;
-/** What is the maximum number of links renderend within one chip? */
-const MAX_LINKS_PER_CHIP = 3;
-
@customElement('gr-change-summary')
export class GrChangeSummary extends GrLitElement {
private readonly newChangeSummaryUiEnabled = appContext.flagsService.isEnabled(
@@ -273,9 +281,11 @@
}
td.key {
padding-right: var(--spacing-l);
+ padding-bottom: var(--spacing-m);
}
td.value {
padding-right: var(--spacing-l);
+ padding-bottom: var(--spacing-m);
}
iron-icon.launch {
color: var(--gray-foreground);
@@ -320,15 +330,13 @@
if (runs.length <= this.detailsQuota) {
this.detailsQuota -= runs.length;
return runs.map(run => {
- const links = resultFilter(run)
+ const allLinks = resultFilter(run)
.reduce((links, result) => {
return links.concat(result.links ?? []);
}, [] as Link[])
- .filter(link => link.primary)
- .slice(0, MAX_LINKS_PER_CHIP);
- const count = resultFilter(run).length;
- const countText = count > 1 ? ` ${count}` : '';
- const text = `${run.checkName}${countText}`;
+ .filter(link => link.primary);
+ const links = allLinks.length === 1 ? allLinks : [];
+ const text = `${run.checkName}`;
return html`<gr-checks-chip
class="${icon}"
.icon="${icon}"
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 4976503..7cea964 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
@@ -31,9 +31,17 @@
PolymerDeepPropertyChange,
} from '@polymer/polymer/interfaces';
import {ChangeInfo} from '../../../types/common';
-import {CommentThread, isDraft, UIRobot} from '../../../utils/comment-util';
+import {
+ CommentThread,
+ isDraft,
+ UIRobot,
+ isUnresolved,
+ isDraftThread,
+} from '../../../utils/comment-util';
import {pluralize} from '../../../utils/string-util';
import {fireThreadListModifiedEvent} from '../../../utils/event-util';
+import {KnownExperimentId} from '../../../services/flags/flags';
+import {appContext} from '../../../services/app-context';
interface CommentThreadWithInfo {
thread: CommentThread;
@@ -91,6 +99,19 @@
@property({type: Boolean})
hideToggleButtons = false;
+ @property({type: Boolean})
+ _isNewChangeSummaryUiEnabled = false;
+
+ flagsService = appContext.flagsService;
+
+ /** @override */
+ ready() {
+ super.ready();
+ this._isNewChangeSummaryUiEnabled = this.flagsService.isEnabled(
+ KnownExperimentId.NEW_CHANGE_SUMMARY_UI
+ );
+ }
+
_computeShowDraftToggle(loggedIn?: boolean) {
return loggedIn ? 'show' : '';
}
@@ -431,6 +452,37 @@
return !!side;
}
+ _handleOnlyUnresolved() {
+ this.unresolvedOnly = true;
+ this._draftsOnly = false;
+ }
+
+ _handleOnlyDrafts() {
+ this._draftsOnly = true;
+ this.unresolvedOnly = false;
+ }
+
+ _handleAllComments() {
+ this._draftsOnly = false;
+ this.unresolvedOnly = false;
+ }
+
+ _showAllComments(draftsOnly?: boolean, unresolvedOnly?: boolean) {
+ return !draftsOnly && !unresolvedOnly;
+ }
+
+ _countUnresolved(threads?: CommentThread[]) {
+ return threads?.filter(isUnresolved).length ?? 0;
+ }
+
+ _countAllThreads(threads?: CommentThread[]) {
+ return threads?.length ?? 0;
+ }
+
+ _countDrafts(threads?: CommentThread[]) {
+ return threads?.filter(isDraftThread).length ?? 0;
+ }
+
/**
* Work around a issue on iOS when clicking turns into double tap
*/
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 2c21b4a..a4546a2 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
@@ -65,27 +65,72 @@
box-shadow: none;
padding-left: var(--spacing-m);
}
+ .header .categoryRadio {
+ height: 18px;
+ width: 18px;
+ }
+ .header label {
+ padding-left: 8px;
+ margin-right: 16px;
+ }
</style>
<template is="dom-if" if="[[!hideToggleButtons]]">
<div class="header">
- <div class="toggleItem">
- <paper-toggle-button
- id="unresolvedToggle"
- checked="{{!unresolvedOnly}}"
- on-tap="_onTapUnresolvedToggle"
- >All comments</paper-toggle-button
+ <template is="dom-if" if="[[!_isNewChangeSummaryUiEnabled]]">
+ <div class="toggleItem">
+ <paper-toggle-button
+ id="unresolvedToggle"
+ checked="{{!unresolvedOnly}}"
+ on-tap="_onTapUnresolvedToggle"
+ >All comments</paper-toggle-button
+ >
+ </div>
+ <div
+ class$="toggleItem draftToggle [[_computeShowDraftToggle(loggedIn)]]"
>
- </div>
- <div
- class$="toggleItem draftToggle [[_computeShowDraftToggle(loggedIn)]]"
- >
- <paper-toggle-button
- id="draftToggle"
- checked="{{_draftsOnly}}"
- on-tap="_onTapUnresolvedToggle"
- >Comments with drafts</paper-toggle-button
- >
- </div>
+ <paper-toggle-button
+ id="draftToggle"
+ checked="{{_draftsOnly}}"
+ on-tap="_onTapUnresolvedToggle"
+ >Comments with drafts</paper-toggle-button
+ >
+ </div>
+ </template>
+ <template is="dom-if" if="[[_isNewChangeSummaryUiEnabled]]">
+ <input
+ class="categoryRadio"
+ id="unresolvedRadio"
+ name="filterComments"
+ type="radio"
+ on-click="_handleOnlyUnresolved"
+ checked$="[[unresolvedOnly]]"
+ />
+ <label for="unresolvedRadio">
+ Unresolved ([[_countUnresolved(threads)]])
+ </label>
+ <input
+ class="categoryRadio"
+ id="draftsRadio"
+ name="filterComments"
+ type="radio"
+ on-click="_handleOnlyDrafts"
+ checked$="[[_draftsOnly]]"
+ />
+ <label for="draftsRadio">
+ Drafts ([[_countDrafts(threads)]])
+ </label>
+ <input
+ class="categoryRadio"
+ id="allRadio"
+ name="filterComments"
+ type="radio"
+ on-click="_handleAllComments"
+ checked$="[[_showAllComments(_draftsOnly, unresolvedOnly)]]"
+ />
+ <label for="all">
+ All ([[_countAllThreads(threads)]])
+ </label>
+ </template>
</div>
</template>
<div id="threads">
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-results.ts b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
index 4bb11b3..68ff67b 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-results.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
@@ -339,7 +339,10 @@
if (runs.length === 0) return;
return html`
<h3 class="categoryHeader heading-3">
- <iron-icon icon="gr-icons:check-circle" class="success"></iron-icon>
+ <iron-icon
+ icon="gr-icons:check-circle-outline"
+ class="success"
+ ></iron-icon>
Success
</h3>
<table class="resultsTable">
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-runs.ts b/polygerrit-ui/app/elements/checks/gr-checks-runs.ts
index 82c346a..6638c5f 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-runs.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-runs.ts
@@ -15,6 +15,7 @@
* limitations under the License.
*/
import {html} from 'lit-html';
+import {classMap} from 'lit-html/directives/class-map';
import {css, customElement, property} from 'lit-element';
import {GrLitElement} from '../lit/gr-lit-element';
import {CheckRun, RunStatus} from '../../api/checks';
@@ -33,12 +34,116 @@
updateStateSetResults,
} from '../../services/checks/checks-model';
-function renderRun(run: CheckRun) {
- const icon = iconForRun(run);
- return html`<div class="runChip ${icon}">
- <iron-icon icon="gr-icons:${icon}" class="${icon}"></iron-icon>
- <span>${run.checkName}</span>
- </div>`;
+/* The RunSelectedEvent is only used locally to communicate from <gr-checks-run>
+ to its <gr-checks-runs> parent. */
+
+interface RunSelectedEventDetail {
+ checkName: string;
+}
+
+type RunSelectedEvent = CustomEvent<RunSelectedEventDetail>;
+
+declare global {
+ interface HTMLElementEventMap {
+ 'run-selected': RunSelectedEvent;
+ }
+}
+
+function fireRunSelected(target: EventTarget, checkName: string) {
+ target.dispatchEvent(
+ new CustomEvent('run-selected', {
+ detail: {checkName},
+ composed: false,
+ bubbles: false,
+ })
+ );
+}
+
+@customElement('gr-checks-run')
+export class GrChecksRun extends GrLitElement {
+ static get styles() {
+ return [
+ sharedStyles,
+ css`
+ :host {
+ display: block;
+ --thick-border: 6px;
+ }
+ .chip {
+ display: block;
+ font-weight: var(--font-weight-bold);
+ border: 1px solid var(--border-color);
+ border-radius: var(--border-radius);
+ padding: var(--spacing-s) var(--spacing-m);
+ margin-top: var(--spacing-s);
+ cursor: default;
+ }
+ .chip.error {
+ border-left: var(--thick-border) solid var(--error-foreground);
+ }
+ .chip.warning {
+ border-left: var(--thick-border) solid var(--warning-foreground);
+ }
+ .chip.info-outline {
+ border-left: var(--thick-border) solid var(--info-foreground);
+ }
+ .chip.check-circle-outline {
+ border-left: var(--thick-border) solid var(--success-foreground);
+ }
+ .chip.timelapse {
+ border-left: var(--thick-border) solid var(--border-color);
+ }
+ .chip.placeholder {
+ border-left: var(--thick-border) solid var(--border-color);
+ }
+ .chip.error iron-icon {
+ color: var(--error-foreground);
+ }
+ .chip.warning iron-icon {
+ color: var(--warning-foreground);
+ }
+ .chip.info-outline iron-icon {
+ color: var(--info-foreground);
+ }
+ .chip.check-circle-outline iron-icon {
+ color: var(--success-foreground);
+ }
+ /* Additional 'div' for increased specificity. */
+ div.chip.selected {
+ border: 1px solid var(--selected-foreground);
+ background-color: var(--selected-background);
+ padding-left: calc(var(--spacing-m) + var(--thick-border) - 1px);
+ }
+ div.chip.selected iron-icon {
+ color: var(--selected-foreground);
+ }
+ `,
+ ];
+ }
+
+ @property()
+ run!: CheckRun;
+
+ @property()
+ selected = false;
+
+ render() {
+ const icon = this.selected ? 'check-circle' : iconForRun(this.run);
+ const classes = {chip: true, [icon]: true, selected: this.selected};
+
+ return html`
+ <div @click="${this._handleChipClick}" class="${classMap(classes)}">
+ <iron-icon icon="gr-icons:${icon}"></iron-icon>
+ <span>${this.run.checkName}</span>
+ </div>
+ `;
+ }
+
+ _handleChipClick(e: MouseEvent) {
+ e.stopPropagation();
+ e.preventDefault();
+ fireRunSelected(this, this.run.checkName);
+ }
}
@customElement('gr-checks-runs')
@@ -46,6 +151,8 @@
@property()
runs: CheckRun[] = [];
+ private selectedRuns = new Set<string>();
+
constructor() {
super();
this.subscribe('runs', allRuns$);
@@ -63,43 +170,6 @@
padding-top: var(--spacing-l);
text-transform: capitalize;
}
- .runChip {
- font-weight: var(--font-weight-bold);
- border: 1px solid var(--border-color);
- border-radius: var(--border-radius);
- padding: var(--spacing-s) var(--spacing-m);
- margin-top: var(--spacing-s);
- }
- .runChip.error {
- border-left: 6px solid var(--error-foreground);
- }
- .runChip.warning {
- border-left: 6px solid var(--warning-foreground);
- }
- .runChip.info-outline {
- border-left: 6px solid var(--info-foreground);
- }
- .runChip.check-circle {
- border-left: 6px solid var(--success-foreground);
- }
- .runChip.timelapse {
- border-left: 6px solid var(--border-color);
- }
- .runnable .runChip.placeholder iron-icon {
- display: none;
- }
- .runChip.error iron-icon {
- color: var(--error-foreground);
- }
- .runChip.warning iron-icon {
- color: var(--warning-foreground);
- }
- .runChip.info-outline iron-icon {
- color: var(--info-foreground);
- }
- .runChip.check-circle iron-icon {
- color: var(--success-foreground);
- }
.testing {
margin-top: var(--spacing-xxl);
color: var(--deemphasized-text-color);
@@ -175,14 +245,34 @@
return html`
<div class="${status.toLowerCase()}">
<h3 class="statusHeader heading-3">${status.toLowerCase()}</h3>
- ${runs.map(renderRun)}
+ ${runs.map(run => this.renderRun(run))}
</div>
`;
}
+
+ renderRun(run: CheckRun) {
+ const selected = this.selectedRuns.has(run.checkName);
+ return html`<gr-checks-run
+ .run="${run}"
+ .selected="${selected}"
+ @run-selected="${this.handleRunSelected}"
+ ></gr-checks-run>`;
+ }
+
+ handleRunSelected(e: RunSelectedEvent) {
+ const checkName = e.detail.checkName;
+ if (this.selectedRuns.has(checkName)) {
+ this.selectedRuns.delete(checkName);
+ } else {
+ this.selectedRuns.add(checkName);
+ }
+ this.requestUpdate();
+ }
}
declare global {
interface HTMLElementTagNameMap {
+ 'gr-checks-run': GrChecksRun;
'gr-checks-runs': GrChecksRuns;
}
}
diff --git a/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.ts b/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.ts
index e499b8b..0a3ef5b 100644
--- a/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.ts
+++ b/polygerrit-ui/app/elements/shared/gr-icons/gr-icons.ts
@@ -79,8 +79,10 @@
<g id="content-copy"><path d="M16 1H4c-1.1 0-2 .9-2 2v14h2V3h12V1zm3 4H8c-1.1 0-2 .9-2 2v14c0 1.1.9 2 2 2h11c1.1 0 2-.9 2-2V7c0-1.1-.9-2-2-2zm0 16H8V7h11v14z"></path></g>
<!-- This is a custom PolyGerrit SVG -->
<g id="check"><path d="M9 16.17L4.83 12l-1.42 1.41L9 19 21 7l-1.41-1.41z"></path></g>
- <!-- This SVG is a copy from material.io https://material.io/icons/#check-circle-->
- <g id="check-circle"><path d="M0 0h24v24H0V0zm0 0h24v24H0V0z" fill="none"/><path d="M16.59 7.58L10 14.17l-3.59-3.58L5 12l5 5 8-8zM12 2C6.48 2 2 6.48 2 12s4.48 10 10 10 10-4.48 10-10S17.52 2 12 2zm0 18c-4.42 0-8-3.58-8-8s3.58-8 8-8 8 3.58 8 8-3.58 8-8 8z"/></g>
+ <!-- This SVG is a copy from material.io https://material.io/icons/#check_circle-->
+ <g id="check-circle"><path d="M0 0h24v24H0z" fill="none"/><path d="M12 2C6.48 2 2 6.48 2 12s4.48 10 10 10 10-4.48 10-10S17.52 2 12 2zm-2 15l-5-5 1.41-1.41L10 14.17l7.59-7.59L19 8l-9 9z"/></g>
+ <!-- This SVG is a copy from material.io https://material.io/icons/#check_circle_outline-->
+ <g id="check-circle-outline"><path d="M0 0h24v24H0V0zm0 0h24v24H0V0z" fill="none"/><path d="M16.59 7.58L10 14.17l-3.59-3.58L5 12l5 5 8-8zM12 2C6.48 2 2 6.48 2 12s4.48 10 10 10 10-4.48 10-10S17.52 2 12 2zm0 18c-4.42 0-8-3.58-8-8s3.58-8 8-8 8 3.58 8 8-3.58 8-8 8z"/></g>
<!-- This is a custom PolyGerrit SVG -->
<g id="robot"><path d="M4.137453,5.61015591 L4.54835569,1.5340419 C4.5717665,1.30180904 4.76724872,1.12504213 5.00065859,1.12504213 C5.23327176,1.12504213 5.42730868,1.30282046 5.44761309,1.53454578 L5.76084628,5.10933916 C6.16304484,5.03749412 6.57714381,5 7,5 L17,5 C20.8659932,5 24,8.13400675 24,12 L24,15.1250421 C24,18.9910354 20.8659932,22.1250421 17,22.1250421 L7,22.1250421 C3.13400675,22.1250421 2.19029351e-15,18.9910354 0,15.1250421 L0,12 C-3.48556243e-16,9.15382228 1.69864167,6.70438358 4.137453,5.61015591 Z M5.77553049,6.12504213 C3.04904264,6.69038358 1,9.10590202 1,12 L1,15.1250421 C1,18.4387506 3.6862915,21.1250421 7,21.1250421 L17,21.1250421 C20.3137085,21.1250421 23,18.4387506 23,15.1250421 L23,12 C23,8.6862915 20.3137085,6 17,6 L7,6 C6.60617231,6 6.2212068,6.03794347 5.84855971,6.11037415 L5.84984496,6.12504213 L5.77553049,6.12504213 Z M6.93003717,6.95027711 L17.1232083,6.95027711 C19.8638332,6.95027711 22.0855486,9.17199258 22.0855486,11.9126175 C22.0855486,14.6532424 19.8638332,16.8749579 17.1232083,16.8749579 L6.93003717,16.8749579 C4.18941226,16.8749579 1.9676968,14.6532424 1.9676968,11.9126175 C1.9676968,9.17199258 4.18941226,6.95027711 6.93003717,6.95027711 Z M7.60124392,14.0779303 C9.03787127,14.0779303 10.2024878,12.9691885 10.2024878,11.6014862 C10.2024878,10.2337839 9.03787127,9.12504213 7.60124392,9.12504213 C6.16461657,9.12504213 5,10.2337839 5,11.6014862 C5,12.9691885 6.16461657,14.0779303 7.60124392,14.0779303 Z M16.617997,14.1098288 C18.0638768,14.1098288 19.2359939,12.9939463 19.2359939,11.6174355 C19.2359939,10.2409246 18.0638768,9.12504213 16.617997,9.12504213 C15.1721172,9.12504213 14,10.2409246 14,11.6174355 C14,12.9939463 15.1721172,14.1098288 16.617997,14.1098288 Z M9.79751216,18.1250421 L15,18.1250421 L15,19.1250421 C15,19.6773269 14.5522847,20.1250421 14,20.1250421 L10.7975122,20.1250421 C10.2452274,20.1250421 9.79751216,19.6773269 9.79751216,19.1250421 L9.79751216,18.1250421 Z"></path></g>
<!-- This is a custom PolyGerrit SVG -->
diff --git a/polygerrit-ui/app/services/checks/checks-util.ts b/polygerrit-ui/app/services/checks/checks-util.ts
index 344983f..86b9b47 100644
--- a/polygerrit-ui/app/services/checks/checks-util.ts
+++ b/polygerrit-ui/app/services/checks/checks-util.ts
@@ -46,7 +46,7 @@
switch (status) {
// Note that this is only for COMPLETED without results!
case RunStatus.COMPLETED:
- return 'check-circle';
+ return 'check-circle-outline';
case RunStatus.RUNNABLE:
return 'placeholder';
case RunStatus.RUNNING:
diff --git a/polygerrit-ui/app/styles/themes/app-theme.ts b/polygerrit-ui/app/styles/themes/app-theme.ts
index 15099e2..c3b0681 100644
--- a/polygerrit-ui/app/styles/themes/app-theme.ts
+++ b/polygerrit-ui/app/styles/themes/app-theme.ts
@@ -74,6 +74,8 @@
--warning-background: var(--orange-50);
--info-foreground: var(--blue-700);
--info-background: var(--blue-50);
+ --selected-foreground: var(--blue-700);
+ --selected-background: var(--blue-50);
--info-deemphasized-foreground: var(--gray-300);
--info-deemphasized-background: var(--gray-50);
--success-foreground: var(--green-700);
diff --git a/polygerrit-ui/app/utils/common-util.ts b/polygerrit-ui/app/utils/common-util.ts
index 5b332ea..ad76b79 100644
--- a/polygerrit-ui/app/utils/common-util.ts
+++ b/polygerrit-ui/app/utils/common-util.ts
@@ -47,6 +47,26 @@
}
/**
+ * Throws an error with the provided error message if the condition is false.
+ */
+export function check(
+ condition: boolean,
+ errorMessage: string
+): asserts condition {
+ if (!condition) throw new Error(errorMessage);
+}
+
+/**
+ * Throws an error if the property is not defined.
+ */
+export function checkProperty(
+ condition: boolean,
+ propertyName: string
+): asserts condition {
+ check(condition, `missing required property '${propertyName}'`);
+}
+
+/**
* Returns true, if both sets contain the same members.
*/
export function areSetsEqual<T>(a: Set<T>, b: Set<T>): boolean {
diff --git a/proto/BUILD b/proto/BUILD
index 57be265..7aa761d 100644
--- a/proto/BUILD
+++ b/proto/BUILD
@@ -4,6 +4,7 @@
proto_library(
name = "cache_proto",
srcs = ["cache.proto"],
+ deps = [":entities_proto"],
)
java_proto_library(
diff --git a/proto/cache.proto b/proto/cache.proto
index f3db71f..4fd037d 100644
--- a/proto/cache.proto
+++ b/proto/cache.proto
@@ -18,7 +18,9 @@
option java_package = "com.google.gerrit.server.cache.proto";
-// Serialized form of com.google.gerrit.server.change.CHangeKindCacheImpl.Key.
+import "proto/entities.proto";
+
+// Serialized form of com.google.gerrit.server.change.ChangeKindCacheImpl.Key.
// Next ID: 4
message ChangeKindKeyProto {
bytes prior = 1;
@@ -140,11 +142,9 @@
repeated string hashtag = 5;
- // Raw PatchSet proto as produced by PatchSetProtoConverter.
- repeated bytes patch_set = 6;
+ repeated devtools.gerritcodereview.PatchSet patch_set = 6;
- // Raw PatchSetApproval proto as produced by PatchSetApprovalProtoConverter.
- repeated bytes approval = 7;
+ repeated devtools.gerritcodereview.PatchSetApproval approval = 7;
// Next ID: 4
message ReviewerSetEntryProto {
@@ -184,8 +184,7 @@
// com.google.gerrit.server.index.change.ChangeField.StoredSubmitRecord.
repeated string submit_record = 14;
- // Raw ChangeMessage proto as produced by ChangeMessageProtoConverter.
- repeated bytes change_message = 15;
+ repeated devtools.gerritcodereview.ChangeMessage change_message = 15;
// JSON produced from com.google.gerrit.entities.Comment.
repeated string published_comment = 16;
@@ -511,7 +510,7 @@
}
// Serialized form of com.google.gerrit.server.comment.CommentContextCacheImpl.Key
-// Next ID: 6
+// Next ID: 7
message CommentContextKeyProto {
string project = 1;
string change_id = 2;
@@ -520,6 +519,8 @@
// hashed with the murmur3_128 hash function
string path_hash = 5;
+
+ int32 context_padding = 6;
}
// Serialized form of a list of com.google.gerrit.extensions.common.ContextLineInfo
