#381 OWLMaker does not reload opened files

[Ognyan Chernokozhev]

If a file opened for editing in OWLMaker is updated by another application, the file is not reloaded.

[Ognyan Chernokozhev]

Implmented a simple FileWatcher class as described in this article: https://sourceforge.net/p/owlnext/wiki/Monitoring%20file%20for%20changes/

[Ognyan Chernokozhev]

Fixed in [r4509].

[Vidar Hasfjord]

Excellent! This can prevent lost work.

[Vidar Hasfjord]

I've now reviewed your changes — comments below. That said, do not feel obliged to spend more time on this feature. It seems to work satisfactorily in the common case, in which a file is open and modified in two editors. If you leave it as is, I might refactor later for fun and practice.

Issues:

• The warning message in EvMessage has duplicated words ("has has").
• Replacing a file (e.g. Paste in Explorer) does not trigger the change notification.
• Changes to multiple files create multiple dialog boxes on top of each other. The way Visual Studio deals with this, is to accumulate all the changes into one dialog, allowing the user to accept or reject all changes at once (and accept/reject individual changes, also?).
• The notification dialog appears when OWLMaker detects the change, possibly disrupting the user if the focus is elsewhere. The way Visual Studio deals with this, is to delay showing the notification dialog until focus is returned to Visual Studio.
  
  

Nitpicking:

• Default functions in FileWatcher should be defined or deleted (rule-of-five).
• Eliminate conversion warning in FileWatcher::ProcessMessage.
• There is no error handling for the StartWatching return code (ignored).
• FileWatcher::ProcessMessage is not exception safe (std::string constructor may throw, in which case SHChangeNotification_Unlock will not be called). Either eliminate the use of the string constructor (do C-style string comparison) and mark the whole function "noexcept", or use RAII to ensure unlocking is done.
• FileWatcher files should follow the same coding conventions as the rest of OWLMaker, i.e. modern C++ style with trailing return types and auto-style declarations with const-correctness ("const auto a = ..."), brace initialisation preferred instead of call syntax, "nullptr" used instead of 0, exception handling preferred instead of error return codes etc.
• FileWatcher should follow naming conventions (i.e. rename to "TFileWatcher").
• TOWLMakerEdit::watcher should follow naming conventions (i.e. rename to "Watcher").
• TOWLMakerEdit::watcher should be initialised in the TOWLMakerEdit constructor.
• Prefer single-step initialisation of FileWatcher in its constructor. TOWLMakerEdit can hold a unique_ptr <FileWatcher> (or use std::optional). Then a construction of FileWatcher can be performed when a file is opened/saved (SetupWindow and FileSaveAs), rather than using delayed initialisation and resetting (Init, StartWatching and StopWatching then becomes obsolete — a watcher only exists during the time a particular file is being watched). This would also eliminate members hwnd and msg, which are only needed for the delayed initialisation.
• Rename TOWLMakerEdit::EvMessage to something more descriptive, e.g. "EvShChangeNotification".
• The member FileWatcher::file is only needed to verify that the notification passed to ProcessMessage concerns the file watched. However, the class already holds the member pidl identifying the file. Hence, the file member can be eliminated and verification done using the pidl directly (using IShellFolder::CompareIDs).

[Vidar Hasfjord]

Hi Jogy, I have now performed some major refactoring of your code. Sorry! :-)

I was curious to understand this code (I did similar things around the ReadDirectoryChanges API, to support progress indication during build), and it was too much fun to resist refactoring. I've done most of the things mentioned in my last post, and I fixed the warning message for the message box. The issues regarding multiple notifications and disruption remain, but I think those are probably not worth the hassle to address.

See [r4510].

PS. I will release an update for OWLMaker.

[Ognyan Chernokozhev]

I implemented a very simple safeguard to prevent the dialog from showing multiple times - if it is already shown, and we receive another modified notification, there is no sense of displaying it again. See [r4516].

Probably the modified dialog should also not be displayed if the SaveAs dialog is shown...

