Merge "Fix the icons of RUNNING chips"
diff --git a/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java b/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java
index 9f58aaf..63e0b7a 100644
--- a/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java
+++ b/java/com/google/gerrit/server/patch/SubmitWithStickyApprovalDiff.java
@@ -88,8 +88,10 @@
// If the latest approved patchset is the current patchset, no need to return anything.
return "";
}
- String diff =
- String.format("\n\n%d is the latest approved patch-set.\n", latestApprovedPatchsetId.get());
+ StringBuilder diff =
+ new StringBuilder(
+ String.format(
+ "\n\n%d is the latest approved patch-set.\n", latestApprovedPatchsetId.get()));
PatchList patchList =
getPatchList(
notes.getProjectName(),
@@ -103,19 +105,19 @@
.collect(Collectors.toList());
if (patchListEntryList.isEmpty()) {
- diff +=
- "No files were changed between the latest approved patch-set and the submitted one.\n";
- return diff;
+ diff.append(
+ "No files were changed between the latest approved patch-set and the submitted one.\n");
+ return diff.toString();
}
- diff += "The change was submitted with unreviewed changes in the following files:\n\n";
+ diff.append("The change was submitted with unreviewed changes in the following files:\n\n");
for (PatchListEntry patchListEntry : patchListEntryList) {
- diff +=
+ diff.append(
getDiffForFile(
- notes, currentPatchset.id(), latestApprovedPatchsetId, patchListEntry, currentUser);
+ notes, currentPatchset.id(), latestApprovedPatchsetId, patchListEntry, currentUser));
}
- return diff;
+ return diff.toString();
}
private String getDiffForFile(
@@ -126,12 +128,13 @@
CurrentUser currentUser)
throws AuthException, InvalidChangeOperationException, IOException,
PermissionBackendException {
- String diff =
- String.format(
- "The name of the file: %s\nInsertions: %d, Deletions: %d.\n\n",
- patchListEntry.getNewName(),
- patchListEntry.getInsertions(),
- patchListEntry.getDeletions());
+ StringBuilder diff =
+ new StringBuilder(
+ String.format(
+ "The name of the file: %s\nInsertions: %d, Deletions: %d.\n\n",
+ patchListEntry.getNewName(),
+ patchListEntry.getInsertions(),
+ patchListEntry.getDeletions()));
DiffPreferencesInfo diffPreferencesInfo = createDefaultDiffPreferencesInfo();
PatchScriptFactory patchScriptFactory =
patchScriptFactoryFactory.create(
@@ -145,66 +148,66 @@
try {
patchScript = patchScriptFactory.call();
} catch (LargeObjectException exception) {
- diff += "The file content is too large for showing the full diff. \n\n";
- return diff;
+ diff.append("The file content is too large for showing the full diff. \n\n");
+ return diff.toString();
}
if (patchScript.getChangeType() == ChangeType.RENAMED) {
- diff +=
+ diff.append(
String.format(
"The file %s was renamed to %s\n",
- patchListEntry.getOldName(), patchListEntry.getNewName());
+ patchListEntry.getOldName(), patchListEntry.getNewName()));
}
SparseFileContent.Accessor fileA = patchScript.getA().createAccessor();
SparseFileContent.Accessor fileB = patchScript.getB().createAccessor();
boolean editsExist = false;
if (patchScript.getEdits().stream().anyMatch(e -> e.getType() != Edit.Type.EMPTY)) {
- diff += "```\n";
+ diff.append("```\n");
editsExist = true;
}
for (Edit edit : patchScript.getEdits()) {
- diff += getDiffForEdit(fileA, fileB, edit);
+ diff.append(getDiffForEdit(fileA, fileB, edit));
}
if (editsExist) {
- diff += "```\n";
+ diff.append("```\n");
}
- return diff;
+ return diff.toString();
}
private String getDiffForEdit(Accessor fileA, Accessor fileB, Edit edit) {
- String diff = "";
+ StringBuilder diff = new StringBuilder();
Edit.Type type = edit.getType();
switch (type) {
case INSERT:
- diff += String.format("@@ +%d:%d @@\n", edit.getBeginB(), edit.getEndB());
- diff += getModifiedLines(fileB, edit.getBeginB(), edit.getEndB(), '+');
- diff += "\n";
+ diff.append(String.format("@@ +%d:%d @@\n", edit.getBeginB(), edit.getEndB()));
+ diff.append(getModifiedLines(fileB, edit.getBeginB(), edit.getEndB(), '+'));
+ diff.append("\n");
break;
case DELETE:
- diff += String.format("@@ -%d:%d @@\n", edit.getBeginA(), edit.getEndA());
- diff += getModifiedLines(fileA, edit.getBeginA(), edit.getEndA(), '-');
- diff += "\n";
+ diff.append(String.format("@@ -%d:%d @@\n", edit.getBeginA(), edit.getEndA()));
+ diff.append(getModifiedLines(fileA, edit.getBeginA(), edit.getEndA(), '-'));
+ diff.append("\n");
break;
case REPLACE:
- diff +=
+ diff.append(
String.format(
"@@ -%d:%d, +%d:%d @@\n",
- edit.getBeginA(), edit.getEndA(), edit.getBeginB(), edit.getEndB());
- diff += getModifiedLines(fileA, edit.getBeginA(), edit.getEndA(), '-');
- diff += getModifiedLines(fileB, edit.getBeginB(), edit.getEndB(), '+');
- diff += "\n";
+ edit.getBeginA(), edit.getEndA(), edit.getBeginB(), edit.getEndB()));
+ diff.append(getModifiedLines(fileA, edit.getBeginA(), edit.getEndA(), '-'));
+ diff.append(getModifiedLines(fileB, edit.getBeginB(), edit.getEndB(), '+'));
+ diff.append("\n");
break;
case EMPTY:
// do nothing since there is no change here.
}
- return diff;
+ return diff.toString();
}
private String getModifiedLines(Accessor file, int begin, int end, char modificationType) {
- String diff = "";
+ StringBuilder diff = new StringBuilder();
for (int i = begin; i < end; i++) {
- diff += String.format("%c %s\n", modificationType, file.get(i));
+ diff.append(String.format("%c %s\n", modificationType, file.get(i)));
}
- return diff;
+ return diff.toString();
}
private DiffPreferencesInfo createDefaultDiffPreferencesInfo() {
diff --git a/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary.ts b/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary.ts
index beec0a4..538fa4969 100644
--- a/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-summary/gr-change-summary.ts
@@ -22,7 +22,8 @@
import {KnownExperimentId} from '../../../services/flags/flags';
import {
allRuns$,
- aPluginHasRegistered,
+ aPluginHasRegistered$,
+ someProvidersAreLoading$,
} from '../../../services/checks/checks-model';
import {
Category,
@@ -262,13 +263,17 @@
@property()
showChecksSummary = false;
+ @property()
+ someProvidersAreLoading = false;
+
/** Is reset when rendering beings and decreases while chips are rendered. */
private detailsQuota = DETAILS_QUOTA;
constructor() {
super();
this.subscribe('runs', allRuns$);
- this.subscribe('showChecksSummary', aPluginHasRegistered);
+ this.subscribe('showChecksSummary', aPluginHasRegistered$);
+ this.subscribe('someProvidersAreLoading', someProvidersAreLoading$);
}
static get styles() {
@@ -312,7 +317,8 @@
renderChecksZeroState() {
if (this.runs.some(isRunningOrHasCompleted)) return;
- return html`<span class="font-small zeroState">No results</span>`;
+ const msg = this.someProvidersAreLoading ? 'Loading...' : 'No results';
+ return html`<span class="font-small zeroState">${msg}</span>`;
}
renderChecksChipForCategory(category: Category) {
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
index 3d7646f..a7f5ea7 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.ts
@@ -171,7 +171,7 @@
import {fireTitleChange} from '../../../utils/event-util';
import {GerritView} from '../../../services/router/router-model';
import {takeUntil} from 'rxjs/operators';
-import {aPluginHasRegistered} from '../../../services/checks/checks-model';
+import {aPluginHasRegistered$} from '../../../services/checks/checks-model';
import {Subject} from 'rxjs';
import {GrRelatedChangesListExperimental} from '../gr-related-changes-list-experimental/gr-related-changes-list-experimental';
@@ -274,11 +274,13 @@
* @event show-auth-required
*/
- reporting = appContext.reportingService;
+ private readonly reporting = appContext.reportingService;
- flagsService = appContext.flagsService;
+ private readonly flagsService = appContext.flagsService;
- readonly jsAPI = appContext.jsApiService;
+ private readonly jsAPI = appContext.jsApiService;
+
+ private readonly changeService = appContext.changeService;
/**
* URL params passed from the router.
@@ -594,7 +596,7 @@
/** @override */
ready() {
super.ready();
- aPluginHasRegistered.pipe(takeUntil(this.disconnected$)).subscribe(b => {
+ aPluginHasRegistered$.pipe(takeUntil(this.disconnected$)).subscribe(b => {
this._showChecksTab = b;
});
this._isNewChangeSummaryUiEnabled = this.flagsService.isEnabled(
@@ -2022,6 +2024,7 @@
this._lineHeight = Number(lineHeight.slice(0, lineHeight.length - 2));
this._change = change;
+ this.changeService.updateChange(change);
if (
!this._patchRange ||
!this._patchRange.patchNum ||
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
index 99e5356..ae446cd 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.ts
@@ -105,6 +105,7 @@
import {GerritView} from '../../../services/router/router-model';
import {ParsedChangeInfo} from '../../../types/types';
import {GrRelatedChangesList} from '../gr-related-changes-list/gr-related-changes-list';
+import {appContext} from '../../../services/app-context';
const pluginApi = _testOnly_initGerritPluginApi();
const fixture = fixtureFromElement('gr-change-view');
@@ -2766,7 +2767,7 @@
element._change = {...change};
element._patchRange = {patchNum: 4 as PatchSetNum};
element._mergeable = true;
- const showStub = sinon.stub(element.jsAPI, 'handleEvent');
+ const showStub = sinon.stub(appContext.jsApiService, 'handleEvent');
element._sendShowChangeEvent();
assert.isTrue(showStub.calledOnce);
assert.equal(showStub.lastCall.args[0], EventType.SHOW_CHANGE);
@@ -2968,11 +2969,11 @@
test("don't report changeDisplayed on reply", done => {
const changeDisplayStub = sinon.stub(
- element.reporting,
+ appContext.reportingService,
'changeDisplayed'
);
const changeFullyLoadedStub = sinon.stub(
- element.reporting,
+ appContext.reportingService,
'changeFullyLoaded'
);
element._handleReplySent();
@@ -2985,11 +2986,11 @@
test('report changeDisplayed on _paramsChanged', done => {
const changeDisplayStub = sinon.stub(
- element.reporting,
+ appContext.reportingService,
'changeDisplayed'
);
const changeFullyLoadedStub = sinon.stub(
- element.reporting,
+ appContext.reportingService,
'changeFullyLoaded'
);
element._paramsChanged({
diff --git a/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog_html.ts b/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog_html.ts
index 99094d2..29b3752 100644
--- a/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog_html.ts
+++ b/polygerrit-ui/app/elements/change/gr-confirm-submit-dialog/gr-confirm-submit-dialog_html.ts
@@ -25,7 +25,7 @@
margin-bottom: var(--spacing-l);
}
.warningBeforeSubmit {
- color: var(--error-text-color);
+ color: var(--warning-foreground);
vertical-align: top;
margin-right: var(--spacing-s);
}
diff --git a/polygerrit-ui/app/elements/checks/gr-checks-results.ts b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
index 002c8c3..ef2430c 100644
--- a/polygerrit-ui/app/elements/checks/gr-checks-results.ts
+++ b/polygerrit-ui/app/elements/checks/gr-checks-results.ts
@@ -24,7 +24,14 @@
query,
} from 'lit-element';
import {GrLitElement} from '../lit/gr-lit-element';
-import {Category, CheckRun, Link, RunStatus, Tag} from '../../api/checks';
+import {
+ Category,
+ CheckRun,
+ Link,
+ LinkIcon,
+ RunStatus,
+ Tag,
+} from '../../api/checks';
import {sharedStyles} from '../../styles/shared-styles';
import {RunResult} from '../../services/checks/checks-model';
import {
@@ -35,6 +42,7 @@
} from '../../services/checks/checks-util';
import {assertIsDefined} from '../../utils/common-util';
import {whenVisible} from '../../utils/dom-util';
+import {durationString} from '../../utils/date-util';
@customElement('gr-result-row')
class GrResultRow extends GrLitElement {
@@ -473,9 +481,26 @@
const adaptedRun: RunResult = {
category: Category.INFO, // will not be used, but is required
summary: run.statusDescription ?? '',
- message: 'Completed without results.',
...run,
};
+ if (!run.statusDescription) {
+ const start = run.scheduledTimestamp ?? run.startedTimestamp;
+ const end = run.finishedTimestamp;
+ let duration = '';
+ if (start && end) {
+ duration = ` in ${durationString(start, end, true)}`;
+ }
+ adaptedRun.message = `Completed without results${duration}.`;
+ }
+ if (run.statusLink) {
+ adaptedRun.links = [
+ {
+ url: run.statusLink,
+ primary: true,
+ icon: LinkIcon.EXTERNAL,
+ },
+ ];
+ }
return html`<gr-result-row .result="${adaptedRun}"></gr-result-row>`;
}
}
diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
index 3a76112..676ef7b 100644
--- a/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
+++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.ts
@@ -308,15 +308,6 @@
private readonly restApiService = appContext.restApiService;
- private readonly changeService = appContext.changeService;
-
- constructor() {
- super();
- // TODO: This is just an artificical dependdency such that the service is
- // instantiated and its observables subscribed. Remove this later.
- this.changeService.dontDoAnything();
- }
-
start() {
if (!this._app) {
return;
diff --git a/polygerrit-ui/app/services/app-context-init.ts b/polygerrit-ui/app/services/app-context-init.ts
index 09e0724..0369ccf 100644
--- a/polygerrit-ui/app/services/app-context-init.ts
+++ b/polygerrit-ui/app/services/app-context-init.ts
@@ -70,7 +70,7 @@
eventEmitter: () => new EventEmitter(),
authService: () => new Auth(appContext.eventEmitter),
restApiService: () => new GrRestApiInterface(appContext.authService),
- changeService: () => new ChangeService(appContext.restApiService),
+ changeService: () => new ChangeService(),
checksService: () => new ChecksService(),
jsApiService: () => new GrJsApiInterface(),
});
diff --git a/polygerrit-ui/app/services/change/change-service.ts b/polygerrit-ui/app/services/change/change-service.ts
index 6a9a5e9..4524813 100644
--- a/polygerrit-ui/app/services/change/change-service.ts
+++ b/polygerrit-ui/app/services/change/change-service.ts
@@ -14,28 +14,33 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-
import {routerChangeNum$} from '../router/router-model';
import {updateState} from './change-model';
-import {RestApiService} from '../gr-rest-api/gr-rest-api';
-import {switchMap, tap} from 'rxjs/operators';
-import {of, from} from 'rxjs';
+import {tap} from 'rxjs/operators';
+import {ParsedChangeInfo} from '../../types/types';
export class ChangeService {
+ // TODO: In the future we will want to make restApiService.getChangeDetail()
+ // calls from a switchMap() here. For now just make sure to invalidate the
+ // change when no changeNum is set.
private routerChangeNumEffect = routerChangeNum$.pipe(
- switchMap(changeNum => {
- if (!changeNum) return of(undefined);
- return from(this.restApiService.getChangeDetail(changeNum));
- }),
- tap(change => {
- updateState(change ?? undefined);
+ tap(changeNum => {
+ if (!changeNum) updateState(undefined);
})
);
- constructor(private readonly restApiService: RestApiService) {
+ constructor() {
this.routerChangeNumEffect.subscribe();
}
- // TODO: Remove.
- dontDoAnything() {}
+ /**
+ * This is a temporary indirection between change-view, which currently
+ * manages what the current change is, and the change-model, which will
+ * become the source of truth in the future. We will extract a substantial
+ * amount of code from change-view and move it into this change-service. This
+ * will take some time ...
+ */
+ updateChange(change: ParsedChangeInfo) {
+ updateState(change);
+ }
}
diff --git a/polygerrit-ui/app/services/checks/checks-model.ts b/polygerrit-ui/app/services/checks/checks-model.ts
index 6705a85..28aca73 100644
--- a/polygerrit-ui/app/services/checks/checks-model.ts
+++ b/polygerrit-ui/app/services/checks/checks-model.ts
@@ -34,6 +34,7 @@
interface ChecksProviderState {
pluginName: string;
+ loading: boolean;
config?: ChecksApiConfig;
runs: CheckRun[];
actions: Action[];
@@ -50,11 +51,18 @@
// Re-exporting as Observable so that you can only subscribe, but not emit.
export const checksState$: Observable<ChecksState> = privateState$;
-export const aPluginHasRegistered = checksState$.pipe(
+export const aPluginHasRegistered$ = checksState$.pipe(
map(state => Object.keys(state).length > 0),
distinctUntilChanged()
);
+export const someProvidersAreLoading$ = checksState$.pipe(
+ map(state => {
+ return Object.values(state).some(providerState => providerState.loading);
+ }),
+ distinctUntilChanged()
+);
+
export const allActions$ = checksState$.pipe(
map(state => {
return Object.values(state).reduce(
@@ -106,6 +114,7 @@
const nextState = {...privateState$.getValue()};
nextState[pluginName] = {
pluginName,
+ loading: false,
config,
runs: [],
actions: [],
@@ -177,6 +186,15 @@
status: RunStatus.COMPLETED,
};
+export function updateStateSetLoading(pluginName: string) {
+ const nextState = {...privateState$.getValue()};
+ nextState[pluginName] = {
+ ...nextState[pluginName],
+ loading: true,
+ };
+ privateState$.next(nextState);
+}
+
export function updateStateSetResults(
pluginName: string,
runs: CheckRun[],
@@ -185,6 +203,7 @@
const nextState = {...privateState$.getValue()};
nextState[pluginName] = {
...nextState[pluginName],
+ loading: false,
runs: [...runs],
actions: [...actions],
};
diff --git a/polygerrit-ui/app/services/checks/checks-service.ts b/polygerrit-ui/app/services/checks/checks-service.ts
index 2e63f98..b11eada 100644
--- a/polygerrit-ui/app/services/checks/checks-service.ts
+++ b/polygerrit-ui/app/services/checks/checks-service.ts
@@ -30,7 +30,11 @@
ResponseCode,
} from '../../api/checks';
import {change$, currentPatchNum$} from '../change/change-model';
-import {updateStateSetProvider, updateStateSetResults} from './checks-model';
+import {
+ updateStateSetLoading,
+ updateStateSetProvider,
+ updateStateSetResults,
+} from './checks-model';
import {
BehaviorSubject,
combineLatest,
@@ -83,6 +87,7 @@
patchsetNumber: patchNum,
repo: change.project,
};
+ updateStateSetLoading(pluginName);
return from(this.providers[pluginName].fetch(data));
}
),
diff --git a/polygerrit-ui/app/utils/date-util.ts b/polygerrit-ui/app/utils/date-util.ts
index fad5041..3af8c59 100644
--- a/polygerrit-ui/app/utils/date-util.ts
+++ b/polygerrit-ui/app/utils/date-util.ts
@@ -37,9 +37,13 @@
// similar to fromNow from moment.js
export function fromNow(date: Date, noAgo = false) {
- const now = new Date();
+ return durationString(date, new Date(), noAgo);
+}
+
+// similar to fromNow from moment.js
+export function durationString(from: Date, to: Date, noAgo = false) {
const ago = noAgo ? '' : ' ago';
- const secondsAgo = Math.floor((now.valueOf() - date.valueOf()) / 1000);
+ const secondsAgo = Math.floor((to.valueOf() - from.valueOf()) / 1000);
if (secondsAgo <= 59) return 'just now';
if (secondsAgo <= 119) return `1 minute${ago}`;
const minutesAgo = Math.floor(secondsAgo / 60);
