Files saved with QSaveFile don't get dirty. They are deleted and replaced.
Thus, inotify and KDirWatch don't emit a "dirty" signal (which is the correct behaviour).
Listening for the "created" signal of KDirWatch allows us to get notified on replaced files.
BUG: 384185
Opened a markdown document in Okular.
Edited and saved the document with Kate.
The file is reloaded in Okular as expected.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
Nice, solves the issues for me (and confirms my suspicion that this is not a regression or porting issue, as this is broken for me even on an ancient KDE4 distro), but I'll let Albert give the final "ship it".
Do you think we should add a @note to that effect to the KDirWatch API Docs? Seems like a common gotcha to me.
I don't know who told you this is the correct behaviour of kdirwatch, but i kind of disagree.
Since we're not watching the file but *also* the folder.
Albert: Do you remember why f93ccd7 was necessary? There is no bug linked and no autotest. Using just the commit message as a testcase, with D7671 applied and watcher->addDir(fi.absolutePath()) removed I can still trigger a reload with rm file.txt; touch file.txt. At first sight, watching a directory when we are interested in the file feels like a workaround for bugs or missing features in KDirWatch at the time (and not needed anymore today), perhaps something like inode vs. path watchers.
We could think about whether f93ccd7 can be reverted in total (not sure, did not grasp the other code parts yet) in favour of D7671. The former seems complicated and does not support Kate, the latter is easy to understand and supports both the original case and Kate.
If there is interest, I can provide logs from the ancient system gathered with inotifywait -m . which show what's going on.
(Julian: Sorry to intrude, feel free to pick up from here.)
From the KDirWatch class reference:
void KDirWatch::dirty ( const QString & path ) signal
Emitted when a watched object is changed.For a directory this signal is emitted when files therein are created or deleted. [...]
When you watch a directory, changes in the size or attributes of contained files may or may not trigger this signal to be emitted depending on which backend is used by KDirWatch.
So, let's assume we have a file path/file loaded in Okular and we watch the directory path for changes. We remove file and create a new file named file. The directory path will get dirty, as a new file is created.
This means, a signal dirty("path") is emitted. However, Okular does not reload the document when the signal dirty("path") is received, but waits for a signal dirty("path/file").
The behaviour of KDirWatch might not be the most convenient, but from my perspective it is consistent with the documentation.
Maybe the docs could be a little clearer at this point, like Henrik and Oliver proposed.
Always happy when someone contributes good ideas to a discussion.
It was added to make this work, and it did work at some point, i don't add code for nothing ;)
No, the code doesn't wait *only* for a dirty for path/file, read Part::slotFileDirty better
The problem here is that dirty for the path is not being emitted, https://mail.kde.org/pipermail/kde-frameworks-devel/2017-August/048813.html if the dirty for the dir was getting emitted on delete and on addition i'm positive this would work.
The behaviour of KDirWatch might not be the most convenient, but from my perspective it is consistent with the documentation.
Maybe the docs could be a little clearer at this point, like Henrik and Oliver proposed.
Always happy when someone contributes good ideas to a discussion.
Ah, now I get what you meant to do there.
You're right, the dirty signal for the path should be emitted on creation and deletion. And I agree, that your code should work then.
I still don't see, why we would want to track deleted and added files by ourselves, instead of just connecting to the "created" signal, but I can understand it if you wanted to keep your code.
Okay, I modified KDirWatch so we actually get a dirty signal for the directory.
Now a new problem arised.
QSaveFile does not delete and recreate the file as we thought. It just moves the swap file to the old file's location.
On move, KDirWatch still sends a "created" signal for the file and a dirty signal for the directory, but the file exists all the time. This is why Okular fails to notice that the file changed as long as we don't listen for the "created" signal.
kf5.kcoreaddons.kdirwatch: "KDirWatch-1" emitting deleted "/home/wolff/repos/performer/README.md" org.kde.okular.ui: deleted signal org.kde.okular.ui: slotFileDirty: "/home/wolff/repos/performer" org.kde.okular.ui: slotFileDirty: "/home/wolff/repos/performer/README.md.Sm7640" org.kde.okular.ui: slotFileDirty: "/home/wolff/repos/performer" kf5.kcoreaddons.kdirwatch: "KDirWatch-1" emitting created "/home/wolff/repos/performer/README.md" org.kde.okular.ui: created signal
Have you read my email? There clearly says what happens and what the documentation says it should happen (at least to my understanding of reading it).
Because the documentation says my code should work, and thus it should work. Or the documentation should be fixed.
Of course, and your code still works today: it fixes the rm/touch case (with D7671 not applied yet). I guess for Kate it broke in 2012 (your patch is from 2008) with the introduction of KSaveFile in R40:a188aa8. My question was more about why you went for the directory approach instead of just listening for KDirWatch::created.
Concerning f93ccd7, I find the part re-adding the watch for removed files is also not needed with D7671. Will do some more testing over the weekend and maybe prepare a patch. (You can tell I'm not a fan of encoding state in bools with updates spread over the codebase. As far as I can see, in conjunction with the directory watch this led to several bugs, e.g. 384185, 234139, 321880 and maybe more.)
Here's my suggestion going forward:
TL;DR: Do both: Fix KDirWatch and apply D7671.
Sure, I read your mail. But I still don't think that KDirWatch behaves wrong.
On saving a file with QSaveFile or on a plain mv somefile watchedfile the watched file is never removed. As the docs say, the dirty signal of a directory is emitted when a file in this directory is removed or created. This is not the case here.
KDirWatch yet does send a "created" signal on inotify's MOVE_TO flag and a "removed" signal on inotify's MOVE_FROM flag. This does not mean, the file actually has been removed or added at any time.
So, this behaviour is a little strange and confusing, but from my perspective it's still coherent with the documentation.
Even if KDirWatch would work as you expect it to, there are cases where Okular still does not react to those signals:
Consider the command rm watchedfile && touch watchedfile.
KDirWatch will send the signals: file removed, directory dirty, file added, directory dirty.
Okular receives the first dirty signal asynchronously. Okular checks if the file exists via QFile::exists. It is likely that the file has already been added (touch) by that time, so Okular will miss to reload the file.
Actually, this case seems to work with your code, but I hope you understand what I want to say. It just doesn't cover every potential case. Same problems apply for mv.
I agree. I think KDirWatch does this internally. Something that could be mentioned in the docs too.
I'd be perfectly happy with this. It's probably a good idea to recheck the docs of KDirWatch and mention the move_to and move_from cases there.
Thanks for your constructive participation!
I don't know, that was long time ago.
Concerning f93ccd7, I find the part re-adding the watch for removed files is also not needed with D7671. Will do some more testing over the weekend and maybe prepare a patch. (You can tell I'm not a fan of encoding state in bools with updates spread over the codebase. As far as I can see, in conjunction with the directory watch this led to several bugs, e.g. 384185, 234139, 321880 and maybe more.)
Because there's a much better way of encoding state that using variables that encode state, right?
Here's my suggestion going forward:
- Independently from Okular's case, evaluate accuracy of KDirWatch autotests and docs regarding "move/rename" (currently not mentioned at all, thus to be considered undefined behaviour…) as well as directory operations in general, fix potential code bugs and doc confusions
- Agree to accept D7671 in any case (reasons: code is simpler and less error-prone, fixes the issue even for users on LTS distros with old KF5 libs)
- Try to revert f93ccd7 including later fixups
TL;DR: Do both: Fix KDirWatch and apply D7671.
I'm hesitant of applying something that just workarounds a library not working.
Why?
On saving a file with QSaveFile or on a plain mv somefile watchedfile the watched file is never removed.
According to KDirwatch, it is
As the docs say, the dirty signal of a directory is emitted when a file in this directory is removed or created. This is not the case here.
According to KDirwatch, it is
KDirWatch yet does send a "created" signal on inotify's MOVE_TO flag and a "removed" signal on inotify's MOVE_FROM flag. This does not mean, the file actually has been removed or added at any time.
The file may "in reality never be creater or removed", but KDirWatch is sending the removed and created signals, so if the documentation says "dirty is triggered when the file is removed and created", well it should send the signal, otherwise it's pretty confusing, and i don't know why we're discussing this here instead of on the mailing list where i actually asked for clarification.
So, this behaviour is a little strange and confusing, but from my perspective it's still coherent with the documentation.
I disagree.
Even if KDirWatch would work as you expect it to, there are cases where Okular still does not react to those signals:
Consider the command rm watchedfile && touch watchedfile.
KDirWatch will send the signals: file removed, directory dirty, file added, directory dirty.
Okular receives the first dirty signal asynchronously. Okular checks if the file exists via QFile::exists. It is likely that the file has already been added (touch) by that time, so Okular will miss to reload the file.
Actually, this case seems to work with your code, but I hope you understand what I want to say. It just doesn't cover every potential case. Same problems apply for mv.
This is actually the only good explanation of why the code is not robust enough, you're right there's races involved in the logic. So yeah let's commit this fix then.
After further analysis, I can draw these conclusions (contradicting some assumptions from above, confirming others):
This can be shown as follows (separately for each conclusion):
Therefore, the only thing left to clarify in the docs for KDirWatch is the behaviour on move. E.g., currently for file watches a move_to results in "created" and "deleted", but not "dirty" (could be made more explicit). As the rest of the discussion is not Okular specific anymore, let's continue on kde-frameworks-devel.
"Correctly" is an overstatement in my opinion, it should emit as many dirty as things that make it dirty, i.e. i disagree KDirWatch is not a reason for this bug (if we ignore possible races)
- The directory code path in Okular only works for deleted files, but never works for moved files (that's the real reason for the bug).
- The file code path in Okular would work, if KDirWatch emitted dirty for a file on move_to (but KDirWatch does not and should not).
Again i disagree here, it's emitting a removed/added signal, so why wouldn't it emit dirty?
For completeness, for a file watch on "x" (not sure whether you may have confused file and dir watches as well as file and dir Okular code pathes in your reply), we get:
To me this seems logical and consistent, only the documentation regarding "move" is lacking. Of course we could argue that touch (no x yet), mv and rm should also send an additional "dirty" signal. Above I said "it should not", because it breaks backwards compatibility with existing KDirWatch users (they get signals they may have wanted to avoid, or they have a separate code path for "create") and there are already more specialized signals. In addition, only adding to the documentation is less risky than invasive changes to KDirWatch's internals (it has to work for all backends and on all platforms, after all).
i'm not going to keep discussing this here, makes no sense, if you want to discuss on KDirwarch use the kde-frameworks-devel thread.