Merge "PostReviewOp: Sort votes in change messages"
diff --git a/java/com/google/gerrit/entities/LabelFunction.java b/java/com/google/gerrit/entities/LabelFunction.java
index f361741..d49ab0f 100644
--- a/java/com/google/gerrit/entities/LabelFunction.java
+++ b/java/com/google/gerrit/entities/LabelFunction.java
@@ -14,6 +14,7 @@
package com.google.gerrit.entities;
+import com.google.common.collect.ImmutableSet;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.SubmitRecord.Label;
import java.util.Collections;
@@ -48,6 +49,16 @@
ALL = Collections.unmodifiableMap(all);
}
+ public static final Map<String, LabelFunction> ALL_NON_DEPRECATED;
+
+ static {
+ Map<String, LabelFunction> allNonDeprecated = new LinkedHashMap<>();
+ for (LabelFunction f : ImmutableSet.of(NO_BLOCK, NO_OP, PATCH_SET_LOCK)) {
+ allNonDeprecated.put(f.getFunctionName(), f);
+ }
+ ALL_NON_DEPRECATED = Collections.unmodifiableMap(allNonDeprecated);
+ }
+
public static Optional<LabelFunction> parse(@Nullable String str) {
return Optional.ofNullable(ALL.get(str));
}
diff --git a/java/com/google/gerrit/server/args4j/ObjectIdHandler.java b/java/com/google/gerrit/server/args4j/ObjectIdHandler.java
index aa8a958..3cad7ce 100644
--- a/java/com/google/gerrit/server/args4j/ObjectIdHandler.java
+++ b/java/com/google/gerrit/server/args4j/ObjectIdHandler.java
@@ -16,9 +16,11 @@
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
+import org.eclipse.jgit.errors.InvalidObjectIdException;
import org.eclipse.jgit.lib.ObjectId;
import org.kohsuke.args4j.CmdLineException;
import org.kohsuke.args4j.CmdLineParser;
+import org.kohsuke.args4j.NamedOptionDef;
import org.kohsuke.args4j.OptionDef;
import org.kohsuke.args4j.spi.OptionHandler;
import org.kohsuke.args4j.spi.Parameters;
@@ -37,7 +39,14 @@
@Override
public int parseArguments(Parameters params) throws CmdLineException {
final String n = params.getParameter(0);
- setter.addValue(ObjectId.fromString(n));
+ try {
+ setter.addValue(ObjectId.fromString(n));
+ } catch (InvalidObjectIdException e) {
+ throw new CmdLineException(
+ owner,
+ String.format("expected SHA1 for option %s: %s", ((NamedOptionDef) option).name(), n),
+ e);
+ }
return 1;
}
diff --git a/java/com/google/gerrit/server/change/GetRelatedChangesUtil.java b/java/com/google/gerrit/server/change/GetRelatedChangesUtil.java
index b1f9726..194a4f0 100644
--- a/java/com/google/gerrit/server/change/GetRelatedChangesUtil.java
+++ b/java/com/google/gerrit/server/change/GetRelatedChangesUtil.java
@@ -72,8 +72,8 @@
}
List<ChangeData> cds =
- InternalChangeQuery.byProjectGroups(
- queryProvider, indexConfig, changeData.project(), groups);
+ InternalChangeQuery.byBranchGroups(
+ queryProvider, indexConfig, changeData.change().getDest(), groups);
if (cds.isEmpty()) {
return Collections.emptyList();
}
diff --git a/java/com/google/gerrit/server/change/LabelNormalizer.java b/java/com/google/gerrit/server/change/LabelNormalizer.java
index 79e2054..fb6b177 100644
--- a/java/com/google/gerrit/server/change/LabelNormalizer.java
+++ b/java/com/google/gerrit/server/change/LabelNormalizer.java
@@ -43,6 +43,16 @@
* what labels are defined for the project. The label definition can change between the time a vote
* is originally made and a later point, for example when a change is submitted. This class
* normalizes old votes against current project configuration.
+ *
+ * <p>Normalizing a vote means making it compliant with the current label definition:
+ *
+ * <ul>
+ * <li>If the voting value is greater than the max allowed value according to the label
+ * definition, the voting value is changed to the max allowed value.
+ * <li>If the voting value is lower than the min allowed value according to the label definition,
+ * the voting value is changed to the min allowed value.
+ * <li>If the label definition for a vote is missing, the vote is deleted.
+ * </ul>
*/
@Singleton
public class LabelNormalizer {
diff --git a/java/com/google/gerrit/server/events/EventFactory.java b/java/com/google/gerrit/server/events/EventFactory.java
index 60e30bc..cd7e29a 100644
--- a/java/com/google/gerrit/server/events/EventFactory.java
+++ b/java/com/google/gerrit/server/events/EventFactory.java
@@ -295,8 +295,8 @@
// Find changes in the same related group as this patch set, having a patch
// set whose parent matches this patch set's revision.
for (ChangeData cd :
- InternalChangeQuery.byProjectGroups(
- queryProvider, indexConfig, change.getProject(), currentPs.groups())) {
+ InternalChangeQuery.byBranchGroups(
+ queryProvider, indexConfig, change.getDest(), currentPs.groups())) {
PATCH_SETS:
for (PatchSet ps : cd.patchSets()) {
RevCommit commit = rw.parseCommit(ps.commitId());
diff --git a/java/com/google/gerrit/server/project/ProjectConfig.java b/java/com/google/gerrit/server/project/ProjectConfig.java
index a964ee1..fc1256e 100644
--- a/java/com/google/gerrit/server/project/ProjectConfig.java
+++ b/java/com/google/gerrit/server/project/ProjectConfig.java
@@ -1143,9 +1143,11 @@
error(
String.format(
"Invalid %s for label \"%s\". Valid names are: %s",
- KEY_FUNCTION, name, Joiner.on(", ").join(LabelFunction.ALL.keySet())));
+ KEY_FUNCTION,
+ name,
+ Joiner.on(", ").join(LabelFunction.ALL_NON_DEPRECATED.keySet())));
}
- label.setFunction(function.orElse(null));
+ function.ifPresent(label::setFunction);
label.setCopyCondition(rc.getString(LABEL, name, KEY_COPY_CONDITION));
if (!values.isEmpty()) {
diff --git a/java/com/google/gerrit/server/project/RefUtil.java b/java/com/google/gerrit/server/project/RefUtil.java
index e86ad41..07f7ba5 100644
--- a/java/com/google/gerrit/server/project/RefUtil.java
+++ b/java/com/google/gerrit/server/project/RefUtil.java
@@ -23,6 +23,7 @@
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import java.io.IOException;
import java.util.Collections;
+import org.eclipse.jgit.errors.AmbiguousObjectException;
import org.eclipse.jgit.errors.IncorrectObjectTypeException;
import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.errors.RevisionSyntaxException;
@@ -49,6 +50,9 @@
} catch (RevisionSyntaxException e) {
throw new UnprocessableEntityException(
String.format("base revision \"%s\" is invalid", baseRevision), e);
+ } catch (AmbiguousObjectException e) {
+ throw new UnprocessableEntityException(
+ String.format("base revision \"%s\" is ambiguous", baseRevision), e);
}
}
diff --git a/java/com/google/gerrit/server/query/change/InternalChangeQuery.java b/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
index 99c1ca1..ccd645b 100644
--- a/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
+++ b/java/com/google/gerrit/server/query/change/InternalChangeQuery.java
@@ -271,21 +271,21 @@
return query(ChangePredicates.submissionId(cs));
}
- private static Predicate<ChangeData> byProjectGroupsPredicate(
- IndexConfig indexConfig, Project.NameKey project, Collection<String> groups) {
- int n = indexConfig.maxTerms() - 1;
+ private static Predicate<ChangeData> byBranchGroupsPredicate(
+ IndexConfig indexConfig, BranchNameKey branchAndProject, Collection<String> groups) {
+ int n = indexConfig.maxTerms() - 2;
checkArgument(groups.size() <= n, "cannot exceed %s groups", n);
List<GroupPredicate> groupPredicates = new ArrayList<>(groups.size());
for (String g : groups) {
groupPredicates.add(new GroupPredicate(g));
}
- return and(project(project), or(groupPredicates));
+ return and(project(branchAndProject.project()), ref(branchAndProject), or(groupPredicates));
}
- public static ImmutableList<ChangeData> byProjectGroups(
+ public static ImmutableList<ChangeData> byBranchGroups(
Provider<InternalChangeQuery> queryProvider,
IndexConfig indexConfig,
- Project.NameKey project,
+ BranchNameKey branchAndProject,
Collection<String> groups) {
// These queries may be complex along multiple dimensions:
// * Many groups per change, if there are very many patch sets. This requires partitioning the
@@ -296,16 +296,17 @@
// InternalChangeQuery is single-use.
Supplier<InternalChangeQuery> querySupplier = () -> queryProvider.get().enforceVisibility(true);
- int batchSize = indexConfig.maxTerms() - 1;
+ int batchSize = indexConfig.maxTerms() - 2;
if (groups.size() <= batchSize) {
return queryExhaustively(
- querySupplier, byProjectGroupsPredicate(indexConfig, project, groups));
+ querySupplier, byBranchGroupsPredicate(indexConfig, branchAndProject, groups));
}
Set<Change.Id> seen = new HashSet<>();
ImmutableList.Builder<ChangeData> result = ImmutableList.builder();
for (List<String> part : Iterables.partition(groups, batchSize)) {
for (ChangeData cd :
- queryExhaustively(querySupplier, byProjectGroupsPredicate(indexConfig, project, part))) {
+ queryExhaustively(
+ querySupplier, byBranchGroupsPredicate(indexConfig, branchAndProject, part))) {
if (!seen.add(cd.getId())) {
result.add(cd);
}
diff --git a/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java b/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java
index 52207db..5fd2159 100644
--- a/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/project/ProjectConfigIT.java
@@ -134,6 +134,27 @@
}
@Test
+ public void rejectCreatingLabelWithInvalidFunction() throws Exception {
+ fetchRefsMetaConfig();
+ PushOneCommit push =
+ pushFactory.create(
+ admin.newIdent(),
+ testRepo,
+ "Test Change",
+ ProjectConfig.PROJECT_CONFIG,
+ "[label \"Foo\"]\n function = INVALID");
+ PushOneCommit.Result r = push.to(RefNames.REFS_CONFIG);
+ r.assertErrorStatus(
+ String.format("commit %s: invalid project configuration", abbreviateName(r.getCommit())));
+ r.assertMessage(
+ String.format(
+ "ERROR: commit %s: invalid project configuration:\n"
+ + "ERROR: commit %s: project.config: Invalid function for label \"foo\"."
+ + " Valid names are: NoBlock, NoOp, PatchSetLock",
+ abbreviateName(r.getCommit()), abbreviateName(r.getCommit())));
+ }
+
+ @Test
public void rejectSettingCopyMinScore() throws Exception {
testRejectSettingLabelFlag(
LabelConfigValidator.KEY_COPY_MIN_SCORE, /* value= */ true, "is:MIN");
diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index 50bb8ae..b738324 100644
--- a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
+++ b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
@@ -3082,6 +3082,12 @@
assertThat(r.getChange().attentionSet()).isEmpty();
}
+ @Test
+ public void pushWithInvalidBaseIsRejected() throws Exception {
+ PushOneCommit.Result r = pushTo("refs/for/master%base=invalid");
+ r.assertErrorStatus("expected SHA1 for option --base: invalid");
+ }
+
private DraftInput newDraft(String path, int line, String message) {
DraftInput d = new DraftInput();
d.path = path;
diff --git a/javatests/com/google/gerrit/acceptance/server/change/GetRelatedIT.java b/javatests/com/google/gerrit/acceptance/server/change/GetRelatedIT.java
index 70b5701..21db45c 100644
--- a/javatests/com/google/gerrit/acceptance/server/change/GetRelatedIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/change/GetRelatedIT.java
@@ -39,11 +39,13 @@
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AccountGroup;
+import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.extensions.api.changes.GetRelatedOption;
import com.google.gerrit.extensions.api.changes.RelatedChangeAndCommitInfo;
import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.api.projects.BranchInput;
import com.google.gerrit.extensions.common.CommitInfo;
import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.index.IndexConfig;
@@ -67,6 +69,8 @@
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.revwalk.RevCommit;
+import org.eclipse.jgit.transport.PushResult;
+import org.eclipse.jgit.transport.RemoteRefUpdate;
import org.junit.Test;
@NoHttpd
@@ -664,6 +668,71 @@
}
}
+ @Test
+ public void getRelatedLinearSameCommitPushedTwice() throws Exception {
+ RevCommit base = projectOperations.project(project).getHead("master");
+
+ // 1,1---2,1 on master
+ PushOneCommit.Result r1 =
+ createChange(
+ testRepo,
+ "master",
+ "subject: 1",
+ "a.txt",
+ "1",
+ /** topic= */
+ null);
+ RevCommit c1_1 = r1.getCommit();
+ PatchSet.Id ps1_1 = r1.getPatchSetId();
+
+ PushOneCommit.Result r2 =
+ createChange(
+ testRepo,
+ "master",
+ "subject: 2",
+ "b.txt",
+ "2",
+ /** topic= */
+ null);
+ RevCommit c2_1 = r2.getCommit();
+ PatchSet.Id ps2_1 = r2.getPatchSetId();
+
+ // 3,1---4,1 on stable
+ gApi.projects().name(project.get()).branch("stable").create(new BranchInput());
+ testRepo.reset(c1_1);
+ PushResult r3 = pushHead(testRepo, "refs/for/stable%base=" + base.getName());
+ assertThat(r3.getRemoteUpdate("refs/for/stable%base=" + base.getName()).getStatus())
+ .isEqualTo(RemoteRefUpdate.Status.OK);
+ ChangeData change3 =
+ Iterables.getOnlyElement(
+ queryProvider
+ .get()
+ .byBranchCommit(BranchNameKey.create(project, "stable"), c1_1.getName()));
+ assertThat(change3.currentPatchSet().commitId()).isEqualTo(c1_1);
+ RevCommit c3_1 = c1_1;
+ PatchSet.Id ps3_1 = change3.currentPatchSet().id();
+
+ PushOneCommit.Result r4 =
+ createChange(
+ testRepo,
+ "stable",
+ "subject: 4",
+ "d.txt",
+ "4",
+ /** topic= */
+ null);
+ RevCommit c4_1 = r4.getCommit();
+ PatchSet.Id ps4_1 = r4.getPatchSetId();
+
+ for (PatchSet.Id ps : ImmutableList.of(ps2_1, ps1_1)) {
+ assertRelated(ps, changeAndCommit(ps2_1, c2_1, 1), changeAndCommit(ps1_1, c1_1, 1));
+ }
+
+ for (PatchSet.Id ps : ImmutableList.of(ps4_1, ps3_1)) {
+ assertRelated(ps, changeAndCommit(ps4_1, c4_1, 1), changeAndCommit(ps3_1, c3_1, 1));
+ }
+ }
+
private static Correspondence<RelatedChangeAndCommitInfo, String>
getRelatedChangeToStatusCorrespondence() {
return Correspondence.transforming(
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
index 1d10128..a1021a0 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
@@ -1422,6 +1422,39 @@
await element.reload();
});
+ test('revert change payload', async () => {
+ await element.updateComplete;
+ queryAndAssert<GrButton>(
+ element,
+ 'gr-button[data-action-key="revert"]'
+ ).click();
+ const revertAction = {
+ __key: 'revert',
+ __type: 'change',
+ __primary: false,
+ method: HttpMethod.POST,
+ label: 'Revert',
+ title: 'Revert the change',
+ enabled: true,
+ };
+ queryAndAssert(element, 'gr-confirm-revert-dialog').dispatchEvent(
+ new CustomEvent('confirm', {
+ detail: {
+ message: 'foo message',
+ revertType: 1,
+ },
+ })
+ );
+ assert.deepEqual(fireActionStub.lastCall.args, [
+ '/revert',
+ assertUIActionInfo(revertAction),
+ false,
+ {
+ message: 'foo message',
+ },
+ ]);
+ });
+
test('revert change with plugin hook', async () => {
const newRevertMsg = 'Modified revert msg';
sinon
diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-change.ts b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-change.ts
index 69fd142..a2beee2 100644
--- a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-change.ts
+++ b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-change.ts
@@ -99,7 +99,7 @@
override render() {
const change = this.change;
if (!change) throw new Error('Missing change');
- const linkClass = this._computeLinkClass(change);
+ const linkClass = this.computeLinkClass(change);
return html`
<div class="changeContainer">
<a
@@ -118,16 +118,16 @@
>✓</span
>`
: ''}
- ${this.showChangeStatus && !isChangeInfo(change)
- ? html`<span class=${this._computeChangeStatusClass(change)}>
- (${this._computeChangeStatus(change)})
+ ${this.showChangeStatus
+ ? html`<span class=${this.computeChangeStatusClass(change)}>
+ (${this.computeChangeStatus(change)})
</span>`
: ''}
</div>
`;
}
- _computeLinkClass(change: ChangeInfo | RelatedChangeAndCommitInfo) {
+ private computeLinkClass(change: ChangeInfo | RelatedChangeAndCommitInfo) {
const statuses = [];
if (change.status === ChangeStatus.ABANDONED) {
statuses.push('strikethrough');
@@ -138,11 +138,16 @@
return statuses.join(' ');
}
- _computeChangeStatusClass(change: RelatedChangeAndCommitInfo) {
+ private computeChangeStatusClass(
+ change: RelatedChangeAndCommitInfo | ChangeInfo
+ ) {
const classes = ['status'];
- if (change._revision_number !== change._current_revision_number) {
+ if (
+ !isChangeInfo(change) &&
+ change._revision_number !== change._current_revision_number
+ ) {
classes.push('notCurrent');
- } else if (this._isIndirectAncestor(change)) {
+ } else if (!isChangeInfo(change) && this.isIndirectAncestor(change)) {
classes.push('indirectAncestor');
} else if (change.submittable) {
classes.push('submittable');
@@ -152,16 +157,19 @@
return classes.join(' ');
}
- _computeChangeStatus(change: RelatedChangeAndCommitInfo) {
+ private computeChangeStatus(change: RelatedChangeAndCommitInfo | ChangeInfo) {
switch (change.status) {
case ChangeStatus.MERGED:
return 'Merged';
case ChangeStatus.ABANDONED:
return 'Abandoned';
}
- if (change._revision_number !== change._current_revision_number) {
+ if (
+ !isChangeInfo(change) &&
+ change._revision_number !== change._current_revision_number
+ ) {
return 'Not current';
- } else if (this._isIndirectAncestor(change)) {
+ } else if (!isChangeInfo(change) && this.isIndirectAncestor(change)) {
return 'Indirect ancestor';
} else if (change.submittable) {
return 'Submittable';
@@ -169,7 +177,7 @@
return '';
}
- _isIndirectAncestor(change: RelatedChangeAndCommitInfo) {
+ private isIndirectAncestor(change: RelatedChangeAndCommitInfo) {
return (
this.connectedRevisions &&
!this.connectedRevisions.includes(change.commit.commit)
diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
index bdb95ea..cac9ae5 100644
--- a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list.ts
@@ -418,6 +418,7 @@
)}<gr-related-change
.change=${change}
.href=${createChangeUrl({change, usp: 'cherry-pick'})}
+ show-change-status
>${change.branch}: ${change.subject}</gr-related-change
>
</div>`
diff --git a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.ts b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.ts
index 676a468..3e90145 100644
--- a/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-related-changes-list/gr-related-changes-list_test.ts
@@ -249,7 +249,7 @@
<gr-related-collapse title="Cherry picks">
<div class="relatedChangeLine show-when-collapsed">
<span class="marker space"> </span>
- <gr-related-change>
+ <gr-related-change show-change-status="">
test-branch: Test subject
</gr-related-change>
</div>
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 3ff0780..11406b4 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
@@ -606,13 +606,13 @@
.patchsetLevelContainer.unresolved {
background-color: var(--unresolved-comment-background-color);
}
- .infoMessage {
+ .privateVisiblityInfo {
display: flex;
justify-content: center;
background-color: var(--info-background);
padding: var(--spacing-s) 0;
}
- .infoMessage gr-icon {
+ .privateVisiblityInfo gr-icon {
margin-right: var(--spacing-m);
color: var(--info-foreground);
}
@@ -802,7 +802,6 @@
</gr-endpoint-decorator>
${this.renderCCList()} ${this.renderReviewConfirmation()}
${this.renderPrivateVisiblityInfo()}
- ${this.renderNotificationForWipChangesInfo()}
</section>
<section class="labelsContainer">${this.renderLabels()}</section>
<section class="newReplyDialog textareaContainer">
@@ -912,7 +911,7 @@
];
if (!this.change?.is_private || !addedAccounts.length) return nothing;
return html`
- <div class="infoMessage">
+ <div class="privateVisiblityInfo">
<gr-icon icon="info"></gr-icon>
<div>
Adding a reviewer/CC will make this private change visible to them
@@ -921,20 +920,6 @@
`;
}
- private renderNotificationForWipChangesInfo() {
- const addedAccounts = [
- ...(this.reviewersList?.additions() ?? []),
- ...(this.ccsList?.additions() ?? []),
- ];
- if (!this.change?.work_in_progress || !addedAccounts.length) return nothing;
- return html`
- <div class="infoMessage">
- <gr-icon icon="info"></gr-icon>
- <div>Reviewers will not be notified for WIP changes</div>
- </div>
- `;
- }
-
private renderLabels() {
if (!this.change || !this.account || !this.permittedLabels) return;
return html`
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
index 8582fd8..52ea252 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.ts
@@ -432,7 +432,7 @@
</gr-button>
</div>
</dialog>
- <div class="infoMessage">
+ <div class="privateVisiblityInfo">
<gr-icon icon="info">
</gr-icon>
<div>
@@ -444,123 +444,6 @@
);
});
- test('renders wip change info when reviewer is added', async () => {
- element.change!.work_in_progress = true;
- element.requestUpdate();
- await element.updateComplete;
- const peopleContainer = queryAndAssert<HTMLDivElement>(
- element,
- '.peopleContainer'
- );
-
- // Info is rendered only if reviewer is added
- assert.dom.equal(
- peopleContainer,
- `
- <section class="peopleContainer">
- <gr-endpoint-decorator name="reply-reviewers">
- <gr-endpoint-param name="change"> </gr-endpoint-param>
- <gr-endpoint-param name="reviewers"> </gr-endpoint-param>
- <div class="peopleList">
- <div class="peopleListLabel">Reviewers</div>
- <gr-account-list id="reviewers"> </gr-account-list>
- <gr-endpoint-slot name="right"> </gr-endpoint-slot>
- </div>
- <gr-endpoint-slot name="below"> </gr-endpoint-slot>
- </gr-endpoint-decorator>
- <div class="peopleList">
- <div class="peopleListLabel">CC</div>
- <gr-account-list allow-any-input="" id="ccs"> </gr-account-list>
- </div>
- <dialog
- tabindex="-1"
- id="reviewerConfirmationModal"
- >
- <div class="reviewerConfirmation">
- Group
- <span class="groupName"> </span>
- has
- <span class="groupSize"> </span>
- members.
- <br />
- Are you sure you want to add them all?
- </div>
- <div class="reviewerConfirmationButtons">
- <gr-button aria-disabled="false" role="button" tabindex="0">
- Yes
- </gr-button>
- <gr-button aria-disabled="false" role="button" tabindex="0">
- No
- </gr-button>
- </div>
- </dialog>
- </section>
- `
- );
-
- const account = createAccountWithId(22);
- element.reviewersList!.accounts = [];
- element.reviewersList!.addAccountItem({account, count: 1});
- element.reviewersList!.dispatchEvent(
- new CustomEvent('account-added', {
- detail: {account},
- })
- );
- element.requestUpdate();
- await element.updateComplete;
-
- assert.dom.equal(
- peopleContainer,
- `
- <section class="peopleContainer">
- <gr-endpoint-decorator name="reply-reviewers">
- <gr-endpoint-param name="change"> </gr-endpoint-param>
- <gr-endpoint-param name="reviewers"> </gr-endpoint-param>
- <div class="peopleList">
- <div class="peopleListLabel">Reviewers</div>
- <gr-account-list id="reviewers"> </gr-account-list>
- <gr-endpoint-slot name="right"> </gr-endpoint-slot>
- </div>
- <gr-endpoint-slot name="below"> </gr-endpoint-slot>
- </gr-endpoint-decorator>
- <div class="peopleList">
- <div class="peopleListLabel">CC</div>
- <gr-account-list allow-any-input="" id="ccs"> </gr-account-list>
- </div>
- <dialog
- tabindex="-1"
- id="reviewerConfirmationModal"
- >
- <div class="reviewerConfirmation">
- Group
- <span class="groupName"> </span>
- has
- <span class="groupSize"> </span>
- members.
- <br />
- Are you sure you want to add them all?
- </div>
- <div class="reviewerConfirmationButtons">
- <gr-button aria-disabled="false" role="button" tabindex="0">
- Yes
- </gr-button>
- <gr-button aria-disabled="false" role="button" tabindex="0">
- No
- </gr-button>
- </div>
- </dialog>
- <div class="infoMessage">
- <gr-icon icon="info">
- </gr-icon>
- <div>
- Reviewers will not be notified for WIP changes
- </div>
- </div>
- </section>
- `
- );
- });
-
test('default to publishing draft comments with reply', async () => {
// Async tick is needed because iron-selector content is distributed and
// distributed content requires an observer to be set up.
diff --git a/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown.ts b/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown.ts
index 4005bb3..140b752 100644
--- a/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown.ts
+++ b/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown.ts
@@ -165,12 +165,7 @@
override render() {
return html`
- <div
- class="dropdown-content"
- slot="dropdown-content"
- id="suggestions"
- role="listbox"
- >
+ <div class="dropdown-content" id="suggestions" role="listbox">
<ul>
${repeat(
this.suggestions,
@@ -209,7 +204,7 @@
}
setPositionTarget(target: HTMLElement) {
- this.fitController?.setPositionTarget(target);
+ this.fitController.setPositionTarget(target);
}
private handleUp() {
@@ -300,7 +295,7 @@
} else {
this.cursor.stops = [];
}
- this.fitController?.refit();
+ this.fitController.refit();
}
private setIndex() {
diff --git a/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown_test.ts b/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown_test.ts
index 641dd2d..cb83396 100644
--- a/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-autocomplete-dropdown/gr-autocomplete-dropdown_test.ts
@@ -42,12 +42,7 @@
assert.shadowDom.equal(
element,
/* HTML */ `
- <div
- class="dropdown-content"
- id="suggestions"
- role="listbox"
- slot="dropdown-content"
- >
+ <div class="dropdown-content" id="suggestions" role="listbox">
<ul>
<li
aria-label="test name 1"
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
index 096c28e..804d101 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element.ts
@@ -523,7 +523,7 @@
// If endIndex isn't present, continue to the end of the line.
const endIndex =
highlight.endIndex === undefined
- ? line.text.length
+ ? GrAnnotation.getStringLength(line.text)
: highlight.endIndex;
GrAnnotation.annotateElement(
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts
index 5eabc59..0f02d71 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-builder/gr-diff-builder-element_test.ts
@@ -271,10 +271,11 @@
const str0 = slice(str, 0, 6);
const str1 = slice(str, 6);
+ const numHighlightedChars = GrAnnotation.getStringLength(str1);
layer.annotate(el, lineNumberEl, l, Side.LEFT);
- assert.isTrue(annotateElementSpy.called);
+ assert.isTrue(annotateElementSpy.calledWith(el, 6, numHighlightedChars));
assert.equal(el.childNodes.length, 2);
assert.instanceOf(el.childNodes[0], Text);
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation.ts b/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation.ts
index cc7cd49..92175eb 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation.ts
@@ -22,8 +22,14 @@
return this.getStringLength(node.textContent || '');
},
+ /**
+ * Returns the number of Unicode code points in the given string
+ *
+ * This is not necessarily the same as the number of visible symbols.
+ * See https://mathiasbynens.be/notes/javascript-unicode for more details.
+ */
getStringLength(str: string) {
- return str.replace(REGEX_ASTRAL_SYMBOL, '_').length;
+ return [...str].length;
},
/**
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation_test.ts
index 6c45f20..fdf1785 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-highlight/gr-annotation_test.ts
@@ -327,4 +327,20 @@
assert.equal(el.getAttribute('class'), 'hello world');
});
});
+
+ suite('getStringLength', () => {
+ test('ASCII characters are counted correctly', () => {
+ assert.equal(GrAnnotation.getStringLength('ASCII'), 5);
+ });
+
+ test('Unicode surrogate pairs count as one symbol', () => {
+ assert.equal(GrAnnotation.getStringLength('Unic💢de'), 7);
+ assert.equal(GrAnnotation.getStringLength('💢💢'), 2);
+ });
+
+ test('Grapheme clusters count as multiple symbols', () => {
+ assert.equal(GrAnnotation.getStringLength('man\u0303ana'), 7); // mañana
+ assert.equal(GrAnnotation.getStringLength('q\u0307\u0323'), 3); // q̣̇
+ });
+ });
});
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts
index 9f874b8..22a71a5 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts
@@ -21,6 +21,7 @@
import {debounce, DelayedTask} from '../../../utils/async-util';
import {RenderPreferences} from '../../../api/diff';
import {assertIsDefined} from '../../../utils/common-util';
+import {GrAnnotation} from '../gr-diff-highlight/gr-annotation';
const WHOLE_FILE = -1;
@@ -639,16 +640,19 @@
rows: string[],
intralineInfos: number[][]
): Highlights[] {
+ // +1 to account for the \n that is not part of the rows passed here
+ const lineLengths = rows.map(r => GrAnnotation.getStringLength(r) + 1);
+
let rowIndex = 0;
let idx = 0;
const normalized = [];
for (const [skipLength, markLength] of intralineInfos) {
- let line = rows[rowIndex] + '\n';
+ let lineLength = lineLengths[rowIndex];
let j = 0;
while (j < skipLength) {
- if (idx === line.length) {
+ if (idx === lineLength) {
idx = 0;
- line = rows[++rowIndex] + '\n';
+ lineLength = lineLengths[++rowIndex];
continue;
}
idx++;
@@ -660,10 +664,10 @@
};
j = 0;
- while (line && j < markLength) {
- if (idx === line.length) {
+ while (lineLength && j < markLength) {
+ if (idx === lineLength) {
idx = 0;
- line = rows[++rowIndex] + '\n';
+ lineLength = lineLengths[++rowIndex];
normalized.push(lineHighlight);
lineHighlight = {
contentIndex: rowIndex,
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor_test.ts
index 6caeb62..f6f7052 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor_test.ts
@@ -723,6 +723,25 @@
endIndex: 41,
},
]);
+
+ content = ['🙈 a', '🙉 b', '🙊 c'];
+ highlights = [[2, 7]];
+ results = element.convertIntralineInfos(content, highlights);
+ assert.deepEqual(results, [
+ {
+ contentIndex: 0,
+ startIndex: 2,
+ },
+ {
+ contentIndex: 1,
+ startIndex: 0,
+ },
+ {
+ contentIndex: 2,
+ startIndex: 0,
+ endIndex: 1,
+ },
+ ]);
});
test('scrolling pauses rendering', () => {