Skip to content

fix #1383: missing port labels #1412

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.

Sign up for GitHub

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 3 commits into from
Sep 15, 2022
Merged

fix #1383: missing port labels #1412

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a6a7af4
fallback to port.address if addressLabel is false
davegarthsimpson Sep 7, 2022
0b0ce7b
addressLabel if possible, reconcileAvailableBoards
davegarthsimpson Sep 8, 2022
d3f29cb
remove fallback
davegarthsimpson Sep 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion arduino-ide-extension/src/browser/boards/boards-toolbar-item.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.

</div>
</div>
{selected ? <div className="fa fa-check" /> : ''}
Expand Down