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.

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

Jump to bottom

#854 fix platform installation only offered if port is selected #1130

Merged
merged 3 commits into from
Jul 6, 2022
Merged

#854 fix platform installation only offered if port is selected #1130

merged 3 commits into from
Jul 6, 2022

Conversation

davegarthsimpson
Copy link
Contributor

@davegarthsimpson davegarthsimpson commented Jun 30, 2022 •
edited by ubidefeo
Loading

Motivation

Users reported that when selecting a board without a port, the auto-install prompt was erroneously not being shown.

Change description

Ensures we are showing the prompt when desired, whilst satisfying the aim of #485 which sought to prevent an "already closed" auto-install prompt being shown when a board is unplugged. Also includes refactoring to improve readability and logic modularity.

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)

ubidefeo
ubidefeo reviewed Jun 30, 2022
View reviewed changes
Copy link

@ubidefeo ubidefeo left a comment

Choose a reason for hiding this comment

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

I don't get the prompt to install the board support on Mac OS 12.3 Monterey

Also Mac having the standard Bluetooth port it gets selected automatically so testing this is not really possible
CleanShot 2022-06-30 at 16 11 16@2x

@davegarthsimpson
Copy link
Contributor Author

davegarthsimpson commented Jun 30, 2022 •
edited
Loading

I don't get the prompt to install the board support on Mac OS 12.3 Monterey

Also Mac having the standard Bluetooth port it gets selected automatically so testing this is not really possible

Hi @ubidefeo. In this case I think you're not seeing the prompt because of another issue this one, which I have also resolved but in another PR. Bear in mind the issues overlap, so testing for resolution of 979 requires a port is selected due to the PR that this issue is related to.

p.s. no worries for testing this one if you're unable to have "no port selected" on this issue.

@davegarthsimpson davegarthsimpson requested review from ubidefeo and removed request for ubidefeo June 30, 2022 14:53
@kittaakos kittaakos self-requested a review June 30, 2022 16:06
@per1234 per1234 linked an issue Jul 1, 2022 that may be closed by this pull request
Platform installation only offered if port is selected #854
Closed
per1234
per1234 approved these changes Jul 1, 2022
View reviewed 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.

I have verified that this fixes #854, while also not resulting in a regression of #1100

Thanks Dave!

@per1234 per1234 added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Jul 1, 2022
@kittaakos kittaakos mentioned this pull request Jul 4, 2022
Closed
3 tasks
kittaakos
kittaakos reviewed Jul 4, 2022
View reviewed 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.

From time to time, it does not work. I uploaded a screencast here.

I have the avr platform installed, and when I select ATmega* board, the installer does not propose anything.

arduino-ide-extension/src/browser/boards/boards-auto-installer.ts Outdated Show resolved Hide resolved
arduino-ide-extension/src/browser/boards/boards-auto-installer.ts Show resolved Hide resolved
arduino-ide-extension/src/browser/boards/boards-service-provider.ts Outdated Show resolved Hide resolved
arduino-ide-extension/src/browser/boards/boards-auto-installer.ts Outdated Show resolved Hide resolved
arduino-ide-extension/src/browser/boards/boards-auto-installer.ts Outdated Show resolved Hide resolved
@davegarthsimpson
Copy link
Contributor Author

From time to time, it does not work. I uploaded a screencast here.

I have the avr platform installed, and when I select ATmega* board, the installer does not propose anything.

Hey @kittaakos this might be for the same issue that I responded to with @ubidefeo above. I'll merge main into this to verify.

@kittaakos
Copy link
Contributor

I thought the missing ports were due to this:

root ERROR Uncaught Exception:  Error: 14 UNAVAILABLE: Error starting board discoveries: []
root ERROR Error: 14 UNAVAILABLE: Error starting board discoveries: []
    at Object.callErrorFromStatus (/Users/a.kitta/dev/git/arduino-ide/node_modules/@grpc/grpc-js/build/src/call.js:31:26)
    at Object.onReceiveStatus (/Users/a.kitta/dev/git/arduino-ide/node_modules/@grpc/grpc-js/build/src/client.js:409:49)
    at Object.onReceiveStatus (/Users/a.kitta/dev/git/arduino-ide/node_modules/@grpc/grpc-js/build/src/client-interceptors.js:328:181)
    at /Users/a.kitta/dev/git/arduino-ide/node_modules/@grpc/grpc-js/build/src/call-stream.js:187:78
    at processTicksAndRejections (node:internal/process/task_queues:78:11)

My issue seemed to be unrelated.

@davegarthsimpson
Copy link
Contributor Author

I thought the missing ports were due to this:

I was referring to this:

Hi @ubidefeo. In this case I think you're not seeing the prompt because of another issue this one, which I have also resolved but in another PR. Bear in mind the issues overlap, so testing for resolution of 979 requires a port is selected due to the PR that this issue is related to.

@kittaakos
Copy link
Contributor

From time to time, it does not work.

It looks OK now. Thank you!

davegarthsimpson
davegarthsimpson commented Jul 4, 2022
View reviewed changes
arduino-ide-extension/src/browser/boards/boards-service-provider.ts
@@ -209,7 +220,7 @@ export class BoardsServiceProvider implements FrontendApplicationContribution {
}
}

protected async tryReconnect(): Promise<boolean> {
protected tryReconnect(): boolean {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

couldn't see a reason for this to be async, it's not called across the wire either

kittaakos
kittaakos approved these changes Jul 6, 2022
View reviewed 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.

I tried it, and it worked. Thank you!

@davegarthsimpson davegarthsimpson merged commit 7f2b849 into main Jul 6, 2022
@davegarthsimpson davegarthsimpson deleted the 854-platform-install-prompt branch July 6, 2022 06:38
@Defragster Defragster mentioned this pull request Sep 6, 2022
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ubidefeo ubidefeo ubidefeo left review comments

@per1234 per1234 per1234 approved these changes

@kittaakos kittaakos kittaakos approved these changes

@fstasi fstasi Awaiting requested review from fstasi

Assignees
No one assigned
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Platform installation only offered if port is selected
4 participants
@davegarthsimpson @kittaakos @ubidefeo @per1234