aacid accepted this revision.
aacid added a comment.
This revision is now accepted and ready to land.
About the code itself, it looks good, and i've tested dolphin with copy and
removes and it seems ok, but on the other hand i understand this is mostly an
optimization, so i'm not sure if i
aacid added a comment.
In https://phabricator.kde.org/D5856#109721, @dfaure wrote:
> So I think we need to remove the qDebug in KDirWatch so that removeDir is
silent if the dir wasn't watched [this is faster than checking before removing,
from the outside].
+1
REPOSITORY
R241
This revision was automatically updated to reflect the committed changes.
Closed by commit R244:792680d43877: Do not watch QRC's paths (authored by
elvisangelaccio).
REPOSITORY
R244 KCoreAddons
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D5877?vs=14736=14738
REVISION DETAIL
dfaure accepted this revision.
This revision is now accepted and ready to land.
REPOSITORY
R244 KCoreAddons
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D5877
To: elvisangelaccio, dfaure
Cc: #frameworks
Albert Astals Cid ha scritto:
> El dimecres, 17 de maig de 2017, a les 21:55:57 CEST, Ben Cooksley va
> escriure:
>> On Wed, May 17, 2017 at 10:22 AM, Albert Astals Cid wrote:
>>> El dimarts, 16 de maig de 2017, a les 22:42:52 CEST, Ben Cooksley va
> escriure:
Hi all,
elvisangelaccio marked an inline comment as done.
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.kde.org/D5877
To: elvisangelaccio, dfaure
Cc: #frameworks
elvisangelaccio updated this revision to Diff 14736.
elvisangelaccio added a comment.
- Fixup comment
REPOSITORY
R244 KCoreAddons
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D5877?vs=14735=14736
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D5877
AFFECTED
El dimecres, 17 de maig de 2017, a les 21:55:57 CEST, Ben Cooksley va
escriure:
> On Wed, May 17, 2017 at 10:22 AM, Albert Astals Cid wrote:
> > El dimarts, 16 de maig de 2017, a les 22:42:52 CEST, Ben Cooksley va
escriure:
> >> Hi all,
> >>
> >> On the Windows CI i'm currently
dfaure added a comment.
Looks good otherwise.
INLINE COMMENTS
> kdirwatch_unittest.cpp:760
> +watch.addDir(QDir::homePath());
> +// NOTE: addDir(".") it's not enough, we really need this path to
> trigger bug #374075.
> +watch.addDir(QStringLiteral(":/kio5/newfile-templates"));
kossebau added a comment.
If noone objects or has further comments, will commit this Wednesday, 24th.
REPOSITORY
R240 Extra CMake Modules
REVISION DETAIL
https://phabricator.kde.org/D5866
To: kossebau, #frameworks, #build_system
Cc: elvisangelaccio
elvisangelaccio marked 2 inline comments as done.
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.kde.org/D5877
To: elvisangelaccio, dfaure
Cc: #frameworks
elvisangelaccio retitled this revision from "Use absolute path instead of "."
as path to watch" to "Do not watch QRC's paths".
elvisangelaccio edited the summary of this revision.
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.kde.org/D5877
To: elvisangelaccio, dfaure
Cc:
elvisangelaccio updated this revision to Diff 14735.
elvisangelaccio added a comment.
- Explicitly ignore QRC paths.
REPOSITORY
R244 KCoreAddons
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D5877?vs=14575=14735
BRANCH
master
REVISION DETAIL
dfaure added a comment.
Yes. KDirWatch doesn't support watching such paths.
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.kde.org/D5877
To: elvisangelaccio, dfaure
Cc: #frameworks
elvisangelaccio added inline comments.
INLINE COMMENTS
> dfaure wrote in kdirwatch_unittest.cpp:760
> Where does ":/foo" become "." then?
> Why not catch ":/foo" rather than check for "." after some unknown conversion
> somewhere? Feels fragile.
I don't know exactly. So do you think we should
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> kdirwatch_unittest.cpp:115
> void stopAndRestart();
> +void bug374075();
>
I prefer more descriptive method names (the bug number as a comment
16 matches
Mail list logo