Skip to content

Put Arduino libs on top of the Library Manager #1541

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 14, 2022

Conversation

francescospissu
Copy link
Contributor

Motivation

Libraries maintained by Arduino should be more visible in the Library Manager.

Change description

Put the libraries maintained by Arduino on top of the Library Manager, in this way when looking for a library those made by Arduino are the first to show up.

Other information

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)

@francescospissu francescospissu added type: enhancement Proposed improvement topic: code Related to content of the project itself labels Oct 6, 2022
@@ -456,5 +456,6 @@ function toLibrary(
summary: lib.getParagraph(),
category: lib.getCategory(),
types: lib.getTypesList(),
isArduinoMaintained: lib.getAuthor() === 'Arduino', // TODO check if .getMaintainer is more appropriate
Copy link
Contributor

Choose a reason for hiding this comment

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

This information is redundant, as it is already present in author. Would you please move this check (lib.getAuthor() === 'Arduino') inside the comparator function?

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be based on the types field:

https://arduino.github.io/arduino-cli/dev/rpc/commands/#library

We intentionally don't place any restrictions on specifying "Arduino" as author because this is appropriate to give attribution for forks of the official Arduino libraries.

We do have rules about the use of "Arduino" in the maintainer field:

https://arduino.github.io/arduino-lint/dev/rules/library/#maintainer-starts-with-arduino-lp027

But due to the legacy of not enforcing this over the history of the Arduino Library Manager violation of these rules are only warnings at the "permissive" compliance level used by the Library Manager indexer engine.

But the types field is under Arduino's complete control so we can be sure that when a library has the value Arduino, it is an official library (unfortunately this has not been applied consistently, so some official libraries also don't have this categorization, but that is something to fix in the registry, not in the IDE.

@@ -456,5 +456,6 @@ function toLibrary(
summary: lib.getParagraph(),
category: lib.getCategory(),
types: lib.getTypesList(),
isArduinoMaintained: lib.getAuthor() === 'Arduino', // TODO check if .getMaintainer is more appropriate
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be based on the types field:

https://arduino.github.io/arduino-cli/dev/rpc/commands/#library

We intentionally don't place any restrictions on specifying "Arduino" as author because this is appropriate to give attribution for forks of the official Arduino libraries.

We do have rules about the use of "Arduino" in the maintainer field:

https://arduino.github.io/arduino-lint/dev/rules/library/#maintainer-starts-with-arduino-lp027

But due to the legacy of not enforcing this over the history of the Arduino Library Manager violation of these rules are only warnings at the "permissive" compliance level used by the Library Manager indexer engine.

But the types field is under Arduino's complete control so we can be sure that when a library has the value Arduino, it is an official library (unfortunately this has not been applied consistently, so some official libraries also don't have this categorization, but that is something to fix in the registry, not in the IDE.

@kittaakos
Copy link
Contributor

kittaakos commented Oct 7, 2022

As discussed offline, please make the following changes:

  • also boost the platforms where types.includes('Arduino') so that the IDE2 UX is the same for platforms and libs,
  • you can remove the superfluous isArduinoMaintained from the ArduinoCompoent and rely on the types prop,
  • for consistency with the other class-based React components in IDE2, please use a function property, not a function. See the details in the Theia Coding-guidelines for React here:
class MyComponent extends Component {
  constructor() {
    this.filterableListSort = this.filterableListSort.bind(this);
  }
  protected filterableListSort(items: T[]): T[] {
    // code
  }
}
class MyComponent extends Component {
  constructor() { }
  protected filterableListSort = (items: T[]): T[] => {
    // code
  }
}

Please let me know if there is anything you disagree with or are unclear about. Thank you!


Update: pipe-dream:

Probably, in the long run, we should not use an API like sort: (items: T[]) => T[], but an API like score(item: T): number, and then sort by the score. Then, IDE2 could boost exact matches, etc. The API change would not be limited to a single use case, but it would be easy to extend.

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.

It works for both libs and platforms. I have tried the changes from the sources locally.

Thank you!

Copy link
Contributor

@AlbyIanna AlbyIanna left a comment

Choose a reason for hiding this comment

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

Code LGTM

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.

There is a possible regression in the handling of "deprecated" platforms.

Arduino boards platforms listed in a package index may be marked as deprecated by setting the packages[*].platforms[*].deprecated field to true.

In order to encourage users to select replacement platforms instead of the deprecated one, platforms marked as "deprecated" should be shown at the bottom of the Boards Manager listing:

https://arduino.github.io/arduino-cli/dev/package_index_json-specification/#platforms-definitions:~:text=causes%20the%20platform%20to%20be%20moved%20to%20the%20bottom

deprecated: (optional) setting to true causes the platform to be moved to the bottom of all Boards Manager and arduino-cli core listings

An example is the "[DEPRECATED - Please install standalone packages] Arduino Mbed OS Boards" boards platform (machine identifier arduino:mbed).

The changes made in this PR cause the deprecated platform to no longer be shown at the bottom of the Boards Manager listings, but instead at the bottom of the segment of the listings for its type (e.g., Arduino, Contributed).

I'm not sure this is necessarily wrong, but it does not match a strict interpretation of the described behavior in the specification ("all Boards Manager [...] listings"), and also does not match the established behavior:

Arduino IDE 2.x:

image

Arduino IDE 1.x:

(reference: arduino/Arduino#11508)

image

Arduino CLI:

(reference: arduino/arduino-cli#1278)

$ arduino-cli core list --all
ID                    Installed Latest    Name
arduino:avr           1.8.5     1.8.5     Arduino AVR Boards
arduino:mbed_edge               3.3.0     Arduino Mbed OS Edge Boards
arduino:mbed_nano               3.3.0     Arduino Mbed OS Nano Boards
arduino:mbed_nicla              3.3.0     Arduino Mbed OS Nicla Boards
arduino:mbed_portenta           3.3.0     Arduino Mbed OS Portenta Boards
arduino:mbed_rp2040             3.3.0     Arduino Mbed OS RP2040 Boards
arduino:megaavr                 1.8.7     Arduino megaAVR Boards
arduino:nrf52                   1.0.2     Arduino nRF52 Boards
arduino:sam                     1.6.12    Arduino SAM Boards (32-bits ARM Cortex-M3)
arduino:samd                    1.8.13    Arduino SAMD Boards (32-bits ARM Cortex-M0+)
Arrow:samd                      2.1.0     Arrow Boards
atmel-avr-xminis:avr            0.6.0     Atmel AVR Xplained-minis
emoro:avr                       3.2.2     EMORO 2560
industruino:samd                1.0.1     Industruino SAMD Boards (32-bits ARM Cortex-M0+)
Intel:arc32                     2.0.5     Intel Curie Boards
Intel:i586                      1.6.7+1.0 Intel i586 Boards
Intel:i686                      1.6.7+1.0 Intel i686 Boards
littleBits:avr                  1.0.0     littleBits Arduino AVR Modules
Microsoft:win10                 1.1.2     Windows 10 Iot Core
arduino:mbed                    3.3.0     [DEPRECATED] [DEPRECATED - Please install standalone packages] Arduino Mbed OS Boards

To reproduce

  1. Open the Boards Manager view.
  2. Clear any search queries and filters to get the full list.
  3. Scroll down in the unfiltered listings until you find "[DEPRECATED - Please install standalone packages] Arduino Mbed OS Boards by Arduino"

🐛 The platform is listed above other non-deprecated items:

image

Arduino IDE version

2.0.1-snapshot-a0e7fd5 (tester build for 9f82175)

Operating system

Windows

Operating system version

10

@davegarthsimpson
Copy link
Contributor

davegarthsimpson commented Oct 7, 2022

Regarding this

This information is redundant, as it is already present in author. Would you please move this check (lib.getAuthor() === 'Arduino') inside the comparator function?

For me having the necessary check done and then stored as a library property in the toLibrary function keeps the comparator function cleaner, and might avoid additional computation (in the case of checking .types with .includes):

  1. If the same info is needed elsewhere;
  2. If I've thought it through correctly, having the check .includes('Arduino') in the comparator function could result in more computation, doing it in toLibrary it's one .includes per library, within the JavaScript .sort it would be more;

This is of course comparing teeny overheads (.includes vs Boolean evaluation in a comparator) and probably not worth debating, I just wanted to note that I wouldn't necessarily consider putting the .includes in toLibrary as suboptimal.

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 Francesco!

@francescospissu francescospissu merged commit e577de4 into main Oct 14, 2022
@francescospissu francescospissu deleted the fspissu/arduino-libs-above branch October 14, 2022 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants