Additional docs for the kernel notifications, move them to inner class

[kyanha]

Adding additional remarks for the kernel notification functions.

DokanNetMirror would learn about the need to call these functions from System.IO.FileSystemWatcher. A database-backed filesystem might learn about the need to call these if it queried the file name table, and compared it to its in-memory cache. Specifically, the notification functions tell the kernel about changes that happened outside of the normal filesystem flow. (Changes that happen as a result of normal filesystem calls are automatically known by the kernel, but things that happen to the filesystem as a result of external events aren't.)

[Liryna]

Hi @kyanha ,

By looking at this and the duplicate doc (and seems like there is no other ways like a "group" documentation :'( ), maybe we could have a subclass DokanNotify (or Notify ?) inside Dokan that would have the global documentation and each function Create, Rename,.... inside ? What do you think ?

[kyanha]

I don't have a problem with relocating these functions into a Dokan.NotifyTheKernel internal class (pick a better name, and please let me know what it should be), and adding documentation on that class for Doxygen to pick up and put into the docs. But, I have to be honest: I almost never look at the Doxygen documentation.

Instead, I use Visual Studio's Intellisense lookup of the XML documentation, and its accessibility to people who didn't come up into programming through formal schooling. The <remarks> tag contents show up when I hover over the name of the method, which is the first thing I look at when something doesn't behave properly.

I'll implement this, and if you want me to change the name of the internal class just let me know.

(On a slight tangent: As far as I can see, we can't really do much handholding for the programmer as far as constructing the path to notify for. For Mirror, the implementation could become aware of changes through System.IO.FileSystemWatcher. For a filesystem implemented in a database, the implementation could become aware of changes through triggered notifications from that database (which I'd expect in a database filesystem that allowed multiple users/machines to connect to it, and properly handled sharing and byte-range locking). It must be up to the implementation to track its mountpoint, and to map its files onto it. Can you think of a way we could make it easier, slightly better than a minimal CreateFullPathname(string MountPoint, string FileName) utility method?)

[Liryna]

For the documentation we are mostly using <summary> supported by doxygen and visual studio. Group was an example from dokan.h source that yeah ... is more doxygen than vs as you pointed 👍
Here we will do a class so we will be able to use the <summary> instead of a group doc.

About System.IO.FileSystemWatcher I fully agree, as there are so many possibilities of dokan fs implementation, I do not see very valuable to create some default FileSystemWatcher. Either, CreateFullPathname will be up to the dev, I am not sure it is really required and helpful 🤔

[kyanha]

@Liryna @Lukas0610 Could you please look this over? I moved the notification functions to an inner class per Liryna's comment, and I want to make sure the docs make sense.

Again, if the inner class name doesn't make sense, I'll change it (or you can, since I marked 'allow maintainers to edit' on the PR).

[Lukas0610]

NotifyTheKernel sounds rather strange for me, maybe keep it at just Notify + in that case you could remove the DokanNotify* prefixes from the methods, Dokan.Notify.Create(...) should be clear what the functions does, maybe FileCreated, FileDeleted, etc.

For the documentation, and it's of course just in my opinion, my proposed description at dokan-dev/dokany#833 describes the notification functions and how they should be used in a more clear way.

[Liryna]

Thank you @kyanha for the quick changes 👍
I agree @Lukas0610 for the naming changes. Dokan.Notify.Create(...) looks good. I just think we should not call FileCreated as it could be a file or a folder....even if we follow windows we should ( CreateFile)

@kyanha I just merged #245 , you might want to pull/rebase to not have any merge conflict.
For the documentation, just reuse the one from dokan.h, it should be good enough or if you find any improvement, it is always welcome 👍

[kyanha]

I agree @Lukas0610 for the naming changes. Dokan.Notify.Create(...) looks good. I just think we should not call FileCreated as it could be a file or a folder....even if we follow windows we should ( CreateFile)

How about ExternalCreate, ExternalDelete, ExternalUpdate, ExternalXAttrUpdate, and ExternalRename, to make the names more self-documenting? i.e., Dokan.Notify.ExternalCreate to notify Dokan of an external file creation.

(I discussed this with a couple professional programmer friends [one game programmer and one hardware/firmware/driver/user full-stack programmer], and they like the self-documenting nature of C#'s library structure. They preferred 'Dokan.Notify.ExternalOperation' as the method name schema. I will write it to whatever spec you choose; I just want to give it some thought before committing to a final design.)

Edit to add: I implemented this without the 'External' prefix on the method names, to Lukas's spec as modified. After putting the docs on Dokan.Notify, it's a lot clearer that they are always external to the IDokanOperations implementation.

[Liryna]

ExternalCreate, ExternalDelete, ExternalUpdate, ExternalXAttrUpdate, and ExternalRename looks to be a great idea ! I am OK for it 👍 what is your opinion @Lukas0610 ?

[Lukas0610]

I'd append a d to each functions, to have a functionally correct function-name, as we don't actively create/update/delete something, but just notify someone: External[File/Directory has been]Created.

ExternalCreated, ExternalDeleted, ExternalUpdated, ExternalXAttrUpdated, and ExternalRenamed would be my suggestions in that case

[kyanha]

Now we're getting into English grammar (its tense rules), and I'm not sure it makes sense to do it that way. To me, ExternalCreate suggests "an external create operation occurred" versus "A file was externally created". If we're going with English tense rules, I'd feel better with Dokan.Notify.ExternallyCreated.

But that also gets into "XAttr Externally Updated", versus "an External XAttr Update operation occurred", which breaks the name of ExternalXAttrUpdated.

When I told my friends I needed some help with method naming, one of the questions I was asked was, "Do the clients care about the source of the operation?" I said yes, because if the operation is through the standard filesystem operations the kernel already knows and doesn't need to be notified. If the operation is performed outside of the filesystem interface, it does. But thinking on it, I'm not so sure.

Suppose you have a filesystem that exposes .jpg file JFIF data as a separate file: when a .jpg file is created, the filesystem automatically 'creates' a new .jfif file for it, and needs to notify the kernel of its creation and availability. In that case, the creation happens internal to the filesystem, and 'External' isn't really a good descriptor for it. (external to the filesystem? external to the kernel filesystem interface?).

In the Mirror example, 'External' is a good descriptor, because everything that happens to it is external to the mirror's presentation. But not every implementation is going to need to notify based on externalities -- some implementations will need to notify based on internal actions that still don't come from the kernel.

tl;dr: naming is hard. I'd like to withdraw my suggestion for prefixing the names with 'External'. This PR was already implemented without said prefix, and the docs make it clear that notifications only need to happen outside the normal IDokanOperations flow.

[Liryna]

For me, current state PR is good and documentation is more than enough thank to both of you.
I can merge it if @Lukas0610 has nothing against it.
This seems to be the last requirement to release a 2.1.3.0 #158 (comment) ?

[Lukas0610]

No problems on my side

[Liryna]

Thanks to both of you again @kyanha @Lukas0610 🏆