Merge "SubmitStrategy: Use directly SubmoduleCommit"
diff --git a/Documentation/pg-plugin-dev.txt b/Documentation/pg-plugin-dev.txt
index adb5d20..91bc476 100644
--- a/Documentation/pg-plugin-dev.txt
+++ b/Documentation/pg-plugin-dev.txt
@@ -375,93 +375,4 @@
Note: TODO
=== url
-`plugin.url(opt_path)`
-
-Note: TODO
-
-[[deprecated-api]]
-== Deprecated APIs
-
-Some of the deprecated APIs have limited implementation in PolyGerrit to serve
-as a "stepping stone" to allow gradual migration.
-
-=== install
-`plugin.deprecated.install()`
-
-.Params:
-- none
-
-Replaces plugin APIs with a deprecated version. This allows use of deprecated
-APIs without changing JS code. For example, `onAction` is not available by
-default, and after `plugin.deprecated.install()` it's accessible via
-`self.onAction()`.
-
-=== onAction
-`plugin.deprecated.onAction(type, view_name, callback)`
-
-.Params:
-- `*string* type` Action type.
-- `*string* view_name` REST API action.
-- `*function(actionContext)* callback` Callback invoked on action button click.
-
-Adds a button to the UI with a click callback. Exact button location depends on
-parameters. Callback is triggered with an instance of
-link:#deprecated-action-context[action context].
-
-Support is limited:
-
-- type is either `change` or `revision`.
-
-See link:js-api.html#self_onAction[self.onAction] for more info.
-
-=== panel
-`plugin.deprecated.panel(extensionpoint, callback)`
-
-.Params:
-- `*string* extensionpoint`
-- `*function(screenContext)* callback`
-
-Adds a UI DOM element and triggers a callback with context to allow direct DOM
-access.
-
-Support is limited:
-
-- extensionpoint is one of the following:
- * CHANGE_SCREEN_BELOW_COMMIT_INFO_BLOCK
- * CHANGE_SCREEN_BELOW_CHANGE_INFO_BLOCK
-
-See link:js-api.html#self_panel[self.panel] for more info.
-
-=== settingsScreen
-`plugin.deprecated.settingsScreent(path, menu, callback)`
-
-.Params:
-- `*string* path` URL path fragment of the screen for direct link.
-- `*string* menu` Menu item title.
-- `*function(settingsScreenContext)* callback`
-
-Adds a settings menu item and a section in the settings screen that is provided
-to plugin for setup.
-
-See link:js-api.html#self_settingsScreen[self.settingsScreen] for more info.
-
-[[deprecated-action-context]]
-=== Action Context (deprecated)
-Instance of Action Context is passed to `onAction()` callback.
-
-Support is limited:
-
-- `popup()`
-- `hide()`
-- `refresh()`
-- `textfield()`
-- `br()`
-- `msg()`
-- `div()`
-- `button()`
-- `checkbox()`
-- `label()`
-- `prependLabel()`
-- `call()`
-
-See link:js-api.html#ActionContext[Action Context] for more info.
+`plugin.url(opt_path)`
\ No newline at end of file
diff --git a/Documentation/pg-plugin-migration.txt b/Documentation/pg-plugin-migration.txt
index bca4b7a..061c687 100644
--- a/Documentation/pg-plugin-migration.txt
+++ b/Documentation/pg-plugin-migration.txt
@@ -79,9 +79,6 @@
<script>
Gerrit.install(plugin => {
// Setup block, is executed before sampleplugin.js
-
- // Install deprecated JS APIs (onAction, popup, etc)
- plugin.deprecated.install();
});
</script>
@@ -105,8 +102,6 @@
- `sampleplugin.js` is loaded since it's referenced in `sampleplugin.html`
- setup script tag code is executed before `sampleplugin.js`
- cleanup script tag code is executed after `sampleplugin.js`
-- `plugin.deprecated.install()` enables deprecated APIs (onAction(), popup(),
-etc) before `sampleplugin.js` is loaded
This means the plugin instance is shared between .html-based and .js-based
code. This allows to gradually and incrementally transfer code to the new API.
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index e09d936..f2f5c15 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -2039,6 +2039,10 @@
comments for each path are sorted by patch set number. Each comment has
the `patch_set` and `author` fields set.
+If the `enable_context` request parameter is set to true, the comment entries
+will contain a list of link:#context-line[ContextLine] containing the lines of
+the source file where the comment was written.
+
.Request
----
GET /changes/myProject~master~I8473b95934b5732ac55d26311a706c9c2bde9940/comments HTTP/1.0
@@ -6676,6 +6680,11 @@
|`commit_id` |optional|
Hex commit SHA1 (40 characters string) of the commit of the patchset to which
this comment applies.
+|`context_lines` |optional|
+A list of link:#context-line[ContextLine] containing the lines of the source
+file where the comment was written. Available only if the "enable_context"
+parameter (see link:#list-change-comments[List Change Comments]) is set.
+
|===========================
[[comment-input]]
@@ -6747,6 +6756,18 @@
|`end_character` ||The character position in the end line. (0-based)
|===========================
+[[context-line]]
+=== ContextLine
+The `ContextLine` entity contains the line number and line text of a single
+line of the source file content.
+
+[options="header",cols="1,6"]
+|===========================
+|Field Name |Description
+|`line_number` |The line number of the source line.
+|`context_line` |String containing the line text.
+|===========================
+
[[commit-info]]
=== CommitInfo
The `CommitInfo` entity contains information about a commit.
diff --git a/java/com/google/gerrit/extensions/api/changes/ChangeApi.java b/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
index e8b58f9..3b3a5fc 100644
--- a/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
+++ b/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
@@ -326,8 +326,12 @@
* @return comments in a map keyed by path; comments have the {@code revision} field set to
* indicate their patch set.
* @throws RestApiException
+ * @deprecated Callers should use {@link #commentsRequest()} instead
*/
- Map<String, List<CommentInfo>> comments() throws RestApiException;
+ @Deprecated
+ default Map<String, List<CommentInfo>> comments() throws RestApiException {
+ return commentsRequest().get();
+ }
/**
* Get all published comments on a change as a list.
@@ -335,8 +339,21 @@
* @return comments as a list; comments have the {@code revision} field set to indicate their
* patch set.
* @throws RestApiException
+ * @deprecate Callers should use {@link #commentsRequest()} instead
*/
- List<CommentInfo> commentsAsList() throws RestApiException;
+ @Deprecated
+ default List<CommentInfo> commentsAsList() throws RestApiException {
+ return commentsRequest().getAsList();
+ }
+
+ /**
+ * Get a {@link CommentsRequest} entity that can be used to retrieve published comments.
+ *
+ * @return A {@link CommentsRequest} entity that can be used to retrieve the comments using the
+ * {@link CommentsRequest#get()} or {@link CommentsRequest#getAsList()}.
+ * @throws RestApiException
+ */
+ CommentsRequest commentsRequest() throws RestApiException;
/**
* Get all robot comments on a change.
@@ -395,6 +412,42 @@
*/
ChangeMessageApi message(String id) throws RestApiException;
+ abstract class CommentsRequest {
+ private boolean enableContext;
+
+ /**
+ * Get all published comments on a change.
+ *
+ * @return comments in a map keyed by path; comments have the {@code revision} field set to
+ * indicate their patch set.
+ * @throws RestApiException
+ */
+ public abstract Map<String, List<CommentInfo>> get() throws RestApiException;
+
+ /**
+ * Get all published comments on a change as a list.
+ *
+ * @return comments as a list; comments have the {@code revision} field set to indicate their
+ * patch set.
+ * @throws RestApiException
+ */
+ public abstract List<CommentInfo> getAsList() throws RestApiException;
+
+ public CommentsRequest withContext(boolean enableContext) {
+ this.enableContext = enableContext;
+ return this;
+ }
+
+ public CommentsRequest withContext() {
+ this.enableContext = true;
+ return this;
+ }
+
+ public boolean getContext() {
+ return enableContext;
+ }
+ }
+
abstract class SuggestedReviewersRequest {
private String query;
private int limit;
@@ -603,16 +656,23 @@
}
@Override
+ @Deprecated
public Map<String, List<CommentInfo>> comments() throws RestApiException {
throw new NotImplementedException();
}
@Override
+ @Deprecated
public List<CommentInfo> commentsAsList() throws RestApiException {
throw new NotImplementedException();
}
@Override
+ public CommentsRequest commentsRequest() throws RestApiException {
+ throw new NotImplementedException();
+ }
+
+ @Override
public Map<String, List<RobotCommentInfo>> robotComments() throws RestApiException {
throw new NotImplementedException();
}
diff --git a/java/com/google/gerrit/extensions/api/changes/CommentInput.java b/java/com/google/gerrit/extensions/api/changes/CommentInput.java
new file mode 100644
index 0000000..4e2f033
--- /dev/null
+++ b/java/com/google/gerrit/extensions/api/changes/CommentInput.java
@@ -0,0 +1,20 @@
+// 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.extensions.api.changes;
+
+/** Input to the {@link ChangeApi#comments(CommentInput)}. */
+public class CommentInput {
+ public boolean enableContext;
+}
diff --git a/java/com/google/gerrit/extensions/common/CommentInfo.java b/java/com/google/gerrit/extensions/common/CommentInfo.java
index 19e002a..19d721b 100644
--- a/java/com/google/gerrit/extensions/common/CommentInfo.java
+++ b/java/com/google/gerrit/extensions/common/CommentInfo.java
@@ -15,6 +15,7 @@
package com.google.gerrit.extensions.common;
import com.google.gerrit.extensions.client.Comment;
+import java.util.List;
import java.util.Objects;
public class CommentInfo extends Comment {
@@ -22,6 +23,12 @@
public String tag;
public String changeMessageId;
+ /**
+ * A list of {@link ContextLineInfo}, that is, a list of pairs of {line_num, line_text} of the
+ * actual source file content surrounding and including the lines where the comment was written.
+ */
+ public List<ContextLineInfo> contextLines;
+
@Override
public boolean equals(Object o) {
if (super.equals(o)) {
diff --git a/java/com/google/gerrit/extensions/common/ContextLineInfo.java b/java/com/google/gerrit/extensions/common/ContextLineInfo.java
new file mode 100644
index 0000000..3062e85
--- /dev/null
+++ b/java/com/google/gerrit/extensions/common/ContextLineInfo.java
@@ -0,0 +1,47 @@
+// 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.extensions.common;
+
+import java.util.Objects;
+
+/**
+ * An entity class representing 1 line of context {line number, line text} of the source file where
+ * a comment was written.
+ */
+public class ContextLineInfo {
+ public int lineNumber;
+ public String contextLine;
+
+ public ContextLineInfo() {}
+
+ public ContextLineInfo(int lineNumber, String contextLine) {
+ this.lineNumber = lineNumber;
+ this.contextLine = contextLine;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (o instanceof ContextLineInfo) {
+ ContextLineInfo l = (ContextLineInfo) o;
+ return lineNumber == l.lineNumber && contextLine.equals(l.contextLine);
+ }
+ return false;
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(lineNumber, contextLine);
+ }
+}
diff --git a/java/com/google/gerrit/server/CommentContextLoader.java b/java/com/google/gerrit/server/CommentContextLoader.java
new file mode 100644
index 0000000..813dad7
--- /dev/null
+++ b/java/com/google/gerrit/server/CommentContextLoader.java
@@ -0,0 +1,164 @@
+// 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;
+
+import static java.util.stream.Collectors.groupingBy;
+
+import com.google.auto.value.AutoValue;
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.entities.Project;
+import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.extensions.common.CommentInfo;
+import com.google.gerrit.extensions.common.ContextLineInfo;
+import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.patch.Text;
+import com.google.inject.Inject;
+import com.google.inject.assistedinject.Assisted;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.revwalk.RevWalk;
+import org.eclipse.jgit.treewalk.TreeWalk;
+
+/**
+ * Computes the list of {@link ContextLineInfo} for a given comment, that is, the lines of the
+ * source file surrounding and including the area where the comment was written.
+ */
+public class CommentContextLoader {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ private final GitRepositoryManager repoManager;
+ private final Project.NameKey project;
+ private Map<ContextData, List<ContextLineInfo>> candidates;
+
+ public interface Factory {
+ CommentContextLoader create(Project.NameKey project);
+ }
+
+ @Inject
+ public CommentContextLoader(GitRepositoryManager repoManager, @Assisted Project.NameKey project) {
+ this.repoManager = repoManager;
+ this.project = project;
+ this.candidates = new HashMap<>();
+ }
+
+ /**
+ * Returns an empty list of {@link ContextLineInfo}. Clients are expected to call this method one
+ * or more times. Each call returns a reference to an empty {@link List<ContextLineInfo>}.
+ *
+ * <p>A single call to {@link #fill()} will cause all list references returned from this method to
+ * be populated. If a client calls this method again with a comment that was passed before calling
+ * {@link #fill()}, the new populated list will be returned.
+ *
+ * @param comment the comment entity for which we want to load the context
+ * @return a list of {@link ContextLineInfo}
+ */
+ public List<ContextLineInfo> getContext(CommentInfo comment) {
+ ContextData key =
+ ContextData.create(
+ comment.id,
+ ObjectId.fromString(comment.commitId),
+ comment.path,
+ getStartAndEndLines(comment));
+ List<ContextLineInfo> context = candidates.get(key);
+ if (context == null) {
+ context = new ArrayList<>();
+ candidates.put(key, context);
+ }
+ return context;
+ }
+
+ /**
+ * A call to this method loads the context for all comments stored in {@link
+ * CommentContextLoader#candidates}. This is useful so that the repository is opened once for all
+ * comments.
+ */
+ public void fill() {
+ // Group comments by commit ID so that each commit is parsed only once
+ Map<ObjectId, List<ContextData>> commentsByCommitId =
+ candidates.keySet().stream().collect(groupingBy(ContextData::commitId));
+
+ try (Repository repo = repoManager.openRepository(project);
+ RevWalk rw = new RevWalk(repo)) {
+ for (ObjectId commitId : commentsByCommitId.keySet()) {
+ RevCommit commit = rw.parseCommit(commitId);
+ for (ContextData k : commentsByCommitId.get(commitId)) {
+ if (!k.range().isPresent()) {
+ continue;
+ }
+ try (TreeWalk tw = TreeWalk.forPath(rw.getObjectReader(), k.path(), commit.getTree())) {
+ if (tw == null) {
+ logger.atWarning().log(
+ "Failed to find path %s in the git tree of ID %s.",
+ k.path(), commit.getTree().getId());
+ continue;
+ }
+ ObjectId id = tw.getObjectId(0);
+ Text src = new Text(repo.open(id, Constants.OBJ_BLOB));
+ List<ContextLineInfo> contextLines = candidates.get(k);
+ Range r = k.range().get();
+ for (int i = r.start(); i <= r.end(); i++) {
+ contextLines.add(new ContextLineInfo(i, src.getString(i - 1)));
+ }
+ }
+ }
+ }
+ } catch (IOException e) {
+ throw new StorageException("Failed to load the comment context", e);
+ }
+ }
+
+ private static Optional<Range> getStartAndEndLines(CommentInfo comment) {
+ if (comment.range != null) {
+ return Optional.of(Range.create(comment.range.startLine, comment.range.endLine));
+ } else if (comment.line != null) {
+ return Optional.of(Range.create(comment.line, comment.line));
+ }
+ return Optional.empty();
+ }
+
+ @AutoValue
+ abstract static class Range {
+ static Range create(int start, int end) {
+ return new AutoValue_CommentContextLoader_Range(start, end);
+ }
+
+ abstract int start();
+
+ abstract int end();
+ }
+
+ @AutoValue
+ abstract static class ContextData {
+ static ContextData create(String id, ObjectId commitId, String path, Optional<Range> range) {
+ return new AutoValue_CommentContextLoader_ContextData(id, commitId, path, range);
+ }
+
+ abstract String id();
+
+ abstract ObjectId commitId();
+
+ abstract String path();
+
+ abstract Optional<Range> range();
+ }
+}
diff --git a/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java b/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
index b4a5da7..0992bcd 100644
--- a/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
+++ b/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java
@@ -158,7 +158,7 @@
private final GetAssignee getAssignee;
private final GetPastAssignees getPastAssignees;
private final DeleteAssignee deleteAssignee;
- private final ListChangeComments listComments;
+ private final Provider<ListChangeComments> listCommentsProvider;
private final ListChangeRobotComments listChangeRobotComments;
private final ListChangeDrafts listDrafts;
private final ChangeEditApiImpl.Factory changeEditApi;
@@ -211,7 +211,7 @@
GetAssignee getAssignee,
GetPastAssignees getPastAssignees,
DeleteAssignee deleteAssignee,
- ListChangeComments listComments,
+ Provider<ListChangeComments> listCommentsProvider,
ListChangeRobotComments listChangeRobotComments,
ListChangeDrafts listDrafts,
ChangeEditApiImpl.Factory changeEditApi,
@@ -262,7 +262,7 @@
this.getAssignee = getAssignee;
this.getPastAssignees = getPastAssignees;
this.deleteAssignee = deleteAssignee;
- this.listComments = listComments;
+ this.listCommentsProvider = listCommentsProvider;
this.listChangeRobotComments = listChangeRobotComments;
this.listDrafts = listDrafts;
this.changeEditApi = changeEditApi;
@@ -599,21 +599,30 @@
}
@Override
- public Map<String, List<CommentInfo>> comments() throws RestApiException {
- try {
- return listComments.apply(change).value();
- } catch (Exception e) {
- throw asRestApiException("Cannot get comments", e);
- }
- }
+ public CommentsRequest commentsRequest() throws RestApiException {
+ return new CommentsRequest() {
+ @Override
+ public Map<String, List<CommentInfo>> get() throws RestApiException {
+ try {
+ ListChangeComments listComments = listCommentsProvider.get();
+ listComments.setContext(this.getContext());
+ return listComments.apply(change).value();
+ } catch (Exception e) {
+ throw asRestApiException("Cannot get comments", e);
+ }
+ }
- @Override
- public List<CommentInfo> commentsAsList() throws RestApiException {
- try {
- return listComments.getComments(change);
- } catch (Exception e) {
- throw asRestApiException("Cannot get comments", e);
- }
+ @Override
+ public List<CommentInfo> getAsList() throws RestApiException {
+ try {
+ ListChangeComments listComments = listCommentsProvider.get();
+ listComments.setContext(this.getContext());
+ return listComments.getComments(change);
+ } catch (Exception e) {
+ throw asRestApiException("Cannot get comments", e);
+ }
+ }
+ };
}
@Override
diff --git a/java/com/google/gerrit/server/restapi/change/CommentJson.java b/java/com/google/gerrit/server/restapi/change/CommentJson.java
index 20c4b48..67049e8 100644
--- a/java/com/google/gerrit/server/restapi/change/CommentJson.java
+++ b/java/com/google/gerrit/server/restapi/change/CommentJson.java
@@ -26,6 +26,7 @@
import com.google.gerrit.entities.FixReplacement;
import com.google.gerrit.entities.FixSuggestion;
import com.google.gerrit.entities.HumanComment;
+import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RobotComment;
import com.google.gerrit.extensions.client.Comment.Range;
import com.google.gerrit.extensions.client.Side;
@@ -34,6 +35,7 @@
import com.google.gerrit.extensions.common.FixSuggestionInfo;
import com.google.gerrit.extensions.common.RobotCommentInfo;
import com.google.gerrit.extensions.restapi.Url;
+import com.google.gerrit.server.CommentContextLoader;
import com.google.gerrit.server.account.AccountLoader;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.inject.Inject;
@@ -48,10 +50,15 @@
private boolean fillAccounts = true;
private boolean fillPatchSet;
+ private CommentContextLoader.Factory commentContextLoaderFactory;
+ private CommentContextLoader commentContextLoader;
@Inject
- CommentJson(AccountLoader.Factory accountLoaderFactory) {
+ CommentJson(
+ AccountLoader.Factory accountLoaderFactory,
+ CommentContextLoader.Factory commentContextLoaderFactory) {
this.accountLoaderFactory = accountLoaderFactory;
+ this.commentContextLoaderFactory = commentContextLoaderFactory;
}
CommentJson setFillAccounts(boolean fillAccounts) {
@@ -64,6 +71,13 @@
return this;
}
+ CommentJson setEnableContext(boolean enableContext, Project.NameKey project) {
+ if (enableContext) {
+ this.commentContextLoader = commentContextLoaderFactory.create(project);
+ }
+ return this;
+ }
+
public HumanCommentFormatter newHumanCommentFormatter() {
return new HumanCommentFormatter();
}
@@ -79,6 +93,9 @@
if (loader != null) {
loader.fill();
}
+ if (commentContextLoader != null) {
+ commentContextLoader.fill();
+ }
return info;
}
@@ -103,6 +120,9 @@
if (loader != null) {
loader.fill();
}
+ if (commentContextLoader != null) {
+ commentContextLoader.fill();
+ }
return out;
}
@@ -118,6 +138,9 @@
if (loader != null) {
loader.fill();
}
+ if (commentContextLoader != null) {
+ commentContextLoader.fill();
+ }
return out;
}
@@ -148,6 +171,9 @@
r.author = loader.get(c.author.getId());
}
r.commitId = c.getCommitId().getName();
+ if (commentContextLoader != null) {
+ r.contextLines = commentContextLoader.getContext(r);
+ }
}
protected Range toRange(Comment.Range commentRange) {
diff --git a/java/com/google/gerrit/server/restapi/change/ListChangeComments.java b/java/com/google/gerrit/server/restapi/change/ListChangeComments.java
index b842f55..fec7bdc 100644
--- a/java/com/google/gerrit/server/restapi/change/ListChangeComments.java
+++ b/java/com/google/gerrit/server/restapi/change/ListChangeComments.java
@@ -19,38 +19,56 @@
import com.google.common.collect.ImmutableList;
import com.google.gerrit.entities.ChangeMessage;
import com.google.gerrit.entities.HumanComment;
+import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.common.CommentInfo;
+import com.google.gerrit.extensions.common.ContextLineInfo;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.server.ChangeMessagesUtil;
+import com.google.gerrit.server.CommentContextLoader;
import com.google.gerrit.server.CommentsUtil;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
import com.google.inject.Provider;
-import com.google.inject.Singleton;
import java.util.List;
import java.util.Map;
+import org.kohsuke.args4j.Option;
-@Singleton
public class ListChangeComments implements RestReadView<ChangeResource> {
private final ChangeMessagesUtil changeMessagesUtil;
private final ChangeData.Factory changeDataFactory;
private final Provider<CommentJson> commentJson;
private final CommentsUtil commentsUtil;
+ private final CommentContextLoader.Factory commentContextFactory;
+
+ private boolean includeContext;
+
+ /**
+ * Optional parameter. If set, the contextLines field of the {@link ContextLineInfo} of the
+ * response will contain the lines of the source file where the comment was written.
+ *
+ * @param context If true, comment context will be attached to the response
+ */
+ @Option(name = "--enable-context")
+ public void setContext(boolean context) {
+ this.includeContext = context;
+ }
@Inject
ListChangeComments(
ChangeData.Factory changeDataFactory,
Provider<CommentJson> commentJson,
CommentsUtil commentsUtil,
- ChangeMessagesUtil changeMessagesUtil) {
+ ChangeMessagesUtil changeMessagesUtil,
+ CommentContextLoader.Factory commentContextFactory) {
this.changeDataFactory = changeDataFactory;
this.commentJson = commentJson;
this.commentsUtil = commentsUtil;
this.changeMessagesUtil = changeMessagesUtil;
+ this.commentContextFactory = commentContextFactory;
}
@Override
@@ -70,7 +88,8 @@
private ImmutableList<CommentInfo> getAsList(Iterable<HumanComment> comments, ChangeResource rsrc)
throws PermissionBackendException {
- ImmutableList<CommentInfo> commentInfos = getCommentFormatter().formatAsList(comments);
+ ImmutableList<CommentInfo> commentInfos =
+ getCommentFormatter(rsrc.getProject()).formatAsList(comments);
List<ChangeMessage> changeMessages = changeMessagesUtil.byChange(rsrc.getNotes());
CommentsUtil.linkCommentsToChangeMessages(commentInfos, changeMessages, true);
return commentInfos;
@@ -78,7 +97,8 @@
private Map<String, List<CommentInfo>> getAsMap(
Iterable<HumanComment> comments, ChangeResource rsrc) throws PermissionBackendException {
- Map<String, List<CommentInfo>> commentInfosMap = getCommentFormatter().format(comments);
+ Map<String, List<CommentInfo>> commentInfosMap =
+ getCommentFormatter(rsrc.getProject()).format(comments);
List<CommentInfo> commentInfos =
commentInfosMap.values().stream().flatMap(List::stream).collect(toList());
List<ChangeMessage> changeMessages = changeMessagesUtil.byChange(rsrc.getNotes());
@@ -86,7 +106,12 @@
return commentInfosMap;
}
- private CommentJson.HumanCommentFormatter getCommentFormatter() {
- return commentJson.get().setFillAccounts(true).setFillPatchSet(true).newHumanCommentFormatter();
+ private CommentJson.HumanCommentFormatter getCommentFormatter(Project.NameKey project) {
+ return commentJson
+ .get()
+ .setFillAccounts(true)
+ .setFillPatchSet(true)
+ .setEnableContext(includeContext, project)
+ .newHumanCommentFormatter();
}
}
diff --git a/java/com/google/gerrit/server/restapi/change/Module.java b/java/com/google/gerrit/server/restapi/change/Module.java
index 62d89ee..69e2788 100644
--- a/java/com/google/gerrit/server/restapi/change/Module.java
+++ b/java/com/google/gerrit/server/restapi/change/Module.java
@@ -29,6 +29,7 @@
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.restapi.RestApiModule;
+import com.google.gerrit.server.CommentContextLoader;
import com.google.gerrit.server.account.AccountLoader;
import com.google.gerrit.server.change.AddReviewersOp;
import com.google.gerrit.server.change.AddToAttentionSetOp;
@@ -205,6 +206,7 @@
factory(AccountLoader.Factory.class);
factory(ChangeInserter.Factory.class);
factory(ChangeResource.Factory.class);
+ factory(CommentContextLoader.Factory.class);
factory(DeleteChangeOp.Factory.class);
factory(DeleteReviewerByEmailOp.Factory.class);
factory(DeleteReviewerOp.Factory.class);
diff --git a/java/com/google/gerrit/sshd/commands/DefaultCommandModule.java b/java/com/google/gerrit/sshd/commands/DefaultCommandModule.java
index c7fe74e..cfd17f4 100644
--- a/java/com/google/gerrit/sshd/commands/DefaultCommandModule.java
+++ b/java/com/google/gerrit/sshd/commands/DefaultCommandModule.java
@@ -90,9 +90,12 @@
command("git-receive-pack").to(Commands.key(git, "receive-pack"));
command("gerrit-receive-pack").to(Commands.key(git, "receive-pack"));
command(git, "receive-pack").to(Commands.key(gerrit, "receive-pack"));
- command(gerrit, "test-submit").toProvider(new DispatchCommandProvider(testSubmit));
}
}
+
+ if (!slaveMode) {
+ command(gerrit, "test-submit").toProvider(new DispatchCommandProvider(testSubmit));
+ }
command("suexec").to(SuExec.class);
listener().to(ShowCaches.StartupListener.class);
diff --git a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
index 4f61d79..72453fd 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/CommentsIT.java
@@ -30,6 +30,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
+import com.google.common.collect.MoreCollectors;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
@@ -53,6 +54,7 @@
import com.google.gerrit.extensions.client.Side;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.CommentInfo;
+import com.google.gerrit.extensions.common.ContextLineInfo;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.IdString;
@@ -79,6 +81,7 @@
import java.util.function.Supplier;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
+import java.util.stream.Collectors;
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
@@ -1019,12 +1022,16 @@
addComment(r1, "nit: trailing whitespace");
addComment(r2, "typo: content");
- Map<String, List<CommentInfo>> actual = gApi.changes().id(r2.getChangeId()).comments();
+ Map<String, List<CommentInfo>> actual =
+ gApi.changes().id(r2.getChangeId()).commentsRequest().get();
assertThat(actual.keySet()).containsExactly(FILE_NAME);
List<CommentInfo> comments = actual.get(FILE_NAME);
assertThat(comments).hasSize(2);
+ // Comment context is disabled by default
+ assertThat(comments.stream().filter(c -> c.contextLines != null)).isEmpty();
+
CommentInfo c1 = comments.get(0);
assertThat(c1.author._accountId).isEqualTo(user.id().get());
assertThat(c1.patchSet).isEqualTo(1);
@@ -1041,6 +1048,61 @@
}
@Test
+ public void listChangeCommentsWithContextEnabled() throws Exception {
+ PushOneCommit.Result r1 = createChange();
+
+ ImmutableList.Builder<String> content = ImmutableList.builder();
+ for (int i = 1; i <= 10; i++) {
+ content.add("line_" + i);
+ }
+
+ PushOneCommit.Result r2 =
+ pushFactory
+ .create(
+ admin.newIdent(),
+ testRepo,
+ SUBJECT,
+ FILE_NAME,
+ content.build().stream().collect(Collectors.joining("\n")),
+ r1.getChangeId())
+ .to("refs/for/master");
+
+ addCommentOnLine(r2, "nit: please fix", 1);
+ addCommentOnRange(r2, "looks good", commentRangeInLines(2, 5));
+
+ List<CommentInfo> comments =
+ gApi.changes().id(r2.getChangeId()).commentsRequest().withContext(true).getAsList();
+
+ assertThat(comments).hasSize(2);
+
+ assertThat(
+ comments.stream()
+ .filter(c -> c.message.equals("nit: please fix"))
+ .collect(MoreCollectors.onlyElement())
+ .contextLines)
+ .containsExactlyElementsIn(contextLines("1", "line_1"));
+
+ assertThat(
+ comments.stream()
+ .filter(c -> c.message.equals("looks good"))
+ .collect(MoreCollectors.onlyElement())
+ .contextLines)
+ .containsExactlyElementsIn(
+ contextLines("2", "line_2", "3", "line_3", "4", "line_4", "5", "line_5"));
+ }
+
+ private List<ContextLineInfo> contextLines(String... args) {
+ List<ContextLineInfo> result = new ArrayList<>();
+ for (int i = 0; i < args.length; i += 2) {
+ int lineNbr = Integer.parseInt(args[i]);
+ String contextLine = args[i + 1];
+ ContextLineInfo info = new ContextLineInfo(lineNbr, contextLine);
+ result.add(info);
+ }
+ return result;
+ }
+
+ @Test
public void listChangeCommentsAnonymousDoesNotRequireAuth() throws Exception {
PushOneCommit.Result r1 = createChange();
@@ -1052,12 +1114,12 @@
addComment(r1, "nit: trailing whitespace");
addComment(r2, "typo: content");
- List<CommentInfo> comments = gApi.changes().id(r1.getChangeId()).commentsAsList();
+ List<CommentInfo> comments = gApi.changes().id(r1.getChangeId()).commentsRequest().getAsList();
assertThat(comments.stream().map(c -> c.message).collect(toList()))
.containsExactly("nit: trailing whitespace", "typo: content");
requestScopeOperations.setApiUserAnonymous();
- comments = gApi.changes().id(r1.getChangeId()).commentsAsList();
+ comments = gApi.changes().id(r1.getChangeId()).commentsRequest().getAsList();
assertThat(comments.stream().map(c -> c.message).collect(toList()))
.containsExactly("nit: trailing whitespace", "typo: content");
}
@@ -1713,7 +1775,23 @@
}
private void addComment(PushOneCommit.Result r, String message) throws Exception {
- addComment(r, message, false, false, null);
+ addComment(r, message, false, false, null, 1, null);
+ }
+
+ private void addCommentOnLine(PushOneCommit.Result r, String message, int line) throws Exception {
+ addComment(r, message, false, false, null, line, null);
+ }
+
+ private void addCommentOnRange(PushOneCommit.Result r, String message, Comment.Range range)
+ throws Exception {
+ addComment(r, message, false, false, null, null, range);
+ }
+
+ private Comment.Range commentRangeInLines(int startLine, int endLine) {
+ Comment.Range range = new Comment.Range();
+ range.startLine = startLine;
+ range.endLine = endLine;
+ return range;
}
private void addComment(
@@ -1723,12 +1801,25 @@
Boolean unresolved,
String inReplyTo)
throws Exception {
+ addComment(r, message, omitDuplicateComments, unresolved, inReplyTo, 1, null);
+ }
+
+ private void addComment(
+ PushOneCommit.Result r,
+ String message,
+ boolean omitDuplicateComments,
+ Boolean unresolved,
+ String inReplyTo,
+ Integer line,
+ Comment.Range range)
+ throws Exception {
CommentInput c = new CommentInput();
c.line = 1;
c.message = message;
c.path = FILE_NAME;
c.unresolved = unresolved;
c.inReplyTo = inReplyTo;
+ c.range = range;
ReviewInput in = newInput(c);
in.omitDuplicateComments = omitDuplicateComments;
gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(in);
@@ -1766,11 +1857,11 @@
}
private List<CommentInfo> getPublishedCommentsAsList(String changeId) throws Exception {
- return gApi.changes().id(changeId).commentsAsList();
+ return gApi.changes().id(changeId).commentsRequest().getAsList();
}
private List<CommentInfo> getPublishedCommentsAsList(Change.Id changeId) throws Exception {
- return gApi.changes().id(changeId.get()).commentsAsList();
+ return gApi.changes().id(changeId.get()).commentsRequest().getAsList();
}
private Map<String, List<CommentInfo>> getDraftComments(String changeId, String revId)
diff --git a/javatests/com/google/gerrit/acceptance/testsuite/change/PatchsetOperationsImplTest.java b/javatests/com/google/gerrit/acceptance/testsuite/change/PatchsetOperationsImplTest.java
index c29cf99..9161928 100644
--- a/javatests/com/google/gerrit/acceptance/testsuite/change/PatchsetOperationsImplTest.java
+++ b/javatests/com/google/gerrit/acceptance/testsuite/change/PatchsetOperationsImplTest.java
@@ -213,11 +213,16 @@
@Test
public void commentCanBeCreatedOnSecondParentCommit() throws Exception {
- // Second parents only exist for merge commits. The test API currently doesn't support the
- // creation of changes with merge commits yet, though. As there's no explicit validation keeping
- // us from adding comments on the non-existing second parent of a regular commit, just use the
- // latter. That's still better than not having this test at all.
- Change.Id changeId = changeOperations.newChange().create();
+ Change.Id parent1ChangeId = changeOperations.newChange().create();
+ Change.Id parent2ChangeId = changeOperations.newChange().create();
+ Change.Id changeId =
+ changeOperations
+ .newChange()
+ .mergeOf()
+ .change(parent1ChangeId)
+ .and()
+ .change(parent2ChangeId)
+ .create();
String commentUuid =
changeOperations
@@ -233,12 +238,38 @@
}
@Test
+ public void commentCanBeCreatedOnNonExistingSecondParentCommit() throws Exception {
+ Change.Id parentChangeId = changeOperations.newChange().create();
+ Change.Id changeId = changeOperations.newChange().childOf().change(parentChangeId).create();
+
+ String commentUuid =
+ changeOperations
+ .change(changeId)
+ .currentPatchset()
+ .newComment()
+ .onSecondParentCommit()
+ .create();
+
+ // We want to be able to create such invalid comments for testing purposes (e.g. testing error
+ // handling or resilience of an endpoint) and hence we need to allow such invalid comments in
+ // the test API.
+ CommentInfo comment = getCommentFromServer(changeId, commentUuid);
+ assertThat(comment).side().isEqualTo(Side.PARENT);
+ assertThat(comment).parent().isEqualTo(2);
+ }
+
+ @Test
public void commentCanBeCreatedOnAutoMergeCommit() throws Exception {
- // Second parents only exist for merge commits. The test API currently doesn't support the
- // creation of changes with merge commits yet, though. As there's no explicit validation keeping
- // us from adding comments on the non-existing second parent of a regular commit, just use the
- // latter. That's still better than not having this test at all.
- Change.Id changeId = changeOperations.newChange().create();
+ Change.Id parent1ChangeId = changeOperations.newChange().create();
+ Change.Id parent2ChangeId = changeOperations.newChange().create();
+ Change.Id changeId =
+ changeOperations
+ .newChange()
+ .mergeOf()
+ .change(parent1ChangeId)
+ .and()
+ .change(parent2ChangeId)
+ .create();
String commentUuid =
changeOperations
@@ -550,11 +581,16 @@
@Test
public void draftCommentCanBeCreatedOnSecondParentCommit() throws Exception {
- // Second parents only exist for merge commits. The test API currently doesn't support the
- // creation of changes with merge commits yet, though. As there's no explicit validation keeping
- // us from adding comments on the non-existing second parent of a regular commit, just use the
- // latter. That's still better than not having this test at all.
- Change.Id changeId = changeOperations.newChange().create();
+ Change.Id parent1ChangeId = changeOperations.newChange().create();
+ Change.Id parent2ChangeId = changeOperations.newChange().create();
+ Change.Id changeId =
+ changeOperations
+ .newChange()
+ .mergeOf()
+ .change(parent1ChangeId)
+ .and()
+ .change(parent2ChangeId)
+ .create();
String commentUuid =
changeOperations
@@ -570,12 +606,38 @@
}
@Test
+ public void draftCommentCanBeCreatedOnNonExistingSecondParentCommit() throws Exception {
+ Change.Id parentChangeId = changeOperations.newChange().create();
+ Change.Id changeId = changeOperations.newChange().childOf().change(parentChangeId).create();
+
+ String commentUuid =
+ changeOperations
+ .change(changeId)
+ .currentPatchset()
+ .newDraftComment()
+ .onSecondParentCommit()
+ .create();
+
+ // We want to be able to create such invalid comments for testing purposes (e.g. testing error
+ // handling or resilience of an endpoint) and hence we need to allow such invalid comments in
+ // the test API.
+ CommentInfo comment = getDraftCommentFromServer(changeId, commentUuid);
+ assertThat(comment).side().isEqualTo(Side.PARENT);
+ assertThat(comment).parent().isEqualTo(2);
+ }
+
+ @Test
public void draftCommentCanBeCreatedOnAutoMergeCommit() throws Exception {
- // Second parents only exist for merge commits. The test API currently doesn't support the
- // creation of changes with merge commits yet, though. As there's no explicit validation keeping
- // us from adding comments on the non-existing second parent of a regular commit, just use the
- // latter. That's still better than not having this test at all.
- Change.Id changeId = changeOperations.newChange().create();
+ Change.Id parent1ChangeId = changeOperations.newChange().create();
+ Change.Id parent2ChangeId = changeOperations.newChange().create();
+ Change.Id changeId =
+ changeOperations
+ .newChange()
+ .mergeOf()
+ .change(parent1ChangeId)
+ .and()
+ .change(parent2ChangeId)
+ .create();
String commentUuid =
changeOperations
diff --git a/javatests/com/google/gerrit/server/cache/BUILD b/javatests/com/google/gerrit/server/cache/BUILD
index c255e61..b3b2f5a 100644
--- a/javatests/com/google/gerrit/server/cache/BUILD
+++ b/javatests/com/google/gerrit/server/cache/BUILD
@@ -1,8 +1,9 @@
load("//tools/bzl:junit.bzl", "junit_tests")
+load("//javatests/com/google/gerrit/acceptance:tests.bzl", "acceptance_tests")
junit_tests(
name = "tests",
- srcs = glob(["*.java"]),
+ srcs = glob(["*Test.java"]),
deps = [
"//java/com/google/gerrit/server",
"//java/com/google/gerrit/testing:gerrit-test-util",
@@ -10,3 +11,9 @@
"//lib/truth",
],
)
+
+acceptance_tests(
+ srcs = glob(["*IT.java"]),
+ group = "server_cache",
+ labels = ["server"],
+)
diff --git a/javatests/com/google/gerrit/server/cache/PersistentCacheFactoryIT.java b/javatests/com/google/gerrit/server/cache/PersistentCacheFactoryIT.java
new file mode 100644
index 0000000..d8c6fe2
--- /dev/null
+++ b/javatests/com/google/gerrit/server/cache/PersistentCacheFactoryIT.java
@@ -0,0 +1,74 @@
+// 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.cache;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.server.ModuleImpl;
+import com.google.inject.AbstractModule;
+import com.google.inject.Inject;
+import org.junit.Test;
+
+public class PersistentCacheFactoryIT extends AbstractDaemonTest {
+
+ @Inject PersistentCacheFactory persistentCacheFactory;
+
+ @ModuleImpl(name = CacheModule.PERSISTENT_MODULE)
+ public static class Module extends AbstractModule {
+
+ @Override
+ protected void configure() {
+ bind(PersistentCacheFactory.class).to(TestCacheFactory.class);
+ }
+ }
+
+ @Override
+ public com.google.inject.Module createModule() {
+ return new Module();
+ }
+
+ @Test
+ public void shouldH2PersistentCacheBeReplaceableByADifferentCacheImplementation() {
+ assertThat(persistentCacheFactory).isInstanceOf(TestCacheFactory.class);
+ }
+
+ public static class TestCacheFactory implements PersistentCacheFactory {
+
+ private final MemoryCacheFactory memoryCacheFactory;
+
+ @Inject
+ TestCacheFactory(MemoryCacheFactory memoryCacheFactory) {
+ this.memoryCacheFactory = memoryCacheFactory;
+ }
+
+ @Override
+ public <K, V> com.google.common.cache.Cache<K, V> build(
+ PersistentCacheDef<K, V> def, CacheBackend backend) {
+ return memoryCacheFactory.build(def, backend);
+ }
+
+ @Override
+ public <K, V> LoadingCache<K, V> build(
+ PersistentCacheDef<K, V> def, CacheLoader<K, V> loader, CacheBackend backend) {
+ return memoryCacheFactory.build(def, loader, backend);
+ }
+
+ @Override
+ public void onStop(String plugin) {}
+ }
+}
diff --git a/polygerrit-ui/app/elements/plugins/gr-plugin-types.ts b/polygerrit-ui/app/elements/plugins/gr-plugin-types.ts
index 538c78a..9eeddcc 100644
--- a/polygerrit-ui/app/elements/plugins/gr-plugin-types.ts
+++ b/polygerrit-ui/app/elements/plugins/gr-plugin-types.ts
@@ -18,7 +18,6 @@
import {GrPluginRestApi} from '../shared/gr-js-api-interface/gr-plugin-rest-api';
import {GrEventHelper} from './gr-event-helper/gr-event-helper';
import {GrPopupInterface} from './gr-popup-interface/gr-popup-interface';
-import {GrPluginActionContext} from '../shared/gr-js-api-interface/gr-plugin-action-context';
import {ConfigInfo} from '../../types/common';
interface GerritElementExtensions {
@@ -85,7 +84,9 @@
export interface PluginApi {
_url?: URL;
- deprecated: PluginDeprecatedApi;
+ popup(): Promise<GrPopupInterface>;
+ popup(moduleName: string): Promise<GrPopupInterface>;
+ popup(moduleName?: string): Promise<GrPopupInterface | null>;
hook(endpointName: string, opt_options?: RegisterOptions): HookApi;
getPluginName(): string;
on(eventName: string, target: any): void;
@@ -93,21 +94,3 @@
restApi(): GrPluginRestApi;
eventHelper(element: Node): GrEventHelper;
}
-
-export interface PluginDeprecatedApi {
- _loadedGwt(): void;
- install: () => void;
- popup(element: Node): GrPopupInterface;
- onAction(
- type: string,
- action: string,
- callback: (ctx: GrPluginActionContext) => void
- ): void;
- panel(extensionpoint: string, callback: (panel: PanelInfo) => void): void;
- screen(pattern: string, callback: (settings: SettingsInfo) => void): void;
- settingsScreen(
- path: string,
- menu: string,
- callback: (settings: SettingsInfo) => void
- ): void;
-}
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 e856788..873cc4b 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
@@ -20,7 +20,6 @@
import {GrPopupInterface} from '../../plugins/gr-popup-interface/gr-popup-interface.js';
import {GrSettingsApi} from '../../plugins/gr-settings-api/gr-settings-api.js';
import {EventType} from '../../plugins/gr-plugin-types.js';
-import {GrPluginActionContext} from './gr-plugin-action-context.js';
import {PLUGIN_LOADING_TIMEOUT_MS} from './gr-api-utils.js';
import {getPluginLoader} from './gr-plugin-loader.js';
import {_testOnly_initGerritPluginApi} from './gr-gerrit.js';
@@ -353,15 +352,6 @@
assert.isOk(plugin.attributeHelper());
});
- test('deprecated.install', () => {
- assert.notStrictEqual(plugin.popup, plugin.deprecated.popup);
- assert.notStrictEqual(plugin.onAction, plugin.deprecated.onAction);
- plugin.deprecated.install();
- assert.strictEqual(plugin.popup, plugin.deprecated.popup);
- assert.strictEqual(plugin.onAction, plugin.deprecated.onAction);
- assert.notStrictEqual(plugin.install, plugin.deprecated.install);
- });
-
test('getAdminMenuLinks', () => {
const links = [{text: 'a', url: 'b'}, {text: 'c', url: 'd'}];
const getCallbacksStub = sinon.stub(element, '_getEventCallbacks')
@@ -415,56 +405,6 @@
plugin.popup('some-name');
assert.isTrue(openStub.calledOnce);
});
-
- test('deprecated.popup(element) creates popup with element', () => {
- const el = document.createElement('div');
- el.textContent = 'some text here';
- const openStub = sinon.stub(GrPopupInterface.prototype, 'open');
- openStub.returns(Promise.resolve({
- _getElement() {
- return document.createElement('div');
- }}));
- plugin.deprecated.popup(el);
- assert.isTrue(openStub.calledOnce);
- });
- });
-
- suite('onAction', () => {
- let change;
- let revision;
- let actionDetails;
-
- setup(() => {
- change = {};
- revision = {};
- actionDetails = {__key: 'some'};
- sinon.stub(plugin, 'on').callsArgWith(1, change, revision);
- sinon.stub(plugin, 'changeActions').returns({
- addTapListener: sinon.stub().callsArg(1),
- getActionDetails: () => actionDetails,
- });
- });
-
- test('returns GrPluginActionContext', () => {
- const stub = sinon.stub();
- plugin.deprecated.onAction('change', 'foo', ctx => {
- assert.isTrue(ctx instanceof GrPluginActionContext);
- assert.strictEqual(ctx.change, change);
- assert.strictEqual(ctx.revision, revision);
- assert.strictEqual(ctx.action, actionDetails);
- assert.strictEqual(ctx.plugin, plugin);
- stub();
- });
- assert.isTrue(stub.called);
- });
-
- test('other actions', () => {
- const stub = sinon.stub();
- plugin.deprecated.onAction('project', 'foo', stub);
- plugin.deprecated.onAction('edit', 'foo', stub);
- plugin.deprecated.onAction('branch', 'foo', stub);
- assert.isFalse(stub.called);
- });
});
suite('screen', () => {
@@ -480,18 +420,6 @@
);
});
- test('deprecated works', () => {
- const stub = sinon.stub();
- const hookStub = {onAttached: sinon.stub()};
- sinon.stub(plugin, 'hook').returns(hookStub);
- plugin.deprecated.screen('foo', stub);
- assert.isTrue(plugin.hook.calledWith('testplugin-screen-foo'));
- const fakeEl = {style: {display: ''}};
- hookStub.onAttached.callArgWith(0, fakeEl);
- assert.isTrue(stub.called);
- assert.equal(fakeEl.style.display, 'none');
- });
-
test('works', () => {
sinon.stub(plugin, 'registerCustomComponent');
plugin.screen('foo', 'some-module');
@@ -500,82 +428,11 @@
});
});
- suite('panel', () => {
- let fakeEl;
- let emulateAttached;
-
- setup(()=> {
- fakeEl = {change: {}, revision: {}};
- const hookStub = {onAttached: sinon.stub()};
- sinon.stub(plugin, 'hook').returns(hookStub);
- emulateAttached = () => hookStub.onAttached.callArgWith(0, fakeEl);
- });
-
- test('plugin.panel is deprecated', () => {
- plugin.panel('rubbish');
- assert.isTrue(console.error.called);
- });
-
- [
- ['CHANGE_SCREEN_BELOW_COMMIT_INFO_BLOCK', 'change-view-integration'],
- ['CHANGE_SCREEN_BELOW_CHANGE_INFO_BLOCK', 'change-metadata-item'],
- ].forEach(([panelName, endpointName]) => {
- test(`deprecated.panel works for ${panelName}`, () => {
- const callback = sinon.stub();
- plugin.deprecated.panel(panelName, callback);
- assert.isTrue(plugin.hook.calledWith(endpointName));
- emulateAttached();
- assert.isTrue(callback.called);
- const args = callback.args[0][0];
- assert.strictEqual(args.body, fakeEl);
- assert.strictEqual(args.p.CHANGE_INFO, fakeEl.change);
- assert.strictEqual(args.p.REVISION_INFO, fakeEl.revision);
- });
- });
- });
-
suite('settingsScreen', () => {
- test('plugin.settingsScreen is deprecated', () => {
- plugin.settingsScreen('rubbish');
- assert.isTrue(console.error.called);
- });
-
test('plugin.settings() returns GrSettingsApi', () => {
assert.isOk(plugin.settings());
assert.isTrue(plugin.settings() instanceof GrSettingsApi);
});
-
- test('plugin.deprecated.settingsScreen() works', () => {
- const hookStub = {onAttached: sinon.stub()};
- sinon.stub(plugin, 'hook').returns(hookStub);
- const fakeSettings = {};
- fakeSettings.title = sinon.stub().returns(fakeSettings);
- fakeSettings.token = sinon.stub().returns(fakeSettings);
- fakeSettings.module = sinon.stub().returns(fakeSettings);
- fakeSettings.build = sinon.stub().returns(hookStub);
- sinon.stub(plugin, 'settings').returns(fakeSettings);
- const callback = sinon.stub();
-
- plugin.deprecated.settingsScreen('path', 'menu', callback);
- assert.isTrue(fakeSettings.title.calledWith('menu'));
- assert.isTrue(fakeSettings.token.calledWith('path'));
- assert.isTrue(fakeSettings.module.calledWith('div'));
- assert.equal(fakeSettings.build.callCount, 1);
-
- const fakeBody = {};
- const fakeEl = {
- style: {
- display: '',
- },
- querySelector: sinon.stub().returns(fakeBody),
- };
- // Emulate settings screen attached
- hookStub.onAttached.callArgWith(0, fakeEl);
- assert.isTrue(callback.called);
- const args = callback.args[0][0];
- assert.strictEqual(args.body, fakeBody);
- assert.equal(fakeEl.style.display, 'none');
- });
});
});
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-action-context.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-action-context.ts
index 34ecd9e..4366dd5 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-action-context.ts
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-action-context.ts
@@ -42,7 +42,14 @@
) {}
popup(element: Node) {
- this._popups.push(this.plugin.deprecated.popup(element));
+ this.plugin.popup().then(popApi => {
+ const popupEl = popApi._getElement();
+ if (!popupEl) {
+ throw new Error('Popup element not found');
+ }
+ popupEl.appendChild(element);
+ this._popups.push(popApi);
+ });
}
hide() {
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-action-context_test.js b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-action-context_test.js
index e93ffd2..27c9c28 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-action-context_test.js
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-plugin-action-context_test.js
@@ -33,17 +33,20 @@
instance = new GrPluginActionContext(plugin);
});
- test('popup() and hide()', () => {
+ test('popup() and hide()', done => {
const popupApiStub = {
+ _getElement: sinon.stub().returns(document.createElement('div')),
close: sinon.stub(),
};
- sinon.stub(plugin.deprecated, 'popup').returns(popupApiStub);
- const el = {};
+ sinon.stub(plugin, 'popup').returns(Promise.resolve(popupApiStub));
+ const el = document.createElement('span');
instance.popup(el);
- assert.isTrue(instance.plugin.deprecated.popup.calledWith(el));
-
- instance.hide();
- assert.isTrue(popupApiStub.close.called);
+ flush(() => {
+ assert.isTrue(popupApiStub._getElement.called);
+ instance.hide();
+ assert.isTrue(popupApiStub.close.called);
+ done();
+ });
});
test('textfield', () => {
diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-public-js-api.ts b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-public-js-api.ts
index c6b3485..043293f 100644
--- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-public-js-api.ts
+++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-public-js-api.ts
@@ -31,7 +31,6 @@
import {GrRepoApi} from '../../plugins/gr-repo-api/gr-repo-api';
import {GrSettingsApi} from '../../plugins/gr-settings-api/gr-settings-api';
import {GrStylesApi} from '../../plugins/gr-styles-api/gr-styles-api';
-import {GrPluginActionContext} from './gr-plugin-action-context';
import {getPluginEndpoints} from './gr-plugin-endpoints';
import {PRELOADED_PROTOCOL, getPluginNameFromUrl, send} from './gr-api-utils';
@@ -39,23 +38,15 @@
import {
EventType,
HookApi,
- PanelInfo,
PluginApi,
- PluginDeprecatedApi,
RegisterOptions,
- SettingsInfo,
TargetElement,
} from '../../plugins/gr-plugin-types';
-import {ActionInfo, RequestPayload} from '../../../types/common';
+import {RequestPayload} from '../../../types/common';
import {HttpMethod} from '../../../constants/constants';
import {JsApiService} from './gr-js-api-types';
import {GrChangeActions} from '../../../services/services/gr-rest-api/gr-rest-api';
-const PANEL_ENDPOINTS_MAPPING = {
- CHANGE_SCREEN_BELOW_COMMIT_INFO_BLOCK: 'change-view-integration',
- CHANGE_SCREEN_BELOW_CHANGE_INFO_BLOCK: 'change-metadata-item',
-};
-
/**
* Plugin-provided custom components can affect content in extension
* points using one of following methods:
@@ -77,8 +68,6 @@
export type SendCallback = (response: unknown) => void;
export class Plugin implements PluginApi {
- readonly deprecated: PluginDeprecatedApi;
-
readonly _url?: URL;
private _domHooks: GrDomHooksManager;
@@ -89,25 +78,6 @@
private readonly sharedApiElement: JsApiService;
constructor(url?: string) {
- this.deprecated = {
- _loadedGwt: () => {},
- install: () => this.deprecatedInstall(),
- onAction: (
- type: string,
- action: string,
- callback: (ctx: GrPluginActionContext) => void
- ) => this.deprecatedOnAction(type, action, callback),
- panel: (extensionpoint: string, callback: (panel: PanelInfo) => void) =>
- this.deprecatedPanel(extensionpoint, callback),
- popup: (el: Element) => this.deprecatedPopup(el),
- screen: (pattern: string, callback: (settings: SettingsInfo) => void) =>
- this.deprecatedScreen(pattern, callback),
- settingsScreen: (
- path: string,
- menu: string,
- callback: (settings: SettingsInfo) => void
- ) => this.deprecatedSettingsScreen(path, menu, callback),
- };
this.sharedApiElement = getSharedApiEl();
this._domHooks = new GrDomHooksManager(this);
@@ -327,25 +297,16 @@
return new GrEventHelper(element);
}
- popup(moduleName: string) {
- if (typeof moduleName !== 'string') {
+ popup(): Promise<GrPopupInterface>;
+
+ popup(moduleName: string): Promise<GrPopupInterface>;
+
+ popup(moduleName?: string): Promise<GrPopupInterface | null> {
+ if (moduleName !== undefined && typeof moduleName !== 'string') {
console.error('.popup(element) deprecated, use .popup(moduleName)!');
- return;
+ return Promise.resolve(null);
}
- const api = new GrPopupInterface(this, moduleName);
- return api.open();
- }
-
- panel() {
- console.error(
- '.panel() is deprecated! ' + 'Use registerCustomComponent() instead.'
- );
- }
-
- settingsScreen() {
- console.error(
- '.settingsScreen() is deprecated! ' + 'Use .settings() instead.'
- );
+ return new GrPopupInterface(this, moduleName).open();
}
screen(screenName: string, moduleName?: string) {
@@ -365,142 +326,4 @@
_getScreenName(screenName: string) {
return `${this.getPluginName()}-screen-${screenName}`;
}
-
- // !!! DEPRECATED !!!
- // All methods below are deprecated!
- // TODO: should be removed soon after all core plugins moved away from it.
-
- deprecatedInstall() {
- console.info('Installing deprecated APIs is deprecated!');
- const deprecatedThis = (this as unknown) as PluginDeprecatedApi;
- deprecatedThis._loadedGwt = this.deprecated._loadedGwt;
- deprecatedThis.onAction = this.deprecated.onAction;
- deprecatedThis.panel = this.deprecated.panel;
- deprecatedThis.popup = this.deprecated.popup;
- deprecatedThis.screen = this.deprecated.screen;
- deprecatedThis.settingsScreen = this.deprecated.settingsScreen;
- }
-
- deprecatedPopup(el: Element): GrPopupInterface {
- console.warn(
- 'plugin.deprecated.popup() is deprecated, ' + 'use plugin.popup() insted!'
- );
- if (!el) {
- throw new Error('Popup contents not found');
- }
- const api = new GrPopupInterface(this);
- api.open().then(api => {
- const popupEl = api._getElement();
- if (!popupEl) {
- throw new Error('Popup element not found');
- }
- popupEl.appendChild(el);
- });
- return api;
- }
-
- deprecatedOnAction(
- type: string,
- action: string,
- callback: (ctx: GrPluginActionContext) => void
- ) {
- console.warn(
- 'plugin.deprecated.onAction() is deprecated,' +
- ' use plugin.changeActions() instead!'
- );
- if (type !== 'change' && type !== 'revision') {
- console.warn(`${type} actions are not supported.`);
- return;
- }
- this.on(EventType.SHOW_CHANGE, (change, revision) => {
- const details: ActionInfo = this.changeActions().getActionDetails(action);
- if (!details) {
- console.warn(
- `${this.getPluginName()} onAction error: ${action} not found!`
- );
- return;
- }
- if (!details.__key) {
- console.warn(
- `${this.getPluginName()} onAction error: ${action} has no key!`
- );
- return;
- }
- this.changeActions().addTapListener(details.__key, () => {
- callback(new GrPluginActionContext(this, details, change, revision));
- });
- });
- }
-
- deprecatedScreen(
- pattern: string,
- callback: (settings: SettingsInfo) => void
- ) {
- console.warn(
- 'plugin.deprecated.screen is deprecated,' + ' use plugin.screen instead!'
- );
- this.hook(this._getScreenName(pattern)).onAttached(el => {
- el.style.display = 'none';
- callback({
- body: el,
- token: el.token,
- onUnload: () => {},
- setTitle: () => {},
- setWindowTitle: () => {},
- show: () => {
- el.style.display = 'initial';
- },
- });
- });
- }
-
- deprecatedSettingsScreen(
- path: string,
- menu: string,
- callback: (settings: SettingsInfo) => void
- ) {
- console.warn('.settingsScreen() is deprecated! Use .settings() instead.');
- const hook = this.settings().title(menu).token(path).module('div').build();
- hook.onAttached(el => {
- el.style.display = 'none';
- const body = el.querySelector('div');
- if (!body) return;
- callback({
- body,
- onUnload: () => {},
- setTitle: () => {},
- setWindowTitle: () => {},
- show: () => {
- el.style.display = 'initial';
- },
- });
- });
- }
-
- deprecatedPanel(
- extensionpoint: string,
- callback: (panel: PanelInfo) => void
- ) {
- console.warn(
- '.panel() is deprecated! ' + 'Use registerCustomComponent() instead.'
- );
- let endpoint;
- for (const [key, value] of Object.entries(PANEL_ENDPOINTS_MAPPING)) {
- if (key === extensionpoint) endpoint = value;
- }
- if (!endpoint) {
- console.warn(`.panel ${extensionpoint} not supported!`);
- return;
- }
- this.hook(endpoint).onAttached(el =>
- callback({
- body: el,
- p: {
- CHANGE_INFO: el.change,
- REVISION_INFO: el.revision,
- },
- onUnload: () => {},
- })
- );
- }
}
