SpartanJ / efsw

efsw is a C++ cross-platform file system watcher and notifier.
Other
645 stars 98 forks source link

Customization of file watchers #162

Closed kreuli closed 1 year ago

kreuli commented 1 year ago

This PR covers improvement #153: https://github.com/SpartanJ/efsw/issues/153

It adds a concept of 'WatcherOptions' that can be used to customize file watchers. Two such options have been implemented, to customize the Windows implementation of FileWatcher: an option to specify the buffer size for the ReadDirectoryChangesW API and another option to reduce the events specified in NotificationFilter. This can be used to adjust the FileWatchers for Windows, in case the currently used buffer of 63kB is not enough and the FileWatcher therefore does not work as of now.

Additionally, the return code of ReadDirectoryChangesW is read and LastError is set to a newly introduced error. This can be used to detect issues with defining a too small buffer size.

SpartanJ commented 1 year ago

Thanks for your contribution! I'll leave some minor comments on the source changes.

SpartanJ commented 1 year ago

@kreuli are you interested in implementing some of the changes requested? Let me know so I know how to proceed. Thank you.

kreuli commented 1 year ago

Hello Martin,yes, I‘d like to do that. Should I already see comments with requests for changes somewhere, because I am not aware of any.Cheers,UlrichAm 04.07.2023 um 15:56 schrieb Martín Lucas Golini @.***>: @kreuli are you interested in implementing some of the changes requested? Let me know so I know how to proceed. Thank you.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

SpartanJ commented 1 year ago

Hi @kreuli ! You can see the comments above your comment in the Pull Request. I guess you answered from your email: https://github.com/SpartanJ/efsw/pull/162 Also in the file changes there is more visibility: https://github.com/SpartanJ/efsw/pull/162/files You can also subscribe to the PR so you can get notifications each time I write here. Thanks

kreuli commented 1 year ago

Your review is shown as pending, which is why I can‘t see your code comments, I guess.

kreuli commented 1 year ago

Hi,I‘ve already fixed the typo on 28th of June. I still cannot see review requests btw 😢.Cheers,UlrichAm 12.07.2023 um 08:19 schrieb Sung Po-Han @.***>: @solarispika commented on this pull request.

In README.md:

@@ -78,6 +78,10 @@ efsw::WatchID watchID = fileWatcher->addWatch( "/tmp", listener, true ); // Adds another directory to watch. This time as non-recursive. efsw::WatchID watchID2 = fileWatcher->addWatch( "/usr", listener, false );

+// For Windows, adds another watch, specifying to use a bigger buffer, to not miss events +// (do not use for network locations, see efsw.hpp for detaisl!).

A typo here: detaisl should be details.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

solarispika commented 1 year ago

I found you've fixed the typo in the next commit, so I just removed the review ;) I was not to send the review immediately, but accidentally did it...

SpartanJ commented 1 year ago

@kreuli sorry, I forgot about this. I just published the review. Thank you

kreuli commented 1 year ago

I've just pushed changes as requested and also have updated the base of my branch to your newest master. There is now also a Standard C test application, basically the same as the C++ test application, to test the C wrapper.

SpartanJ commented 1 year ago

This is looking really good! Thank you so much for this. Please give me a few days since I'm not available to do a proper review right now but meanwhile I'll just send you some 2 minor comments.

kreuli commented 1 year ago

Just to be sure: is there something missing from my side?

SpartanJ commented 1 year ago

Just to be sure: is there something missing from my side?

Nothing missing! Thanks for your contribution!