fix: moved UI labels into the model

arduino · Aug 10, 2023 · 6a6329e · 6a6329e
1 parent a57cdff
commit 6a6329e
Show file tree

Hide file tree

Showing 4 changed files with 67 additions and 62 deletions.
diff --git a/arduino-ide-extension/src/browser/boards/boards-toolbar-item.tsx b/arduino-ide-extension/src/browser/boards/boards-toolbar-item.tsx
@@ -12,6 +12,7 @@ import classNames from 'classnames';
 import { boardIdentifierLabel, Port } from '../../common/protocol';
 import {
   BoardListItem,
+  BoardListItemUI,
   InferredBoardListItem,
   isInferredBoardListItem,
 } from '../../common/protocol/board-list';
@@ -105,7 +106,7 @@ export class BoardListDropDown extends React.Component<
     );
   }
 
-  private readonly onDefaultAction = (item: BoardListItem): unknown => {
+  private readonly onDefaultAction = (item: BoardListItemUI): unknown => {
     const { boardList } = this.props;
     const { type, params } = boardList.defaultAction(item);
     switch (type) {
@@ -124,11 +125,10 @@ export class BoardListDropDown extends React.Component<
     item,
     selected,
   }: {
-    item: BoardListItem;
+    item: BoardListItemUI;
     selected: boolean;
   }): React.ReactNode {
-    const { boardLabel, portLabel, portProtocol, tooltip } =
-      this.props.boardList.labelOf(item);
+    const { boardLabel, portLabel, portProtocol, tooltip } = item.labels;
     const port = item.port;
     const onKeyUp = (e: React.KeyboardEvent) => {
       if (e.key === 'Enter') {

diff --git a/arduino-ide-extension/src/common/protocol/board-list.ts b/arduino-ide-extension/src/common/protocol/board-list.ts
@@ -1,5 +1,5 @@
-import { Mutable } from '@theia/core';
-import { Defined } from '../types';
+import type { Mutable } from '@theia/core/lib/common/types';
+import type { Defined } from '../types';
 import { naturalCompare } from '../utils';
 import {
   BoardIdentifier,
@@ -225,11 +225,28 @@ function boardListItemComparator(
   if (!isMultiBoardsBoardListItem(left) && !isMultiBoardsBoardListItem(right)) {
     return 1;
   }
+  // ambiguous boards with a unique board name comes first than other ambiguous ones
+  if (isMultiBoardsBoardListItem(left) && isMultiBoardsBoardListItem(right)) {
+    const leftUniqueName = findUniqueBoardName(left);
+    const rightUniqueName = findUniqueBoardName(right);
+    if (leftUniqueName && !rightUniqueName) {
+      return -1;
+    }
+    if (!leftUniqueName && rightUniqueName) {
+      return 1;
+    }
+    if (leftUniqueName && rightUniqueName) {
+      return naturalCompare(leftUniqueName, rightUniqueName);
+    }
+  }
 
   // fallback compare based on the address
   return naturalCompare(left.port.address, right.port.address);
 }
 
+/**
+ * What is show in the UI with all the refinements, fallbacks, and tooltips.
+ */
 export interface BoardListItemLabels {
   readonly boardLabel: string;
   readonly boardLabelWithFqbn: string;
@@ -238,9 +255,11 @@ export interface BoardListItemLabels {
   readonly tooltip: string;
 }
 
-export function createBoardListItemLabels(
-  item: BoardListItem
-): BoardListItemLabels {
+export interface BoardListItemUI extends BoardListItem {
+  readonly labels: BoardListItemLabels;
+}
+
+function createBoardListItemLabels(item: BoardListItem): BoardListItemLabels {
   const { port } = item;
   const portLabel = port.address;
   const portProtocol = port.protocol;
@@ -271,7 +290,11 @@ export function createBoardListItemLabels(
   };
 }
 
-type BoardListItemOrIndex = BoardListItem | number;
+/**
+ * When it's not a `number`, you must use a reference from `BoardList#items` at call-site.
+ * More formally, `boardList.items.includes(itemOrIndex)` must be `true`.
+ */
+type BoardListItemUIOrIndex = BoardListItemUI | number;
 
 /**
  * A list of boards discovered by the Arduino CLI. With the `board list --watch` gRPC equivalent command,
@@ -282,7 +305,7 @@ export interface BoardList {
   /**
    * All detected ports with zero to many boards and optional inferred information based on historical selection/usage.
    */
-  readonly items: readonly BoardListItem[]; // TODO: expose `labels`
+  readonly items: readonly BoardListItemUI[];
   /**
    * A snapshot of the board and port configuration this board list has been initialized with.
    */
@@ -312,19 +335,10 @@ export interface BoardList {
     Record<'serial' | 'network' | string, ReturnType<BoardList['ports']>>
   >;
 
-  /**
-   * What is show in the UI with all the fallbacks and tooltips.
-   *
-   * _Notes:_ when the argument is not a number, `boardList.items.includes(itemOrIndex)` must be `true`.
-   */
-  labelOf(itemOrIndex: BoardListItemOrIndex): BoardListItemLabels;
-
   /**
    * What's the default action when the argument item is selected in the UI.
-   *
-   * _Notes:_ when the argument is not a number, `boardList.items.includes(itemOrIndex)` must be `true`.
    */
-  defaultAction(itemOrIndex: BoardListItemOrIndex): BoardListItemAction;
+  defaultAction(itemOrIndex: BoardListItemUIOrIndex): BoardListItemAction;
 
   /**
    * For dumping the current state of board list for debugging purposes.
@@ -356,27 +370,28 @@ export function createBoardList(
   boardsConfig: Readonly<BoardsConfig> = emptyBoardsConfig(),
   boardListHistory: BoardListHistory = {}
 ): BoardList {
-  const items: BoardListItem[] = [];
+  const items: BoardListItemUI[] = [];
   for (const detectedPort of Object.values(detectedPorts)) {
     const { port, boards } = detectedPort;
     // boards with arduino vendor should come first
     boards?.sort(boardIdentifierComparator);
     const portKey = Port.keyOf(port);
     const inferredBoard = boardListHistory[portKey];
+    let item: BoardListItem;
     if (!boards?.length) {
-      let item: BoardListItem | InferredBoardListItem = { port };
+      let unknownItem: BoardListItem | InferredBoardListItem = { port };
       // Infer unrecognized boards from the history
       if (inferredBoard) {
-        item = {
-          ...item,
+        unknownItem = {
+          ...unknownItem,
           inferredBoard,
           type: 'manually-selected',
         };
       }
-      items.push(item);
+      item = unknownItem;
     } else if (boards.length === 1) {
       const board = boards[0];
-      let item: BoardListItemWithBoard | InferredBoardListItem = {
+      let detectedItem: BoardListItemWithBoard | InferredBoardListItem = {
         port,
         board,
       };
@@ -385,30 +400,31 @@ export function createBoardList(
         // ignore the inferred item if it's the same as the discovered board
         !boardIdentifierEquals(board, inferredBoard)
       ) {
-        item = {
-          ...item,
+        detectedItem = {
+          ...detectedItem,
           inferredBoard,
           type: 'board-overridden',
         };
       }
-      items.push(item);
+      item = detectedItem;
     } else {
-      let item: MultiBoardsBoardListItem | InferredBoardListItem = {
+      let ambiguousItem: MultiBoardsBoardListItem | InferredBoardListItem = {
         port,
         boards,
       };
       if (inferredBoard) {
-        item = {
-          ...item,
+        ambiguousItem = {
+          ...ambiguousItem,
           inferredBoard,
           type: 'manually-selected',
         };
       }
-      items.push(item);
+      item = ambiguousItem;
     }
+    const labels = createBoardListItemLabels(item);
+    items.push(Object.assign(item, { labels }));
   }
   items.sort(boardListItemComparator);
-  const labels = items.map(createBoardListItemLabels);
   const length = items.length;
   const findSelectedIndex = (): number => {
     if (!isDefinedBoardsConfig(boardsConfig)) {
@@ -521,23 +537,14 @@ export function createBoardList(
         portsOnProtocol.push(detectedPort);
       }
       const matchItem = allPorts[allPorts.matchingIndex];
-      // match index is per all ports, IDE2 needs to adjust it per protocol
+      // the cached match index is per all ports. Here, IDE2 needs to adjust the match index per grouped protocol
       if (matchItem) {
         const matchProtocol = matchItem.port.protocol;
         const matchPorts = result[matchProtocol];
         matchPorts.matchingIndex = matchPorts.indexOf(matchItem);
       }
       return result;
     },
-    labelOf(itemOrIndex) {
-      let index: number | undefined = undefined;
-      if (typeof itemOrIndex === 'number') {
-        index = itemOrIndex;
-      } else {
-        index = items.indexOf(itemOrIndex);
-      }
-      return labels[index];
-    },
     defaultAction(itemOrIndex) {
       let item: BoardListItem | undefined = undefined;
       if (typeof itemOrIndex === 'number') {
@@ -592,7 +599,6 @@ export function createBoardList(
           items,
           selectedIndex,
           boardListHistory,
-          labels,
         },
         null,
         2

diff --git a/arduino-ide-extension/src/common/protocol/boards-service.ts b/arduino-ide-extension/src/common/protocol/boards-service.ts
@@ -1,14 +1,14 @@
 import { nls } from '@theia/core/lib/common/nls';
 import type { MaybePromise } from '@theia/core/lib/common/types';
-import URI from '@theia/core/lib/common/uri';
+import type URI from '@theia/core/lib/common/uri';
 import {
   All,
   Contributed,
   Partner,
   Type as TypeLabel,
   Updatable,
 } from '../nls';
-import { Defined } from '../types';
+import type { Defined } from '../types';
 import { naturalCompare } from './../utils';
 import type { ArduinoComponent } from './arduino-component';
 import type { BoardList } from './board-list';

diff --git a/arduino-ide-extension/src/test/common/board-list.test.ts b/arduino-ide-extension/src/test/common/board-list.test.ts
@@ -2,7 +2,6 @@ import { expect } from 'chai';
 import {
   createBoardList,
   isMultiBoardsBoardListItem,
-  MultiBoardsBoardListItem,
 } from '../../common/protocol/board-list';
 import {
   BoardIdentifier,
@@ -155,14 +154,14 @@ const detectedPorts: DetectedPorts = {
   ...detectedPort(mkr1000NetworkPort, mkr1000),
   ...detectedPort(undiscoveredSerialPort),
   ...detectedPort(undiscoveredUsbToUARTSerialPort),
-  // multiple discovered on the same port with the same board name
-  ...detectedPort(nanoEsp32SerialPort, arduinoNanoEsp32, esp32NanoEsp32),
   // multiple discovered on the same port with different board names
   ...detectedPort(
     nanoEsp32DetectsMultipleEsp32BoardsSerialPort,
     esp32S3DevModule,
     esp32S3Box
   ),
+  // multiple discovered on the same port with the same board name
+  ...detectedPort(nanoEsp32SerialPort, arduinoNanoEsp32, esp32NanoEsp32),
 };
 
 describe('board-list', () => {
@@ -171,19 +170,19 @@ describe('board-list', () => {
       const { items } = createBoardList(detectedPorts);
       expect(items.length).to.be.equal(Object.keys(detectedPorts).length);
       expect(items[0].board).deep.equal(mkr1000);
-      expect(items[1].board).deep.equal(arduinoNanoEsp32);
-      expect(isMultiBoardsBoardListItem(items[1])).to.be.true;
-      expect((<MultiBoardsBoardListItem>items[1]).boards).deep.equal([
-        arduinoNanoEsp32,
-        esp32NanoEsp32,
-      ]);
-      expect(items[2].board).deep.equal(uno);
+      expect(items[1].board).deep.equal(uno);
+      expect(items[2].board).is.undefined;
+      expect(isMultiBoardsBoardListItem(items[2])).to.be.true;
+      const boards2 = isMultiBoardsBoardListItem(items[2])
+        ? items[2].boards
+        : undefined;
+      expect(boards2).deep.equal([arduinoNanoEsp32, esp32NanoEsp32]);
       expect(items[3].board).is.undefined;
       expect(isMultiBoardsBoardListItem(items[3])).to.be.true;
-      expect((<MultiBoardsBoardListItem>items[3]).boards).deep.equal([
-        esp32S3Box, // alphabetic ordering here, `arduino` is not in FQBN
-        esp32S3DevModule,
-      ]);
+      const boards3 = isMultiBoardsBoardListItem(items[3])
+        ? items[3].boards
+        : undefined;
+      expect(boards3).deep.equal([esp32S3Box, esp32S3DevModule]);
       expect(items[4].port).deep.equal(builtinSerialPort);
       expect(items[5].port).deep.equal(bluetoothSerialPort);
       expect(items[6].port).deep.equal(undiscoveredSerialPort);