Merge "Remove obsolete methods and props from KeyboardShortcutMixinInterface"
diff --git a/java/com/google/gerrit/server/notedb/CommitRewriter.java b/java/com/google/gerrit/server/notedb/CommitRewriter.java
index 7d743dc..7f13731 100644
--- a/java/com/google/gerrit/server/notedb/CommitRewriter.java
+++ b/java/com/google/gerrit/server/notedb/CommitRewriter.java
@@ -58,7 +58,6 @@
import java.util.HashSet;
import java.util.List;
import java.util.Map;
-import java.util.Map.Entry;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
@@ -386,7 +385,7 @@
RevCommit originalCommit;
boolean rewriteStarted = false;
- ChangeFixProgress changeFixProgress = new ChangeFixProgress();
+ ChangeFixProgress changeFixProgress = new ChangeFixProgress(ref.getName());
while ((originalCommit = revWalk.next()) != null) {
changeFixProgress.updateAuthorId =
@@ -538,7 +537,9 @@
return Optional.of(
"Assignee deleted: "
+ getPossibleAccountReplacement(
- changeFixProgress, oldAssignee, assigneeDeletedMatcher.group(1)));
+ changeFixProgress,
+ oldAssignee,
+ ParsedAccountInfo.create(assigneeDeletedMatcher.group(1))));
}
return Optional.empty();
}
@@ -549,7 +550,9 @@
return Optional.of(
"Assignee added: "
+ getPossibleAccountReplacement(
- changeFixProgress, newAssignee, assigneeAddedMatcher.group(1)));
+ changeFixProgress,
+ newAssignee,
+ ParsedAccountInfo.create(assigneeAddedMatcher.group(1))));
}
return Optional.empty();
}
@@ -561,9 +564,13 @@
String.format(
"Assignee changed from: %s to: %s",
getPossibleAccountReplacement(
- changeFixProgress, oldAssignee, assigneeChangedMatcher.group(1)),
+ changeFixProgress,
+ oldAssignee,
+ ParsedAccountInfo.create(assigneeChangedMatcher.group(1))),
getPossibleAccountReplacement(
- changeFixProgress, newAssignee, assigneeChangedMatcher.group(2))));
+ changeFixProgress,
+ newAssignee,
+ ParsedAccountInfo.create(assigneeChangedMatcher.group(2)))));
}
return Optional.empty();
}
@@ -599,7 +606,7 @@
"Removed %s by %s",
matcher.group(1),
getPossibleAccountReplacement(
- changeFixProgress, reviewer, getNameFromNameEmail(matcher.group(2)))));
+ changeFixProgress, reviewer, getAccountInfoFromNameEmail(matcher.group(2)))));
}
return Optional.empty();
}
@@ -612,21 +619,27 @@
}
String[] lines = originalChangeMessage.split("\\r?\\n");
StringBuilder fixedLines = new StringBuilder();
+ boolean anyFixed = false;
for (int i = 1; i < lines.length; i++) {
if (lines[i].isEmpty()) {
continue;
}
Matcher matcher = REMOVED_VOTES_CHANGE_MESSAGE_PATTERN.matcher(lines[i]);
+ String replacementLine = lines[i];
if (matcher.matches() && !NON_REPLACE_ACCOUNT_PATTERN.matcher(matcher.group(2)).matches()) {
- fixedLines.append(
+ anyFixed = true;
+ replacementLine =
String.format(
"* %s by %s\n",
matcher.group(1),
getPossibleAccountReplacement(
- changeFixProgress, Optional.empty(), getNameFromNameEmail(matcher.group(2)))));
+ changeFixProgress,
+ Optional.empty(),
+ getAccountInfoFromNameEmail(matcher.group(2))));
}
+ fixedLines.append(replacementLine);
}
- if (fixedLines.length() == 0) {
+ if (!anyFixed) {
return Optional.empty();
}
return Optional.of(REMOVED_VOTES_CHANGE_MESSAGE_START + "\n" + fixedLines);
@@ -687,7 +700,8 @@
while (onAddReviewerMatcher.find()) {
String reviewerName = normalizeOnCodeOwnerAddReviewerMatch(onAddReviewerMatcher.group(1));
String replacementName =
- getPossibleAccountReplacement(changeFixProgress, Optional.empty(), reviewerName);
+ getPossibleAccountReplacement(
+ changeFixProgress, Optional.empty(), ParsedAccountInfo.create(reviewerName));
onAddReviewerMatcher.appendReplacement(
sb, replacementName + ", who was added as reviewer owns the following files");
}
@@ -971,9 +985,10 @@
private Optional<Account.Id> parseIdent(ChangeFixProgress changeFixProgress, PersonIdent ident) {
Optional<Account.Id> account = NoteDbUtil.parseIdent(ident);
if (account.isPresent()) {
- changeFixProgress.parsedAccounts.putIfAbsent(account.get(), "");
+ changeFixProgress.parsedAccounts.putIfAbsent(account.get(), Optional.empty());
} else {
- logger.atWarning().log("Failed to parse id %s", ident);
+ logger.atWarning().log(
+ "Fixing ref %s, failed to parse id %s", changeFixProgress.changeMetaRef, ident);
}
return account;
}
@@ -1022,10 +1037,16 @@
return fixIdentResult;
}
- /** Extracts {@link Account#getName} from {@link Account#getNameEmail} */
- private String getNameFromNameEmail(String nameEmail) {
+ /** Extracts {@link ParsedAccountInfo} from {@link Account#getNameEmail} */
+ private ParsedAccountInfo getAccountInfoFromNameEmail(String nameEmail) {
Matcher nameEmailMatcher = NAME_EMAIL_PATTERN.matcher(nameEmail);
- return nameEmailMatcher.matches() ? nameEmailMatcher.group(1) : nameEmail;
+ if (!nameEmailMatcher.matches()) {
+ return ParsedAccountInfo.create(nameEmail);
+ }
+
+ return ParsedAccountInfo.create(
+ nameEmailMatcher.group(1),
+ nameEmailMatcher.group(2).substring(1, nameEmailMatcher.group(2).length() - 1));
}
/**
@@ -1038,39 +1059,73 @@
*
* @param changeFixProgress see {@link ChangeFixProgress}
* @param account account that should be used for replacement, if known
- * @param accountName {@link Account#getName} to replace.
+ * @param accountInfo {@link ParsedAccountInfo} to replace.
* @return replacement for {@code accountName}
*/
private String getPossibleAccountReplacement(
- ChangeFixProgress changeFixProgress, Optional<Account.Id> account, String accountName) {
+ ChangeFixProgress changeFixProgress,
+ Optional<Account.Id> account,
+ ParsedAccountInfo accountInfo) {
if (account.isPresent()) {
return AccountTemplateUtil.getAccountTemplate(account.get());
}
// Retrieve reviewer accounts from cache and try to match by their name.
- Map<Account.Id, AccountState> missingUserNameReviewers =
+ Map<Account.Id, AccountState> missingAccountStateReviewers =
accountCache.get(
changeFixProgress.parsedAccounts.entrySet().stream()
- .filter(entry -> entry.getValue().isEmpty())
+ .filter(entry -> !entry.getValue().isPresent())
.map(Map.Entry::getKey)
.collect(ImmutableSet.toImmutableSet()));
changeFixProgress.parsedAccounts.putAll(
- missingUserNameReviewers.entrySet().stream()
+ missingAccountStateReviewers.entrySet().stream()
.collect(
ImmutableMap.toImmutableMap(
- Map.Entry::getKey, e -> e.getValue().account().getName())));
- Set<Account.Id> possibleReplacements =
- changeFixProgress.parsedAccounts.entrySet().stream()
- .filter(e -> e.getValue().equals(accountName))
- .map(Entry::getKey)
- .collect(ImmutableSet.toImmutableSet());
+ Map.Entry::getKey, e -> Optional.ofNullable(e.getValue()))));
+ Map<Account.Id, AccountState> possibleReplacements = ImmutableMap.of();
+ if (accountInfo.email().isPresent()) {
+ possibleReplacements =
+ changeFixProgress.parsedAccounts.entrySet().stream()
+ .filter(
+ e ->
+ e.getValue().isPresent()
+ && Objects.equals(
+ e.getValue().get().account().preferredEmail(),
+ accountInfo.email().get()))
+ .collect(ImmutableMap.toImmutableMap(Map.Entry::getKey, e -> e.getValue().get()));
+ // Filter further so we match both email & name
+ if (possibleReplacements.size() > 1) {
+ logger.atWarning().log(
+ "Fixing ref %s, multiple accounts found with the same email address, while replacing %s",
+ changeFixProgress.changeMetaRef, accountInfo);
+ possibleReplacements =
+ possibleReplacements.entrySet().stream()
+ .filter(e -> Objects.equals(e.getValue().account().getName(), accountInfo.name()))
+ .collect(ImmutableMap.toImmutableMap(Map.Entry::getKey, Map.Entry::getValue));
+ }
+ }
+ if (possibleReplacements.isEmpty()) {
+ possibleReplacements =
+ changeFixProgress.parsedAccounts.entrySet().stream()
+ .filter(
+ e ->
+ e.getValue().isPresent()
+ && Objects.equals(
+ e.getValue().get().account().getName(), accountInfo.name()))
+ .collect(ImmutableMap.toImmutableMap(Map.Entry::getKey, e -> e.getValue().get()));
+ }
String replacementName = DEFAULT_ACCOUNT_REPLACEMENT;
if (possibleReplacements.isEmpty()) {
- logger.atWarning().log("Could not find reviewer account matching name %s", accountName);
+ logger.atWarning().log(
+ "Fixing ref %s, could not find reviewer account matching name %s",
+ changeFixProgress.changeMetaRef, accountInfo);
} else if (possibleReplacements.size() > 1) {
- logger.atWarning().log("Found multiple reviewer account matching name %s", accountName);
+ logger.atWarning().log(
+ "Fixing ref %s found multiple reviewer account matching name %s",
+ changeFixProgress.changeMetaRef, accountInfo);
} else {
replacementName =
- AccountTemplateUtil.getAccountTemplate(Iterables.getOnlyElement(possibleReplacements));
+ AccountTemplateUtil.getAccountTemplate(
+ Iterables.getOnlyElement(possibleReplacements.keySet()));
}
return replacementName;
}
@@ -1135,6 +1190,10 @@
* recent update.
*/
private static class ChangeFixProgress {
+
+ /** {@link RefNames#changeMetaRef} of the change that is being fixed. */
+ final String changeMetaRef;
+
/** Assignee at current commit update. */
Account.Id assigneeId = null;
@@ -1146,7 +1205,7 @@
* #accountCache} if needed by rewrite. Maps to empty string if was not requested from cache
* yet.
*/
- Map<Account.Id, String> parsedAccounts = new HashMap<>();
+ Map<Account.Id, Optional<AccountState>> parsedAccounts = new HashMap<>();
/** Id of the current commit in rewriter walk. */
ObjectId newTipId = null;
@@ -1160,5 +1219,29 @@
boolean isValidAfterFix = true;
List<CommitDiff> commitDiffs = new ArrayList<>();
+
+ public ChangeFixProgress(String changeMetaRef) {
+ this.changeMetaRef = changeMetaRef;
+ }
+ }
+
+ /**
+ * Account info parsed from {@link Account#getNameEmail}. See {@link
+ * #getAccountInfoFromNameEmail}.
+ */
+ @AutoValue
+ abstract static class ParsedAccountInfo {
+
+ static ParsedAccountInfo create(String fullName, String email) {
+ return new AutoValue_CommitRewriter_ParsedAccountInfo(fullName, Optional.ofNullable(email));
+ }
+
+ static ParsedAccountInfo create(String fullName) {
+ return new AutoValue_CommitRewriter_ParsedAccountInfo(fullName, Optional.empty());
+ }
+
+ abstract String name();
+
+ abstract Optional<String> email();
}
}
diff --git a/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java b/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java
index 26e1881..f316660 100644
--- a/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java
+++ b/javatests/com/google/gerrit/server/notedb/CommitRewriterTest.java
@@ -837,6 +837,118 @@
}
@Test
+ public void fixRemoveVoteChangeMessageWithNoFooterLabel_matchByEmail() throws Exception {
+ Change c = newChange();
+ ChangeUpdate approvalUpdate = newUpdate(c, changeOwner);
+ approvalUpdate.putApproval(VERIFIED, (short) +2);
+
+ approvalUpdate.putApprovalFor(otherUserId, VERIFIED, (short) -1);
+ approvalUpdate.commit();
+ writeUpdate(
+ RefNames.changeMetaRef(c.getId()),
+ getChangeUpdateBody(
+ c, /*changeMessage=*/ "Removed Verified+2 by Renamed Change Owner <change@owner.com>"),
+ getAuthorIdent(changeOwner.getAccount()));
+
+ RunOptions options = new RunOptions();
+ options.dryRun = false;
+ BackfillResult result = rewriter.backfillProject(project, repo, options);
+ assertThat(result.fixedRefDiff.keySet()).containsExactly(RefNames.changeMetaRef(c.getId()));
+
+ List<String> commitHistoryDiff = commitHistoryDiff(result, c.getId());
+ assertThat(commitHistoryDiff)
+ .containsExactly(
+ "@@ -6 +6 @@\n"
+ + "-Removed Verified+2 by Renamed Change Owner <change@owner.com>\n"
+ + "+Removed Verified+2 by <GERRIT_ACCOUNT_1>\n");
+ BackfillResult secondRunResult = rewriter.backfillProject(project, repo, options);
+ assertThat(secondRunResult.fixedRefDiff.keySet()).isEmpty();
+ assertThat(secondRunResult.refsFailedToFix).isEmpty();
+ }
+
+ @Test
+ public void fixRemoveVoteChangeMessageWithNoFooterLabel_matchByName() throws Exception {
+ Change c = newChange();
+ ChangeUpdate approvalUpdate = newUpdate(c, changeOwner);
+ approvalUpdate.putApproval(VERIFIED, (short) +2);
+
+ approvalUpdate.putApprovalFor(otherUserId, VERIFIED, (short) -1);
+ approvalUpdate.commit();
+ writeUpdate(
+ RefNames.changeMetaRef(c.getId()),
+ getChangeUpdateBody(c, /*changeMessage=*/ "Removed Verified+2 by Change Owner"),
+ getAuthorIdent(changeOwner.getAccount()));
+
+ RunOptions options = new RunOptions();
+ options.dryRun = false;
+ BackfillResult result = rewriter.backfillProject(project, repo, options);
+ assertThat(result.fixedRefDiff.keySet()).containsExactly(RefNames.changeMetaRef(c.getId()));
+
+ List<String> commitHistoryDiff = commitHistoryDiff(result, c.getId());
+ assertThat(commitHistoryDiff)
+ .containsExactly(
+ "@@ -6 +6 @@\n"
+ + "-Removed Verified+2 by Change Owner\n"
+ + "+Removed Verified+2 by <GERRIT_ACCOUNT_1>\n");
+ BackfillResult secondRunResult = rewriter.backfillProject(project, repo, options);
+ assertThat(secondRunResult.fixedRefDiff.keySet()).isEmpty();
+ assertThat(secondRunResult.refsFailedToFix).isEmpty();
+ }
+
+ @Test
+ public void fixRemoveVoteChangeMessageWithNoFooterLabel_matchDuplicateAccounts()
+ throws Exception {
+ Account duplicateCodeOwner =
+ Account.builder(Account.id(4), TimeUtil.nowTs())
+ .setFullName(changeOwner.getName())
+ .setPreferredEmail("other@test.com")
+ .build();
+ accountCache.put(duplicateCodeOwner);
+ Change c = newChange();
+ ChangeUpdate approvalUpdate = newUpdate(c, changeOwner);
+ approvalUpdate.putApproval(VERIFIED, (short) +2);
+
+ approvalUpdate.putApprovalFor(duplicateCodeOwner.id(), VERIFIED, (short) -1);
+ approvalUpdate.commit();
+ writeUpdate(
+ RefNames.changeMetaRef(c.getId()),
+ getChangeUpdateBody(
+ c, /*changeMessage=*/ "Removed Verified+2 by Change Owner <other@test.com>"),
+ getAuthorIdent(changeOwner.getAccount()));
+ writeUpdate(
+ RefNames.changeMetaRef(c.getId()),
+ getChangeUpdateBody(
+ c, /*changeMessage=*/ "Removed Verified+2 by Change Owner <change@owner.com>"),
+ getAuthorIdent(changeOwner.getAccount()));
+ writeUpdate(
+ RefNames.changeMetaRef(c.getId()),
+ getChangeUpdateBody(
+ c, /*changeMessage=*/ "Removed Verified-1 by Change Owner <other@test.com>"),
+ getAuthorIdent(changeOwner.getAccount()));
+
+ RunOptions options = new RunOptions();
+ options.dryRun = false;
+ BackfillResult result = rewriter.backfillProject(project, repo, options);
+ assertThat(result.fixedRefDiff.keySet()).containsExactly(RefNames.changeMetaRef(c.getId()));
+
+ List<String> commitHistoryDiff = commitHistoryDiff(result, c.getId());
+ assertThat(commitHistoryDiff)
+ .containsExactly(
+ "@@ -6 +6 @@\n"
+ + "-Removed Verified+2 by Change Owner <other@test.com>\n"
+ + "+Removed Verified+2 by <GERRIT_ACCOUNT_4>\n",
+ "@@ -6 +6 @@\n"
+ + "-Removed Verified+2 by Change Owner <change@owner.com>\n"
+ + "+Removed Verified+2 by <GERRIT_ACCOUNT_1>\n",
+ "@@ -6 +6 @@\n"
+ + "-Removed Verified-1 by Change Owner <other@test.com>\n"
+ + "+Removed Verified-1 by <GERRIT_ACCOUNT_4>\n");
+ BackfillResult secondRunResult = rewriter.backfillProject(project, repo, options);
+ assertThat(secondRunResult.fixedRefDiff.keySet()).isEmpty();
+ assertThat(secondRunResult.refsFailedToFix).isEmpty();
+ }
+
+ @Test
public void fixRemoveVotesChangeMessage() throws Exception {
Change c = newChange();
ChangeUpdate approvalUpdate = newUpdate(c, changeOwner);
diff --git a/plugins/plugin-manager b/plugins/plugin-manager
index 5b87f63..ea992c3 160000
--- a/plugins/plugin-manager
+++ b/plugins/plugin-manager
@@ -1 +1 @@
-Subproject commit 5b87f63f3e9c5817bcddf008c0b4005494059368
+Subproject commit ea992c3b37eed5493c7031ee20faba9dd875170f
diff --git a/polygerrit-ui/app/elements/change-list/gr-user-header/gr-user-header.ts b/polygerrit-ui/app/elements/change-list/gr-user-header/gr-user-header.ts
index e87b4f9..05b5390 100644
--- a/polygerrit-ui/app/elements/change-list/gr-user-header/gr-user-header.ts
+++ b/polygerrit-ui/app/elements/change-list/gr-user-header/gr-user-header.ts
@@ -85,7 +85,7 @@
<div>
<span>Joined:</span>
<gr-date-formatter
- date-str="${this._computeDetail(
+ dateStr="${this._computeDetail(
this._accountDetails,
'registered_on'
)}"
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
index b8f01de..40a2e1f 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.ts
@@ -227,9 +227,6 @@
@property({type: Object, notify: true, observer: '_updateDiffPreferences'})
diffPrefs?: DiffPreferencesInfo;
- @property({type: Boolean})
- _showInlineDiffs?: boolean;
-
@property({type: Number, notify: true})
numFilesShown: number = DEFAULT_NUM_FILES_SHOWN;
@@ -629,8 +626,6 @@
}
expandAllDiffs() {
- this._showInlineDiffs = true;
-
// Find the list of paths that are in the file list, but not in the
// expanded list.
const newFiles: PatchSetFile[] = [];
@@ -646,7 +641,6 @@
}
collapseAllDiffs() {
- this._showInlineDiffs = false;
this._expandedFiles = [];
this.filesExpanded = this._computeExpandedFiles(
this._expandedFiles.length,
@@ -950,7 +944,7 @@
return;
}
- if (this._showInlineDiffs) {
+ if (this.filesExpanded === FilesExpandedState.ALL) {
e.preventDefault();
this.diffCursor.moveDown();
this._displayLine = true;
@@ -970,7 +964,7 @@
return;
}
- if (this._showInlineDiffs) {
+ if (this.filesExpanded === FilesExpandedState.ALL) {
e.preventDefault();
this.diffCursor.moveUp();
this._displayLine = true;
@@ -1020,7 +1014,7 @@
}
e.preventDefault();
- if (this._showInlineDiffs) {
+ if (this.filesExpanded === FilesExpandedState.ALL) {
this._openCursorFile();
return;
}
@@ -1086,7 +1080,7 @@
}
_toggleInlineDiffs() {
- if (this._showInlineDiffs) {
+ if (this.filesExpanded === FilesExpandedState.ALL) {
this.collapseAllDiffs();
} else {
this.expandAllDiffs();
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js
index 4acf245..b8c6cde 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.js
@@ -593,7 +593,8 @@
for (const diff of element.diffs) {
assert.isTrue(element._expandedFiles.some(f => f.path === diff.path));
}
-
+ // since _expandedFilesChanged is stubbed
+ element.filesExpanded = FilesExpandedState.ALL;
MockInteractions.keyUpOn(element, 73, 'shift', 'i');
flush();
assert.equal(element.diffs.length, 0);
@@ -653,17 +654,17 @@
});
test('open from selected file', () => {
- element._showInlineDiffs = false;
+ element.filesExpanded = FilesExpandedState.NONE;
assert.deepEqual(interact(), {opened_selected: true});
});
test('open from diff cursor', () => {
- element._showInlineDiffs = true;
+ element.filesExpanded = FilesExpandedState.ALL;
assert.deepEqual(interact(), {opened_cursor: true});
});
test('expand when user prefers', () => {
- element._showInlineDiffs = false;
+ element.filesExpanded = FilesExpandedState.NONE;
assert.deepEqual(interact(), {opened_selected: true});
element._userPrefs = {};
assert.deepEqual(interact(), {opened_selected: true});
@@ -929,14 +930,14 @@
element._filesByPath = {[path]: {}};
element.expandAllDiffs();
flush();
- assert.isTrue(element._showInlineDiffs);
+ assert.equal(element.filesExpanded, FilesExpandedState.ALL);
assert.isTrue(reInitStub.calledOnce);
assert.equal(collapseStub.lastCall.args[0].length, 0);
element.collapseAllDiffs();
flush();
assert.equal(element._expandedFiles.length, 0);
- assert.isFalse(element._showInlineDiffs);
+ assert.equal(element.filesExpanded, FilesExpandedState.NONE);
assert.isTrue(cursorUpdateStub.calledOnce);
assert.equal(collapseStub.lastCall.args[0].length, 1);
});
@@ -1699,7 +1700,7 @@
// This is also called in diffCursor.moveToFirstChunk.
assert.equal(nextChunkStub.callCount, 1);
- assert.isTrue(element._showInlineDiffs);
+ assert.equal(element.filesExpanded, FilesExpandedState.ALL);
});
test('n key without all files expanded and no shift key', async () => {
@@ -1712,7 +1713,7 @@
// This is also called in diffCursor.moveToFirstChunk.
assert.equal(nextChunkStub.callCount, 0);
- assert.isTrue(element._showInlineDiffs);
+ assert.equal(element.filesExpanded, FilesExpandedState.ALL);
});
});
@@ -1736,7 +1737,7 @@
.callsFake(() => false);
sinon.stub(element, 'modifierPressed')
.callsFake(() => false);
- element._showInlineDiffs = true;
+ element.filesExpanded = FilesExpandedState.ALL;
const mockEvent = {preventDefault() {}};
element._displayLine = false;
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
index 463163c..4641897 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.ts
@@ -223,9 +223,6 @@
@property({type: Boolean})
_loggedIn = false;
- @property({type: Boolean})
- disableTokenHighlighting = false;
-
@property({type: String})
_errorMessage: string | null = null;
@@ -303,11 +300,6 @@
this.addEventListener('diff-context-expanded', event =>
this._handleDiffContextExpanded(event)
);
- appContext.restApiService.getPreferences().then(prefs => {
- if (prefs?.disable_token_highlighting) {
- this.disableTokenHighlighting = prefs.disable_token_highlighting;
- }
- });
}
override ready() {
@@ -335,18 +327,21 @@
super.disconnectedCallback();
}
- initLayers() {
- return getPluginLoader()
- .awaitPluginsLoaded()
- .then(() => {
- assertIsDefined(this.path, 'path');
- this._layers = this._getLayers(this.path);
- this._coverageRanges = [];
- // We kick off fetching the data here, but we don't return the promise,
- // so awaiting initLayers() will not wait for coverage data to be
- // completely loaded.
- this._getCoverageData();
- });
+ async initLayers() {
+ const preferencesPromise = appContext.restApiService.getPreferences();
+ await getPluginLoader().awaitPluginsLoaded();
+ const prefs = await preferencesPromise;
+ const enableTokenHighlight =
+ appContext.flagsService.isEnabled(KnownExperimentId.TOKEN_HIGHLIGHTING) &&
+ !prefs?.disable_token_highlighting;
+
+ assertIsDefined(this.path, 'path');
+ this._layers = this.getLayers(this.path, enableTokenHighlight);
+ this._coverageRanges = [];
+ // We kick off fetching the data here, but we don't return the promise,
+ // so awaiting initLayers() will not wait for coverage data to be
+ // completely loaded.
+ this._getCoverageData();
}
diffChanged(diff?: DiffInfo) {
@@ -418,12 +413,9 @@
}
}
- private _getLayers(path: string): DiffLayer[] {
+ private getLayers(path: string, enableTokenHighlight: boolean): DiffLayer[] {
const layers = [];
- if (
- appContext.flagsService.isEnabled(KnownExperimentId.TOKEN_HIGHLIGHTING) &&
- !this.disableTokenHighlighting
- ) {
+ if (enableTokenHighlight) {
layers.push(new TokenHighlightLayer(this));
}
layers.push(this.syntaxLayer);
@@ -740,7 +732,9 @@
_threadsChanged(threads: CommentThread[]) {
const threadEls = new Set<Object>();
for (const thread of threads) {
- threadEls.add(this._getOrCreateThread(thread));
+ const threadEl = this._createThreadElement(thread);
+ this._attachThreadElement(threadEl);
+ threadEls.add(threadEl);
}
// Remove all threads that are no longer existing.
for (const threadEl of this.getThreadEls()) {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
index 344f9d8..ed3ffe0 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host_test.js
@@ -17,19 +17,16 @@
import '../../../test/common-test-setup-karma.js';
import './gr-diff-host.js';
-import {GrDiffBuilderImage} from '../gr-diff-builder/gr-diff-builder-image.js';
-import {GerritNav} from '../../core/gr-navigation/gr-navigation.js';
+
import {dom} from '@polymer/polymer/lib/legacy/polymer.dom.js';
-import {Side, createDefaultDiffPrefs} from '../../../constants/constants.js';
-import {createChange} from '../../../test/test-data-generators.js';
-import {CoverageType} from '../../../types/types.js';
-import {
- addListenerForTest,
- mockPromise,
- stubRestApi,
-} from '../../../test/test-utils.js';
-import {EditPatchSetNum, ParentPatchSetNum} from '../../../types/common.js';
+import {createDefaultDiffPrefs, Side} from '../../../constants/constants.js';
import {_testOnly_resetState} from '../../../services/comments/comments-model.js';
+import {createChange, createComment, createCommentThread} from '../../../test/test-data-generators.js';
+import {addListenerForTest, mockPromise, stubRestApi} from '../../../test/test-utils.js';
+import {EditPatchSetNum, ParentPatchSetNum} from '../../../types/common.js';
+import {CoverageType} from '../../../types/types.js';
+import {GerritNav} from '../../core/gr-navigation/gr-navigation.js';
+import {GrDiffBuilderImage} from '../gr-diff-builder/gr-diff-builder-image.js';
const basicFixture = fixtureFromElement('gr-diff-host');
@@ -1127,6 +1124,35 @@
assert.equal(threads[0].path, element.file.path);
});
+ test('multiple threads created on the same range', () => {
+ element.patchRange = {
+ basePatchNum: 2,
+ patchNum: 3,
+ };
+ element.file = {basePath: 'file_renamed.txt', path: element.path};
+
+ const comment = createComment();
+ comment.range = {
+ start_line: 1,
+ start_character: 1,
+ end_line: 2,
+ end_character: 2,
+ };
+ const thread = createCommentThread([comment]);
+ element.threads = [thread];
+
+ let threads = dom(element.$.diff)
+ .queryDistributedElements('gr-comment-thread');
+
+ assert.equal(threads.length, 1);
+
+ element.threads= [...element.threads, thread];
+
+ threads = dom(element.$.diff)
+ .queryDistributedElements('gr-comment-thread');
+ assert.equal(threads.length, 2);
+ });
+
test('thread should use new file path if first created' +
'on patch set (left) but is base', () => {
const diffSide = Side.LEFT;
@@ -1143,8 +1169,8 @@
},
}));
- const threads = dom(element.$.diff)
- .queryDistributedElements('gr-comment-thread');
+ const threads =
+ dom(element.$.diff).queryDistributedElements('gr-comment-thread');
assert.equal(threads.length, 1);
assert.equal(threads[0].diffSide, diffSide);
@@ -1167,8 +1193,8 @@
},
}));
- const threads = dom(element.$.diff)
- .queryDistributedElements('gr-comment-thread');
+ const threads =
+ dom(element.$.diff).queryDistributedElements('gr-comment-thread');
assert.equal(threads.length, 0);
assert.isTrue(alertSpy.called);
});
diff --git a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.ts b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.ts
index 3b026b3..8821725 100644
--- a/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.ts
+++ b/polygerrit-ui/app/elements/diff/gr-selection-action-box/gr-selection-action-box.ts
@@ -15,6 +15,7 @@
* limitations under the License.
*/
import '../../../styles/shared-styles';
+import '../../shared/gr-tooltip/gr-tooltip';
import {GrTooltip} from '../../shared/gr-tooltip/gr-tooltip';
import {customElement, property} from '@polymer/decorators';
import {flush} from '@polymer/polymer/lib/legacy/polymer.dom';
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
index b37613a..fe4a66a 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread.ts
@@ -57,7 +57,7 @@
import {GrButton} from '../gr-button/gr-button';
import {KnownExperimentId} from '../../../services/flags/flags';
import {DiffInfo, DiffPreferencesInfo} from '../../../types/diff';
-import {RenderPreferences} from '../../../api/diff';
+import {DiffLayer, RenderPreferences} from '../../../api/diff';
import {
check,
assertIsDefined,
@@ -205,8 +205,8 @@
@property({type: Object})
_selfAccount?: AccountDetailInfo;
- @property({type: Boolean})
- disableTokenHighlighting = false;
+ @property({type: Array})
+ layers: DiffLayer[] = [];
get keyBindings() {
return {
@@ -232,9 +232,7 @@
this._handleCommentUpdate(e as CustomEvent)
);
appContext.restApiService.getPreferences().then(prefs => {
- if (prefs?.disable_token_highlighting) {
- this.disableTokenHighlighting = prefs.disable_token_highlighting;
- }
+ this._initLayers(!!prefs?.disable_token_highlighting);
});
}
@@ -366,17 +364,14 @@
return undefined;
}
- _getLayers(diff?: DiffInfo) {
- if (!diff) return [];
- const layers = [];
+ _initLayers(disableTokenHighlighting: boolean) {
if (
this.flagsService.isEnabled(KnownExperimentId.TOKEN_HIGHLIGHTING) &&
- !this.disableTokenHighlighting
+ !disableTokenHighlighting
) {
- layers.push(new TokenHighlightLayer(this));
+ this.layers.push(new TokenHighlightLayer(this));
}
- layers.push(this.syntaxLayer);
- return layers;
+ this.layers.push(this.syntaxLayer);
}
_getUrlForViewDiff(
diff --git a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_html.ts b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_html.ts
index 0752a82..245f71c 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_html.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment-thread/gr-comment-thread_html.ts
@@ -232,7 +232,7 @@
id="diff"
change-num="[[changeNum]]"
diff="[[_diff]]"
- layers="[[_getLayers(_diff)]]"
+ layers="[[layers]]"
path="[[path]]"
prefs="[[_prefs]]"
render-prefs="[[_renderPrefs]]"
