fix: custom board options handling in data store

Signed-off-by: Akos Kitta <[email protected]>

Author: Akos Kitta

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<boolean> {
-    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<ConfigValue> = 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<ConfigValue>)
+            .forEach((value) => (value.selected = false));
+          const mutableConfigValue: Mutable<ConfigValue> =
+            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;
   }
 

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<BoardIdentifier> = 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;

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 (

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

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,

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);