KDirWatch only works correctly with canonical paths, i.e. symbolic links resolved.
Details
Details
- Reviewers
dfaure elvisangelaccio - Group Reviewers
Dolphin - Commits
- R318:17e127edcd25: Add only canonical paths to dirWatcher
- Create dirs:
$ mkdir test1
$ mkdir test1/subdir
$ ln -s test1 test2
- Start dolphin, navigate to test2/subdir, then in terminal:
$ echo test > test2/subdir/test
Without the patch, test2/subdir/test won't be shown automatically, only after reload (F5).
With the patch applied, test2/subdir/test will be shown automatically.
- Restart dolphin, navigate to test2/subdir, then in terminal:
$ echo test >> test2/subdir/test
Without the patch, the increased size of test2/subdir/test won't be shown automatically,
only after reload (F5). With the patch applied, it will be shown automatically.
- Restart dolphin, navigate to test2/subdir, then in terminal:
$ rm test2/subdir/test
Without the patch, test2/subdir/test stays visible, will only disappear after reload (F5).
With the patch applied, it will disappear automatically.
Diff Detail
Diff Detail
- Repository
- R318 Dolphin
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
This should go into KDirWatch itself then, to avoid the risk of other callers falling into that trap.
Comment Actions
See also commit 6be886bafae463ba in kcoreaddons (which was wrong, it would seem -- there are better ways to prevent infinite loops than to say "let's not follow symlinks at all").
Doing this in kcoreaddons is also an opportunity to add a unittest for this :)
What I wonder, however, is whether we also want to be notified if someone does rm test2, because then test2/subdir/test no longer exists (under that name)...
Comment Actions
I'm not sure, if that's a good idea.
- It's legal to add non-existing directories to KDirWatch. Calling QFileInfo::canonicalFilePath() on such a dir would return an empty string for it.
- Ok, you could check if it exists before canonicalizing it, but it still could be a path containing symlinks, so you need to know what you are doing anyway, when adding something to KDirWatch.
- In other places (e.g. KIO KCoreDirLister), paths added to KDirWatch are already canonicalized. Adding the QFileInfo::canonicalFilePath() call to KDirWatch would result in doing this operation twice.
But initializing a QFileInfo and especially calling canonicalFilePath() are quite expensive operations. On locally mounted network paths these operations can take several seconds if the network is slow, for example when using a laptop with a VPN connection. Clicking through a directory tree can be a tedious task if you have to wait ages until the next dir level is shown.
Because of this, at our LiMux project in Munich we don't use QFileInfo::canonicalFilePath() on locally mounted netshares paths (cifs, no symlinks), but an own fast method which resolves the symlinks in the users' netshares directories to the mounted netshares (below /mnt) and caches them. This symlink construction is the reason why this bug showed up, by the way.
Comment Actions
Thanks for your well thought-out reply.
The usual solution to 1. and 2. is rather "call canonicalFilePath, and if the return value is empty, use the original path". We do this in many places. But indeed this means, if a non-existing dir gets created later, it won't be canonicalized. But that's not worse than the current situation, and not an issue for the problem at hand (different use case, could be fixed in KDirWatch if needed one day).
You are very correct about 3, resolving in two places is slow. This is the usual problem with shifting responsibilities around...
I did some archeology, and KDirLister needs to keep a mapping because kdirwatch needs canonical paths while the model (upon receiving notifications of changes) only understands the paths as initially given (with symlinks). On hindsight, maybe I should have moved that one level down. It would be convenient for all KDirWatch users if KDirWatch itself would do this mapping, making things transparent for users of KDirWatch. You ask to watch X, you get notifications about X.
So I consider the "cleanest solution, in an ideal world" to implement such a mapping in KDirWatch, and remove it from KCoreDirLister (to avoid doing it twice).
However I can see how this is a much more involved change than what is proposed here, which continues in the same direction as what was already done, resolving in the callers of KDirWatch. So I'll accept it.
It would be nice if someone did the right thing in KDirWatch though, so we can avoid N applications hitting this problem separately and all of them having to implement workarounds :-)
Comment Actions
(forgot to paste more details about the archeology: kdelibs commit d3a9f1037d1410, bugs 213799 and 219547)
Comment Actions
Yes, I do, but this will be my first push and I don't want to break something. So all I have to do is
arc land
in my feature branch to merge these two commits and push (push remote is git://git.kde.org/dolphin.git). Correct?