Do not watch QRC's paths
ClosedPublic

Authored by elvisangelaccio on May 15 2017, 10:09 PM.

Details

Summary

Watching a QRC path is not supported and results in "." being
watched which can lead to problems.

For example in bug #374075 KIO adds ":/kio5/newfile-templates"
as path to watch (this is probably another bug in itself).
If we are already watching "/home/user", this breaks the emission
of the dirty() signal for every new children of "/home/user" (somehow,
the relative path is used for them, e.g. "./foo.txt" instead of
"/home/user/foo.txt"). In particular, in inotifyEventReceived()
e->m_client is empty and so e->path is not added to
e->m_pendingFileChanges. This only happens if "/home/user" is also
the cwd of the process using KDirWatch.

Ignoring QRC paths fixes this issue.

BUG: 374075
FIXED-IN: 5.35

Test Plan

From dolphin, Create New -> Text File in a folder which is also the current working
directory of the dolphin process.

Diff Detail

Repository
R244 KCoreAddons
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Restricted Application added a project: Frameworks. · View Herald TranscriptMay 15 2017, 10:09 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
elvisangelaccio edited the summary of this revision. (Show Details)May 15 2017, 10:13 PM
elvisangelaccio edited the summary of this revision. (Show Details)May 15 2017, 10:15 PM
elvisangelaccio edited the summary of this revision. (Show Details)May 15 2017, 10:17 PM
dfaure requested changes to this revision.May 21 2017, 11:44 AM
dfaure added inline comments.
autotests/kdirwatch_unittest.cpp
115 ↗(On Diff #14575)

I prefer more descriptive method names (the bug number as a comment next to the implementation).

760 ↗(On Diff #14575)

Where does ":/foo" become "." then?
Why not catch ":/foo" rather than check for "." after some unknown conversion somewhere? Feels fragile.

This revision now requires changes to proceed.May 21 2017, 11:44 AM
autotests/kdirwatch_unittest.cpp
760 ↗(On Diff #14575)

I don't know exactly. So do you think we should just ignore qrc's paths? (i.e. check for leading ":/")

Yes. KDirWatch doesn't support watching such paths.

elvisangelaccio edited edge metadata.
  • Explicitly ignore QRC paths.
elvisangelaccio retitled this revision from Use absolute path instead of "." as path to watch to Do not watch QRC's paths.May 21 2017, 3:30 PM
elvisangelaccio edited the summary of this revision. (Show Details)
elvisangelaccio marked 2 inline comments as done.

Looks good otherwise.

autotests/kdirwatch_unittest.cpp
760

Isn't this comment outdated now that the test and the fix are all about qrc paths rather than "." ?

  • Fixup comment
elvisangelaccio marked an inline comment as done.May 21 2017, 5:23 PM
dfaure accepted this revision.May 21 2017, 6:39 PM
This revision is now accepted and ready to land.May 21 2017, 6:39 PM
This revision was automatically updated to reflect the committed changes.