Show user fields dialog again if upload fails #1415

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

Jump to bottom

Merged

AlbyIanna merged 3 commits into main from show-user-fields-dialog

Sep 20, 2022

Merged

Show user fields dialog again if upload fails #1415

AlbyIanna merged 3 commits into main from show-user-fields-dialog

Sep 20, 2022

Conversation

@AlbyIanna

Copy link

Contributor

@AlbyIanna AlbyIanna commented Sep 8, 2022

Motivation

When uploading a sketch with user fields, if the user made a mistake while filling in the dialog, or made a change that requires updating the fields, it might not be clear to them how to do that 🙁

Change description

As requested in #1386, I've made a change to show again the user field dialog when pressing the Upload button if the previous upload failed.

Other information

Closes #1386

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)

@AlbyIanna AlbyIanna requested review from davegarthsimpson, francescospissu, per1234 and ubidefeo

September 8, 2022 14:14

@kittaakos

Copy link

Contributor

kittaakos commented Sep 8, 2022

Is there a chance to move the entire user-fields logic to another module and let upload-sketch do the uploading only? Of course, this new thing should communicate with the upload contribution before upload. All user-field dialogs, prompts, etc. could go into their own service, class, or contribution. What do you think?

@per1234 per1234 added type: enhancement topic: code labels

Sep 8, 2022

@AlbyIanna

Copy link

Contributor Author

AlbyIanna commented Sep 9, 2022

Is there a chance to move the entire user-fields logic to another module and let upload-sketch do the uploading only? Of course, this new thing should communicate with the upload contribution before upload. All user-field dialogs, prompts, etc. could go into their own service, class, or contribution. What do you think?

Yes, it would make sense. I'll work on it later.

@AlbyIanna

Copy link

Contributor Author

AlbyIanna commented Sep 9, 2022

@kittaakos please have a look at the last commit, I've created a separate contribution for the user fields.

per1234

per1234 approved these changes

Sep 11, 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.

It is working perfectly for me.

Thanks Alberto!

@AlbyIanna AlbyIanna marked this pull request as ready for review

September 15, 2022 07:38

@AlbyIanna AlbyIanna requested a review from kittaakos

September 15, 2022 08:02

kittaakos

kittaakos reviewed

Sep 15, 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.

It's looking great. Thank you for the refactoring. I left a few remarks.

arduino-ide-extension/src/browser/contributions/user-fields.ts Outdated Show resolved Hide resolved

arduino-ide-extension/src/browser/contributions/user-fields.ts

@injectable()

export class UserFields extends Contribution {

private boardRequiresUserFields = false;

private userFieldsSet = false;

Copy link

Contributor

@kittaakos kittaakos Sep 15, 2022

Choose a reason for hiding this comment

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

Can we not simplify this and if there are user fields for an fqbn in the map, it means true. Otherwise, false.

Copy link

Contributor Author

@AlbyIanna AlbyIanna Sep 15, 2022

Choose a reason for hiding this comment

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

In that we would lose the value of the user field. As far as I know, we don't want that, we want to preserve the value, but give the user the possibility to change it.

arduino-ide-extension/src/browser/contributions/user-fields.ts Outdated Show resolved Hide resolved

arduino-ide-extension/src/browser/contributions/user-fields.ts Show resolved Hide resolved

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

return undefined;

}

const userFields = this.userFields();

const userFields = this.userFields.getUserFields();

Copy link

Contributor

@kittaakos kittaakos Sep 15, 2022

Choose a reason for hiding this comment

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

Why not pass the fqbn from here?

Copy link

Contributor Author

@AlbyIanna AlbyIanna Sep 15, 2022

Choose a reason for hiding this comment

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

I don't see any reason since I can easily get them from the boardsServiceProvider. Passing it here would just add a parameter, with no benefit.

Copy link

Contributor

@kittaakos kittaakos Sep 15, 2022

Choose a reason for hiding this comment

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

Exactly this; why do you want to calculate it twice? verify, and upload will require the fqbn anyway.

Copy link

Contributor Author

@AlbyIanna AlbyIanna Sep 15, 2022

Choose a reason for hiding this comment

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

IMO how the user fields are stored is not related to the upload. I may want to open that dialog even if I'm not uploading.

@per1234 per1234 mentioned this pull request

Sep 15, 2022

OTA - on wrong password fails with error instead of prompting for password again #1443

Closed

3 tasks

Alberto Iannaccone added 3 commits

September 16, 2022 08:58


 Show user fields dialog again if upload fails

bc2ce86


 move user fields logic into own contribution

a34f6ff


 apply suggestions

94ca5f3

@AlbyIanna AlbyIanna force-pushed the show-user-fields-dialog branch from b00d70d to 94ca5f3 Compare

September 16, 2022 07:00

@AlbyIanna AlbyIanna requested a review from kittaakos

September 16, 2022 09:16

kittaakos

kittaakos approved these changes

Sep 19, 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.

Verified. Thank you!

user_fields_prompt.mp4

@AlbyIanna AlbyIanna merged commit 671d2ea into main

Sep 20, 2022

@AlbyIanna AlbyIanna deleted the show-user-fields-dialog branch

September 20, 2022 07:27

@per1234 per1234 mentioned this pull request

Nov 27, 2022

Upload fails silently when no board is selected #1714

Open

3 tasks

Labels

topic: code type: enhancement

3 participants

@AlbyIanna @kittaakos @per1234

Uh oh!

Show user fields dialog again if upload fails #1415

Show user fields dialog again if upload fails #1415

Uh oh!

Conversation

@AlbyIanna AlbyIanna commented Sep 8, 2022

Motivation

Change description

Other information

Reviewer checklist

Uh oh!

kittaakos commented Sep 8, 2022

Uh oh!

AlbyIanna commented Sep 9, 2022

Uh oh!

AlbyIanna commented Sep 9, 2022

Uh oh!

@per1234 per1234 left a comment

Choose a reason for hiding this comment

Uh oh!

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

@kittaakos kittaakos Sep 15, 2022

Choose a reason for hiding this comment

Uh oh!

@AlbyIanna AlbyIanna Sep 15, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

@kittaakos kittaakos Sep 15, 2022

Choose a reason for hiding this comment

Uh oh!

@AlbyIanna AlbyIanna Sep 15, 2022

Choose a reason for hiding this comment

Uh oh!

@kittaakos kittaakos Sep 15, 2022

Choose a reason for hiding this comment

Uh oh!

@AlbyIanna AlbyIanna Sep 15, 2022

Choose a reason for hiding this comment

Uh oh!

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants