diff --git a/arduino-ide-extension/src/browser/boards/boards-data-store.ts b/arduino-ide-extension/src/browser/boards/boards-data-store.ts index f5a655748..1c23fb38f 100644 --- a/arduino-ide-extension/src/browser/boards/boards-data-store.ts +++ b/arduino-ide-extension/src/browser/boards/boards-data-store.ts @@ -21,6 +21,7 @@ import { Programmer, isBoardIdentifierChangeEvent, isProgrammer, + sanitizeFqbn, } from '../../common/protocol'; import { notEmpty } from '../../common/utils'; import type { @@ -130,7 +131,7 @@ export class BoardsDataStore if (!fqbn) { return undefined; } else { - const data = await this.getData(fqbn); + const data = await this.getData(sanitizeFqbn(fqbn)); if (data === BoardsDataStore.Data.EMPTY) { return undefined; } @@ -228,15 +229,19 @@ export class BoardsDataStore fqbn: string; selectedProgrammer: Programmer; }): Promise { - const storedData = deepClone(await this.getData(fqbn)); + const sanitizedFQBN = sanitizeFqbn(fqbn); + const storedData = deepClone(await this.getData(sanitizedFQBN)); const { programmers } = storedData; if (!programmers.find((p) => Programmer.equals(selectedProgrammer, p))) { return false; } - const data = { ...storedData, selectedProgrammer }; - await this.setData({ fqbn, data }); - this.fireChanged({ fqbn, data }); + const change: BoardsDataStoreChange = { + fqbn: sanitizedFQBN, + data: { ...storedData, selectedProgrammer }, + }; + await this.setData(change); + this.fireChanged(change); return true; } @@ -246,24 +251,26 @@ export class BoardsDataStore return false; } - const mutableData = deepClone(await this.getData(fqbn)); + const sanitizedFQBN = sanitizeFqbn(fqbn); + const mutableData = deepClone(await this.getData(sanitizedFQBN)); let didChange = false; for (const { option, selectedValue } of optionsToUpdate) { const { configOptions } = mutableData; const configOption = configOptions.find((c) => c.option === option); if (configOption) { - let updated = false; - for (const value of configOption.values) { - const mutable: Mutable = value; - if (mutable.value === selectedValue) { - mutable.selected = true; - updated = true; - } else { - mutable.selected = false; - } - } - if (updated) { + const configOptionValueIndex = configOption.values.findIndex( + (configOptionValue) => configOptionValue.value === selectedValue + ); + if (configOptionValueIndex >= 0) { + // unselect all + configOption.values + .map((value) => value as Mutable) + .forEach((value) => (value.selected = false)); + const mutableConfigValue: Mutable = + configOption.values[configOptionValueIndex]; + // make the new value `selected` + mutableConfigValue.selected = true; didChange = true; } } @@ -273,8 +280,12 @@ export class BoardsDataStore return false; } - await this.setData({ fqbn, data: mutableData }); - this.fireChanged({ fqbn, data: mutableData }); + const change: BoardsDataStoreChange = { + fqbn: sanitizedFQBN, + data: mutableData, + }; + await this.setData(change); + this.fireChanged(change); return true; } 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 a59b0dee9..29bccb242 100644 --- a/arduino-ide-extension/src/browser/boards/boards-service-provider.ts +++ b/arduino-ide-extension/src/browser/boards/boards-service-provider.ts @@ -12,6 +12,7 @@ import { Emitter } from '@theia/core/lib/common/event'; import { ILogger } from '@theia/core/lib/common/logger'; import { MessageService } from '@theia/core/lib/common/message-service'; import { nls } from '@theia/core/lib/common/nls'; +import { deepClone } from '@theia/core/lib/common/objects'; import { Deferred } from '@theia/core/lib/common/promise-util'; import type { Mutable } from '@theia/core/lib/common/types'; import { inject, injectable, optional } from '@theia/core/shared/inversify'; @@ -21,31 +22,32 @@ import { } from '@theia/output/lib/browser/output-channel'; import { BoardIdentifier, - boardIdentifierEquals, + BoardUserField, + BoardWithPackage, BoardsConfig, BoardsConfigChangeEvent, BoardsPackage, BoardsService, - BoardUserField, - BoardWithPackage, DetectedPorts, + Port, + PortIdentifier, + boardIdentifierEquals, emptyBoardsConfig, isBoardIdentifier, isBoardIdentifierChangeEvent, isPortIdentifier, isPortIdentifierChangeEvent, - Port, - PortIdentifier, portIdentifierEquals, + sanitizeFqbn, serializePlatformIdentifier, } from '../../common/protocol'; import { BoardList, BoardListHistory, - createBoardList, EditBoardsConfigActionParams, - isBoardListHistory, SelectBoardsConfigActionParams, + createBoardList, + isBoardListHistory, } from '../../common/protocol/board-list'; import type { Defined } from '../../common/types'; import type { @@ -104,6 +106,21 @@ type BoardListHistoryUpdateResult = type BoardToSelect = BoardIdentifier | undefined | 'ignore-board'; type PortToSelect = PortIdentifier | undefined | 'ignore-port'; +function sanitizeBoardToSelectFQBN(board: BoardToSelect): BoardToSelect { + if (isBoardIdentifier(board)) { + return sanitizeBoardIdentifierFQBN(board); + } + return board; +} +function sanitizeBoardIdentifierFQBN(board: BoardIdentifier): BoardIdentifier { + if (board.fqbn) { + const copy: Mutable = deepClone(board); + copy.fqbn = sanitizeFqbn(board.fqbn); + return copy; + } + return board; +} + interface UpdateBoardListHistoryParams { readonly portToSelect: PortToSelect; readonly boardToSelect: BoardToSelect; @@ -356,7 +373,8 @@ export class BoardsServiceProvider portToSelect !== 'ignore-port' && !portIdentifierEquals(portToSelect, previousSelectedPort); const boardDidChangeEvent = boardDidChange - ? { selectedBoard: boardToSelect, previousSelectedBoard } + ? // The change event must always contain any custom board options. Hence the board to select is not sanitized. + { selectedBoard: boardToSelect, previousSelectedBoard } : undefined; const portDidChangeEvent = portDidChange ? { selectedPort: portToSelect, previousSelectedPort } @@ -377,11 +395,22 @@ export class BoardsServiceProvider return false; } - this.maybeUpdateBoardListHistory({ portToSelect, boardToSelect }); - this.maybeUpdateBoardsData({ boardToSelect, reason }); + // unlike for the board change event, every persistent state must not contain custom board config options in the FQBN + const sanitizedBoardToSelect = sanitizeBoardToSelectFQBN(boardToSelect); + + this.maybeUpdateBoardListHistory({ + portToSelect, + boardToSelect: sanitizedBoardToSelect, + }); + this.maybeUpdateBoardsData({ + boardToSelect: sanitizedBoardToSelect, + reason, + }); if (isBoardIdentifierChangeEvent(event)) { - this._boardsConfig.selectedBoard = event.selectedBoard; + this._boardsConfig.selectedBoard = event.selectedBoard + ? sanitizeBoardIdentifierFQBN(event.selectedBoard) + : event.selectedBoard; } if (isPortIdentifierChangeEvent(event)) { this._boardsConfig.selectedPort = event.selectedPort; diff --git a/arduino-ide-extension/src/browser/contributions/ino-language.ts b/arduino-ide-extension/src/browser/contributions/ino-language.ts index 3ca0b49bc..4f336ef3d 100644 --- a/arduino-ide-extension/src/browser/contributions/ino-language.ts +++ b/arduino-ide-extension/src/browser/contributions/ino-language.ts @@ -10,8 +10,8 @@ import { BoardsService, ExecutableService, isBoardIdentifierChangeEvent, + sanitizeFqbn, } from '../../common/protocol'; -import { FQBN } from 'fqbn'; import { defaultAsyncWorkers, maxAsyncWorkers, @@ -158,11 +158,11 @@ export class InoLanguage extends SketchContribution { this.notificationCenter.onDidReinitialize(() => forceRestart()), this.boardDataStore.onDidChange((event) => { if (this.languageServerFqbn) { - const sanitizedFqbn = new FQBN(this.languageServerFqbn).sanitize(); + const sanitizedFQBN = sanitizeFqbn(this.languageServerFqbn); // The incoming FQBNs might contain custom boards configs, sanitize them before the comparison. // https://github.com/arduino/arduino-ide/pull/2113#pullrequestreview-1499998328 - const matchingChange = event.changes.find((change) => - new FQBN(change.fqbn).sanitize().equals(sanitizedFqbn) + const matchingChange = event.changes.find( + (change) => sanitizedFQBN === sanitizeFqbn(change.fqbn) ); const { boardsConfig } = this.boardsServiceProvider; if ( diff --git a/arduino-ide-extension/src/common/protocol/boards-service.ts b/arduino-ide-extension/src/common/protocol/boards-service.ts index a59865c5a..a6c2ae06a 100644 --- a/arduino-ide-extension/src/common/protocol/boards-service.ts +++ b/arduino-ide-extension/src/common/protocol/boards-service.ts @@ -541,6 +541,15 @@ export namespace Board { } } +/** + * Converts the `VENDOR:ARCHITECTURE:BOARD_ID[:MENU_ID=OPTION_ID[,MENU2_ID=OPTION_ID ...]]` FQBN to + * `VENDOR:ARCHITECTURE:BOARD_ID` format. + * See the details of the `{build.fqbn}` entry in the [specs](https://arduino.github.io/arduino-cli/latest/platform-specification/#global-predefined-properties). + */ +export function sanitizeFqbn(fqbn: string): string { + return new FQBN(fqbn).sanitize().toString(); +} + export type PlatformIdentifier = Readonly<{ vendorId: string; arch: string }>; export function createPlatformIdentifier( board: BoardWithPackage diff --git a/arduino-ide-extension/src/test/browser/board-service-provider.test.ts b/arduino-ide-extension/src/test/browser/board-service-provider.test.ts index 54d3aa8ba..dd733a2c6 100644 --- a/arduino-ide-extension/src/test/browser/board-service-provider.test.ts +++ b/arduino-ide-extension/src/test/browser/board-service-provider.test.ts @@ -170,6 +170,36 @@ describe('board-service-provider', () => { expect(events).deep.equals([expectedEvent]); }); + it('should ignore custom board configs from the FQBN', () => { + boardsServiceProvider['_boardsConfig'] = { + selectedBoard: uno, + selectedPort: unoSerialPort, + }; + const events: BoardsConfigChangeEvent[] = []; + toDisposeAfterEach.push( + boardsServiceProvider.onBoardsConfigDidChange((event) => + events.push(event) + ) + ); + const mkr1000WithCustomOptions = { + ...mkr1000, + fqbn: `${mkr1000.fqbn}:c1=v1`, + }; + const didUpdate = boardsServiceProvider.updateConfig( + mkr1000WithCustomOptions + ); + expect(didUpdate).to.be.true; + const expectedEvent: BoardIdentifierChangeEvent = { + previousSelectedBoard: uno, + selectedBoard: mkr1000WithCustomOptions, // the even has the custom board options + }; + expect(events).deep.equals([expectedEvent]); + // the persisted state does not have the config options property + expect(boardsServiceProvider.boardsConfig.selectedBoard?.fqbn).to.equal( + mkr1000.fqbn + ); + }); + it('should not update the board if did not change (board identifier)', () => { boardsServiceProvider['_boardsConfig'] = { selectedBoard: uno, diff --git a/arduino-ide-extension/src/test/browser/boards-data-store.test.ts b/arduino-ide-extension/src/test/browser/boards-data-store.test.ts index 8800b3ffd..2ed808ad1 100644 --- a/arduino-ide-extension/src/test/browser/boards-data-store.test.ts +++ b/arduino-ide-extension/src/test/browser/boards-data-store.test.ts @@ -15,11 +15,14 @@ import { DisposableCollection, } from '@theia/core/lib/common/disposable'; import { MessageService } from '@theia/core/lib/common/message-service'; -import { wait } from '@theia/core/lib/common/promise-util'; +import { wait, waitForEvent } from '@theia/core/lib/common/promise-util'; import { Container, ContainerModule } from '@theia/core/shared/inversify'; import { expect } from 'chai'; import { BoardsDataStore } from '../../browser/boards/boards-data-store'; -import { BoardsServiceProvider } from '../../browser/boards/boards-service-provider'; +import { + BoardsServiceProvider, + UpdateBoardsConfigParams, +} from '../../browser/boards/boards-service-provider'; import { NotificationCenter } from '../../browser/notification-center'; import { BoardDetails, @@ -30,6 +33,7 @@ import { } from '../../common/protocol/boards-service'; import { NotificationServiceServer } from '../../common/protocol/notification-service'; import { bindBrowser } from './browser-test-bindings'; +import { unoSerialPort } from '../common/fixtures'; disableJSDOM(); @@ -438,6 +442,220 @@ describe('boards-data-store', function () { }); }); + it('should select multiple config options', async () => { + // reconfigure the board details mock for this test case to have multiple config options + toDisposeAfterEach.push( + mockBoardDetails([ + { + fqbn, + ...baseDetails, + configOptions: [configOption1, configOption2], + }, + ]) + ); + + let data = await boardsDataStore.getData(fqbn); + expect(data).to.be.deep.equal({ + configOptions: [configOption1, configOption2], + programmers: [edbg, jlink], + }); + + let didChangeCounter = 0; + toDisposeAfterEach.push( + boardsDataStore.onDidChange(() => didChangeCounter++) + ); + const result = await boardsDataStore.selectConfigOption({ + fqbn, + optionsToUpdate: [ + { + option: configOption1.option, + selectedValue: configOption1.values[1].value, + }, + { + option: configOption2.option, + selectedValue: configOption2.values[1].value, + }, + ], + }); + expect(result).to.be.ok; + expect(didChangeCounter).to.be.equal(1); + + data = await boardsDataStore.getData(fqbn); + expect(data).to.be.deep.equal({ + configOptions: [ + { + ...configOption1, + values: [ + { label: 'C1V1', selected: false, value: 'v1' }, + { label: 'C1V2', selected: true, value: 'v2' }, + ], + }, + { + ...configOption2, + values: [ + { label: 'C2V1', selected: false, value: 'v1' }, + { label: 'C2V2', selected: true, value: 'v2' }, + ], + }, + ], + programmers: [edbg, jlink], + }); + }); + + it('should emit a did change event when updating with multiple config options and at least one of them is known (valid option + valid value)', async () => { + // reconfigure the board details mock for this test case to have multiple config options + toDisposeAfterEach.push( + mockBoardDetails([ + { + fqbn, + ...baseDetails, + configOptions: [configOption1, configOption2], + }, + ]) + ); + + let data = await boardsDataStore.getData(fqbn); + expect(data).to.be.deep.equal({ + configOptions: [configOption1, configOption2], + programmers: [edbg, jlink], + }); + + let didChangeCounter = 0; + toDisposeAfterEach.push( + boardsDataStore.onDidChange(() => didChangeCounter++) + ); + const result = await boardsDataStore.selectConfigOption({ + fqbn, + optionsToUpdate: [ + { + option: 'an unknown option', + selectedValue: configOption1.values[1].value, + }, + { + option: configOption1.option, + selectedValue: configOption1.values[1].value, + }, + { + option: configOption2.option, + selectedValue: 'an unknown value', + }, + ], + }); + expect(result).to.be.ok; + expect(didChangeCounter).to.be.equal(1); + + data = await boardsDataStore.getData(fqbn); + expect(data).to.be.deep.equal({ + configOptions: [ + { + ...configOption1, + values: [ + { label: 'C1V1', selected: false, value: 'v1' }, + { label: 'C1V2', selected: true, value: 'v2' }, + ], + }, + configOption2, + ], + programmers: [edbg, jlink], + }); + }); + + it('should not emit a did change event when updating with multiple config options and all of the are unknown', async () => { + let data = await boardsDataStore.getData(fqbn); + expect(data).to.be.deep.equal({ + configOptions: [configOption1], + programmers: [edbg, jlink], + }); + + let didChangeCounter = 0; + toDisposeAfterEach.push( + boardsDataStore.onDidChange(() => didChangeCounter++) + ); + const result = await boardsDataStore.selectConfigOption({ + fqbn, + optionsToUpdate: [ + { + option: 'an unknown option', + selectedValue: configOption1.values[1].value, + }, + { + option: configOption1.option, + selectedValue: 'an unknown value', + }, + ], + }); + expect(result).to.be.not.ok; + expect(didChangeCounter).to.be.equal(0); + + data = await boardsDataStore.getData(fqbn); + expect(data).to.be.deep.equal({ + configOptions: [configOption1], + programmers: [edbg, jlink], + }); + }); + + it("should automatically update the selected config options if the boards config change 'reason' is the 'toolbar' and the (CLI) detected FQBN has config options", async () => { + // reconfigure the board details mock for this test case to have multiple config options + toDisposeAfterEach.push( + mockBoardDetails([ + { + fqbn, + ...baseDetails, + configOptions: [configOption1, configOption2], + }, + ]) + ); + + let data = await boardsDataStore.getData(fqbn); + expect(data).to.be.deep.equal({ + configOptions: [configOption1, configOption2], + programmers: [edbg, jlink], + }); + + let didChangeCounter = 0; + toDisposeAfterEach.push( + boardsDataStore.onDidChange(() => didChangeCounter++) + ); + + const boardsConfig = { + selectedPort: unoSerialPort, // the port value does not matter here, but the change must come from a toolbar as a boards config: with port+board, + selectedBoard: { + fqbn: `${board.fqbn}:${configOption1.option}=${configOption1.values[1].value},${configOption2.option}=${configOption2.values[1].value}`, + name: board.name, + }, + }; + const params: UpdateBoardsConfigParams = { + ...boardsConfig, + reason: 'toolbar', + }; + const updated = boardsServiceProvider.updateConfig(params); + expect(updated).to.be.ok; + + await waitForEvent(boardsDataStore.onDidChange, 100); + + expect(didChangeCounter).to.be.equal(1); + data = await boardsDataStore.getData(fqbn); + expect(data).to.be.deep.equal({ + configOptions: [ + { + ...configOption1, + values: [ + { label: 'C1V1', selected: false, value: 'v1' }, + { label: 'C1V2', selected: true, value: 'v2' }, + ], + }, + { + ...configOption2, + values: [ + { label: 'C2V1', selected: false, value: 'v1' }, + { label: 'C2V2', selected: true, value: 'v2' }, + ], + }, + ], + programmers: [edbg, jlink], + }); + }); + it('should not select a config option if the option is absent', async () => { const fqbn = 'a:b:c'; let data = await boardsDataStore.getData(fqbn);