Handle open preference files

[colin-grant-work]

Signed-off-by: Colin Grant [email protected]

What it does

This PR moves the preference UI in the direction of VSCode in two respects:

• It fixes Preference changes not registered when settings.json open in editor. #7721 by saving the preference file when an edit is made.

• It fixes Setting two preferences synchronously creates both .vscode and .theia folders. #9115 by processing preference settings in sequence.

• It fixes Cannot add a value via the preference widget #2547 by prompting the user when the settings file can't be modified.

• It changes the behavior of the AbstractPreferenceProvider.set method to ensure that no changes are made to a file when that file is open in a dirty editor. Instead, an error toast is shown.

It improves the UI further by resetting the UI to the pre-change-attempt state if a preference change attempt fails.

How to test

1. Open preference UI
2. Open the file corresponding to the current scope ({} in upper right)
3. In the UI, change preferences
4. Observe that the file is modified and saved and the change registers in the UI (unless Preference change does not fire an onPreferencedChanged event #7685 - better not to rely on application.confirmExit as the sole test case)
5. In the file editor, make a change to the file and don't save the change
6. Return to the UI and attempt to change a preference.
7. Observe that a toast appears warning that the change was not made.
8. Make a change to another preference in the UI - the toast should remain.
9. Exit the toast without action.
10. Observe that all preference values reverts to their pre-change state in the UI.
11. Repeat 7-9 selecting one of the toast buttons and observe that the behavior is correct in each case.

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[vince-fugnitto]

Functionally the changes work really well 👍
I verified the following:

• making preference updates through the UI saves the document
• making preference updates through the UI with a dirty document results in a prompt
• the prompt option 'save and retry' successfully saves the document and reapplies the update
• the prompt option 'open file' opens the document

I don't have any objections with the code, I'll however give others a chance to review as well

[akosyakov]

We should test how it work without editors. When Theia/VS Code extensions do concurrent updates. It would be good to provide sample VS Code extension to verify it. It should not cause any notifications or unnecessary save cycles.

[colin-grant-work]

@akosyakov, I have finally gotten around to writing tests for this PR, and in the process, I have changed the code quite a bit. Basically, I have implemented a queue mechanism for calls to the setPreference method. Previously, the behavior of the method was quite different depending on whether an editor was open or not:

• With editor open: all calls handled (in non-deterministic order), none saved, no preference event fired.
• Without editor: all calls handled in non-deterministic order, all saved individually, many preference events fired

With the queue mechanism I've implemented, the behavior is the same regardless of whether an editor is open or not (assuming the editor is not dirty):

• All calls are queued
• Processing the queue begins immediately, but if additional calls arrive before all edits are completed, those calls are added to the same transaction queue
• When the queue is empty, a transaction is begun, and handling new calls is deferred until the editor has been saved and the preference changes have been processed - this is to distinguish between the editor being temporarily dirty because a save cycle hasn't completed and the editor being more persistently dirty because the user is working in it
• When the transaction is complete, any calls made in the meantime are handled

This means that any number of concurrent calls result in only one save cycle, whether the editor is dirty or not.

If the editor is dirty:

• Calls are queued as normal, but none are handled until the user chooses what to do.
• If the user chooses not to retry, all pending calls are rejected and resolve to false.

We should test how it work without editors. When Theia/VS Code extensions do concurrent updates. It would be good to provide sample VS Code extension to verify it. It should not cause any notifications or unnecessary save cycles.

I have added tests to the saveable.spec.js file in the examples/api-tests folder, but I haven't created a sample VSCode extension to test its behavior. The tests are a bit brutely forced, but they work for demonstration. Please let me know what you think of the queue idea, and any suggestions you have to improve the implementation or the tests.

[colin-grant-work]

@vince-fugnitto, I've rebased this PR and run tests locally (and now they've finished in CI - not sure why Ubuntu + Python 2 doesn't like me). It comes in two forms:

• Originally, it was fairly basic and just checked if the editor was open and dirty, and if so, warned the user, if not, it saved the file. The commit is likely gone from the reflog, but wouldn't be hard to reimplement.
• After Anton mentioned concerns about saving and requested tests, I implemented the more complex system I describe here and added tests to show that it was behaving as intended. I think that system is a pretty good way to handle challenging (and mostly hypothetical?) cases where the system is hit with many edits in a short period, but it's also a much larger change to the existing code.

The fact of the matter is that the current code generates tons of save cycles if the file is not open, so it doesn't make very much sense to me to quibble about doing the same when the file is open. But, if it's desirable to reduce the number of saves when the system is under pressure, what I've implemented is a move in that direction, and I'm happy to work on improving it if anyone has suggestions on how it can be made better.

[vince-fugnitto]

@colin-grant-work do you mind rebasing the pull-request (there were recent significant changes to the preferences). I can review afterwards 👍

[vince-fugnitto]

@colin-grant-work I don't have any objections to the approach taken by the pull-request, if you are to rebase it should fix the failing api-tests 👍

I've asked that @marechal-p also take a look, namely how we handle queued preference changes.

[colin-grant-work]

@vince-fugnitto, @paul-marechal, with the addition of async-mutex to our toolkit, this PR got a fair bit simpler. I've updated the code and significantly reduced the number of things affected. If you guys wouldn't mind running it through a few paces again, I'd appreciate it.

[msujew]

I was able to disconnect the UI from the settings editor. Afterwards, any changes made to the UI or the settings file are not reflected in the other one:

1. Update a preference in the UI.
2. Open the settings.json file.
3. Change the preference file, but don't save it.
4. Change the value again in the UI, and click on the "Open file" action in the resulting notification (at this point the UI and file editor are disconnected)
5. Change the value in the settings json, save it
6. The change is not reflected in the UI

The "save and retry" and cancel actions work perfectly fine though.

[colin-grant-work]

I was able to disconnect the UI from the settings editor. Afterwards, any changes made to the UI or the settings file are not reflected in the other one:

1. Update a preference in the UI.
2. Open the settings.json file.
3. Change the preference file, but don't save it.
4. Change the value again in the UI, and click on the "Open file" action in the resulting notification (at this point the UI and file editor are disconnected)
5. Change the value in the settings json, save it
6. The change is not reflected in the UI

The "save and retry" and cancel actions work perfectly fine though.

@msujew, thanks for the test. Older versions of this PR - before async-mutex - had code to revert the UI if the user canceled the action, but I forgot about it when redoing the implementation - I'll bring that back.

[colin-grant-work]

@msujew, I've brought back / reimplemented the UI reversion behavior, and added another step to the testing to confirm that even multiple changes to the UI while the toast is up will all be reverted.

[vince-fugnitto]

I confirmed that the following work well:

• #7721 is fixed
• #9115 is fixed
• #2547 is fixed
• confirmed that setting preferences with existing errors in the settings.json works
• confirmed that settings preferences with a dirty settings.json will prompt users to cancel, open the file, or save and retry - all actions work well
• confirmed that saving a dirty settings.json file will update the UI properly
• confirmed the behavior on all available scopes - user, workspace, folder (multi-root)

[msujew]

The changes look good to me as well 👍

• Changes to preferences in the UI or the respective settings.json file are updated immediately in the other
• The actions when the settings file contains unsaved changes all work as expected
• Works correctly for all preference scopes