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

Fix for #11401 #11409

Merged

Conversation

bbelyavsky-ti
Copy link
Contributor

@bbelyavsky-ti bbelyavsky-ti commented Jul 11, 2022

Signed-off-by: Baltasar Belyavsky [email protected]

What it does

Fixes: #11401

How to test

Unit tests implemented and included in this PR. In addition to that, in our own implementation of a Theia-based IDE, I've tested the fix using our implementation of a C++ error-parser which contributes markers to the Problems view when a C++ project is built. This bug was causing all markers on a C++ project scope to be cleared every time any file is deleted within the project. For more context - the class MarkerManager uses the FileChangesEvent.contains() method to clear markers associated with any deleted files. The FileChangesEvent.contains() intends to check if a file's ancestor directory has been deleted, as can be seen in the code comments. But this bug is causing the opposite effect, causing markers to be cleared on a project when any contained file is deleted.

Review checklist

Reminder for reviewers

Signed-off-by: Baltasar Belyavsky <[email protected]>
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

@bbelyavsky-ti Given that the changes require a special setup to test manually, do you mind addressing Vince's comment and add a unit test for this? While the changes look reasonable, it'd be great to confirm that it does work as intended before merging.

packages/filesystem/src/common/files.ts Outdated Show resolved Hide resolved
@vince-fugnitto vince-fugnitto added filesystem issues related to the filesystem file-watchers issues related to filesystem watchers - nsfw labels Jul 12, 2022
Signed-off-by: Baltasar Belyavsky <[email protected]>
Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

I was able to reproduce the problem and verify the solution. Two minor comments.

packages/filesystem/src/common/files.spec.ts Outdated Show resolved Hide resolved
packages/filesystem/src/common/files.spec.ts Outdated Show resolved Hide resolved
Signed-off-by: Baltasar Belyavsky <[email protected]>
Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

The fix works and the code and the tests look good 👍

@bbelyavsky-ti
Copy link
Contributor Author

Thank you for your input, @colin-grant-work, @msujew, @vince-fugnitto. Would one of you be able to merge this PR? I do not have the required permissions. Thank you!

@colin-grant-work colin-grant-work merged commit 433bed8 into eclipse-theia:master Jul 13, 2022
@bbelyavsky-ti bbelyavsky-ti deleted the bb/fix-file-changes-event branch July 13, 2022 16:55
@vince-fugnitto vince-fugnitto added this to the 1.28.0 milestone Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
file-watchers issues related to filesystem watchers - nsfw filesystem issues related to the filesystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in FileChangesEvent.contains() method causes unexpected behaviour
4 participants