Skip to content

#39 Avoid deleting the workspace when it's still in use #1409

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
Merged
merged 1 commit into from
Sep 15, 2022
Merged

#39 Avoid deleting the workspace when it's still in use #1409

merged 1 commit into from
Sep 15, 2022

Conversation

kittaakos
Copy link
Contributor

@kittaakos kittaakos commented Sep 7, 2022 •
edited
Loading

Motivation

To fix the unable to watch file changes in this large workspace notification when saving a temp sketch.

Change description

  • From now on, NSFW service disposes after removing the last reference. No more 10sec delay.
  • Moved the temp workspace deletion to a startup task.
  • Can set the initial task for the window from electron-main.
  • Removed the browser-app.

Other information

Closes #39

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)

@kittaakos kittaakos added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Sep 7, 2022
@kittaakos kittaakos force-pushed the #39 branch 6 times, most recently from ca59331 to 25989de Compare September 8, 2022 16:07
@kittaakos kittaakos marked this pull request as ready for review September 8, 2022 16:09
@kittaakos kittaakos changed the title #39 Gracefully dispose the NSFW service when deleting temp sketch and reloading the window after Save as... #39 Avoid deleting the workspace when it's still in use Sep 8, 2022
per1234
per1234 approved these changes Sep 9, 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.

Unfortunately I am not able to confirm the fix because, even though I have encountered #39 periodically throughout the years of using this application, I never managed to figure out how to reproduce it.

What I have done is try various shenanigans involving workspaces and file watcher on both my Windows and Linux machines to try to break something and didn't manage to find any misbehavior that doesn't also occur with the build from the main branch.

So at least I can say that I didn't find any regressions.

Thanks for your work to eradicate this confusing error Akos!

@kittaakos
Copy link
Contributor Author

periodically throughout the years of using this application, I never managed to figure out how to reproduce it.

I can reproduce it on macOS with the following steps (same as here):

  • have a sketch opened from your sketchbook in IDE2,
  • create a new sketch and save it with Ctrl/Cmd+S (it's crucial to save and not save as) without doing anything else,
  • see the error in the first window.

I hope this helps; I could consistently reproduce the issue with these steps 👆 Let me know how it went. Thank you!

@per1234
Copy link
Contributor

per1234 commented Sep 9, 2022

Thanks! I am able to reproduce the bug by following the procedure you provided when using 2.0.0-rc9.4 on both Windows and Linux. When I do the same using the build from this PR, I am not able to reproduce the bug.

AlbyIanna
AlbyIanna approved these changes Sep 15, 2022
View reviewed changes
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.

Code LGTM.

A minor remark I've got is that there are still mentions to browser-app

@kittaakos
Copy link
Contributor Author

Thank you for the remarks. 👍

I will take care of them. (By the way, they're the same.)

I do not understand this feedback 👆

Do you want me to update the comment or remove the step?

Avoid deleting the workspace when it's still in use.
f79e2de 
 - From now on, NSFW service disposes after last reference
is removed. No more 10sec delay.
 - Moved the temp workspace deletion to a startup task.
 - Can set initial task for the window from electron-main.
 - Removed the `browser-app`.

Closes arduino#39

Signed-off-by: Akos Kitta <[email protected]>
@AlbyIanna
Copy link
Contributor

Do you want me to update the comment or remove the step?

I was just referring to the comment, but I see you already changed it.

Thank you!

@kittaakos
Copy link
Contributor Author

Thank you!

Thanks for reviewing it and finding the leftovers. Once the build is green, I merge the PR.

@kittaakos kittaakos merged commit fdf6f0f into arduino:main Sep 15, 2022
@per1234 per1234 mentioned this pull request Oct 12, 2022
Closed
3 tasks
@per1234 per1234 mentioned this pull request Nov 28, 2022
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlbyIanna AlbyIanna AlbyIanna approved these changes

@per1234 per1234 per1234 approved these changes

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.

Occasional "Unable to watch for file changes in this large workspace" warnings
3 participants
@kittaakos @per1234 @AlbyIanna