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 bug + Add FS Event for iOS #20

Merged
merged 7 commits into from
May 8, 2016
Merged

Fix bug + Add FS Event for iOS #20

merged 7 commits into from
May 8, 2016

Conversation

ijumps
Copy link

@ijumps ijumps commented May 6, 2016

Add GCD based FS Event. Support for iOS
Fix bug: crash when NSFileManager delegate deinit.
Fix bug: commonAncestor
Fix bug: miss self in FileKit/Core/File.swift
Using _rawValue in API calls to avoid empty string which may cause crash.
Using isAny to detect any type of file exists at path.
Using stdWithTilde to standardize the path without expanding tilde.
Better support for relative path. Like Path("./../dir1/../dir2/file/.//").parent is Path("../../dir2/file")
Annotations for follow links or not.
Support for expanding tilde.

ijump@mac added 2 commits May 6, 2016 21:41
Using _rawValue in API calls to avoid empty string which may cause crash.
Using isAny to detect any type of file exists at path.
Using stdWithTilde to standardize the path without expanding tilde.
Better support for relative path. Like Path("./../dir1/../dir2/file/.//").parent is Path("../../dir2/file")
Annotations for follow links or not.
Support for expanding tilde.
@nvzqz
Copy link
Owner

nvzqz commented May 8, 2016

These are some great changes overall, however there's a number of issues/quirks that should be fixed before merging. Great idea with the RelativePathType enum, by the way!

Can you please change static properties' names from caps to upper camel case? For example, GCDVNodeEvents and GCDSourceType. Also, if the name is shortened, expand it. For example, "ATTRIB" should be "Attribute".

Please don't make a type public if it has no public values or methods, such as with GCDSourceType.

Also, Path's watch2() method isn't any different from the watch() method that was made exclusively for iOS. Also, why is that for iOS only?

Also, please expand "std" to "standard" in property names. stdRaw should be expanded to standardRawValue. The same goes for stdRawWithTilde.

Public API shouldn't be prefixed with "", such as in the case with "rawValue" for Path. The "" prefix should be reserved for internal or private names. A suggested rename would be "dotOrRawValue" or something more descriptive since it's essentially a dot unless there's a nonempty raw value. Also, since it's only being used internally to work with NS-API, it should preferably be marked as internal. Also, if the parent of a private value is also private, that value shouldn't have a "" prefix, such as in the case of _FMWrapper's fileManager property. To avoid confusion, there shouldn't be two versions of something with and without a "_" prefix. Their names should clearly and concisely identify the difference.

The manual initializer and rawValue aren't necessary for RelativePathType to have a String raw value. Swift can handle that for you.

Many of the #if clauses only take into account iOS and OS X, but not tvOS and watchOS. API such as imageFromURLString will be broken on those platforms.

ijump@mac added 5 commits May 8, 2016 18:54
rename "GCDVNodeEvents" to "DispatchVnodeEvents"
rename "GCDSourceType" to "DispatchSourceType"
rename "GCDVNodeWatcher" to "DispatchVnodeWatcher"
rename "GCDFSWatcherDelegate" to "DispatchVnodeWatcherDelegate"
public "_rawValue" -> private "_rawValue", use "safeRawValue" instead
_fileManager -> unsafeFileManager
fix crash on unowned self.
call watch2 when on OSX.
@nvzqz nvzqz merged commit fa65d08 into nvzqz:develop May 8, 2016
@nvzqz
Copy link
Owner

nvzqz commented May 8, 2016

Thanks for the contributions!

@ijumps
Copy link
Author

ijumps commented May 8, 2016

GCDSourceType or say DispatchSourceType is public, type like Read maybe use in the future to monitor for pending bytes available to be read.

Since keyword watch is used in OSX, I just use use watch2 if you want to monitor events with GCD. I check the GCD API available on all platforms, but only test it on iOS. I'm not familiar with watchOS and tvOS, maybe someone help to test it.

I keep _rawValue but offer a new public function safeRawValue, since some extension may need to call it out of file.

I change the name _ fileManager to unsafeFileManager. Since fileManager's delegate is defined as unowned(unsafe), that may cause crash.

Try in swift can't catch Exception for now, empty string "" in some NSAPI may cause crash.

imageFromURLString is available on iOS and OSX only(I use if and elseif). I check all the #if, and no unavailable API used, should be OK.

components function is used for standardize a path, even it's a relative path. A standardize relative path should like this: ../../path or .. or "" or path. Family functions support relative path too. Note: If any of two paths is a relative path, they are both absolutized first.

DispatchVnodeWatcher can use delegate / blocks as a callback. When a block is defined, delegate is ignored. DispatchVnodeWatcher use a hack way to monitor a create event, by monitor its parent's write event, and check if path exist each time, not a efficient way. I don't use a weak/unowned self in dispatch_source_set_event_handler, so self will not deinit while running.

Since there is a lot of changes, maybe exist some bugs. Anyway thanks for your work too!

@nvzqz
Copy link
Owner

nvzqz commented May 8, 2016

The GCD-based watch() function seems to work a lot more nicely than the filesystem event one. I might discard that one in place of this.

From my understanding, watchOS and tvOS work pretty much just like how iOS does. I could be wrong, though.

Edit: When using your watch() function in the FileKit playground, the callback is never called on iOS and tvOS. However, on OS X it is.

@ijumps
Copy link
Author

ijumps commented May 9, 2016

I figure out why this happened. When watch a non exists path, an asynchronous delegate/blocks is set to monitor its parent. Even call dispatch_resume(source!) is synchronous, the handler in dispatch_source_set_event_handler(source!) call asynchronous. For a iOS or tvOS simulator, the speed is much slow than on a Mac(OS X). So when a parent watch is set, and the queue is about to run, operate for create the path already finished. A create event only happens one time, so the callback is never called .

I test it on a real iOS device, this happens less. However, change the code from let watch = { parent.watch2($0, callback: $1) } to let watch = { parent.watch($0, queue: dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_HIGH, 0), callback: $1) } is likely to solve the problem(Test ok for me).

The queue call on a global concurrent queue. This may cause some weird behavior when the caller run on a different queue.

Edit: I have done some more test, weird behavior still happens. General reason is that: after let watcher = pathToWatch.watch {...}, operation() is call immediately. But GCD works asynchronous.

When an event occurs, the dispatch source submits your task code asynchronously to the specified dispatch queue for processing.

The order is unpredictable. Maybe call sleep(1) before operation() to delay the operation.

Edit: I add some test and bugfix on https://github.com/ijumps/FileKit/tree/bugfix-watch, more details can be found in the iOS test file.

Behave differs between test on a simulator and a device. It's more likely to lose some events on a simulator.

Create event implement by a non native GCD api. Maybe lose some events even on a device

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.

2 participants