Skip to content
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

fix #1383: missing port labels #1412

Merged
merged 3 commits into from
Sep 15, 2022
Merged

Conversation

davegarthsimpson
Copy link
Contributor

@davegarthsimpson davegarthsimpson commented Sep 7, 2022

Motivation

Reported that upon opening a new sketch port labels can go missing for "Unknown board ports"

Change description

Ensures that we do not lose addressLabel(s) in "availableBoard objects" when reconciling our available boards.

Issue Identified

Described below.

Closes #1383

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)

@davegarthsimpson davegarthsimpson changed the title fallback to port.address if addressLabel is false fix #1383: missing port labels Sep 7, 2022
@@ -138,7 +138,7 @@ export class BoardsDropDown extends React.Component<BoardsDropDown.Props> {
{boardLabel}
</div>
<div className="arduino-boards-dropdown-item--port-label noWrapInfo noselect">
{port.addressLabel}
{port.addressLabel || port.address}
Copy link
Contributor Author

@davegarthsimpson davegarthsimpson Sep 7, 2022

Choose a reason for hiding this comment

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

maybe this is not appropriate, I imagine it depends on whether or not .addressLabel was intended to be a "catch all" or not, for the sake of efficiency I've plugged this quick fix with a question:

@kittaakos
are you able to confirm if a fallback is appropriate here, or should port.addressLabel be our only reference as asserted in this PR, UPDATE: in that case we could go with the approach below (or change the boardsConfig object to store .addressLabel) Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Were you able to determine why port.addressLabel is empty after opening a new window @davegarthsimpson?

Copy link
Contributor Author

@davegarthsimpson davegarthsimpson Sep 8, 2022

Choose a reason for hiding this comment

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

Were you able to determine why port.addressLabel is empty after opening a new window

Yes, thanks for the important question, it's due to a chunk of code in our reconcileAvailableBoards method of the boards-service-provider here where we set the port in question by copying the boardsConfig.selectedPort object which does not contain .addressLabel only .address, this comment explains the reason why the author wrote this logic generally.

Copy link
Contributor Author

@davegarthsimpson davegarthsimpson Sep 8, 2022

Choose a reason for hiding this comment

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

The other alternative would be something like this in the "culprit code":

before:

581     ...
        const found = availableBoards.findIndex(
           (board) => board.port?.address === boardsConfig.selectedPort?.address
         );
         if (found >= 0) {
           availableBoards.splice(found, 1);
         }
         availableBoards.push({
           ...boardsConfig.selectedBoard,
           port: boardsConfig.selectedPort,
           selected: true,
           state: AvailableBoard.State.incomplete,
         });

after:

581     let port = boardsConfig.selectedPort
         ...
         const found = availableBoards.findIndex(
           (board) => board.port?.address === boardsConfig.selectedPort?.address
         );
         if (found >= 0) {
           port = availableBoards[found].port // get the "Unknown board port" that we will substitute
           availableBoards.splice(found, 1);
         }
         availableBoards.push({
           ...boardsConfig.selectedBoard,
           port, // "copy" the port info here where we'll have an "addressLabel" too
           selected: true,
           state: AvailableBoard.State.incomplete,
         });

Copy link
Contributor

Choose a reason for hiding this comment

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

That looks like the correct approach to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch; the fix looks good to me. The breaking change was probably here when new props were added to the Port but on board#port.

@davegarthsimpson davegarthsimpson marked this pull request as draft September 7, 2022 17:00
@per1234 per1234 added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Sep 7, 2022
@davegarthsimpson davegarthsimpson self-assigned this Sep 7, 2022
@davegarthsimpson davegarthsimpson added the criticality: medium Of moderate impact label Sep 7, 2022
@davegarthsimpson davegarthsimpson marked this pull request as ready for review September 8, 2022 10:04
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.

I verified this fixes #1383

Thanks Dave!

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.

Thank you!

@davegarthsimpson davegarthsimpson merged commit 026e80e into main Sep 15, 2022
@davegarthsimpson davegarthsimpson deleted the 1383-missing-port-label branch September 15, 2022 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
criticality: medium Of moderate impact topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Label of unidentified port missing in "Board Selector" after opening new window
4 participants