Listen for file renames #987

rchande · 2017-10-24T20:36:09Z

Use the improved file watching in dotnet/vscode-csharp#1805 to better handle file renames.
When adding files to the workspace, we now set up file watching for each directory for which we add a document. If a file deletion comes in from such a directory, we'll remove any applicable documents from the workspace. If a .cs file is created, we kick off a task for the buffer change notification service to use its logic to add the file to applicable projects.

These changes allow users to rename files from the VS Code file tree without getting errors about duplicate type declarations. For projects with file globbing, the workspace will automatically wire up the new file and the user won't see any errors about missing types.

…t was made to the file.

* Pass through FileChangeTypes when available * Allow clients to watch a directory (in addition to individual files)

rchande · 2017-10-24T20:36:29Z

Tag @DustinCampbell @filipw

rchande · 2017-10-24T20:40:39Z

Fixes dotnet/vscode-csharp#785

DustinCampbell

Looks very good! Just a couple of things.

DustinCampbell · 2017-10-24T20:59:11Z

src/OmniSharp.Abstractions/FileWatching/IFileSystemWatcher.cs

@@ -1,12 +1,20 @@
-using System;
+using OmniSharp.Models.FilesChanged;
+using System;


sort usings should have System on top.

DustinCampbell · 2017-10-24T21:00:08Z

src/OmniSharp.Abstractions/Models/v1/FilesChanged/ChangeTypeConverter.cs