P.S. The refactored class looks cool - I will publish it's source in the article as well, so that it can be comapred with the original classic C++ version.

[Vidar Hasfjord]

Hi Jogy, well spotted! I didn't notice that problem. As you say, subsequent notifications for the same file can be ignored until the first is handled.

There is a minor technical issue with your solution, though. It isn't exception-safe. If an exception is thrown by std::ostringstream, GetFileName or MessageBox, the flag IsDialogDisplayed is never reset. You can use a local RAII class to make the code more robust.

:::C++
const struct TGuard
{
  bool& Flag;
  TGuard(bool& f) : Flag{f} {Flag = true;}
  ~TGuard() {Flag = false;}
}
guard{IsDialogDisplayed};

[Vidar Hasfjord]

Hi Jogy, I've now added type-safe dispatch for the file change notification. I have also refactored TOWLMakerEdit::EvFileChangeNotification for exception-safety, as mentioned. See [r4517].

[Vidar Hasfjord]

Hi Jogy, I've reopened this issue, as it seems your fix has been broken, most likely by a regression in one of my refactorings or changes to TFileWatcher:

https://sourceforge.net/p/owlnext/code/5487/log/?path=/tools/OWLMaker/trunk/FileWatcher.cpp

Any clue about what has gone wrong?

[Ognyan Chernokozhev]

Hmm, I just tried it and seems to be working - an opened file updated in the OWLMaker when I edited it and saved it in Notepad++ at the same time.

Does the problem happen with every file for you?
Is the file located on a network or in a local folder?

[Ognyan Chernokozhev]

Actually, I also reproduced it - with the 64-bit version.
While the 32-bit version works fine.

More weirdness - it seems that it mostly works when running under a debugger, but does not work when not under a debugger, even when starting the same executable.

Using Spy++, looks like the message is not sent at all - maybe some problem with the registration of the message.

[Ognyan Chernokozhev]

Some more observations:
This file path always receives notification, even in the downloaded OWLMaker

D:\Work\OWLNext\Temp\test

while this file path never receives notifications in the downloaded OWLMaker:

D:\Work\OWLNext\Subversion\trunk\include\owl\defs.h

but sometimes it works in the version that I am building.

Also, reverting the code that used filesystem::canonical back to just using file.c_str seem to help with the notification working.

I am still confused as why this is the case.

[Vidar Hasfjord]

Hi Jogy, great investigation so far!

this file path always receives notification [...] while this file path never receives notifications [...]

I can confirm the same. With 64-bit OWLMaker build 5368, the paths "C:\OWLNext\test1.txt" and "C:\OWLNext\Setup\test2.txt" work fine, but "C:\OWLNext\trunk\test3.txt" does not.

So the length of the path is not the issue, nor hierarchy, as the latter two have the same length and hierarchical structure. The only obvious difference, is that the latter directory is a Subversion working copy. I might also have disabled backup and real-time virus checking for (parts of) that directory.

For sanity, I also tested "C:\OWLNext\Patches\test4.txt" and "C:\OWLNext\tronk\test5.txt", and they both work fine.

Of course, these symptoms could be completely arbitrary. I think we need to add thorough return value checking and diagnostics on all calls. Then we can investigate the issue with the diagnostics window and not be misled by running under the debugger. First goal would be to indentify the call that sometimes fails.

Edit: I retested "C:\OWLNext\trunk\test3.txt" and it now works, as do "C:\OWLNext\trunk\test6.txt", "C:\OWLNext\trunk\include\ocf\test8.txt" and "C:\OWLNext\trunk\include\owl\test9.txt". So, it does seems like something inconsistent and sporadic is going on. Note that all my testing here is with the 64-bit release edition of OWLMaker build 5368, running it normally (no debugging, no administrator mode).

[Ognyan Chernokozhev]

Still baffled by this problem.
For example, I copied the D:\Work\OWLNext\Subversion\trunk\include\owl\defs.h file to D:\Work\OWLNext\Subversion\trunk\include\owl\defs.g - and the new file receives notification changes, while the original one does not.

