Merge "ChangeInfo: Remove outdated hint about counts possibly not being populated"
diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt
index 3e75a53..7d531ff 100644
--- a/Documentation/access-control.txt
+++ b/Documentation/access-control.txt
@@ -97,7 +97,7 @@
groups are created on Gerrit site initialization and unique UUIDs are assigned
to those groups. These UUIDs are different on different Gerrit sites.
-Gerrit comes with two predefined groups:
+Gerrit comes with three predefined groups:
* link:#administrators[Administrators]
* link:#service_users[Service Users]
diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt
index bcc96b4..4fa5e68 100644
--- a/Documentation/dev-plugins.txt
+++ b/Documentation/dev-plugins.txt
@@ -623,6 +623,105 @@
.to(MyListener.class);
----
+Blocking inside onStop() is a good choice for QOS limits which are
+attempting to restrict total usage of resources as might be done to
+to prevent a server overload. In these cases, when a server's resources
+are being exhausted, it is important to throttle all `Tasks`, and blocking
+the current thread from being used by any Task makes sense. However,
+Task parking (see below) is more appropriate if it desirable to limit a
+specific resource usage in favor of other resources, such as when
+prioritization or fairness policies are desired.
+
+[[taskParker]]
+== TaskParkers
+
+It is possible to park `com.google.gerrit.server.git.WorkQueue$Task`s
+before they run without depriving other `Tasks` of a thread. Parking is
+particularly useful for (de-)prioritizing certain `Tasks` based on resource
+quotas without blocking `Tasks` not using those resources. For example,
+when there is a desire to limit how many commands a single user can run
+concurrently it is typically also desirable to not limit the total amount
+of concurrently running commands to the same limit. The Task parking
+mechanism is useful for such Task limiting scenarios.
+
+The `TaskParker` interface works well with a Semaphore's `tryAcquire()`
+method even when the Semaphore is unavailable. However, blocking should
+not be done with a `TaskParker` and if it is desired, such as when using a
+Semaphore's `acquire()` method, use a `TaskListener` interface instead.
+
+To make use of Task parking, implement a
+`com.google.gerrit.server.git.WorkQueue$TaskParker` and register the
+TaskParker (as a TaskListener) from a plugin like this:
+
+[source,java]
+----
+ public class MyParker implements TaskParker {
+ Semaphore semaphore = new Semaphore(3);
+
+ @Override
+ public boolean isReadyToStart(Task<?> task) {
+ try {
+ return semaphore.tryAcquire();
+ } catch (InterruptedException e) {
+ return false;
+ }
+ }
+
+ @Override
+ public void onNotReadyToStart(Task<?> task) {
+ semaphore.release();
+ }
+
+ @Override
+ public void onStart(Task<?> task) {}
+
+ @Override
+ public void onStop(Task<?> task) {
+ semaphore.release();
+ }
+ }
+
+ bind(TaskListener.class)
+ .annotatedWith(Exports.named("MyParker"))
+ .to(MyParker.class);
+----
+
+Before running a Task, the executor will query each `TaskParker` to see
+if the Task may be run by calling `isReadyToStart()`. If any `TaskParker`
+returns `false` from `isReadyToStart()`, then the Task will get parked
+and the executor will wait until another Task completes before
+attempting to run a parked task again.
+
+Since parked `Tasks` are not actually running and consuming resources,
+they generally should also not be contributing towards those resource
+quotas which caused the task to be parked. For this reason, once it is
+determined that a Task will be parked, the executor will call
+`onNotReadyToStart()` on every `TaskParker` that previously returned `true`
+from `isReadyToStart()`. This allows those TaskParkers to reduce their
+resource usage counts which they bumped up in `isReadyToStart()` with
+the expectation that the Task may run. Since the Task is not running and
+the resource is not being used, reducing the resource usage count allows
+other `Tasks` needing that resource to run while the Task is parked.
+
+Once a running Task completes, the executor will attempt to run
+parked `Tasks` (in the order in which they were parked) by again calling
+`isReadyToStart()` on the TaskParkers, even the TaskParkers which
+previously returned a `true` before the Task was parked. This is
+necessary because although a Task may not have exceeded a specific
+resource limit before it was parked, another Task may since have been
+allowed to run and its usage of that resource may now cause the parked
+task under evaluation to need to be throttled and parked again.
+
+Note, the reason that it is important to not block inside the
+`isReadyToStart()` method is to avoid delaying the executor from calling
+`onNotReadyToStart()` on other TaskParkers holding resources, as this
+would prevent them from freeing those resources. Also, just as it is
+important to later release any resources acquired within
+`isReadyToStart()` in `onStop()`, it is even more important to release
+those resources in `onNotReadyToStart()` since `isReadyToStart()` may
+be called many times per `TaskParker`, but `onStop()` will only ever be
+be called once.
+
[[change-message-modifier]]
== Change Message Modifier
diff --git a/Documentation/js_licenses.txt b/Documentation/js_licenses.txt
index 8b6049e..ab77d1b 100644
--- a/Documentation/js_licenses.txt
+++ b/Documentation/js_licenses.txt
@@ -1137,6 +1137,45 @@
----
+[[highlightjs-epp]]
+highlightjs-epp
+
+* highlightjs-epp
+
+[[highlightjs-epp_license]]
+----
+BSD 3-Clause License
+
+Copyright (c) 2024, highlight.js
+
+Redistribution and use in source and binary forms, with or without
+modification, are permitted provided that the following conditions are met:
+
+1. Redistributions of source code must retain the above copyright notice, this
+ list of conditions and the following disclaimer.
+
+2. Redistributions in binary form must reproduce the above copyright notice,
+ this list of conditions and the following disclaimer in the documentation
+ and/or other materials provided with the distribution.
+
+3. Neither the name of the copyright holder nor the names of its
+ contributors may be used to endorse or promote products derived from
+ this software without specific prior written permission.
+
+THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE
+FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+----
+
+
[[highlightjs-structured-text]]
highlightjs-structured-text
diff --git a/Documentation/licenses.txt b/Documentation/licenses.txt
index 7e3fa9a..91b6227 100644
--- a/Documentation/licenses.txt
+++ b/Documentation/licenses.txt
@@ -4021,6 +4021,45 @@
----
+[[highlightjs-epp]]
+highlightjs-epp
+
+* highlightjs-epp
+
+[[highlightjs-epp_license]]
+----
+BSD 3-Clause License
+
+Copyright (c) 2024, highlight.js
+
+Redistribution and use in source and binary forms, with or without
+modification, are permitted provided that the following conditions are met:
+
+1. Redistributions of source code must retain the above copyright notice, this
+ list of conditions and the following disclaimer.
+
+2. Redistributions in binary form must reproduce the above copyright notice,
+ this list of conditions and the following disclaimer in the documentation
+ and/or other materials provided with the distribution.
+
+3. Neither the name of the copyright holder nor the names of its
+ contributors may be used to endorse or promote products derived from
+ this software without specific prior written permission.
+
+THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE
+FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+----
+
+
[[highlightjs-structured-text]]
highlightjs-structured-text
diff --git a/Documentation/rest-api-accounts.txt b/Documentation/rest-api-accounts.txt
index d0c4553..384585f 100644
--- a/Documentation/rest-api-accounts.txt
+++ b/Documentation/rest-api-accounts.txt
@@ -1317,6 +1317,7 @@
"work_in_progress_by_default": true,
"allow_browser_notifications": true,
"allow_suggest_code_while_commenting": true,
+ "allow_autocompleting_comments": true,
"diff_page_sidebar": "plugin-foo",
"default_base_for_merges": "FIRST_PARENT",
"my": [
@@ -1372,6 +1373,7 @@
"disable_token_highlighting": true,
"allow_browser_notifications": false,
"allow_suggest_code_while_commenting": false,
+ "allow_autocompleting_comments": false,
"diff_page_sidebar": "NONE",
"diff_view": "SIDE_BY_SIDE",
"mute_common_path_prefixes": true,
@@ -2723,6 +2725,9 @@
|`allow_suggest_code_while_commenting` |not set if `false`|
Whether to receive suggested code while writing comments. This feature needs
a plugin implementation.
+|`allow_autocompleting_comments` |not set if `false`|
+Whether to receive autocompletions while writing comments. This feature needs
+a plugin implementation.
|`diff_page_sidebar` |optional|
String indicating which sidebar should be open on the diff page. Set to "NONE"
if no sidebars should be open. Plugin-supplied sidebars will be prefixed with
@@ -2801,6 +2806,9 @@
|`allow_suggest_code_while_commenting` |not set if `false`|
Whether to receive suggested code while writing comments. This feature needs
a plugin implementation.
+|`allow_autocompleting_comments` |not set if `false`|
+Whether to receive autocompletions while writing comments. This feature needs
+a plugin implementation.
|`diff_page_sidebar` |optional|
String indicating which sidebar should be open on the diff page. Set to "NONE"
if no sidebars should be open. Plugin-supplied sidebars will be prefixed with
diff --git a/Documentation/user-notify.txt b/Documentation/user-notify.txt
index f420fe7..2da3802 100644
--- a/Documentation/user-notify.txt
+++ b/Documentation/user-notify.txt
@@ -245,11 +245,11 @@
comments had been posted in that notification using "Yes" or "No", for
example `Gerrit-HasComments: Yes`.
-[[Gerrit-HasLabels]]Gerrit-HasLabels::
+[[Gerrit-Has-Labels]]Gerrit-Has-Labels::
In comment emails, the has-labels footer states whether label votes had
been posted in that notification using "Yes" or "No", for
-example `Gerrit-HasLabels: No`.
+example `Gerrit-Has-Labels: No`.
[[Gerrit-Comment-In-Reply-To]]Gerrit-Comment-In-Reply-To::
diff --git a/java/com/google/gerrit/entities/Account.java b/java/com/google/gerrit/entities/Account.java
index efbac97..4b7694a 100644
--- a/java/com/google/gerrit/entities/Account.java
+++ b/java/com/google/gerrit/entities/Account.java
@@ -22,6 +22,7 @@
import com.google.common.base.MoreObjects;
import com.google.common.primitives.Ints;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
+import com.google.gerrit.common.ConvertibleToProto;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.client.DiffPreferencesInfo;
import java.time.Instant;
@@ -58,6 +59,7 @@
/** Key local to Gerrit to identify a user. */
@AutoValue
+ @ConvertibleToProto
public abstract static class Id implements Comparable<Id> {
/** Parse an Account.Id out of a string representation. */
public static Optional<Id> tryParse(String str) {
diff --git a/java/com/google/gerrit/entities/Change.java b/java/com/google/gerrit/entities/Change.java
index fad3aa8..51585f3 100644
--- a/java/com/google/gerrit/entities/Change.java
+++ b/java/com/google/gerrit/entities/Change.java
@@ -18,6 +18,7 @@
import com.google.auto.value.AutoValue;
import com.google.common.primitives.Ints;
+import com.google.gerrit.common.ConvertibleToProto;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gson.Gson;
@@ -104,6 +105,7 @@
/** The numeric change ID */
@AutoValue
+ @ConvertibleToProto
public abstract static class Id {
/**
* Parse a Change.Id out of a string representation.
@@ -271,6 +273,7 @@
* "Ixxxxxx...", and is stored in the Change-Id footer of a commit.
*/
@AutoValue
+ @ConvertibleToProto
public abstract static class Key {
// TODO(dborowitz): This hardly seems worth it: why would someone pass a URL-encoded change key?
// Ideally the standard key() factory method would enforce the format and throw IAE.
@@ -434,7 +437,7 @@
private Id changeId;
/** ServerId of the Gerrit instance that has created the change */
- private String serverId;
+ @Nullable private String serverId;
/** Globally assigned unique identifier of the change */
private Key changeKey;
@@ -545,7 +548,8 @@
* ServerId of the Gerrit instance that created the change. It could be null when the change is
* not fetched from NoteDb but obtained through protobuf deserialisation.
*/
- public @Nullable String getServerId() {
+ @Nullable
+ public String getServerId() {
return serverId;
}
@@ -607,6 +611,7 @@
return originalSubject != null ? originalSubject : subject;
}
+ @Nullable
public String getOriginalSubjectOrNull() {
return originalSubject;
}
@@ -652,6 +657,7 @@
originalSubject = null;
}
+ @Nullable
public String getSubmissionId() {
return submissionId;
}
@@ -684,6 +690,7 @@
return isAbandoned() || isMerged();
}
+ @Nullable
public String getTopic() {
return topic;
}
@@ -720,10 +727,12 @@
this.revertOf = revertOf;
}
+ @Nullable
public Id getRevertOf() {
return this.revertOf;
}
+ @Nullable
public PatchSet.Id getCherryPickOf() {
return cherryPickOf;
}
diff --git a/java/com/google/gerrit/entities/ChangeMessage.java b/java/com/google/gerrit/entities/ChangeMessage.java
index dea070f..c8fc7d2 100644
--- a/java/com/google/gerrit/entities/ChangeMessage.java
+++ b/java/com/google/gerrit/entities/ChangeMessage.java
@@ -15,6 +15,7 @@
package com.google.gerrit.entities;
import com.google.auto.value.AutoValue;
+import com.google.gerrit.common.ConvertibleToProto;
import com.google.gerrit.common.Nullable;
import java.time.Instant;
import java.util.Objects;
@@ -34,6 +35,7 @@
}
@AutoValue
+ @ConvertibleToProto
public abstract static class Key {
public abstract Change.Id changeId();
diff --git a/java/com/google/gerrit/entities/HumanComment.java b/java/com/google/gerrit/entities/HumanComment.java
index 1e48f11..325bd6c 100644
--- a/java/com/google/gerrit/entities/HumanComment.java
+++ b/java/com/google/gerrit/entities/HumanComment.java
@@ -14,6 +14,7 @@
package com.google.gerrit.entities;
+import com.google.gerrit.common.ConvertibleToProto;
import java.time.Instant;
import java.util.Objects;
@@ -26,6 +27,7 @@
*
* <p>Consider updating {@link #getApproximateSize()} when adding/changing fields.
*/
+@ConvertibleToProto
public class HumanComment extends Comment {
public boolean unresolved;
diff --git a/java/com/google/gerrit/entities/LabelId.java b/java/com/google/gerrit/entities/LabelId.java
index 2426818..e3b3024 100644
--- a/java/com/google/gerrit/entities/LabelId.java
+++ b/java/com/google/gerrit/entities/LabelId.java
@@ -15,8 +15,10 @@
package com.google.gerrit.entities;
import com.google.auto.value.AutoValue;
+import com.google.gerrit.common.ConvertibleToProto;
@AutoValue
+@ConvertibleToProto
public abstract class LabelId {
public static final String LEGACY_SUBMIT_NAME = "SUBM";
public static final String CODE_REVIEW = "Code-Review";
diff --git a/java/com/google/gerrit/entities/PatchSet.java b/java/com/google/gerrit/entities/PatchSet.java
index e8759fa..6f71874 100644
--- a/java/com/google/gerrit/entities/PatchSet.java
+++ b/java/com/google/gerrit/entities/PatchSet.java
@@ -23,6 +23,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.primitives.Ints;
import com.google.errorprone.annotations.InlineMe;
+import com.google.gerrit.common.ConvertibleToProto;
import com.google.gerrit.common.Nullable;
import java.time.Instant;
import java.util.List;
@@ -31,6 +32,7 @@
/** A single revision of a {@link Change}. */
@AutoValue
+@ConvertibleToProto
public abstract class PatchSet {
/** Is the reference name a change reference? */
public static boolean isChangeRef(String name) {
@@ -67,6 +69,7 @@
}
@AutoValue
+ @ConvertibleToProto
public abstract static class Id implements Comparable<Id> {
/** Parse a PatchSet.Id out of a string representation. */
public static Id parse(String str) {
diff --git a/java/com/google/gerrit/entities/PatchSetApproval.java b/java/com/google/gerrit/entities/PatchSetApproval.java
index 608cf0d..f78167b 100644
--- a/java/com/google/gerrit/entities/PatchSetApproval.java
+++ b/java/com/google/gerrit/entities/PatchSetApproval.java
@@ -16,6 +16,7 @@
import com.google.auto.value.AutoValue;
import com.google.common.primitives.Shorts;
+import com.google.gerrit.common.ConvertibleToProto;
import java.time.Instant;
import java.util.Optional;
@@ -27,6 +28,7 @@
}
@AutoValue
+ @ConvertibleToProto
public abstract static class Key {
public abstract PatchSet.Id patchSetId();
diff --git a/java/com/google/gerrit/entities/Project.java b/java/com/google/gerrit/entities/Project.java
index 9c2866c..7b02597 100644
--- a/java/com/google/gerrit/entities/Project.java
+++ b/java/com/google/gerrit/entities/Project.java
@@ -20,6 +20,7 @@
import com.google.common.collect.ImmutableMap;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.errorprone.annotations.Immutable;
+import com.google.gerrit.common.ConvertibleToProto;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.gerrit.extensions.client.ProjectState;
@@ -57,6 +58,7 @@
* <p>This class is immutable and thread safe.
*/
@Immutable
+ @ConvertibleToProto
public static class NameKey implements Serializable, Comparable<NameKey> {
private static final long serialVersionUID = 1L;
diff --git a/java/com/google/gerrit/entities/converter/AccountIdProtoConverter.java b/java/com/google/gerrit/entities/converter/AccountIdProtoConverter.java
index 1e846fb..6106090 100644
--- a/java/com/google/gerrit/entities/converter/AccountIdProtoConverter.java
+++ b/java/com/google/gerrit/entities/converter/AccountIdProtoConverter.java
@@ -20,7 +20,7 @@
import com.google.protobuf.Parser;
@Immutable
-public enum AccountIdProtoConverter implements ProtoConverter<Entities.Account_Id, Account.Id> {
+public enum AccountIdProtoConverter implements SafeProtoConverter<Entities.Account_Id, Account.Id> {
INSTANCE;
@Override
@@ -37,4 +37,14 @@
public Parser<Entities.Account_Id> getParser() {
return Entities.Account_Id.parser();
}
+
+ @Override
+ public Class<Entities.Account_Id> getProtoClass() {
+ return Entities.Account_Id.class;
+ }
+
+ @Override
+ public Class<Account.Id> getEntityClass() {
+ return Account.Id.class;
+ }
}
diff --git a/java/com/google/gerrit/entities/converter/ChangeIdProtoConverter.java b/java/com/google/gerrit/entities/converter/ChangeIdProtoConverter.java
index 0d4ec70..909b4d3 100644
--- a/java/com/google/gerrit/entities/converter/ChangeIdProtoConverter.java
+++ b/java/com/google/gerrit/entities/converter/ChangeIdProtoConverter.java
@@ -17,10 +17,11 @@
import com.google.errorprone.annotations.Immutable;
import com.google.gerrit.entities.Change;
import com.google.gerrit.proto.Entities;
+import com.google.gerrit.proto.Entities.Change_Id;
import com.google.protobuf.Parser;
@Immutable
-public enum ChangeIdProtoConverter implements ProtoConverter<Entities.Change_Id, Change.Id> {
+public enum ChangeIdProtoConverter implements SafeProtoConverter<Entities.Change_Id, Change.Id> {
INSTANCE;
@Override
@@ -37,4 +38,14 @@
public Parser<Entities.Change_Id> getParser() {
return Entities.Change_Id.parser();
}
+
+ @Override
+ public Class<Change_Id> getProtoClass() {
+ return Change_Id.class;
+ }
+
+ @Override
+ public Class<Change.Id> getEntityClass() {
+ return Change.Id.class;
+ }
}
diff --git a/java/com/google/gerrit/entities/converter/ChangeKeyProtoConverter.java b/java/com/google/gerrit/entities/converter/ChangeKeyProtoConverter.java
index f3ccdfa..0620c70 100644
--- a/java/com/google/gerrit/entities/converter/ChangeKeyProtoConverter.java
+++ b/java/com/google/gerrit/entities/converter/ChangeKeyProtoConverter.java
@@ -17,10 +17,11 @@
import com.google.errorprone.annotations.Immutable;
import com.google.gerrit.entities.Change;
import com.google.gerrit.proto.Entities;
+import com.google.gerrit.proto.Entities.Change_Key;
import com.google.protobuf.Parser;
@Immutable
-public enum ChangeKeyProtoConverter implements ProtoConverter<Entities.Change_Key, Change.Key> {
+public enum ChangeKeyProtoConverter implements SafeProtoConverter<Entities.Change_Key, Change.Key> {
INSTANCE;
@Override
@@ -37,4 +38,14 @@
public Parser<Entities.Change_Key> getParser() {
return Entities.Change_Key.parser();
}
+
+ @Override
+ public Class<Change_Key> getProtoClass() {
+ return Change_Key.class;
+ }
+
+ @Override
+ public Class<Change.Key> getEntityClass() {
+ return Change.Key.class;
+ }
}
diff --git a/java/com/google/gerrit/entities/converter/ChangeMessageKeyProtoConverter.java b/java/com/google/gerrit/entities/converter/ChangeMessageKeyProtoConverter.java
index 3e93c5a..a76ab98 100644
--- a/java/com/google/gerrit/entities/converter/ChangeMessageKeyProtoConverter.java
+++ b/java/com/google/gerrit/entities/converter/ChangeMessageKeyProtoConverter.java
@@ -18,11 +18,12 @@
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.ChangeMessage;
import com.google.gerrit.proto.Entities;
+import com.google.gerrit.proto.Entities.ChangeMessage_Key;
import com.google.protobuf.Parser;
@Immutable
public enum ChangeMessageKeyProtoConverter
- implements ProtoConverter<Entities.ChangeMessage_Key, ChangeMessage.Key> {
+ implements SafeProtoConverter<Entities.ChangeMessage_Key, ChangeMessage.Key> {
INSTANCE;
private final ProtoConverter<Entities.Change_Id, Change.Id> changeIdConverter =
@@ -45,4 +46,14 @@
public Parser<Entities.ChangeMessage_Key> getParser() {
return Entities.ChangeMessage_Key.parser();
}
+
+ @Override
+ public Class<ChangeMessage_Key> getProtoClass() {
+ return ChangeMessage_Key.class;
+ }
+
+ @Override
+ public Class<ChangeMessage.Key> getEntityClass() {
+ return ChangeMessage.Key.class;
+ }
}
diff --git a/java/com/google/gerrit/entities/converter/HumanCommentProtoConverter.java b/java/com/google/gerrit/entities/converter/HumanCommentProtoConverter.java
index 6e8c907..316a042 100644
--- a/java/com/google/gerrit/entities/converter/HumanCommentProtoConverter.java
+++ b/java/com/google/gerrit/entities/converter/HumanCommentProtoConverter.java
@@ -35,7 +35,7 @@
*/
@Immutable
public enum HumanCommentProtoConverter
- implements ProtoConverter<Entities.HumanComment, HumanComment> {
+ implements SafeProtoConverter<Entities.HumanComment, HumanComment> {
INSTANCE;
private final ProtoConverter<Entities.Account_Id, Account.Id> accountIdConverter =
@@ -142,4 +142,14 @@
public Parser<Entities.HumanComment> getParser() {
return Entities.HumanComment.parser();
}
+
+ @Override
+ public Class<Entities.HumanComment> getProtoClass() {
+ return Entities.HumanComment.class;
+ }
+
+ @Override
+ public Class<HumanComment> getEntityClass() {
+ return HumanComment.class;
+ }
}
diff --git a/java/com/google/gerrit/entities/converter/LabelIdProtoConverter.java b/java/com/google/gerrit/entities/converter/LabelIdProtoConverter.java
index a1894ac..e6e1be7f 100644
--- a/java/com/google/gerrit/entities/converter/LabelIdProtoConverter.java
+++ b/java/com/google/gerrit/entities/converter/LabelIdProtoConverter.java
@@ -20,7 +20,7 @@
import com.google.protobuf.Parser;
@Immutable
-public enum LabelIdProtoConverter implements ProtoConverter<Entities.LabelId, LabelId> {
+public enum LabelIdProtoConverter implements SafeProtoConverter<Entities.LabelId, LabelId> {
INSTANCE;
@Override
@@ -37,4 +37,14 @@
public Parser<Entities.LabelId> getParser() {
return Entities.LabelId.parser();
}
+
+ @Override
+ public Class<Entities.LabelId> getProtoClass() {
+ return Entities.LabelId.class;
+ }
+
+ @Override
+ public Class<LabelId> getEntityClass() {
+ return LabelId.class;
+ }
}
diff --git a/java/com/google/gerrit/entities/converter/NotifyInfoProtoConverter.java b/java/com/google/gerrit/entities/converter/NotifyInfoProtoConverter.java
index 201dd78..fc963df 100644
--- a/java/com/google/gerrit/entities/converter/NotifyInfoProtoConverter.java
+++ b/java/com/google/gerrit/entities/converter/NotifyInfoProtoConverter.java
@@ -26,7 +26,8 @@
* com.google.gerrit.proto.Entities.NotifyInfo}.
*/
@Immutable
-public enum NotifyInfoProtoConverter implements ProtoConverter<Entities.NotifyInfo, NotifyInfo> {
+public enum NotifyInfoProtoConverter
+ implements SafeProtoConverter<Entities.NotifyInfo, NotifyInfo> {
INSTANCE;
@Override
@@ -47,4 +48,14 @@
public Parser<Entities.NotifyInfo> getParser() {
return Entities.NotifyInfo.parser();
}
+
+ @Override
+ public Class<Entities.NotifyInfo> getProtoClass() {
+ return Entities.NotifyInfo.class;
+ }
+
+ @Override
+ public Class<NotifyInfo> getEntityClass() {
+ return NotifyInfo.class;
+ }
}
diff --git a/java/com/google/gerrit/entities/converter/PatchSetApprovalKeyProtoConverter.java b/java/com/google/gerrit/entities/converter/PatchSetApprovalKeyProtoConverter.java
index c7d1714..3ea14e6 100644
--- a/java/com/google/gerrit/entities/converter/PatchSetApprovalKeyProtoConverter.java
+++ b/java/com/google/gerrit/entities/converter/PatchSetApprovalKeyProtoConverter.java
@@ -20,11 +20,12 @@
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.PatchSetApproval;
import com.google.gerrit.proto.Entities;
+import com.google.gerrit.proto.Entities.PatchSetApproval_Key;
import com.google.protobuf.Parser;
@Immutable
public enum PatchSetApprovalKeyProtoConverter
- implements ProtoConverter<Entities.PatchSetApproval_Key, PatchSetApproval.Key> {
+ implements SafeProtoConverter<Entities.PatchSetApproval_Key, PatchSetApproval.Key> {
INSTANCE;
private final ProtoConverter<Entities.PatchSet_Id, PatchSet.Id> patchSetIdConverter =
@@ -55,4 +56,14 @@
public Parser<Entities.PatchSetApproval_Key> getParser() {
return Entities.PatchSetApproval_Key.parser();
}
+
+ @Override
+ public Class<PatchSetApproval_Key> getProtoClass() {
+ return PatchSetApproval_Key.class;
+ }
+
+ @Override
+ public Class<PatchSetApproval.Key> getEntityClass() {
+ return PatchSetApproval.Key.class;
+ }
}
diff --git a/java/com/google/gerrit/entities/converter/PatchSetIdProtoConverter.java b/java/com/google/gerrit/entities/converter/PatchSetIdProtoConverter.java
index 60c13f1..f6671cf 100644
--- a/java/com/google/gerrit/entities/converter/PatchSetIdProtoConverter.java
+++ b/java/com/google/gerrit/entities/converter/PatchSetIdProtoConverter.java
@@ -18,10 +18,12 @@
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.proto.Entities;
+import com.google.gerrit.proto.Entities.PatchSet_Id;
import com.google.protobuf.Parser;
@Immutable
-public enum PatchSetIdProtoConverter implements ProtoConverter<Entities.PatchSet_Id, PatchSet.Id> {
+public enum PatchSetIdProtoConverter
+ implements SafeProtoConverter<Entities.PatchSet_Id, PatchSet.Id> {
INSTANCE;
private final ProtoConverter<Entities.Change_Id, Change.Id> changeIdConverter =
@@ -44,4 +46,14 @@
public Parser<Entities.PatchSet_Id> getParser() {
return Entities.PatchSet_Id.parser();
}
+
+ @Override
+ public Class<PatchSet_Id> getProtoClass() {
+ return PatchSet_Id.class;
+ }
+
+ @Override
+ public Class<PatchSet.Id> getEntityClass() {
+ return PatchSet.Id.class;
+ }
}
diff --git a/java/com/google/gerrit/entities/converter/PatchSetProtoConverter.java b/java/com/google/gerrit/entities/converter/PatchSetProtoConverter.java
index 196deca..22985d9 100644
--- a/java/com/google/gerrit/entities/converter/PatchSetProtoConverter.java
+++ b/java/com/google/gerrit/entities/converter/PatchSetProtoConverter.java
@@ -24,7 +24,7 @@
import org.eclipse.jgit.lib.ObjectId;
@Immutable
-public enum PatchSetProtoConverter implements ProtoConverter<Entities.PatchSet, PatchSet> {
+public enum PatchSetProtoConverter implements SafeProtoConverter<Entities.PatchSet, PatchSet> {
INSTANCE;
private final ProtoConverter<Entities.PatchSet_Id, PatchSet.Id> patchSetIdConverter =
@@ -103,4 +103,14 @@
public Parser<Entities.PatchSet> getParser() {
return Entities.PatchSet.parser();
}
+
+ @Override
+ public Class<Entities.PatchSet> getProtoClass() {
+ return Entities.PatchSet.class;
+ }
+
+ @Override
+ public Class<PatchSet> getEntityClass() {
+ return PatchSet.class;
+ }
}
diff --git a/java/com/google/gerrit/entities/converter/ProjectNameKeyProtoConverter.java b/java/com/google/gerrit/entities/converter/ProjectNameKeyProtoConverter.java
index 6bb0f79..320b8fc 100644
--- a/java/com/google/gerrit/entities/converter/ProjectNameKeyProtoConverter.java
+++ b/java/com/google/gerrit/entities/converter/ProjectNameKeyProtoConverter.java
@@ -16,12 +16,14 @@
import com.google.errorprone.annotations.Immutable;
import com.google.gerrit.entities.Project;
+import com.google.gerrit.entities.Project.NameKey;
import com.google.gerrit.proto.Entities;
+import com.google.gerrit.proto.Entities.Project_NameKey;
import com.google.protobuf.Parser;
@Immutable
public enum ProjectNameKeyProtoConverter
- implements ProtoConverter<Entities.Project_NameKey, Project.NameKey> {
+ implements SafeProtoConverter<Entities.Project_NameKey, Project.NameKey> {
INSTANCE;
@Override
@@ -38,4 +40,14 @@
public Parser<Entities.Project_NameKey> getParser() {
return Entities.Project_NameKey.parser();
}
+
+ @Override
+ public Class<Project_NameKey> getProtoClass() {
+ return Project_NameKey.class;
+ }
+
+ @Override
+ public Class<NameKey> getEntityClass() {
+ return NameKey.class;
+ }
}
diff --git a/java/com/google/gerrit/entities/converter/SafeProtoConverter.java b/java/com/google/gerrit/entities/converter/SafeProtoConverter.java
new file mode 100644
index 0000000..f4a66a0
--- /dev/null
+++ b/java/com/google/gerrit/entities/converter/SafeProtoConverter.java
@@ -0,0 +1,29 @@
+package com.google.gerrit.entities.converter;
+
+import com.google.errorprone.annotations.Immutable;
+import com.google.gerrit.common.ConvertibleToProto;
+import com.google.protobuf.Message;
+
+/**
+ * An extension to {@link ProtoConverter} that enforces the Entity class and the Proto class to stay
+ * in sync. The enforcement is done by {@link SafeProtoConverterTest}.
+ *
+ * <p>Requirements:
+ *
+ * <ul>
+ * <li>Implementing classes must be enums with a single value. Please prefer descriptive enum and
+ * instance names, such as {@code MyTypeConverter::MY_TYPE_CONVERTER}.
+ * <li>The Java Entity class must be annotated with {@link ConvertibleToProto}.
+ * </ul>
+ *
+ * <p>All safe converters are tested using {@link SafeProtoConverterTest}. Therefore, unless your
+ * Entity class has a {@code defaults()} method, or other methods besides simple getters and
+ * setters, there is no need to explicitly test your safe converter.
+ */
+@Immutable
+public interface SafeProtoConverter<P extends Message, C> extends ProtoConverter<P, C> {
+
+ Class<P> getProtoClass();
+
+ Class<C> getEntityClass();
+}
diff --git a/java/com/google/gerrit/extensions/api/changes/NotifyInfo.java b/java/com/google/gerrit/extensions/api/changes/NotifyInfo.java
index dd29635..21bf886 100644
--- a/java/com/google/gerrit/extensions/api/changes/NotifyInfo.java
+++ b/java/com/google/gerrit/extensions/api/changes/NotifyInfo.java
@@ -15,10 +15,12 @@
package com.google.gerrit.extensions.api.changes;
import com.google.common.base.MoreObjects;
+import com.google.gerrit.common.ConvertibleToProto;
import java.util.List;
import java.util.Objects;
/** Detailed information about who should be notified about an update. */
+@ConvertibleToProto
public class NotifyInfo {
public List<String> accounts;
diff --git a/java/com/google/gerrit/extensions/client/DiffPreferencesInfo.java b/java/com/google/gerrit/extensions/client/DiffPreferencesInfo.java
index 109afd6..ad494cb 100644
--- a/java/com/google/gerrit/extensions/client/DiffPreferencesInfo.java
+++ b/java/com/google/gerrit/extensions/client/DiffPreferencesInfo.java
@@ -15,8 +15,10 @@
package com.google.gerrit.extensions.client;
import com.google.common.base.MoreObjects;
+import com.google.gerrit.common.ConvertibleToProto;
import java.util.Objects;
+@ConvertibleToProto
public class DiffPreferencesInfo {
/** Default number of lines of context. */
diff --git a/java/com/google/gerrit/extensions/client/EditPreferencesInfo.java b/java/com/google/gerrit/extensions/client/EditPreferencesInfo.java
index 0a3ec0a..5da211e 100644
--- a/java/com/google/gerrit/extensions/client/EditPreferencesInfo.java
+++ b/java/com/google/gerrit/extensions/client/EditPreferencesInfo.java
@@ -15,9 +15,11 @@
package com.google.gerrit.extensions.client;
import com.google.common.base.MoreObjects;
+import com.google.gerrit.common.ConvertibleToProto;
import java.util.Objects;
/* This class is stored in Git config file. */
+@ConvertibleToProto
public class EditPreferencesInfo {
public Integer tabSize;
public Integer lineLength;
diff --git a/java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java b/java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java
index 1ed9793..44fc4d5 100644
--- a/java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java
+++ b/java/com/google/gerrit/extensions/client/GeneralPreferencesInfo.java
@@ -15,10 +15,12 @@
package com.google.gerrit.extensions.client;
import com.google.common.base.MoreObjects;
+import com.google.gerrit.common.ConvertibleToProto;
import java.util.List;
import java.util.Objects;
/** Preferences about a single user. */
+@ConvertibleToProto
public class GeneralPreferencesInfo {
/** Default number of items to display per page. */
@@ -143,6 +145,8 @@
public List<String> changeTable;
public Boolean allowBrowserNotifications;
public Boolean allowSuggestCodeWhileCommenting;
+ public Boolean allowAutocompletingComments;
+
/**
* The sidebar section that the user prefers to have open on the diff page, or "NONE" if all
* sidebars should be closed.
@@ -214,6 +218,9 @@
&& Objects.equals(this.my, other.my)
&& Objects.equals(this.changeTable, other.changeTable)
&& Objects.equals(this.allowBrowserNotifications, other.allowBrowserNotifications)
+ && Objects.equals(
+ this.allowSuggestCodeWhileCommenting, other.allowSuggestCodeWhileCommenting)
+ && Objects.equals(this.allowAutocompletingComments, other.allowAutocompletingComments)
&& Objects.equals(this.diffPageSidebar, other.diffPageSidebar);
}
@@ -242,6 +249,8 @@
my,
changeTable,
allowBrowserNotifications,
+ allowSuggestCodeWhileCommenting,
+ allowAutocompletingComments,
diffPageSidebar);
}
@@ -270,6 +279,8 @@
.add("my", my)
.add("changeTable", changeTable)
.add("allowBrowserNotifications", allowBrowserNotifications)
+ .add("allowSuggestCodeWhileCommenting", allowSuggestCodeWhileCommenting)
+ .add("allowAutocompletingComments", allowAutocompletingComments)
.add("diffPageSidebar", diffPageSidebar)
.toString();
}
@@ -296,8 +307,9 @@
p.disableTokenHighlighting = false;
p.workInProgressByDefault = false;
p.allowBrowserNotifications = true;
- p.diffPageSidebar = "NONE";
p.allowSuggestCodeWhileCommenting = true;
+ p.allowAutocompletingComments = true;
+ p.diffPageSidebar = "NONE";
return p;
}
}
diff --git a/java/com/google/gerrit/server/account/AccountsUpdate.java b/java/com/google/gerrit/server/account/AccountsUpdate.java
index 5951a73..45ff128 100644
--- a/java/com/google/gerrit/server/account/AccountsUpdate.java
+++ b/java/com/google/gerrit/server/account/AccountsUpdate.java
@@ -20,6 +20,8 @@
import static java.lang.annotation.ElementType.PARAMETER;
import static java.lang.annotation.RetentionPolicy.RUNTIME;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.MoreObjects;
import com.google.common.collect.ImmutableList;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.gerrit.entities.Account;
@@ -97,6 +99,14 @@
this.accountId = accountId;
this.configureDeltaFromState = configureDeltaFromState;
}
+
+ @Override
+ public String toString() {
+ return MoreObjects.toStringHelper(this)
+ .add("message", message)
+ .add("accountId", accountId)
+ .toString();
+ }
}
/**
@@ -145,7 +155,7 @@
* would only contain the account ID).
*/
@CanIgnoreReturnValue
- public AccountState insert(
+ public final AccountState insert(
String message, Account.Id accountId, Consumer<AccountDelta.Builder> init)
throws IOException, ConfigInvalidException {
return insert(message, accountId, AccountsUpdate.fromConsumer(init));
@@ -171,7 +181,7 @@
* instead, i.e. the update does not depend on the current account state.
*/
@CanIgnoreReturnValue
- public Optional<AccountState> update(
+ public final Optional<AccountState> update(
String message, Account.Id accountId, Consumer<AccountDelta.Builder> update)
throws IOException, ConfigInvalidException {
return update(message, accountId, AccountsUpdate.fromConsumer(update));
@@ -193,7 +203,7 @@
* @throws ConfigInvalidException if any of the account fields has an invalid value
*/
@CanIgnoreReturnValue
- public Optional<AccountState> update(
+ public final Optional<AccountState> update(
String message, Account.Id accountId, ConfigureDeltaFromState configureDeltaFromState)
throws IOException, ConfigInvalidException {
return updateBatch(
@@ -212,7 +222,7 @@
* together have this property) will always prevent the entire batch from being executed.
*/
@CanIgnoreReturnValue
- public ImmutableList<Optional<AccountState>> updateBatch(List<UpdateArguments> updates)
+ public final ImmutableList<Optional<AccountState>> updateBatch(List<UpdateArguments> updates)
throws IOException, ConfigInvalidException {
checkArgument(
updates.stream().map(u -> u.accountId.get()).distinct().count() == updates.size(),
@@ -231,7 +241,8 @@
public abstract void delete(String message, Account.Id accountId)
throws IOException, ConfigInvalidException;
- protected abstract ImmutableList<Optional<AccountState>> executeUpdates(
+ @VisibleForTesting // productionVisibility: protected
+ public abstract ImmutableList<Optional<AccountState>> executeUpdates(
List<UpdateArguments> updates) throws ConfigInvalidException, IOException;
private static PersonIdent createPersonIdent(
diff --git a/java/com/google/gerrit/server/account/storage/notedb/AccountsUpdateNoteDbImpl.java b/java/com/google/gerrit/server/account/storage/notedb/AccountsUpdateNoteDbImpl.java
index ad3681d..27a43ba 100644
--- a/java/com/google/gerrit/server/account/storage/notedb/AccountsUpdateNoteDbImpl.java
+++ b/java/com/google/gerrit/server/account/storage/notedb/AccountsUpdateNoteDbImpl.java
@@ -380,7 +380,8 @@
}
@Override
- protected ImmutableList<Optional<AccountState>> executeUpdates(List<UpdateArguments> updates)
+ @VisibleForTesting
+ public ImmutableList<Optional<AccountState>> executeUpdates(List<UpdateArguments> updates)
throws ConfigInvalidException, IOException {
return execute(updates.stream().map(this::createExecutableUpdate).collect(toImmutableList()));
}
diff --git a/java/com/google/gerrit/server/config/UserPreferencesConverter.java b/java/com/google/gerrit/server/config/UserPreferencesConverter.java
index 912abc4..4b2f6d2 100644
--- a/java/com/google/gerrit/server/config/UserPreferencesConverter.java
+++ b/java/com/google/gerrit/server/config/UserPreferencesConverter.java
@@ -17,7 +17,7 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import com.google.gerrit.common.Nullable;
-import com.google.gerrit.entities.converter.ProtoConverter;
+import com.google.gerrit.entities.converter.SafeProtoConverter;
import com.google.gerrit.extensions.client.DiffPreferencesInfo;
import com.google.gerrit.extensions.client.EditPreferencesInfo;
import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
@@ -36,7 +36,8 @@
*/
public final class UserPreferencesConverter {
public enum GeneralPreferencesInfoConverter
- implements ProtoConverter<UserPreferences.GeneralPreferencesInfo, GeneralPreferencesInfo> {
+ implements
+ SafeProtoConverter<UserPreferences.GeneralPreferencesInfo, GeneralPreferencesInfo> {
GENERAL_PREFERENCES_INFO_CONVERTER;
@Override
@@ -118,6 +119,14 @@
builder =
setIfNotNull(
builder, builder::setAllowBrowserNotifications, info.allowBrowserNotifications);
+ builder =
+ setIfNotNull(
+ builder,
+ builder::setAllowSuggestCodeWhileCommenting,
+ info.allowSuggestCodeWhileCommenting);
+ builder =
+ setIfNotNull(
+ builder, builder::setAllowAutocompletingComments, info.allowAutocompletingComments);
builder = setIfNotNull(builder, builder::setDiffPageSidebar, info.diffPageSidebar);
return builder.build();
}
@@ -180,6 +189,12 @@
res.changeTable = proto.getChangeTableCount() != 0 ? proto.getChangeTableList() : null;
res.allowBrowserNotifications =
proto.hasAllowBrowserNotifications() ? proto.getAllowBrowserNotifications() : null;
+ res.allowSuggestCodeWhileCommenting =
+ proto.hasAllowSuggestCodeWhileCommenting()
+ ? proto.getAllowSuggestCodeWhileCommenting()
+ : null;
+ res.allowAutocompletingComments =
+ proto.hasAllowAutocompletingComments() ? proto.getAllowAutocompletingComments() : null;
res.diffPageSidebar = proto.hasDiffPageSidebar() ? proto.getDiffPageSidebar() : null;
return res;
}
@@ -212,10 +227,20 @@
proto.hasTarget() ? proto.getTarget().trim() : null,
proto.hasId() ? proto.getId().trim() : null);
}
+
+ @Override
+ public Class<UserPreferences.GeneralPreferencesInfo> getProtoClass() {
+ return UserPreferences.GeneralPreferencesInfo.class;
+ }
+
+ @Override
+ public Class<GeneralPreferencesInfo> getEntityClass() {
+ return GeneralPreferencesInfo.class;
+ }
}
public enum DiffPreferencesInfoConverter
- implements ProtoConverter<UserPreferences.DiffPreferencesInfo, DiffPreferencesInfo> {
+ implements SafeProtoConverter<UserPreferences.DiffPreferencesInfo, DiffPreferencesInfo> {
DIFF_PREFERENCES_INFO_CONVERTER;
@Override
@@ -295,10 +320,20 @@
public Parser<UserPreferences.DiffPreferencesInfo> getParser() {
return UserPreferences.DiffPreferencesInfo.parser();
}
+
+ @Override
+ public Class<UserPreferences.DiffPreferencesInfo> getProtoClass() {
+ return UserPreferences.DiffPreferencesInfo.class;
+ }
+
+ @Override
+ public Class<DiffPreferencesInfo> getEntityClass() {
+ return DiffPreferencesInfo.class;
+ }
}
public enum EditPreferencesInfoConverter
- implements ProtoConverter<UserPreferences.EditPreferencesInfo, EditPreferencesInfo> {
+ implements SafeProtoConverter<UserPreferences.EditPreferencesInfo, EditPreferencesInfo> {
EDIT_PREFERENCES_INFO_CONVERTER;
@Override
@@ -347,6 +382,16 @@
public Parser<UserPreferences.EditPreferencesInfo> getParser() {
return UserPreferences.EditPreferencesInfo.parser();
}
+
+ @Override
+ public Class<UserPreferences.EditPreferencesInfo> getProtoClass() {
+ return UserPreferences.EditPreferencesInfo.class;
+ }
+
+ @Override
+ public Class<EditPreferencesInfo> getEntityClass() {
+ return EditPreferencesInfo.class;
+ }
}
private static <ValueT, BuilderT extends Message.Builder> BuilderT setIfNotNull(
diff --git a/java/com/google/gerrit/server/git/ChangesByProjectCacheImpl.java b/java/com/google/gerrit/server/git/ChangesByProjectCacheImpl.java
index 2bbd261..17cc5a0 100644
--- a/java/com/google/gerrit/server/git/ChangesByProjectCacheImpl.java
+++ b/java/com/google/gerrit/server/git/ChangesByProjectCacheImpl.java
@@ -301,7 +301,9 @@
size += JavaWeights.REFERENCE + (c.getTopic() == null ? 0 : c.getTopic().length());
size +=
JavaWeights.REFERENCE
- + (c.getOriginalSubject().equals(c.getSubject()) ? 0 : c.getSubject().length());
+ + (c.getOriginalSubject().equals(c.getSubject())
+ ? 0
+ : c.getOriginalSubject().length());
size +=
JavaWeights.REFERENCE + (c.getSubmissionId() == null ? 0 : c.getSubmissionId().length());
size += JavaWeights.REFERENCE + JavaWeights.BOOLEAN; // isPrivate;
@@ -309,7 +311,7 @@
size += JavaWeights.REFERENCE + JavaWeights.BOOLEAN; // reviewStarted;
size += JavaWeights.REFERENCE + (c.getRevertOf() == null ? 0 : GerritWeights.CHANGE_NUM);
size +=
- JavaWeights.REFERENCE + (c.getCherryPickOf() == null ? 0 : GerritWeights.PACTCH_SET_ID);
+ JavaWeights.REFERENCE + (c.getCherryPickOf() == null ? 0 : GerritWeights.PATCH_SET_ID);
return size;
}
@@ -343,7 +345,7 @@
public static final int KEY_INT = JavaWeights.OBJECT + JavaWeights.INT; // IntKey
public static final int CHANGE_NUM = KEY_INT;
public static final int ACCOUNT_ID = KEY_INT;
- public static final int PACTCH_SET_ID =
+ public static final int PATCH_SET_ID =
JavaWeights.OBJECT
+ (JavaWeights.REFERENCE + GerritWeights.CHANGE_NUM) // PatchSet.Id.changeId
+ JavaWeights.INT; // PatchSet.Id patch_num;
diff --git a/java/com/google/gerrit/server/git/WorkQueue.java b/java/com/google/gerrit/server/git/WorkQueue.java
index 307ec5c..6f904ea 100644
--- a/java/com/google/gerrit/server/git/WorkQueue.java
+++ b/java/com/google/gerrit/server/git/WorkQueue.java
@@ -39,15 +39,20 @@
import java.time.Instant;
import java.util.ArrayList;
import java.util.Collection;
+import java.util.HashSet;
import java.util.List;
+import java.util.Optional;
+import java.util.Set;
import java.util.concurrent.Callable;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.CountDownLatch;
import java.util.concurrent.Delayed;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.FutureTask;
+import java.util.concurrent.PriorityBlockingQueue;
import java.util.concurrent.RunnableScheduledFuture;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
@@ -56,7 +61,9 @@
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicLong;
import java.util.concurrent.atomic.AtomicReference;
+import org.apache.commons.lang3.mutable.MutableBoolean;
import org.eclipse.jgit.lib.Config;
/** Delayed execution of tasks using a background thread pool. */
@@ -88,6 +95,54 @@
void onStop(Task<?> task);
}
+ /**
+ * Register a TaskParker from a plugin like this:
+ *
+ * <p>bind(TaskListener.class).annotatedWith(Exports.named("MyParker")).to(MyParker.class);
+ */
+ public interface TaskParker extends TaskListener {
+ class NoOp extends TaskListener.NoOp implements TaskParker {
+ @Override
+ public boolean isReadyToStart(Task<?> task) {
+ return true;
+ }
+
+ @Override
+ public void onNotReadyToStart(Task<?> task) {}
+ }
+
+ /**
+ * Determine whether a {@link Task} is ready to run or whether it should get parked.
+ *
+ * <p>Tasks that are not ready to run will get parked and will not run until all {@link
+ * TaskParker}s return {@code true} from this method for the {@link Task}. This method may be
+ * called more than once, but will always be followed by a call to {@link
+ * #onNotReadyToStart(Task)} before being called again.
+ *
+ * <p>Resources should be acquired in this method via non-blocking means to avoid delaying the
+ * executor from calling {@link #onNotReadyToStart(Task)} on other {@link TaskParker}s holding
+ * resources.
+ *
+ * @param task the {@link Task} being considered for starting/parking
+ * @return a boolean indicating if the given {@link Task} is ready to run ({@code true}) or
+ * should be parked ({@code false})
+ */
+ boolean isReadyToStart(Task<?> task);
+
+ /**
+ * This method will be called after this {@link TaskParker} returns {@code true} from {@link
+ * #isReadyToStart(Task)} and another {@link TaskParker} returns {@code false}, thus preventing
+ * the start.
+ *
+ * <p>Implementors should use this method to free any resources acquired in {@link
+ * #isReadyToStart(Task)} based on the expectation that the task would start. Those resources
+ * can be re-acquired when {@link #isReadyToStart(Task)} is called again later.
+ *
+ * @param task the {@link Task} that was prevented from starting by another {@link TaskParker}
+ */
+ void onNotReadyToStart(Task<?> task);
+ }
+
public static class Lifecycle implements LifecycleListener {
private final WorkQueue workQueue;
@@ -288,9 +343,75 @@
/** An isolated queue. */
private class Executor extends ScheduledThreadPoolExecutor {
+ private class ParkedTask implements Comparable<ParkedTask> {
+ public final CancellableCountDownLatch latch = new CancellableCountDownLatch(1);
+ public final Task<?> task;
+ private final Long priority = priorityGenerator.getAndIncrement();
+
+ public ParkedTask(Task<?> task) {
+ this.task = task;
+ }
+
+ @Override
+ public int compareTo(ParkedTask o) {
+ return priority.compareTo(o.priority);
+ }
+
+ /**
+ * Cancel a parked {@link Task}.
+ *
+ * <p>Tasks awaiting in {@link #onStart(Task)} to be un-parked can be interrupted using this
+ * method.
+ */
+ public void cancel() {
+ latch.cancel();
+ }
+
+ public boolean isEqualTo(Task task) {
+ return this.task.taskId == task.taskId;
+ }
+ }
+
+ private class CancellableCountDownLatch extends CountDownLatch {
+ protected volatile boolean cancelled = false;
+
+ public CancellableCountDownLatch(int count) {
+ super(count);
+ }
+
+ /**
+ * Unblocks threads which are waiting until the latch has counted down to zero.
+ *
+ * <p>If the current count is zero, then this method returns immediately.
+ *
+ * <p>If the current count is greater than zero, then it decrements until the count reaches
+ * zero and causes all threads waiting on the latch using {@link CountDownLatch#await()} to
+ * throw an {@link InterruptedException}.
+ */
+ public void cancel() {
+ if (getCount() == 0) {
+ return;
+ }
+ this.cancelled = true;
+ while (getCount() > 0) {
+ countDown();
+ }
+ }
+
+ @Override
+ public void await() throws InterruptedException {
+ super.await();
+ if (cancelled) {
+ throw new InterruptedException();
+ }
+ }
+ }
+
private final ConcurrentHashMap<Integer, Task<?>> all;
private final ConcurrentHashMap<Runnable, Long> nanosPeriodByRunnable;
private final String queueName;
+ private final AtomicLong priorityGenerator = new AtomicLong();
+ private final PriorityBlockingQueue<ParkedTask> parked = new PriorityBlockingQueue<>();
Executor(int corePoolSize, final String queueName) {
super(
@@ -488,7 +609,17 @@
}
void remove(Task<?> task) {
- all.remove(task.getTaskId(), task);
+ boolean isRemoved = all.remove(task.getTaskId(), task);
+ if (isRemoved && !listeners.isEmpty()) {
+ cancelIfParked(task);
+ }
+ }
+
+ void cancelIfParked(Task<?> task) {
+ Optional<ParkedTask> parkedTask = parked.stream().filter(p -> p.isEqualTo(task)).findFirst();
+ if (parkedTask.isPresent()) {
+ parkedTask.get().cancel();
+ }
}
Task<?> getTask(int id) {
@@ -503,12 +634,86 @@
return all.values();
}
+ public void waitUntilReadyToStart(Task<?> task) {
+ if (!listeners.isEmpty() && !isReadyToStart(task)) {
+ incrementCorePoolSizeBy(1);
+ ParkedTask parkedTask = new ParkedTask(task);
+ parked.offer(parkedTask);
+ task.runningState.set(Task.State.PARKED);
+ try {
+ parkedTask.latch.await();
+ } catch (InterruptedException e) {
+ logger.atSevere().withCause(e).log("Parked Task(%s) Interrupted", task);
+ parked.remove(parkedTask);
+ incrementCorePoolSizeBy(-1);
+ }
+ }
+ }
+
public void onStart(Task<?> task) {
listeners.runEach(extension -> extension.get().onStart(task));
}
public void onStop(Task<?> task) {
listeners.runEach(extension -> extension.get().onStop(task));
+ updateParked();
+ }
+
+ protected boolean isReadyToStart(Task<?> task) {
+ MutableBoolean isReady = new MutableBoolean(true);
+ Set<TaskParker> readyParkers = new HashSet<>();
+ listeners.runEach(
+ extension -> {
+ if (isReady.isTrue()) {
+ TaskListener listener = extension.get();
+ if (listener instanceof TaskParker) {
+ TaskParker parker = (TaskParker) listener;
+ if (parker.isReadyToStart(task)) {
+ readyParkers.add(parker);
+ } else {
+ isReady.setFalse();
+ }
+ }
+ }
+ });
+
+ if (isReady.isFalse()) {
+ listeners.runEach(
+ extension -> {
+ TaskListener listener = extension.get();
+ if (readyParkers.contains(listener)) {
+ ((TaskParker) listener).onNotReadyToStart(task);
+ }
+ });
+ }
+ return isReady.getValue();
+ }
+
+ public void updateParked() {
+ ParkedTask ready = parked.poll();
+ if (ready == null) {
+ return;
+ }
+ List<ParkedTask> notReady = new ArrayList<>();
+ while (ready != null && !isReadyToStart(ready.task)) {
+ notReady.add(ready);
+ ready = parked.poll();
+ }
+ parked.addAll(notReady);
+
+ if (ready != null) {
+ incrementCorePoolSizeBy(-1);
+ ready.latch.countDown();
+ }
+ }
+
+ public synchronized void incrementCorePoolSizeBy(int i) {
+ super.setCorePoolSize(getCorePoolSize() + i);
+ }
+
+ @Override
+ public synchronized void setCorePoolSize(int s) {
+ super.setCorePoolSize(s);
}
}
@@ -556,13 +761,14 @@
// Ordered like this so ordinal matches the order we would
// prefer to see tasks sorted in: done before running,
// stopping before running, running before starting,
- // starting before ready, ready before sleeping.
+ // starting before parked, parked before ready, ready before sleeping.
//
DONE,
CANCELLED,
STOPPING,
RUNNING,
STARTING,
+ PARKED,
READY,
SLEEPING,
OTHER
@@ -694,12 +900,14 @@
@Override
public void run() {
- if (runningState.compareAndSet(null, State.STARTING)) {
+ if (runningState.compareAndSet(null, State.READY)) {
String oldThreadName = Thread.currentThread().getName();
try {
+ Thread.currentThread().setName(oldThreadName + "[" + this + "]");
+ executor.waitUntilReadyToStart(this); // Transitions to PARKED while not ready to start
+ runningState.set(State.STARTING);
executor.onStart(this);
runningState.set(State.RUNNING);
- Thread.currentThread().setName(oldThreadName + "[" + this + "]");
task.run();
} finally {
Thread.currentThread().setName(oldThreadName);
diff --git a/java/com/google/gerrit/server/restapi/change/GetPatch.java b/java/com/google/gerrit/server/restapi/change/GetPatch.java
index d8946a7..749a241 100644
--- a/java/com/google/gerrit/server/restapi/change/GetPatch.java
+++ b/java/com/google/gerrit/server/restapi/change/GetPatch.java
@@ -17,6 +17,7 @@
import static com.google.gerrit.git.ObjectIds.abbreviateName;
import static java.nio.charset.StandardCharsets.UTF_8;
+import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
@@ -51,6 +52,10 @@
@Option(name = "--path")
private String path;
+ /** 1-based index of the parent's position in the commit object. */
+ @Option(name = "--parent", metaVar = "parent-number")
+ private Integer parentNum;
+
@Inject
GetPatch(GitRepositoryManager repoManager) {
this.repoManager = repoManager;
@@ -58,7 +63,8 @@
@Override
public Response<BinaryResult> apply(RevisionResource rsrc)
- throws ResourceConflictException, IOException, ResourceNotFoundException {
+ throws BadRequestException, ResourceConflictException, IOException,
+ ResourceNotFoundException {
final Repository repo = repoManager.openRepository(rsrc.getProject());
boolean close = true;
try {
@@ -67,12 +73,16 @@
try {
final RevCommit commit = rw.parseCommit(rsrc.getPatchSet().commitId());
RevCommit[] parents = commit.getParents();
- if (parents.length > 1) {
+ if (parentNum == null && parents.length > 1) {
throw new ResourceConflictException("Revision has more than 1 parent.");
- } else if (parents.length == 0) {
+ }
+ if (parents.length == 0) {
throw new ResourceConflictException("Revision has no parent.");
}
- final RevCommit base = parents[0];
+ if (parentNum != null && (parentNum < 1 || parentNum > parents.length)) {
+ throw new BadRequestException(String.format("invalid parent number: %d", parentNum));
+ }
+ final RevCommit base = parents[parentNum == null ? 0 : parentNum - 1];
rw.parseBody(base);
bin =
diff --git a/java/com/google/gerrit/server/restapi/config/GetSummary.java b/java/com/google/gerrit/server/restapi/config/GetSummary.java
index 77af0f3..c76f0a4 100644
--- a/java/com/google/gerrit/server/restapi/config/GetSummary.java
+++ b/java/com/google/gerrit/server/restapi/config/GetSummary.java
@@ -77,6 +77,7 @@
int tasksTotal = pending.size();
int tasksStopping = 0;
int tasksRunning = 0;
+ int tasksParked = 0;
int tasksStarting = 0;
int tasksReady = 0;
int tasksSleeping = 0;
@@ -88,6 +89,9 @@
case RUNNING:
tasksRunning++;
break;
+ case PARKED:
+ tasksParked++;
+ break;
case STARTING:
tasksStarting++;
break;
@@ -108,6 +112,7 @@
taskSummary.total = toInteger(tasksTotal);
taskSummary.stopping = toInteger(tasksStopping);
taskSummary.running = toInteger(tasksRunning);
+ taskSummary.parked = toInteger(tasksParked);
taskSummary.starting = toInteger(tasksStarting);
taskSummary.ready = toInteger(tasksReady);
taskSummary.sleeping = toInteger(tasksSleeping);
@@ -245,6 +250,7 @@
public Integer total;
public Integer stopping;
public Integer running;
+ public Integer parked;
public Integer starting;
public Integer ready;
public Integer sleeping;
diff --git a/java/com/google/gerrit/sshd/commands/ShowQueue.java b/java/com/google/gerrit/sshd/commands/ShowQueue.java
index 00361ad..14915bf 100644
--- a/java/com/google/gerrit/sshd/commands/ShowQueue.java
+++ b/java/com/google/gerrit/sshd/commands/ShowQueue.java
@@ -134,6 +134,7 @@
switch (task.state) {
case DONE:
case CANCELLED:
+ case PARKED:
case STARTING:
case RUNNING:
case STOPPING:
@@ -212,6 +213,8 @@
return "";
case STARTING:
return "starting ...";
+ case PARKED:
+ return "parked .....";
case READY:
return "waiting ....";
case SLEEPING:
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java
index 0b28f6f..afa9bca 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java
@@ -85,6 +85,7 @@
i.signedOffBy ^= true;
i.allowBrowserNotifications ^= false;
i.allowSuggestCodeWhileCommenting ^= false;
+ i.allowAutocompletingComments ^= false;
i.diffPageSidebar = "plugin-insight";
i.diffView = DiffView.UNIFIED_DIFF;
i.my = new ArrayList<>();
@@ -99,6 +100,7 @@
assertThat(o.theme).isEqualTo(i.theme);
assertThat(o.allowBrowserNotifications).isEqualTo(i.allowBrowserNotifications);
assertThat(o.allowSuggestCodeWhileCommenting).isEqualTo(i.allowSuggestCodeWhileCommenting);
+ assertThat(o.allowAutocompletingComments).isEqualTo(i.allowAutocompletingComments);
assertThat(o.diffPageSidebar).isEqualTo(i.diffPageSidebar);
assertThat(o.disableKeyboardShortcuts).isEqualTo(i.disableKeyboardShortcuts);
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
index df7cd8f..1e03f2d 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
@@ -1186,6 +1186,52 @@
}
@Test
+ public void changePatch_multipleParents_success() throws Exception {
+ changeInTwoBranches("branchA", "a.txt", "branchB", "b.txt");
+ ChangeInput in = newMergeChangeInput("branchA", "branchB", "");
+ ChangeInfo change = assertCreateSucceeds(in);
+
+ RestResponse patchResp =
+ userRestSession.get("/changes/" + change.id + "/revisions/current/patch?parent=1");
+ patchResp.assertOK();
+ assertThat(new String(Base64.decode(patchResp.getEntityContent()), UTF_8))
+ .contains("+B content");
+
+ patchResp = userRestSession.get("/changes/" + change.id + "/revisions/current/patch?parent=2");
+ patchResp.assertOK();
+ assertThat(new String(Base64.decode(patchResp.getEntityContent()), UTF_8))
+ .contains("+A content");
+ }
+
+ @Test
+ public void changePatch_multipleParents_failure() throws Exception {
+ changeInTwoBranches("branchA", "a.txt", "branchB", "b.txt");
+ ChangeInput in = newMergeChangeInput("branchA", "branchB", "");
+ ChangeInfo change = assertCreateSucceeds(in);
+
+ RestResponse patchResp =
+ userRestSession.get("/changes/" + change.id + "/revisions/current/patch");
+ // Maintaining historic logic of failing with 409 Conflict in this case.
+ patchResp.assertConflict();
+ }
+
+ @Test
+ public void changePatch_parent_badRequest() throws Exception {
+ changeInTwoBranches("branchA", "a.txt", "branchB", "b.txt");
+ ChangeInput in = newMergeChangeInput("branchA", "branchB", "");
+ ChangeInfo change = assertCreateSucceeds(in);
+
+ RestResponse patchResp =
+ userRestSession.get("/changes/" + change.id + "/revisions/current/patch?parent=3");
+ // Parent 3 does not exist.
+ patchResp.assertBadRequest();
+
+ patchResp = userRestSession.get("/changes/" + change.id + "/revisions/current/patch?parent=0");
+ // Parent 0 does not exist.
+ patchResp.assertBadRequest();
+ }
+
+ @Test
@UseSystemTime
public void sha1sOfTwoNewChangesDiffer() throws Exception {
ChangeInput changeInput = newChangeInput(ChangeStatus.NEW);
diff --git a/javatests/com/google/gerrit/acceptance/server/util/TaskListenerIT.java b/javatests/com/google/gerrit/acceptance/server/util/TaskListenerIT.java
index 809cee9..9e9e1c2 100644
--- a/javatests/com/google/gerrit/acceptance/server/util/TaskListenerIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/util/TaskListenerIT.java
@@ -34,109 +34,147 @@
public class TaskListenerIT extends AbstractDaemonTest {
/**
* Use a LatchedMethod in a method to allow another thread to await the method's call. Once
- * called, the Latch.call() method will block until another thread calls its LatchedMethods's
- * complete() method.
+ * called, the call() method will block until another thread calls the complete() method or until
+ * a preset timeout is reached.
*/
- private static class LatchedMethod {
- private static final int AWAIT_TIMEOUT = 20;
- private static final TimeUnit AWAIT_TIMEUNIT = TimeUnit.MILLISECONDS;
-
- /** API class meant be used by the class whose method is being latched */
- private class Latch {
- /** Ensure that the latched method calls this on entry */
- public void call() {
- called.countDown();
- await(complete);
- }
- }
-
- public Latch latch = new Latch();
+ public static class LatchedMethod<T> {
+ private volatile T value;
private final CountDownLatch called = new CountDownLatch(1);
private final CountDownLatch complete = new CountDownLatch(1);
- /** Assert that the Latch's call() method has not yet been called */
+ /** Assert that the call() method has not yet been called */
public void assertUncalled() {
assertThat(called.getCount()).isEqualTo(1);
}
/**
- * Assert that a timeout does not occur while awaiting Latch's call() method to be called. Fails
- * if the waiting time elapses before Latch's call() method is called, otherwise passes.
+ * Assert that a timeout does not occur while awaiting the call() to be called. Fails if the
+ * waiting time elapses before the call() method is called, otherwise passes.
*/
- public void assertAwait() {
+ public void assertCalledEventually() {
assertThat(await(called)).isEqualTo(true);
}
- /** Unblock the Latch's call() method so that it can complete */
+ public T call() {
+ called.countDown();
+ await(complete);
+ return getValue();
+ }
+
+ public T call(T val) {
+ set(val);
+ return call();
+ }
+
+ public T callAndAwaitComplete() throws InterruptedException {
+ called.countDown();
+ complete.await();
+ return getValue();
+ }
+
public void complete() {
complete.countDown();
}
- @CanIgnoreReturnValue
- private static boolean await(CountDownLatch latch) {
- try {
- return latch.await(AWAIT_TIMEOUT, AWAIT_TIMEUNIT);
- } catch (InterruptedException e) {
- return false;
- }
+ public void set(T val) {
+ value = val;
+ }
+
+ public void complete(T val) {
+ set(val);
+ complete();
+ }
+
+ public void assertCalledEventuallyThenComplete(T val) {
+ assertCalledEventually();
+ complete(val);
+ }
+
+ protected T getValue() {
+ return value;
}
}
- private static class LatchedRunnable implements Runnable {
- public LatchedMethod run = new LatchedMethod();
+ public static class LatchedRunnable implements Runnable {
+ public LatchedMethod<?> run = new LatchedMethod<>();
+ public String name = "latched-runnable";
+
+ public LatchedRunnable(String name) {
+ this.name = name;
+ }
+
+ public LatchedRunnable() {}
@Override
public void run() {
- run.latch.call();
+ run.call();
+ }
+
+ @Override
+ public String toString() {
+ return name;
}
}
- private static class ForwardingListener implements TaskListener {
- public volatile TaskListener delegate;
+ public static class ForwardingListener<T extends TaskListener> implements TaskListener {
+ public volatile T delegate;
public volatile Task<?> task;
- public void resetDelegate(TaskListener listener) {
+ public void resetDelegate(T listener) {
delegate = listener;
task = null;
}
@Override
public void onStart(Task<?> task) {
- if (delegate != null) {
- if (this.task == null || this.task == task) {
- this.task = task;
- delegate.onStart(task);
- }
+ if (isDelegatable(task)) {
+ delegate.onStart(task);
}
}
@Override
public void onStop(Task<?> task) {
+ if (isDelegatable(task)) {
+ delegate.onStop(task);
+ }
+ }
+
+ protected boolean isDelegatable(Task<?> task) {
if (delegate != null) {
if (this.task == task) {
- delegate.onStop(task);
+ return true;
+ }
+ if (this.task == null) {
+ this.task = task;
+ return true;
}
}
+ return false;
}
}
- private static class LatchedListener implements TaskListener {
- public LatchedMethod onStart = new LatchedMethod();
- public LatchedMethod onStop = new LatchedMethod();
+ public static class LatchedListener implements TaskListener {
+ public LatchedMethod<?> onStart = new LatchedMethod<>();
+ public LatchedMethod<?> onStop = new LatchedMethod<>();
@Override
public void onStart(Task<?> task) {
- onStart.latch.call();
+ onStart.call();
}
@Override
public void onStop(Task<?> task) {
- onStop.latch.call();
+ onStop.call();
}
}
- private static ForwardingListener forwarder;
+ private static final int AWAIT_TIMEOUT = 20;
+ private static final TimeUnit AWAIT_TIMEUNIT = TimeUnit.MILLISECONDS;
+ private static final long MS_EMPTY_QUEUE =
+ TimeUnit.MILLISECONDS.convert(50, TimeUnit.MILLISECONDS);
+
+ private static ForwardingListener<TaskListener> forwarder;
@Inject private WorkQueue workQueue;
private ScheduledExecutorService executor;
@@ -149,9 +187,9 @@
return new AbstractModule() {
@Override
public void configure() {
- // Forwarder.delegate is empty on start to protect test listener from non test tasks
- // (such as the "Log File Manager") interference
- forwarder = new ForwardingListener(); // Only gets bound once for all tests
+ // Forwarder.delegate is empty on start to protect test listener from non-test tasks (such
+ // as the "Log File Manager") interference
+ forwarder = new ForwardingListener<>(); // Only gets bound once for all tests
bind(TaskListener.class).annotatedWith(Exports.named("listener")).toInstance(forwarder);
}
};
@@ -184,23 +222,23 @@
int size = assertQueueBlockedOnExecution(runnable);
// onStartThenRunThenOnStopAreCalled -> onStart...Called
- listener.onStart.assertAwait();
+ listener.onStart.assertCalledEventually();
assertQueueSize(size);
runnable.run.assertUncalled();
listener.onStop.assertUncalled();
listener.onStart.complete();
// onStartThenRunThenOnStopAreCalled -> ...ThenRun...Called
- runnable.run.assertAwait();
+ runnable.run.assertCalledEventually();
listener.onStop.assertUncalled();
runnable.run.complete();
// onStartThenRunThenOnStopAreCalled -> ...ThenOnStop...Called
- listener.onStop.assertAwait();
+ listener.onStop.assertCalledEventually();
assertQueueSize(size);
listener.onStop.complete();
- assertAwaitQueueSize(--size);
+ assertTaskCountIsEventually(--size);
}
@Test
@@ -208,7 +246,7 @@
int size = assertQueueBlockedOnExecution(runnable);
// firstBlocksSecond -> first...
- listener.onStart.assertAwait();
+ listener.onStart.assertCalledEventually();
assertQueueSize(size);
LatchedRunnable runnable2 = new LatchedRunnable();
@@ -219,35 +257,35 @@
assertQueueSize(size); // waiting on first
listener.onStart.complete();
- runnable.run.assertAwait();
+ runnable.run.assertCalledEventually();
assertQueueSize(size); // waiting on first
runnable2.run.assertUncalled();
runnable.run.complete();
- listener.onStop.assertAwait();
+ listener.onStop.assertCalledEventually();
assertQueueSize(size); // waiting on first
runnable2.run.assertUncalled();
listener.onStop.complete();
- runnable2.run.assertAwait();
+ runnable2.run.assertCalledEventually();
assertQueueSize(--size);
runnable2.run.complete();
- assertAwaitQueueSize(--size);
+ assertTaskCountIsEventually(--size);
}
@Test
public void states() throws Exception {
executor.execute(runnable);
- listener.onStart.assertAwait();
+ listener.onStart.assertCalledEventually();
assertStateIs(Task.State.STARTING);
listener.onStart.complete();
- runnable.run.assertAwait();
+ runnable.run.assertCalledEventually();
assertStateIs(Task.State.RUNNING);
runnable.run.complete();
- listener.onStop.assertAwait();
+ listener.onStop.assertCalledEventually();
assertStateIs(Task.State.STOPPING);
listener.onStop.complete();
@@ -255,8 +293,40 @@
assertStateIs(Task.State.DONE);
}
+ /** Fails if the waiting time elapses before the count is reached, otherwise passes */
+ public static void assertTaskCountIsEventually(WorkQueue workQueue, int count)
+ throws InterruptedException {
+ long ms = 0;
+ while (count != workQueue.getTasks().size()) {
+ assertThat(ms++).isLessThan(MS_EMPTY_QUEUE);
+ TimeUnit.MILLISECONDS.sleep(1);
+ }
+ }
+
+ public static void assertQueueSize(WorkQueue workQueue, int size) {
+ assertThat(workQueue.getTasks().size()).isEqualTo(size);
+ }
+
+ @CanIgnoreReturnValue
+ public static boolean await(CountDownLatch latch) {
+ try {
+ return latch.await(AWAIT_TIMEOUT, AWAIT_TIMEUNIT);
+ } catch (InterruptedException e) {
+ return false;
+ }
+ }
+
+ public void assertTaskCountIsEventually(int count) throws InterruptedException {
+ TaskListenerIT.assertTaskCountIsEventually(workQueue, count);
+ }
+
+ public static void assertStateIs(Task<?> task, Task.State state) {
+ assertThat(task).isNotNull();
+ assertThat(task.getState()).isEqualTo(state);
+ }
+
private void assertStateIs(Task.State state) {
- assertThat(forwarder.task.getState()).isEqualTo(state);
+ assertStateIs(forwarder.task, state);
}
private int assertQueueBlockedOnExecution(Runnable runnable) {
@@ -267,19 +337,10 @@
}
private void assertQueueSize(int size) {
- assertThat(workQueue.getTasks().size()).isEqualTo(size);
+ assertQueueSize(workQueue, size);
}
private void assertAwaitQueueIsEmpty() throws InterruptedException {
- assertAwaitQueueSize(0);
- }
-
- /** Fails if the waiting time elapses before the count is reached, otherwise passes */
- private void assertAwaitQueueSize(int size) throws InterruptedException {
- long i = 0;
- do {
- TimeUnit.NANOSECONDS.sleep(100);
- assertThat(i++).isLessThan(1000);
- } while (size != workQueue.getTasks().size());
+ assertTaskCountIsEventually(0);
}
}
diff --git a/javatests/com/google/gerrit/acceptance/server/util/TaskParkerIT.java b/javatests/com/google/gerrit/acceptance/server/util/TaskParkerIT.java
new file mode 100644
index 0000000..3b82ebe
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/server/util/TaskParkerIT.java
@@ -0,0 +1,548 @@
+// Copyright (C) 2022 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.acceptance.server.util;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.server.util.TaskListenerIT.LatchedMethod;
+import com.google.gerrit.acceptance.server.util.TaskListenerIT.LatchedRunnable;
+import com.google.gerrit.extensions.annotations.Exports;
+import com.google.gerrit.server.git.WorkQueue;
+import com.google.gerrit.server.git.WorkQueue.Task;
+import com.google.gerrit.server.git.WorkQueue.Task.State;
+import com.google.gerrit.server.git.WorkQueue.TaskListener;
+import com.google.gerrit.server.git.WorkQueue.TaskParker;
+import com.google.inject.AbstractModule;
+import com.google.inject.Inject;
+import com.google.inject.Module;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledThreadPoolExecutor;
+import java.util.concurrent.Semaphore;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+public class TaskParkerIT extends AbstractDaemonTest {
+ private static class ForwardingParker extends TaskListenerIT.ForwardingListener<LatchedParker>
+ implements TaskParker {
+ public AtomicInteger isReadyToStartCounter = new AtomicInteger(0);
+ public AtomicInteger onNotReadyToStartCounter = new AtomicInteger(0);
+
+ @Override
+ public boolean isReadyToStart(Task<?> task) {
+ isReadyToStartCounter.incrementAndGet();
+ if (isDelegatable(task)) {
+ return delegate.isReadyToStart(task);
+ }
+ return true;
+ }
+
+ @Override
+ public void onNotReadyToStart(Task<?> task) {
+ onNotReadyToStartCounter.incrementAndGet();
+ if (isDelegatable(task)) {
+ delegate.onNotReadyToStart(task);
+ }
+ }
+
+ public void resetCounters() {
+ isReadyToStartCounter.getAndSet(0);
+ onNotReadyToStartCounter.getAndSet(0);
+ }
+ }
+
+ public static class LatchedParker extends TaskListenerIT.LatchedListener implements TaskParker {
+ private static final String EXPENSIVE_TASK = "expensive-task";
+ private final Semaphore expensiveTaskSemaphore = new Semaphore(1, true);
+ public volatile LatchedMethod<Boolean> isReadyToStart = new LatchedMethod<>();
+ public volatile LatchedMethod<?> onNotReadyToStart = new LatchedMethod<>();
+
+ @Override
+ public boolean isReadyToStart(Task<?> task) {
+ Boolean rtn = isReadyToStart.call();
+ if (EXPENSIVE_TASK.equals(task.toString()) && !expensiveTaskSemaphore.tryAcquire()) {
+ return false;
+ }
+ isReadyToStart = new LatchedMethod<>();
+ if (rtn != null) {
+ return rtn;
+ }
+ return true;
+ }
+
+ @Override
+ public void onNotReadyToStart(Task<?> task) {
+ onNotReadyToStart.call();
+ onNotReadyToStart = new LatchedMethod<>();
+ }
+
+ @Override
+ public void onStop(Task<?> task) {
+ if (EXPENSIVE_TASK.equals(task.toString())) {
+ expensiveTaskSemaphore.release();
+ }
+ super.onStop(task);
+ }
+ }
+
+ public static class LatchedForeverRunnable extends LatchedRunnable {
+ public LatchedForeverRunnable(String name) {
+ super(name);
+ }
+
+ @Override
+ public void run() {
+ try {
+ run.callAndAwaitComplete();
+ } catch (InterruptedException e) {
+ throw new RuntimeException(e);
+ }
+ }
+ }
+
+ private static ForwardingParker forwarder;
+ private static ForwardingParker forwarder2;
+ public static final long TIMEOUT = TimeUnit.MILLISECONDS.convert(200, TimeUnit.MILLISECONDS);
+
+ private final LatchedParker parker = new LatchedParker();
+
+ @Inject private WorkQueue workQueue;
+ private ScheduledExecutorService executor;
+
+ @Before
+ public void setupExecutorAndForwarder() throws InterruptedException {
+ executor = workQueue.createQueue(1, "TaskParkers");
+ // "Log File Manager"s are likely running and will interfere with tests
+ while (0 != workQueue.getTasks().size()) {
+ for (Task<?> t : workQueue.getTasks()) {
+ t.cancel(true);
+ }
+ TimeUnit.MILLISECONDS.sleep(1);
+ }
+ forwarder.delegate = parker;
+ forwarder.task = null;
+ forwarder.resetCounters();
+ forwarder2.delegate = null; // load only if test needs it
+ forwarder2.task = null;
+ forwarder2.resetCounters();
+ }
+
+ @After
+ public void shutdownExecutor() throws InterruptedException {
+ executor.shutdownNow();
+ executor.awaitTermination(1, TimeUnit.SECONDS);
+ }
+
+ @Override
+ public Module createModule() {
+ return new AbstractModule() {
+ @Override
+ public void configure() {
+ // Forwarder.delegate is empty on start to protect test parker from non-test tasks (such as
+ // the "Log File Manager") interference
+ forwarder = new ForwardingParker(); // Only gets bound once for all tests
+ bind(TaskListener.class).annotatedWith(Exports.named("parker")).toInstance(forwarder);
+ forwarder2 = new ForwardingParker();
+ bind(TaskListener.class).annotatedWith(Exports.named("parker2")).toInstance(forwarder2);
+ }
+ };
+ }
+
+ @Test
+ public void noParkFlow() throws Exception {
+ LatchedRunnable runnable = new LatchedRunnable();
+
+ assertTaskCountIs(0);
+ assertThat(forwarder.task).isEqualTo(null);
+ parker.isReadyToStart.assertUncalled();
+ parker.onNotReadyToStart.assertUncalled();
+ parker.onStart.assertUncalled();
+ runnable.run.assertUncalled();
+ parker.onStop.assertUncalled();
+ assertCorePoolSizeIs(1);
+
+ executor.execute(runnable);
+ assertTaskCountIs(1);
+
+ parker.isReadyToStart.assertCalledEventually();
+ assertTaskCountIs(1);
+ assertStateIs(State.READY);
+ parker.onNotReadyToStart.assertUncalled();
+ parker.onStart.assertUncalled();
+ runnable.run.assertUncalled();
+ parker.onStop.assertUncalled();
+
+ parker.isReadyToStart.complete();
+ parker.onStart.assertCalledEventually();
+ assertStateIs(State.STARTING);
+ assertTaskCountIs(1);
+ parker.onNotReadyToStart.assertUncalled();
+ runnable.run.assertUncalled();
+ parker.onStop.assertUncalled();
+
+ parker.onStart.complete();
+ runnable.run.assertCalledEventually();
+ assertStateIs(State.RUNNING);
+ assertTaskCountIs(1);
+ parker.onNotReadyToStart.assertUncalled();
+ parker.onStop.assertUncalled();
+
+ runnable.run.complete();
+ parker.onStop.assertCalledEventually();
+ assertStateIs(State.STOPPING);
+ assertTaskCountIs(1);
+ parker.onNotReadyToStart.assertUncalled();
+
+ parker.onStop.complete();
+ assertTaskCountIsEventually(0);
+ assertStateIs(State.DONE);
+ parker.onNotReadyToStart.assertUncalled();
+ assertCorePoolSizeIs(1);
+ assertCounterIsEventually(forwarder.isReadyToStartCounter, 1);
+ assertCounter(forwarder.onNotReadyToStartCounter, 0);
+ }
+
+ @Test
+ public void parkFirstSoSecondRuns() throws Exception {
+ LatchedRunnable runnable1 = new LatchedRunnable();
+ LatchedRunnable runnable2 = new LatchedRunnable();
+ assertCorePoolSizeIs(1);
+
+ executor.execute(runnable1);
+ parker.isReadyToStart.assertCalledEventually();
+ Task<?> task1 = forwarder.task; // task for runnable1
+ assertCounterIsEventually(forwarder.isReadyToStartCounter, 1);
+ assertCounter(forwarder.onNotReadyToStartCounter, 0);
+ executor.execute(runnable2);
+ assertTaskCountIs(2);
+ parker.onNotReadyToStart.assertUncalled();
+ parker.onStart.assertUncalled();
+ runnable1.run.assertUncalled();
+ parker.onStop.assertUncalled();
+
+ // park runnable1
+ parker.isReadyToStart.complete(false);
+ assertCorePoolSizeIsEventually(2);
+ assertStateIs(task1, State.PARKED);
+
+ runnable2.run.assertCalledEventually();
+ assertTaskCountIs(2);
+ parker.onNotReadyToStart.assertUncalled();
+ parker.onStart.assertUncalled();
+ runnable1.run.assertUncalled();
+ parker.onStop.assertUncalled();
+ assertStateIs(task1, State.PARKED);
+
+ assertCounterIsEventually(forwarder.isReadyToStartCounter, 2);
+ assertCounter(forwarder.onNotReadyToStartCounter, 0);
+ runnable2.run.complete();
+
+ parker.isReadyToStart.assertCalledEventually();
+ parker.onNotReadyToStart.assertUncalled();
+ parker.onStart.assertUncalled();
+ runnable1.run.assertUncalled();
+ parker.onStop.assertUncalled();
+
+ parker.isReadyToStart.complete(true);
+ parker.onStart.assertCalledEventually();
+ assertStateIs(task1, State.STARTING);
+ assertTaskCountIsEventually(1);
+ parker.onNotReadyToStart.assertUncalled();
+ runnable1.run.assertUncalled();
+ parker.onStop.assertUncalled();
+
+ parker.onStart.complete();
+ runnable1.run.assertCalledEventually();
+ assertStateIs(task1, State.RUNNING);
+ assertTaskCountIs(1);
+ parker.onNotReadyToStart.assertUncalled();
+ parker.onStop.assertUncalled();
+
+ runnable1.run.complete();
+ parker.onStop.assertCalledEventually();
+ assertStateIs(task1, State.STOPPING);
+ assertTaskCountIs(1);
+ parker.onNotReadyToStart.assertUncalled();
+
+ parker.onStop.complete();
+ assertCorePoolSizeIsEventually(1);
+ assertTaskCountIsEventually(0);
+ assertStateIs(task1, State.DONE);
+ parker.onNotReadyToStart.assertUncalled();
+ assertCounterIsEventually(forwarder.isReadyToStartCounter, 3);
+ assertCounter(forwarder.onNotReadyToStartCounter, 0);
+ }
+
+ @Test
+ public void unParkPriorityOrder() throws Exception {
+ LatchedRunnable runnable1 = new LatchedRunnable();
+ LatchedRunnable runnable2 = new LatchedRunnable();
+ LatchedRunnable runnable3 = new LatchedRunnable();
+
+ // park runnable1
+ assertCorePoolSizeIs(1);
+ executor.execute(runnable1);
+ parker.isReadyToStart.assertCalledEventuallyThenComplete(false);
+ Task<?> task1 = forwarder.task; // task for runnable1
+ assertStateIsEventually(task1, State.PARKED);
+ assertCounterIsEventually(forwarder.isReadyToStartCounter, 1);
+ assertCounter(forwarder.onNotReadyToStartCounter, 0);
+ assertTaskCountIsEventually(1);
+ assertCorePoolSizeIsEventually(2);
+
+ // park runnable2
+ forwarder.resetDelegate(parker);
+ executor.execute(runnable2);
+ parker.isReadyToStart.assertCalledEventuallyThenComplete(false);
+ Task<?> task2 = forwarder.task; // task for runnable2
+ assertStateIsEventually(task2, State.PARKED);
+
+ assertCounterIsEventually(forwarder.isReadyToStartCounter, 2);
+ assertCounter(forwarder.onNotReadyToStartCounter, 0);
+ assertTaskCountIsEventually(2);
+ assertCorePoolSizeIsEventually(3);
+
+ // set parker to ready and execute runnable3
+ forwarder.resetDelegate(parker);
+ executor.execute(runnable3);
+
+ // assert runnable3 finishes executing and runnable1, runnable2 stay parked
+ assertCounterIsEventually(forwarder.isReadyToStartCounter, 3);
+ assertCounter(forwarder.onNotReadyToStartCounter, 0);
+ parker.isReadyToStart.assertCalledEventually();
+ Task<?> task3 = forwarder.task; // task for runnable3
+ assertStateIs(task3, State.READY);
+ parker.isReadyToStart.complete(true);
+ parker.onStart.assertCalledEventually();
+ assertStateIs(task3, State.STARTING);
+ parker.onStart.complete();
+ runnable3.run.assertCalledEventually();
+ assertStateIs(task3, State.RUNNING);
+ runnable1.run.assertUncalled();
+ runnable2.run.assertUncalled();
+ runnable3.run.complete();
+ parker.onStop.assertCalledEventually();
+ assertStateIs(task3, State.STOPPING);
+ parker.onStop.complete();
+ assertTaskCountIsEventually(2);
+ assertStateIs(task3, State.DONE);
+
+ // assert runnable1 finishes executing and runnable2 stays parked
+ runnable1.run.assertCalledEventually();
+ assertStateIs(task1, State.RUNNING);
+ assertCounterIsEventually(forwarder.isReadyToStartCounter, 4);
+ assertCounter(forwarder.onNotReadyToStartCounter, 0);
+ runnable2.run.assertUncalled();
+ assertStateIs(task2, State.PARKED);
+ runnable1.run.complete();
+ assertCorePoolSizeIsEventually(2);
+ assertTaskCountIsEventually(1);
+ assertStateIs(task1, State.DONE);
+
+ // assert runnable2 finishes executing
+ runnable2.run.assertCalledEventually();
+ assertStateIs(task2, State.RUNNING);
+ assertCounterIsEventually(forwarder.isReadyToStartCounter, 5);
+ assertCounter(forwarder.onNotReadyToStartCounter, 0);
+ runnable2.run.complete();
+ assertCorePoolSizeIsEventually(1);
+ assertTaskCountIsEventually(0);
+ assertStateIs(task2, State.DONE);
+ }
+
+ @Test
+ public void notReadyToStartIsCalledOnReadyListenerWhenAnotherListenerIsNotReady()
+ throws InterruptedException {
+ LatchedRunnable runnable1 = new LatchedRunnable();
+ LatchedRunnable runnable2 = new LatchedRunnable();
+
+ LatchedParker parker2 = new LatchedParker();
+ forwarder2.delegate = parker2;
+
+ // park runnable1 (parker1 is ready and parker2 is not ready)
+ assertCorePoolSizeIs(1);
+ executor.execute(runnable1);
+ parker2.isReadyToStart.complete(false);
+
+ assertTaskCountIsEventually(1);
+ assertCorePoolSizeIsEventually(2);
+
+ assertCounterIsEventually(forwarder.isReadyToStartCounter, 1);
+ assertCounterIsEventually(forwarder.onNotReadyToStartCounter, 1);
+ assertCounterIsEventually(forwarder2.isReadyToStartCounter, 1);
+ assertCounter(forwarder2.onNotReadyToStartCounter, 0);
+ Task<?> task1 = forwarder.task; // task for runnable1
+ assertStateIsEventually(task1, State.PARKED);
+
+ // set parker2 to ready and execute runnable-2
+ parker2.isReadyToStart.set(true);
+ forwarder.resetDelegate(parker);
+ forwarder2.resetDelegate(parker2);
+ executor.execute(runnable2);
+
+ assertCounterIsEventually(forwarder.isReadyToStartCounter, 2);
+ assertCounterIsEventually(forwarder.onNotReadyToStartCounter, 1);
+ assertCounterIsEventually(forwarder2.isReadyToStartCounter, 2);
+ assertCounter(forwarder2.onNotReadyToStartCounter, 0);
+ Task<?> task2 = forwarder.task; // task for runnable2
+
+ assertCorePoolSizeIsEventually(1);
+ runnable2.run.assertCalledEventually();
+ runnable2.run.complete();
+ assertTaskCountIsEventually(1);
+ assertStateIs(task2, State.DONE);
+
+ assertCounterIsEventually(forwarder.isReadyToStartCounter, 3);
+ assertCounterIsEventually(forwarder.onNotReadyToStartCounter, 1);
+ assertCounterIsEventually(forwarder2.isReadyToStartCounter, 3);
+ assertCounter(forwarder2.onNotReadyToStartCounter, 0);
+
+ runnable1.run.assertCalledEventually();
+ runnable1.run.complete();
+ assertTaskCountIsEventually(0);
+ assertStateIs(task1, State.DONE);
+ }
+
+ @Test
+ public void runFirstParkSecondUsingTaskName() throws InterruptedException {
+ LatchedForeverRunnable runnable1 = new LatchedForeverRunnable("expensive-task");
+ LatchedRunnable runnable2 = new LatchedRunnable("expensive-task");
+ LatchedParker parker = new LatchedParker();
+ executor = workQueue.createQueue(2, "TaskParkers");
+ assertCorePoolSizeIs(2);
+
+ forwarder.resetDelegate(parker);
+ executor.execute(runnable1);
+ parker.isReadyToStart.complete();
+ parker.onStart.complete();
+ runnable1.run.assertCalledEventually();
+ assertTaskCountIsEventually(1);
+ assertCorePoolSizeIs(2);
+ Task<?> task1 = forwarder.task; // task for runnable1
+ assertStateIs(task1, State.RUNNING);
+
+ forwarder.resetDelegate(parker);
+ executor.execute(runnable2);
+ parker.isReadyToStart.assertCalledEventually();
+ assertCorePoolSizeIsEventually(3);
+ Task<?> task2 = forwarder.task; // task for runnable2
+ assertStateIs(task2, State.PARKED);
+
+ forwarder.resetDelegate(parker);
+ runnable1.run.complete(); // unblock runnable1
+
+ assertCorePoolSizeIsEventually(2);
+ assertTaskCountIsEventually(0); // assert both tasks finish
+ }
+
+ @Test
+ public void interruptingParkedTaskDecrementsCorePoolSize() throws InterruptedException {
+ String taskName = "to-be-parked";
+ LatchedRunnable runnable1 = new LatchedRunnable(taskName);
+ assertCorePoolSizeIs(1);
+
+ // park runnable1
+ executor.execute(runnable1);
+ parker.isReadyToStart.assertCalledEventuallyThenComplete(false);
+ assertCorePoolSizeIsEventually(2);
+ assertStateIsEventually(forwarder.task, State.PARKED);
+
+ // interrupt the thread with parked task
+ for (Thread t : Thread.getAllStackTraces().keySet()) {
+ if (t.getName().contains(taskName)) {
+ t.interrupt();
+ break;
+ }
+ }
+
+ assertCorePoolSizeIsEventually(1);
+ }
+
+ @Test
+ public void canCancelParkedTask() throws InterruptedException {
+ LatchedRunnable runnable1 = new LatchedRunnable();
+ assertCorePoolSizeIs(1);
+
+ // park runnable1
+ executor.execute(runnable1);
+ parker.isReadyToStart.assertCalledEventuallyThenComplete(false);
+ assertCorePoolSizeIsEventually(2);
+ Task<?> task = forwarder.task;
+ assertStateIsEventually(task, State.PARKED);
+
+ // cancel parked task
+ task.cancel(true);
+
+ // assert core pool size is reduced and task is cancelled
+ assertCorePoolSizeIsEventually(1);
+ assertTaskCountIsEventually(0);
+ assertStateIs(State.CANCELLED);
+ }
+
+ private void assertTaskCountIs(int size) {
+ TaskListenerIT.assertQueueSize(workQueue, size);
+ }
+
+ private void assertTaskCountIsEventually(int count) throws InterruptedException {
+ TaskListenerIT.assertTaskCountIsEventually(workQueue, count);
+ }
+
+ private void assertCorePoolSizeIs(int count) {
+ assertThat(count).isEqualTo(((ScheduledThreadPoolExecutor) executor).getCorePoolSize());
+ }
+
+ private void assertCorePoolSizeIsEventually(int count) throws InterruptedException {
+ ScheduledThreadPoolExecutor scheduledThreadPoolExecutor =
+ (ScheduledThreadPoolExecutor) executor;
+ long ms = 0;
+ while (count != scheduledThreadPoolExecutor.getCorePoolSize()) {
+ assertThat(ms++).isLessThan(TIMEOUT);
+ TimeUnit.MILLISECONDS.sleep(1);
+ }
+ }
+
+ private void assertCounter(AtomicInteger counter, int desiredCount) {
+ assertThat(counter.get()).isEqualTo(desiredCount);
+ }
+
+ private void assertCounterIsEventually(AtomicInteger counter, int desiredCount)
+ throws InterruptedException {
+ long ms = 0;
+ while (desiredCount != counter.get()) {
+ assertThat(ms++).isLessThan(TIMEOUT);
+ TimeUnit.MILLISECONDS.sleep(1);
+ }
+ }
+
+ private void assertStateIs(Task.State state) {
+ TaskListenerIT.assertStateIs(forwarder.task, state);
+ }
+
+ private void assertStateIs(Task<?> task, Task.State state) {
+ TaskListenerIT.assertStateIs(task, state);
+ }
+
+ private void assertStateIsEventually(Task<?> task, Task.State state) throws InterruptedException {
+ long ms = 0;
+ assertThat(task).isNotNull();
+ while (!task.getState().equals(state)) {
+ assertThat(ms++).isLessThan(TIMEOUT);
+ TimeUnit.MILLISECONDS.sleep(1);
+ }
+ }
+}
diff --git a/javatests/com/google/gerrit/entities/converter/BUILD b/javatests/com/google/gerrit/entities/converter/BUILD
index 0ca9478..804397b 100644
--- a/javatests/com/google/gerrit/entities/converter/BUILD
+++ b/javatests/com/google/gerrit/entities/converter/BUILD
@@ -4,16 +4,19 @@
name = "proto_converter_tests",
srcs = glob(["*.java"]),
deps = [
+ "//java/com/google/gerrit/common:annotations",
"//java/com/google/gerrit/entities",
"//java/com/google/gerrit/extensions:api",
"//java/com/google/gerrit/proto/testing",
"//java/com/google/gerrit/server",
"//lib:guava",
+ "//lib:guava-testlib",
"//lib:jgit",
"//lib:protobuf",
"//lib/guice",
"//lib/truth",
"//lib/truth:truth-proto-extension",
"//proto:entities_java_proto",
+ "@commons-lang3//jar",
],
)
diff --git a/javatests/com/google/gerrit/entities/converter/SafeProtoConverterTest.java b/javatests/com/google/gerrit/entities/converter/SafeProtoConverterTest.java
new file mode 100644
index 0000000..eb69d53
--- /dev/null
+++ b/javatests/com/google/gerrit/entities/converter/SafeProtoConverterTest.java
@@ -0,0 +1,286 @@
+package com.google.gerrit.entities.converter;
+
+import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.primitives.Primitives;
+import com.google.common.reflect.ClassPath;
+import com.google.common.reflect.ClassPath.ClassInfo;
+import com.google.common.testing.ArbitraryInstances;
+import com.google.gerrit.common.ConvertibleToProto;
+import com.google.gerrit.common.Nullable;
+import com.google.protobuf.Descriptors.FieldDescriptor;
+import com.google.protobuf.Message;
+import com.google.protobuf.MessageLite;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Field;
+import java.lang.reflect.Modifier;
+import java.lang.reflect.ParameterizedType;
+import java.lang.reflect.Type;
+import java.sql.Timestamp;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Comparator;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Stream;
+import javax.annotation.Nonnull;
+import org.apache.commons.lang3.builder.EqualsBuilder;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
+import org.junit.runners.Parameterized.Parameters;
+import org.junit.runners.Suite;
+
+@RunWith(Suite.class)
+@Suite.SuiteClasses({
+ SafeProtoConverterTest.ListSafeProtoConverterTest.class, //
+ SafeProtoConverterTest.PerTypeSafeProtoConverterTest.class, //
+})
+public class SafeProtoConverterTest {
+ public static class ListSafeProtoConverterTest {
+ @Test
+ public void areAllConvertersEnums() throws Exception {
+ Stream<? extends Class<?>> safeConverters =
+ ClassPath.from(ClassLoader.getSystemClassLoader()).getAllClasses().stream()
+ .filter(type -> type.getPackageName().contains("gerrit"))
+ .map(ClassInfo::load)
+ .filter(SafeProtoConverter.class::isAssignableFrom)
+ .filter(clz -> !SafeProtoConverter.class.equals(clz));
+ // Safe converters must be enums. See also `isConverterAValidEnum` test below.
+ assertThat(safeConverters.allMatch(Class::isEnum)).isTrue();
+ }
+ }
+
+ @RunWith(Parameterized.class)
+ public static class PerTypeSafeProtoConverterTest {
+ @Parameter(0)
+ public SafeProtoConverter<Message, Object> converter;
+
+ @Parameter(1)
+ public String converterName;
+
+ @Parameters(name = "PerTypeSafeProtoConverterTest${1}")
+ public static ImmutableList<Object[]> listSafeConverters() throws Exception {
+ return ClassPath.from(ClassLoader.getSystemClassLoader()).getAllClasses().stream()
+ .filter(type -> type.getPackageName().contains("gerrit"))
+ .map(ClassInfo::load)
+ .filter(SafeProtoConverter.class::isAssignableFrom)
+ .filter(clz -> !SafeProtoConverter.class.equals(clz))
+ .filter(Class::isEnum)
+ .map(clz -> (SafeProtoConverter<Message, Object>) clz.getEnumConstants()[0])
+ .map(clz -> new Object[] {clz, clz.getClass().getSimpleName()})
+ .collect(toImmutableList());
+ }
+
+ /**
+ * For rising visibility, all Java Entity classes which have a {@link SafeProtoConverter}, must
+ * be annotated with {@link ConvertibleToProto}.
+ */
+ @Test
+ public void isJavaClassMarkedAsConvertibleToProto() {
+ assertThat(converter.getEntityClass().getDeclaredAnnotation(ConvertibleToProto.class))
+ .isNotNull();
+ }
+
+ /**
+ * All {@link SafeProtoConverter} implementations must be enums with a single instance. Please
+ * prefer descriptive enum and instance names, such as {@code
+ * MyTypeConverter::MY_TYPE_CONVERTER}.
+ */
+ @Test
+ public void isConverterAValidEnum() {
+ assertThat(converter.getClass().isEnum()).isTrue();
+ assertThat(converter.getClass().getEnumConstants().length).isEqualTo(1);
+ }
+
+ /**
+ * If this test fails, it's likely that you added a field to a Java class that has a {@link
+ * SafeProtoConverter} set, or that you have changed the default value for such a field. Please
+ * update the corresponding proto accordingly.
+ */
+ @Test
+ public void javaDefaultsKeptOnDoubleConversion() {
+ Object orig;
+ try {
+ orig = buildObjectWithFullFieldsOrThrow(converter.getEntityClass());
+ } catch (Exception e) {
+ throw new IllegalStateException(
+ String.format(
+ "Failed to build object for type %s, this likely means the buildObjectWithFullFieldsOrThrow should be adapted.",
+ converter.getEntityClass().getName()),
+ e);
+ }
+ Object res = converter.fromProto(converter.toProto(converter.getEntityClass().cast(orig)));
+ assertThat(orig).isEqualTo(res);
+ // If this assertion fails, it's likely that you forgot to update the `equals` method to
+ // include your new field.
+ assertThat(EqualsBuilder.reflectionEquals(orig, res)).isTrue();
+ }
+
+ /**
+ * If this test fails, it's likely that you added a field to a proto that has a {@link
+ * SafeProtoConverter} set, or that you have changed the default value for such a field. Please
+ * update the corresponding Java class accordingly.
+ */
+ @Test
+ public void protoDefaultsKeptOnDoubleConversion() {
+ Message defaultInstance = getProtoDefaultInstance(converter.getProtoClass());
+ Message preFilled = explicitlyFillProtoDefaults(defaultInstance);
+ Message resFromDefault =
+ converter.toProto(converter.fromProto(converter.getProtoClass().cast(preFilled)));
+ Message resFromPrefilled =
+ converter.toProto(converter.fromProto(converter.getProtoClass().cast(preFilled)));
+ assertThat(resFromDefault).isEqualTo(preFilled);
+ assertThat(resFromPrefilled).isEqualTo(preFilled);
+ }
+
+ @Nullable
+ private static Object buildObjectWithFullFieldsOrThrow(Class<?> clz) throws Exception {
+ if (clz == null) {
+ return null;
+ }
+ Object obj = construct(clz);
+ if (isSimple(clz)) {
+ return obj;
+ }
+ for (Field field : clz.getDeclaredFields()) {
+ if (Modifier.isStatic(field.getModifiers())) {
+ continue;
+ }
+ Class<?> parameterizedType = getParameterizedType(field);
+ if (!field.getType().isArray()
+ && !Map.class.isAssignableFrom(field.getType())
+ && !Collection.class.isAssignableFrom(field.getType())) {
+ if (!field.trySetAccessible()) {
+ return null;
+ }
+ field.set(obj, buildObjectWithFullFieldsOrThrow(field.getType()));
+ } else if (Collection.class.isAssignableFrom(field.getType())
+ && parameterizedType != null) {
+ field.set(obj, ImmutableList.of(buildObjectWithFullFieldsOrThrow(parameterizedType)));
+ }
+ }
+ return obj;
+ }
+
+ /**
+ * AutoValue annotations are not retained on runtime. We can only find out if a class is an
+ * AutoValue, by trying to load the expected AutoValue class.
+ *
+ * <p>For the class {@code package.Clz}, the AutoValue class name is {@code
+ * package.AutoValue_Clz}, for {@code package.Enclosing$Clz}, it is {@code
+ * package.AutoValue_Enclosing_Clz}
+ */
+ static Optional<Class<?>> toRepresentingAutoValueClass(Class<?> clz) {
+ String origClzName = clz.getName();
+ String autoValueClzName =
+ origClzName.substring(0, origClzName.lastIndexOf("."))
+ + ".AutoValue_"
+ + origClzName.substring(origClzName.lastIndexOf(".") + 1);
+ autoValueClzName = autoValueClzName.replace('$', '_');
+ try {
+ return Optional.of(clz.getClassLoader().loadClass(autoValueClzName));
+ } catch (Exception e) {
+ return Optional.empty();
+ }
+ }
+
+ @Nullable
+ private static Class<?> getParameterizedType(Field field) {
+ if (!Collection.class.isAssignableFrom(field.getType())) {
+ return null;
+ }
+ Type genericType = field.getGenericType();
+ if (genericType instanceof ParameterizedType) {
+ return (Class<?>) ((ParameterizedType) genericType).getActualTypeArguments()[0];
+ }
+ return null;
+ }
+
+ @Nonnull
+ static Object construct(@Nonnull Class<?> clz) {
+ try {
+ Object arbitrary = ArbitraryInstances.get(clz);
+ if (arbitrary != null) {
+ return arbitrary;
+ }
+ Optional<Class<?>> optionalAutoValueRepresentation = toRepresentingAutoValueClass(clz);
+ if (optionalAutoValueRepresentation.isPresent()) {
+ return construct(optionalAutoValueRepresentation.get());
+ }
+ Constructor<?> constructor =
+ Arrays.stream(clz.getDeclaredConstructors())
+ // Filter out copy constructors
+ .filter(
+ c ->
+ c.getParameterCount() != 1
+ || !c.getParameterTypes()[0].isAssignableFrom(clz))
+ // Filter out private constructors which cannot be set accessible.
+ .filter(c -> c.canAccess(null) || c.trySetAccessible())
+ .min(Comparator.comparingInt(Constructor::getParameterCount))
+ .get();
+ List<Object> args = new ArrayList<>();
+ for (Class<?> f : constructor.getParameterTypes()) {
+ args.add(construct(f));
+ }
+ return constructor.newInstance(args.toArray());
+ } catch (Exception e) {
+ throw new IllegalStateException("Failed to construct class " + clz.getName(), e);
+ }
+ }
+
+ static boolean isSimple(Class<?> c) {
+ return c.isPrimitive()
+ || c.isEnum()
+ || Primitives.isWrapperType(c)
+ || String.class.isAssignableFrom(c)
+ || Timestamp.class.isAssignableFrom(c);
+ }
+
+ /**
+ * Returns the default instance for the given MessageLite class, if it has the {@code
+ * getDefaultInstance} static method.
+ *
+ * @param type the protobuf message class
+ * @throws IllegalArgumentException if the given class doesn't have the static {@code
+ * getDefaultInstance} method
+ */
+ public static <T extends MessageLite> T getProtoDefaultInstance(Class<T> type) {
+ try {
+ return type.cast(type.getMethod("getDefaultInstance").invoke(null));
+ } catch (ReflectiveOperationException | ClassCastException e) {
+ throw new IllegalStateException("Cannot get default instance for " + type, e);
+ }
+ }
+
+ private static Message explicitlyFillProtoDefaults(Message defaultInstance) {
+ Message.Builder res = defaultInstance.toBuilder();
+ for (FieldDescriptor f : defaultInstance.getDescriptorForType().getFields()) {
+ try {
+ if (f.getType().equals(FieldDescriptor.Type.MESSAGE)) {
+ if (f.isRepeated()) {
+ res.addRepeatedField(
+ f,
+ explicitlyFillProtoDefaults(
+ explicitlyFillProtoDefaults(
+ getProtoDefaultInstance(res.newBuilderForField(f).build().getClass()))));
+ } else {
+ res.setField(f, explicitlyFillProtoDefaults((Message) defaultInstance.getField(f)));
+ }
+ } else {
+ res.setField(f, defaultInstance.getField(f));
+ }
+ } catch (Exception e) {
+ throw new IllegalStateException("Failed to fill default instance for " + f.getName(), e);
+ }
+ }
+ return res.build();
+ }
+ }
+}
diff --git a/lib/highlightjs/BUILD b/lib/highlightjs/BUILD
index 4105d85..d55a273 100644
--- a/lib/highlightjs/BUILD
+++ b/lib/highlightjs/BUILD
@@ -12,6 +12,7 @@
srcs = [
"@ui_npm//highlight.js",
"@ui_npm//highlightjs-closure-templates",
+ "@ui_npm//highlightjs-epp",
"@ui_npm//highlightjs-structured-text",
],
config_file = "rollup.config.js",
diff --git a/lib/highlightjs/index.js b/lib/highlightjs/index.js
index c2d048d..811dec7 100644
--- a/lib/highlightjs/index.js
+++ b/lib/highlightjs/index.js
@@ -17,9 +17,11 @@
import hljs from 'highlight.js';
import soy from 'highlightjs-closure-templates';
+import epp from 'highlightjs-epp';
import iecst from 'highlightjs-structured-text';
hljs.registerLanguage('soy', soy);
+hljs.registerLanguage('epp', epp);
hljs.registerLanguage('iecst', iecst);
export default hljs;
diff --git a/lib/nongoogle_test.sh b/lib/nongoogle_test.sh
index 6865340..37cee04 100755
--- a/lib/nongoogle_test.sh
+++ b/lib/nongoogle_test.sh
@@ -11,6 +11,11 @@
grep 'name = "[^"]*"' ${bzl} | sed 's|^[^"]*"||g;s|".*$||g' | sort > $TMP/names
cat << EOF > $TMP/want
+auto-common
+auto-factory
+auto-service-annotations
+auto-value
+auto-value-annotations
cglib-3_2
commons-io
dropwizard-core
@@ -20,6 +25,7 @@
flogger-google-extensions
flogger-log4j-backend
flogger-system-backend
+gson
guava
guava-testlib
guice-assistedinject
diff --git a/modules/jgit b/modules/jgit
index c0b415f..d06722a 160000
--- a/modules/jgit
+++ b/modules/jgit
@@ -1 +1 @@
-Subproject commit c0b415fb028b4c1f29b6df749323bbb11599495d
+Subproject commit d06722a15998eeee04cfefd7c3c7e79fe67f1494
diff --git a/polygerrit-ui/app/api/embed.ts b/polygerrit-ui/app/api/embed.ts
index 2faeeda..d6425be 100644
--- a/polygerrit-ui/app/api/embed.ts
+++ b/polygerrit-ui/app/api/embed.ts
@@ -45,6 +45,7 @@
/** <gr-textarea> event when showing a hint */
export declare interface HintShownEventDetail {
hint: string;
+ oldValue: string;
}
/** <gr-textarea> event when a hint was dismissed */
diff --git a/polygerrit-ui/app/api/suggestions.ts b/polygerrit-ui/app/api/suggestions.ts
index c3089a9..b952180 100644
--- a/polygerrit-ui/app/api/suggestions.ts
+++ b/polygerrit-ui/app/api/suggestions.ts
@@ -72,6 +72,7 @@
export declare interface AutocompleteCommentResponse {
responseCode: ResponseCode;
completion?: string;
+ modelVersion?: string;
}
export declare interface SuggestCodeResponse {
diff --git a/polygerrit-ui/app/constants/constants.ts b/polygerrit-ui/app/constants/constants.ts
index 0fa58f4..b21663a 100644
--- a/polygerrit-ui/app/constants/constants.ts
+++ b/polygerrit-ui/app/constants/constants.ts
@@ -264,6 +264,7 @@
default_base_for_merges: DefaultBase.AUTO_MERGE,
allow_browser_notifications: false,
allow_suggest_code_while_commenting: true,
+ allow_autocompleting_comments: true,
diff_page_sidebar: 'NONE',
};
}
diff --git a/polygerrit-ui/app/constants/reporting.ts b/polygerrit-ui/app/constants/reporting.ts
index 37a17ba..643f8aa 100644
--- a/polygerrit-ui/app/constants/reporting.ts
+++ b/polygerrit-ui/app/constants/reporting.ts
@@ -102,6 +102,8 @@
APPLY_FIX_LOAD = 'ApplyFixLoad',
// Time to copy target to clipboard
COPY_TO_CLIPBOARD = 'CopyToClipboard',
+ // Time to autocomplete a comment
+ COMMENT_COMPLETION = 'CommentCompletion',
}
export enum Interaction {
@@ -156,4 +158,9 @@
// The very first reporting event with `ChangeId` set when visiting a change
// related page. Can be used as a starting point for user journeys.
CHANGE_ID_CHANGED = 'change-id-changed',
+
+ COMMENT_COMPLETION_SUGGESTION_SHOWN = 'comment-completion-suggestion-shown',
+ COMMENT_COMPLETION_SUGGESTION_ACCEPTED = 'comment-completion-suggestion-accepted',
+ COMMENT_COMPLETION_SAVE_DRAFT = 'comment-completion-save-draft',
+ COMMENT_COMPLETION_SUGGESTION_FETCHED = 'comment-completion-suggestion-fetched',
}
diff --git a/polygerrit-ui/app/elements/admin/gr-repo/gr-repo.ts b/polygerrit-ui/app/elements/admin/gr-repo/gr-repo.ts
index 4e8841e..2251321 100644
--- a/polygerrit-ui/app/elements/admin/gr-repo/gr-repo.ts
+++ b/polygerrit-ui/app/elements/admin/gr-repo/gr-repo.ts
@@ -815,7 +815,7 @@
config.max_object_size_limit.configured_value = '';
}
this.repoConfig = config;
- this.originalConfig = deepClone(config) as ConfigInfo;
+ this.originalConfig = deepClone<ConfigInfo>(config);
this.loading = false;
};
promises.push(repoConfigHelper());
@@ -920,7 +920,7 @@
this.repo,
this.formatRepoConfigForSave(this.repoConfig)
);
- this.originalConfig = deepClone(this.repoConfig) as ConfigInfo;
+ this.originalConfig = deepClone<ConfigInfo>(this.repoConfig);
this.pluginConfigChanged = false;
return;
}
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
index 50ec339..836d86a 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
@@ -485,7 +485,7 @@
@state() pluginsLoaded = false;
- @state() threadsWithSuggestions?: CommentThread[];
+ @state() threadsWithUnappliedSuggestions?: CommentThread[];
private readonly restApiService = getAppContext().restApiService;
@@ -579,8 +579,8 @@
);
subscribe(
this,
- () => this.getCommentsModel().threadsWithSuggestions$,
- x => (this.threadsWithSuggestions = x)
+ () => this.getCommentsModel().threadsWithUnappliedSuggestions$,
+ x => (this.threadsWithUnappliedSuggestions = x)
);
}
@@ -820,11 +820,11 @@
<div class="header" slot="header">Publish Change Edit</div>
<div class="main" slot="main">
${when(
- this.numberOfThreadsWithSuggestions() > 0,
+ this.numberOfThreadsWithUnappliedSuggestions() > 0,
() => html`<p class="info">
<gr-icon id="icon" icon="info" small></gr-icon>
- Heads Up! ${this.numberOfThreadsWithSuggestions()} comments have
- suggestions you can apply before publishing
+ Heads Up! ${this.numberOfThreadsWithUnappliedSuggestions()}
+ comments have suggestions you can apply before publishing
</p>`
)}
Do you really want to publish the edit?
@@ -2105,8 +2105,16 @@
}
private handlePublishEditTap() {
- assertIsDefined(this.confirmPublishEditDialog, 'confirmPublishEditDialog');
- this.showActionDialog(this.confirmPublishEditDialog);
+ if (this.numberOfThreadsWithUnappliedSuggestions() > 0) {
+ assertIsDefined(
+ this.confirmPublishEditDialog,
+ 'confirmPublishEditDialog'
+ );
+ this.showActionDialog(this.confirmPublishEditDialog);
+ } else {
+ // Skip confirmation dialog and publish immediately.
+ this.handlePublishEditConfirm();
+ }
}
private handleRebaseEditTap() {
@@ -2264,9 +2272,9 @@
fireNoBubbleNoCompose(this, 'stop-edit-tap', {});
}
- private numberOfThreadsWithSuggestions() {
- if (!this.threadsWithSuggestions) return 0;
- return this.threadsWithSuggestions.length;
+ private numberOfThreadsWithUnappliedSuggestions() {
+ if (!this.threadsWithUnappliedSuggestions) return 0;
+ return this.threadsWithUnappliedSuggestions.length;
}
}
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
index b2b045a..c76e928 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
@@ -123,7 +123,7 @@
import {Key, Modifier, whenVisible} from '../../../utils/dom-util';
import {GrThreadList} from '../gr-thread-list/gr-thread-list';
import {userModelToken} from '../../../models/user/user-model';
-import {accountsModelToken} from '../../../models/accounts-model/accounts-model';
+import {accountsModelToken} from '../../../models/accounts/accounts-model';
import {pluginLoaderToken} from '../../shared/gr-js-api-interface/gr-plugin-loader';
import {modalStyles} from '../../../styles/gr-modal-styles';
import {ironAnnouncerRequestAvailability} from '../../polymer-util';
diff --git a/polygerrit-ui/app/elements/change/gr-revision-parents/gr-revision-parents.ts b/polygerrit-ui/app/elements/change/gr-revision-parents/gr-revision-parents.ts
index 9cf5423..2dbbd26 100644
--- a/polygerrit-ui/app/elements/change/gr-revision-parents/gr-revision-parents.ts
+++ b/polygerrit-ui/app/elements/change/gr-revision-parents/gr-revision-parents.ts
@@ -3,6 +3,8 @@
* Copyright 2023 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
+import '../gr-commit-info/gr-commit-info';
+import '../../shared/gr-button/gr-button';
import {customElement, state} from 'lit/decorators.js';
import {css, html, HTMLTemplateResult, LitElement} from 'lit';
import {resolve} from '../../../models/dependency';
diff --git a/polygerrit-ui/app/elements/change/gr-revision-parents/gr-revision-parents_test.ts b/polygerrit-ui/app/elements/change/gr-revision-parents/gr-revision-parents_test.ts
index b9aa63d..d3b46ff 100644
--- a/polygerrit-ui/app/elements/change/gr-revision-parents/gr-revision-parents_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-revision-parents/gr-revision-parents_test.ts
@@ -167,7 +167,7 @@
The diff below may not be meaningful and may<br/>
even be hiding relevant changes.
<a href="/Documentation/user-review-ui.html#hazardous-rebases">Learn more</a>
- </p><p><gr-button link="">Show details</gr-button></p></div></div>`
+ </p><p><gr-button aria-disabled="false" link="" role="button" tabindex="0">Show details</gr-button></p></div></div>`
);
});
@@ -183,7 +183,7 @@
The diff below may not be meaningful and may<br/>
even be hiding relevant changes.
<a href="/Documentation/user-review-ui.html#hazardous-rebases">Learn more</a>
- </p><p><gr-button link="">Show details</gr-button></p></div></div>`
+ </p><p><gr-button aria-disabled="false" link="" role="button" tabindex="0">Show details</gr-button></p></div></div>`
);
});
@@ -235,7 +235,7 @@
The diff below may not be meaningful and may<br/>
even be hiding relevant changes.
<a href="/Documentation/user-review-ui.html#hazardous-rebases">Learn more</a>
- </p><p><gr-button link="">Show details</gr-button></p></div></div>`
+ </p><p><gr-button aria-disabled="false" link="" role="button" tabindex="0">Show details</gr-button></p></div></div>`
);
});
});
diff --git a/polygerrit-ui/app/elements/core/gr-account-dropdown/gr-account-dropdown.ts b/polygerrit-ui/app/elements/core/gr-account-dropdown/gr-account-dropdown.ts
index e63ac8f..644871a4 100644
--- a/polygerrit-ui/app/elements/core/gr-account-dropdown/gr-account-dropdown.ts
+++ b/polygerrit-ui/app/elements/core/gr-account-dropdown/gr-account-dropdown.ts
@@ -32,6 +32,9 @@
@property({type: Object})
account?: AccountInfo;
+ @property({type: Boolean})
+ showMobile?: boolean;
+
// Private but used in test
@state()
config?: ServerInfo;
@@ -45,6 +48,9 @@
@state()
private switchAccountUrl = '';
+ // private but used in test
+ @state() feedbackURL = '';
+
// Private but used in test
readonly getConfigModel = resolve(this, configModelToken);
@@ -56,6 +62,10 @@
cfg => {
this.config = cfg;
+ if (cfg?.gerrit?.report_bug_url) {
+ this.feedbackURL = cfg?.gerrit.report_bug_url;
+ }
+
if (cfg && cfg.auth && cfg.auth.switch_account_url) {
this.switchAccountUrl = cfg.auth.switch_account_url;
} else {
@@ -103,7 +113,11 @@
@tap-item-shortcuts=${this.handleShortcutsTap}
.horizontalAlign=${'right'}
>
- <span ?hidden=${this.hasAvatars}>${this.accountName(this.account)}</span>
+ ${this.showMobile && !this.hasAvatars
+ ? html`<gr-icon icon="account_circle" filled></gr-icon>`
+ : html`<span ?hidden=${this.hasAvatars}
+ >${this.accountName(this.account)}</span
+ >`}
<gr-avatar
.account=${this.account}
?hidden=${!this.hasAvatars}
@@ -135,6 +149,15 @@
const url = this.interpolateUrl(switchAccountUrl, replacements);
links.push({name: 'Switch account', url, external: true});
}
+ if (this.showMobile && this.feedbackURL) {
+ links.push({
+ name: 'Feedback',
+ id: 'feedback',
+ url: this.feedbackURL,
+ external: true,
+ target: '_blank',
+ });
+ }
links.push({name: 'Sign out', id: 'signout', url: '/logout'});
return links;
}
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 ae59fb6..3be77db 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
@@ -22,13 +22,14 @@
import {getAppContext} from '../../../services/app-context';
import {sharedStyles} from '../../../styles/shared-styles';
import {LitElement, PropertyValues, html, css} from 'lit';
-import {customElement, property, state} from 'lit/decorators.js';
+import {customElement, property, query, state} from 'lit/decorators.js';
import {fire} from '../../../utils/event-util';
import {resolve} from '../../../models/dependency';
import {configModelToken} from '../../../models/config/config-model';
import {userModelToken} from '../../../models/user/user-model';
import {pluginLoaderToken} from '../../shared/gr-js-api-interface/gr-plugin-loader';
import {subscribe} from '../../lit/subscription-controller';
+import {ifDefined} from 'lit/directives/if-defined.js';
type MainHeaderLink = RequireProperties<DropdownLink, 'url' | 'name'>;
@@ -104,6 +105,9 @@
AuthType.CUSTOM_EXTENSION,
]);
+const REL_NOOPENER = 'noopener';
+const REL_EXTERNAL = 'external';
+
declare global {
interface HTMLElementTagNameMap {
'gr-main-header': GrMainHeader;
@@ -149,6 +153,15 @@
// private but used in test
@state() feedbackURL = '';
+ @state() hamburgerClose? = false;
+
+ @query('.nav-sidebar') navSidebar?: HTMLDivElement;
+
+ @query('.modelBackground') modelBackground?: HTMLDivElement;
+
+ @query('.has-collapsible.active')
+ hasCollapsibleActive?: HTMLLIElement;
+
private readonly restApiService = getAppContext().restApiService;
private readonly getPluginLoader = resolve(this, pluginLoaderToken);
@@ -202,10 +215,29 @@
:host {
display: block;
}
- nav {
+ .hideOnDesktop {
+ display: none;
+ }
+
+ nav.hideOnMobile {
align-items: center;
display: flex;
}
+ nav.hideOnMobile ul {
+ list-style: none;
+ padding-left: var(--spacing-l);
+ }
+ nav.hideOnMobile .links > li {
+ cursor: default;
+ display: inline-block;
+ padding: 0;
+ position: relative;
+ }
+
+ .mobileTitle {
+ display: none;
+ }
+
.bigTitle {
color: var(--header-text-color);
font-size: var(--header-title-font-size);
@@ -215,7 +247,8 @@
.bigTitle:hover {
text-decoration: underline;
}
- .titleText {
+ .titleText,
+ .mobileTitleText {
/* Vertical alignment of icons and text with just block/inline display is too troublesome. */
display: flex;
align-items: center;
@@ -239,16 +272,45 @@
content: var(--header-title-content);
white-space: nowrap;
}
- ul {
- list-style: none;
- padding-left: var(--spacing-l);
+
+ .mobileTitleText::before {
+ --icon-width: var(
+ --header-icon-width,
+ var(--header-mobile-icon-size, var(--header-icon-size, 0))
+ );
+ --icon-height: var(
+ --header-icon-height,
+ var(--header-mobile-icon-size, var(--header-icon-size, 0))
+ );
+ background-image: var(--header-mobile-icon, var(--header-icon));
+ background-size: var(--mobile-icon-width, var(--icon-width))
+ var(--mobile-icon-height, var(--icon-height));
+ background-repeat: no-repeat;
+ content: '';
+ /* Any direct child of a flex element implicitly has 'display: block', but let's make that explicit here. */
+ display: block;
+ width: var(--mobile-icon-width, var(--icon-width));
+ height: var(--mobile-icon-height, var(--icon-height));
+ /* If size or height are set, then use 'spacing-m', 0px otherwise. */
+ margin-right: clamp(
+ 0px,
+ var(--mobile-icon-height, var(--icon-height)),
+ var(--spacing-m)
+ );
}
- .links > li {
- cursor: default;
- display: inline-block;
- padding: 0;
- position: relative;
+ .mobileTitleText::after {
+ /* The height will be determined by the line-height of the .bigTitle element. */
+ content: var(
+ --header-mobile-title-content,
+ var(--header-title-content)
+ );
+ white-space: nowrap;
+ text-overflow: ellipsis;
+ flex: 1;
+ overflow: hidden;
+ min-width: 0;
}
+
.linksTitle {
display: inline-block;
font-weight: var(--font-weight-bold);
@@ -264,7 +326,24 @@
flex: 1;
justify-content: flex-end;
}
- .rightItems gr-endpoint-decorator:not(:empty) {
+ .mobileRightItems {
+ align-items: center;
+ justify-content: flex-end;
+
+ display: inline-block;
+ vertical-align: middle;
+ cursor: pointer;
+ position: relative;
+ top: 0px;
+ right: 0px;
+ margin-right: 0;
+ margin-left: auto;
+ min-height: 50px;
+ padding-top: 12px;
+ }
+
+ .rightItems gr-endpoint-decorator:not(:empty),
+ .mobileRightItems gr-endpoint-decorator:not(:empty) {
margin-left: var(--spacing-l);
}
gr-smart-search {
@@ -299,13 +378,17 @@
}
:host([loading]) .accountContainer,
:host([loggedIn]) .loginButton,
- :host([loggedIn]) .registerButton {
+ :host([loggedIn]) .registerButton,
+ :host([loggedIn]) .moreMenu {
display: none;
}
:host([loggedIn]) .settingsButton,
:host([loggedIn]) gr-account-dropdown {
display: inline;
}
+ :host:not([loggedIn]) .moreMenu {
+ display: inline;
+ }
.accountContainer {
flex: 0 0 auto;
align-items: center;
@@ -363,7 +446,158 @@
margin-left: var(--spacing-m) !important;
}
gr-dropdown {
- padding: var(--spacing-m) 0 var(--spacing-m) var(--spacing-m);
+ padding: 0 var(--spacing-m);
+ }
+ .nav-sidebar {
+ background: var(--table-header-background-color);
+ width: 200px;
+ height: 100%;
+ display: block;
+ position: fixed;
+ left: -200px;
+ top: 0px;
+ transition: left 0.25s ease;
+ margin: 0;
+ border: 0;
+ overflow-y: auto;
+ overflow-x: hidden;
+ height: 100%;
+ margin-bottom: 15px 0;
+ box-shadow: 0 2px 5px 0 rgba(0, 0, 0, 0.26);
+ border-radius: 3px;
+ z-index: 2;
+ }
+ .nav-sidebar.visible {
+ left: 0px;
+ transition: left 0.25s ease;
+ width: 80%;
+ z-index: 200;
+ }
+ .mobileTitle {
+ position: relative;
+ display: block;
+ top: 10px;
+ font-size: 20px;
+ left: 100px;
+ right: 100px;
+ text-align: center;
+ text-overflow: ellipsis;
+ overflow: hidden;
+ width: 50%;
+ }
+ .nav-header {
+ display: flex;
+ }
+ .hamburger {
+ display: inline-block;
+ vertical-align: middle;
+ height: 50px;
+ cursor: pointer;
+ margin: 0;
+ position: absolute;
+ top: 0;
+ left: 0;
+ padding: 12px;
+ z-index: 200;
+ }
+ .nav-sidebar ul {
+ list-style-type: none;
+ margin: 0;
+ padding: 0;
+ display: block;
+ padding-top: 50px;
+ }
+ .nav-sidebar li {
+ list-style-type: none;
+ margin: 0;
+ padding: 0;
+ display: inline-block;
+ position: relative;
+ font-size: 14;
+ color: var(--primary-text-color);
+ display: block;
+ }
+ .cover {
+ background: rgba(0, 0, 0, 0.5);
+ position: fixed;
+ top: 0;
+ bottom: 0;
+ left: 0;
+ right: 0;
+ overflow: none;
+ z-index: 199;
+ }
+ .hideOnDesktop {
+ display: block;
+ }
+ nav.hideOnMobile {
+ display: none;
+ }
+ .nav-sidebar .menu ul {
+ list-style-type: none;
+ margin: 0;
+ padding: 0;
+ display: block;
+ padding-top: 50px;
+ }
+ .nav-sidebar .menu li {
+ list-style-type: none;
+ margin: 0;
+ padding: 0;
+ display: inline-block;
+ position: relative;
+ font-size: 14;
+ color: var(--primary-text-color);
+ display: block;
+ }
+ .nav-sidebar .menu li a {
+ padding: 15px 20px;
+ font-size: 14;
+ outline: 0;
+ display: block;
+ color: var(--primary-text-color);
+ font-weight: 600;
+ }
+ .nav-sidebar .menu li.active ul.dropdown {
+ display: block;
+ }
+ .nav-sidebar .menu li ul.dropdown {
+ position: absolute;
+ display: none;
+ width: 100%;
+ box-shadow: 0 2px 5px 0 rgba(0, 0, 0, 0.26);
+ padding-top: 0;
+ position: relative;
+ }
+ .nav-sidebar .menu li ul.dropdown li {
+ display: block;
+ list-style-type: none;
+ }
+ .nav-sidebar .menu li ul.dropdown li a {
+ padding: 15px 20px;
+ font-size: 15px;
+ display: block;
+ font-weight: 400;
+ border-bottom: none;
+ padding: 10px 10px 10px 30px;
+ }
+ .nav-sidebar .menu li ul.dropdown li:last-child a {
+ border-bottom: none;
+ }
+ .nav-sidebar .menu a {
+ text-decoration: none;
+ }
+ .nav-sidebar .menu li.active:first-child a {
+ border-radius: 3px 0 0 3px;
+ border-radius: 0;
+ }
+ .nav-sidebar .menu li ul.dropdown li.active:first-child a {
+ border-radius: 0;
+ }
+ .arrow-down {
+ position: absolute;
+ top: 10px;
+ right: 10px;
}
}
`,
@@ -371,35 +605,130 @@
}
override render() {
+ return html` ${this.renderDesktop()} ${this.renderMobile()} `;
+ }
+
+ private renderDesktop() {
return html`
- <nav>
- <a href=${`//${window.location.host}${getBaseUrl()}/`} class="bigTitle">
- <gr-endpoint-decorator name="header-title">
- <div class="titleText"></div>
- </gr-endpoint-decorator>
- </a>
- <ul class="links">
- ${this.computeLinks(this.userLinks, this.adminLinks, this.topMenus).map(
- linkGroup => this.renderLinkGroup(linkGroup)
- )}
- </ul>
- <div class="rightItems">
- <gr-endpoint-decorator
- class="hideOnMobile"
- name="header-small-banner"
- ></gr-endpoint-decorator>
- <gr-smart-search id="search"></gr-smart-search>
- <gr-endpoint-decorator
- class="hideOnMobile"
- name="header-top-right"
- ></gr-endpoint-decorator>
- <gr-endpoint-decorator class="feedbackButton" name="header-feedback">
- ${this.renderFeedback()}
- </gr-endpoint-decorator>
- </div>
- ${this.renderAccount()}
- </div>
- </nav>
+ <nav class="hideOnMobile">
+ <a href=${`//${window.location.host}${getBaseUrl()}/`} class="bigTitle">
+ <gr-endpoint-decorator name="header-title">
+ <div class="titleText"></div>
+ </gr-endpoint-decorator>
+ </a>
+ <ul class="links">
+ ${this.computeLinks(
+ this.userLinks,
+ this.adminLinks,
+ this.topMenus
+ ).map(linkGroup => this.renderLinkGroup(linkGroup))}
+ </ul>
+ <div class="rightItems">
+ <gr-endpoint-decorator
+ class="hideOnMobile"
+ name="header-small-banner"
+ ></gr-endpoint-decorator>
+ <gr-smart-search id="search"></gr-smart-search>
+ <gr-endpoint-decorator
+ class="hideOnMobile"
+ name="header-top-right"
+ ></gr-endpoint-decorator>
+ <gr-endpoint-decorator class="feedbackButton" name="header-feedback">
+ ${this.renderFeedback()}
+ </gr-endpoint-decorator>
+ </div>
+ ${this.renderAccount()}
+ </div>
+ </nav>
+ `;
+ }
+
+ private renderMobile() {
+ const moreMenu: MainHeaderLink[] = [
+ {
+ name: this.registerText,
+ url: this.registerURL,
+ },
+ {
+ name: this.loginText,
+ url: this.loginUrl,
+ },
+ ];
+ if (!this.registerURL) {
+ moreMenu.shift();
+ }
+ if (this.feedbackURL) {
+ moreMenu.push({
+ name: 'Feedback',
+ url: this.feedbackURL,
+ external: true,
+ target: '_blank',
+ });
+ }
+
+ return html`
+ <nav class="hideOnDesktop">
+ <div class="nav-sidebar">
+ <ul class="menu">
+ ${this.computeLinks(
+ this.userLinks,
+ this.adminLinks,
+ this.topMenus
+ ).map(linkGroup => this.renderLinkGroupMobile(linkGroup))}
+ </ul>
+ </div>
+ <div class="nav-header">
+ <a
+ class="hamburger"
+ href=""
+ title="Hamburger"
+ aria-label="${!this.hamburgerClose ? 'Open' : 'Close'} hamburger"
+ role="button"
+ @click=${() => {
+ this.handleSidebar();
+ }}
+ >
+ ${!this.hamburgerClose
+ ? html`<gr-icon icon="menu" filled></gr-icon>`
+ : html`<gr-icon icon="menu_open" filled></gr-icon>`}
+ </a>
+ <a
+ href=${`//${window.location.host}${getBaseUrl()}/`}
+ class="mobileTitle bigTitle"
+ >
+ <gr-endpoint-decorator name="header-mobile-title">
+ <div class="mobileTitleText"></div>
+ </gr-endpoint-decorator>
+ </a>
+ <div class="mobileRightItems">
+ <a
+ class="searchButton"
+ title="Search"
+ @click=${(e: Event) => {
+ this.onMobileSearchTap(e);
+ }}
+ role="button"
+ aria-label=${this.mobileSearchHidden
+ ? 'Show Searchbar'
+ : 'Hide Searchbar'}
+ >
+ <gr-icon icon="search" filled></gr-icon>
+ </a>
+ <gr-dropdown
+ class="moreMenu"
+ link=""
+ .items=${moreMenu}
+ horizontal-align="center"
+ >
+ <span class="linksTitle">
+ <gr-icon icon="more_horiz" filled></gr-icon>
+ </span>
+ </gr-dropdown>
+ ${this.renderAccountDropdown(true)}
+ </div>
+ </div>
+ </nav>
+ <div class="modelBackground" @click=${() => this.handleSidebar()}></div>
`;
}
@@ -420,6 +749,41 @@
`;
}
+ private renderLinkGroupMobile(linkGroup: MainHeaderLinkGroup) {
+ return html`
+ <li class="has-collapsible" @click=${this.handleCollapsible}>
+ <a class="main" href="" data-title=${linkGroup.title}
+ >${linkGroup.title}<gr-icon
+ icon="arrow_drop_down"
+ class="arrow-down"
+ ></gr-icon
+ ></a>
+ <ul class="dropdown">
+ ${linkGroup.links.map(link => this.renderLinkMobile(link))}
+ </ul>
+ </li>
+ `;
+ }
+
+ private renderLinkMobile(link: DropdownLink) {
+ return html`
+ <li tabindex="-1">
+ <span ?hidden=${!!link.url} tabindex="-1">${link.name}</span>
+ <a
+ class="itemAction"
+ href=${this.computeLinkURL(link)}
+ ?download=${!!link.download}
+ rel=${ifDefined(this.computeLinkRel(link) ?? undefined)}
+ target=${ifDefined(link.target ?? undefined)}
+ ?hidden=${!link.url}
+ tabindex="-1"
+ @click=${() => this.handleSidebar()}
+ >${link.name}</a
+ >
+ </li>
+ `;
+ }
+
private renderFeedback() {
if (!this.feedbackURL) return;
@@ -483,11 +847,14 @@
`;
}
- private renderAccountDropdown() {
+ private renderAccountDropdown(showOnMobile?: boolean) {
if (!this.account) return;
return html`
- <gr-account-dropdown .account=${this.account}></gr-account-dropdown>
+ <gr-account-dropdown
+ .account=${this.account}
+ ?showMobile=${showOnMobile}
+ ></gr-account-dropdown>
`;
}
@@ -533,7 +900,6 @@
links.push({
title: 'Documentation',
links: docLinks,
- class: 'hideOnMobile',
});
}
links.push({
@@ -637,4 +1003,96 @@
e.stopPropagation();
fire(this, 'mobile-search', {});
}
+
+ /**
+ * Build a URL for the given host and path. The base URL will be only added,
+ * if it is not already included in the path.
+ *
+ * TODO: Move to util handler to remove duplication.
+ * @return The scheme-relative URL.
+ */
+ private computeURLHelper(host: string, path: string) {
+ const base = path.startsWith(getBaseUrl()) ? '' : getBaseUrl();
+ return '//' + host + base + path;
+ }
+
+ /**
+ * Build a scheme-relative URL for the current host. Will include the base
+ * URL if one is present. Note: the URL will be scheme-relative but absolute
+ * with regard to the host.
+ *
+ * TODO: Move to util handler to remove duplication.
+ * @param path The path for the URL.
+ * @return The scheme-relative URL.
+ */
+ private computeRelativeURL(path: string) {
+ const host = window.location.host;
+ return this.computeURLHelper(host, path);
+ }
+
+ /**
+ * Compute the URL for a link object.
+ *
+ * Private but used in tests.
+ *
+ * TODO: Move to util handler to remove duplication.
+ */
+ private computeLinkURL(link: DropdownLink) {
+ if (typeof link.url === 'undefined') {
+ return '';
+ }
+ if (link.target || !link.url.startsWith('/')) {
+ return link.url;
+ }
+ return this.computeRelativeURL(link.url);
+ }
+
+ /**
+ * Compute the value for the rel attribute of an anchor for the given link
+ * object. If the link has a target value, then the rel must be "noopener"
+ * for security reasons.
+ * Private but used in tests.
+ *
+ * TODO: Move to util handler to remove duplication.
+ */
+ private computeLinkRel(link: DropdownLink) {
+ // Note: noopener takes precedence over external.
+ if (link.target) {
+ return REL_NOOPENER;
+ }
+ if (link.external) {
+ return REL_EXTERNAL;
+ }
+ return null;
+ }
+
+ private handleCollapsible(e: MouseEvent) {
+ const target = e.target as HTMLSpanElement;
+ if (target.hasAttribute('data-title')) {
+ if (target.parentElement?.classList.contains('active')) {
+ target.parentElement.classList.remove('active');
+ } else {
+ if (this.hasCollapsibleActive) {
+ this.hasCollapsibleActive.classList.remove('active');
+ }
+ target.parentElement?.classList.toggle('active');
+ }
+ }
+ }
+
+ private handleSidebar() {
+ this.navSidebar?.classList.toggle('visible');
+ if (!this.modelBackground?.classList.contains('cover')) {
+ if (document.getElementsByTagName('html')) {
+ document.getElementsByTagName('html')[0].style.overflow = 'hidden';
+ }
+ } else {
+ if (document.getElementsByTagName('html')) {
+ document.getElementsByTagName('html')[0].style.overflow = '';
+ }
+ }
+ this.modelBackground?.classList.toggle('cover');
+ this.hasCollapsibleActive?.classList.remove('active');
+ this.hamburgerClose = !this.hamburgerClose;
+ }
}
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 40430fb..b4a0600 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
@@ -39,7 +39,7 @@
assert.shadowDom.equal(
element,
/* HTML */ `
- <nav>
+ <nav class="hideOnMobile">
<a class="bigTitle" href="//localhost:9876/">
<gr-endpoint-decorator name="header-title">
<div class="titleText"></div>
@@ -51,7 +51,7 @@
<span class="linksTitle" id="Changes"> Changes </span>
</gr-dropdown>
</li>
- <li class="hideOnMobile">
+ <li>
<gr-dropdown down-arrow="" horizontal-align="left" link="">
<span class="linksTitle" id="Documentation">Documentation</span>
</gr-dropdown>
@@ -101,6 +101,169 @@
</a>
</div>
</nav>
+ <nav class="hideOnDesktop">
+ <div class="nav-sidebar">
+ <ul class="menu">
+ <li class="has-collapsible">
+ <a class="main" data-title="Changes" href="">
+ Changes
+ <gr-icon class="arrow-down" icon="arrow_drop_down"> </gr-icon>
+ </a>
+ <ul class="dropdown">
+ <li tabindex="-1">
+ <span hidden="" tabindex="-1"> Open </span>
+ <a
+ class="itemAction"
+ href="//localhost:9876/q/status:open+-is:wip"
+ tabindex="-1"
+ >
+ Open
+ </a>
+ </li>
+ <li tabindex="-1">
+ <span hidden="" tabindex="-1"> Merged </span>
+ <a
+ class="itemAction"
+ href="//localhost:9876/q/status:merged"
+ tabindex="-1"
+ >
+ Merged
+ </a>
+ </li>
+ <li tabindex="-1">
+ <span hidden="" tabindex="-1"> Abandoned </span>
+ <a
+ class="itemAction"
+ href="//localhost:9876/q/status:abandoned"
+ tabindex="-1"
+ >
+ Abandoned
+ </a>
+ </li>
+ </ul>
+ </li>
+ <li class="has-collapsible">
+ <a class="main" data-title="Documentation" href="">
+ Documentation
+ <gr-icon class="arrow-down" icon="arrow_drop_down"> </gr-icon>
+ </a>
+ <ul class="dropdown">
+ <li tabindex="-1">
+ <span hidden="" tabindex="-1"> Table of Contents </span>
+ <a
+ class="itemAction"
+ href="https://gerrit-review.googlesource.com/Documentation/index.html"
+ rel="noopener"
+ tabindex="-1"
+ target="_blank"
+ >
+ Table of Contents
+ </a>
+ </li>
+ <li tabindex="-1">
+ <span hidden="" tabindex="-1"> Searching </span>
+ <a
+ class="itemAction"
+ href="https://gerrit-review.googlesource.com/Documentation/user-search.html"
+ rel="noopener"
+ tabindex="-1"
+ target="_blank"
+ >
+ Searching
+ </a>
+ </li>
+ <li tabindex="-1">
+ <span hidden="" tabindex="-1"> Uploading </span>
+ <a
+ class="itemAction"
+ href="https://gerrit-review.googlesource.com/Documentation/user-upload.html"
+ rel="noopener"
+ tabindex="-1"
+ target="_blank"
+ >
+ Uploading
+ </a>
+ </li>
+ <li tabindex="-1">
+ <span hidden="" tabindex="-1"> Access Control </span>
+ <a
+ class="itemAction"
+ href="https://gerrit-review.googlesource.com/Documentation/access-control.html"
+ rel="noopener"
+ tabindex="-1"
+ target="_blank"
+ >
+ Access Control
+ </a>
+ </li>
+ <li tabindex="-1">
+ <span hidden="" tabindex="-1"> REST API </span>
+ <a
+ class="itemAction"
+ href="https://gerrit-review.googlesource.com/Documentation/rest-api.html"
+ rel="noopener"
+ tabindex="-1"
+ target="_blank"
+ >
+ REST API
+ </a>
+ </li>
+ <li tabindex="-1">
+ <span hidden="" tabindex="-1"> Project Owner Guide </span>
+ <a
+ class="itemAction"
+ href="https://gerrit-review.googlesource.com/Documentation/intro-project-owner.html"
+ rel="noopener"
+ tabindex="-1"
+ target="_blank"
+ >
+ Project Owner Guide
+ </a>
+ </li>
+ </ul>
+ </li>
+ <li class="has-collapsible">
+ <a class="main" data-title="Browse" href="">
+ Browse
+ <gr-icon class="arrow-down" icon="arrow_drop_down"> </gr-icon>
+ </a>
+ <ul class="dropdown"></ul>
+ </li>
+ </ul>
+ </div>
+ <div class="nav-header">
+ <a
+ aria-label="Open hamburger"
+ class="hamburger"
+ href=""
+ role="button"
+ title="Hamburger"
+ >
+ <gr-icon filled="" icon="menu"> </gr-icon>
+ </a>
+ <a class="bigTitle mobileTitle" href="//localhost:9876/">
+ <gr-endpoint-decorator name="header-mobile-title">
+ <div class="mobileTitleText"></div>
+ </gr-endpoint-decorator>
+ </a>
+ <div class="mobileRightItems">
+ <a
+ aria-label="Hide Searchbar"
+ class="searchButton"
+ role="button"
+ title="Search"
+ >
+ <gr-icon filled="" icon="search"> </gr-icon>
+ </a>
+ <gr-dropdown class="moreMenu" horizontal-align="center" link="">
+ <span class="linksTitle">
+ <gr-icon filled="" icon="more_horiz"> </gr-icon>
+ </span>
+ </gr-dropdown>
+ </div>
+ </div>
+ </nav>
+ <div class="modelBackground"></div>
`
);
});
diff --git a/polygerrit-ui/app/elements/settings/gr-email-editor/gr-email-editor.ts b/polygerrit-ui/app/elements/settings/gr-email-editor/gr-email-editor.ts
index 9c99ae0..2220d2f 100644
--- a/polygerrit-ui/app/elements/settings/gr-email-editor/gr-email-editor.ts
+++ b/polygerrit-ui/app/elements/settings/gr-email-editor/gr-email-editor.ts
@@ -3,20 +3,23 @@
* Copyright 2016 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
-import '@polymer/iron-input/iron-input';
import '../../shared/gr-button/gr-button';
import {EmailInfo} from '../../../types/common';
import {getAppContext} from '../../../services/app-context';
import {LitElement, css, html} from 'lit';
-import {customElement, property, state} from 'lit/decorators.js';
+import {customElement, state} from 'lit/decorators.js';
import {sharedStyles} from '../../../styles/shared-styles';
import {grFormStyles} from '../../../styles/gr-form-styles';
import {ValueChangedEvent} from '../../../types/events';
import {fire} from '../../../utils/event-util';
+import {deepClone} from '../../../utils/deep-util';
+import {userModelToken} from '../../../models/user/user-model';
+import {resolve} from '../../../models/dependency';
+import {subscribe} from '../../lit/subscription-controller';
@customElement('gr-email-editor')
export class GrEmailEditor extends LitElement {
- @property({type: Boolean}) hasUnsavedChanges = false;
+ @state() private originalEmails: EmailInfo[] = [];
/* private but used in test */
@state() emails: EmailInfo[] = [];
@@ -29,6 +32,21 @@
readonly restApiService = getAppContext().restApiService;
+ private readonly getUserModel = resolve(this, userModelToken);
+
+ constructor() {
+ super();
+ subscribe(
+ this,
+ () => this.getUserModel().emails$,
+ x => {
+ if (!x) return;
+ this.originalEmails = deepClone<EmailInfo[]>(x);
+ this.emails = deepClone<EmailInfo[]>(x);
+ }
+ );
+ }
+
static override get styles() {
return [
sharedStyles,
@@ -82,24 +100,20 @@
return html`<tr>
<td class="emailColumn">${email.email}</td>
<td class="preferredControl" @click=${this.handlePreferredControlClick}>
- <iron-input
+ <!-- We have to use \`.checked\` rather then \`?checked\` as there
+ appears to be an issue when deleting, checked doesn't work correctly. -->
+ <input
class="preferredRadio"
+ type="radio"
+ name="preferred"
+ .value=${email.email}
+ .checked=${email.preferred}
@change=${this.handlePreferredChange}
- .bindValue=${email.email}
- >
- <input
- class="preferredRadio"
- type="radio"
- @change=${this.handlePreferredChange}
- name="preferred"
- ?checked=${email.preferred}
- />
- </iron-input>
+ />
</td>
<td>
<gr-button
- data-index=${index}
- @click=${this.handleDeleteButton}
+ @click=${() => this.handleDeleteButton(index)}
?disabled=${this.checkPreferred(email.preferred)}
class="remove-button"
>Delete</gr-button
@@ -108,12 +122,6 @@
</tr>`;
}
- loadData() {
- return this.restApiService.getAccountEmails().then(emails => {
- this.emails = emails ?? [];
- });
- }
-
save() {
const promises: Promise<unknown>[] = [];
@@ -127,24 +135,27 @@
);
}
- return Promise.all(promises).then(() => {
+ return Promise.all(promises).then(async () => {
this.emailsToRemove = [];
this.newPreferred = '';
- this.setHasUnsavedChanges(false);
+ await this.getUserModel().loadEmails(true);
+ this.setHasUnsavedChanges();
});
}
- private handleDeleteButton(e: Event) {
- const target = e.target;
- if (!(target instanceof Element)) return;
- const indexStr = target.getAttribute('data-index');
- if (indexStr === null) return;
- const index = Number(indexStr);
+ private handleDeleteButton(index: number) {
const email = this.emails[index];
- this.emailsToRemove = [...this.emailsToRemove, email];
+ // Don't add project to emailsToRemove if it wasn't in
+ // emails.
+ // We have to use JSON.stringify as we cloned the array
+ // so the reference is not the same.
+ const emails = this.emails.some(
+ x => JSON.stringify(email) === JSON.stringify(x)
+ );
+ if (emails) this.emailsToRemove.push(email);
this.emails.splice(index, 1);
this.requestUpdate();
- this.setHasUnsavedChanges(true);
+ this.setHasUnsavedChanges();
}
private handlePreferredControlClick(e: Event) {
@@ -165,9 +176,10 @@
this.emails[i].preferred = true;
this.requestUpdate();
this.newPreferred = preferred;
- this.setHasUnsavedChanges(true);
+ this.setHasUnsavedChanges();
} else if (this.emails[i].preferred) {
- this.emails[i].preferred = false;
+ delete this.emails[i].preferred;
+ this.setHasUnsavedChanges();
this.requestUpdate();
}
}
@@ -177,9 +189,11 @@
return preferred ?? false;
}
- private setHasUnsavedChanges(value: boolean) {
- this.hasUnsavedChanges = value;
- fire(this, 'has-unsaved-changes-changed', {value});
+ private setHasUnsavedChanges() {
+ const hasUnsavedChanges =
+ JSON.stringify(this.originalEmails) !== JSON.stringify(this.emails) ||
+ this.emailsToRemove.length > 0;
+ fire(this, 'has-unsaved-changes-changed', {value: hasUnsavedChanges});
}
}
diff --git a/polygerrit-ui/app/elements/settings/gr-email-editor/gr-email-editor_test.ts b/polygerrit-ui/app/elements/settings/gr-email-editor/gr-email-editor_test.ts
index 25c9b97..84ed8dc 100644
--- a/polygerrit-ui/app/elements/settings/gr-email-editor/gr-email-editor_test.ts
+++ b/polygerrit-ui/app/elements/settings/gr-email-editor/gr-email-editor_test.ts
@@ -11,6 +11,7 @@
suite('gr-email-editor tests', () => {
let element: GrEmailEditor;
+ let accountEmailStub: sinon.SinonStub;
setup(async () => {
const emails = [
@@ -19,13 +20,14 @@
{email: 'email@three.com'},
];
- stubRestApi('getAccountEmails').returns(Promise.resolve(emails));
+ accountEmailStub = stubRestApi('getAccountEmails').returns(
+ Promise.resolve(emails)
+ );
element = await fixture<GrEmailEditor>(
html`<gr-email-editor></gr-email-editor>`
);
- await element.loadData();
await element.updateComplete;
});
@@ -45,20 +47,17 @@
<tr>
<td class="emailColumn">email@one.com</td>
<td class="preferredControl">
- <iron-input class="preferredRadio">
- <input
- class="preferredRadio"
- name="preferred"
- type="radio"
- value="email@one.com"
- />
- </iron-input>
+ <input
+ class="preferredRadio"
+ name="preferred"
+ type="radio"
+ value="email@one.com"
+ />
</td>
<td>
<gr-button
aria-disabled="false"
class="remove-button"
- data-index="0"
role="button"
tabindex="0"
>
@@ -69,21 +68,17 @@
<tr>
<td class="emailColumn">email@two.com</td>
<td class="preferredControl">
- <iron-input class="preferredRadio">
- <input
- checked=""
- class="preferredRadio"
- name="preferred"
- type="radio"
- value="email@two.com"
- />
- </iron-input>
+ <input
+ class="preferredRadio"
+ name="preferred"
+ type="radio"
+ value="email@two.com"
+ />
</td>
<td>
<gr-button
aria-disabled="true"
class="remove-button"
- data-index="1"
disabled=""
role="button"
tabindex="-1"
@@ -95,20 +90,17 @@
<tr>
<td class="emailColumn">email@three.com</td>
<td class="preferredControl">
- <iron-input class="preferredRadio">
- <input
- class="preferredRadio"
- name="preferred"
- type="radio"
- value="email@three.com"
- />
- </iron-input>
+ <input
+ class="preferredRadio"
+ name="preferred"
+ type="radio"
+ value="email@three.com"
+ />
</td>
<td>
<gr-button
aria-disabled="false"
class="remove-button"
- data-index="2"
role="button"
tabindex="0"
>
@@ -123,6 +115,12 @@
});
test('renders', () => {
+ const hasUnsavedChangesSpy = sinon.spy();
+ element.addEventListener(
+ 'has-unsaved-changes-changed',
+ hasUnsavedChangesSpy
+ );
+
const rows = element
.shadowRoot!.querySelector('table')!
.querySelectorAll('tbody tr');
@@ -144,15 +142,21 @@
);
assert.isNotOk(rows[2].querySelector('gr-button')!.disabled);
- assert.isFalse(element.hasUnsavedChanges);
+ assert.isFalse(hasUnsavedChangesSpy.called);
});
test('edit preferred', () => {
+ const hasUnsavedChangesSpy = sinon.spy();
+ element.addEventListener(
+ 'has-unsaved-changes-changed',
+ hasUnsavedChangesSpy
+ );
+
const radios = element
.shadowRoot!.querySelector('table')!
.querySelectorAll<HTMLInputElement>('input[type=radio]');
- assert.isFalse(element.hasUnsavedChanges);
+ assert.isFalse(hasUnsavedChangesSpy.called);
assert.isNotOk(element.newPreferred);
assert.equal(element.emailsToRemove.length, 0);
assert.equal(element.emails.length, 3);
@@ -162,7 +166,7 @@
radios[0].click();
- assert.isTrue(element.hasUnsavedChanges);
+ assert.isTrue(hasUnsavedChangesSpy.called);
assert.isOk(element.newPreferred);
assert.equal(element.emailsToRemove.length, 0);
assert.equal(element.emails.length, 3);
@@ -172,18 +176,24 @@
});
test('delete email', () => {
+ const hasUnsavedChangesSpy = sinon.spy();
+ element.addEventListener(
+ 'has-unsaved-changes-changed',
+ hasUnsavedChangesSpy
+ );
+
const buttons = element
.shadowRoot!.querySelector('table')!
.querySelectorAll('gr-button');
- assert.isFalse(element.hasUnsavedChanges);
+ assert.isFalse(hasUnsavedChangesSpy.called);
assert.isNotOk(element.newPreferred);
assert.equal(element.emailsToRemove.length, 0);
assert.equal(element.emails.length, 3);
buttons[2].click();
- assert.isTrue(element.hasUnsavedChanges);
+ assert.isTrue(hasUnsavedChangesSpy.called);
assert.isNotOk(element.newPreferred);
assert.equal(element.emailsToRemove.length, 1);
assert.equal(element.emails.length, 2);
@@ -192,6 +202,12 @@
});
test('save changes', async () => {
+ const hasUnsavedChangesSpy = sinon.spy();
+ element.addEventListener(
+ 'has-unsaved-changes-changed',
+ hasUnsavedChangesSpy
+ );
+
const deleteEmailSpy = spyRestApi('deleteAccountEmail');
const setPreferredSpy = spyRestApi('setPreferredAccountEmail');
@@ -199,7 +215,7 @@
.shadowRoot!.querySelector('table')!
.querySelectorAll('tbody tr');
- assert.isFalse(element.hasUnsavedChanges);
+ assert.isFalse(hasUnsavedChangesSpy.called);
assert.isNotOk(element.newPreferred);
assert.equal(element.emailsToRemove.length, 0);
assert.equal(element.emails.length, 3);
@@ -208,16 +224,24 @@
rows[0].querySelector('gr-button')!.click();
rows[2].querySelector<HTMLInputElement>('input[type=radio]')!.click();
- assert.isTrue(element.hasUnsavedChanges);
+ assert.isTrue(hasUnsavedChangesSpy.called);
+ assert.isTrue(hasUnsavedChangesSpy.lastCall.args[0].detail.value);
assert.equal(element.newPreferred, 'email@three.com');
assert.equal(element.emailsToRemove.length, 1);
assert.equal(element.emailsToRemove[0].email, 'email@one.com');
assert.equal(element.emails.length, 2);
+ accountEmailStub.restore();
+
+ accountEmailStub = stubRestApi('getAccountEmails').returns(
+ Promise.resolve(element.emails)
+ );
+
await element.save();
assert.equal(deleteEmailSpy.callCount, 1);
assert.equal(deleteEmailSpy.getCall(0).args[0], 'email@one.com');
assert.isTrue(setPreferredSpy.called);
assert.equal(setPreferredSpy.getCall(0).args[0], 'email@three.com');
+ assert.isFalse(hasUnsavedChangesSpy.lastCall.args[0].detail.value);
});
});
diff --git a/polygerrit-ui/app/elements/settings/gr-preferences/gr-preferences.ts b/polygerrit-ui/app/elements/settings/gr-preferences/gr-preferences.ts
index bb0f1e9..fdb4f22 100644
--- a/polygerrit-ui/app/elements/settings/gr-preferences/gr-preferences.ts
+++ b/polygerrit-ui/app/elements/settings/gr-preferences/gr-preferences.ts
@@ -57,6 +57,9 @@
@query('#allowSuggestCodeWhileCommenting')
allowSuggestCodeWhileCommenting?: HTMLInputElement;
+ @query('#allowAiCommentAutocompletion')
+ allowAiCommentAutocompletion?: HTMLInputElement;
+
@query('#defaultBaseForMergesSelect')
defaultBaseForMergesSelect!: HTMLInputElement;
@@ -284,6 +287,7 @@
</section>
${this.renderBrowserNotifications()}
${this.renderGenerateSuggestionWhenCommenting()}
+ ${this.renderAiCommentAutocompletion()}
${this.renderDefaultBaseForMerges()}
<section>
<label class="title" for="relativeDateInChangeTable"
@@ -519,6 +523,37 @@
`;
}
+ // When the experiment is over, move this back to render(),
+ // removing this function.
+ private renderAiCommentAutocompletion() {
+ if (
+ !this.flagsService.isEnabled(KnownExperimentId.COMMENT_AUTOCOMPLETION) ||
+ !this.suggestionsProvider
+ )
+ return nothing;
+ return html`
+ <section id="allowAiCommentAutocompletionSection">
+ <div class="title">
+ <label for="allowAiCommentAutocompletion"
+ >AI suggested text completions while commenting</label
+ >
+ </div>
+ <span class="value">
+ <input
+ id="allowAiCommentAutocompletion"
+ type="checkbox"
+ ?checked=${this.prefs?.allow_autocompleting_comments}
+ @change=${() => {
+ this.prefs!.allow_autocompleting_comments =
+ this.allowAiCommentAutocompletion!.checked;
+ this.requestUpdate();
+ }}
+ />
+ </span>
+ </section>
+ `;
+ }
+
// When this is fixed and can be re-enabled, move this back to render()
// and remove function.
private renderDefaultBaseForMerges() {
diff --git a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-item.ts b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-item.ts
deleted file mode 100644
index c7118f7..0000000
--- a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-item.ts
+++ /dev/null
@@ -1,40 +0,0 @@
-/**
- * @license
- * Copyright 2017 Google LLC
- * SPDX-License-Identifier: Apache-2.0
- */
-import {LitElement, css, html} from 'lit';
-import {customElement, property} from 'lit/decorators.js';
-import {grFormStyles} from '../../../styles/gr-form-styles';
-
-declare global {
- interface HTMLElementTagNameMap {
- 'gr-settings-item': GrSettingsItem;
- }
-}
-
-@customElement('gr-settings-item')
-export class GrSettingsItem extends LitElement {
- @property({type: String})
- anchor?: string;
-
- @property({type: String})
- override title = '';
-
- static override get styles() {
- return [
- grFormStyles,
- css`
- :host {
- display: block;
- margin-bottom: var(--spacing-xxl);
- }
- `,
- ];
- }
-
- override render() {
- const anchor = this.anchor ?? '';
- return html`<h2 id=${anchor} class="heading-2">${this.title}</h2>`;
- }
-}
diff --git a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-menu-item.ts b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-menu-item.ts
deleted file mode 100644
index 6c83bea..0000000
--- a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-menu-item.ts
+++ /dev/null
@@ -1,35 +0,0 @@
-/**
- * @license
- * Copyright 2017 Google LLC
- * SPDX-License-Identifier: Apache-2.0
- */
-import {pageNavStyles} from '../../../styles/gr-page-nav-styles';
-import {sharedStyles} from '../../../styles/shared-styles';
-import {LitElement, html} from 'lit';
-import {customElement, property} from 'lit/decorators.js';
-
-declare global {
- interface HTMLElementTagNameMap {
- 'gr-settings-menu-item': GrSettingsMenuItem;
- }
-}
-
-@customElement('gr-settings-menu-item')
-export class GrSettingsMenuItem extends LitElement {
- @property({type: String})
- href?: string;
-
- @property({type: String})
- override title = '';
-
- static override get styles() {
- return [sharedStyles, pageNavStyles];
- }
-
- override render() {
- const href = this.href ?? '';
- return html` <div class="navStyles">
- <li><a href=${href}>${this.title}</a></li>
- </div>`;
- }
-}
diff --git a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.ts b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.ts
index fd7d67b..7626e14 100644
--- a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.ts
+++ b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.ts
@@ -38,7 +38,7 @@
import {GrSshEditor} from '../gr-ssh-editor/gr-ssh-editor';
import {GrGpgEditor} from '../gr-gpg-editor/gr-gpg-editor';
import {GrEmailEditor} from '../gr-email-editor/gr-email-editor';
-import {fireAlert, fireTitleChange} from '../../../utils/event-util';
+import {fire, fireAlert, fireTitleChange} from '../../../utils/event-util';
import {getAppContext} from '../../../services/app-context';
import {BindValueChangeEvent, ValueChangedEvent} from '../../../types/events';
import {LitElement, css, html} from 'lit';
@@ -190,7 +190,7 @@
const message = await this.restApiService.confirmEmail(this.emailToken);
if (message) fireAlert(this, message);
this.getViewModel().clearToken();
- await this.emailEditor.loadData();
+ await this.getUserModel().loadEmails(true);
}
override connectedCallback() {
@@ -230,8 +230,6 @@
})
);
- promises.push(this.emailEditor.loadData());
-
this._testOnly_loadingPromise = Promise.all(promises).then(() => {
this.loading = false;
@@ -252,6 +250,7 @@
css`
:host {
color: var(--primary-text-color);
+ overflow: auto;
}
h2 {
font-family: var(--header-font-family);
@@ -340,6 +339,9 @@
@unsaved-changes-changed=${(e: ValueChangedEvent<boolean>) => {
this.accountInfoChanged = e.detail.value;
}}
+ @account-detail-update=${() => {
+ fire(this, 'account-detail-update', {});
+ }}
></gr-account-info>
<gr-button
@click=${() => {
@@ -437,7 +439,6 @@
</h2>
<fieldset id="watchedProjects">
<gr-watched-projects-editor
- ?hasUnsavedChanges=${this.watchedProjectsChanged}
@has-unsaved-changes-changed=${(
e: ValueChangedEvent<boolean>
) => {
@@ -463,7 +464,6 @@
<fieldset id="email">
<gr-email-editor
id="emailEditor"
- ?hasUnsavedChanges=${this.emailsChanged}
@has-unsaved-changes-changed=${(
e: ValueChangedEvent<boolean>
) => {
@@ -471,8 +471,8 @@
}}
></gr-email-editor>
<gr-button
- @click=${() => {
- this.emailEditor.save();
+ @click=${async () => {
+ await this.emailEditor.save();
}}
?disabled=${!this.emailsChanged}
>Save changes</gr-button
@@ -605,7 +605,7 @@
};
reloadAccountDetail() {
- Promise.all([this.accountInfo.loadData(), this.emailEditor.loadData()]);
+ Promise.all([this.accountInfo.loadData()]);
}
// private but used in test
@@ -643,7 +643,7 @@
if (!this.isNewEmailValid(this.newEmail)) return;
this.addingEmail = true;
- this.restApiService.addAccountEmail(this.newEmail).then(response => {
+ this.restApiService.addAccountEmail(this.newEmail).then(async response => {
this.addingEmail = false;
// If it was unsuccessful.
@@ -653,6 +653,8 @@
this.lastSentVerificationEmail = this.newEmail;
this.newEmail = '';
+
+ await this.getUserModel().loadEmails(true);
});
}
diff --git a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view_test.ts b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view_test.ts
index b6690b6..74bcca9 100644
--- a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view_test.ts
+++ b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view_test.ts
@@ -331,12 +331,6 @@
assert.isNotOk(element.lastSentVerificationEmail);
});
- test('emails are loaded without emailToken', () => {
- const emailEditorLoadDataStub = sinon.stub(element.emailEditor, 'loadData');
- element.firstUpdated();
- assert.isTrue(emailEditorLoadDataStub.calledOnce);
- });
-
test('handleSaveChangeTable', () => {
let newColumns = ['Owner', 'Project', 'Branch'];
element.localChangeTableColumns = newColumns.slice(0);
@@ -387,10 +381,8 @@
value: string | PromiseLike<string | null> | null
) => void;
let confirmEmailStub: sinon.SinonStub;
- let emailEditorLoadDataStub: sinon.SinonStub;
setup(() => {
- emailEditorLoadDataStub = sinon.stub(element.emailEditor, 'loadData');
confirmEmailStub = stubRestApi('confirmEmail').returns(
new Promise(resolve => {
resolveConfirm = resolve;
@@ -406,16 +398,6 @@
assert.isTrue(confirmEmailStub.calledWith('foo'));
});
- test('emails are not loaded initially', () => {
- assert.isFalse(emailEditorLoadDataStub.called);
- });
-
- test('user emails are loaded after email confirmed', async () => {
- resolveConfirm('bar');
- await element._testOnly_loadingPromise;
- assert.isTrue(emailEditorLoadDataStub.calledOnce);
- });
-
test('show-alert is fired when email is confirmed', async () => {
const dispatchEventSpy = sinon.spy(element, 'dispatchEvent');
resolveConfirm('bar');
diff --git a/polygerrit-ui/app/elements/settings/gr-watched-projects-editor/gr-watched-projects-editor.ts b/polygerrit-ui/app/elements/settings/gr-watched-projects-editor/gr-watched-projects-editor.ts
index f0f0f2f..583798c 100644
--- a/polygerrit-ui/app/elements/settings/gr-watched-projects-editor/gr-watched-projects-editor.ts
+++ b/polygerrit-ui/app/elements/settings/gr-watched-projects-editor/gr-watched-projects-editor.ts
@@ -6,7 +6,7 @@
import '@polymer/iron-input/iron-input';
import '../../shared/gr-autocomplete/gr-autocomplete';
import '../../shared/gr-button/gr-button';
-import {customElement, property, query} from 'lit/decorators.js';
+import {customElement, query, state} from 'lit/decorators.js';
import {
AutocompleteQuery,
GrAutocomplete,
@@ -22,6 +22,7 @@
import {fire} from '../../../utils/event-util';
import {PropertiesOfType} from '../../../utils/type-util';
import {throwingErrorCallback} from '../../shared/gr-rest-api-interface/gr-rest-apis/gr-rest-api-helper';
+import {notDeepEqual} from '../../../utils/deep-util';
type NotificationKey = PropertiesOfType<Required<ProjectWatchInfo>, boolean>;
@@ -43,13 +44,13 @@
@query('#newProject')
newProject?: GrAutocomplete;
- @property({type: Boolean})
- hasUnsavedChanges = false;
+ @state()
+ originalProjects?: ProjectWatchInfo[];
- @property({type: Array})
+ @state()
projects?: ProjectWatchInfo[];
- @property({type: Array})
+ @state()
projectsToRemove: ProjectWatchInfo[] = [];
private readonly query: AutocompleteQuery = input =>
@@ -163,7 +164,8 @@
loadData() {
return this.restApiService.getWatchedProjects().then(projs => {
- this.projects = projs;
+ this.originalProjects = projs;
+ this.projects = projs ? [...projs] : [];
});
}
@@ -186,9 +188,10 @@
}
})
.then(projects => {
- this.projects = projects;
+ this.originalProjects = projects;
+ this.projects = projects ? [...projects] : [];
this.projectsToRemove = [];
- this.setHasUnsavedChanges(false);
+ this.setHasUnsavedChanges();
});
}
@@ -206,13 +209,16 @@
}
private handleRemoveProject(project: ProjectWatchInfo) {
- if (!this.projects) return;
+ if (!this.projects || !this.originalProjects) return;
const index = this.projects.indexOf(project);
if (index < 0) return;
this.projects.splice(index, 1);
- this.projectsToRemove.push(project);
+ // Don't add project to projectsToRemove if it wasn't in
+ // originalProjects.
+ if (this.originalProjects.includes(project))
+ this.projectsToRemove.push(project);
this.requestUpdate();
- this.setHasUnsavedChanges(true);
+ this.setHasUnsavedChanges();
}
// private but used in tests.
@@ -288,7 +294,7 @@
this.newProject.clear();
this.newFilter.value = '';
- this.setHasUnsavedChanges(true);
+ this.setHasUnsavedChanges();
}
private handleCheckboxChange(
@@ -300,7 +306,7 @@
const checked = el.checked;
project[key] = !!checked;
this.requestUpdate();
- this.setHasUnsavedChanges(true);
+ this.setHasUnsavedChanges();
}
private handleNotifCellClick(e: Event) {
@@ -311,9 +317,11 @@
}
}
- private setHasUnsavedChanges(value: boolean) {
- this.hasUnsavedChanges = value;
- fire(this, 'has-unsaved-changes-changed', {value});
+ private setHasUnsavedChanges() {
+ const hasUnsavedChanges =
+ notDeepEqual(this.originalProjects, this.projects) ||
+ this.projectsToRemove.length > 0;
+ fire(this, 'has-unsaved-changes-changed', {value: hasUnsavedChanges});
}
isFilterDefined(filter: string | null | undefined) {
diff --git a/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.ts b/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.ts
index d77bec9..704cecf 100644
--- a/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.ts
+++ b/polygerrit-ui/app/elements/shared/gr-account-label/gr-account-label.ts
@@ -10,9 +10,13 @@
import '../../plugins/gr-endpoint-param/gr-endpoint-param';
import {getAppContext} from '../../../services/app-context';
import {getDisplayName} from '../../../utils/display-name-util';
-import {isSelf, isServiceUser} from '../../../utils/account-util';
+import {
+ isDetailedAccount,
+ isSelf,
+ isServiceUser,
+} from '../../../utils/account-util';
import {ChangeInfo, AccountInfo, ServerInfo} from '../../../types/common';
-import {assertIsDefined, hasOwnProperty} from '../../../utils/common-util';
+import {hasOwnProperty} from '../../../utils/common-util';
import {fire} from '../../../utils/event-util';
import {isInvolved} from '../../../utils/change-util';
import {LitElement, css, html, TemplateResult} from 'lit';
@@ -21,7 +25,7 @@
import {getRemovedByIconClickReason} from '../../../utils/attention-set-util';
import {ifDefined} from 'lit/directives/if-defined.js';
import {createSearchUrl} from '../../../models/views/search';
-import {accountsModelToken} from '../../../models/accounts-model/accounts-model';
+import {accountsModelToken} from '../../../models/accounts/accounts-model';
import {resolve} from '../../../models/dependency';
import {configModelToken} from '../../../models/config/config-model';
import {userModelToken} from '../../../models/user/user-model';
@@ -198,15 +202,29 @@
];
}
- override async updated() {
- assertIsDefined(this.account, 'account');
+ override updated() {
+ this.computeDetailedAccount();
+ }
+
+ private async computeDetailedAccount() {
+ if (!this.account) return;
+ // If this.account is already a detailed object, then there is no need to fill it.
+ if (isDetailedAccount(this.account)) return;
const account = await this.getAccountsModel().fillDetails(this.account);
- // AccountInfo returned by fillDetails has the email property set
- // to the primary email of the account. This poses a problem in
- // cases where a secondary email is used as the committer or author
- // email. Therefore, only fill in the missing details to avoid
- // displaying incorrect author or committer email.
- if (account) this.account = Object.assign(account, this.account);
+ if (
+ account &&
+ // If we were not able to get a detailed object, then there is no point in updating the
+ // account.
+ isDetailedAccount(account) &&
+ account !== this.account &&
+ account._account_id === this.account._account_id
+ ) {
+ // AccountInfo returned by fillDetails has the email property set
+ // to the primary email of the account. This poses a problem in
+ // cases where a secondary email is used as the committer or author
+ // email. Therefore, only fill in the *missing* properties.
+ this.account = {...account, ...this.account};
+ }
}
override render() {
diff --git a/polygerrit-ui/app/elements/shared/gr-avatar/gr-avatar-stack.ts b/polygerrit-ui/app/elements/shared/gr-avatar/gr-avatar-stack.ts
index 863ee90..b445f75 100644
--- a/polygerrit-ui/app/elements/shared/gr-avatar/gr-avatar-stack.ts
+++ b/polygerrit-ui/app/elements/shared/gr-avatar/gr-avatar-stack.ts
@@ -17,7 +17,7 @@
import {configModelToken} from '../../../models/config/config-model';
import {subscribe} from '../../lit/subscription-controller';
import {getDisplayName} from '../../../utils/display-name-util';
-import {accountsModelToken} from '../../../models/accounts-model/accounts-model';
+import {accountsModelToken} from '../../../models/accounts/accounts-model';
import {isDefined} from '../../../types/types';
import {when} from 'lit/directives/when.js';
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 c27ca5b..68ab132 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -79,8 +79,12 @@
commentModelToken,
} from '../gr-comment-model/gr-comment-model';
import {formStyles} from '../../../styles/form-styles';
-import {Interaction} from '../../../constants/reporting';
-import {Suggestion, SuggestionsProvider} from '../../../api/suggestions';
+import {Interaction, Timing} from '../../../constants/reporting';
+import {
+ AutocompleteCommentResponse,
+ Suggestion,
+ SuggestionsProvider,
+} from '../../../api/suggestions';
import {when} from 'lit/directives/when.js';
import {getDocUrl} from '../../../utils/url-util';
import {configModelToken} from '../../../models/config/config-model';
@@ -89,6 +93,12 @@
import {deepEqual} from '../../../utils/deep-util';
import {GrSuggestionDiffPreview} from '../gr-suggestion-diff-preview/gr-suggestion-diff-preview';
import {waitUntil} from '../../../utils/async-util';
+import {
+ AutocompleteCache,
+ AutocompletionContext,
+} from '../../../utils/autocomplete-cache';
+import {HintAppliedEventDetail, HintShownEventDetail} from '../../../api/embed';
+import {levenshteinDistance} from '../../../utils/string-util';
// visible for testing
export const AUTO_SAVE_DEBOUNCE_DELAY_MS = 2000;
@@ -224,7 +234,14 @@
* An hint for autocompleting the comment message from plugin suggestion
* providers.
*/
- @state() autocompleteHint = '';
+ @state() autocompleteHint?: AutocompletionContext;
+
+ private autocompleteAcceptedHints: string[] = [];
+
+ /** Based on user preferences. */
+ @state() autocompleteEnabled = true;
+
+ readonly autocompleteCache = new AutocompleteCache();
/* The 'dirty' state of !comment.unresolved, which will be saved on demand. */
@state()
@@ -440,6 +457,7 @@
this,
() => this.getUserModel().preferences$,
prefs => {
+ this.autocompleteEnabled = !!prefs.allow_autocompleting_comments;
if (
this.generateSuggestion !==
!!prefs.allow_suggest_code_while_commenting
@@ -671,6 +689,10 @@
/* Making up for the 2px reduced height above. */
top: 1px;
}
+ gr-suggestion-diff-preview,
+ gr-fix-suggestions {
+ margin-top: var(--spacing-s);
+ }
`,
];
}
@@ -893,12 +915,70 @@
rows="4"
.placeholder=${this.messagePlaceholder}
text=${this.messageText}
- autocompleteHint=${this.autocompleteHint}
+ autocompleteHint=${this.autocompleteHint?.commentCompletion ?? ''}
@text-changed=${this.handleTextChanged}
+ @hintShown=${this.handleHintShown}
+ @hintApplied=${this.handleHintApplied}
></gr-suggestion-textarea>
`;
}
+ private handleHintShown(e: CustomEvent<HintShownEventDetail>) {
+ const context = this.autocompleteCache.get(e.detail.oldValue);
+ if (context?.commentCompletion !== e.detail.hint) return;
+
+ this.reportHintInteraction(
+ Interaction.COMMENT_COMPLETION_SUGGESTION_SHOWN,
+ context
+ );
+ }
+
+ private handleHintApplied(e: CustomEvent<HintAppliedEventDetail>) {
+ const context = this.autocompleteCache.get(e.detail.oldValue);
+ if (context?.commentCompletion !== e.detail.hint) return;
+
+ this.autocompleteAcceptedHints.push(e.detail.hint);
+ this.reportHintInteraction(
+ Interaction.COMMENT_COMPLETION_SUGGESTION_ACCEPTED,
+ context
+ );
+ }
+
+ private reportHintInteractionSaved() {
+ const content = this.messageText.trimEnd();
+ const acceptedHintsConcatenated = this.autocompleteAcceptedHints.join('');
+ const numExtraCharacters =
+ content.length - acceptedHintsConcatenated.length;
+ let distance = levenshteinDistance(acceptedHintsConcatenated, content);
+ if (numExtraCharacters > 0) {
+ distance -= numExtraCharacters;
+ }
+ const context = {
+ ...this.createAutocompletionBaseContext(),
+ similarCharacters: acceptedHintsConcatenated.length - distance,
+ maxSimilarCharacters: acceptedHintsConcatenated.length,
+ acceptedSuggestionsCount: this.autocompleteAcceptedHints.length,
+ totalAcceptedCharacters: acceptedHintsConcatenated.length,
+ savedDraftLength: content.length,
+ };
+ this.reportHintInteraction(
+ Interaction.COMMENT_COMPLETION_SAVE_DRAFT,
+ context
+ );
+ }
+
+ private reportHintInteraction(
+ interaction: Interaction,
+ context: Partial<AutocompletionContext>
+ ) {
+ context = {
+ ...context,
+ draftContent: '[REDACTED]',
+ commentCompletion: '[REDACTED]',
+ };
+ this.reporting.reportInteraction(interaction, context);
+ }
+
private handleTextChanged(e: ValueChangedEvent) {
const oldValue = this.messageText;
const newValue = e.detail.value;
@@ -909,33 +989,20 @@
// of the textare instead of needing a dedicated property.
this.messageText = newValue;
- this.handleTextChangedForAutocomplete(oldValue, newValue);
+ this.handleTextChangedForAutocomplete();
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;
+ handleTextChangedForAutocomplete() {
+ const cachedHint = this.autocompleteCache.get(this.messageText);
+ if (cachedHint) {
+ this.autocompleteHint = cachedHint;
+ } else {
+ this.autocompleteHint = undefined;
+ this.autocompleteTrigger$.next();
}
-
- // The default behavior is to reset the hint and to generate a new
- // autocomplete suggestion.
- this.autocompleteHint = '';
- this.autocompleteTrigger$.next();
}
private renderCommentMessage() {
@@ -1349,6 +1416,7 @@
const change = this.getChangeModel().getChange();
if (
!enabled ||
+ !this.autocompleteEnabled ||
!suggestionsProvider?.autocompleteComment ||
!change ||
!this.comment?.patch_set ||
@@ -1358,6 +1426,7 @@
return;
}
const commentText = this.messageText;
+ this.reporting.time(Timing.COMMENT_COMPLETION);
const response = await suggestionsProvider.autocompleteComment({
id: id(this.comment),
commentText,
@@ -1367,11 +1436,51 @@
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;
+ const elapsed = this.reporting.timeEnd(Timing.COMMENT_COMPLETION);
+ const context = this.createAutocompletionContext(
+ commentText,
+ response,
+ elapsed
+ );
+ this.reportHintInteraction(
+ Interaction.COMMENT_COMPLETION_SUGGESTION_FETCHED,
+ context
+ );
if (!response?.completion) return;
- this.autocompleteHint = response.completion;
+ // Note that we are setting the cache value for `commentText` and getting the value
+ // for `this.messageText`.
+ this.autocompleteCache.set(context);
+ this.autocompleteHint = this.autocompleteCache.get(this.messageText);
+ }
+
+ private createAutocompletionBaseContext(): Partial<AutocompletionContext> {
+ return {
+ commentId: id(this.comment!),
+ commentNumber: this.comments?.length ?? 0,
+ filePath: this.comment!.path,
+ fileExtension: getFileExtension(this.comment!.path ?? ''),
+ };
+ }
+
+ private createAutocompletionContext(
+ draftContent: string,
+ response: AutocompleteCommentResponse,
+ requestDurationMs: number
+ ): AutocompletionContext {
+ const commentCompletion = response.completion ?? '';
+ return {
+ ...this.createAutocompletionBaseContext(),
+
+ draftContent,
+ draftContentLength: draftContent.length,
+ commentCompletion,
+ commentCompletionLength: commentCompletion.length,
+
+ isFullCommentPrediction: draftContent.length === 0,
+ draftInSyncWithSuggestionLength: 0,
+ modelVersion: response.modelVersion ?? '',
+ requestDurationMs,
+ };
}
private renderRobotActions() {
@@ -1800,6 +1909,7 @@
if (this.isFixSuggestionChanged()) {
draft.fix_suggestions = this.getFixSuggestions();
}
+ this.reportHintInteractionSaved();
return this.getCommentsModel().saveDraft(draft, options.showToast);
}
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 7b4f63a..444a43f 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
@@ -898,27 +898,39 @@
suite('handleTextChangedForAutocomplete', () => {
test('foo -> foo with asdf', async () => {
- element.autocompleteHint = 'asdf';
- element.handleTextChangedForAutocomplete('foo', 'foo');
- assert.equal(element.autocompleteHint, 'asdf');
+ const ctx = {draftContent: 'foo', commentCompletion: 'asdf'};
+ element.autocompleteHint = ctx;
+ element.autocompleteCache.set(ctx);
+ element.messageText = 'foo';
+ element.handleTextChangedForAutocomplete();
+ assert.equal(element.autocompleteHint.commentCompletion, 'asdf');
});
test('foo -> bar with asdf', async () => {
- element.autocompleteHint = 'asdf';
- element.handleTextChangedForAutocomplete('foo', 'bar');
- assert.equal(element.autocompleteHint, '');
+ const ctx = {draftContent: 'foo', commentCompletion: 'asdf'};
+ element.autocompleteHint = ctx;
+ element.autocompleteCache.set(ctx);
+ element.messageText = 'bar';
+ element.handleTextChangedForAutocomplete();
+ assert.isUndefined(element.autocompleteHint);
});
test('foo -> foofoo with asdf', async () => {
- element.autocompleteHint = 'asdf';
- element.handleTextChangedForAutocomplete('foo', 'foofoo');
- assert.equal(element.autocompleteHint, '');
+ const ctx = {draftContent: 'foo', commentCompletion: 'asdf'};
+ element.autocompleteHint = ctx;
+ element.autocompleteCache.set(ctx);
+ element.messageText = 'foofoo';
+ element.handleTextChangedForAutocomplete();
+ assert.isUndefined(element.autocompleteHint);
});
test('foo -> foofoo with foomore', async () => {
- element.autocompleteHint = 'foomore';
- element.handleTextChangedForAutocomplete('foo', 'foofoo');
- assert.equal(element.autocompleteHint, 'more');
+ const ctx = {draftContent: 'foo', commentCompletion: 'foomore'};
+ element.autocompleteHint = ctx;
+ element.autocompleteCache.set(ctx);
+ element.messageText = 'foofoo';
+ element.handleTextChangedForAutocomplete();
+ assert.equal(element.autocompleteHint.commentCompletion, 'more');
});
});
diff --git a/polygerrit-ui/app/elements/shared/gr-fix-suggestions/gr-fix-suggestions.ts b/polygerrit-ui/app/elements/shared/gr-fix-suggestions/gr-fix-suggestions.ts
index 63c3832..195eeb6 100644
--- a/polygerrit-ui/app/elements/shared/gr-fix-suggestions/gr-fix-suggestions.ts
+++ b/polygerrit-ui/app/elements/shared/gr-fix-suggestions/gr-fix-suggestions.ts
@@ -87,6 +87,9 @@
static override get styles() {
return [
css`
+ :host {
+ display: block;
+ }
.header {
background-color: var(--background-color-primary);
border: 1px solid var(--border-color);
diff --git a/polygerrit-ui/app/elements/shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview.ts b/polygerrit-ui/app/elements/shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview.ts
index c533a5e..e1d4f89 100644
--- a/polygerrit-ui/app/elements/shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview.ts
+++ b/polygerrit-ui/app/elements/shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview.ts
@@ -152,9 +152,16 @@
static override get styles() {
return [
css`
+ :host {
+ display: block;
+ }
.buttons {
text-align: right;
}
+ .diff-container {
+ border: 1px solid var(--border-color);
+ border-top: none;
+ }
code {
max-width: var(--gr-formatted-text-prose-max-width, none);
background-color: var(--background-color-secondary);
@@ -221,14 +228,16 @@
if (!anyLineTooLong(diff)) {
this.syntaxLayer.process(diff);
}
- return html`<gr-diff
- .prefs=${this.overridePartialDiffPrefs()}
- .path=${this.preview.filepath}
- .diff=${diff}
- .layers=${this.layers}
- .renderPrefs=${this.renderPrefs}
- .viewMode=${DiffViewMode.UNIFIED}
- ></gr-diff>`;
+ return html`<div class="diff-container">
+ <gr-diff
+ .prefs=${this.overridePartialDiffPrefs()}
+ .path=${this.preview.filepath}
+ .diff=${diff}
+ .layers=${this.layers}
+ .renderPrefs=${this.renderPrefs}
+ .viewMode=${DiffViewMode.UNIFIED}
+ ></gr-diff>
+ </div>`;
}
private async fetchFixPreview() {
diff --git a/polygerrit-ui/app/elements/shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview_test.ts b/polygerrit-ui/app/elements/shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview_test.ts
index 86be868..2630aad 100644
--- a/polygerrit-ui/app/elements/shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview_test.ts
@@ -103,10 +103,12 @@
assert.shadowDom.equal(
element,
/* HTML */ `
- <gr-diff
- class="disable-context-control-buttons hide-line-length-indicator"
- >
- </gr-diff>
+ <div class="diff-container">
+ <gr-diff
+ class="disable-context-control-buttons hide-line-length-indicator"
+ >
+ </gr-diff>
+ </div>
`,
{ignoreAttributes: ['style']}
);
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 2d22f6a..a8a779d 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
@@ -348,6 +348,12 @@
position: relative;
--gr-textarea-padding: var(--spacing-s);
--gr-textarea-border-width: 0px;
+ --gr-textarea-border-color: var(--border-color);
+ --input-field-bg: var(--view-background-color);
+ --input-field-disabled-bg: var(--view-background-color);
+ --secondary-bg-color: var(--background-color-secondary);
+ --text-default: var(--primary-text-color);
+ --text-disabled: var(--deemphasized-text-color);
--text-secondary: var(--deemphasized-text-color);
--iron-autogrow-textarea_-_padding: var(--spacing-s);
}
diff --git a/polygerrit-ui/app/embed/diff/gr-syntax-layer/gr-syntax-layer-worker.ts b/polygerrit-ui/app/embed/diff/gr-syntax-layer/gr-syntax-layer-worker.ts
index 5074290..9337951 100644
--- a/polygerrit-ui/app/embed/diff/gr-syntax-layer/gr-syntax-layer-worker.ts
+++ b/polygerrit-ui/app/embed/diff/gr-syntax-layer/gr-syntax-layer-worker.ts
@@ -23,6 +23,7 @@
['application/typescript', 'typescript'],
['application/xml', 'xml'],
['application/xquery', 'xquery'],
+ ['application/x-epp', 'epp'],
['application/x-erb', 'erb'],
['text/css', 'css'],
['text/html', 'html'],
diff --git a/polygerrit-ui/app/embed/gr-textarea.ts b/polygerrit-ui/app/embed/gr-textarea.ts
index 628b65b..9b88e9c 100644
--- a/polygerrit-ui/app/embed/gr-textarea.ts
+++ b/polygerrit-ui/app/embed/gr-textarea.ts
@@ -456,13 +456,7 @@
const value = await this.getValue();
this.innerValue = value;
- this.dispatchEvent(
- new CustomEvent('input', {
- detail: {
- value: this.value,
- },
- })
- );
+ this.fire('input', {value: this.value});
}
private onFocus(event: Event) {
@@ -492,7 +486,7 @@
(event.ctrlKey || event.metaKey)
) {
event.preventDefault();
- this.dispatchEvent(new CustomEvent('saveShortcut'));
+ this.fire('saveShortcut');
}
await this.toggleHintVisibilityIfAny();
}
@@ -507,7 +501,13 @@
}
private handleScroll() {
- this.dispatchEvent(new CustomEvent('scroll'));
+ this.fire('scroll');
+ }
+
+ private fire<T>(type: string, detail?: T) {
+ this.dispatchEvent(
+ new CustomEvent(type, {detail, bubbles: true, composed: true})
+ );
}
private async handleTabKeyPress(event: KeyboardEvent) {
@@ -529,14 +529,7 @@
await this.putCursorAtEnd();
await this.onInput(event);
- this.dispatchEvent(
- new CustomEvent('hintApplied', {
- detail: {
- hint,
- oldValue,
- },
- })
- );
+ this.fire('hintApplied', {hint, oldValue});
}
private async toggleHintVisibilityIfAny() {
@@ -572,6 +565,7 @@
}
private addHintSpanAtEndOfContent(editableDivElement: Node, hint: string) {
+ const oldValue = this.value ?? '';
const hintSpan = document.createElement('span');
hintSpan.classList.add(AUTOCOMPLETE_HINT_CLASS);
hintSpan.setAttribute('role', 'alert');
@@ -581,26 +575,16 @@
);
hintSpan.dataset['hint'] = hint;
editableDivElement.appendChild(hintSpan);
- this.dispatchEvent(
- new CustomEvent('hintShown', {
- detail: {
- hint,
- },
- })
- );
+ this.fire('hintShown', {hint, oldValue});
}
private removeHintSpanIfShown() {
const hintSpan = this.hintSpan();
if (hintSpan) {
hintSpan.remove();
- this.dispatchEvent(
- new CustomEvent('hintDismissed', {
- detail: {
- hint: (hintSpan as HTMLElement).dataset['hint'],
- },
- })
- );
+ this.fire('hintDismissed', {
+ hint: (hintSpan as HTMLElement).dataset['hint'],
+ });
}
}
@@ -616,13 +600,7 @@
event?.preventDefault();
event?.stopImmediatePropagation();
- this.dispatchEvent(
- new CustomEvent('cursorPositionChange', {
- detail: {
- position: this.getCursorPosition(),
- },
- })
- );
+ this.fire('cursorPositionChange', {position: this.getCursorPosition()});
}
private async updateValueInDom() {
diff --git a/polygerrit-ui/app/models/accounts-model/accounts-model_test.ts b/polygerrit-ui/app/models/accounts-model/accounts-model_test.ts
deleted file mode 100644
index 53c90a6..0000000
--- a/polygerrit-ui/app/models/accounts-model/accounts-model_test.ts
+++ /dev/null
@@ -1,42 +0,0 @@
-/**
- * @license
- * Copyright 2022 Google LLC
- * SPDX-License-Identifier: Apache-2.0
- */
-
-import '../../test/common-test-setup';
-import {EmailAddress} from '../../api/rest-api';
-import {getAppContext} from '../../services/app-context';
-import {stubRestApi} from '../../test/test-utils';
-import {AccountsModel} from './accounts-model';
-import {assert} from '@open-wc/testing';
-
-suite('accounts-model tests', () => {
- let model: AccountsModel;
-
- setup(() => {
- model = new AccountsModel(getAppContext().restApiService);
- });
-
- teardown(() => {
- model.finalize();
- });
-
- test('invalid account makes only one request', () => {
- const response = {...new Response(), status: 404};
- const getAccountDetails = stubRestApi('getAccountDetails').callsFake(
- (_, errFn) => {
- if (errFn !== undefined) {
- errFn(response);
- }
- return Promise.resolve(undefined);
- }
- );
-
- model.fillDetails({email: 'Invalid_email@def.com' as EmailAddress});
- assert.equal(getAccountDetails.callCount, 1);
-
- model.fillDetails({email: 'Invalid_email@def.com' as EmailAddress});
- assert.equal(getAccountDetails.callCount, 1);
- });
-});
diff --git a/polygerrit-ui/app/models/accounts-model/accounts-model.ts b/polygerrit-ui/app/models/accounts/accounts-model.ts
similarity index 95%
rename from polygerrit-ui/app/models/accounts-model/accounts-model.ts
rename to polygerrit-ui/app/models/accounts/accounts-model.ts
index 0802f06..6eedcbe8 100644
--- a/polygerrit-ui/app/models/accounts-model/accounts-model.ts
+++ b/polygerrit-ui/app/models/accounts/accounts-model.ts
@@ -42,7 +42,7 @@
): Promise<AccountDetailInfo | AccountInfo> {
const current = this.getState();
const id = getUserId(partialAccount);
- if (hasOwnProperty(current.accounts, id)) return current.accounts[id];
+ if (hasOwnProperty(current.accounts, id)) return {...current.accounts[id]};
// It is possible to add emails to CC when they don't have a Gerrit
// account. In this case getAccountDetails will return a 404 error then
// we at least use what is in partialAccount.
diff --git a/polygerrit-ui/app/models/accounts/accounts-model_test.ts b/polygerrit-ui/app/models/accounts/accounts-model_test.ts
new file mode 100644
index 0000000..e84723c
--- /dev/null
+++ b/polygerrit-ui/app/models/accounts/accounts-model_test.ts
@@ -0,0 +1,71 @@
+/**
+ * @license
+ * Copyright 2022 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import '../../test/common-test-setup';
+import {
+ AccountDetailInfo,
+ AccountId,
+ EmailAddress,
+ Timestamp,
+} from '../../api/rest-api';
+import {getAppContext} from '../../services/app-context';
+import {stubRestApi} from '../../test/test-utils';
+import {AccountsModel} from './accounts-model';
+import {assert} from '@open-wc/testing';
+
+const KERMIT: AccountDetailInfo = {
+ _account_id: 1 as AccountId,
+ name: 'Kermit',
+ registered_on: '2015-03-12 18:32:08.000000000' as Timestamp,
+};
+
+suite('accounts-model tests', () => {
+ let model: AccountsModel;
+
+ setup(() => {
+ model = new AccountsModel(getAppContext().restApiService);
+ });
+
+ teardown(() => {
+ model.finalize();
+ });
+
+ test('basic lookup', async () => {
+ const stub = stubRestApi('getAccountDetails').returns(
+ Promise.resolve(KERMIT)
+ );
+
+ let filled = await model.fillDetails({_account_id: 1 as AccountId});
+ assert.equal(filled.name, 'Kermit');
+ assert.equal(filled, KERMIT);
+ assert.equal(stub.callCount, 1);
+
+ filled = await model.fillDetails({_account_id: 1 as AccountId});
+ assert.equal(filled.name, 'Kermit');
+ // Cache objects are cloned on lookup, so this is a different object.
+ assert.notEqual(filled, KERMIT);
+ // Did not have to call the REST API again.
+ assert.equal(stub.callCount, 1);
+ });
+
+ test('invalid account makes only one request', () => {
+ const response = {...new Response(), status: 404};
+ const getAccountDetails = stubRestApi('getAccountDetails').callsFake(
+ (_, errFn) => {
+ if (errFn !== undefined) {
+ errFn(response);
+ }
+ return Promise.resolve(undefined);
+ }
+ );
+
+ model.fillDetails({email: 'Invalid_email@def.com' as EmailAddress});
+ assert.equal(getAccountDetails.callCount, 1);
+
+ model.fillDetails({email: 'Invalid_email@def.com' as EmailAddress});
+ assert.equal(getAccountDetails.callCount, 1);
+ });
+});
diff --git a/polygerrit-ui/app/models/comments/comments-model.ts b/polygerrit-ui/app/models/comments/comments-model.ts
index f9a8401..497962f 100644
--- a/polygerrit-ui/app/models/comments/comments-model.ts
+++ b/polygerrit-ui/app/models/comments/comments-model.ts
@@ -56,7 +56,7 @@
import {Deduping} from '../../api/reporting';
import {extractMentionedUsers, getUserId} from '../../utils/account-util';
import {SpecialFilePath} from '../../constants/constants';
-import {AccountsModel} from '../accounts-model/accounts-model';
+import {AccountsModel} from '../accounts/accounts-model';
import {
distinctUntilChanged,
map,
@@ -432,7 +432,7 @@
threads.filter(t => !isNewThread(t) && isDraftThread(t))
);
- public readonly threadsWithSuggestions$ = select(
+ public readonly threadsWithUnappliedSuggestions$ = select(
combineLatest([this.threads$, this.changeModel.latestPatchNum$]),
([threads, latestPs]) =>
threads.filter(
diff --git a/polygerrit-ui/app/models/comments/comments-model_test.ts b/polygerrit-ui/app/models/comments/comments-model_test.ts
index 0d3df42..b3dbc08 100644
--- a/polygerrit-ui/app/models/comments/comments-model_test.ts
+++ b/polygerrit-ui/app/models/comments/comments-model_test.ts
@@ -28,7 +28,7 @@
import {changeModelToken} from '../change/change-model';
import {assert} from '@open-wc/testing';
import {testResolver} from '../../test/common-test-setup';
-import {accountsModelToken} from '../accounts-model/accounts-model';
+import {accountsModelToken} from '../accounts/accounts-model';
import {ChangeComments} from '../../elements/diff/gr-comment-api/gr-comment-api';
import {changeViewModelToken} from '../views/change';
import {navigationToken} from '../../elements/core/gr-navigation/gr-navigation';
diff --git a/polygerrit-ui/app/models/user/user-model.ts b/polygerrit-ui/app/models/user/user-model.ts
index cd6a66a..4973307 100644
--- a/polygerrit-ui/app/models/user/user-model.ts
+++ b/polygerrit-ui/app/models/user/user-model.ts
@@ -4,7 +4,7 @@
* SPDX-License-Identifier: Apache-2.0
*/
import {from, of, Observable} from 'rxjs';
-import {filter, switchMap} from 'rxjs/operators';
+import {filter, switchMap, tap} from 'rxjs/operators';
import {
DiffPreferencesInfo as DiffPreferencesInfoAPI,
DiffViewMode,
@@ -13,6 +13,7 @@
AccountCapabilityInfo,
AccountDetailInfo,
EditPreferencesInfo,
+ EmailInfo,
PreferencesInfo,
TopMenuItemInfo,
} from '../../types/common';
@@ -48,6 +49,7 @@
* `account` is known, then use `accountLoaded` below.
*/
account?: AccountDetailInfo;
+ emails?: EmailInfo[];
/**
* Starts as `false` and switches to `true` after the first `getAccount` call.
* A common use case for this is to wait with loading or doing something until
@@ -82,6 +84,15 @@
userState => userState.account
);
+ readonly emails$: Observable<EmailInfo[] | undefined> = select(
+ this.state$,
+ userState => userState.emails
+ ).pipe(
+ tap(emails => {
+ if (emails === undefined) this.loadEmails();
+ })
+ );
+
/**
* Only emits once we have tried to actually load the account. Note that
* this does not initially emit a value.
@@ -148,12 +159,8 @@
super({
accountLoaded: false,
});
+ this.loadAccount();
this.subscriptions = [
- from(this.restApiService.getAccount()).subscribe(
- (account?: AccountDetailInfo) => {
- this.setAccount(account);
- }
- ),
this.loadedAccount$
.pipe(
switchMap(account => {
@@ -261,4 +268,22 @@
setAccount(account?: AccountDetailInfo) {
this.updateState({account, accountLoaded: true});
}
+
+ private setAccountEmails(emails?: EmailInfo[]) {
+ this.updateState({emails});
+ }
+
+ loadAccount(noCache?: boolean) {
+ if (noCache) this.restApiService.invalidateAccountsDetailCache();
+ return this.restApiService.getAccount().then(account => {
+ this.setAccount(account);
+ });
+ }
+
+ loadEmails(noCache?: boolean) {
+ if (noCache) this.restApiService.invalidateAccountsEmailCache();
+ return this.restApiService.getAccountEmails().then(emails => {
+ this.setAccountEmails(emails);
+ });
+ }
}
diff --git a/polygerrit-ui/app/node_modules_licenses/licenses.ts b/polygerrit-ui/app/node_modules_licenses/licenses.ts
index 9fa2e89..21fab53 100644
--- a/polygerrit-ui/app/node_modules_licenses/licenses.ts
+++ b/polygerrit-ui/app/node_modules_licenses/licenses.ts
@@ -454,6 +454,14 @@
},
},
{
+ name: 'highlightjs-epp',
+ license: {
+ name: 'highlightjs-epp',
+ type: LicenseTypes.Bsd3,
+ packageLicenseFile: 'LICENSE',
+ },
+ },
+ {
name: 'highlightjs-structured-text',
license: {
name: 'highlightjs-structured-text',
diff --git a/polygerrit-ui/app/package.json b/polygerrit-ui/app/package.json
index 73816d2..dea0e8b 100644
--- a/polygerrit-ui/app/package.json
+++ b/polygerrit-ui/app/package.json
@@ -35,8 +35,9 @@
"@webcomponents/shadycss": "^1.11.2",
"@webcomponents/webcomponentsjs": "^1.3.3",
"highlight.js": "^11.9.0",
- "highlightjs-closure-templates": "https://github.com/highlightjs/highlightjs-closure-templates",
- "highlightjs-structured-text": "https://github.com/highlightjs/highlightjs-structured-text",
+ "highlightjs-closure-templates": "https://github.com/highlightjs/highlightjs-closure-templates#02fb0646e0499084f96a99b8c6f4a0d7bd1d33ba",
+ "highlightjs-epp": "https://github.com/highlightjs/highlightjs-epp#9f9e1a92f37c217c68899c7d3bdccb4d134681b9",
+ "highlightjs-structured-text": "https://github.com/highlightjs/highlightjs-structured-text#e68dd7aa829529fb6c40d6287585f43273605a9e",
"immer": "^9.0.21",
"lit": "^3.1.2",
"polymer-bridges": "file:../../polymer-bridges",
diff --git a/polygerrit-ui/app/services/app-context-init.ts b/polygerrit-ui/app/services/app-context-init.ts
index 40517ac..8536492 100644
--- a/polygerrit-ui/app/services/app-context-init.ts
+++ b/polygerrit-ui/app/services/app-context-init.ts
@@ -34,7 +34,7 @@
import {
AccountsModel,
accountsModelToken,
-} from '../models/accounts-model/accounts-model';
+} from '../models/accounts/accounts-model';
import {
DashboardViewModel,
dashboardViewModelToken,
diff --git a/polygerrit-ui/app/services/gr-reporting/gr-reporting.ts b/polygerrit-ui/app/services/gr-reporting/gr-reporting.ts
index 6df2c67..e175228 100644
--- a/polygerrit-ui/app/services/gr-reporting/gr-reporting.ts
+++ b/polygerrit-ui/app/services/gr-reporting/gr-reporting.ts
@@ -55,7 +55,7 @@
/**
* Finish named timer and report it to server.
*/
- timeEnd(name: Timing, eventDetails?: EventDetails): void;
+ timeEnd(name: Timing, eventDetails?: EventDetails): number;
/**
* Get a timer object for reporting a user timing. The start time will be
* the time that the object has been created, and the end time will be the
diff --git a/polygerrit-ui/app/services/gr-reporting/gr-reporting_impl.ts b/polygerrit-ui/app/services/gr-reporting/gr-reporting_impl.ts
index 1eb3bc2..781f370 100644
--- a/polygerrit-ui/app/services/gr-reporting/gr-reporting_impl.ts
+++ b/polygerrit-ui/app/services/gr-reporting/gr-reporting_impl.ts
@@ -770,23 +770,26 @@
/**
* Finish named timer and report it to server.
*/
- timeEnd(name: Timing, eventDetails?: EventDetails) {
+ timeEnd(name: Timing, eventDetails?: EventDetails): number {
if (!hasOwnProperty(this._baselines, name)) {
- return;
+ return 0;
}
- const baseTime = this._baselines[name];
+ const begin = this._baselines[name];
delete this._baselines[name];
- this._reportTiming(name, now() - baseTime, eventDetails);
+ const end = now();
+ const elapsed = end - begin;
+ this._reportTiming(name, elapsed, eventDetails);
// Finalize the interval. Either from a registered start mark or
// the navigation start time (if baseTime is 0).
- if (baseTime !== 0) {
+ if (begin !== 0) {
window.performance.measure(name, `${name}-start`);
} else {
// Microsoft Edge does not handle the 2nd param correctly
// (if undefined).
window.performance.measure(name);
}
+ return elapsed;
}
/**
diff --git a/polygerrit-ui/app/services/gr-reporting/gr-reporting_mock.ts b/polygerrit-ui/app/services/gr-reporting/gr-reporting_mock.ts
index fb1f0c3..7cb777a 100644
--- a/polygerrit-ui/app/services/gr-reporting/gr-reporting_mock.ts
+++ b/polygerrit-ui/app/services/gr-reporting/gr-reporting_mock.ts
@@ -71,5 +71,5 @@
setRepoName: () => {},
setChangeId: () => {},
time: () => {},
- timeEnd: () => {},
+ timeEnd: () => 0,
};
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 0be3ab5..7a12e7a 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
@@ -1565,6 +1565,10 @@
this._restApiHelper.invalidateFetchPromisesPrefix('/accounts/self/detail');
}
+ invalidateAccountsEmailCache() {
+ this._restApiHelper.invalidateFetchPromisesPrefix('/accounts/self/emails');
+ }
+
getGroups(filter: string, groupsPerPage: number, offset?: number) {
const url = this._getGroupsUrl(filter, groupsPerPage, offset);
diff --git a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts
index 947952c..814e97a 100644
--- a/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts
+++ b/polygerrit-ui/app/services/gr-rest-api/gr-rest-api.ts
@@ -542,6 +542,7 @@
invalidateReposCache(): void;
invalidateAccountsCache(): void;
invalidateAccountsDetailCache(): void;
+ invalidateAccountsEmailCache(): void;
removeFromAttentionSet(
changeNum: NumericChangeId,
user: AccountId,
diff --git a/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts b/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
index 77f2498..570b50a 100644
--- a/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
+++ b/polygerrit-ui/app/test/mocks/gr-rest-api_mock.ts
@@ -442,6 +442,7 @@
invalidateGroupsCache(): void {},
invalidateReposCache(): void {},
invalidateAccountsDetailCache(): void {},
+ invalidateAccountsEmailCache(): void {},
probePath(): Promise<boolean> {
return Promise.resolve(true);
},
diff --git a/polygerrit-ui/app/test/test-data-generators.ts b/polygerrit-ui/app/test/test-data-generators.ts
index 3829f57..7d666e8 100644
--- a/polygerrit-ui/app/test/test-data-generators.ts
+++ b/polygerrit-ui/app/test/test-data-generators.ts
@@ -712,6 +712,7 @@
email_strategy: EmailStrategy.ENABLED,
allow_browser_notifications: true,
allow_suggest_code_while_commenting: true,
+ allow_autocompleting_comments: true,
};
}
diff --git a/polygerrit-ui/app/types/common.ts b/polygerrit-ui/app/types/common.ts
index 4dd965c..072a872 100644
--- a/polygerrit-ui/app/types/common.ts
+++ b/polygerrit-ui/app/types/common.ts
@@ -1344,6 +1344,7 @@
email_format?: EmailFormat;
allow_browser_notifications?: boolean;
allow_suggest_code_while_commenting?: boolean;
+ allow_autocompleting_comments?: boolean;
diff_page_sidebar?: DiffPageSidebar;
}
diff --git a/polygerrit-ui/app/utils/account-util.ts b/polygerrit-ui/app/utils/account-util.ts
index 0853941..b93acc6 100644
--- a/polygerrit-ui/app/utils/account-util.ts
+++ b/polygerrit-ui/app/utils/account-util.ts
@@ -140,10 +140,10 @@
export function isDetailedAccount(account?: AccountInfo) {
// In case ChangeInfo is requested without DetailedAccount option, the
- // reviewer entry is returned as just {_account_id: 123}
- // This object should also be treated as not detailed account if they have
- // an AccountId and no email
- return !!account?.email && !!account?._account_id;
+ // reviewer entry is returned as just {_account_id: 123}.
+ // At least a name or an email must be set for the account to be treated as
+ // "detailed".
+ return (!!account?.email || !!account?.name) && !!account?._account_id;
}
/**
diff --git a/polygerrit-ui/app/utils/account-util_test.ts b/polygerrit-ui/app/utils/account-util_test.ts
index 72fa791..b1ee50e 100644
--- a/polygerrit-ui/app/utils/account-util_test.ts
+++ b/polygerrit-ui/app/utils/account-util_test.ts
@@ -263,6 +263,7 @@
test('isDetailedAccount', () => {
assert.isFalse(isDetailedAccount({_account_id: 12345 as AccountId}));
assert.isFalse(isDetailedAccount({email: 'abcd' as EmailAddress}));
+ assert.isFalse(isDetailedAccount({name: 'Kermit'}));
assert.isTrue(
isDetailedAccount({
@@ -270,6 +271,12 @@
email: 'abcd' as EmailAddress,
})
);
+ assert.isTrue(
+ isDetailedAccount({
+ _account_id: 12345 as AccountId,
+ name: 'Kermit',
+ })
+ );
});
test('fails gracefully when all is not included', async () => {
diff --git a/polygerrit-ui/app/utils/autocomplete-cache.ts b/polygerrit-ui/app/utils/autocomplete-cache.ts
new file mode 100644
index 0000000..c8077ab
--- /dev/null
+++ b/polygerrit-ui/app/utils/autocomplete-cache.ts
@@ -0,0 +1,79 @@
+/**
+ * @license
+ * Copyright 2024 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+export interface AutocompletionContext {
+ draftContent: string;
+ draftContentLength?: number;
+ commentCompletion: string;
+ commentCompletionLength?: number;
+
+ isFullCommentPrediction?: boolean;
+ draftInSyncWithSuggestionLength?: number;
+ modelVersion?: string;
+ requestDurationMs?: number;
+
+ commentId?: string;
+ commentNumber?: number;
+ filePath?: string;
+ fileExtension?: string;
+
+ similarCharacters?: number;
+ maxSimilarCharacters?: number;
+ acceptedSuggestionsCount?: number;
+ totalAcceptedCharacters?: number;
+ savedDraftLength?: number;
+}
+
+/**
+ * Caching for autocompleting text, e.g. comments.
+ *
+ * If the user continues typing text that matches the completion hint, then keep the hint.
+ *
+ * If the user backspaces, then continue using previous hint.
+ */
+export class AutocompleteCache {
+ /**
+ * We are using an ordered list instead of a map here, because we want to evict the oldest
+ * entries, if the capacity is exceeded. And we want to prefer newer entries over older
+ * entries, if both match the criteria for being reused.
+ */
+ private cache: AutocompletionContext[] = [];
+
+ constructor(private readonly capacity = 10) {}
+
+ get(content: string): AutocompletionContext | undefined {
+ if (content === '') return undefined;
+ for (let i = this.cache.length - 1; i >= 0; i--) {
+ const cachedContext = this.cache[i];
+ const completionContent = cachedContext.draftContent;
+ const completionHint = cachedContext.commentCompletion;
+ const completionFull = completionContent + completionHint;
+ if (completionContent.length > content.length) continue;
+ if (!completionFull.startsWith(content)) continue;
+ if (completionFull === content) continue;
+ const hint = completionFull.substring(content.length);
+ return {
+ ...cachedContext,
+ draftContent: content,
+ commentCompletion: hint,
+ draftInSyncWithSuggestionLength:
+ content.length - completionContent.length,
+ };
+ }
+ return undefined;
+ }
+
+ set(context: AutocompletionContext) {
+ const index = this.cache.findIndex(
+ c => c.draftContent === context.draftContent
+ );
+ if (index !== -1) {
+ this.cache.splice(index, 1);
+ } else if (this.cache.length >= this.capacity) {
+ this.cache.shift();
+ }
+ this.cache.push(context);
+ }
+}
diff --git a/polygerrit-ui/app/utils/autocomplete-cache_test.ts b/polygerrit-ui/app/utils/autocomplete-cache_test.ts
new file mode 100644
index 0000000..970436b
--- /dev/null
+++ b/polygerrit-ui/app/utils/autocomplete-cache_test.ts
@@ -0,0 +1,89 @@
+/**
+ * @license
+ * Copyright 2024 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+import {AutocompleteCache} from './autocomplete-cache';
+import {assert} from '@open-wc/testing';
+
+suite('AutocompleteCache', () => {
+ let cache: AutocompleteCache;
+
+ setup(() => {
+ cache = new AutocompleteCache();
+ });
+
+ const cacheSet = (draftContent: string, commentCompletion: string) => {
+ cache.set({draftContent, commentCompletion});
+ };
+
+ const assertCacheEqual = (
+ draftContent: string,
+ expectedCommentCompletion?: string
+ ) => {
+ assert.equal(
+ cache.get(draftContent)?.commentCompletion,
+ expectedCommentCompletion
+ );
+ };
+
+ test('should get and set values', () => {
+ cacheSet('foo', 'bar');
+ assertCacheEqual('foo', 'bar');
+ });
+
+ test('should return undefined for empty content string', () => {
+ cacheSet('foo', 'bar');
+ assertCacheEqual('', undefined);
+ });
+
+ test('should return a value, if completion content+hint start with content', () => {
+ cacheSet('foo', 'bar');
+ assertCacheEqual('foo', 'bar');
+ assertCacheEqual('foob', 'ar');
+ assertCacheEqual('fooba', 'r');
+ assertCacheEqual('foobar', undefined);
+ });
+
+ test('should not return a value, if content is shorter than completion content', () => {
+ cacheSet('foo', 'bar');
+ assertCacheEqual('f', undefined);
+ assertCacheEqual('fo', undefined);
+ });
+
+ test('should not get values that are not set', () => {
+ assertCacheEqual('foo', undefined);
+ });
+
+ test('should not return an empty completion, if content equals completion content+hint', () => {
+ cacheSet('foo', 'bar');
+ assertCacheEqual('foobar', undefined);
+ });
+
+ test('skips over the first entry, but returns the second entry', () => {
+ cacheSet('foobar', 'bang');
+ cacheSet('foo', 'bar');
+ assertCacheEqual('foobar', 'bang');
+ });
+
+ test('replaces entries', () => {
+ cacheSet('foo', 'bar');
+ cacheSet('foo', 'baz');
+ assertCacheEqual('foo', 'baz');
+ });
+
+ test('prefers newer entries, but also returns older entries', () => {
+ cacheSet('foo', 'bar');
+ assertCacheEqual('foob', 'ar');
+ cacheSet('foob', 'arg');
+ assertCacheEqual('foob', 'arg');
+ assertCacheEqual('foo', 'bar');
+ });
+
+ test('capacity', () => {
+ cache = new AutocompleteCache(1);
+ cacheSet('foo', 'bar');
+ cacheSet('boom', 'bang');
+ assertCacheEqual('foo', undefined);
+ });
+});
diff --git a/polygerrit-ui/app/utils/deep-util.ts b/polygerrit-ui/app/utils/deep-util.ts
index eca528e..3e41e61 100644
--- a/polygerrit-ui/app/utils/deep-util.ts
+++ b/polygerrit-ui/app/utils/deep-util.ts
@@ -82,7 +82,7 @@
/**
* @param obj Object
*/
-export function deepClone(obj?: object) {
- if (!obj) return undefined;
- return JSON.parse(JSON.stringify(obj));
+export function deepClone<T>(obj: T): T {
+ if (!obj) throw new Error('undefined object for deepClone');
+ return JSON.parse(JSON.stringify(obj)) as T;
}
diff --git a/polygerrit-ui/app/utils/string-util.ts b/polygerrit-ui/app/utils/string-util.ts
index 81dcde1..abc5529 100644
--- a/polygerrit-ui/app/utils/string-util.ts
+++ b/polygerrit-ui/app/utils/string-util.ts
@@ -115,3 +115,42 @@
fileName: fileNameSection,
};
}
+
+/**
+ * Computes the Levenshtein edit distance between two strings.
+ */
+export function levenshteinDistance(str1: string, str2: string): number {
+ const m = str1.length;
+ const n = str2.length;
+
+ // Create a matrix to store edit distances
+ const dp: number[][] = Array.from({length: m + 1}, () =>
+ Array(n + 1).fill(0)
+ );
+
+ // Initialize first row and column with base cases
+ for (let i = 0; i <= m; i++) {
+ dp[i][0] = i;
+ }
+ for (let j = 0; j <= n; j++) {
+ dp[0][j] = j;
+ }
+
+ // Calculate edit distances for all substrings
+ for (let i = 1; i <= m; i++) {
+ for (let j = 1; j <= n; j++) {
+ if (str1[i - 1] === str2[j - 1]) {
+ dp[i][j] = dp[i - 1][j - 1];
+ } else {
+ dp[i][j] = Math.min(
+ dp[i - 1][j] + 1, // Deletion
+ dp[i][j - 1] + 1, // Insertion
+ dp[i - 1][j - 1] + 1 // Substitution
+ );
+ }
+ }
+ }
+
+ // Return the final edit distance
+ return dp[m][n];
+}
diff --git a/polygerrit-ui/app/yarn.lock b/polygerrit-ui/app/yarn.lock
index b2f722f..a8f10e2 100644
--- a/polygerrit-ui/app/yarn.lock
+++ b/polygerrit-ui/app/yarn.lock
@@ -610,18 +610,24 @@
resolved "https://registry.yarnpkg.com/highlight.js/-/highlight.js-10.7.3.tgz#697272e3991356e40c3cac566a74eef681756531"
integrity sha512-tzcUFauisWKNHaRkN4Wjl/ZA07gENAjFl3J/c480dprkGTg5EQstgaNFqBfUqCq54kZRIEcreTsAgF/m2quD7A==
-"highlight.js@^11.5.0 || ^10.4.1", highlight.js@^11.9.0:
+highlight.js@^11.9.0, "highlight.js@^11.9.0 || ^10.4.1":
version "11.9.0"
resolved "https://registry.yarnpkg.com/highlight.js/-/highlight.js-11.9.0.tgz#04ab9ee43b52a41a047432c8103e2158a1b8b5b0"
integrity sha512-fJ7cW7fQGCYAkgv4CPfwFHrfd/cLS4Hau96JuJ+ZTOWhjnhoeN1ub1tFmALm/+lW5z4WCAuAV9bm05AP0mS6Gw==
-"highlightjs-closure-templates@https://github.com/highlightjs/highlightjs-closure-templates":
+"highlightjs-closure-templates@https://github.com/highlightjs/highlightjs-closure-templates#02fb0646e0499084f96a99b8c6f4a0d7bd1d33ba":
version "0.0.1"
- resolved "https://github.com/highlightjs/highlightjs-closure-templates#7922b1e68def8b10199e186bb679600de3ebb711"
+ resolved "https://github.com/highlightjs/highlightjs-closure-templates#02fb0646e0499084f96a99b8c6f4a0d7bd1d33ba"
dependencies:
- highlight.js "^11.5.0 || ^10.4.1"
+ highlight.js "^11.9.0 || ^10.4.1"
-"highlightjs-structured-text@https://github.com/highlightjs/highlightjs-structured-text":
+"highlightjs-epp@https://github.com/highlightjs/highlightjs-epp#9f9e1a92f37c217c68899c7d3bdccb4d134681b9":
+ version "0.0.1"
+ resolved "https://github.com/highlightjs/highlightjs-epp#9f9e1a92f37c217c68899c7d3bdccb4d134681b9"
+ dependencies:
+ highlight.js "^11.9.0"
+
+"highlightjs-structured-text@https://github.com/highlightjs/highlightjs-structured-text#e68dd7aa829529fb6c40d6287585f43273605a9e":
version "1.4.9"
resolved "https://github.com/highlightjs/highlightjs-structured-text#e68dd7aa829529fb6c40d6287585f43273605a9e"
dependencies:
diff --git a/proto/entities.proto b/proto/entities.proto
index be04e97..6d5ac04 100644
--- a/proto/entities.proto
+++ b/proto/entities.proto
@@ -277,7 +277,7 @@
// Next ID: 2
message ObjectId {
// Hex string representation of the ID.
- optional string name = 1;
+ optional string name = 1 [default="0000000000000000000000000000000000000000"];
}
// Serialized form of a continuation token used for pagination.
@@ -289,7 +289,7 @@
// Proto representation of the User preferences classes
// Next ID: 4
message UserPreferences {
- // Next ID: 24
+ // Next ID: 26
message GeneralPreferencesInfo {
// Number of changes to show in a screen.
optional int32 changes_per_page = 1 [default = 25];
@@ -368,6 +368,8 @@
repeated string change_table = 18;
optional bool allow_browser_notifications = 19 [default = true];
+ optional bool allow_suggest_code_while_commenting = 24 [default = true];
+ optional bool allow_autocompleting_comments = 25 [default = true];
optional string diff_page_sidebar = 23 [default = "NONE"];
}
optional GeneralPreferencesInfo general_preferences_info = 1;
@@ -453,11 +455,11 @@
optional Side side = 2 [default = REVISION];
message Range {
// 1-based
- optional int32 start_line = 1;
+ optional int32 start_line = 1 [default = 1];
// 0-based
optional int32 start_char = 2;
// 1-based
- optional int32 end_line = 3;
+ optional int32 end_line = 3 [default = 1];
// 0-based
optional int32 end_char = 4;
}
@@ -466,7 +468,7 @@
// number is identical to the range's end line.
optional Range position_range = 3;
// 1-based
- optional int32 line_number = 4;
+ optional int32 line_number = 4 [default = 1];
}
// If not set, the comment is on the patchset level.
@@ -488,4 +490,4 @@
optional fixed64 written_on_millis = 11;
// Required.
optional string server_id = 12;
-}
\ No newline at end of file
+}
diff --git a/resources/com/google/gerrit/server/mime/mime-types.properties b/resources/com/google/gerrit/server/mime/mime-types.properties
index 642ef474..432d088 100644
--- a/resources/com/google/gerrit/server/mime/mime-types.properties
+++ b/resources/com/google/gerrit/server/mime/mime-types.properties
@@ -63,6 +63,7 @@
el = text/x-common-lisp
elm = text/x-elm
ejs = application/x-ejs
+epp = application/x-epp
erb = application/x-erb
erl = text/x-erlang
es6 = text/jsx
diff --git a/tools/bzl/pkg_war.bzl b/tools/bzl/pkg_war.bzl
index 4792de2..52fa1dd 100644
--- a/tools/bzl/pkg_war.bzl
+++ b/tools/bzl/pkg_war.bzl
@@ -14,7 +14,8 @@
# War packaging.
-load("//tools:deps.bzl", "AUTO_FACTORY_VERSION", "AUTO_VALUE_GSON_VERSION", "AUTO_VALUE_VERSION")
+load("//tools:deps.bzl", "AUTO_VALUE_GSON_VERSION")
+load("//tools:nongoogle.bzl", "AUTO_FACTORY_VERSION", "AUTO_VALUE_VERSION")
jar_filetype = [".jar"]
diff --git a/tools/deps.bzl b/tools/deps.bzl
index d056483..bc37010 100644
--- a/tools/deps.bzl
+++ b/tools/deps.bzl
@@ -11,9 +11,6 @@
MAIL_VERS = "1.6.0"
MIME4J_VERS = "0.8.1"
OW2_VERS = "9.2"
-AUTO_COMMON_VERSION = "1.2.1"
-AUTO_FACTORY_VERSION = "1.0.1"
-AUTO_VALUE_VERSION = "1.10.4"
AUTO_VALUE_GSON_VERSION = "1.3.1"
PROLOG_VERS = "1.4.4"
PROLOG_REPO = GERRIT
@@ -90,12 +87,6 @@
)
maven_jar(
- name = "gson",
- artifact = "com.google.code.gson:gson:2.9.0",
- sha1 = "8a1167e089096758b49f9b34066ef98b2f4b37aa",
- )
-
- maven_jar(
name = "caffeine",
artifact = "com.github.ben-manes.caffeine:caffeine:" + CAFFEINE_VERS,
sha1 = "0a17ed335e0ce2d337750772c0709b79af35a842",
@@ -283,36 +274,6 @@
)
maven_jar(
- name = "auto-common",
- artifact = "com.google.auto:auto-common:" + AUTO_COMMON_VERSION,
- sha1 = "f6da26895f759010f5f170c8044e84c1b17ef83e",
- )
-
- maven_jar(
- name = "auto-factory",
- artifact = "com.google.auto.factory:auto-factory:" + AUTO_FACTORY_VERSION,
- sha1 = "f81ece06b6525085da217cd900116f44caafe877",
- )
-
- maven_jar(
- name = "auto-service-annotations",
- artifact = "com.google.auto.service:auto-service-annotations:" + AUTO_FACTORY_VERSION,
- sha1 = "ac86dacc0eb9285ea9d42eee6aad8629ca3a7432",
- )
-
- maven_jar(
- name = "auto-value",
- artifact = "com.google.auto.value:auto-value:" + AUTO_VALUE_VERSION,
- sha1 = "90f9629eaa123f88551cc26a64bc386967ee24cc",
- )
-
- maven_jar(
- name = "auto-value-annotations",
- artifact = "com.google.auto.value:auto-value-annotations:" + AUTO_VALUE_VERSION,
- sha1 = "9679de8286eb0a151db6538ba297a8951c4a1224",
- )
-
- maven_jar(
name = "auto-value-gson-runtime",
artifact = "com.ryanharter.auto.value:auto-value-gson-runtime:" + AUTO_VALUE_GSON_VERSION,
sha1 = "addda2ae6cce9f855788274df5de55dde4de7b71",
diff --git a/tools/nongoogle.bzl b/tools/nongoogle.bzl
index ac3f668..e185141 100644
--- a/tools/nongoogle.bzl
+++ b/tools/nongoogle.bzl
@@ -7,6 +7,12 @@
load("@bazel_tools//tools/build_defs/repo:utils.bzl", "maybe")
load("//tools/bzl:maven_jar.bzl", "maven_jar")
+AUTO_COMMON_VERSION = "1.2.1"
+
+AUTO_FACTORY_VERSION = "1.0.1"
+
+AUTO_VALUE_VERSION = "1.10.4"
+
GUAVA_VERSION = "33.0.0-jre"
GUAVA_BIN_SHA1 = "161ba27964a62f241533807a46b8711b13c1d94b"
@@ -182,6 +188,36 @@
# no concern about version skew.
maven_jar(
+ name = "auto-common",
+ artifact = "com.google.auto:auto-common:" + AUTO_COMMON_VERSION,
+ sha1 = "f6da26895f759010f5f170c8044e84c1b17ef83e",
+ )
+
+ maven_jar(
+ name = "auto-factory",
+ artifact = "com.google.auto.factory:auto-factory:" + AUTO_FACTORY_VERSION,
+ sha1 = "f81ece06b6525085da217cd900116f44caafe877",
+ )
+
+ maven_jar(
+ name = "auto-service-annotations",
+ artifact = "com.google.auto.service:auto-service-annotations:" + AUTO_FACTORY_VERSION,
+ sha1 = "ac86dacc0eb9285ea9d42eee6aad8629ca3a7432",
+ )
+
+ maven_jar(
+ name = "auto-value",
+ artifact = "com.google.auto.value:auto-value:" + AUTO_VALUE_VERSION,
+ sha1 = "90f9629eaa123f88551cc26a64bc386967ee24cc",
+ )
+
+ maven_jar(
+ name = "auto-value-annotations",
+ artifact = "com.google.auto.value:auto-value-annotations:" + AUTO_VALUE_VERSION,
+ sha1 = "9679de8286eb0a151db6538ba297a8951c4a1224",
+ )
+
+ maven_jar(
name = "error-prone-annotations",
artifact = "com.google.errorprone:error_prone_annotations:2.22.0",
sha1 = "bfb9e4281a4cea34f0ec85b3acd47621cfab35b4",
@@ -252,6 +288,12 @@
sha1 = "6e9ccb00926325c7a9293ed05a2eaf56ea15d60e",
)
+ maven_jar(
+ name = "gson",
+ artifact = "com.google.code.gson:gson:2.10.1",
+ sha1 = "b3add478d4382b78ea20b1671390a858002feb6c",
+ )
+
# Test-only dependencies below.
maven_jar(
name = "cglib-3_2",
