Merge "Nearly convert <gr-rest-api-interface> element to a simple class"
diff --git a/java/com/google/gerrit/entities/CommentContext.java b/java/com/google/gerrit/entities/CommentContext.java
index 183f6d0..c8c8a76 100644
--- a/java/com/google/gerrit/entities/CommentContext.java
+++ b/java/com/google/gerrit/entities/CommentContext.java
@@ -20,10 +20,16 @@
/** An entity class representing all context lines of a comment. */
@AutoValue
public abstract class CommentContext {
+ private static final CommentContext EMPTY = new AutoValue_CommentContext(ImmutableMap.of());
+
public static CommentContext create(ImmutableMap<Integer, String> lines) {
return new AutoValue_CommentContext(lines);
}
/** Map of {line number, line text} of the context lines of a comment */
public abstract ImmutableMap<Integer, String> lines();
+
+ public static CommentContext empty() {
+ return EMPTY;
+ }
}
diff --git a/java/com/google/gerrit/server/CommentContextLoader.java b/java/com/google/gerrit/server/CommentContextLoader.java
index 68a80c3..0422cb9 100644
--- a/java/com/google/gerrit/server/CommentContextLoader.java
+++ b/java/com/google/gerrit/server/CommentContextLoader.java
@@ -84,6 +84,7 @@
for (Comment comment : commentsByCommitId.get(commitId)) {
Optional<Range> range = getStartAndEndLines(comment);
if (!range.isPresent()) {
+ result.put(comment, CommentContext.empty());
continue;
}
// TODO(ghareeb): We can further group the comments by file paths to avoid opening
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index cb4b534..dd39262 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -161,6 +161,7 @@
import com.google.gerrit.server.mime.FileTypeRegistry;
import com.google.gerrit.server.mime.MimeUtilFileTypeRegistry;
import com.google.gerrit.server.notedb.NoteDbModule;
+import com.google.gerrit.server.patch.DiffOperationsImpl;
import com.google.gerrit.server.patch.PatchListCacheImpl;
import com.google.gerrit.server.patch.PatchScriptFactory;
import com.google.gerrit.server.patch.PatchScriptFactoryForAutoFix;
@@ -243,6 +244,7 @@
install(ServiceUserClassifierImpl.module());
install(PatchListCacheImpl.module());
install(ProjectCacheImpl.module());
+ install(DiffOperationsImpl.module());
install(SectionSortCache.module());
install(SubmitStrategy.module());
install(TagCache.module());
diff --git a/java/com/google/gerrit/server/patch/BaseCommitUtil.java b/java/com/google/gerrit/server/patch/BaseCommitUtil.java
new file mode 100644
index 0000000..de4a10e
--- /dev/null
+++ b/java/com/google/gerrit/server/patch/BaseCommitUtil.java
@@ -0,0 +1,130 @@
+// Copyright (C) 2020 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.patch;
+
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.git.InMemoryInserter;
+import com.google.gerrit.server.git.MergeUtil;
+import com.google.inject.Inject;
+import java.io.IOException;
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectInserter;
+import org.eclipse.jgit.lib.ObjectReader;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.merge.ThreeWayMergeStrategy;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevObject;
+import org.eclipse.jgit.revwalk.RevWalk;
+
+/** A utility class for computing the base commit / parent for a specific patchset commit. */
+class BaseCommitUtil {
+ private final AutoMerger autoMerger;
+ private final ThreeWayMergeStrategy mergeStrategy;
+
+ /** If true, auto-merge results are stored in the repository. */
+ private final boolean saveAutomerge;
+
+ private final GitRepositoryManager repoManager;
+
+ @Inject
+ BaseCommitUtil(AutoMerger am, @GerritServerConfig Config cfg, GitRepositoryManager repoManager) {
+ this.autoMerger = am;
+ this.saveAutomerge = AutoMerger.cacheAutomerge(cfg);
+ this.mergeStrategy = MergeUtil.getMergeStrategy(cfg);
+ this.repoManager = repoManager;
+ }
+
+ RevObject getBaseCommit(Project.NameKey project, ObjectId newCommit, @Nullable Integer parentNum)
+ throws IOException {
+ try (Repository repo = repoManager.openRepository(project);
+ ObjectInserter ins = newInserter(repo);
+ ObjectReader reader = ins.newReader();
+ RevWalk rw = new RevWalk(reader)) {
+ return getParentCommit(repo, ins, rw, parentNum, newCommit);
+ }
+ }
+
+ /**
+ * Returns the number of parent commits of the commit represented by the commitId parameter.
+ *
+ * @param project a specific git repository.
+ * @param commitId 20 bytes commitId SHA-1 hash.
+ * @return an integer representing the number of parents of the designated commit.
+ */
+ int getNumParents(Project.NameKey project, ObjectId commitId) throws IOException {
+ try (Repository repo = repoManager.openRepository(project);
+ ObjectInserter ins = repo.newObjectInserter();
+ ObjectReader reader = ins.newReader();
+ RevWalk rw = new RevWalk(reader)) {
+ RevCommit current = rw.parseCommit(commitId);
+ return current.getParentCount();
+ }
+ }
+
+ /**
+ * Returns the parent commit Object of the commit represented by the commitId parameter.
+ *
+ * @param repo a git repository.
+ * @param ins a git object inserter in the database.
+ * @param rw a {@link RevWalk} object of the repository.
+ * @param parentNum used to identify the parent number for merge commits. If parentNum is null and
+ * {@code commitId} has two parents, the auto-merge commit will be returned. If {@code
+ * commitId} has a single parent, it will be returned.
+ * @param commitId 20 bytes commitId SHA-1 hash.
+ * @return Returns the parent commit of the commit represented by the commitId parameter. Note
+ * that auto-merge is not supported for commits having more than two parents.
+ */
+ RevObject getParentCommit(
+ Repository repo,
+ ObjectInserter ins,
+ RevWalk rw,
+ @Nullable Integer parentNum,
+ ObjectId commitId)
+ throws IOException {
+ RevCommit current = rw.parseCommit(commitId);
+ switch (current.getParentCount()) {
+ case 0:
+ return rw.parseAny(emptyTree(ins));
+ case 1:
+ return current.getParent(0);
+ default:
+ if (parentNum != null) {
+ RevCommit r = current.getParent(parentNum - 1);
+ rw.parseBody(r);
+ return r;
+ }
+ // Only support auto-merge for 2 parents, not octopus merges
+ if (current.getParentCount() == 2) {
+ return autoMerger.merge(repo, rw, ins, current, mergeStrategy);
+ }
+ return null;
+ }
+ }
+
+ private ObjectInserter newInserter(Repository repo) {
+ return saveAutomerge ? repo.newObjectInserter() : new InMemoryInserter(repo);
+ }
+
+ private static ObjectId emptyTree(ObjectInserter ins) throws IOException {
+ ObjectId id = ins.insert(Constants.OBJ_TREE, new byte[] {});
+ ins.flush();
+ return id;
+ }
+}
diff --git a/java/com/google/gerrit/server/patch/DiffOperations.java b/java/com/google/gerrit/server/patch/DiffOperations.java
new file mode 100644
index 0000000..a6c7c81
--- /dev/null
+++ b/java/com/google/gerrit/server/patch/DiffOperations.java
@@ -0,0 +1,72 @@
+// Copyright (C) 2020 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.patch;
+
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.Patch;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.server.patch.filediff.FileDiffOutput;
+import java.util.Map;
+import org.eclipse.jgit.lib.ObjectId;
+
+/**
+ * An interface for all file diff related operations. Clients should use this interface to request:
+ *
+ * <ul>
+ * <li>The list of modified files between two commits.
+ * <li>The list of modified files between a commit and its parent or the auto-merge.
+ * <li>The detailed file diff for a single file path (TODO:ghareeb).
+ * <li>The Intra-line diffs for a single file path (TODO:ghareeb).
+ * </ul>
+ */
+public interface DiffOperations {
+
+ /**
+ * Returns the list of added, deleted or modified files between a commit against its base. The
+ * {@link Patch#COMMIT_MSG} and {@link Patch#MERGE_LIST} (for merge commits) are also returned.
+ *
+ * <p>If parentNum is set, it is used as the old commit in the diff. Otherwise, if the {@code
+ * newCommit} has only one parent, it is used as base. If {@code newCommit} has two parents, the
+ * auto-merge commit is computed and used as base. The auto-merge for more than two parents is not
+ * supported.
+ *
+ * @param project a project name representing a git repository.
+ * @param newCommit 20 bytes SHA-1 of the new commit used in the diff.
+ * @param parentNum integer specifying which parent to use as base. If null, the only parent will
+ * be used or the auto-merge if {@code newCommit} is a merge commit.
+ * @return the list of modified files between the two commits.
+ * @throws DiffNotAvailableException if auto-merge is requested for a commit having more than two
+ * parents, if the {@code newCommit} could not be parsed for extracting the base commit, or if
+ * an internal error occurred in Git while evaluating the diff.
+ */
+ Map<String, FileDiffOutput> getModifiedFilesAgainstParentOrAutoMerge(
+ Project.NameKey project, ObjectId newCommit, @Nullable Integer parentNum)
+ throws DiffNotAvailableException;
+
+ /**
+ * Returns the list of added, deleted or modified files between two commits (patchsets). The
+ * commit message and merge list (for merge commits) are also returned.
+ *
+ * @param project a project name representing a git repository.
+ * @param oldCommit 20 bytes SHA-1 of the old commit used in the diff.
+ * @param newCommit 20 bytes SHA-1 of the new commit used in the diff.
+ * @return the list of modified files between the two commits.
+ * @throws DiffNotAvailableException if an internal error occurred in Git while evaluating the
+ * diff.
+ */
+ Map<String, FileDiffOutput> getModifiedFilesBetweenPatchsets(
+ Project.NameKey project, ObjectId oldCommit, ObjectId newCommit)
+ throws DiffNotAvailableException;
+}
diff --git a/java/com/google/gerrit/server/patch/DiffOperationsImpl.java b/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
new file mode 100644
index 0000000..1a8d85b
--- /dev/null
+++ b/java/com/google/gerrit/server/patch/DiffOperationsImpl.java
@@ -0,0 +1,199 @@
+// Copyright (C) 2020 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.patch;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.Patch;
+import com.google.gerrit.entities.Patch.ChangeType;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
+import com.google.gerrit.server.cache.CacheModule;
+import com.google.gerrit.server.patch.diff.ModifiedFilesCache;
+import com.google.gerrit.server.patch.diff.ModifiedFilesCacheImpl;
+import com.google.gerrit.server.patch.diff.ModifiedFilesCacheKey;
+import com.google.gerrit.server.patch.filediff.FileDiffCache;
+import com.google.gerrit.server.patch.filediff.FileDiffCacheImpl;
+import com.google.gerrit.server.patch.filediff.FileDiffCacheKey;
+import com.google.gerrit.server.patch.filediff.FileDiffOutput;
+import com.google.gerrit.server.patch.gitdiff.GitModifiedFilesCacheImpl;
+import com.google.gerrit.server.patch.gitdiff.ModifiedFile;
+import com.google.gerrit.server.patch.gitfilediff.GitFileDiffCacheImpl;
+import com.google.gerrit.server.patch.gitfilediff.GitFileDiffCacheImpl.DiffAlgorithm;
+import com.google.inject.Inject;
+import com.google.inject.Module;
+import java.io.IOException;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.revwalk.RevObject;
+
+/**
+ * Provides different file diff operations. Uses the underlying Git/Gerrit caches to speed up the
+ * diff computation.
+ */
+public class DiffOperationsImpl implements DiffOperations {
+ private static final int RENAME_SCORE = 60;
+ private static final DiffAlgorithm DEFAULT_DIFF_ALGORITHM = DiffAlgorithm.HISTOGRAM;
+
+ private final ModifiedFilesCache modifiedFilesCache;
+ private final FileDiffCache fileDiffCache;
+ private final BaseCommitUtil baseCommitUtil;
+
+ public static Module module() {
+ return new CacheModule() {
+ @Override
+ protected void configure() {
+ bind(DiffOperations.class).to(DiffOperationsImpl.class);
+ install(GitModifiedFilesCacheImpl.module());
+ install(ModifiedFilesCacheImpl.module());
+ install(GitFileDiffCacheImpl.module());
+ install(FileDiffCacheImpl.module());
+ }
+ };
+ }
+
+ @Inject
+ public DiffOperationsImpl(
+ ModifiedFilesCache modifiedFilesCache,
+ FileDiffCache fileDiffCache,
+ BaseCommitUtil baseCommit) {
+ this.modifiedFilesCache = modifiedFilesCache;
+ this.fileDiffCache = fileDiffCache;
+ this.baseCommitUtil = baseCommit;
+ }
+
+ @Override
+ public Map<String, FileDiffOutput> getModifiedFilesAgainstParentOrAutoMerge(
+ Project.NameKey project, ObjectId newCommit, @Nullable Integer parent)
+ throws DiffNotAvailableException {
+ try {
+ if (parent != null) {
+ RevObject base = baseCommitUtil.getBaseCommit(project, newCommit, parent);
+ return getModifiedFiles(project, base, newCommit, ComparisonType.againstParent(parent));
+ }
+ int numParents = baseCommitUtil.getNumParents(project, newCommit);
+ if (numParents > 2) {
+ throw new DiffNotAvailableException(
+ "Diff against auto-merge for merge commits "
+ + "with more than two parents is not supported. Commit "
+ + newCommit
+ + " has "
+ + numParents
+ + " parents");
+ }
+ if (numParents == 1) {
+ RevObject base = baseCommitUtil.getBaseCommit(project, newCommit, parent);
+ ComparisonType cmp = ComparisonType.againstParent(1);
+ return getModifiedFiles(project, base, newCommit, cmp);
+ }
+ RevObject autoMerge = baseCommitUtil.getBaseCommit(project, newCommit, null);
+ return getModifiedFiles(project, autoMerge, newCommit, ComparisonType.againstAutoMerge());
+ } catch (IOException e) {
+ throw new DiffNotAvailableException(e);
+ }
+ }
+
+ @Override
+ public Map<String, FileDiffOutput> getModifiedFilesBetweenPatchsets(
+ Project.NameKey project, ObjectId oldCommit, ObjectId newCommit)
+ throws DiffNotAvailableException {
+ ComparisonType cmp = ComparisonType.againstOtherPatchSet();
+ return getModifiedFiles(project, oldCommit, newCommit, cmp);
+ }
+
+ private Map<String, FileDiffOutput> getModifiedFiles(
+ Project.NameKey project, ObjectId oldCommit, ObjectId newCommit, ComparisonType cmp)
+ throws DiffNotAvailableException {
+ ImmutableMap.Builder<String, FileDiffOutput> files = ImmutableMap.builder();
+ try {
+ ImmutableList<ModifiedFile> modifiedFiles =
+ modifiedFilesCache.get(createModifiedFilesKey(project, oldCommit, newCommit));
+
+ List<FileDiffCacheKey> fileCacheKeys =
+ modifiedFiles.stream()
+ .map(
+ entity ->
+ createFileDiffCacheKey(
+ project,
+ oldCommit,
+ newCommit,
+ entity.newPath().isPresent()
+ ? entity.newPath().get()
+ : entity.oldPath().get()))
+ .collect(Collectors.toList());
+
+ fileCacheKeys.add(createFileDiffCacheKey(project, oldCommit, newCommit, Patch.COMMIT_MSG));
+
+ if (cmp.isAgainstAutoMerge() || isMergeAgainstParent(cmp, project, newCommit)) {
+ fileCacheKeys.add(createFileDiffCacheKey(project, oldCommit, newCommit, Patch.MERGE_LIST));
+ }
+
+ ImmutableMap<FileDiffCacheKey, FileDiffOutput> fileDiffs =
+ fileDiffCache.getAll(fileCacheKeys);
+
+ for (Map.Entry<FileDiffCacheKey, FileDiffOutput> entry : fileDiffs.entrySet()) {
+ FileDiffOutput fileDiffOutput = entry.getValue();
+ if (fileDiffOutput.isEmpty() || allDueToRebase(fileDiffOutput)) {
+ continue;
+ }
+ if (fileDiffOutput.changeType().get() == Patch.ChangeType.DELETED) {
+ files.put(fileDiffOutput.oldPath().get(), fileDiffOutput);
+ } else {
+ files.put(fileDiffOutput.newPath().get(), fileDiffOutput);
+ }
+ }
+ } catch (IOException e) {
+ throw new DiffNotAvailableException(e);
+ }
+ return files.build();
+ }
+
+ private static boolean allDueToRebase(FileDiffOutput fileDiffOutput) {
+ return fileDiffOutput.allEditsDueToRebase()
+ && (!(fileDiffOutput.changeType().get() == ChangeType.RENAMED
+ || fileDiffOutput.changeType().get() == ChangeType.COPIED));
+ }
+
+ private boolean isMergeAgainstParent(ComparisonType cmp, Project.NameKey project, ObjectId commit)
+ throws IOException {
+ return (cmp.isAgainstParent() && baseCommitUtil.getNumParents(project, commit) > 1);
+ }
+
+ private static ModifiedFilesCacheKey createModifiedFilesKey(
+ Project.NameKey project, ObjectId aCommit, ObjectId bCommit) {
+ return ModifiedFilesCacheKey.builder()
+ .project(project)
+ .aCommit(aCommit)
+ .bCommit(bCommit)
+ .renameScore(RENAME_SCORE)
+ .build();
+ }
+
+ private static FileDiffCacheKey createFileDiffCacheKey(
+ Project.NameKey project, ObjectId aCommit, ObjectId bCommit, String newPath) {
+ return FileDiffCacheKey.builder()
+ .project(project)
+ .oldCommit(aCommit)
+ .newCommit(bCommit)
+ .newFilePath(newPath)
+ .renameScore(RENAME_SCORE)
+ .diffAlgorithm(DEFAULT_DIFF_ALGORITHM)
+ .whitespace(Whitespace.IGNORE_NONE)
+ .build();
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
index 2c42d0a..1e4acef 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
@@ -1091,6 +1091,22 @@
}
@Test
+ public void commentContextIsEmptyForPatchsetLevelComments() throws Exception {
+ PushOneCommit.Result result = createChange();
+ String changeId = result.getChangeId();
+ String ps1 = result.getCommit().name();
+
+ CommentInput comment = newCommentWithOnlyMandatoryFields(PATCHSET_LEVEL, "comment");
+ addComments(changeId, ps1, comment);
+
+ List<CommentInfo> comments =
+ gApi.changes().id(changeId).commentsRequest().withContext(true).getAsList();
+
+ assertThat(comments).hasSize(1);
+ assertThat(comments.get(0).contextLines).isEmpty();
+ }
+
+ @Test
public void commentContextForCommentsOnDifferentPatchsets() throws Exception {
PushOneCommit.Result r1 = createChange();
diff --git a/javatests/com/google/gerrit/acceptance/tests.bzl b/javatests/com/google/gerrit/acceptance/tests.bzl
index 08556a0..70c6e94 100644
--- a/javatests/com/google/gerrit/acceptance/tests.bzl
+++ b/javatests/com/google/gerrit/acceptance/tests.bzl
@@ -4,7 +4,7 @@
group,
deps = [],
labels = [],
- vm_args = ["-Xmx256m"],
+ vm_args = ["-Xmx512m"],
**kwargs):
junit_tests(
name = group,
diff --git a/polygerrit-ui/app/BUILD b/polygerrit-ui/app/BUILD
index c29663c..2f83182 100644
--- a/polygerrit-ui/app/BUILD
+++ b/polygerrit-ui/app/BUILD
@@ -6,6 +6,7 @@
# This list must be in sync with the "include" list in the follwoing files:
# tsconfig.json, tsconfig_bazel.json, tsconfig_bazel_test.json
src_dirs = [
+ "api",
"constants",
"elements",
"embed",
diff --git a/polygerrit-ui/app/api/README.md b/polygerrit-ui/app/api/README.md
new file mode 100644
index 0000000..550063f
--- /dev/null
+++ b/polygerrit-ui/app/api/README.md
@@ -0,0 +1,23 @@
+# API
+
+In this folder, we declare the API of various parts of the Gerrit webclient.
+There are two primary use cases for this:
+
+* apps that embed our diff viewer, gr-diff
+* Gerrit plugins that need to access some part of Gerrit to extend it
+
+Both may be built as a separate bundle, but would like to type check against
+the same types the Gerrit/gr-diff bundle uses. For this reason, this folder
+should contain only types, with the exception of enums, where having the
+value side is deemed an acceptable duplication.
+
+All types in here should use the `declare` keyword to prevent bundlers from
+renaming fields, which would break communication across separately built
+bundles. Again enums are the exception, because their keys are not referenced
+across bundles, and values will not be renamed by bundlers as they are strings.
+
+This API is used by other apps embedding gr-diff and any breaking changes
+should be discussed with the Gerrit core team and properly versioned.
+
+Gerrit types should either directly use or extend these types, so that
+breaking changes to the implementation require changes to these files.
diff --git a/polygerrit-ui/app/api/core.ts b/polygerrit-ui/app/api/core.ts
new file mode 100644
index 0000000..f582915
--- /dev/null
+++ b/polygerrit-ui/app/api/core.ts
@@ -0,0 +1,22 @@
+/**
+ * @fileoverview Core API types for Gerrit.
+ *
+ * Core types are types used in many places in Gerrit, such as the Side enum.
+ *
+ * @license
+ * Copyright (C) 2020 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.
+ */
+
+// TODO(oler): Copy/move types here.
diff --git a/polygerrit-ui/app/types/diff.d.ts b/polygerrit-ui/app/api/diff.ts
similarity index 70%
rename from polygerrit-ui/app/types/diff.d.ts
rename to polygerrit-ui/app/api/diff.ts
index ed03dcf..59e4d42 100644
--- a/polygerrit-ui/app/types/diff.d.ts
+++ b/polygerrit-ui/app/api/diff.ts
@@ -1,16 +1,8 @@
/**
- * @fileoverview The Gerrit diff API.
+ * @fileoverview The API of Gerrit's diff viewer, gr-diff.
*
- * This API is used by other apps embedding gr-diff and any breaking changes
- * should be discussed with the Gerrit core team and properly versioned.
- *
- * Should only contain types, no values, so that other apps using gr-diff can
- * use this solely to type check and generate externs for their separate ts
- * bundles.
- *
- * Should declare all types, to avoid renaming breaking multi-bundle setups.
- *
- * Enums should be converted to union types to avoid values in this file.
+ * This includes some types which are also defined as part of Gerrit's JSON API
+ * which are used as inputs to gr-diff.
*
* @license
* Copyright (C) 2020 The Android Open Source Project
@@ -29,6 +21,15 @@
*/
/**
+ * Diff type in preferences
+ * https://gerrit-review.googlesource.com/Documentation/rest-api-accounts.html#preferences-input
+ */
+export enum DiffViewMode {
+ SIDE_BY_SIDE = 'SIDE_BY_SIDE',
+ UNIFIED = 'UNIFIED_DIFF',
+}
+
+/**
* The DiffInfo entity contains information about the diff of a file in a
* revision.
*
@@ -43,15 +44,8 @@
change_type: ChangeType;
/** Intraline status (OK, ERROR, TIMEOUT). */
intraline_status: 'OK' | 'Error' | 'Timeout';
- /** A list of strings representing the patch set diff header. */
- diff_header: string[];
/** The content differences in the file as a list of DiffContent entities. */
content: DiffContent[];
- /**
- * Links to the file diff in external sites as a list of DiffWebLinkInfo
- * entries.
- */
- web_links?: DiffWebLinkInfo[];
/** Whether the file is binary. */
binary?: boolean;
}
@@ -67,8 +61,6 @@
content_type: string;
/** The total number of lines in the file. */
lines: number;
- /** Links to the file in external sites as a list of WebLinkInfo entries. */
- web_links: WebLinkInfo[];
// TODO: Not documented.
language?: string;
}
@@ -124,8 +116,6 @@
* whitespace. When present and true a and b are used instead of ab.
*/
common?: boolean;
- // TODO: Undocumented, but used in code.
- keyLocation?: boolean;
}
/**
@@ -145,23 +135,6 @@
}
/**
- * The DiffWebLinkInfo entity describes a link on a diff screen to an external
- * site.
- * https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#diff-web-link-info
- */
-export declare interface DiffWebLinkInfo {
- /** The link name. */
- name: string;
- /** The link URL. */
- url: string;
- /** URL to the icon of the link. */
- image_url: string;
- // TODO: Are these really of type string? Not able to trigger them, but the
- // docs sound more like boolean.
- show_on_side_by_side_diff_view: string;
- show_on_unified_diff_view: string;
-}
-/**
* The DiffIntralineInfo entity contains information about intraline edits in a
* file.
*
@@ -180,56 +153,29 @@
export declare type DiffIntralineInfo = [SkipLength, MarkLength];
/**
- * The WebLinkInfo entity describes a link to an external site.
- * https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#web-link-info
- */
-export declare interface WebLinkInfo {
- /** The link name. */
- name: string;
- /** The link URL. */
- url: string;
- /** URL to the icon of the link. */
- image_url: string;
-}
-
-/**
* The DiffPreferencesInfo entity contains information about the diff
* preferences of a user.
* https://gerrit-review.googlesource.com/Documentation/rest-api-accounts.html#diff-preferences-info
*/
export declare interface DiffPreferencesInfo {
context: number;
- expand_all_comments?: boolean;
ignore_whitespace: IgnoreWhitespaceType;
intraline_difference?: boolean;
line_length: number;
- cursor_blink_rate: number;
- manual_review?: boolean;
- retain_header?: boolean;
show_line_endings?: boolean;
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;
- hide_empty_pane?: boolean;
- match_brackets?: boolean;
- line_wrapping?: boolean;
- // TODO(TS): show_file_comment_button exists in JS code, but doesn't exist in
- // the doc. Either remove or update doc
+ // TODO: Missing documentation
show_file_comment_button?: boolean;
- // TODO(TS): theme exists in JS code, but doesn't exist in the doc.
- // Either remove or update doc
+ // TODO: Missing documentation
theme?: string;
}
-export declare type DiffPreferencesInfoKey = keyof DiffPreferencesInfo;
-
/**
* Whether whitespace changes should be ignored and if yes, which whitespace
* changes should be ignored
@@ -240,3 +186,39 @@
| 'IGNORE_TRAILING'
| 'IGNORE_LEADING_AND_TRAILING'
| 'IGNORE_ALL';
+
+export enum Side {
+ LEFT = 'left',
+ RIGHT = 'right',
+}
+
+export enum CoverageType {
+ /**
+ * start_character and end_character of the range will be ignored for this
+ * type.
+ */
+ COVERED = 'COVERED',
+ /**
+ * start_character and end_character of the range will be ignored for this
+ * type.
+ */
+ NOT_COVERED = 'NOT_COVERED',
+ PARTIALLY_COVERED = 'PARTIALLY_COVERED',
+ /**
+ * You don't have to use this. If there is no coverage information for a
+ * range, then it implicitly means NOT_INSTRUMENTED. start_character and
+ * end_character of the range will be ignored for this type.
+ */
+ NOT_INSTRUMENTED = 'NOT_INSTRUMENTED',
+}
+
+export declare interface LineRange {
+ start_line: number;
+ end_line: number;
+}
+
+export interface CoverageRange {
+ type: CoverageType;
+ side: Side;
+ code_range: LineRange;
+}
diff --git a/polygerrit-ui/app/constants/constants.ts b/polygerrit-ui/app/constants/constants.ts
index 50be7ee..b0f87d7 100644
--- a/polygerrit-ui/app/constants/constants.ts
+++ b/polygerrit-ui/app/constants/constants.ts
@@ -18,6 +18,7 @@
/**
* @desc Tab names for primary tabs on change view page.
*/
+import {DiffViewMode} from '../api/diff';
import {DiffPreferencesInfo} from '../types/diff';
import {EditPreferencesInfo, PreferencesInfo} from '../types/common';
@@ -160,10 +161,7 @@
HIDDEN = 'HIDDEN',
}
-export enum Side {
- LEFT = 'left',
- RIGHT = 'right',
-}
+export {Side} from '../api/diff';
/**
* The type in ConfigParameterInfo entity.
@@ -291,14 +289,7 @@
HHMM_24 = 'HHMM_24',
}
-/**
- * Diff type in preferences
- * https://gerrit-review.googlesource.com/Documentation/rest-api-accounts.html#preferences-input
- */
-export enum DiffViewMode {
- SIDE_BY_SIDE = 'SIDE_BY_SIDE',
- UNIFIED = 'UNIFIED_DIFF',
-}
+export {DiffViewMode};
/**
* The type of email strategy to use.
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts
index c45e801..9ad0b05f 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts
@@ -395,6 +395,7 @@
e.preventDefault();
this.$.cursor.next();
+ this.selectedIndex = this.$.cursor.index;
}
_prevChange(e: CustomKeyboardEvent) {
@@ -404,6 +405,7 @@
e.preventDefault();
this.$.cursor.previous();
+ this.selectedIndex = this.$.cursor.index;
}
_openChange(e: CustomKeyboardEvent) {
@@ -518,6 +520,8 @@
afterNextRender(this, () => {
this.$.cursor.stops = this._getListItems();
this.$.cursor.moveToStart();
+ if (this.selectedIndex)
+ this.$.cursor.setCursorAtIndex(this.selectedIndex);
});
}
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_html.ts b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_html.ts
index 990525c..75ed783 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_html.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_html.ts
@@ -159,7 +159,6 @@
</table>
<gr-cursor-manager
id="cursor"
- index="{{selectedIndex}}"
scroll-mode="keep-visible"
focus-on-move=""
></gr-cursor-manager>
diff --git a/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view.ts b/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view.ts
index e40f6f4..7008b48 100644
--- a/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view.ts
@@ -117,6 +117,9 @@
@property({type: Boolean})
_showNewUserHelp = false;
+ @property({type: Number})
+ _selectedChangeIndex?: number;
+
private reporting = appContext.reportingService;
private restApiService = appContext.restApiService;
@@ -204,12 +207,21 @@
return params.view === GerritView.DASHBOARD;
}
+ @observe('_selectedChangeIndex')
+ _selectedChangeIndexChanged(selectedChangeIndex: number) {
+ if (!this.params || !this._isViewActive(this.params)) return;
+ if (!this.viewState) throw new Error('view state undefined');
+ if (!this.params.user) throw new Error('user for dashboard is undefined');
+ this.viewState[this.params.user] = selectedChangeIndex;
+ }
+
@observe('params.*')
_paramsChanged(
paramsChangeRecord: ElementPropertyDeepChange<GrDashboardView, 'params'>
) {
const params = paramsChangeRecord.base;
-
+ if (params && this._isViewActive(params) && params.user && this.viewState)
+ this._selectedChangeIndex = this.viewState[params.user] || 0;
return this._reload(params);
}
diff --git a/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view_html.ts b/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view_html.ts
index c23a1cc..b28dfef 100644
--- a/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view_html.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view_html.ts
@@ -84,7 +84,7 @@
show-reviewed-state=""
account="[[account]]"
preferences="[[preferences]]"
- selected-index="{{viewState.selectedChangeIndex}}"
+ selected-index="{{_selectedChangeIndex}}"
sections="[[_results]]"
on-toggle-star="_handleToggleStar"
on-toggle-reviewed="_handleToggleReviewed"
diff --git a/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view_test.js b/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view_test.js
index 2fd2232..47f885b 100644
--- a/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view_test.js
+++ b/polygerrit-ui/app/elements/change-list/gr-dashboard-view/gr-dashboard-view_test.js
@@ -402,5 +402,26 @@
await paramsChangedPromise;
assert.isTrue(element.reporting.dashboardDisplayed.calledOnce);
});
+
+ test('selectedChangeIndex is derived from the params', () => {
+ stubRestApi('getDashboard').returns(Promise.resolve({
+ title: 'title',
+ sections: [],
+ }));
+ element.viewState = {
+ 101001: 23,
+ };
+ element.params = {
+ view: GerritNav.View.DASHBOARD,
+ project: 'project',
+ dashboard: 'dashboard',
+ user: '101001',
+ };
+ flush();
+ sinon.stub(element.reporting, 'dashboardDisplayed');
+ paramsChangedPromise.then(() => {
+ assert.equal(element._selectedChangeIndex, 23);
+ });
+ });
});
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
index daa7274..bd2f105 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
@@ -1605,12 +1605,12 @@
// TODO(rmistry): Redo this after
// https://bugs.chromium.org/p/gerrit/issues/detail?id=4671 is resolved.
- _setLabelValuesOnRevert(newChangeId: NumericChangeId) {
- const labels = this.$.jsAPI.getLabelValuesPostRevert(this.change);
- if (!labels) {
+ _setReviewOnRevert(newChangeId: NumericChangeId) {
+ const review = this.$.jsAPI.getReviewPostRevert(this.change);
+ if (!review) {
return Promise.resolve(undefined);
}
- return this.restApiService.saveChangeReview(newChangeId, CURRENT, {labels});
+ return this.restApiService.saveChangeReview(newChangeId, CURRENT, review);
}
_handleResponse(action: UIActionInfo, response?: Response) {
@@ -1622,7 +1622,7 @@
case ChangeActions.REVERT: {
const revertChangeInfo: ChangeInfo = (obj as unknown) as ChangeInfo;
this._waitForChangeReachable(revertChangeInfo._number)
- .then(() => this._setLabelValuesOnRevert(revertChangeInfo._number))
+ .then(() => this._setReviewOnRevert(revertChangeInfo._number))
.then(() => {
GerritNav.navigateToChange(revertChangeInfo);
});
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.js b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.js
index 0641182..b58519b 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.js
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.js
@@ -464,16 +464,16 @@
assert.isFalse(element.$.mainContent.classList.contains('overlayOpen'));
});
- test('_setLabelValuesOnRevert', () => {
- const labels = {'Foo': 1, 'Bar-Baz': -2};
+ test('_setReviewOnRevert', () => {
+ const review = {labels: {'Foo': 1, 'Bar-Baz': -2}};
const changeId = 1234;
- sinon.stub(element.$.jsAPI, 'getLabelValuesPostRevert').returns(labels);
+ sinon.stub(element.$.jsAPI, 'getReviewPostRevert').returns(review);
const saveStub = stubRestApi('saveChangeReview')
.returns(Promise.resolve());
- return element._setLabelValuesOnRevert(changeId).then(() => {
+ return element._setReviewOnRevert(changeId).then(() => {
assert.isTrue(saveStub.calledOnce);
assert.equal(saveStub.lastCall.args[0], changeId);
- assert.deepEqual(saveStub.lastCall.args[2], {labels});
+ assert.deepEqual(saveStub.lastCall.args[2], review);
});
});
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 8657e0c..1e3dfff 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
@@ -60,7 +60,9 @@
import {getPluginEndpoints} from '../../shared/gr-js-api-interface/gr-plugin-endpoints';
import {getPluginLoader} from '../../shared/gr-js-api-interface/gr-plugin-loader';
import {RevisionInfo as RevisionInfoClass} from '../../shared/revision-info/revision-info';
+import {DiffViewMode} from '../../../api/diff';
import {PrimaryTab, SecondaryTab} from '../../../constants/constants';
+
import {NO_ROBOT_COMMENTS_THREADS_MSG} from '../../../constants/messages';
import {appContext} from '../../../services/app-context';
import {ChangeStatus} from '../../../constants/constants';
@@ -192,11 +194,6 @@
NEW_MESSAGE: 'There are new messages on this change',
};
-enum DiffViewMode {
- SIDE_BY_SIDE = 'SIDE_BY_SIDE',
- UNIFIED = 'UNIFIED_DIFF',
-}
-
const CHANGE_DATA_TIMING_LABEL = 'ChangeDataLoaded';
const CHANGE_RELOAD_TIMING_LABEL = 'ChangeReloaded';
const SEND_REPLY_TIMING_LABEL = 'SendReply';
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-results.ts b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
index c2381bc..f27c2ee 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-results.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
@@ -15,7 +15,7 @@
* limitations under the License.
*/
import {html} from 'lit-html';
-import {css, customElement, property} from 'lit-element';
+import {css, customElement, property, PropertyValues} from 'lit-element';
import {GrLitElement} from '../lit/gr-lit-element';
import {
Category,
@@ -47,12 +47,23 @@
@property()
result?: RunResult;
+ @property()
+ isExpanded = false;
+
+ @property({type: Boolean, reflect: true})
+ isExpandable = false;
+
static get styles() {
return [
sharedStyles,
css`
:host {
- display: table-row;
+ display: contents;
+ }
+ :host([isexpandable]) {
+ cursor: pointer;
+ }
+ tr {
border-top: 1px solid var(--border-color);
}
iron-icon.launch {
@@ -123,39 +134,71 @@
];
}
+ update(changedProperties: PropertyValues) {
+ if (changedProperties.has('result')) {
+ this.isExpandable = !!this.result?.message;
+ }
+ super.update(changedProperties);
+ }
+
render() {
if (!this.result) return '';
return html`
- <td class="iconCol">
- <div>${this.renderIcon()}</div>
- </td>
- <td class="nameCol">
- <div><span>${this.result.checkName}</span></div>
- </td>
- <td class="summaryCol">
- <div class="summary-cell">
- ${(this.result.links ?? []).map(this.renderLink)}
- <!-- The is for being able to shrink a tiny amount without
- the text itself getting shrunk with an ellipsis. -->
- <div class="summary">${this.result.summary} </div>
- <div class="message">${this.result.message}</div>
- <div class="tags">
- ${(this.result.tags ?? []).map(t => this.renderTag(t))}
+ <tr class="container" @click="${this.toggleExpanded}">
+ <td class="iconCol">
+ <div>${this.renderIcon()}</div>
+ </td>
+ <td class="nameCol">
+ <div><span>${this.result.checkName}</span></div>
+ </td>
+ <td class="summaryCol">
+ <div class="summary-cell">
+ ${(this.result.links ?? []).map(this.renderLink)}
+ <!-- The is for being able to shrink a tiny amount without
+ the text itself getting shrunk with an ellipsis. -->
+ <div class="summary">${this.result.summary} </div>
+ <div class="message">
+ ${this.isExpanded ? '' : this.result.message}
+ </div>
+ <div class="tags">
+ ${(this.result.tags ?? []).map(t => this.renderTag(t))}
+ </div>
+ ${this.renderLabel()}
</div>
- ${this.renderLabel()}
- </div>
- </td>
- <td class="expanderCol">
- <div>
- <iron-icon
- aria-label="expand result row"
- icon="gr-icons:expand-more"
- ></iron-icon>
- </div>
- </td>
+ <gr-result-expanded
+ .result="${this.result}"
+ ?hidden="${!this.isExpanded}"
+ ></gr-result-expanded>
+ </td>
+ <td class="expanderCol">
+ <div
+ class="show-hide"
+ role="switch"
+ tabindex="0"
+ ?hidden="${!this.isExpandable}"
+ ?aria-checked="${this.isExpanded}"
+ aria-label="${this.isExpanded
+ ? 'Collapse result row'
+ : 'Expand result row'}"
+ @click="${this.toggleExpanded}"
+ @keydown="${this.toggleExpanded}"
+ >
+ <iron-icon
+ icon="${this.isExpanded
+ ? 'gr-icons:expand-less'
+ : 'gr-icons:expand-more'}"
+ ></iron-icon>
+ </div>
+ </td>
+ </tr>
`;
}
+ private toggleExpanded() {
+ if (!this.isExpandable) return;
+ this.isExpanded = !this.isExpanded;
+ }
+
renderLink(link: Link) {
return html`
<a href="${link.url}" target="_blank">
@@ -184,6 +227,32 @@
}
}
+@customElement('gr-result-expanded')
+class GrResultExpanded extends GrLitElement {
+ @property()
+ result?: RunResult;
+
+ static get styles() {
+ return [
+ sharedStyles,
+ css`
+ .message {
+ padding: var(--spacing-m) var(--spacing-m) var(--spacing-m) 0;
+ }
+ `,
+ ];
+ }
+
+ render() {
+ if (!this.result) return '';
+ return html`
+ <div class="message">
+ ${this.result.message}
+ </div>
+ `;
+ }
+}
+
@customElement('gr-checks-results')
export class GrChecksResults extends GrLitElement {
@property()
@@ -315,6 +384,7 @@
declare global {
interface HTMLElementTagNameMap {
'gr-result-row': GrResultRow;
+ 'gr-result-expanded': GrResultExpanded;
'gr-checks-results': GrChecksResults;
}
}
diff --git a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.ts b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.ts
index 2482497..c91c82e 100644
--- a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.ts
+++ b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header.ts
@@ -156,6 +156,9 @@
@property({type: String})
_registerURL = '';
+ @property({type: String})
+ _feedbackURL = '';
+
@property({type: Boolean})
mobileSearchHidden = false;
@@ -307,6 +310,7 @@
if (!config) {
throw new Error('getConfig returned undefined');
}
+ this._retreiveFeedbackURL(config);
this._retrieveRegisterURL(config);
return getDocsBaseUrl(config, this.restApiService);
})
@@ -327,6 +331,12 @@
});
}
+ _retreiveFeedbackURL(config: ServerInfo) {
+ if (config.gerrit?.report_bug_url) {
+ this._feedbackURL = config.gerrit.report_bug_url;
+ }
+ }
+
_retrieveRegisterURL(config: ServerInfo) {
if (AUTH_TYPES_WITH_REGISTER_URL.has(config.auth.auth_type)) {
this._registerURL = config.auth.register_url ?? '';
diff --git a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_html.ts b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_html.ts
index 0cdd0f2..28bb090 100644
--- a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_html.ts
+++ b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_html.ts
@@ -206,6 +206,17 @@
class="hideOnMobile"
name="header-browse-source"
></gr-endpoint-decorator>
+ <template is="dom-if" if="[[_feedbackURL]]">
+ <a class="feedbackButton"
+ href$="[[_feedbackURL]]"
+ title="File a bug"
+ aria-label="File a bug"
+ role="button"
+ >
+ <iron-icon icon="gr-icons:bug"></iron-icon>
+ </a>
+ </template>
+ </div>
<div class="accountContainer" id="accountContainer">
<iron-icon
id="mobileSearch"
diff --git a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_test.ts b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_test.ts
index b13e2e6..7430cc0 100644
--- a/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_test.ts
+++ b/polygerrit-ui/app/elements/core/gr-main-header/gr-main-header_test.ts
@@ -21,6 +21,7 @@
import {GrMainHeader} from './gr-main-header';
import {
createAccountDetailWithId,
+ createGerritInfo,
createServerInfo,
} from '../../../test/test-data-generators';
import {NavLink} from '../../../utils/admin-nav-util';
@@ -475,6 +476,25 @@
);
});
+ test('shows feedback icon when URL provided', async () => {
+ assert.isEmpty(element._feedbackURL);
+ assert.isNull(query(element, '.feedbackButton'));
+
+ const url = 'report_bug_url';
+ const config: ServerInfo = {
+ ...createServerInfo(),
+ gerrit: {
+ ...createGerritInfo(),
+ report_bug_url: url,
+ },
+ };
+ element._retreiveFeedbackURL(config);
+ await flush();
+
+ assert.equal(element._feedbackURL, url);
+ assert.ok(query(element, '.feedbackButton'));
+ });
+
test('register URL', () => {
assert.isTrue(isHidden(query(element, '.registerDiv')));
const config: ServerInfo = {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element.ts b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element.ts
index 6e613d3..3298745 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element.ts
@@ -43,6 +43,7 @@
GrRangedCommentLayer,
} from '../gr-ranged-comment-layer/gr-ranged-comment-layer';
import {GrCoverageLayer} from '../gr-coverage-layer/gr-coverage-layer';
+import {DiffViewMode} from '../../../api/diff';
import {Side} from '../../../constants/constants';
import {GrDiffLine, LineNumber} from '../gr-diff/gr-diff-line';
import {GrDiffGroup} from '../gr-diff/gr-diff-group';
@@ -50,11 +51,6 @@
import {getLineNumber} from '../gr-diff/gr-diff-utils';
import {fireAlert, fireEvent} from '../../../utils/event-util';
-const DiffViewMode = {
- SIDE_BY_SIDE: 'SIDE_BY_SIDE',
- UNIFIED: 'UNIFIED_DIFF',
-};
-
const TRAILING_WHITESPACE_PATTERN = /\s+$/;
// https://gerrit.googlesource.com/gerrit/+/234616a8627334686769f1de989d286039f4d6a5/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.js#740
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element_test.js b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element_test.js
index 532922c..4dbac66 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder-element_test.js
@@ -28,6 +28,7 @@
import {GrDiffBuilder} from './gr-diff-builder.js';
import {GrDiffBuilderSideBySide} from './gr-diff-builder-side-by-side.js';
import {html} from '@polymer/polymer/lib/utils/html-tag.js';
+import {DiffViewMode} from '../../../api/diff.js';
import {stubRestApi} from '../../../test/test-utils.js';
const basicFixture = fixtureFromTemplate(html`
@@ -46,11 +47,6 @@
</gr-diff-builder>
`);
-const DiffViewMode = {
- SIDE_BY_SIDE: 'SIDE_BY_SIDE',
- UNIFIED: 'UNIFIED_DIFF',
-};
-
suite('gr-diff-builder tests', () => {
let prefs;
let element;
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.ts b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.ts
index 52ac7cc..74c339a 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-cursor/gr-diff-cursor.ts
@@ -29,6 +29,7 @@
import {LegacyElementMixin} from '@polymer/polymer/lib/legacy/legacy-element-mixin';
import {PolymerElement} from '@polymer/polymer/polymer-element';
import {htmlTemplate} from './gr-diff-cursor_html';
+import {DiffViewMode} from '../../../api/diff';
import {ScrollMode, Side} from '../../../constants/constants';
import {customElement, property, observe} from '@polymer/decorators';
import {GrDiffLineType} from '../gr-diff/gr-diff-line';
@@ -38,11 +39,6 @@
import {GrDiff} from '../gr-diff/gr-diff';
import {fireAlert, fireEvent} from '../../../utils/event-util';
-const DiffViewMode = {
- SIDE_BY_SIDE: 'SIDE_BY_SIDE',
- UNIFIED: 'UNIFIED_DIFF',
-};
-
type GrDiffRowType = GrDiffLineType | GrDiffGroupType;
const LEFT_SIDE_CLASS = 'target-side-left';
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
index 58c40e5..7610e5e 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
@@ -472,13 +472,13 @@
this.projectName,
this.commitRange.baseCommit,
this.path,
- {weblinks: diff && diff.meta_a && diff.meta_a.web_links}
+ {weblinks: diff?.meta_a?.web_links}
),
meta_b: GerritNav.getFileWebLinks(
this.projectName,
this.commitRange.commit,
this.path,
- {weblinks: diff && diff.meta_b && diff.meta_b.web_links}
+ {weblinks: diff?.meta_b?.web_links}
),
};
}
diff --git a/polygerrit-ui/app/elements/gr-app-element.ts b/polygerrit-ui/app/elements/gr-app-element.ts
index f6477a9..15e8923 100644
--- a/polygerrit-ui/app/elements/gr-app-element.ts
+++ b/polygerrit-ui/app/elements/gr-app-element.ts
@@ -277,9 +277,7 @@
offset: 0,
selectedChangeIndex: 0,
},
- dashboardView: {
- selectedChangeIndex: 0,
- },
+ dashboardView: {},
};
}
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts
index 5658f13..1d54896 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface-element.ts
@@ -20,9 +20,11 @@
import {getPluginLoader} from './gr-plugin-loader';
import {customElement} from '@polymer/decorators';
+import {hasOwnProperty} from '../../../utils/common-util';
import {
ChangeInfo,
LabelNameToValuesMap,
+ ReviewInput,
RevisionInfo,
} from '../../../types/common';
import {GrAnnotationActionsInterface} from './gr-annotation-actions-js-api';
@@ -307,16 +309,21 @@
return links;
}
- getLabelValuesPostRevert(change?: ChangeInfo): LabelNameToValuesMap {
- let labels: LabelNameToValuesMap = {};
+ getReviewPostRevert(change?: ChangeInfo): ReviewInput {
+ let review: ReviewInput = {};
for (const cb of this._getEventCallbacks(EventType.POST_REVERT)) {
try {
- labels = cb(change);
+ const r = cb(change);
+ if (hasOwnProperty(r, 'labels')) {
+ review = r as ReviewInput;
+ } else {
+ review = {labels: r as LabelNameToValuesMap};
+ }
} catch (err) {
this.reporting.error(err);
}
}
- return labels;
+ return review;
}
_getEventCallbacks(type: EventType) {
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface_test.js b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface_test.js
index ce57f92..d5d734a 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface_test.js
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface_test.js
@@ -283,18 +283,33 @@
assert.isTrue(errorStub.calledTwice);
});
- test('postrevert event', () => {
+ test('postrevert event labels', () => {
function getLabels(c) {
return {'Code-Review': 1};
}
- assert.deepEqual(element.getLabelValuesPostRevert(null), {});
+ assert.deepEqual(element.getReviewPostRevert(null), {});
assert.equal(errorStub.callCount, 0);
plugin.on(EventType.POST_REVERT, throwErrFn);
plugin.on(EventType.POST_REVERT, getLabels);
assert.deepEqual(
- element.getLabelValuesPostRevert(null), {'Code-Review': 1});
+ element.getReviewPostRevert(null), {labels: {'Code-Review': 1}});
+ assert.isTrue(errorStub.calledOnce);
+ });
+
+ test('postrevert event review', () => {
+ function getReview(c) {
+ return {labels: {'Code-Review': 1}};
+ }
+
+ assert.deepEqual(element.getReviewPostRevert(null), {});
+ assert.equal(errorStub.callCount, 0);
+
+ plugin.on(EventType.POST_REVERT, throwErrFn);
+ plugin.on(EventType.POST_REVERT, getReview);
+ assert.deepEqual(
+ element.getReviewPostRevert(null), {labels: {'Code-Review': 1}});
assert.isTrue(errorStub.calledOnce);
});
diff --git a/polygerrit-ui/app/tsconfig.json b/polygerrit-ui/app/tsconfig.json
index 15294f4..0d751ab 100644
--- a/polygerrit-ui/app/tsconfig.json
+++ b/polygerrit-ui/app/tsconfig.json
@@ -49,6 +49,7 @@
// Items below must be in sync with the src_dirs list in the BUILD file
// Also items must be in sync with tsconfig_bazel.json, tsconfig_bazel_test.json
// (include and exclude arrays are overriden when extends)
+ "api/**/*",
"constants/**/*",
"elements/**/*",
"embed/**/*",
diff --git a/polygerrit-ui/app/tsconfig_bazel.json b/polygerrit-ui/app/tsconfig_bazel.json
index 6365bf0..46bd926 100644
--- a/polygerrit-ui/app/tsconfig_bazel.json
+++ b/polygerrit-ui/app/tsconfig_bazel.json
@@ -10,6 +10,7 @@
// Items below must be in sync with the src_dirs list in the BUILD file
// Also items must be in sync with tsconfig.json, tsconfig_bazel_test.json
// (include and exclude arrays are overriden when extends)
+ "api/**/*",
"constants/**/*",
"elements/**/*",
"embed/**/*",
diff --git a/polygerrit-ui/app/tsconfig_bazel_test.json b/polygerrit-ui/app/tsconfig_bazel_test.json
index efd2978..1b6c226 100644
--- a/polygerrit-ui/app/tsconfig_bazel_test.json
+++ b/polygerrit-ui/app/tsconfig_bazel_test.json
@@ -15,6 +15,7 @@
// Items below must be in sync with the src_dirs list in the BUILD file
// Also items must be in sync with tsconfig.json, tsconfig_test.json
// (include and exclude arrays are overriden when extends)
+ "api/**/*",
"constants/**/*",
"elements/**/*",
"embed/**/*",
diff --git a/polygerrit-ui/app/types/diff.ts b/polygerrit-ui/app/types/diff.ts
new file mode 100644
index 0000000..c3b38c6
--- /dev/null
+++ b/polygerrit-ui/app/types/diff.ts
@@ -0,0 +1,110 @@
+/**
+ * @fileoverview Types related to diffing.
+ *
+ * As gr-diff is an embeddable component, many of these types are actually
+ * defined in api/diff.ts. This file re-exports them and adds any
+ * internal fields that Gerrit may use.
+ *
+ * @license
+ * Copyright (C) 2020 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.
+ */
+
+export {
+ ChangeType,
+ MoveDetails,
+ SkipLength,
+ MarkLength,
+ DiffIntralineInfo,
+ IgnoreWhitespaceType,
+} from '../api/diff';
+
+import {
+ DiffInfo as DiffInfoApi,
+ DiffFileMetaInfo as DiffFileMetaInfoApi,
+ DiffContent as DiffContentApi,
+ DiffPreferencesInfo as DiffPreferenceInfoApi,
+} from '../api/diff';
+
+export interface DiffInfo extends DiffInfoApi {
+ /** Meta information about the file on side A as a DiffFileMetaInfo entity. */
+ meta_a: DiffFileMetaInfo;
+ /** Meta information about the file on side B as a DiffFileMetaInfo entity. */
+ meta_b: DiffFileMetaInfo;
+
+ /** A list of strings representing the patch set diff header. */
+ diff_header?: string[];
+
+ /**
+ * Links to the file diff in external sites as a list of DiffWebLinkInfo
+ * entries.
+ */
+ web_links?: DiffWebLinkInfo[];
+}
+
+/**
+ * The DiffWebLinkInfo entity describes a link on a diff screen to an external
+ * site.
+ * https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#diff-web-link-info
+ */
+export declare interface DiffWebLinkInfo {
+ /** The link name. */
+ name: string;
+ /** The link URL. */
+ url: string;
+ /** URL to the icon of the link. */
+ image_url: string;
+ // TODO: Are these really of type string? Not able to trigger them, but the
+ // docs sound more like boolean.
+ show_on_side_by_side_diff_view: string;
+ show_on_unified_diff_view: string;
+}
+
+export interface DiffFileMetaInfo extends DiffFileMetaInfoApi {
+ /** Links to the file in external sites as a list of WebLinkInfo entries. */
+ web_links?: WebLinkInfo[];
+}
+
+/**
+ * The WebLinkInfo entity describes a link to an external site.
+ * https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#web-link-info
+ */
+export declare interface WebLinkInfo {
+ /** The link name. */
+ name: string;
+ /** The link URL. */
+ url: string;
+ /** URL to the icon of the link. */
+ image_url: string;
+}
+
+export interface DiffContent extends DiffContentApi {
+ // TODO: Undocumented, but used in code.
+ keyLocation?: boolean;
+}
+
+export interface DiffPreferencesInfo extends DiffPreferenceInfoApi {
+ expand_all_comments?: boolean;
+ cursor_blink_rate: number;
+ manual_review?: boolean;
+ retain_header?: boolean;
+ skip_deleted?: boolean;
+ hide_top_menu?: boolean;
+ hide_line_numbers?: boolean;
+ hide_empty_pane?: boolean;
+ match_brackets?: boolean;
+ line_wrapping?: boolean;
+}
+
+export declare type DiffPreferencesInfoKey = keyof DiffPreferencesInfo;
diff --git a/polygerrit-ui/app/types/types.ts b/polygerrit-ui/app/types/types.ts
index a838f8f..a7e654c 100644
--- a/polygerrit-ui/app/types/types.ts
+++ b/polygerrit-ui/app/types/types.ts
@@ -48,31 +48,7 @@
commit: CommitId;
}
-export interface CoverageRange {
- type: CoverageType;
- side: Side;
- code_range: {end_line: number; start_line: number};
-}
-
-export enum CoverageType {
- /**
- * start_character and end_character of the range will be ignored for this
- * type.
- */
- COVERED = 'COVERED',
- /**
- * start_character and end_character of the range will be ignored for this
- * type.
- */
- NOT_COVERED = 'NOT_COVERED',
- PARTIALLY_COVERED = 'PARTIALLY_COVERED',
- /**
- * You don't have to use this. If there is no coverage information for a
- * range, then it implicitly means NOT_INSTRUMENTED. start_character and
- * end_character of the range will be ignored for this type.
- */
- NOT_INSTRUMENTED = 'NOT_INSTRUMENTED',
-}
+export {CoverageRange, CoverageType} from '../api/diff';
export enum ErrorType {
AUTH = 'AUTH',
@@ -208,7 +184,7 @@
}
export interface DashboardViewState {
- selectedChangeIndex: number;
+ [key: string]: number;
}
export interface ViewState {
diff --git a/polygerrit-ui/app/utils/date-util.ts b/polygerrit-ui/app/utils/date-util.ts
index 5101d11..fad5041 100644
--- a/polygerrit-ui/app/utils/date-util.ts
+++ b/polygerrit-ui/app/utils/date-util.ts
@@ -39,25 +39,25 @@
export function fromNow(date: Date, noAgo = false) {
const now = new Date();
const ago = noAgo ? '' : ' ago';
- const secondsAgo = Math.round((now.valueOf() - date.valueOf()) / 1000);
+ const secondsAgo = Math.floor((now.valueOf() - date.valueOf()) / 1000);
if (secondsAgo <= 59) return 'just now';
if (secondsAgo <= 119) return `1 minute${ago}`;
- const minutesAgo = Math.round(secondsAgo / 60);
+ const minutesAgo = Math.floor(secondsAgo / 60);
if (minutesAgo <= 59) return `${minutesAgo} minutes${ago}`;
if (minutesAgo === 60) return `1 hour${ago}`;
if (minutesAgo <= 119) return `1 hour ${minutesAgo - 60} min${ago}`;
- const hoursAgo = Math.round(minutesAgo / 60);
+ const hoursAgo = Math.floor(minutesAgo / 60);
if (hoursAgo <= 23) return `${hoursAgo} hours${ago}`;
if (hoursAgo === 24) return `1 day${ago}`;
if (hoursAgo <= 47) return `1 day ${hoursAgo - 24} hr${ago}`;
- const daysAgo = Math.round(hoursAgo / 24);
+ const daysAgo = Math.floor(hoursAgo / 24);
if (daysAgo <= 30) return `${daysAgo} days${ago}`;
if (daysAgo <= 60) return `1 month${ago}`;
- const monthsAgo = Math.round(daysAgo / 30);
+ const monthsAgo = Math.floor(daysAgo / 30);
if (monthsAgo <= 11) return `${monthsAgo} months${ago}`;
if (monthsAgo === 12) return `1 year${ago}`;
if (monthsAgo <= 24) return `1 year ${monthsAgo - 12} m${ago}`;
- const yearsAgo = Math.round(daysAgo / 365);
+ const yearsAgo = Math.floor(daysAgo / 365);
return `${yearsAgo} years${ago}`;
}
diff --git a/polygerrit-ui/app/utils/date-util_test.js b/polygerrit-ui/app/utils/date-util_test.js
index 76d8516..96d5bc1 100644
--- a/polygerrit-ui/app/utils/date-util_test.js
+++ b/polygerrit-ui/app/utils/date-util_test.js
@@ -53,6 +53,11 @@
assert.equal('1 year ago', fromNow(new Date('May 05 2019 12:00:00')));
assert.equal('10 years ago', fromNow(new Date('May 05 2010 12:00:00')));
});
+ test('rounding error', () => {
+ const fakeNow = new Date('May 08 2020 12:00:00');
+ sinon.useFakeTimers(fakeNow.getTime());
+ assert.equal('2 hours ago', fromNow(new Date('May 08 2020 9:30:00')));
+ });
});
suite('isWithinDay', () => {