The only difference I can think of, is that the original file is source controlled, and the SVN might in some way interfere with the change notifications.

I think we need to add thorough return value checking and diagnostics on all calls.

The problem is that the SHChangeNotifyRegister call always returns successfully, otherwise we will throw an error.
But then, using Spy++, I can see that the registered notification is never sent to the window (or any other window in the process).

Lastly. comparing my original code with the new code, one subtle difference is that in my original code, the TFileWatcher class was keeping a string member of the passed file name, while in the new code a temporary string is passed to ILCreateFromPath to get the PIDLIST_ABSOLUTE.

I changed the code to add the canonicalized filename as a member of the class, and after that, at least after few tests, the notifications are working even for the defs.h file.

I am going to check in the change, so you can do some tests as well.

[Ognyan Chernokozhev]

Possible fix in [r5492]

[Vidar Hasfjord]

Hi Jogy,

As you point out, passing a temporary string to ILCreateFromPath is a bad code smell. Even if it is not the main issue, good coding practice should ensure a passed pointer stays alive while the client to whom it is passed is using it. I have a tendency to treat strings differently, assuming the pointer only needs to be valid for the duration of the call, and that is dangerous. That said, there is nothing in the API documentation that indicates that the pointer is retained by the callee, so I doubt it is an issue.

In any case, storing the string doesn't solve the problem. I have rebuilt and tested OWLMaker with your change [r5492], built with the latest OWLNext version on the trunk [r5496], and I am still able to reproduce the issue.

The only difference I can think of, is that the original file is source controlled, and the SVN might in some way interfere with the change notifications.

I eliminated that hypothesis in my earlier testing, as I was able to reproduce the issue with files not under version control. My latest testing also fails with files outside version control:

• "C:\OWLNext\Setup\test1.txt": FAIL/SUCCESS
• "C:\OWLNext\Setup\test2.txt": FAIL/SUCCESS
• "C:\OWLNext\trunk\include\test7.txt": FAIL/SUCCESS
• "C:\OWLNext\trunk\include\test8.txt": SUCCESS
  
  

The issue seems very sporadic. The files above that failed, suddenly passed the test successfully after adding another test file in the same folder. The file "test8" was newly added.

Note that the folder "C:\OWLNext\Setup" is not within a Subversion working copy, while the folder "C:\OWLNext\trunk\include" is. So the issue is present, whether or not the folder is under version control.

Edit: It seems even browsing a folder can suddenly cause all the files in that folder to pass the test. In particular, all my test files in folder "C:\OWLNext\Setup" suddenly passed the test after I just navigated to the folder in File Explorer. Then, if I navigated out of the folder in File Explorer, the file failed the test again:

1. Open "C:\OWLNext\Setup\test2.txt" in OWLMaker.
2. Open "C:\OWLNext\Setup\test2.txt" in Notepad.
3. Navigate to "C:\OWLNext\Setup" in File Explorer.
4. Change the file in Notepad: OWLMaker updates the file (SUCCESS).
5. Navigate out of "C:\OWLNext\Setup" to parent folder in File Explorer.
6. Change the file in Notepad: OWLMaker does not update the file (FAIL).
7. Navigate back into "C:\OWLNext\Setup" in File Explorer.
8. Change the file in Notepad: OWLMaker updates the file (SUCCESS).

I was able to repeat this by navigating in and out of the folder in File Explorer, and the test always failed unless File Explorer was viewing the folder. However, after closing and reopening File Explorer, the test succeeded even if viewing the parent folder. But closing File Explorer caused the test to fail again. It is all very sporadic.

[Ognyan Chernokozhev]

In my latest round of experiments I found the following:
When I build OWLMaker from source, then start it, and open defs.h.
It works - receiving change notifications. Then at some point it stops receiving those notifications. Sometimes it happens after several notifications, sometimes I have to close and reopen the file, and sometimes I have to close and restart OWLMaker.

