Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

#1089: IDE falls back to new sketch if opening failed #1152

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
kittaakos merged 22 commits into main from #1089-signed
Jul 18, 2022
Merged

Conversation

@kittaakos
Copy link
Contributor

@kittaakos kittaakos commented Jul 6, 2022 •
edited by per1234
Loading

Motivation

IDE must automatically open a new sketch (in the temp folder) if the sketch was deleted between the IDE stop/start cycle.

Requirements:

  • At least three valid sketches in the sketchbook folder. My bare minimum setup is the following:
a.kitta@Akoss-MacBook-Pro Arduino % ls -al
total 16
drwxr-xr-x 6 a.kitta staff 192 Jul 14 15:03 .
drwx------+ 5 a.kitta staff 160 Jul 6 15:46 ..
-rw-r--r--@ 1 a.kitta staff 6148 Jul 14 15:03 .DS_Store
drwxr-xr-x 3 a.kitta staff 96 Jul 7 16:42 sketch_1
drwxr-xr-x 3 a.kitta staff 96 Jul 7 16:43 sketch_2
drwxr-xr-x 3 a.kitta staff 96 Jul 7 16:43 sketch_3
a.kitta@Akoss-MacBook-Pro Arduino % cat sketch_1/sketch_1.ino 
void setup() {}
void loop() {}

(sketch_2 and sketch_3 have the same content as sketch_1).

Preconditions:

  • Open IDE, if sketch_1 is not loaded, open sketch_1,
  • Close any other opened sketches
  • When sketch_1 is the only opened sketch in IDE the verification can start.

1: Opening deleted sketch from File > Sketchbook

  • Note: the file-system watcher might miss file deletion events and cannot update the Sketchbook submenu content.
  • Check File > Sketchbook, sketch_3 is there,
  • Delete sketch_3 outside from IDE,
  • Open File > Sketchbook > sketch_3,
  • See error notification that it's missing.
  • Check File > Sketchbook, sketch_3 is not there.
open_deleted_from_menu.mp4
  1. Delete the sketch folder when IDE is stopped:
  • Open sketch_1 in IDE,
  • Close IDE,
  • Delete sketch_1 from the sketchbook,
  • Start IDE,
  • Without any error notifications, IDE opens a new fallback sketch
restore_deleted_sketch_on_start.mp4
  1. Delete the main sketch file when IDE is stopped:
  • Open sketch_2 in IDE,
  • Close IDE,
  • Delete the main sketch file sketch_2/sketch_2.ino,
  • Start the IDE,
  • IDE loads, expect a reload,
  • New sketck is created and opened but no error notification is shown by IDE.
restotre_invalid_sketch_on_start.mp4

Change description

Other information

Closes #1089

Breaking: Optimize for Debugging now has its preference: arduino.compile.optimizeForDebug. The default value is false. If you had Optimize for Debugging enabled before this IDE version, you must click on Optimized for Debugging again.

Screen Shot 2022年07月08日 at 11 05 51

Breaking: Changed arduino.cloud.sketchSyncEnpoint to arduino.cloud.sketchSyncEndpoint (#1067). If you had a custom sketch sync endpoint defined, you must redefine it again with the version of the IDE.

IDE uses the default spinner from Theia:

default_theia_spinner.mp4

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)

per1234 reacted with thumbs up emoji
@per1234 per1234 added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Jul 7, 2022
@kittaakos kittaakos force-pushed the #1089-signed branch 2 times, most recently from 50cfe0d to 1da6d7c Compare July 8, 2022 11:13
@kittaakos kittaakos marked this pull request as ready for review July 14, 2022 13:41
@kittaakos kittaakos changed the title (削除) [WIP]: #1089: IDE2 falls back to new sketch if opening failed. (削除ここまで) (追記) #1089: IDE2 falls back to new sketch if opening failed. (追記ここまで) Jul 14, 2022
Copy link
Contributor Author

kittaakos commented Jul 14, 2022 •
edited by per1234
Loading

