Save dialog for closing temporary sketch and unsaved files #893

msujew · 2022-03-08T13:16:44Z

Motivation

Closes #595
Closes #862

Change description

When closing the app, it will check whether the current sketch is temporary and ask the user to save it somewhere else.

Other information

When disabling autoSave and editing a file in a temp sketch and trying to close the app, two dialogs will appear. This is an issue in upstream Theia, see eclipse-theia/theia#10860. A PR to fix this is linked.

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 · 2022-03-09T09:19:15Z

This doesn't seem to solve #595.
I'm on MacOS and what's happening is this: 👇

Screen.Recording.2022-03-09.at.09.58.25.mov

As you can see, no dialog appears when closing unsaved sketches, be they temporary or permanent sketches.

AlbyIanna · 2022-03-09T09:37:20Z

I noticed another funny behaviour though. It appears that from the Menu bar there are 3 different ways of closing the IDE:

Arduino IDE > Quit Arduino IDE
this one closes all IDE windows, as expected, but doesn't ask to save unsaved changes.
File > Close
this seems to work properly with temporary sketches, but not with permanent sketches, as shown in the video below. Also, I noticed that this works the same in the main, then I guess it's not an effect of this PR.

Screen.Recording.2022-03-09.at.10.18.38.mov

File > Close Editor
This doesn't seem to do anything at all when clicked.

Notice that 2. File > Close and 3. File > Close Editor are bound to the same shortcut (CMD+W). When I use the shortcut CMD+W, the behaviour of 2. File > Close is performed.

Consider also that I cannot see differences between this branch and the main branch. @msujew am I missing something?

msujew · 2022-03-09T11:10:17Z

@AlbyIanna Thanks for looking into it. I'm confused, because it works as the requirements state on Windows:

2022-03-09.12-07-47.mp4

The only way I can imagine this to break is if the SketchService#isTemp method does not work correctly on MacOS? Can you try other instances where it is used? Alternatively can you debug through the following method and see were it stops?

arduino-ide/arduino-ide-extension/src/browser/arduino-frontend-contribution.tsx

Line 558 in 192b8c0

const sketch = await this.sketchServiceClient.currentSketch();

AlbyIanna · 2022-03-09T13:29:30Z

Yes, I'll give it a try. In the meanwhile, @per1234 would you please test this on Linux? I wonder if it works there at this point.

AlbyIanna · 2022-03-09T14:52:49Z

@msujew okay, now it gets interesting. I've just launched the IDE in debug mode and it worked (almost) perfectly. The build artifact from the CI is still broken though 🤔

AlbyIanna · 2022-03-09T16:40:01Z

@msujew I would suggest you try to download the build artifact produced in the GitHub workflow and see if it works on Windows.

arduino-ide-extension/src/browser/arduino-frontend-contribution.tsx

arduino-ide-extension/src/browser/dialogs/temp-sketch-dialog.ts

per1234 · 2022-03-10T04:22:48Z

From #893 (comment):

no dialog appears when closing unsaved sketches, be they temporary or permanent sketches.

This is tracked at #862

From #893 (comment)

I noticed that this works the same in the main, then I guess it's not an effect of this PR.

Correct. #862 was introduced by the last Theia bump (#791)

per1234

RESOLVED!

~~I tested the latest CI build for this PR (Version: 2.0.0-rc4-snapshot-de6aa05) on Windows 10 and Ubuntu 20.04.~~

Unfortunately, it did not fix #595 for me:

Select File > Preferences... from the Arduino IDE menus.

Check the box next to "☐ Auto save".

Click the OK button.

Select File > New from the Arduino IDE menus.

Make any change to the sketch.

