Skip to content
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

[monaco] non-blocking bulk edits #8329

Merged
merged 3 commits into from
Aug 11, 2020
Merged

Conversation

akosyakov
Copy link
Member

@akosyakov akosyakov commented Aug 8, 2020

What it does

  • fix [monaco] bulk editor service is not safe to use concurrently #8328: apply text edits without requiring an editor to be revealed or focused, otherwise it is not guarantee that edits from multiple bulk calls will be applied in the proper order. I've discovered an issue with the private VS Code extension doing modifications in the background, by the time when a window was revealed the document was total mess.
  • This PR as well fixes bugs of:
    • marking a document as dirty with changes which lead to the same state, thinking replace the entire doc with the same content;
    • not respecting EOL changes.

How to test

Apply some cross file refactoring for Typescript or Java, like rename type referenced from other files. Other files still should be opened as dirty and changes should be applied.

Review checklist

Reminder for reviewers

@akosyakov akosyakov added the monaco issues related to monaco label Aug 8, 2020
Copy link
Contributor

@RomanNikitenko RomanNikitenko left a comment

Choose a reason for hiding this comment

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

@akosyakov
I tested renaming for a typescript type with and without Auto Save.
The changes are applied for both cases.

rename_theia

Other files still should be opened as dirty

I can see that behavior if Auto Save is turned off.

I see some difference with VS Code behavior:

  • VS Code does not open related files for applying changes if Auto Save is turned on:
  • VS Code opens related files if Auto Save is turned off

rename_vsCode

Just a question - if it was expected to align the behavior with VS Code within current PR?

@akosyakov
Copy link
Member Author

Just a question - if it was expected to align the behavior with VS Code within current PR?

To some extent yes, my main concern was to resolve blocking issues though. I will look how hard is to align auto save handling.

@akosyakov
Copy link
Member Author

@RomanNikitenko I've pushed another commit to only save model if auto save is on.

@kittaakos
Copy link
Contributor

@akosyakov, please remove the EditorOpenerOptions arg as it is unused. As a downstream consumer, I want to see a compiler error if the behavior has changed. Please add the breaking changes you made to the changelog. Thanks!

@akosyakov
Copy link
Member Author

please remove the EditorOpenerOptions arg as it is unused As a downstream consumer, I want to see a compiler error if the behavior has changed.

sorry did not get it. EditorOpenerOptions is used in EditorManager. The behaviour is changed only for case with auto save on, in other cases it should work as before but non-blocking and respect EOL changes.

@kittaakos
Copy link
Contributor

sorry did not get it.

I do not see the options arg used by the applyBulkEdit method.

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>
@akosyakov
Copy link
Member Author

@kittaakos good catch! removed and updated CHANGELOG

Copy link
Contributor

@RomanNikitenko RomanNikitenko left a comment

Choose a reason for hiding this comment

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

Tested after update: mentioned use case was aligned with VS Code behavior

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes work well (tested using a cross-file rename symbol):

  • autoSave = on:
    The edit completes successfully without the need to open the editors
  • autoSave = off:
    The edit completes successfully, corresponding files are opened (marked as dirty) with the proper edit changes

@akosyakov akosyakov merged commit c205cfb into master Aug 11, 2020
@akosyakov akosyakov deleted the ak/non_blocking_bulk_edit branch August 11, 2020 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
monaco issues related to monaco
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[monaco] bulk editor service is not safe to use concurrently
4 participants