Search code, repositories, users, issues, pull requests...

Copy link

Contributor

@AlbyIanna AlbyIanna commented Oct 4, 2022 •

edited

Loading

Motivation

The IDE needs to sort the menu for each platform in Tools > Board (e.g., Tools > Board > Arduino AVR Boards) according to the order of occurrence of the name property for each board definitions in the platform's boards.txt configuration file.

Change description

use listall instead of search command in the arduino-cli
add a proper order to the menu item when registering a board
update the arduino-cli to use the sorting fix

Other information

Fixes #802.

This needed a fix in the CLI to get the correct order: arduino/arduino-cli#1903

This PR is in draft because there isn't a release for the arduino-cli yet, but it can already be tested.

Arduino IDE 2.x before this change:
image

Arduino IDE 2.x after this change:
Screenshot 2022年10月04日 at 10 51 36

Arduino IDE 1.x:
Screenshot 2022年10月04日 at 10 51 46

Reviewer checklist

PR addresses a single concern.
The PR has no duplicates (please search among the Pull Requests before creating one)
PR title and description are properly filled.
Docs have been added / updated (for bug fixes / features)


 Fix boards listing

3546f20

@AlbyIanna AlbyIanna requested review from davegarthsimpson, francescospissu and per1234

October 4, 2022 09:15

@kittaakos kittaakos self-requested a review

October 4, 2022 11:18

kittaakos

kittaakos suggested changes

Copy link

Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work. If IDE2 uses board list instead of board search, then only boards from installed platforms will be searched.

Steps to reproduce:

Delete ⚠️ (or rename) the directories/data location,
Start IDE2 from this PR,
Use the board search.

board-search.mp4

@kittaakos kittaakos added topic: code type: imperfection labels

@AlbyIanna AlbyIanna force-pushed the fix-board-listing branch from 8f9ab6f to 10d2662 Compare

October 4, 2022 13:15

Copy link

Contributor Author

AlbyIanna commented Oct 4, 2022

This won't work. If IDE2 uses board list instead of board search, then only boards from installed platforms will be searched.

Right. I've fixed it by adding a separate method to get only the installed boards.

@AlbyIanna AlbyIanna requested a review from kittaakos

October 4, 2022 13:15

kittaakos

kittaakos suggested changes

https://github.com/arduino/arduino-ide/pull/1520/files#diff-c0fc7c4dd9afbf95ea0168ace3b7e01e84078a50ba6cd54896f8a095f83f30ffL202-R225

Copy link

Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please reuse the code instead of copy/pasting it.

@AlbyIanna AlbyIanna requested a review from kittaakos

October 4, 2022 14:50

Copy link

Contributor Author

AlbyIanna commented Oct 4, 2022

Please reuse the code instead of copy/pasting it.

Done

@kittaakos

Copy link

Contributor

kittaakos commented Oct 4, 2022

Done

Thank you! I will do the verification soon. Which issue does this PR suppose to close? I could not find it in the commits or PR description. The closest was #801.

Copy link

Contributor Author

AlbyIanna commented Oct 4, 2022

The closest was #801.

Pretty close, it fixes #802. Added to the PR's description.

per1234

per1234 requested changes

Copy link

Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order of the boards is not correct for me:

image

Arduino IDE version

2.0.1-snapshot-3f7230e (tester build for 56f2ac3)

Operating system

Windows 10, Ubuntu 20.04

Additional context

The order is as expected when using Arduino CLI:

$ arduino-cli version
arduino-cli.exe Version: git-snapshot Commit: 3d5a87e1 Date: 2022年10月04日T18:01:41Z
$ arduino-cli board listall arduino:avr --format json | jq '.boards[] | .name'
"Arduino Yún"
"Arduino Uno"
"Arduino Uno Mini"
"Arduino Duemilanove or Diecimila"
"Arduino Nano"
"Arduino Mega or Mega 2560"
"Arduino Mega ADK"
"Arduino Leonardo"
"Arduino Leonardo ETH"
"Arduino Micro"
"Arduino Esplora"
"Arduino Mini"
"Arduino Ethernet"
"Arduino Fio"
"Arduino BT"
"LilyPad Arduino USB"
"LilyPad Arduino"
"Arduino Pro or Pro Mini"
"Arduino NG or older"
"Arduino Robot Control"
"Arduino Robot Motor"
"Arduino Gemma"
"Adafruit Circuit Playground"
"Arduino Yún Mini"
"Arduino Industrial 101"
"Linino One"
"Arduino Uno WiFi"