When I get it into the state to not receive notifications, it stays that way.
The way to "fix" it is to rebuild OWLMaker or restart the computer.
Then it starts working for a while, until it stops working again.

I wonder, if something is not properly cleaned up after receiving notifications.
But I looked carefully at the code, I tried changing it, reverting some parts to be more like my original implementation, but none of those helped.

So I am still confused at this behavior.

[Vidar Hasfjord]

So I am still confused at this behavior.

Yes. It is confounding. We have both confirmed that external, seemingly unrelated actions — such as navigating in the File Explorer, or closing and reopening the file outside OWLMaker — can affect whether or not a change notification is sent.

That said, you have made an important observation: The issue is that the notification message is not sent at all.

So, this means that the issue cannot be in the event handler. The issue must be in the setup (or a bug in the system service itself).

So my main question now is: Do we set up the request for change events correctly?

Do you have a reference to some example code using SHChangeNotifyRegister reliably?

Since the system seems to arbitrarily decide to send or not to send the notification, and this seems to correlate with external actions in the Windows Shell (such as opening and closing File Explorer), is there perhaps some Shell service that we need to register with and keep awake?

Could it be that SCHNE_UPDATEITEM is combined into a different event that also needs to be monitored? Update: I tried SCHNE_ALLEVENTS, and it didn't help.

Stab in the dark: Try to turn off SHCNRF_NewDelivery. See Remarks in the documentation for SHChangeNotifyRegister. Maybe they introduced bugs in the new delivery method. Update: I tried this, and it didn't help. Stabbing in the dark rarely helps.

Of course, it could be that SHChangeNotifyRegister is unreliable. E.g. see "SHChangeNotifyRegister notifications on Windows 10 are inconsistent" at StackOverlow.

Edit: I also found this: "SHChangeNotifyRegister is being a dick" at Xentax Game Research Forums, describing similar sporadic symptoms involving missing SHCNE_UPDATEITEM events. Also: "SHChangeNotifyRegister and missing messages" at GameDev.

Some interesting discussion here: "Understanding ReadDirectoryChangesW" at Technical Blog for Jim Beveridge:

First, a brief overview of monitoring directories and files. In the beginning there was SHChangeNotifyRegister. It was implemented using Windows messages and so required a window handle. It was driven by notifications from the shell (Explorer), so your application was only notified about things that the shell cared about - which almost never aligned with what you cared about. It was useful for monitoring things that the user did in Explorer, but not much else. [...] SHChangeNotifyRegister also had a performance problem, since it was based on Windows messages. If there were too many changes, your application would start receiving roll-up messages that just said "something changed" and you had to figure out for yourself what had really happened. Fine for some applications, rather painful for others.

[Ognyan Chernokozhev]

I tried reverting to the original FileWatcher code, from [r4509] - and it also was experiencing the same issue.

Next, I downloaded Microsoft's ChangeNotifyWatcher sample:
https://github.com/microsoft/Windows-classic-samples/tree/master/Samples/Win7Samples/winui/shell/appplatform/ChangeNotifyWatcher
build it and tried it - and it was receiving notifications, even is at the same moment OWLMaker was not.

Looking at the code, one obvious difference is that the sample also uses the SHCNRF_ShellLevel flag.
So, I set the flag in OWLMaker as well, and now it seems to be working reliably, but I don't fully trust the positive results.

[Ognyan Chernokozhev]

Some related observations:

• In .NET desktop application I am working, we are using the FileSystemWatcher class, which under the hood uses ReadDirectoryChangesW API, and we have not noticed any problems with it.
• Notepad++ also uses ReadDirectoryChangesW - the code from Jim Beveridge from the link you have posted.
  I am using Notepad++ to view our application log files, and I have noticed that occasionally it does not detect that the log file has changed until I open the log files folder in Explorer and refresh it

[Vidar Hasfjord]

SHCNRF_ShellLevel

It makes no diffence in my test. Unless File Explorer is open, I get no notification message.

Do you get a notification message with only OWLMaker and Notepad open?