Merge changes I75b79dc0,I489703dc,I87c6f803,Ib07315e2,I48ea2d9b * changes: Add performance log for loading robot comments ChangeIT.changeDetailsDoesNotRequireIndex: Add comment about error in log GetChange: Record performance event for loading the meta revision GetChange: Avoid double loading of change notes when meta rev ID is not given GetChange: Return Optional from getMetaRevId rather than possibly null
diff --git a/.bazelproject b/.bazelproject index ad7b022..0f2ff90 100644 --- a/.bazelproject +++ b/.bazelproject
@@ -16,7 +16,7 @@ targets: //...:all -java_language_level: 11 +java_language_level: 17 workspace_type: java
diff --git a/.bazelrc b/.bazelrc index 9662078..480bea7 100644 --- a/.bazelrc +++ b/.bazelrc
@@ -32,25 +32,6 @@ build:remote_bb --config=config_bb build:remote_bb --config=build_shared -# Define configuration using remotejdk_11, executes using remotejdk_11 or local_jdk -build:build_java11_shared --java_language_version=11 -build:build_java11_shared --java_runtime_version=remotejdk_11 -build:build_java11_shared --tool_java_language_version=11 -build:build_java11_shared --tool_java_runtime_version=remotejdk_11 - -build:java11 --config=build_java11_shared - -# Builds and executes on Google GCP RBE using remotejdk_11 -build:remote11 --config=config_gcp -build:remote11 --config=build_java11_shared - -# Define remote11 configuration alias -build:remote11_gcp --config=remote11 - -# Builds and executes on BuildBuddy RBE using remotejdk_11 -build:remote11_bb --config=config_bb -build:remote11_bb --config=build_java11_shared - # Builds using remotejdk_21, executes using remotejdk_21 or local_jdk build:build_java21_shared --java_language_version=21 build:build_java21_shared --java_runtime_version=remotejdk_21
diff --git a/Documentation/cmd-index-changes.txt b/Documentation/cmd-index-changes.txt index 0ee7aab..1d4cbe34 100644 --- a/Documentation/cmd-index-changes.txt +++ b/Documentation/cmd-index-changes.txt
@@ -16,8 +16,7 @@ supported by the REST API. == ACCESS -Caller must have the 'Maintain Server' capability, or be the owner of the change -to be indexed. +Caller must have the 'Maintain Server' capability. == SCRIPTING This command is intended to be used in scripts.
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index dbda410..361fd3c 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt
@@ -1641,13 +1641,6 @@ + Default is `true`. -[[change.showAssigneeInChangesTable]]change.showAssigneeInChangesTable:: -+ -Show assignee field in changes table. If set to `false`, assignees will -not be visible in changes table. -+ -Default is `false`. - [[change.strictLabels]]change.strictLabels:: + Reject invalid label votes: invalid labels or invalid values. This @@ -4648,6 +4641,37 @@ This config can be used when gerrit migrates from a deprecated plugin to the new one. The new plugin can (temporary) accept push options of the old plugin without registering such options. +[[plugins.loadPriority]]plugins.loadPriority:: ++ +List of `pluginName`s required to have a specific loading order during Gerrit startup. ++ +Each entry should contain a plugin name defined in the `MANIFEST.MF` under +`Gerrit-PluginName` or a plugin JAR file name. During the Gerrit startup +the `loadPriority` will influence the loading sequence of the JAR plugins. ++ +NOTE: Non-JAR plugins (e.g. Scripting, PolyGerrit plugins, ApiModule) are not +influenced by this setting. ++ +Gerrit will always load plugins defining `Gerrit-ApiModule` in their +`MANIFEST.MF` first. Then will load other plugins according to: + + * the order of `plugins.loadPriority` in `gerrit.config`, + * the natural order of plugin JAR file names in `plugins/` directory. ++ +Example: +Assuming we have three plugins: `a-plugin.jar`, `b-plugin.jar`, `c-plugin.jar` +deployed to `plugins/` directory. By default Gerrit will load them in the same order +as they are listed above, as that follows the _natural storing order_. Now assume +the below configuration + +---- +[plugins] + loadPriority = c-plugin + loadPriority = a-plugin +---- + +Gerrit will load `c-plugin` first, followed-up by `a-plugin` and `b-plugin` last. + [[receive]] === Section receive
diff --git a/Documentation/config-robot-comments.txt b/Documentation/config-robot-comments.txt index 04309e5..ef99d80 100644 --- a/Documentation/config-robot-comments.txt +++ b/Documentation/config-robot-comments.txt
@@ -1,5 +1,9 @@ = Gerrit Code Review - Robot Comments +[NOTE] +Robot Comments are deprecated in favour of link:pg-plugin-checks-api.html[Checks API] and human +comments. + Gerrit has special support for inline comments that are generated by automated third-party systems, so called "robot comments". For example robot comments can be used to represent the results of code analyzers.
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index b8160db..50885b6 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt
@@ -7233,6 +7233,12 @@ |`_number` || The change number. (The underscore is just a relict of a prior attempt to deprecate the change number.) +|`virtual_id_number` || +The virtual id number is globally unique. For local changes, it is equal to the +`_number` attribute. For imported changes, the original `_number` is processed +through a function designed to prevent conflicts with local change numbers. +Note that its usage is intended solely for Gerrit's internals and UI, and +adoption outside these scenarios is not advised. |`owner` || The owner of the change as an link:rest-api-accounts.html#account-info[ AccountInfo] entity.
diff --git a/java/Main.java b/java/Main.java index c04db2c..e824a95 100644 --- a/java/Main.java +++ b/java/Main.java
@@ -16,7 +16,7 @@ public final class Main { private static final String FLOGGER_BACKEND_PROPERTY = "flogger.backend_factory"; private static final String FLOGGER_LOGGING_CONTEXT = "flogger.logging_context"; - private static final Runtime.Version MIN_JAVA_VERSION = Runtime.Version.parse("11.0.10"); + private static final Runtime.Version MIN_JAVA_VERSION = Runtime.Version.parse("17.0.5"); // We don't do any real work here because we need to import // the archive lookup code and we cannot import a class in
diff --git a/java/com/google/gerrit/common/BUILD b/java/com/google/gerrit/common/BUILD index 8f85311..ff43bbd 100644 --- a/java/com/google/gerrit/common/BUILD +++ b/java/com/google/gerrit/common/BUILD
@@ -3,6 +3,7 @@ ANNOTATIONS = [ "Nullable.java", "UsedAt.java", + "ConvertibleToProto.java", ] java_library(
diff --git a/java/com/google/gerrit/common/ConvertibleToProto.java b/java/com/google/gerrit/common/ConvertibleToProto.java new file mode 100644 index 0000000..65074d4 --- /dev/null +++ b/java/com/google/gerrit/common/ConvertibleToProto.java
@@ -0,0 +1,12 @@ +package com.google.gerrit.common; + +import static java.lang.annotation.ElementType.TYPE; +import static java.lang.annotation.RetentionPolicy.RUNTIME; + +import java.lang.annotation.Retention; +import java.lang.annotation.Target; + +/** An annotation used to mark Java entities that have equivalent proto representations. */ +@Retention(RUNTIME) +@Target({TYPE}) +public @interface ConvertibleToProto {}
diff --git a/java/com/google/gerrit/extensions/api/changes/ChangeApi.java b/java/com/google/gerrit/extensions/api/changes/ChangeApi.java index 1c83bc2..dec3125 100644 --- a/java/com/google/gerrit/extensions/api/changes/ChangeApi.java +++ b/java/com/google/gerrit/extensions/api/changes/ChangeApi.java
@@ -132,13 +132,21 @@ setReadyForReview(null); } - /** Create a new change that reverts this change. */ + /** + * Create a new change that reverts this change. + * + * @see Changes#id(String, int) + */ @CanIgnoreReturnValue default ChangeApi revert() throws RestApiException { return revert(new RevertInput()); } - /** Create a new change that reverts this change. */ + /** + * Create a new change that reverts this change. + * + * @see Changes#id(String, int) + */ @CanIgnoreReturnValue ChangeApi revert(RevertInput in) throws RestApiException;
diff --git a/java/com/google/gerrit/extensions/api/changes/Changes.java b/java/com/google/gerrit/extensions/api/changes/Changes.java index 5e3d08c..605a92e 100644 --- a/java/com/google/gerrit/extensions/api/changes/Changes.java +++ b/java/com/google/gerrit/extensions/api/changes/Changes.java
@@ -32,14 +32,10 @@ /** * Look up a change by numeric ID. * - * <p><strong>Note:</strong> This method eagerly reads the change. Methods that mutate the change - * do not necessarily re-read the change. Therefore, calling a getter method on an instance after - * calling a mutation method on that same instance is not guaranteed to reflect the mutation. It - * is not recommended to store references to {@code ChangeApi} instances. Also note that the - * change numeric id without a project name parameter may fail to identify a unique change - * element, because the potential conflict with other changes imported from Gerrit instances with - * a different Server-Id. + * <p><strong>Note:</strong> Change number is not guaranteed to unambiguously identify a change. * + * @see #id(String, int) + * @deprecated in favor of {@link #id(String, int)} * @param id change number. * @return API for accessing the change. * @throws RestApiException if an error occurred. @@ -50,7 +46,7 @@ /** * Look up a change by string ID. * - * @see #id(int) + * @see #id(String, int) * @param id any identifier supported by the REST API, including change number, Change-Id, or * project~branch~Change-Id triplet. * @return API for accessing the change. @@ -61,16 +57,23 @@ /** * Look up a change by project, branch, and change ID. * - * @see #id(int) + * @see #id(String, int) */ ChangeApi id(String project, String branch, String id) throws RestApiException; /** * Look up a change by project and numeric ID. * + * <p><strong>Note:</strong> This method eagerly reads the change. Methods that mutate the change + * do not necessarily re-read the change. Therefore, calling a getter method on an instance after + * calling a mutation method on that same instance is not guaranteed to reflect the mutation. It + * is not recommended to store references to {@code ChangeApi} instances. Also note that the + * change numeric id without a project name parameter may fail to identify a unique change + * element, because the potential conflict with other changes imported from Gerrit instances with + * a different Server-Id. + * * @param project project name. * @param id change number. - * @see #id(int) */ ChangeApi id(String project, int id) throws RestApiException;
diff --git a/java/com/google/gerrit/extensions/common/ChangeInfo.java b/java/com/google/gerrit/extensions/common/ChangeInfo.java index 4a3b423..52127e4 100644 --- a/java/com/google/gerrit/extensions/common/ChangeInfo.java +++ b/java/com/google/gerrit/extensions/common/ChangeInfo.java
@@ -102,7 +102,7 @@ public Boolean containsGitConflicts; public Integer _number; - public Integer _virtualIdNumber; + public Integer virtualIdNumber; public AccountInfo owner;
diff --git a/java/com/google/gerrit/metrics/Timer0.java b/java/com/google/gerrit/metrics/Timer0.java index fb71c07..2d91515 100644 --- a/java/com/google/gerrit/metrics/Timer0.java +++ b/java/com/google/gerrit/metrics/Timer0.java
@@ -76,7 +76,7 @@ LoggingContext.getInstance() .addPerformanceLogRecord(() -> PerformanceLogRecord.create(name, durationNanos)); - logger.atFinest().log("%s took %.2f ms", name, durationNanos / 1000.0); + logger.atFinest().log("%s took %.2f ms", name, durationNanos / 1000000.0); doRecord(value, unit); RequestStateContext.abortIfCancelled();
diff --git a/java/com/google/gerrit/metrics/Timer1.java b/java/com/google/gerrit/metrics/Timer1.java index ad21b254..9dc32da 100644 --- a/java/com/google/gerrit/metrics/Timer1.java +++ b/java/com/google/gerrit/metrics/Timer1.java
@@ -90,7 +90,7 @@ LoggingContext.getInstance() .addPerformanceLogRecord(() -> PerformanceLogRecord.create(name, durationNanos, metadata)); logger.atFinest().log( - "%s (%s = %s) took %.2f ms", name, field.name(), fieldValue, durationNanos / 1000.0); + "%s (%s = %s) took %.2f ms", name, field.name(), fieldValue, durationNanos / 1000000.0); doRecord(fieldValue, value, unit); RequestStateContext.abortIfCancelled();
diff --git a/java/com/google/gerrit/metrics/Timer2.java b/java/com/google/gerrit/metrics/Timer2.java index 72709e1..46dd617 100644 --- a/java/com/google/gerrit/metrics/Timer2.java +++ b/java/com/google/gerrit/metrics/Timer2.java
@@ -99,7 +99,7 @@ .addPerformanceLogRecord(() -> PerformanceLogRecord.create(name, durationNanos, metadata)); logger.atFinest().log( "%s (%s = %s, %s = %s) took %.2f ms", - name, field1.name(), fieldValue1, field2.name(), fieldValue2, durationNanos / 1000.0); + name, field1.name(), fieldValue1, field2.name(), fieldValue2, durationNanos / 1000000.0); doRecord(fieldValue1, fieldValue2, value, unit); RequestStateContext.abortIfCancelled();
diff --git a/java/com/google/gerrit/metrics/Timer3.java b/java/com/google/gerrit/metrics/Timer3.java index 5feae3e..922cdd6 100644 --- a/java/com/google/gerrit/metrics/Timer3.java +++ b/java/com/google/gerrit/metrics/Timer3.java
@@ -115,7 +115,7 @@ fieldValue2, field3.name(), fieldValue3, - durationNanos / 1000.0); + durationNanos / 1000000.0); doRecord(fieldValue1, fieldValue2, fieldValue3, value, unit); RequestStateContext.abortIfCancelled();
diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java index a5da7c2..46f3e0e 100644 --- a/java/com/google/gerrit/server/change/ChangeJson.java +++ b/java/com/google/gerrit/server/change/ChangeJson.java
@@ -806,7 +806,7 @@ } } - out._virtualIdNumber = cd.virtualId().get(); + out.virtualIdNumber = cd.virtualId().get(); return out; } @@ -1086,7 +1086,7 @@ // repository only once try (Repository allUsersRepo = repoManager.openRepository(allUsers)) { List<Change.Id> changeIds = - changeInfos.stream().map(c -> Change.id(c._virtualIdNumber)).collect(Collectors.toList()); + changeInfos.stream().map(c -> Change.id(c.virtualIdNumber)).collect(Collectors.toList()); Set<Change.Id> starredChanges = starredChangesreader.areStarred( allUsersRepo, changeIds, userProvider.get().asIdentifiedUser().getAccountId()); @@ -1094,7 +1094,7 @@ return; } changeInfos.stream() - .forEach(c -> c.starred = starredChanges.contains(Change.id(c._virtualIdNumber))); + .forEach(c -> c.starred = starredChanges.contains(Change.id(c.virtualIdNumber))); } catch (IOException e) { logger.atWarning().withCause(e).log("Failed to open All-Users repo."); }
diff --git a/java/com/google/gerrit/server/git/ChangesByProjectCacheImpl.java b/java/com/google/gerrit/server/git/ChangesByProjectCacheImpl.java index ed16006..2bbd261 100644 --- a/java/com/google/gerrit/server/git/ChangesByProjectCacheImpl.java +++ b/java/com/google/gerrit/server/git/ChangesByProjectCacheImpl.java
@@ -286,6 +286,7 @@ int size = 0; size += JavaWeights.OBJECT; // change size += JavaWeights.REFERENCE + GerritWeights.KEY_INT; // changeId + size += JavaWeights.REFERENCE + (c.getServerId() == null ? 0 : c.getServerId().length()); size += JavaWeights.REFERENCE + JavaWeights.OBJECT + 40; // changeKey; size += JavaWeights.REFERENCE + GerritWeights.TIMESTAMP; // createdOn; size += JavaWeights.REFERENCE + GerritWeights.TIMESTAMP; // lastUpdatedOn; @@ -303,12 +304,12 @@ + (c.getOriginalSubject().equals(c.getSubject()) ? 0 : c.getSubject().length()); size += JavaWeights.REFERENCE + (c.getSubmissionId() == null ? 0 : c.getSubmissionId().length()); - size += JavaWeights.REFERENCE + GerritWeights.ACCOUNT_ID; // assignee; size += JavaWeights.REFERENCE + JavaWeights.BOOLEAN; // isPrivate; size += JavaWeights.REFERENCE + JavaWeights.BOOLEAN; // workInProgress; size += JavaWeights.REFERENCE + JavaWeights.BOOLEAN; // reviewStarted; - size += JavaWeights.REFERENCE + GerritWeights.CHANGE_NUM; // revertOf; - size += JavaWeights.REFERENCE + GerritWeights.PACTCH_SET_ID; // cherryPickOf; + size += JavaWeights.REFERENCE + (c.getRevertOf() == null ? 0 : GerritWeights.CHANGE_NUM); + size += + JavaWeights.REFERENCE + (c.getCherryPickOf() == null ? 0 : GerritWeights.PACTCH_SET_ID); return size; }
diff --git a/java/com/google/gerrit/server/git/WorkQueue.java b/java/com/google/gerrit/server/git/WorkQueue.java index d51ee5e..307ec5c 100644 --- a/java/com/google/gerrit/server/git/WorkQueue.java +++ b/java/com/google/gerrit/server/git/WorkQueue.java
@@ -504,11 +504,11 @@ } public void onStart(Task<?> task) { - listeners.runEach(extension -> extension.getProvider().get().onStart(task)); + listeners.runEach(extension -> extension.get().onStart(task)); } public void onStop(Task<?> task) { - listeners.runEach(extension -> extension.getProvider().get().onStop(task)); + listeners.runEach(extension -> extension.get().onStop(task)); } }
diff --git a/java/com/google/gerrit/server/logging/TraceContext.java b/java/com/google/gerrit/server/logging/TraceContext.java index 4d738ea..a5eef65 100644 --- a/java/com/google/gerrit/server/logging/TraceContext.java +++ b/java/com/google/gerrit/server/logging/TraceContext.java
@@ -192,7 +192,7 @@ LoggingContext.getInstance() .addPerformanceLogRecord( () -> PerformanceLogRecord.create(operation, elapsedNanos)); - logger.atFine().log("%s done (%.2f ms)", operation, elapsedNanos / 1000.0); + logger.atFine().log("%s done (%.2f ms)", operation, elapsedNanos / 1000000.0); }); } @@ -207,7 +207,7 @@ () -> PerformanceLogRecord.create(operation, elapsedNanos, metadata)); logger.atFine().log( "%s (%s) done (%.2f ms)", - operation, metadata.toStringForLoggingLazy(), elapsedNanos / 1000.0); + operation, metadata.toStringForLoggingLazy(), elapsedNanos / 1000000.0); }); }
diff --git a/java/com/google/gerrit/server/plugins/PluginLoader.java b/java/com/google/gerrit/server/plugins/PluginLoader.java index e122d8d..7c7d8d5 100644 --- a/java/com/google/gerrit/server/plugins/PluginLoader.java +++ b/java/com/google/gerrit/server/plugins/PluginLoader.java
@@ -18,7 +18,6 @@ import com.google.common.base.Joiner; import com.google.common.base.MoreObjects; import com.google.common.base.Strings; -import com.google.common.collect.ComparisonChain; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.common.collect.LinkedHashMultimap; @@ -52,7 +51,6 @@ import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Collection; -import java.util.Comparator; import java.util.HashSet; import java.util.Iterator; import java.util.List; @@ -62,7 +60,6 @@ import java.util.TreeSet; import java.util.concurrent.ConcurrentMap; import java.util.concurrent.TimeUnit; -import java.util.jar.JarFile; import org.eclipse.jgit.internal.storage.file.FileSnapshot; import org.eclipse.jgit.lib.Config; @@ -93,6 +90,7 @@ private final MandatoryPluginsCollection mandatoryPlugins; private final UniversalServerPluginProvider serverPluginFactory; private final GerritRuntime gerritRuntime; + private final PluginOrderComparator pluginOrderComparator; @Inject public PluginLoader( @@ -122,6 +120,10 @@ mandatoryPlugins = mpc; this.gerritRuntime = gerritRuntime; + ImmutableList<String> pluginOrderOverrides = + ImmutableList.copyOf(cfg.getStringList("plugins", null, "loadPriority")); + pluginOrderComparator = new PluginOrderComparator(pluginOrderOverrides); + long checkFrequency = ConfigUtil.getTimeUnit( cfg, @@ -473,43 +475,7 @@ private TreeSet<Map.Entry<String, Path>> jarsApiFirstSortedPluginsSet( Map<String, Path> activePlugins) { - TreeSet<Map.Entry<String, Path>> sortedPlugins = - Sets.newTreeSet( - new Comparator<Map.Entry<String, Path>>() { - @Override - public int compare(Map.Entry<String, Path> e1, Map.Entry<String, Path> e2) { - Path n1 = e1.getValue().getFileName(); - Path n2 = e2.getValue().getFileName(); - - try { - boolean e1IsApi = isApi(e1.getValue()); - boolean e2IsApi = isApi(e2.getValue()); - return ComparisonChain.start() - .compareTrueFirst(e1IsApi, e2IsApi) - .compareTrueFirst(isJar(n1), isJar(n2)) - .compare(n1, n2) - .result(); - } catch (IOException ioe) { - logger.atSevere().withCause(ioe).log("Unable to compare %s and %s", n1, n2); - return 0; - } - } - - private boolean isJar(Path pluginPath) { - return pluginPath.toString().endsWith(".jar"); - } - - private boolean isApi(Path pluginPath) throws IOException { - return isJar(pluginPath) && hasApiModuleEntryInManifest(pluginPath); - } - - private boolean hasApiModuleEntryInManifest(Path pluginPath) throws IOException { - try (JarFile jarFile = new JarFile(pluginPath.toFile())) { - return !Strings.isNullOrEmpty( - jarFile.getManifest().getMainAttributes().getValue(ServerPlugin.API_MODULE)); - } - } - }); + TreeSet<Map.Entry<String, Path>> sortedPlugins = Sets.newTreeSet(pluginOrderComparator); addAllEntries(activePlugins, sortedPlugins); return sortedPlugins;
diff --git a/java/com/google/gerrit/server/plugins/PluginOrderComparator.java b/java/com/google/gerrit/server/plugins/PluginOrderComparator.java new file mode 100644 index 0000000..45ff9eb --- /dev/null +++ b/java/com/google/gerrit/server/plugins/PluginOrderComparator.java
@@ -0,0 +1,95 @@ +// Copyright (C) 2024 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.plugins; + +import com.google.common.base.Strings; +import com.google.common.collect.ComparisonChain; +import com.google.common.collect.ImmutableList; +import com.google.common.flogger.FluentLogger; +import java.io.IOException; +import java.nio.file.Path; +import java.util.Comparator; +import java.util.Map; +import java.util.jar.JarFile; +import java.util.jar.Manifest; + +class PluginOrderComparator implements Comparator<Map.Entry<String, Path>> { + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + private static final ManifestLoader DEFAULT_LOADER = + pluginPath -> { + try (JarFile jarFile = new JarFile(pluginPath.toFile())) { + return jarFile.getManifest(); + } + }; + + @FunctionalInterface + interface ManifestLoader { + Manifest load(Path pluginPath) throws IOException; + } + + private final ManifestLoader manifestLoader; + private final ImmutableList<String> pluginLoadOrderOverrides; + + PluginOrderComparator(ImmutableList<String> pluginLoadOrderOverrides) { + this(pluginLoadOrderOverrides, DEFAULT_LOADER); + } + + PluginOrderComparator( + ImmutableList<String> pluginLoadOrderOverrides, ManifestLoader manifestLoader) { + this.manifestLoader = manifestLoader; + this.pluginLoadOrderOverrides = pluginLoadOrderOverrides; + } + + @Override + public int compare(Map.Entry<String, Path> e1, Map.Entry<String, Path> e2) { + Path n1 = e1.getValue().getFileName(); + Path n2 = e2.getValue().getFileName(); + + try { + boolean e1IsApi = isApi(e1.getValue()); + boolean e2IsApi = isApi(e2.getValue()); + return ComparisonChain.start() + .compareTrueFirst(e1IsApi, e2IsApi) + .compareTrueFirst(isJar(n1), isJar(n2)) + .compare(loadOrderOverrides(e1.getKey()), loadOrderOverrides(e2.getKey())) + .compare(n1, n2) + .result(); + } catch (IOException ioe) { + logger.atSevere().withCause(ioe).log("Unable to compare %s and %s", n1, n2); + return 0; + } + } + + private boolean isJar(Path pluginPath) { + return pluginPath.toString().endsWith(".jar"); + } + + private boolean isApi(Path pluginPath) throws IOException { + return isJar(pluginPath) && hasApiModuleEntryInManifest(pluginPath); + } + + private boolean hasApiModuleEntryInManifest(Path pluginPath) throws IOException { + return !Strings.isNullOrEmpty( + manifestLoader.load(pluginPath).getMainAttributes().getValue(ServerPlugin.API_MODULE)); + } + + private int loadOrderOverrides(String pluginName) throws IOException { + int pluginNameIndex = pluginLoadOrderOverrides.indexOf(pluginName); + if (pluginNameIndex > -1) { + return pluginNameIndex - pluginLoadOrderOverrides.size(); + } + return 0; + } +}
diff --git a/java/com/google/gerrit/server/plugins/ServerPlugin.java b/java/com/google/gerrit/server/plugins/ServerPlugin.java index 340e431..bd83b98 100644 --- a/java/com/google/gerrit/server/plugins/ServerPlugin.java +++ b/java/com/google/gerrit/server/plugins/ServerPlugin.java
@@ -286,7 +286,7 @@ modules.add(new ServerPluginInfoModule(this, env.getServerMetrics())); return apiInjector .map(injector -> injector.createChildInjector(modules)) - .orElse(Guice.createInjector(modules)); + .orElseGet(() -> Guice.createInjector(modules)); } private Injector newRootInjectorWithApiModule(
diff --git a/javatests/com/google/gerrit/server/config/UserPreferencesConverterTest.java b/javatests/com/google/gerrit/server/config/UserPreferencesConverterTest.java index 621b06b..9888670 100644 --- a/javatests/com/google/gerrit/server/config/UserPreferencesConverterTest.java +++ b/javatests/com/google/gerrit/server/config/UserPreferencesConverterTest.java
@@ -70,6 +70,35 @@ } } + /** + * If this test fails, it's likely that you added a field to {@link GeneralPreferencesInfo}, or + * that you have changed the default value for such a field. Please update the {@link + * UserPreferences.GeneralPreferencesInfo} proto accordingly. + */ + @Test + public void generalPreferencesInfo_javaDefaultsKeptOnDoubleConversion() { + GeneralPreferencesInfo orig = GeneralPreferencesInfo.defaults(); + GeneralPreferencesInfo res = + GENERAL_PREFERENCES_INFO_CONVERTER.fromProto( + GENERAL_PREFERENCES_INFO_CONVERTER.toProto(orig)); + assertThat(res).isEqualTo(orig); + } + + /** + * If this test fails, it's likely that you added a field to {@link + * UserPreferences.GeneralPreferencesInfo}, or that you have changed the default value for such a + * field. Please update the {@link GeneralPreferencesInfo} class accordingly. + */ + @Test + public void generalPreferencesInfo_protoDefaultsKeptOnDoubleConversion() { + UserPreferences.GeneralPreferencesInfo orig = + UserPreferences.GeneralPreferencesInfo.getDefaultInstance(); + UserPreferences.GeneralPreferencesInfo res = + GENERAL_PREFERENCES_INFO_CONVERTER.toProto( + GENERAL_PREFERENCES_INFO_CONVERTER.fromProto(orig)); + assertThat(res).isEqualTo(orig); + } + @Test public void generalPreferencesInfo_doubleConversionWithAllFieldsSet() { UserPreferences.GeneralPreferencesInfo originalProto = @@ -209,6 +238,33 @@ } } + /** + * If this test fails, it's likely that you added a field to {@link DiffPreferencesInfo}, or that + * you have changed the default value for such a field. Please update the {@link + * UserPreferences.DiffPreferencesInfo} proto accordingly. + */ + @Test + public void diffPreferencesInfo_javaDefaultsKeptOnDoubleConversion() { + DiffPreferencesInfo orig = DiffPreferencesInfo.defaults(); + DiffPreferencesInfo res = + DIFF_PREFERENCES_INFO_CONVERTER.fromProto(DIFF_PREFERENCES_INFO_CONVERTER.toProto(orig)); + assertThat(res).isEqualTo(orig); + } + + /** + * If this test fails, it's likely that you added a field to {@link + * UserPreferences.DiffPreferencesInfo}, or that you have changed the default value for such a + * field. Please update the {@link DiffPreferencesInfo} class accordingly. + */ + @Test + public void diffPreferencesInfo_protoDefaultsKeptOnDoubleConversion() { + UserPreferences.DiffPreferencesInfo orig = + UserPreferences.DiffPreferencesInfo.getDefaultInstance(); + UserPreferences.DiffPreferencesInfo res = + DIFF_PREFERENCES_INFO_CONVERTER.toProto(DIFF_PREFERENCES_INFO_CONVERTER.fromProto(orig)); + assertThat(res).isEqualTo(orig); + } + @Test public void diffPreferencesInfo_doubleConversionWithAllFieldsSet() { UserPreferences.DiffPreferencesInfo originalProto = @@ -290,6 +346,33 @@ } } + /** + * If this test fails, it's likely that you added a field to {@link EditPreferencesInfo}, or that + * you have changed the default value for such a field. Please update the {@link + * UserPreferences.EditPreferencesInfo} proto accordingly. + */ + @Test + public void editPreferencesInfo_javaDefaultsKeptOnDoubleConversion() { + EditPreferencesInfo orig = EditPreferencesInfo.defaults(); + EditPreferencesInfo res = + EDIT_PREFERENCES_INFO_CONVERTER.fromProto(EDIT_PREFERENCES_INFO_CONVERTER.toProto(orig)); + assertThat(res).isEqualTo(orig); + } + + /** + * If this test fails, it's likely that you added a field to {@link + * UserPreferences.EditPreferencesInfo}, or that you have changed the default value for such a + * field. Please update the {@link EditPreferencesInfo} class accordingly. + */ + @Test + public void editPreferencesInfo_protoDefaultsKeptOnDoubleConversion() { + UserPreferences.EditPreferencesInfo orig = + UserPreferences.EditPreferencesInfo.getDefaultInstance(); + UserPreferences.EditPreferencesInfo res = + EDIT_PREFERENCES_INFO_CONVERTER.toProto(EDIT_PREFERENCES_INFO_CONVERTER.fromProto(orig)); + assertThat(res).isEqualTo(orig); + } + @Test public void editPreferencesInfo_doubleConversionWithAllFieldsSet() { UserPreferences.EditPreferencesInfo originalProto =
diff --git a/javatests/com/google/gerrit/server/plugins/PluginOrderComparatorTest.java b/javatests/com/google/gerrit/server/plugins/PluginOrderComparatorTest.java new file mode 100644 index 0000000..4f7402a --- /dev/null +++ b/javatests/com/google/gerrit/server/plugins/PluginOrderComparatorTest.java
@@ -0,0 +1,142 @@ +// Copyright (C) 2024 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.plugins; + +import static com.google.common.truth.Truth.assertThat; +import static java.nio.charset.StandardCharsets.UTF_8; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Sets; +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.List; +import java.util.Map; +import java.util.TreeSet; +import java.util.jar.Manifest; +import org.junit.Test; + +public class PluginOrderComparatorTest { + private static final String API_MODULE = "Gerrit-ApiModule: com.google.gerrit.UnitTest"; + + private static final Path FIRST_PLUGIN_PATH = Paths.get("01-first.jar"); + private static final Path SECOND_PLUGIN_PATH = Paths.get("20-second.jar"); + private static final Path THIRD_PLUGIN_PATH = Paths.get("30-third.jar"); + private static final Path LAST_PLUGIN_PATH = Paths.get("99-last.jar"); + + private static final Map.Entry<String, Path> FIRST_ENTRY = Map.entry("first", FIRST_PLUGIN_PATH); + private static final Map.Entry<String, Path> SECOND_ENTRY = + Map.entry("second", SECOND_PLUGIN_PATH); + private static final Map.Entry<String, Path> THIRD_ENTRY = Map.entry("third", THIRD_PLUGIN_PATH); + private static final Map.Entry<String, Path> LAST_ENTRY = Map.entry("last", LAST_PLUGIN_PATH); + + private static final Manifest EMPTY_MANIFEST = newManifest(""); + private static final Manifest API_MODULE_MANIFEST = newManifest(API_MODULE); + + @Test + public void shouldOrderPluginsBasedOnFileName() { + PluginOrderComparator comparator = + new PluginOrderComparator(ImmutableList.of(), pluginPath -> EMPTY_MANIFEST); + + assertOrder(comparator, List.of(FIRST_ENTRY, LAST_ENTRY), List.of(FIRST_ENTRY, LAST_ENTRY)); + } + + @Test + public void shouldReturnPluginWithApiModuleFirst() { + // return empty manifest for the first plugin and manifest with ApiModule for the last + PluginOrderComparator.ManifestLoader loader = customLoader(EMPTY_MANIFEST, API_MODULE_MANIFEST); + + PluginOrderComparator comparator = new PluginOrderComparator(ImmutableList.of(), loader); + + assertOrder(comparator, List.of(FIRST_ENTRY, LAST_ENTRY), List.of(LAST_ENTRY, FIRST_ENTRY)); + } + + @Test + public void shouldUseOrderFromOverrideListBasedOnFileName() { + ImmutableList<String> pluginOrderOverrides = ImmutableList.of("last", "first"); + + PluginOrderComparator comparator = + new PluginOrderComparator(pluginOrderOverrides, pluginPath -> EMPTY_MANIFEST); + + assertOrder(comparator, List.of(FIRST_ENTRY, LAST_ENTRY), List.of(LAST_ENTRY, FIRST_ENTRY)); + } + + @Test + public void shouldCombineOrderOverridesAndNaturalFileNaming() { + ImmutableList<String> pluginOrderOverrides = ImmutableList.of("third", "second"); + + PluginOrderComparator comparator = + new PluginOrderComparator(pluginOrderOverrides, pluginPath -> EMPTY_MANIFEST); + + assertOrder( + comparator, + List.of(FIRST_ENTRY, SECOND_ENTRY, THIRD_ENTRY, LAST_ENTRY), + List.of(THIRD_ENTRY, SECOND_ENTRY, FIRST_ENTRY, LAST_ENTRY)); + } + + @Test + public void shouldReturnApiModuleFirstWithOrderOverrides() { + ImmutableList<String> pluginOrderOverrides = ImmutableList.of("third", "second"); + PluginOrderComparator.ManifestLoader loader = + pluginPath -> { + if (pluginPath.equals(LAST_PLUGIN_PATH)) { + return API_MODULE_MANIFEST; + } + return EMPTY_MANIFEST; + }; + + PluginOrderComparator comparator = new PluginOrderComparator(pluginOrderOverrides, loader); + + assertOrder( + comparator, + List.of(FIRST_ENTRY, SECOND_ENTRY, THIRD_ENTRY, LAST_ENTRY), + List.of(LAST_ENTRY, THIRD_ENTRY, SECOND_ENTRY, FIRST_ENTRY)); + } + + private void assertOrder( + PluginOrderComparator comparator, + List<Map.Entry<String, Path>> input, + List<Map.Entry<String, Path>> expected) { + TreeSet<Map.Entry<String, Path>> actual = Sets.newTreeSet(comparator); + actual.addAll(input); + + assertThat(List.copyOf(actual)).isEqualTo(expected); + } + + private static Manifest newManifest(String content) { + String withEmptyLine = content + "\n"; + try { + Manifest manifest = new Manifest(); + manifest.read(new ByteArrayInputStream(withEmptyLine.getBytes(UTF_8))); + return manifest; + } catch (IOException e) { + throw new RuntimeException(e); + } + } + + private PluginOrderComparator.ManifestLoader customLoader( + Manifest firstManifest, Manifest secondManifest) { + return pluginPath -> { + if (pluginPath.equals(FIRST_PLUGIN_PATH)) { + return firstManifest; + } + if (pluginPath.equals(LAST_PLUGIN_PATH)) { + return secondManifest; + } + throw new IllegalArgumentException("unsupported path: " + pluginPath); + }; + } +}
diff --git a/plugins/codemirror-editor b/plugins/codemirror-editor index 3afc9b3..06719ab 160000 --- a/plugins/codemirror-editor +++ b/plugins/codemirror-editor
@@ -1 +1 @@ -Subproject commit 3afc9b32d26c761d67b21256792720d5c8916b43 +Subproject commit 06719abee6c38d17008599f074050db5153bffa3
diff --git a/plugins/package.json b/plugins/package.json index 6aa106f..37b3dda 100644 --- a/plugins/package.json +++ b/plugins/package.json
@@ -28,7 +28,7 @@ "@codemirror/search": "^6.5.6", "@codemirror/state": "^6.4.1", "@codemirror/view": "^6.26.3", - "@gerritcodereview/typescript-api": "3.8.0", + "@gerritcodereview/typescript-api": "3.9.1", "@open-wc/testing": "^3.2.2", "@polymer/decorators": "^3.0.0", "@polymer/polymer": "^3.5.1",
diff --git a/plugins/yarn.lock b/plugins/yarn.lock index 128f7c2..d19952c 100644 --- a/plugins/yarn.lock +++ b/plugins/yarn.lock
@@ -472,10 +472,10 @@ dependencies: "@types/chai" "^4.2.12" -"@gerritcodereview/typescript-api@3.8.0": - version "3.8.0" - resolved "https://registry.yarnpkg.com/@gerritcodereview/typescript-api/-/typescript-api-3.8.0.tgz#2e418b814d7451c40365b2dc4f88e9965ece0769" - integrity sha512-wUkIWUx99Rj1vxRYQISxyzN0nplqu7t5sRDyJ8R3yNNkvALQAMC6Whj63qzCsZsymVFzC5up3y+ZVxaeh7b+xA== +"@gerritcodereview/typescript-api@3.9.1": + version "3.9.1" + resolved "https://registry.yarnpkg.com/@gerritcodereview/typescript-api/-/typescript-api-3.9.1.tgz#c532e9a39e3d5f16f6a5cb5691988c04cd477531" + integrity sha512-5t8CBhlqQEcjJqNld1/ajcdZjjyrv7vsn4u0N3mX4hc4DnPJimIjYVFvP8nLyhSkioawWDIRLvzlmfzFs02lDg== "@jridgewell/resolve-uri@^3.1.0": version "3.1.2"
diff --git a/polygerrit-ui/app/api/suggestions.ts b/polygerrit-ui/app/api/suggestions.ts index b4158be..c3089a9 100644 --- a/polygerrit-ui/app/api/suggestions.ts +++ b/polygerrit-ui/app/api/suggestions.ts
@@ -27,7 +27,21 @@ lineNumber?: number; } +export declare interface AutocompleteCommentRequest { + id: string; + commentText: string; + changeInfo: ChangeInfo; + patchsetNumber: RevisionPatchSetNum; + filePath: string; + range?: CommentRange; + lineNumber?: number; +} + export declare interface SuggestionsProvider { + autocompleteComment?( + req: AutocompleteCommentRequest + ): Promise<AutocompleteCommentResponse>; + /** * Gerrit calls these methods when ... * - ... user types a comment draft @@ -55,6 +69,11 @@ supportedFileExtensions?: string[]; } +export declare interface AutocompleteCommentResponse { + responseCode: ResponseCode; + completion?: string; +} + export declare interface SuggestCodeResponse { responseCode: ResponseCode; suggestions: Suggestion[];
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 58c10f9..ae59fb6 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
@@ -205,16 +205,21 @@ nav { align-items: center; display: flex; - flex-wrap: wrap; } .bigTitle { color: var(--header-text-color); font-size: var(--header-title-font-size); + line-height: calc(var(--header-title-font-size) * 1.2); text-decoration: none; } .bigTitle:hover { text-decoration: underline; } + .titleText { + /* Vertical alignment of icons and text with just block/inline display is too troublesome. */ + display: flex; + align-items: center; + } .titleText::before { --icon-width: var(--header-icon-width, var(--header-icon-size, 0)); --icon-height: var(--header-icon-height, var(--header-icon-size, 0)); @@ -222,14 +227,15 @@ background-size: var(--icon-width) var(--icon-height); background-repeat: no-repeat; content: ''; - display: inline-block; + /* Any direct child of a flex element implicitly has 'display: block', but let's make that explicit here. */ + display: block; + width: var(--icon-width); height: var(--icon-height); /* If size or height are set, then use 'spacing-m', 0px otherwise. */ margin-right: clamp(0px, var(--icon-height), var(--spacing-m)); - vertical-align: text-bottom; - width: var(--icon-width); } .titleText::after { + /* The height will be determined by the line-height of the .bigTitle element. */ content: var(--header-title-content); white-space: nowrap; } @@ -301,6 +307,7 @@ display: inline; } .accountContainer { + flex: 0 0 auto; align-items: center; display: flex; margin: 0 calc(0 - var(--spacing-m)) 0 var(--spacing-m); @@ -368,7 +375,7 @@ <nav> <a href=${`//${window.location.host}${getBaseUrl()}/`} class="bigTitle"> <gr-endpoint-decorator name="header-title"> - <span class="titleText"></span> + <div class="titleText"></div> </gr-endpoint-decorator> </a> <ul class="links">
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 dfb44b70..40430fb 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
@@ -42,7 +42,7 @@ <nav> <a class="bigTitle" href="//localhost:9876/"> <gr-endpoint-decorator name="header-title"> - <span class="titleText"> </span> + <div class="titleText"></div> </gr-endpoint-decorator> </a> <ul class="links">
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts index 3509146..c27ca5b 100644 --- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts +++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -93,6 +93,7 @@ // visible for testing export const AUTO_SAVE_DEBOUNCE_DELAY_MS = 2000; export const GENERATE_SUGGESTION_DEBOUNCE_DELAY_MS = 500; +export const AUTOCOMPLETE_DEBOUNCE_DELAY_MS = 200; export const ENABLE_GENERATE_SUGGESTION_STORAGE_KEY = 'enableGenerateSuggestionStorageKeyForCommentWithId-'; @@ -219,6 +220,12 @@ @state() messageText = ''; + /** + * An hint for autocompleting the comment message from plugin suggestion + * providers. + */ + @state() autocompleteHint = ''; + /* The 'dirty' state of !comment.unresolved, which will be saved on demand. */ @state() unresolved = true; @@ -300,6 +307,12 @@ private generateSuggestionTrigger$ = new Subject(); /** + * This is triggered when the user types into the editing textarea. We then + * debounce it and call autocompleteComment(). + */ + private autocompleteTrigger$ = new Subject(); + + /** * Set to the content of DraftInfo when entering editing mode. * Only used for "Cancel". */ @@ -392,6 +405,23 @@ () => this.getConfigModel().docsBaseUrl$, docsBaseUrl => (this.docsBaseUrl = docsBaseUrl) ); + subscribe( + this, + () => this.getPluginLoader().pluginsModel.suggestionsPlugins$, + // We currently support results from only 1 provider. + suggestionsPlugins => + (this.suggestionsProvider = suggestionsPlugins?.[0]?.provider) + ); + subscribe( + this, + () => + this.autocompleteTrigger$.pipe( + debounceTime(AUTOCOMPLETE_DEBOUNCE_DELAY_MS) + ), + () => { + this.autocompleteComment(); + } + ); if ( this.flagsService.isEnabled(KnownExperimentId.ML_SUGGESTED_EDIT) || this.flagsService.isEnabled(KnownExperimentId.ML_SUGGESTED_EDIT_V2) @@ -424,15 +454,6 @@ override connectedCallback() { super.connectedCallback(); - this.getPluginLoader() - .awaitPluginsLoaded() - .then(() => { - const suggestionsPlugins = - this.getPluginLoader().pluginsModel.getState().suggestionsPlugins; - // We currently support results from only 1 provider. - this.suggestionsProvider = suggestionsPlugins?.[0]?.provider; - }); - if (this.comment?.id) { const generateSuggestionStoredContent = this.getStorage().getEditableContentItem( @@ -872,19 +893,51 @@ rows="4" .placeholder=${this.messagePlaceholder} text=${this.messageText} - @text-changed=${(e: ValueChangedEvent) => { - // TODO: This is causing a re-render of <gr-comment> on every key - // press. Try to avoid always setting `this.messageText` or at least - // debounce it. Most of the code can just inspect the current value - // of the textare instead of needing a dedicated property. - this.messageText = e.detail.value; - this.autoSaveTrigger$.next(); - this.generateSuggestionTrigger$.next(); - }} + autocompleteHint=${this.autocompleteHint} + @text-changed=${this.handleTextChanged} ></gr-suggestion-textarea> `; } + private handleTextChanged(e: ValueChangedEvent) { + const oldValue = this.messageText; + const newValue = e.detail.value; + if (oldValue === newValue) return; + // TODO: This is causing a re-render of <gr-comment> on every key + // press. Try to avoid always setting `this.messageText` or at least + // debounce it. Most of the code can just inspect the current value + // of the textare instead of needing a dedicated property. + this.messageText = newValue; + + this.handleTextChangedForAutocomplete(oldValue, newValue); + this.autoSaveTrigger$.next(); + this.generateSuggestionTrigger$.next(); + } + + // visible for testing + handleTextChangedForAutocomplete(oldValue: string, newValue: string) { + if (oldValue === newValue) return; + // As soon as the user changes the text the hint for autocompletion + // is invalidated, *if* what the user typed does not match the + // autocompletion! + const charsAdded = newValue.length - oldValue.length; + if ( + charsAdded > 0 && + newValue.startsWith(oldValue) && + this.autocompleteHint.startsWith(newValue.substring(oldValue.length)) + ) { + // What the user typed matches the hint, so we keep the hint, but shorten + // it accordingly. + this.autocompleteHint = this.autocompleteHint.substring(charsAdded); + return; + } + + // The default behavior is to reset the hint and to generate a new + // autocomplete suggestion. + this.autocompleteHint = ''; + this.autocompleteTrigger$.next(); + } + private renderCommentMessage() { if (this.collapsed || this.editing) return; @@ -902,6 +955,7 @@ private renderCopyLinkIcon() { // Only show the icon when the thread contains a published comment. if (!this.comment?.in_reply_to && isDraft(this.comment)) return; + if (this.editing) return; return html` <gr-icon icon="link" @@ -945,6 +999,7 @@ ${this.renderDiscardButton()} ${this.renderEditButton()} ${this.renderCancelButton()} ${this.renderSaveButton()} ${this.renderCopyLinkIcon()} + <gr-endpoint-slot name="draft-actions-end"></gr-endpoint-slot> </div> `; } @@ -1286,6 +1341,39 @@ } } + private async autocompleteComment() { + const enabled = this.flagsService.isEnabled( + KnownExperimentId.COMMENT_AUTOCOMPLETION + ); + const suggestionsProvider = this.suggestionsProvider; + const change = this.getChangeModel().getChange(); + if ( + !enabled || + !suggestionsProvider?.autocompleteComment || + !change || + !this.comment?.patch_set || + !this.comment.path || + this.messageText.length === 0 + ) { + return; + } + const commentText = this.messageText; + const response = await suggestionsProvider.autocompleteComment({ + id: id(this.comment), + commentText, + changeInfo: change as ChangeInfo, + patchsetNumber: this.comment?.patch_set, + filePath: this.comment.path, + range: this.comment.range, + lineNumber: this.comment.line, + }); + // If between request and response the user has changed the message, then + // ignore the suggestion for the old message text. + if (this.messageText !== commentText) return; + if (!response?.completion) return; + this.autocompleteHint = response.completion; + } + private renderRobotActions() { if (!this.account || !isRobot(this.comment)) return; const endpoint = html`
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_test.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_test.ts index 701052d..7b4f63a 100644 --- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_test.ts +++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment_test.ts
@@ -342,6 +342,8 @@ > Edit </gr-button> + <gr-endpoint-slot name="draft-actions-end"> + </gr-endpoint-slot> </div> </div> </div> @@ -408,6 +410,7 @@ <div class="body"> <gr-suggestion-textarea autocomplete="on" + autocompletehint="" class="code editMessage" code="" id="editTextarea" @@ -444,6 +447,8 @@ > Save </gr-button> + <gr-endpoint-slot name="draft-actions-end"> + </gr-endpoint-slot> </div> </div> </div> @@ -891,6 +896,32 @@ }); }); + suite('handleTextChangedForAutocomplete', () => { + test('foo -> foo with asdf', async () => { + element.autocompleteHint = 'asdf'; + element.handleTextChangedForAutocomplete('foo', 'foo'); + assert.equal(element.autocompleteHint, 'asdf'); + }); + + test('foo -> bar with asdf', async () => { + element.autocompleteHint = 'asdf'; + element.handleTextChangedForAutocomplete('foo', 'bar'); + assert.equal(element.autocompleteHint, ''); + }); + + test('foo -> foofoo with asdf', async () => { + element.autocompleteHint = 'asdf'; + element.handleTextChangedForAutocomplete('foo', 'foofoo'); + assert.equal(element.autocompleteHint, ''); + }); + + test('foo -> foofoo with foomore', async () => { + element.autocompleteHint = 'foomore'; + element.handleTextChangedForAutocomplete('foo', 'foofoo'); + assert.equal(element.autocompleteHint, 'more'); + }); + }); + suite('suggest edit', () => { let element: GrComment; setup(async () => {
diff --git a/polygerrit-ui/app/elements/shared/gr-suggestion-textarea/gr-suggestion-textarea.ts b/polygerrit-ui/app/elements/shared/gr-suggestion-textarea/gr-suggestion-textarea.ts index 04f66a7..2d22f6a 100644 --- a/polygerrit-ui/app/elements/shared/gr-suggestion-textarea/gr-suggestion-textarea.ts +++ b/polygerrit-ui/app/elements/shared/gr-suggestion-textarea/gr-suggestion-textarea.ts
@@ -143,6 +143,7 @@ .placeholder=${this.el.placeholder} ?disabled=${this.el.disabled} .value=${this.el.text} + .hint=${this.el.autocompleteHint} @input=${(e: InputEvent) => { const value = (e.target as GrTextarea).value; this.el.text = value ?? ''; @@ -227,6 +228,12 @@ standard monospace font. */ @property({type: Boolean}) code = false; + /** + * An autocompletion hint that is passed to <gr-textarea>, which will allow\ + * the user to accept it by pressing tab. + */ + @property({type: String}) autocompleteHint = ''; + @state() suggestions: (Item | EmojiSuggestion)[] = []; // Accessed in tests.
diff --git a/polygerrit-ui/app/models/plugins/plugins-model.ts b/polygerrit-ui/app/models/plugins/plugins-model.ts index 8e9bead..880bcd0 100644 --- a/polygerrit-ui/app/models/plugins/plugins-model.ts +++ b/polygerrit-ui/app/models/plugins/plugins-model.ts
@@ -89,6 +89,11 @@ public coveragePlugins$ = select(this.state$, state => state.coveragePlugins); + public suggestionsPlugins$ = select( + this.state$, + state => state.suggestionsPlugins + ); + public pluginsLoaded$ = select(this.state$, state => state.pluginsLoaded); constructor() {
diff --git a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts index 5228bc9..0be3ab5 100644 --- a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts +++ b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api-impl.ts
@@ -2118,7 +2118,9 @@ : this._getFileInRevision(changeNum, path, patchNum, suppress404s); return promise.then(res => { - if (!res.ok) { + // A 204 is returned if the file is empty so we have + // to return early. + if (!res.ok || res.status === 204) { return res; }
diff --git a/polygerrit-ui/app/utils/dom-util.ts b/polygerrit-ui/app/utils/dom-util.ts index d557273..04e8c19 100644 --- a/polygerrit-ui/app/utils/dom-util.ts +++ b/polygerrit-ui/app/utils/dom-util.ts
@@ -437,7 +437,7 @@ if (!isElementTarget(rootTarget)) return false; const tagName = rootTarget.tagName; const type = rootTarget.getAttribute('type'); - const editable = rootTarget.hasAttribute('contenteditable'); + const editable = !!(rootTarget as HTMLElement).isContentEditable; if ( editable ||
diff --git a/proto/BUILD b/proto/BUILD index 7aa761d..624568b 100644 --- a/proto/BUILD +++ b/proto/BUILD
@@ -23,3 +23,9 @@ visibility = ["//visibility:public"], deps = [":entities_proto"], ) + +cc_proto_library( + name = "entities_cc_proto", + visibility = ["//visibility:public"], + deps = [":entities_proto"], +)
diff --git a/tools/BUILD b/tools/BUILD index 71ad096..ed55a4f 100644 --- a/tools/BUILD +++ b/tools/BUILD
@@ -7,17 +7,6 @@ exports_files(["nongoogle.bzl"]) -default_java_toolchain( - name = "error_prone_warnings_toolchain_java11", - configuration = NONPREBUILT_TOOLCHAIN_CONFIGURATION, - package_configuration = [ - ":error_prone", - ], - source_version = "11", - target_version = "11", - visibility = ["//visibility:public"], -) - [default_java_toolchain( name = "error_prone_warnings_toolchain_java" + VERSION, configuration = dict(),
diff --git a/tools/defs.bzl b/tools/defs.bzl index ff207b3..672c6f9 100644 --- a/tools/defs.bzl +++ b/tools/defs.bzl
@@ -6,8 +6,6 @@ """ protobuf_deps() - native.register_toolchains("//tools:error_prone_warnings_toolchain_java11_definition") - native.register_toolchains("//tools:error_prone_warnings_toolchain_java17_definition") native.register_toolchains("//tools:error_prone_warnings_toolchain_java21_definition")
diff --git a/tools/eclipse/project.py b/tools/eclipse/project.py index a3f4d8f..1c2443e 100755 --- a/tools/eclipse/project.py +++ b/tools/eclipse/project.py
@@ -49,7 +49,7 @@ opts.add_argument('-b', '--batch', action='store_true', dest='batch', help='Bazel batch option') opts.add_argument('-j', '--java', action='store', - dest='java', help='Post Java 11') + dest='java', help='Post Java 17') opts.add_argument('--bazel', help=('name of the bazel executable. Defaults to using' ' bazelisk if found, or bazel if bazelisk is not' @@ -97,6 +97,7 @@ cmd.append(arg) if custom_java: cmd.append('--config=java%s' % custom_java) + cmd.append('--remote_download_outputs=all') return cmd