+        {
+            switch (reader.Value as string)
+            {
+                case "Change":


These should probably be case-insensitive string matches.

Removed this and switched to the automatic serialization offered by json.net, which is case insensitive.

DustinCampbell · 2017-10-24T21:01:55Z

src/OmniSharp.Abstractions/FileWatching/IFileSystemWatcher.cs

+        /// </summary>
+        /// <param name="path">The path to the file</param>
+        /// <param name="verb">The type of change. Hosts are not required to pass a change type</param>
+        void TriggerChange(string path, FileChangeType? verb);


IMO, the terms type and verb should match. So, if the type is FileChangeType, the parameter here should be type. Or, the type should be FileChangeVerb.

it somehow feels that an enum with default value would be more intuitive than nullable enum? at least that's what MSDN would tell us 😀

Fixed names and added a default value.

DustinCampbell · 2017-10-24T21:02:59Z

src/OmniSharp.DotNet/DotNetProjectSystem.cs

@@ -166,13 +166,13 @@ private void UpdateProject(string projectDirectory)
            _projectStates.Update(projectDirectory, contexts, AddProject, RemoveProject);

            var projectFilePath = contexts.First().ProjectFile.ProjectFilePath;
-            _fileSystemWatcher.Watch(projectFilePath, file =>
+            _fileSystemWatcher.Watch(projectFilePath, (file, verb) =>


See my comment above about type vs. verb. If you go with type, be sure to update this as well.

DustinCampbell · 2017-10-24T21:03:05Z

src/OmniSharp.DotNet/DotNetProjectSystem.cs

            {
                _logger.LogInformation($"Watcher: {file} updated.");
                Update(allowRestore: true);
            });

-            _fileSystemWatcher.Watch(Path.ChangeExtension(projectFilePath, "lock.json"), file =>
+            _fileSystemWatcher.Watch(Path.ChangeExtension(projectFilePath, "lock.json"), (file, verb) =>


DustinCampbell · 2017-10-24T21:06:31Z

src/OmniSharp.MSBuild/MSBuildProjectSystem.cs

+        private void OnDirectoryFileChanged(string path, FileChangeType? changeType)
+        {
+            // Hosts may not have passed through a file change type
+            if (changeType == null && !File.Exists(path) || changeType.GetValueOrDefault() == FileChangeType.Delete)


I don't think you need the GetValueOrDefault() here.

DustinCampbell · 2017-10-24T21:06:36Z

src/OmniSharp.MSBuild/MSBuildProjectSystem.cs

+                }
+            }
+
+            if (changeType == null && File.Exists(path) || changeType.GetValueOrDefault() == FileChangeType.Create)


DustinCampbell · 2017-10-24T21:06:55Z

src/OmniSharp.MSBuild/MSBuildProjectSystem.cs

+            if (changeType == null && File.Exists(path) || changeType.GetValueOrDefault() == FileChangeType.Create)
+            {
+                // Only add .cs files to the workspace
+                if (Path.GetExtension(path) == ".cs")


Should be a case-insensitive comparison.

DustinCampbell · 2017-10-24T21:07:49Z

tests/OmniSharp.Roslyn.CSharp.Tests/FilesChangedFacts.cs

+using System.Threading.Tasks;
+using TestUtility;
+using Xunit;
+using Xunit.Abstractions;


Usings should be sorted with System on top.

DustinCampbell · 2017-10-24T21:09:04Z

tests/OmniSharp.Roslyn.CSharp.Tests/FilesChangedFacts.cs

+                handler.Handle(new[] { new FilesChangedRequest() { FileName = "FileName.cs", FileChangeType = FileChangeType.Create } });
+
+                Assert.Equal("FileName.cs", filePath);
+                Assert.Equal(FileChangeType.Create, ct);


Is it worth checking to see if the file is now included in the workspace? I believe you can get to it with host.Workspace.

david-driscoll

The presence of the custom JsonConveter has me confused, other than that 👍

david-driscoll · 2017-10-24T20:58:42Z

src/OmniSharp.Abstractions/Models/v1/FilesChanged/ChangeTypeConverter.cs

+        public override bool CanConvert(Type objectType) => objectType == typeof(FileChangeType);
+        public override object ReadJson(JsonReader reader, Type objectType, object existingValue, JsonSerializer serializer)
+        {
+            switch (reader.Value as string)


Any reason this isn't using Enum.TryParse<FileChangeType>("abc", true, out var item)?

Either way it should probably be invariant in case other editors send through the wrong casing.

david-driscoll · 2017-10-24T21:00:13Z

src/OmniSharp.Abstractions/Models/v1/FilesChanged/FilesChangedRequest.cs

+    [OmniSharpEndpoint(OmniSharpEndpoints.FilesChanged, typeof(IEnumerable<FilesChangedRequest>), typeof(FilesChangedResponse))]
+    public class FilesChangedRequest : Request
+    {
+        [JsonConverter(typeof(FileChangeTypeConverter))]


Couldn't this just be StringEnumConverter?

I think that's right.

david-driscoll · 2017-10-24T21:02:10Z

src/OmniSharp.MSBuild/MSBuildProjectSystem.cs

+        private void OnDirectoryFileChanged(string path, FileChangeType? changeType)
+        {
+            // Hosts may not have passed through a file change type
+            if (changeType == null && !File.Exists(path) || changeType.GetValueOrDefault() == FileChangeType.Delete)


Minor nit it's more obvious that the value is Nullable<> if we use !changeType.HasValue.

david-driscoll · 2017-10-24T21:06:41Z

src/OmniSharp.Abstractions/Models/v1/FilesChanged/ChangeTypeConverter.cs

+                    return null;
+            }
+        }
+        public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer) => throw new NotImplementedException();


If we keep this we should override CanWrite and return false, would hate to have this throw unexpectedly.

Use json.net implicit enum serialization Add an "unspecified" value to FileChangeType and remove nullables

rchande · 2017-10-25T16:41:51Z

Incorporated feedback. Want to sign off @DustinCampbell @david-driscoll ?

@DustinCampbell regarding the test, it looks like all of the existing test helpers create projects that use the "dotnet" project system. I think it might be worth going through and updating the tests to use the "msbuild" project system, or to run against both project systems.

DustinCampbell

Looking much better! Could you take a look at my comment about delegate registration with the ManualFileSystemWatcher

DustinCampbell · 2017-10-25T17:14:05Z

src/OmniSharp.MSBuild/MSBuildProjectSystem.cs

+using System.Composition;
+using System.IO;
+using System.Linq;
+using System.Threading.Tasks;


Sort usings with System namespaces on top.

I swear my option is resetting underneath me.

bummer 😞

Maybe you've discovered a VS bug via dogfooding?

DustinCampbell · 2017-10-25T17:17:51Z

src/OmniSharp.Host/FileWatching/ManualFileSystemWatcher.cs


 namespace OmniSharp.FileWatching
 {
    public class ManualFileSystemWatcher : IFileSystemWatcher
    {
-        private readonly Dictionary<string, Action<string>> _callbacks = new Dictionary<string, Action<string>>();
+        private readonly Dictionary<string, Action<string, FileChangeType>> _callbacks = new Dictionary<string, Action<string, FileChangeType>>();
+        private readonly Dictionary<string, Action<string, FileChangeType>> _directoryCallBacks = new Dictionary<string, Action<string, FileChangeType>>();


One concern I have here is that only a single callback can be called for a particular path. And, the last callback registered will win. I don't think this will be a problem so far, but it may be in the future -- especially since we're watching directories. I can easily imagine some other project system (like script or cake) wanting to listen to the same directories that the MSBuild project system does. Should we combine callbacks using Delegate.Combine(...) and Delegate.Remove(...)?

That's a good point--will add this

DustinCampbell · 2017-10-25T17:18:25Z

src/OmniSharp.Roslyn.CSharp/Services/Files/OnFilesChangedService.cs

 using OmniSharp.Models.FilesChanged;
+using System.Collections.Generic;
+using System.Composition;
+using System.Threading.Tasks;


Sort usings with System namespaces on top please.

DustinCampbell · 2017-10-25T17:18:37Z

tests/OmniSharp.Roslyn.CSharp.Tests/FilesChangedFacts.cs

+
+namespace OmniSharp.Roslyn.CSharp.Tests
+{
+


DustinCampbell · 2017-10-25T17:21:03Z

Regarding the test, it looks like all of the existing test helpers create projects that use the "dotnet" project system. I think it might be worth going through and updating the tests to use the "msbuild" project system, or to run against both project systems.

The appropriate way to do this is not to create dummy projects in tests. Instead, you can use one of the test projects like we do in the OmniSharp.MSBuild tests like so: https://github.com/OmniSharp/omnisharp-roslyn/blob/master/tests/OmniSharp.MSBuild.Tests/WorkspaceInformationTests.cs#L18-L32.

DustinCampbell · 2017-10-25T17:34:20Z

src/OmniSharp.Abstractions/Models/v1/FilesChanged/FilesChangedRequest.cs

 using OmniSharp.Mef;
+using System.Collections.Generic;


Sort System namespaces first please. You've got the option set incorrectly in VS (this is also the preference in Roslyn).

(TODO: allow unsubscribing)

…oslyn into fileWatching

rchande · 2017-10-25T20:52:39Z

@DustinCampbell Incorporated your latest feedback

DustinCampbell · 2017-10-25T21:01:36Z

src/OmniSharp.Host/FileWatching/ManualFileSystemWatcher.cs

+        {
+            if (_directoryCallBacks.TryGetValue(path, out var existingCallback))
+            {
+                _directoryCallBacks[path] = (Action<string, FileChangeType>)Delegate.Combine(callback, existingCallback);


I'm OK leaving this as is, but it's also possible to just write:

_directoryCallBacks[path] = callback + existingCallback;

Believe it or not, + handles combination of delegates in C#. 😄

The More You Know

So much nicer. Fixing.

DustinCampbell · 2017-10-25T23:34:54Z

@david-driscoll : Does this look better to you?

…into fileWatching

rchande · 2017-10-26T15:49:14Z

Found some issues while testing--let's hold off on merging this for a bit.

david-driscoll

Changes look good, only one comment about the serialization, but I don't really have a preference.

david-driscoll · 2017-10-26T16:14:17Z

src/OmniSharp.Abstractions/Models/v1/FilesChanged/FilesChangedRequest.cs

+    [OmniSharpEndpoint(OmniSharpEndpoints.FilesChanged, typeof(IEnumerable<FilesChangedRequest>), typeof(FilesChangedResponse))]
+    public class FilesChangedRequest : Request
+    {
+        public FileChangeType ChangeType { get; set; }


This will serialize as an int (I think, I didn't see any where we configure Newtonsoft.Json)

…oslyn into fileWatching

rchande · 2017-10-27T00:36:32Z

This is ready to merge. Can you hit the button @DustinCampbell ?

Ravi Chande added 9 commits October 24, 2017 10:46

Add a dedicated FilesChangedRequest type that includes the change tha…

70f8486

…t was made to the file.

Update IFileSystemWatcher to:

47804cf

* Pass through FileChangeTypes when available * Allow clients to watch a directory (in addition to individual files)

Implement new FileWatcher members

2459ea0

Consume better file change notifications from MSBuild project system

1096cd6

Update namespaces

c46fad5

Add a test

2f2ea4c

Delete dead files

d5a8477

Comment

e9e1fd7

Add using block to fix test

2882a33

rchande mentioned this pull request Oct 24, 2017

Renamed documents should be removed from the workspace and not cached #783

Closed

DustinCampbell requested review from david-driscoll, DustinCampbell and filipw October 24, 2017 20:58

DustinCampbell requested changes Oct 24, 2017

View reviewed changes

david-driscoll requested changes Oct 24, 2017

View reviewed changes

Ravi Chande added 5 commits October 25, 2017 08:19

verb -> changeType

c9db400

FileChangeType -> ChangeType

634be55

Use json.net implicit enum serialization Add an "unspecified" value to FileChangeType and remove nullables

Remove FileChangeType?

4db48c7

Sort usings

9847dc4

Merge branch 'master' into fileWatching

2b19763

DustinCampbell reviewed Oct 25, 2017

View reviewed changes

DustinCampbell and others added 4 commits October 25, 2017 13:17

Merge branch 'master' into fileWatching

c7884fe

Wait for buffer update in order to be testable

b3cb182

Sort usings

3ef284e

Allow multiple clients to watch the same directory

14e9626

(TODO: allow unsubscribing)

Ravi Chande added 3 commits October 25, 2017 13:49

Add a test for multiple directory watchers

b122830

Merge branch 'fileWatching' of https://github.com/rchande/omnisharp-r…

9802d7e

…oslyn into fileWatching

sort usings

9796a50

DustinCampbell reviewed Oct 25, 2017

View reviewed changes

DustinCampbell approved these changes Oct 25, 2017

View reviewed changes

Use delegate combining syntax

e8652fa

DustinCampbell and others added 2 commits October 26, 2017 05:55

Merge branch 'master' into fileWatching

9b96a8d

Merge branch 'master' of https://github.com/OmniSharp/omnisharp-roslyn …

f305c86

…into fileWatching

david-driscoll approved these changes Oct 26, 2017

View reviewed changes

Ravi Chande added 4 commits October 26, 2017 09:40

Don't attempt to add Documents for directory names that end in .cs

36c145b

Merge branch 'master' into fileWatching

5d0b91b

Merge branch 'fileWatching' of https://github.com/rchande/omnisharp-r…

858bcbb

…oslyn into fileWatching

Merge branch 'master' into fileWatching

259bc76

DustinCampbell merged commit 22e0183 into OmniSharp:master Oct 27, 2017

Listen for file renames #987

Listen for file renames #987

Conversation

rchande commented Oct 24, 2017

rchande commented Oct 24, 2017

rchande commented Oct 24, 2017

DustinCampbell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-driscoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchande commented Oct 25, 2017

DustinCampbell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell commented Oct 25, 2017 • edited Loading

Choose a reason for hiding this comment

rchande commented Oct 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell commented Oct 25, 2017

rchande commented Oct 26, 2017

david-driscoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchande commented Oct 27, 2017

DustinCampbell commented Oct 25, 2017 •

edited

Loading