Improve "Project does not exists" popup

[kiskoza]

Identify the Bug

This is a first improvement to fix #833.

Description of the Change

I've changed the warning to be dismissable (won't auto-hide anymore) and added two buttons. The first one "Remove all", removes all missing folders from the project. The second one just closes the pop-up for now.

Alternate Designs

On the ticket there were some discussions if we can add an option to fix the paths or remove only some of them. I think just removing all of the deleted folders solves the problem most of the time, I would not write this right now.

There's a reason behind adding a "Snooze" button, something like we already have for new versions' popup. I'm happy to add it, if you also see the need for this.

Possible Drawbacks

Without the snooze button, the auto-hiding popup will became a permanent popup on every start.

Verification Process

Follow the steps described on the issue.

Release Notes

The missing folder popup warning now has a button to remove the missing folders from the workspace.

Other things to discuss

• The function I extended with the new logic is already a huge one, I'd like to move the missing folder handling out to a new function. As I'm not a regular maintainer, I don't know if you have a policy about it (e.g.: don't do this because xy reason, follow some naming convention, etc)
• The extended tests are failing for me on local. I'm not entirely sure why, as most of the jasmine docs indicates that the any helper should work with the old jasmine version we're using here.

[kiskoza]

The CI passed, but it has a silent fail (the same I got on my local)

ALL TESTS THAT FAILED:
AtomEnvironment ::openLocations(locations) when there is no saved state when the opened path does not exist may be required to be an existing directory


Finished in 443.882 seconds
2289 tests, 13116 assertions, 1 failure, 2 skipped

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
*********************
Tests failed. Retrying failed tests...
*********************


yarn run v1.22.22
$ electron --no-sandbox --enable-logging . -f --test spec
[18428:0314/090207.681379:ERROR:bus.cc(393)] Failed to connect to the bus: Could not parse server address: Unknown address type (examples of valid types are "tcp" and on UNIX "unix")
[18456:0314/090207.694566:ERROR:viz_main_impl.cc(150)] Exiting GPU process due to errors during initialization
[18428:0314/090207.768734:INFO:CONSOLE(13)] "(electron) The remote module is deprecated. Use https://github.com/electron/remote instead.", source: electron/js2c/renderer_init.js (13)
[18428:0314/090207.816183:INFO:CONSOLE(113)] "%cElectron Security Warning (Insecure Content-Security-Policy) font-weight: bold; This renderer process has either no Content Security
    Policy set or a policy with "unsafe-eval" enabled. This exposes users of
    this app to unnecessary security risks.

For more information and help, consult
https://electronjs.org/docs/tutorial/security.
This warning will not show up
once the app is packaged.", source: electron/js2c/renderer_init.js (113)
[18428:0314/090207.828468:INFO:CONSOLE(33)] "The vm module of Node.js is deprecated in the renderer process and will be removed.", source: electron/js2c/renderer_init.js (33)
[18428:0314/090208.102230:INFO:CONSOLE(33)] "The vm module of Node.js is deprecated in the renderer process and will be removed.", source: electron/js2c/renderer_init.js (33)
[18428:0314/090208.282855:INFO:CONSOLE(33)] "The vm module of Node.js is deprecated in the renderer process and will be removed.", source: electron/js2c/renderer_init.js (33)
(node:18480) Electron: Loading non-context-aware native module in renderer: '/home/runner/work/pulsar/pulsar/node_modules/pathwatcher/build/Release/pathwatcher.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(Use `exe --trace-warnings ...` to show where the warning was created)
(node:18480) Electron: Loading non-context-aware native module in renderer: '/home/runner/work/pulsar/pulsar/node_modules/superstring/build/Release/superstring.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:18480) Electron: Loading non-context-aware native module in renderer: '/home/runner/work/pulsar/pulsar/node_modules/tree-sitter/build/Release/tree_sitter_runtime_binding.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
(node:18480) Electron: Loading non-context-aware native module in renderer: '/home/runner/work/pulsar/pulsar/node_modules/git-utils/build/Release/git.node'. This is deprecated, see https://github.com/electron/electron/issues/18397.
[18428:0314/090208.682460:ERROR:bus.cc(393)] Failed to connect to the bus: Could not parse server address: Unknown address type (examples of valid types are "tcp" and on UNIX "unix")


Finished in 0.022 seconds
0 tests, 0 assertions, 0 failures, 2291 skipped

Done in 1.42s.

[DeeDeeG]

Regarding the "silent fail":

I'm trying to debug the script/run-tests.js script.

This script notices that some error occurred, so the failing tests should be retried, but it uses an empty list of tests to to filter which ones to retry. Then, since there are no "failing tests" in the "re-run" step (100% of tests are skipped which technically gives a "passing" exit code not "failing")... the "empty re-run" pass exits with a passing/clean exit code and the CI run "succeeds".

[confused-Techie]

@DeeDeeG Thats an alarming discovery. We should probably insure that if script/run-tests.js tries to do a run with zero tests, it will always fail, since I'd rather a false negative than a false positive in our CI

[savetheclocktower]

I think this got stuck in limbo because of the “silent CI failure” issue, but the core change is good. Whether or not we fix that (is #1121 relevant?), we should adopt the code changes in this PR.

Not going to “approve” something that's in draft, especially if we can't yet demonstrate that CI is correctly green… but we should be able to verify on our own if there are genuine spec regressions — and, if not, we can land this.

EDIT: Also, I notice that the CI situation might be different now that we're on Jasmine 2. Maybe I'll just kick CI again and see what it says!

[savetheclocktower]

This is my only quibble on this otherwise excellent PR:

Since the original notification tells you exactly which directories cannot be found, I think it's redundant to spawn an additional notification for each one in order to give the user information they already had. I think we can remove this line.

[savetheclocktower]

I don't have the bandwidth to go deep on this at this instant, but I'm commenting here to mark this as a PR that warrants further examination now that the Electron upgrade project is largely behind us.

Edit: ah, right, this is the one that needs an explicit kick so that CI re-runs. I might have to push a dummy change to this PR to trigger it, since taking it out of draft didn't do the trick.

[kiskoza]

Hey, I re-triggered the CI (and updated a few lines of test to make them pass with jasmine2), it should be fine now. The failing macOS is also failing on master with the very same error, it's not related to these changes.