If IDE has the changes from arduino/arduino-cli#1805, then the error message will be the same for all three use-cases this PR covers:

Screen Shot 2022年07月14日 at 17 33 40

Screen Shot 2022年07月14日 at 17 32 31

Screen Shot 2022年07月14日 at 17 30 07

Please let me know if we need arduino/arduino-cli#1805 as-is for this PR.

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 are two distinct reasons the IDE might attempt to open a sketch:

  • Explicit action by user (e.g., selecting a sketch from File > Sketchbook)
  • Restore state from previous session (#780)

The IDE must gracefully handle a failure of either operation (#1089). However, I feel the need to communicate about the failure to the user differs between them.

If an operation triggered explicitly by the user fails, it is important to communicate this to the user. So I think showing a notification when the sketch opening fails is necessary in this case.

The situation is different when it comes to automatically opening sketches from the previous IDE session. This is only a convenience feature. If it is able to open the sketches, great. But if it is not able to open the sketches, I don't want to hear about it. I might have intentionally deleted that sketch. It might have been a new sketch that was staged in a temporary folder the OS cleaned up. In the latter case, the temporary path makes the error message especially cryptic:
image

So I suggest that no error notification be shown when the sketch opening fails during the restore operation (logging it is fine of course).

Copy link
Contributor Author

Thank you for the review!

So I suggest that no error notification be shown when the sketch opening fails during the restore operation (logging it is fine of course).

Sure 👍

IDE2 (and the CLI) might fail to load the sketch due to two reasons:

  • the sketch folder was deleted,
  • the sketch is not valid (the main file was deleted, does not match the folder name, .etc)

As a user, do you expect that IDE2 will omit the error notification in both cases?

Copy link
Contributor

per1234 commented Jul 15, 2022 •
edited
Loading

do you expect that IDE will omit the error notification in both cases?

In the case of a "restore" operation, yes.
kittaakos reacted with thumbs up emoji

Copy link
Contributor Author

kittaakos commented Jul 15, 2022 •
edited by per1234
Loading

I have updated the PR not to show the error notification when restoring the previously opened sketch has failed (due to a missing or invalid sketch). I updated the screencasts too to show the expected IDE behavior. Please review.

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 works perfectly. Excellent work Akos!

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.

LGTM.

I left some comments, but nothing important.

Comment on lines +42 to +44
@inject(FrontendApplicationStateService)
private readonly appStateService: FrontendApplicationStateService;

Copy link
Contributor

@AlbyIanna AlbyIanna Jul 15, 2022

Choose a reason for hiding this comment

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

Since you're doing some clean-up, what do you think about making this class extend Contribution so that we can get rid of some injected services (ILogger, MessageService, CommandService, FrontendApplicationStateService) and make use of the onReady method?

Copy link
Contributor Author

@kittaakos kittaakos Jul 18, 2022

Choose a reason for hiding this comment

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

Good idea, but I do not want to introduce a hard dependency between contributions. Also, this service does not contribute any commands, menu items, .etc.

Akos Kitta added 7 commits July 18, 2022 10:30 
Closes #1089
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Akos Kitta added 15 commits July 18, 2022 10:32 
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Closes #1067.
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
#1067 
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
Signed-off-by: Akos Kitta <a.kitta@arduino.cc>
@kittaakos kittaakos merged commit 8ad10b5 into main Jul 18, 2022
@kittaakos kittaakos deleted the #1089-signed branch July 18, 2022 09:10
@per1234 per1234 changed the title (削除) #1089: IDE2 falls back to new sketch if opening failed. (削除ここまで) (追記) #1089: IDE falls back to new sketch if opening failed (追記ここまで) Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@per1234 per1234 per1234 approved these changes

@fstasi fstasi Awaiting requested review from fstasi

+1 more reviewer

@AlbyIanna AlbyIanna AlbyIanna approved these changes

Reviewers whose approvals may not affect merge requirements

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.

Open Example, Close, Open again - appears to open copy in %temp% directory

AltStyle によって変換されたページ (->オリジナル) /