Skip to content

Consume IFileWatcherService from CPS #9662

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

Merged
merged 7 commits into from
Jun 3, 2025
Merged

Conversation

LittleLittleCloud
Copy link
Contributor

@LittleLittleCloud LittleLittleCloud commented May 14, 2025

What does this PR do

This PR consumes the IFileWatcherService from CPS in LaunchSettingsProvider class so we can avoid create another system file watcher in project cone

relevant issues

Microsoft Reviewers: Open in CodeFlow

@LittleLittleCloud LittleLittleCloud requested a review from a team as a code owner May 14, 2025 23:27
@LittleLittleCloud LittleLittleCloud changed the title Add IFileWatcherService in order to enable VS Code File Watcher for dotnet-project-system-vscode services Add IFileWatcherService so we can enable VS Code File Watcher for dotnet-project-system-vscode services May 14, 2025
@LittleLittleCloud LittleLittleCloud changed the title Add IFileWatcherService so we can enable VS Code File Watcher for dotnet-project-system-vscode services WIP-Add IFileWatcherService so we can enable VS Code File Watcher for dotnet-project-system-vscode services May 15, 2025
@LittleLittleCloud LittleLittleCloud changed the title WIP-Add IFileWatcherService so we can enable VS Code File Watcher for dotnet-project-system-vscode services WIP - Add IFileWatcherService so we can enable VS Code File Watcher for dotnet-project-system-vscode services May 15, 2025
@LittleLittleCloud LittleLittleCloud changed the title WIP - Add IFileWatcherService so we can enable VS Code File Watcher for dotnet-project-system-vscode services Consume IFileWatcherService from CPS May 28, 2025
@LittleLittleCloud LittleLittleCloud requested a review from Copilot May 28, 2025 18:22
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new public interface IFileWatcherService and integrates it into LaunchSettingsProvider, enabling support for file watching via MEF. It updates both production and test code to stop using the legacy SimpleFileWatcher and instead use the injected IFileWatcherService.

  • Updated tests to pass a mock IFileWatcherService and remove direct use of SimpleFileWatcher.
  • Refactored LaunchSettingsProvider to implement IFileWatcherServiceClient, set up file watchers asynchronously, and handle file change notifications.
  • Removed the legacy CleanupFileWatcher and related code to streamline file watcher management.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
tests/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/ProjectSystem/Debug/LaunchSettingsProviderTests.cs Updated lambda formatting and constructor parameters to use the mock IFileWatcherService instead of creating a real file watcher.
src/Microsoft.VisualStudio.ProjectSystem.Managed/ProjectSystem/Debug/LaunchSettingsProvider.cs Integrated IFileWatcherService and refactored file watcher setup/disposal with asynchronous registration and unregistration.

Copy link
Member

@drewnoakes drewnoakes left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for banning the API too. I think that's a great improvement for future.

Left a few comments on minor things for you to take or leave at your discretion.

@davkean
Copy link
Member

davkean commented Jun 2, 2025

I think you want to think about these scenarios:

  1. The project is renamed Nevermind, forgot that we don't rename the folder underneath.
  2. The properties folder is renamed

…/Debug/LaunchSettingsProvider.cs

Co-authored-by: Drew Noakes <[email protected]>
@LittleLittleCloud
Copy link
Contributor Author

LittleLittleCloud commented Jun 2, 2025

I test the following scenarios

  • renaming properties folder

It seems that in both main and this branch, renaming properties folder doesn't work.

  • If the name of properties folder is Properties (default value), the folder can be renamed to other names like PropertiesA, but it can't be renaming back to Properties, with the following CPS warning
    image
  • If the name of properties folder is not the default value (Properties), then the folder can be renamed to other names that is not Properties and can be renaming back, but the launchSettingsProvider won't able to pick the updates from renamed launchSettings.json

The cause for the second issue is because we are not watching the folder change/renaming event, so LaunchSettingsProvider will not be notified if the folder of launchSettings.json get renamed. To address that issue, the LaunchSettingsProvider needs to be notified when the folder name of app designer properties get changed.

@drewnoakes @davkean how to listen for the changes of appdesigner properties folder name

@drewnoakes
Copy link
Member

Given rename doesn't work on main, it's a nice-to-have here. Feels like an edge case. Great if we can make it work, but we don't have to hold this up on it. Maybe file it as an issue.

A scenario that I do think we should check is the outright deletion and recreation of the Properties folder, and of the launchSettings.json file. Do we do the right thing when the file/folder are deleted/created in all combinations?

@LittleLittleCloud
Copy link
Contributor Author

A scenario that I do think we should check is the outright deletion and recreation of the Properties folder, and of the launchSettings.json file. Do we do the right thing when the file/folder are deleted/created in all combinations?

I created a tracking issue here: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2496401/ and assign myself to look at. Maybe the exact Properties name is held somewhere else given that folder with other names doesn't have that issue?

Given rename doesn't work on main, it's a nice-to-have here. Feels like an edge case. Great if we can make it work, but we don't have to hold this up on it. Maybe file it as an issue.

Seems that there's an issue to track that already: #2316
Maybe the launchSettingsProvider shouldn't cache the appdesignfolder.

@LittleLittleCloud LittleLittleCloud merged commit 9a4874d into main Jun 3, 2025
5 checks passed
@LittleLittleCloud LittleLittleCloud deleted the u/xiaoyun/filewatch branch June 3, 2025 16:37
@dotnet-policy-service dotnet-policy-service bot added this to the 17.14 milestone Jun 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants