From 9891e251bbfe943ff3b6c9d9566d935bdfb166f4 Mon Sep 17 00:00:00 2001 From: David Simpson <45690499+davegarthsimpson@users.noreply.github.com> Date: Thu, 30 Jun 2022 14:26:09 +0200 Subject: [PATCH 1/2] ensure desired prompts shown + refactor --- .../browser/boards/boards-auto-installer.ts | 313 +++++++++++++----- .../browser/boards/boards-service-provider.ts | 28 +- 2 files changed, 247 insertions(+), 94 deletions(-) diff --git a/arduino-ide-extension/src/browser/boards/boards-auto-installer.ts b/arduino-ide-extension/src/browser/boards/boards-auto-installer.ts index 6557ba84c..de72c6b8c 100644 --- a/arduino-ide-extension/src/browser/boards/boards-auto-installer.ts +++ b/arduino-ide-extension/src/browser/boards/boards-auto-installer.ts @@ -5,19 +5,37 @@ import { BoardsService, BoardsPackage, Board, + Port, } from '../../common/protocol/boards-service'; import { BoardsServiceProvider } from './boards-service-provider'; -import { BoardsConfig } from './boards-config'; import { Installable, ResponseServiceArduino } from '../../common/protocol'; import { BoardsListWidgetFrontendContribution } from './boards-widget-frontend-contribution'; import { nls } from '@theia/core/lib/common'; +import { NotificationCenter } from '../notification-center'; + +interface AutoInstallPromptAction { + // isAcceptance, whether or not the action indicates acceptance of auto-install proposal + isAcceptance?: boolean; + key: string; + handler: (...args: unknown[]) => unknown; +} + +type AutoInstallPromptActions = AutoInstallPromptAction[]; /** * Listens on `BoardsConfig.Config` changes, if a board is selected which does not * have the corresponding core installed, it proposes the user to install the core. */ + +// * Cases in which we do not show the auto-install prompt: +// 1. When a related platform is already installed +// 2. When a prompt is already showing in the UI +// 3. When a board is unplugged @injectable() export class BoardsAutoInstaller implements FrontendApplicationContribution { + @inject(NotificationCenter) + protected readonly notificationCenter: NotificationCenter; + @inject(MessageService) protected readonly messageService: MessageService; @@ -36,97 +54,228 @@ export class BoardsAutoInstaller implements FrontendApplicationContribution { // Workaround for https://github.com/eclipse-theia/theia/issues/9349 protected notifications: Board[] = []; - onStart(): void { - this.boardsServiceClient.onBoardsConfigChanged( - this.ensureCoreExists.bind(this) + private spliceNotificationByBoard(selectedBoard: Board): void { + const index = this.notifications.findIndex((notification) => + Board.sameAs(notification, selectedBoard) ); - this.ensureCoreExists(this.boardsServiceClient.boardsConfig); + if (index !== -1) { + this.notifications.splice(index, 1); + } } - protected ensureCoreExists(config: BoardsConfig.Config): void { - const { selectedBoard, selectedPort } = config; - if ( - selectedBoard && - selectedPort && - !this.notifications.find((board) => Board.sameAs(board, selectedBoard)) - ) { - this.notifications.push(selectedBoard); - this.boardsService.search({}).then((packages) => { - // filter packagesForBoard selecting matches from the cli (installed packages) - // and matches based on the board name - // NOTE: this ensures the Deprecated & new packages are all in the array - // so that we can check if any of the valid packages is already installed - const packagesForBoard = packages.filter( - (pkg) => - BoardsPackage.contains(selectedBoard, pkg) || - pkg.boards.some((board) => board.name === selectedBoard.name) - ); - - // check if one of the packages for the board is already installed. if so, no hint + // * "refusal" meaning a "prompt action" not accepting the auto-install offer ("X" or "install manually") + // we can use "portSelectedOnLastRefusal" to deduce when a board is unplugged after a user has "refused" + // an auto-install prompt. Important to know as we do not want "an unplug" to trigger a "refused" prompt + // showing again + private portSelectedOnLastRefusal: Port | undefined; + private lastRefusedPackageId: string | undefined; + + private clearLastRefusedPromptInfo(): void { + this.lastRefusedPackageId = undefined; + this.portSelectedOnLastRefusal = undefined; + } + + private setLastRefusedPromptInfo( + packageId: string, + selectedPort?: Port + ): void { + this.lastRefusedPackageId = packageId; + this.portSelectedOnLastRefusal = selectedPort; + } + + onStart(): void { + const setEventListeners = () => { + this.boardsServiceClient.onBoardsConfigChanged((config) => { + const { selectedBoard, selectedPort } = config; + + const boardWasUnplugged = + !selectedPort && this.portSelectedOnLastRefusal; + + this.clearLastRefusedPromptInfo(); + if ( - packagesForBoard.some(({ installedVersion }) => !!installedVersion) + boardWasUnplugged || + !selectedBoard || + this.promptAlreadyShowingForBoard(selectedBoard) ) { return; } - // filter the installable (not installed) packages, - // CLI returns the packages already sorted with the deprecated ones at the end of the list - // in order to ensure the new ones are preferred - const candidates = packagesForBoard.filter( - ({ installable, installedVersion }) => - installable && !installedVersion - ); - - const candidate = candidates[0]; - if (candidate) { - const version = candidate.availableVersions[0] - ? `[v ${candidate.availableVersions[0]}]` - : ''; - const yes = nls.localize('vscode/extensionsUtils/yes', 'Yes'); - const manualInstall = nls.localize( - 'arduino/board/installManually', - 'Install Manually' - ); - // tslint:disable-next-line:max-line-length - this.messageService - .info( - nls.localize( - 'arduino/board/installNow', - 'The "{0} {1}" core has to be installed for the currently selected "{2}" board. Do you want to install it now?', - candidate.name, - version, - selectedBoard.name - ), - manualInstall, - yes - ) - .then(async (answer) => { - const index = this.notifications.findIndex((board) => - Board.sameAs(board, selectedBoard) - ); - if (index !== -1) { - this.notifications.splice(index, 1); - } - if (answer === yes) { - await Installable.installWithProgress({ - installable: this.boardsService, - item: candidate, - messageService: this.messageService, - responseService: this.responseService, - version: candidate.availableVersions[0], - }); - return; - } - if (answer === manualInstall) { - this.boardsManagerFrontendContribution - .openView({ reveal: true }) - .then((widget) => - widget.refresh(candidate.name.toLocaleLowerCase()) - ); - } - }); + this.ensureCoreExists(selectedBoard, selectedPort); + }); + + // we "clearRefusedPackageInfo" if a "refused" package is eventually + // installed, though this is not strictly necessary. It's more of a + // cleanup, to ensure the related variables are representative of + // current state. + this.notificationCenter.onPlatformInstalled((installed) => { + if (this.lastRefusedPackageId === installed.item.id) { + this.clearLastRefusedPromptInfo(); } }); + }; + + // we should invoke this.ensureCoreExists only once we're sure + // everything has been reconciled + this.boardsServiceClient.reconciled.then(() => { + const { selectedBoard, selectedPort } = + this.boardsServiceClient.boardsConfig; + + if (selectedBoard) { + this.ensureCoreExists(selectedBoard, selectedPort); + } + + setEventListeners(); + }); + } + + private promptAlreadyShowingForBoard(board: Board): boolean { + return Boolean( + this.notifications.find((notification) => + Board.sameAs(notification, board) + ) + ); + } + + protected ensureCoreExists(selectedBoard: Board, selectedPort?: Port): void { + this.notifications.push(selectedBoard); + this.boardsService.search({}).then((packages) => { + const candidate = this.getInstallCandidate(packages, selectedBoard); + + if (candidate) { + this.showAutoInstallPrompt(candidate, selectedBoard, selectedPort); + } else { + this.spliceNotificationByBoard(selectedBoard); + } + }); + } + + private getInstallCandidate( + packages: BoardsPackage[], + selectedBoard: Board + ): BoardsPackage | void { + // filter packagesForBoard selecting matches from the cli (installed packages) + // and matches based on the board name + // NOTE: this ensures the Deprecated & new packages are all in the array + // so that we can check if any of the valid packages is already installed + const packagesForBoard = packages.filter( + (pkg) => + BoardsPackage.contains(selectedBoard, pkg) || + pkg.boards.some((board) => board.name === selectedBoard.name) + ); + + // check if one of the packages for the board is already installed. if so, no hint + if (packagesForBoard.some(({ installedVersion }) => !!installedVersion)) { + return; } + + // filter the installable (not installed) packages, + // CLI returns the packages already sorted with the deprecated ones at the end of the list + // in order to ensure the new ones are preferred + const candidates = packagesForBoard.filter( + ({ installable, installedVersion }) => installable && !installedVersion + ); + + return candidates[0]; + } + + private showAutoInstallPrompt( + candidate: BoardsPackage, + selectedBoard: Board, + selectedPort?: Port + ): void { + const candidateName = candidate.name; + const version = candidate.availableVersions[0] + ? `[v ${candidate.availableVersions[0]}]` + : ''; + + const info = this.generatePromptInfoText( + candidateName, + version, + selectedBoard.name + ); + + const actions = this.createPromptActions(candidate); + + const onRefuse = () => { + this.setLastRefusedPromptInfo(candidate.id, selectedPort); + }; + const handleAction = this.createOnAnswerHandler(actions, onRefuse); + + const onAnswer = (answer: string) => { + this.spliceNotificationByBoard(selectedBoard); + + handleAction(answer); + }; + + this.messageService + .info(info, ...actions.map((action) => action.key)) + .then(onAnswer); + } + + private generatePromptInfoText( + candidateName: string, + version: string, + boardName: string + ): string { + return nls.localize( + 'arduino/board/installNow', + 'The "{0} {1}" core has to be installed for the currently selected "{2}" board. Do you want to install it now?', + candidateName, + version, + boardName + ); + } + + private createPromptActions( + candidate: BoardsPackage + ): AutoInstallPromptActions { + const yes = nls.localize('vscode/extensionsUtils/yes', 'Yes'); + const manualInstall = nls.localize( + 'arduino/board/installManually', + 'Install Manually' + ); + + const actions: AutoInstallPromptActions = [ + { + isAcceptance: true, + key: yes, + handler: () => { + return Installable.installWithProgress({ + installable: this.boardsService, + item: candidate, + messageService: this.messageService, + responseService: this.responseService, + version: candidate.availableVersions[0], + }); + }, + }, + { + key: manualInstall, + handler: () => { + this.boardsManagerFrontendContribution + .openView({ reveal: true }) + .then((widget) => + widget.refresh(candidate.name.toLocaleLowerCase()) + ); + }, + }, + ]; + + return actions; + } + + private createOnAnswerHandler( + actions: AutoInstallPromptActions, + onRefuse?: () => void + ): (answer: string) => void { + return (answer) => { + const actionToHandle = actions.find((action) => action.key === answer); + actionToHandle?.handler(); + + if (!actionToHandle?.isAcceptance && onRefuse) { + onRefuse(); + } + }; } } diff --git a/arduino-ide-extension/src/browser/boards/boards-service-provider.ts b/arduino-ide-extension/src/browser/boards/boards-service-provider.ts index 686e5e004..83795a25d 100644 --- a/arduino-ide-extension/src/browser/boards/boards-service-provider.ts +++ b/arduino-ide-extension/src/browser/boards/boards-service-provider.ts @@ -73,6 +73,8 @@ export class BoardsServiceProvider implements FrontendApplicationContribution { this.onAvailableBoardsChangedEmitter.event; readonly onAvailablePortsChanged = this.onAvailablePortsChangedEmitter.event; + public reconciled: Promise; + onStart(): void { this.notificationCenter.onAttachedBoardsChanged( this.notifyAttachedBoardsChanged.bind(this) @@ -84,7 +86,7 @@ export class BoardsServiceProvider implements FrontendApplicationContribution { this.notifyPlatformUninstalled.bind(this) ); - Promise.all([ + this.reconciled = Promise.all([ this.boardsService.getAttachedBoards(), this.boardsService.getAvailablePorts(), this.loadState(), @@ -185,8 +187,8 @@ export class BoardsServiceProvider implements FrontendApplicationContribution { const selectedAvailableBoard = AvailableBoard.is(selectedBoard) ? selectedBoard : this._availableBoards.find((availableBoard) => - Board.sameAs(availableBoard, selectedBoard) - ); + Board.sameAs(availableBoard, selectedBoard) + ); if ( selectedAvailableBoard && selectedAvailableBoard.selected && @@ -231,7 +233,8 @@ export class BoardsServiceProvider implements FrontendApplicationContribution { if ( this.latestValidBoardsConfig.selectedBoard.fqbn === board.fqbn && this.latestValidBoardsConfig.selectedBoard.name === board.name && - this.latestValidBoardsConfig.selectedPort.protocol === board.port?.protocol + this.latestValidBoardsConfig.selectedPort.protocol === + board.port?.protocol ) { this.boardsConfig = { ...this.latestValidBoardsConfig, @@ -376,14 +379,14 @@ export class BoardsServiceProvider implements FrontendApplicationContribution { const timeoutTask = !!timeout && timeout > 0 ? new Promise((_, reject) => - setTimeout( - () => reject(new Error(`Timeout after ${timeout} ms.`)), - timeout + setTimeout( + () => reject(new Error(`Timeout after ${timeout} ms.`)), + timeout + ) ) - ) : new Promise(() => { - /* never */ - }); + /* never */ + }); const waitUntilTask = new Promise((resolve) => { let candidate = find(what, this.availableBoards); if (candidate) { @@ -534,8 +537,9 @@ export class BoardsServiceProvider implements FrontendApplicationContribution { protected getLastSelectedBoardOnPortKey(port: Port | string): string { // TODO: we lose the port's `protocol` info (`serial`, `network`, etc.) here if the `port` is a `string`. - return `last-selected-board-on-port:${typeof port === 'string' ? port : port.address - }`; + return `last-selected-board-on-port:${ + typeof port === 'string' ? port : port.address + }`; } protected async loadState(): Promise { From 87d2f1f231a62fc91ea2e564b99b1103023df669 Mon Sep 17 00:00:00 2001 From: David Simpson <45690499+davegarthsimpson@users.noreply.github.com> Date: Mon, 4 Jul 2022 17:03:55 +0200 Subject: [PATCH 2/2] pr review changes --- .../browser/boards/boards-auto-installer.ts | 52 +++++++++---------- .../browser/boards/boards-service-provider.ts | 19 +++++-- 2 files changed, 40 insertions(+), 31 deletions(-) diff --git a/arduino-ide-extension/src/browser/boards/boards-auto-installer.ts b/arduino-ide-extension/src/browser/boards/boards-auto-installer.ts index de72c6b8c..62d9c5534 100644 --- a/arduino-ide-extension/src/browser/boards/boards-auto-installer.ts +++ b/arduino-ide-extension/src/browser/boards/boards-auto-installer.ts @@ -34,7 +34,7 @@ type AutoInstallPromptActions = AutoInstallPromptAction[]; @injectable() export class BoardsAutoInstaller implements FrontendApplicationContribution { @inject(NotificationCenter) - protected readonly notificationCenter: NotificationCenter; + private readonly notificationCenter: NotificationCenter; @inject(MessageService) protected readonly messageService: MessageService; @@ -54,15 +54,6 @@ export class BoardsAutoInstaller implements FrontendApplicationContribution { // Workaround for https://github.com/eclipse-theia/theia/issues/9349 protected notifications: Board[] = []; - private spliceNotificationByBoard(selectedBoard: Board): void { - const index = this.notifications.findIndex((notification) => - Board.sameAs(notification, selectedBoard) - ); - if (index !== -1) { - this.notifications.splice(index, 1); - } - } - // * "refusal" meaning a "prompt action" not accepting the auto-install offer ("X" or "install manually") // we can use "portSelectedOnLastRefusal" to deduce when a board is unplugged after a user has "refused" // an auto-install prompt. Important to know as we do not want "an unplug" to trigger a "refused" prompt @@ -70,19 +61,6 @@ export class BoardsAutoInstaller implements FrontendApplicationContribution { private portSelectedOnLastRefusal: Port | undefined; private lastRefusedPackageId: string | undefined; - private clearLastRefusedPromptInfo(): void { - this.lastRefusedPackageId = undefined; - this.portSelectedOnLastRefusal = undefined; - } - - private setLastRefusedPromptInfo( - packageId: string, - selectedPort?: Port - ): void { - this.lastRefusedPackageId = packageId; - this.portSelectedOnLastRefusal = selectedPort; - } - onStart(): void { const setEventListeners = () => { this.boardsServiceClient.onBoardsConfigChanged((config) => { @@ -129,6 +107,28 @@ export class BoardsAutoInstaller implements FrontendApplicationContribution { }); } + private removeNotificationByBoard(selectedBoard: Board): void { + const index = this.notifications.findIndex((notification) => + Board.sameAs(notification, selectedBoard) + ); + if (index !== -1) { + this.notifications.splice(index, 1); + } + } + + private clearLastRefusedPromptInfo(): void { + this.lastRefusedPackageId = undefined; + this.portSelectedOnLastRefusal = undefined; + } + + private setLastRefusedPromptInfo( + packageId: string, + selectedPort?: Port + ): void { + this.lastRefusedPackageId = packageId; + this.portSelectedOnLastRefusal = selectedPort; + } + private promptAlreadyShowingForBoard(board: Board): boolean { return Boolean( this.notifications.find((notification) => @@ -145,7 +145,7 @@ export class BoardsAutoInstaller implements FrontendApplicationContribution { if (candidate) { this.showAutoInstallPrompt(candidate, selectedBoard, selectedPort); } else { - this.spliceNotificationByBoard(selectedBoard); + this.removeNotificationByBoard(selectedBoard); } }); } @@ -153,7 +153,7 @@ export class BoardsAutoInstaller implements FrontendApplicationContribution { private getInstallCandidate( packages: BoardsPackage[], selectedBoard: Board - ): BoardsPackage | void { + ): BoardsPackage | undefined { // filter packagesForBoard selecting matches from the cli (installed packages) // and matches based on the board name // NOTE: this ensures the Deprecated & new packages are all in the array @@ -203,7 +203,7 @@ export class BoardsAutoInstaller implements FrontendApplicationContribution { const handleAction = this.createOnAnswerHandler(actions, onRefuse); const onAnswer = (answer: string) => { - this.spliceNotificationByBoard(selectedBoard); + this.removeNotificationByBoard(selectedBoard); handleAction(answer); }; diff --git a/arduino-ide-extension/src/browser/boards/boards-service-provider.ts b/arduino-ide-extension/src/browser/boards/boards-service-provider.ts index 83795a25d..98bd4fa14 100644 --- a/arduino-ide-extension/src/browser/boards/boards-service-provider.ts +++ b/arduino-ide-extension/src/browser/boards/boards-service-provider.ts @@ -20,6 +20,7 @@ import { NotificationCenter } from '../notification-center'; import { ArduinoCommands } from '../arduino-commands'; import { StorageWrapper } from '../storage-wrapper'; import { nls } from '@theia/core/lib/common'; +import { Deferred } from '@theia/core/lib/common/promise-util'; @injectable() export class BoardsServiceProvider implements FrontendApplicationContribution { @@ -73,7 +74,7 @@ export class BoardsServiceProvider implements FrontendApplicationContribution { this.onAvailableBoardsChangedEmitter.event; readonly onAvailablePortsChanged = this.onAvailablePortsChangedEmitter.event; - public reconciled: Promise; + private readonly _reconciled = new Deferred(); onStart(): void { this.notificationCenter.onAttachedBoardsChanged( @@ -86,18 +87,26 @@ export class BoardsServiceProvider implements FrontendApplicationContribution { this.notifyPlatformUninstalled.bind(this) ); - this.reconciled = Promise.all([ + Promise.all([ this.boardsService.getAttachedBoards(), this.boardsService.getAvailablePorts(), this.loadState(), - ]).then(([attachedBoards, availablePorts]) => { + ]).then(async ([attachedBoards, availablePorts]) => { this._attachedBoards = attachedBoards; this._availablePorts = availablePorts; this.onAvailablePortsChangedEmitter.fire(this._availablePorts); - this.reconcileAvailableBoards().then(() => this.tryReconnect()); + + await this.reconcileAvailableBoards(); + + this.tryReconnect(); + this._reconciled.resolve(); }); } + get reconciled(): Promise { + return this._reconciled.promise; + } + protected notifyAttachedBoardsChanged( event: AttachedBoardsChangeEvent ): void { @@ -211,7 +220,7 @@ export class BoardsServiceProvider implements FrontendApplicationContribution { } } - protected async tryReconnect(): Promise { + protected tryReconnect(): boolean { if (this.latestValidBoardsConfig && !this.canUploadTo(this.boardsConfig)) { for (const board of this.availableBoards.filter( ({ state }) => state !== AvailableBoard.State.incomplete