Use the standard window control (i.e., X or 🔴) to close the window.
🐛 The window closes without the expected confirmation of close with unsaved changes (If autosave is on, user is not prompted to "save as..." when closing a newly created sketch or edited example. #595).

Select File > New from the Arduino IDE menus.

Make any change to the sketch.

Select File > Quit from the Arduino IDE menus.
🐛 The window closes without the expected confirmation of close with unsaved changes (If autosave is on, user is not prompted to "save as..." when closing a newly created sketch or edited example. #595).

AlbyIanna · 2022-03-17T10:54:04Z

@msujew I downloaded the latest workflow build and now it doesn't work at all on MacOs. Have you tested the workflow build?

msujew · 2022-03-17T12:37:41Z

@AlbyIanna downloaded the workflow build and it works as expected (on Windows)

2022-03-17.13-35-12.mp4

I'll probably get to it next week, and ask some of my colleagues to test it for me.

per1234 · 2022-03-18T04:17:15Z

I'm happy to do another round of testing whenever you are ready for it. Just let me know.

AlbyIanna · 2022-03-21T08:59:18Z

@msujew can I help somehow?

fstasi · 2022-03-24T11:03:15Z

@msujew do you have any clue why this does not work on the artifact version on mac?

msujew · 2022-04-06T09:46:44Z

@AlbyIanna @fstasi I am making steady progress on it: It currently seems like the shutdown routine isn't called at all when using the MacOS artifact. I will change the build script a bit locally to leave more debug information in there, so that we can debug the final artifact. I'll keep you updated on that.

msujew · 2022-05-19T10:55:47Z

@per1234 @AlbyIanna I'm back from vacation and rewrote the whole feature. It should work now, even on Mac (tested with a colleague of mine). Please let me know if you find any issues with this approach.

per1234 · 2022-05-25T09:49:48Z

Hi @msujew. There was a notarization service outage yesterday which caused a spurious failure of the build workflow.

Everything is back to normal today. Would you mind rebasing the PR on the main branch to trigger another build so that I can try out your fix?

msujew · 2022-05-25T11:44:27Z

@per1234 Oh, I didn't notice earlier. It took a bit (seems like GH Actions is still a bit slow), but the artifacts have finished building now.

per1234

RESOLVED!

Describe the problem

🐛 The Arduino IDE window can not be closed.

To reproduce

Click the standard window close control (i.e., X or 🔴) on the Arduino IDE window.
🐛 The IDE window does not close.

Select File > Close from the Arduino IDE menus.
🐛 The IDE window does not close.

Select File > Quit from the Arduino IDE menus.
🐛 The IDE windows do not close.

Expected behavior

IDE window(s) close via all the standard methods.

Arduino IDE version

2.0.0-rc6-snapshot-6f1f5e2
(tester build for 366a6de)

Operating system

Windows 10, Ubuntu 20.04

Additional context

The issue does not occur when using the build from the tip of the main branch (522a5c6).

I see output like this in the terminal after attempting to close the window:

Marking workspace as a closed sketch. Workspace URI: file:///c%3A/Users/per/AppData/Local/Temp/.arduinoIDE-unsaved2022425-12916-3dhrhd.jb5c4/sketch_may25a. Date: 1653481012912.

msujew · 2022-05-25T13:30:06Z

@per1234 it seems like the Theia update broke the way I'm triggering the dialogs...

Back to the drawing board, hopefully I'll have something by the start of next week (next two days are public holidays in Germany).

kittaakos · 2022-05-31T09:20:19Z

#862 works for me on macOS.

I did the following from the sources:

open IDE2,
my sketch from directories#user was opened,
disabled auto-save via the preferences, hit OK,
touched a file, the editor was dirty,
clicked on File > Close,
I had the confirm dialog.

kittaakos · 2022-05-31T09:20:19Z

#862 works for me on macOS.

I did the following from the sources:

open IDE2,
my sketch from directories#user was opened,
disabled auto-save via the preferences, hit OK,
touched a file, the editor was dirty,
clicked on File > Close,
I had the confirm dialog.

msujew · 2022-05-31T09:43:37Z

@kittaakos Did you confirm this with the CI build or with a local dev instance? The last time, it didn't work when running the CI build, but building from sources worked correctly (macOS)

kittaakos · 2022-05-31T09:44:55Z

There is no confirmation dialog when closing a dirty sketch. The sketch is under directories#user.

I downloaded the IDE2,
Start IDE, existing sketch opens,
Make sure auto-save is off,
Touch the sketch, close the sketch with Cmd+W.
The sketch closes without the confirmation dialog.

kittaakos · 2022-05-31T09:47:13Z

The last time, it didn't work when running the CI build, but building from sources worked correctly (macOS)

Thanks for the heads-up, yes it was from sources so I downloaded the bundled app and kept testing that. I can confirm, from the bundled app, that the IDE2 quits without asking anything. Strange indeed, I will investigate.

msujew · 2022-05-31T09:53:28Z

Touch the sketch, close the sketch with Cmd+W.
The sketch closes without the confirmation dialog.

Yeah, that was an oversight. Using Cmd+W triggers the CLOSE command, which is completely separately implemented (for some reason). It should be the same as closing the window using the X button now.

kittaakos · 2022-05-31T10:24:47Z

I found the problem; it's here:

https://github.com/eclipse-theia/theia/blob/ede3d6fa8854ffbbd409360fa143a84c10352ce5/packages/core/src/electron-main/theia-electron-window.ts#L125

On my installation, the currentUrl is file:///Applications/Arduino%20IDE.app/Contents/Resources/app/lib/index.html?port=57442#/Users/a.kitta/Documents/Arduino/sketch_may27a, and the frontendUri is /Applications/Arduino IDE.app/Contents/Resources/app/lib/index.html. Note the incorrect URL encoding/escaping problem.

The !currentUrl.includes(frontendUri) will be true, and the default logic shortcuts the boolean evaluation, and the IDE2 does not show the dialog.

The fix is to URI encode the this.globals.THEIA_FRONTEND_HTML_PATH. It seems the space in the path is causing the issue again.

msujew · 2022-05-31T10:29:44Z

@kittaakos Thanks! Finally, the mystery has been solved :D

I'll create an issue in the Theia repo for this 👍

kittaakos · 2022-05-31T10:31:28Z

I'll create an issue in the Theia repo for this 👍

Thank you ❤️

Ref: eclipse-theia/theia#11226 Signed-off-by: Akos Kitta <[email protected]>

Signed-off-by: Akos Kitta <[email protected]>

Removed `electron` from the `nls-extract`. It does not contain app code. Signed-off-by: Akos Kitta <[email protected]>

Signed-off-by: Akos Kitta <[email protected]>

per1234

This fixes #595 and #862 for me on my Windows and Linux machines.

Thanks for this important fix @msujew and @kittaakos!

* Use normal `OnWillStop` event * Align `CLOSE` command to rest of app * Fixed FS path vs encoded URL comparision when handling stop request. Ref: eclipse-theia/theia#11226 Signed-off-by: Akos Kitta <[email protected]> * Fixed the translations. Signed-off-by: Akos Kitta <[email protected]> * Fixed the translations again. Removed `electron` from the `nls-extract`. It does not contain app code. Signed-off-by: Akos Kitta <[email protected]> * Aligned the stop handler code to Theia. Signed-off-by: Akos Kitta <[email protected]> Co-authored-by: Akos Kitta <[email protected]>

* backend structure WIP * Scaffold interfaces and classes for pluggable monitors * Implement MonitorService to handle pluggable monitor lifetime * Rename WebSocketService to WebSocketProvider and uninjected it * Moved some interfaces * Changed upload settings * Enhance MonitorManager APIs * Fixed WebSocketChange event signature * Add monitor proxy functions for the frontend * Moved settings to MonitorService * Remove several unnecessary serial monitor classes * Changed how connection is handled on upload * Proxied more monitor methods to frontend * WebSocketProvider is not injectable anymore * Add generic monitor settings storaging * More serial classes removal * Remove unused file * Changed plotter contribution to use new manager proxy * Changed MonitorWidget and children to use new monitor proxy * Updated MonitorWidget to use new monitor proxy * Fix backend logger bindings * Delete unnecessary Symbol * coreClientProvider is now set when constructing MonitorService * Add missing binding * Fix `MonitorManagerProxy` DI issue * fix monitor connection * delete duplex when connection is closed * update arduino-cli to 0.22.0 * fix upload when monitor is open * add MonitorSettingsProvider interface * monitor settings provider stub * updated pseudo code * refactor monitor settings interfaces * monitor service provider singleton * add unit tests * change MonitorService providers to injectable deps * fix monitor settings client communication * refactor monitor commands protocol * use monitor settings provider properly * add settings to monitor model * add settings to monitor model * reset serial monitor when port changes * fix serial plotter opening * refine monitor connection settings * fix hanging web socket connections * add serial plotter reset command * send port to web socket clients * monitor service wait for success serial port open * fix reset loop * update serial plotter version * update arduino-cli version to 0.23.0-rc1 and regenerate grpc protocol * remove useless plotter protocol file * localize web socket errors * clean-up code * update translation file * Fix duplicated editor tabs (#1012) * Save dialog for closing temporary sketch and unsaved files (#893) * Use normal `OnWillStop` event * Align `CLOSE` command to rest of app * Fixed FS path vs encoded URL comparision when handling stop request. Ref: eclipse-theia/theia#11226 Signed-off-by: Akos Kitta <[email protected]> * Fixed the translations. Signed-off-by: Akos Kitta <[email protected]> * Fixed the translations again. Removed `electron` from the `nls-extract`. It does not contain app code. Signed-off-by: Akos Kitta <[email protected]> * Aligned the stop handler code to Theia. Signed-off-by: Akos Kitta <[email protected]> Co-authored-by: Akos Kitta <[email protected]> * fix serial monitor send line ending * refactor monitor-service poll for test/readability * localize web socket errors * update translation file * Fix duplicated editor tabs (#1012) * i18n:check rerun * Speed up IDE startup time. Signed-off-by: Akos Kitta <[email protected]> * override coreClientProvider in monitor-service * cleanup merged code Co-authored-by: Francesco Stasi <[email protected]> Co-authored-by: Silvano Cerza <[email protected]> Co-authored-by: Mark Sujew <[email protected]> Co-authored-by: David Simpson <[email protected]> Co-authored-by: Akos Kitta <[email protected]>

msujew force-pushed the msujew/save-on-close branch from f07c344 to 192b8c0 Compare March 8, 2022 13:18

msujew force-pushed the msujew/save-on-close branch from 192b8c0 to 5564b91 Compare March 9, 2022 11:24

AlbyIanna reviewed Mar 9, 2022

View reviewed changes

arduino-ide-extension/src/browser/arduino-frontend-contribution.tsx Outdated Show resolved Hide resolved

AlbyIanna reviewed Mar 9, 2022

View reviewed changes

arduino-ide-extension/src/browser/dialogs/temp-sketch-dialog.ts Outdated Show resolved Hide resolved

per1234 added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Mar 9, 2022

per1234 requested changes Mar 10, 2022

View reviewed changes

AlbyIanna mentioned this pull request Mar 10, 2022

Automatically remove editors for deleted files #894

Merged

4 tasks

This was referenced Apr 6, 2022

Save all open editors before running Save As #939

Merged

No confirmation on closing window with unsaved changes #862

Closed

msujew linked an issue Apr 12, 2022 that may be closed by this pull request

No confirmation on closing window with unsaved changes #862

Closed

msujew force-pushed the msujew/save-on-close branch 2 times, most recently from 0add813 to 84f033a Compare May 18, 2022 14:52

msujew requested review from per1234 and AlbyIanna May 19, 2022 10:54

msujew changed the title ~~Save dialog for closing temporary sketch~~ Save dialog for closing temporary sketch and unsaved files May 20, 2022

msujew force-pushed the msujew/save-on-close branch 2 times, most recently from 9e96731 to 366a6de Compare May 25, 2022 10:30

per1234 requested changes May 25, 2022

View reviewed changes

Use normal OnWillStop event

1c82e25

msujew force-pushed the msujew/save-on-close branch from 366a6de to 1c82e25 Compare May 30, 2022 12:44

Align CLOSE command to rest of app

7abfa53

Akos Kitta added 4 commits May 31, 2022 13:48

Fixed FS path vs encoded URL comparision when handling stop request.

c1125f3

Ref: eclipse-theia/theia#11226 Signed-off-by: Akos Kitta <[email protected]>

Fixed the translations.

525233b

Signed-off-by: Akos Kitta <[email protected]>

Fixed the translations again.

e8e0644

Removed `electron` from the `nls-extract`. It does not contain app code. Signed-off-by: Akos Kitta <[email protected]>

Aligned the stop handler code to Theia.

785d261

Signed-off-by: Akos Kitta <[email protected]>

per1234 approved these changes Jun 1, 2022

View reviewed changes

fstasi approved these changes Jun 1, 2022

View reviewed changes

fstasi merged commit 5b486b1 into main Jun 1, 2022

fstasi deleted the msujew/save-on-close branch June 1, 2022 08:55

per1234 mentioned this pull request Jun 1, 2022

My sketch diasappeared #1017

Closed

3 tasks

KurtE mentioned this pull request Jun 8, 2022

Example files - ask to save even if no changes were made to them. #1026

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save dialog for closing temporary sketch and unsaved files #893

Save dialog for closing temporary sketch and unsaved files #893

msujew commented Mar 8, 2022 •

edited

Loading

AlbyIanna commented Mar 9, 2022

AlbyIanna commented Mar 9, 2022 •

edited

Loading

msujew commented Mar 9, 2022 •

edited

Loading

AlbyIanna commented Mar 9, 2022

AlbyIanna commented Mar 9, 2022 •

edited

Loading

AlbyIanna commented Mar 9, 2022

per1234 commented Mar 10, 2022

per1234 left a comment •

edited

Loading

AlbyIanna commented Mar 17, 2022

msujew commented Mar 17, 2022

per1234 commented Mar 18, 2022

AlbyIanna commented Mar 21, 2022

fstasi commented Mar 24, 2022

msujew commented Apr 6, 2022

msujew commented May 19, 2022

per1234 commented May 25, 2022

msujew commented May 25, 2022

per1234 left a comment •

edited

Loading

msujew commented May 25, 2022

kittaakos commented May 31, 2022

kittaakos commented May 31, 2022

msujew commented May 31, 2022

kittaakos commented May 31, 2022

kittaakos commented May 31, 2022

msujew commented May 31, 2022

kittaakos commented May 31, 2022 •

edited

Loading

msujew commented May 31, 2022

kittaakos commented May 31, 2022

per1234 left a comment

Save dialog for closing temporary sketch and unsaved files #893

Save dialog for closing temporary sketch and unsaved files #893

Conversation

msujew commented Mar 8, 2022 • edited Loading

Motivation

Change description

Other information

Reviewer checklist

AlbyIanna commented Mar 9, 2022

AlbyIanna commented Mar 9, 2022 • edited Loading

msujew commented Mar 9, 2022 • edited Loading

AlbyIanna commented Mar 9, 2022

AlbyIanna commented Mar 9, 2022 • edited Loading

AlbyIanna commented Mar 9, 2022

per1234 commented Mar 10, 2022

per1234 left a comment • edited Loading

Choose a reason for hiding this comment

AlbyIanna commented Mar 17, 2022

msujew commented Mar 17, 2022

per1234 commented Mar 18, 2022

AlbyIanna commented Mar 21, 2022

fstasi commented Mar 24, 2022

msujew commented Apr 6, 2022

msujew commented May 19, 2022

per1234 commented May 25, 2022

msujew commented May 25, 2022

per1234 left a comment • edited Loading

Choose a reason for hiding this comment

Describe the problem

To reproduce

Expected behavior

Arduino IDE version

Operating system

Additional context

msujew commented May 25, 2022

kittaakos commented May 31, 2022

kittaakos commented May 31, 2022

msujew commented May 31, 2022

kittaakos commented May 31, 2022

kittaakos commented May 31, 2022

msujew commented May 31, 2022

kittaakos commented May 31, 2022 • edited Loading

msujew commented May 31, 2022

kittaakos commented May 31, 2022

per1234 left a comment

Choose a reason for hiding this comment

msujew commented Mar 8, 2022 •

edited

Loading

AlbyIanna commented Mar 9, 2022 •

edited

Loading

msujew commented Mar 9, 2022 •

edited

Loading

AlbyIanna commented Mar 9, 2022 •

edited

Loading

per1234 left a comment •

edited

Loading

per1234 left a comment •

edited

Loading

kittaakos commented May 31, 2022 •

edited

Loading