Adding initial implementation of the FileSystemWatcher on OS X #1193
Conversation
| /// <summary> | ||
| /// This is the default system path for the CoreFoundation library that PInvoke will use | ||
| /// </summary> | ||
| private const string CoreFoundationLibrary = "/System/Library/Frameworks/CoreFoundation.framework/CoreFoundation"; |
There was a problem hiding this comment.
We should add an Interop.Libraries.cs file under src/Common/src/Interop/OSX, and put this constant in there.
|
Thanks, Jon. Good start. Took a first pass through and left a bunch of comments. I can review it again when you've taken a pass through to address them. |
|
@sokket, When you get a chance can you update your git user.email to be the microsoft.com address you have registered with your github account and then update your commit with git commit --amend --reset-author? Right now it is showing up as an outlook.com address. |
| Debug.Assert(_watcherRunLoop == IntPtr.Zero); | ||
|
|
||
| // Make sure we only do this if there is a valid directory | ||
| if (String.IsNullOrEmpty(_directory) == false) |
There was a problem hiding this comment.
Shouldn't this be if (!String.IsNullOrEmpty(_directory)) instead of checking for == false?
There was a problem hiding this comment.
They both provide the same functionality but I prefer the == false since it's so easy to miss the preceding ! and misread the conditional. If there's strong preference or precedence to using ! over == false then I'll go ahead and change it
|
Updated with feedback! |
| @@ -251,6 +249,7 @@ private sealed class RunningInstance | |||
| _includeSubdirectories = includeSubdirectories; | |||
| _notifyFilters = notifyFilters; | |||
| _cancellationToken = cancellationToken; | |||
| FileSystemWatcher.CaseSensitive = true; | |||
There was a problem hiding this comment.
This is ok, though what I'd had in mind was that each .Platform.cs file would still define its own CaseSensitive... in .Windows.cs and .Linux.cs it'd be a constant, and in .OSX.cs it'd be a property. That way, for the systems where it's a fixed constant, call sites could optimize appropriately. Not a big deal, though.
There was a problem hiding this comment.
I made it a property on the base class since, technically, Windows and NTFS supports POSIX compliant-case-sensitivity for apps that support it (https://support.microsoft.com/en-us/kb/100625).
I believe you can also do some customization on Linux platforms to support case insensitivity, so to support all these potential scenarios, this seemed like the best option. I can change it back to constants if you'd like
There was a problem hiding this comment.
Ok. I don't feel strongly about it. We can always change it in the future if we so desire.
|
Thanks, Jon. A few more minor comments, but otherwise LGTM. Thanks for all the fixes. |
|
@dotnet-bot test this please |
1 similar comment
|
@dotnet-bot test this please |
Adding initial implementation of the FileSystemWatcher on OS X
No description provided.