kittaakos

kittaakos suggested changes

Oct 5, 2022

Copy link

Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have noticed that sometimes the order of installed boards does not match with IDE 1.x Sometimes it's correct.

IDE2 SAMD:
Screen Shot 2022年10月05日 at 16 47 00

IDE 1.x SAMD:
Screen Shot 2022年10月05日 at 16 45 50

IDE2 AVR:
Screen Shot 2022年10月05日 at 16 47 10

IDE 1.x AVR:
Screen Shot 2022年10月05日 at 16 47 24

arduino-ide-extension/src/node/boards-service-impl.ts Outdated

return this.handleListBoards(client.boardListAll.bind(client), req);

}

async handleListBoards(

Copy link

Contributor

@kittaakos kittaakos Oct 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The visibility should be private if it's not called from outside of the module.

kittaakos

kittaakos reviewed

Oct 5, 2022

arduino-ide/arduino-ide-extension/src/browser/boards/boards-data-menu-updater.ts

arduino-ide-extension/src/browser/contributions/board-selection.ts Outdated

const menuAction = {

commandId: id,

label: name,

order: `${index}`,

Copy link

Contributor

@kittaakos kittaakos Oct 5, 2022 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can check them together, but I think this is the classic alphanumeric ordering issue.

% node 
Welcome to Node.js v14.19.0.
Type ".help" for more information.
> ['1', '2', '3', '11', '12', '111'].sort()
[ '1', '11', '111', '12', '2', '3' ]
> .exit

With some zero padding, it works for me locally:

diff --git a/arduino-ide-extension/src/browser/contributions/board-selection.ts b/arduino-ide-extension/src/browser/contributions/board-selection.ts
index 2241c925..466e4f63 100644
--- a/arduino-ide-extension/src/browser/contributions/board-selection.ts
+++ b/arduino-ide-extension/src/browser/contributions/board-selection.ts
@@ -243,7 +243,7 @@ PID: ${PID}`;
 const menuAction = {
 commandId: id,
 label: name,
- order: `${index}`,
+ order: String(index).padStart(4), // pads with leading zeros for alphanumeric sort where order is 1, 2, 11, and NOT 1, 11, 2
 };
 this.commandRegistry.registerCommand(command, handler);
 this.toDisposeBeforeMenuRebuild.push(

Great that your changes revealed the flaw in the codebase. These are probably all incorrect:

Line 114 in 61a11a0

order: `${i}`,

arduino-ide/arduino-ide-extension/src/browser/contributions/sketch-control.ts

Line 179 in 61a11a0

order: `${i}`,

👇 This probably works only because the open recent is limited to ten items.

arduino-ide/arduino-ide-extension/src/browser/contributions/open-recent-sketch.ts

Line 96 in 61a11a0

order: String(order),

Copy link

Contributor Author

@AlbyIanna AlbyIanna Oct 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one! Thank you for the suggestion!

Copy link

Contributor Author

@AlbyIanna AlbyIanna Oct 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed the suggestion, could you please verify it now?

@kittaakos kittaakos mentioned this pull request

Oct 6, 2022

Please make the output of the board listall --format json deterministic arduino/arduino-cli#1909

Closed

3 tasks

Alberto Iannaccone added 2 commits

October 6, 2022 09:37


 use arduio-cli sorting fix

dd0205c


 re-use code to handle board list response

2d9b072

@AlbyIanna AlbyIanna force-pushed the fix-board-listing branch from 56f2ac3 to 2d9b072 Compare

October 6, 2022 07:38

Alberto Iannaccone added 2 commits

October 6, 2022 10:55


 change handleListBoards visibility to private

36593e9


 pad menu items order with leading zeros to fix alphanumeric order

7d4a977

@kittaakos

Copy link

Contributor

kittaakos commented Oct 7, 2022

@AlbyIanna, I have restarted the failed job. If you mark the PR ready, I will do the final verification. The code looks great. Thanks!

per1234

per1234 approved these changes

Oct 7, 2022

Copy link

Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is working perfectly for me now.

Thanks Alberto!

@AlbyIanna AlbyIanna marked this pull request as ready for review

October 7, 2022 12:11

@AlbyIanna AlbyIanna requested a review from kittaakos

October 10, 2022 07:31

kittaakos

kittaakos approved these changes

Oct 17, 2022