Merge changes I2cbc6c31,I8792650f * changes: Prefactor to start unifying plugin-loader and plugins-model Remove pluginLoader from getAppContext()
diff --git a/java/com/google/gerrit/acceptance/BUILD b/java/com/google/gerrit/acceptance/BUILD index 4298663..9d237af 100644 --- a/java/com/google/gerrit/acceptance/BUILD +++ b/java/com/google/gerrit/acceptance/BUILD
@@ -74,6 +74,7 @@ "//java/com/google/gerrit/gpg/testing:gpg-test-util", "//java/com/google/gerrit/git/testing", "//java/com/google/gerrit/index/testing", + "//lib/errorprone:annotations", ] PGM_DEPLOY_ENV = [
diff --git a/java/com/google/gerrit/acceptance/PushOneCommit.java b/java/com/google/gerrit/acceptance/PushOneCommit.java index 36f4ba1..bb95a02 100644 --- a/java/com/google/gerrit/acceptance/PushOneCommit.java +++ b/java/com/google/gerrit/acceptance/PushOneCommit.java
@@ -24,6 +24,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; import com.google.common.collect.Sets; +import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.UsedAt; import com.google.gerrit.common.UsedAt.Project; @@ -276,6 +277,7 @@ return this; } + @CanIgnoreReturnValue public PushOneCommit setTopLevelTreeId(ObjectId treeId) throws Exception { commitBuilder.setTopLevelTree(treeId); return this;
diff --git a/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementPredicateIT.java b/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementPredicateIT.java index bdae0c4..56e23a4 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementPredicateIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementPredicateIT.java
@@ -311,8 +311,10 @@ private PushOneCommit.Result createMergeCommitChange( String ref, RevCommit parent1, RevCommit parent2, @Nullable ObjectId treeId) throws Exception { - PushOneCommit m = pushFactory.create(admin.newIdent(), testRepo); - m.setParents(ImmutableList.of(parent1, parent2)); + PushOneCommit m = + pushFactory + .create(admin.newIdent(), testRepo) + .setParents(ImmutableList.of(parent1, parent2)); if (treeId != null) { m.setTopLevelTreeId(treeId); }
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts index ce7eb54..50f2c02 100644 --- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts +++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.ts
@@ -108,7 +108,7 @@ @state() private dynamicHeaderEndpoints?: string[]; - @property({type: Number}) selectedIndex?: number; + @property({type: Number}) selectedIndex = 0; @property({type: Boolean}) showNumber?: boolean; // No default value to prevent flickering.
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 a05d61e..caecf45 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
@@ -36,7 +36,11 @@ import {flush} from '@polymer/polymer/lib/legacy/polymer.dom'; import {GrEditConstants} from '../../edit/gr-edit-constants'; import {pluralize} from '../../../utils/string-util'; -import {querySelectorAll, windowLocationReload} from '../../../utils/dom-util'; +import { + querySelectorAll, + whenVisible, + windowLocationReload, +} from '../../../utils/dom-util'; import {navigationToken} from '../../core/gr-navigation/gr-navigation'; import {getPluginEndpoints} from '../../shared/gr-js-api-interface/gr-plugin-endpoints'; import {RevisionInfo as RevisionInfoClass} from '../../shared/revision-info/revision-info'; @@ -190,8 +194,6 @@ const REVIEWERS_REGEX = /^(R|CC)=/gm; const MIN_CHECK_INTERVAL_SECS = 0; -const REPLY_REFIT_DEBOUNCE_INTERVAL_MS = 500; - const ACCIDENTAL_STARRING_LIMIT_MS = 10 * 1000; const TRAILING_WHITESPACE_REGEX = /[ \t]+$/gm; @@ -247,7 +249,7 @@ @query('#downloadDialog') downloadDialog?: GrDownloadDialog; - @query('#replyOverlay') replyOverlay?: GrOverlay; + @query('#replyModal') replyModal?: HTMLDialogElement; @query('#replyDialog') replyDialog?: GrReplyDialog; @@ -523,9 +525,9 @@ @state() scrollCommentId?: UrlEncodedCommentId; - /** Just reflects the `opened` prop of the overlay. */ + /** Reflects the `opened` state of the reply dialog. */ @state() - private replyOverlayOpened = false; + replyModalOpened = false; // Accessed in tests. readonly reporting = getAppContext().reportingService; @@ -554,8 +556,6 @@ private readonly getShortcutsService = resolve(this, shortcutsServiceToken); - private replyRefitTask?: DelayedTask; - private scrollTask?: DelayedTask; private lastStarredTimestamp?: number; @@ -592,18 +592,6 @@ } private setupListeners() { - this.addEventListener( - // When an overlay is opened in a mobile viewport, the overlay has a full - // screen view. When it has a full screen view, we do not want the - // background to be scrollable. This will eliminate background scroll by - // hiding most of the contents on the screen upon opening, and showing - // again upon closing. - 'fullscreen-overlay-opened', - () => this.handleHideBackgroundContent() - ); - this.addEventListener('fullscreen-overlay-closed', () => - this.handleShowBackgroundContent() - ); this.addEventListener('open-reply-dialog', () => this.openReplyDialog()); this.addEventListener('change-message-deleted', () => fireReload(this)); this.addEventListener('editable-content-save', e => @@ -846,7 +834,6 @@ this.handleVisibilityChange ); document.removeEventListener('scroll', this.handleScroll); - this.replyRefitTask?.cancel(); this.scrollTask?.cancel(); if (this.updateCheckTimerHandle) { @@ -887,6 +874,11 @@ padding: var(--spacing-s) var(--spacing-l); z-index: 99; /* Less than gr-overlay's backdrop */ } + dialog { + padding: 0; + border: 1px solid var(--border-color); + border-radius: var(--border-radius); + } .header.editMode { background-color: var(--edit-mode-background-color); } @@ -1153,9 +1145,6 @@ min-width: initial; width: 100vw; } - #replyOverlay { - z-index: var(--reply-overlay-z-index); - } } .patch-set-dropdown { margin: var(--spacing-m) 0 0 var(--spacing-m); @@ -1215,18 +1204,9 @@ @close=${this.handleIncludedInDialogClose} ></gr-included-in-dialog> </gr-overlay> - <gr-overlay - id="replyOverlay" - class="scrollable" - no-cancel-on-outside-click="" - no-cancel-on-esc-key="" - scroll-action="lock" - with-backdrop="" - @iron-overlay-canceled=${this.onReplyOverlayCanceled} - @opened-changed=${this.onReplyOverlayOpenedChanged} - > + <dialog id="replyModal" @close=${this.onReplyModalCanceled}> ${when( - this.replyOverlayOpened && this.loggedIn, + this.replyModalOpened && this.loggedIn, () => html` <gr-reply-dialog id="replyDialog" @@ -1236,13 +1216,11 @@ .canBeStarted=${this.canStartReview()} @send=${this.handleReplySent} @cancel=${this.handleReplyCancel} - @autogrow=${this.handleReplyAutogrow} - @send-disabled-changed=${this.resetReplyOverlayFocusStops} > </gr-reply-dialog> ` )} - </gr-overlay> + </dialog> `; } @@ -1970,13 +1948,10 @@ this.openReplyDialog(FocusTarget.ANY); } - private onReplyOverlayCanceled() { + private onReplyModalCanceled() { fireDialogChange(this, {canceled: true}); this.changeViewAriaHidden = false; - } - - private onReplyOverlayOpenedChanged(e: ValueChangedEvent<boolean>) { - this.replyOverlayOpened = e.detail.value; + this.replyModalOpened = false; } private handleOpenDiffPrefs() { @@ -2028,18 +2003,6 @@ } // Private but used in tests. - handleHideBackgroundContent() { - assertIsDefined(this.mainContent); - this.mainContent.classList.add('overlayOpen'); - } - - // Private but used in tests. - handleShowBackgroundContent() { - assertIsDefined(this.mainContent); - this.mainContent.classList.remove('overlayOpen'); - } - - // Private but used in tests. handleReplySent() { this.addEventListener( 'change-details-loaded', @@ -2048,26 +2011,15 @@ }, {once: true} ); - assertIsDefined(this.replyOverlay); - this.replyOverlay.cancel(); + assertIsDefined(this.replyModal); + this.replyModal.close(); fireReload(this); } private handleReplyCancel() { - assertIsDefined(this.replyOverlay); - this.replyOverlay.cancel(); - } - - private handleReplyAutogrow() { - // If the textarea resizes, we need to re-fit the overlay. - this.replyRefitTask = debounce( - this.replyRefitTask, - () => { - assertIsDefined(this.replyOverlay); - this.replyOverlay.refit(); - }, - REPLY_REFIT_DEBOUNCE_INTERVAL_MS - ); + assertIsDefined(this.replyModal); + this.replyModal.close(); + this.onReplyModalCanceled(); } // Private but used in tests. @@ -2646,16 +2598,12 @@ openReplyDialog(focusTarget?: FocusTarget, quote?: string) { if (!this.change) return; - assertIsDefined(this.replyOverlay); - const overlay = this.replyOverlay; - overlay.open().finally(() => { - // the following code should be executed no matter open succeed or not - const dialog = this.replyDialog; - assertIsDefined(dialog, 'reply dialog'); - this.resetReplyOverlayFocusStops(); - dialog.open(focusTarget, quote); - const observer = new ResizeObserver(() => overlay.center()); - observer.observe(dialog); + this.replyModalOpened = true; + assertIsDefined(this.replyModal); + this.replyModal.showModal(); + whenVisible(this.replyModal, () => { + assertIsDefined(this.replyDialog, 'replyDialog'); + this.replyDialog.open(focusTarget, quote); }); fireDialogChange(this, {opened: true}); this.changeViewAriaHidden = true; @@ -3297,14 +3245,6 @@ ); } - private resetReplyOverlayFocusStops() { - const dialog = this.replyDialog; - const focusStops = dialog?.getFocusStops(); - if (!focusStops) return; - assertIsDefined(this.replyOverlay); - this.replyOverlay.setFocusStops(focusStops); - } - // Private but used in tests. async handleToggleStar(e: CustomEvent<ChangeStarToggleStarDetail>) { if (e.detail.starred) {
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 8a30dc9e..5fbbe35 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
@@ -12,7 +12,6 @@ CommentSide, DefaultBase, DiffViewMode, - HttpMethod, MessageTag, createDefaultPreferences, Tab, @@ -30,6 +29,7 @@ waitEventLoop, waitQueryAndAssert, waitUntil, + waitUntilVisible, } from '../../../test/test-utils'; import { createChangeViewState, @@ -91,7 +91,6 @@ LoadingStatus, } from '../../../models/change/change-model'; import {FocusTarget, GrReplyDialog} from '../gr-reply-dialog/gr-reply-dialog'; -import {GrOverlay} from '../../shared/gr-overlay/gr-overlay'; import {GrChangeStar} from '../../shared/gr-change-star/gr-change-star'; import {GrThreadList} from '../gr-thread-list/gr-thread-list'; import {assertIsDefined} from '../../../utils/common-util'; @@ -552,18 +551,7 @@ > <gr-included-in-dialog id="includedInDialog"> </gr-included-in-dialog> </gr-overlay> - <gr-overlay - aria-hidden="true" - class="scrollable" - id="replyOverlay" - no-cancel-on-esc-key="" - no-cancel-on-outside-click="" - scroll-action="lock" - style="outline: none; display: none;" - tabindex="-1" - with-backdrop="" - > - </gr-overlay> + <dialog id="replyModal"></dialog> ` ); }); @@ -823,102 +811,56 @@ element.addEventListener('show-auth-required', loggedInErrorSpy); pressKey(element, 'a'); await element.updateComplete; - assertIsDefined(element.replyOverlay); - assert.isFalse(element.replyOverlay.opened); + assertIsDefined(element.replyModal); + assert.isFalse(element.replyModalOpened); assert.isTrue(loggedInErrorSpy.called); }); test('shift A does not open reply overlay', async () => { pressKey(element, 'a', Modifier.SHIFT_KEY); await element.updateComplete; - assertIsDefined(element.replyOverlay); - assert.isFalse(element.replyOverlay.opened); + assertIsDefined(element.replyModal); + assert.isFalse(element.replyModalOpened); }); test('A toggles overlay when logged in', async () => { - element.change = { + // restore clock so that setTimeout in waitUntil() works as expected + clock.restore(); + stubRestApi('getChangeDetail').returns( + Promise.resolve(createParsedChange()) + ); + sinon.stub(element, 'performPostChangeLoadTasks'); + sinon.stub(element, 'getMergeability'); + const change = { ...createChangeViewChange(), revisions: createRevisions(1), messages: createChangeMessages(1), }; - element.change.labels = {}; + change.labels = {}; + element.change = change; + + changeModel.setState({ + loadingStatus: LoadingStatus.LOADED, + change, + }); + await element.updateComplete; const openSpy = sinon.spy(element, 'openReplyDialog'); pressKey(element, 'a'); await element.updateComplete; - assertIsDefined(element.replyOverlay); - assert.isTrue(element.replyOverlay.opened); - element.replyOverlay.close(); - assert.isFalse(element.replyOverlay.opened); + assertIsDefined(element.replyModal); + assert.isTrue(element.replyModalOpened); + sinon.spy(element.replyDialog!, 'open'); + await waitUntilVisible(element.replyDialog!); + element.replyModal.close(); assert( openSpy.lastCall.calledWithExactly(FocusTarget.ANY), 'openReplyDialog should have been passed ANY' ); assert.equal(openSpy.callCount, 1); - }); - - test('fullscreen-overlay-opened hides content', async () => { - element.loggedIn = true; - element.loading = false; - element.change = { - ...createChangeViewChange(), - labels: {}, - actions: { - abandon: { - enabled: true, - label: 'Abandon', - method: HttpMethod.POST, - title: 'Abandon', - }, - }, - }; - await element.updateComplete; - const handlerSpy = sinon.spy(element, 'handleHideBackgroundContent'); - const overlay = queryAndAssert<GrOverlay>(element, '#replyOverlay'); - overlay.dispatchEvent( - new CustomEvent('fullscreen-overlay-opened', { - composed: true, - bubbles: true, - }) - ); - await element.updateComplete; - assert.isTrue(handlerSpy.called); - assertIsDefined(element.mainContent); - assertIsDefined(element.actions); - assert.isTrue(element.mainContent.classList.contains('overlayOpen')); - assert.equal(getComputedStyle(element.actions).display, 'flex'); - }); - - test('fullscreen-overlay-closed shows content', async () => { - element.loggedIn = true; - element.loading = false; - element.change = { - ...createChangeViewChange(), - labels: {}, - actions: { - abandon: { - enabled: true, - label: 'Abandon', - method: HttpMethod.POST, - title: 'Abandon', - }, - }, - }; - await element.updateComplete; - const handlerSpy = sinon.spy(element, 'handleShowBackgroundContent'); - const overlay = queryAndAssert<GrOverlay>(element, '#replyOverlay'); - overlay.dispatchEvent( - new CustomEvent('fullscreen-overlay-closed', { - composed: true, - bubbles: true, - }) - ); - await element.updateComplete; - assert.isTrue(handlerSpy.called); - assertIsDefined(element.mainContent); - assert.isFalse(element.mainContent.classList.contains('overlayOpen')); + await waitUntil(() => !element.replyModalOpened); }); test('expand all messages when expand-diffs fired', () => { @@ -1996,6 +1938,15 @@ }); test('reply from comment adds quote text', async () => { + const change = { + ...createChangeViewChange(), + revisions: createRevisions(1), + messages: createChangeMessages(1), + }; + changeModel.setState({ + loadingStatus: LoadingStatus.LOADED, + change, + }); const e = new CustomEvent('', { detail: {message: {message: 'quote text'}}, });
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 0056328..a5e18dd 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
@@ -182,13 +182,6 @@ */ /** - * Fired when the main textarea's value changes, which may have triggered - * a change in size for the dialog. - * - * @event autogrow - */ - - /** * Fires to show an alert when a send is attempted on the non-latest patch. * * @event show-alert @@ -754,12 +747,6 @@ this.computeMessagePlaceholder(); this.computeSendButtonLabel(); } - if (changedProperties.has('reviewFormatting')) { - this.handleHeightChanged(); - } - if (changedProperties.has('draftCommentThreads')) { - this.handleHeightChanged(); - } if (changedProperties.has('sendDisabled')) { this.sendDisabledChanged(); } @@ -1982,10 +1969,6 @@ ); } - handleHeightChanged() { - fireEvent(this, 'autogrow'); - } - getLabelScores(): GrLabelScores { return this.labelScores || queryAndAssert(this, 'gr-label-scores'); }
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 ef781ba..4ad76f4 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
@@ -2022,17 +2022,6 @@ await promise; }); - test('fires height change when the drafts comments load', async () => { - // Flush DOM operations before binding to the autogrow event so we don't - // catch the events fired from the initial layout. - await element.updateComplete; - const autoGrowHandler = sinon.stub(); - element.addEventListener('autogrow', autoGrowHandler); - element.draftCommentThreads = []; - await element.updateComplete; - assert.isTrue(autoGrowHandler.called); - }); - suite('start review and save buttons', () => { let sendStub: sinon.SinonStub;
diff --git a/polygerrit-ui/app/elements/gr-app-element.ts b/polygerrit-ui/app/elements/gr-app-element.ts index bc84ac0..7cadddc 100644 --- a/polygerrit-ui/app/elements/gr-app-element.ts +++ b/polygerrit-ui/app/elements/gr-app-element.ts
@@ -81,6 +81,12 @@ moreInfo?: string; } +/** + * This is simple hacky way for allowing certain plugin screens to hide the + * header and the footer of the Gerrit page. + */ +const WHITE_LISTED_FULL_SCREEN_PLUGINS = ['git_source_editor/screen/edit']; + // TODO(TS): implement AppElement interface from gr-app-types.ts @customElement('gr-app-element') export class GrAppElement extends LitElement { @@ -398,6 +404,7 @@ } private renderHeader() { + if (this.hideHeaderAndFooter()) return nothing; return html` <gr-main-header id="mainHeader" @@ -413,6 +420,7 @@ } private renderFooter() { + if (this.hideHeaderAndFooter()) return nothing; return html` <footer ?aria-hidden=${this.footerHeaderAriaHidden}> <div> @@ -434,6 +442,13 @@ `; } + private hideHeaderAndFooter() { + return ( + this.view === GerritView.PLUGIN_SCREEN && + WHITE_LISTED_FULL_SCREEN_PLUGINS.includes(this.computePluginScreenName()) + ); + } + private renderMobileSearch() { if (!this.mobileSearch) return nothing; return html`
diff --git a/polygerrit-ui/app/styles/themes/app-theme.ts b/polygerrit-ui/app/styles/themes/app-theme.ts index e148da9..102520c 100644 --- a/polygerrit-ui/app/styles/themes/app-theme.ts +++ b/polygerrit-ui/app/styles/themes/app-theme.ts
@@ -474,7 +474,6 @@ /* misc */ --border-radius: 4px; - --reply-overlay-z-index: 1000; --line-length-indicator-color: #681da8; /* paper and iron component overrides */
diff --git a/polygerrit-ui/app/test/test-data-generators.ts b/polygerrit-ui/app/test/test-data-generators.ts index 38bf329..486c45d 100644 --- a/polygerrit-ui/app/test/test-data-generators.ts +++ b/polygerrit-ui/app/test/test-data-generators.ts
@@ -388,6 +388,7 @@ messages.push({ ...createChangeMessageInfo((i + messageIdStart).toString(16)), date: dateToTimestamp(messageDate), + author: createAccountDetailWithId(i), }); messageDate = new Date(messageDate); messageDate.setDate(messageDate.getDate() + 1);