D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-06 Thread Jaime Torres Amate
This revision was automatically updated to reflect the committed changes. jtamate marked an inline comment as done. Closed by commit R241:820f622e86bb: kioexecd: watch for creations or modifications of the temporary files (authored by jtamate). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-06 Thread Jaime Torres Amate
jtamate marked an inline comment as done. jtamate added inline comments. INLINE COMMENTS > elvisangelaccio wrote in kioexecd.cpp:129 > Maybe `constBegin()`/`constEnd()` here? I always thought const iterators were meant to be used when the whole container is considered ReadOnly, not only its

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-05 Thread Elvis Angelaccio
elvisangelaccio accepted this revision. elvisangelaccio added inline comments. This revision is now accepted and ready to land. INLINE COMMENTS > kioexecd.cpp:129 > +// check if the deleted (and not recreated) files where deleted 30s ago > or more > +for (auto it = m_deleted.begin(); it

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-05 Thread Jaime Torres Amate
jtamate updated this revision to Diff 41066. jtamate added a comment. Use QDateTime::currentDateTimeUtc() instead of QDateTime::currentDateTime(). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15180?vs=41035=41066 REVISION DETAIL

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-05 Thread Stefan Brüns
bruns added inline comments. INLINE COMMENTS > dfaure wrote in kioexecd.cpp:133 > Move the call to currentDateTime() outside of the loop, so it happens only > once. > It's much more costly than one might think (because of timezone conversion, > which uses tzset() etc.) makes me wonder,

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-05 Thread Jaime Torres Amate
jtamate added a comment. May I commit or should I wait for @elvisangelaccio to accept the changes? This time I have read the arc message, that is usually something about non tracked files: Revision 'D15180 : kioexecd: watch for creations or

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-05 Thread Stefan Brüns
bruns added inline comments. INLINE COMMENTS > anthonyfieroni wrote in kioexecd.cpp:134 > Also for loop should looks like: > > for (it = begin(); it != end();) { > if () { > it = erase(it); > } else { > ++it; > } > } Even better use the

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-05 Thread David Faure
dfaure accepted this revision. dfaure added a comment. Thanks! REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D15180 To: jtamate, #frameworks, broulik, ngraham, dfaure, elvisangelaccio Cc: anthonyfieroni, elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-05 Thread Jaime Torres Amate
jtamate updated this revision to Diff 41035. jtamate added a comment. Tested, dirty is not signaled when created (at least I didn't saw the dialog for uploading the modified file). Removed the header. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-05 Thread David Faure
dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. No worries, that's what code review is for :-) Now this is starting to look good ;) INLINE COMMENTS > kioexecd.cpp:88 > + > +slotDirty(path); > +} Doesn't kdirwatch

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-05 Thread Jaime Torres Amate
jtamate updated this revision to Diff 41030. jtamate added a comment. Why the mutex? I interpreted that QTimer work as an interrumpt (wrong) instead of generating events in the event queue (right). So I've gone through all the possible mistakes one can do here: - Design mistakes -

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-05 Thread David Faure
dfaure requested changes to this revision. dfaure added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > kioexecd.cpp:36 > > +const int predefinedTimeout = 3; // 30s > + static const int... > kioexecd.cpp:86 > +{ > +m_deleted_mutex.lock(); > +

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-05 Thread Jaime Torres Amate
jtamate updated this revision to Diff 41026. jtamate edited the summary of this revision. jtamate edited the test plan for this revision. jtamate added a comment. Do not delete recursively. Do not delete the file after 30s (we know it is already deleted). REPOSITORY R241 KIO CHANGES

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-05 Thread Anthony Fieroni
anthonyfieroni added inline comments. INLINE COMMENTS > jtamate wrote in kioexecd.cpp:65 > There is a slightly problem: QStandardPaths::CacheLocation is application > dependent, and their values doesn't match here: > kioexec: /home/jtorres/.cache/kioexec/ > kioexecd:

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-05 Thread Jaime Torres Amate
jtamate added inline comments. INLINE COMMENTS > elvisangelaccio wrote in kioexecd.cpp:65 > The problem with using `QDir::removeRecursively()` is that the folder we are > going to delete recursively is an input from dbus. What happens if some > malicious software calls `watch("~/dummy.txt")` ?

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-04 Thread Elvis Angelaccio
elvisangelaccio requested changes to this revision. elvisangelaccio added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > kioexecd.cpp:65 > qCDebug(KIOEXEC) << "About to delete" << parentDir << "containing" > << info.fileName(); > -

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-04 Thread Jaime Torres Amate
jtamate updated this revision to Diff 40961. jtamate added a comment. I missed that part, sorry. It is safer that way, the start(time) method was added in Qt 5.8. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15180?vs=40960=40961 REVISION DETAIL

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-04 Thread Anthony Fieroni
anthonyfieroni added a comment. m_timer.start(predefinedTimeout) should be m_timer.start() in 2 places, but let's see @elvisangelaccio and @dfaure comments. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D15180 To: jtamate, #frameworks, broulik, ngraham, dfaure,

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-04 Thread Jaime Torres Amate
jtamate updated this revision to Diff 40960. jtamate marked 3 inline comments as done. jtamate added a comment. Done. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15180?vs=40952=40960 REVISION DETAIL https://phabricator.kde.org/D15180 AFFECTED FILES

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-04 Thread Anthony Fieroni
anthonyfieroni added inline comments. INLINE COMMENTS > kioexecd.cpp:53 > connect(m_watcher, ::deleted, this, ::slotDeleted); > +m_timer.setSingleShot(true); > +connect(_timer, ::timeout, this, > ::slotCheckDeletedFiles); Also add interval here, setInterval, in other place just

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-04 Thread Jaime Torres Amate
jtamate updated this revision to Diff 40952. jtamate marked 4 inline comments as done. jtamate added a comment. Implemented Anthony comments/suggestions. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15180?vs=40908=40952 REVISION DETAIL

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-03 Thread Anthony Fieroni
anthonyfieroni added inline comments. INLINE COMMENTS > kioexecd.cpp:80 > +{ > +if (!m_deleted.contains(path)) { > +return; Contains should be also in the guard. > kioexecd.h:40 > virtual ~KIOExecd(); > +void clearDir(const QString ); > Unused? REPOSITORY R241 KIO

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-03 Thread Anthony Fieroni
anthonyfieroni added inline comments. INLINE COMMENTS > kioexecd.cpp:134 > +QDir(parentDir).removeRecursively(); > +it=m_deleted.erase(it); > +} Also for loop should looks like: for (it = begin(); it != end();) { if () { it =

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-03 Thread Anthony Fieroni
anthonyfieroni added inline comments. INLINE COMMENTS > kioexecd.cpp:122 > +m_deleted_mutex.unlock(); > +QTimer::singleShot(31000, [this]() { // 31s > +m_deleted_mutex.lock(); Better to me, make a class variable single shot timer, then when you add in deleted start it if not,

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-03 Thread Jaime Torres Amate
jtamate updated this revision to Diff 40908. jtamate edited the summary of this revision. jtamate edited the test plan for this revision. jtamate added a comment. As I'm never sure if a timed execution can happen in the middle of other execution, I've added a mutex for m_deleted. REPOSITORY

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-03 Thread Jaime Torres Amate
jtamate added a comment. In D15180#319314 , @anthonyfieroni wrote: > I'm unhappy with that stop watching is on exit == 0, so when it's not, somehow, containers will continue to grow, it'll result in higher memory usage and slower performance.

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-03 Thread Anthony Fieroni
anthonyfieroni added a comment. I'm unhappy with that stop watching is on exit == 0, so when it's not, somehow, containers will continue to grow, it'll result in higher memory usage and slower performance. So stop watching should not depend on process return code, also same command should

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-03 Thread Jaime Torres Amate
jtamate updated this revision to Diff 40888. jtamate added a reviewer: elvisangelaccio. jtamate added a comment. Updated with the code that handles m_openedBy right. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15180?vs=40779=40888 REVISION DETAIL

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-02 Thread Jaime Torres Amate
jtamate edited the summary of this revision. jtamate edited the test plan for this revision. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D15180 To: jtamate, #frameworks, broulik, ngraham, dfaure Cc: elvisangelaccio, kde-frameworks-devel, michaelh, ngraham, bruns

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-09-02 Thread Elvis Angelaccio
elvisangelaccio added a comment. I don't understand what's the counter used for. Can you explain why we need it and can you update the Test plan accordingly? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D15180 To: jtamate, #frameworks, broulik, ngraham, dfaure Cc:

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-08-31 Thread Jaime Torres Amate
jtamate added inline comments. INLINE COMMENTS > broulik wrote in kioexecd.cpp:58 > If you can be sure kioexec creates a folder per temp file (it might does), > then this is probably fine. > Also check kioexec what it does when the process closes, if it also > recursively removes the temp dir

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-08-31 Thread Jaime Torres Amate
jtamate updated this revision to Diff 40779. jtamate marked 3 inline comments as done. jtamate edited the summary of this revision. jtamate edited the test plan for this revision. jtamate added a comment. I'm not sure if I have to keep compatibility in the dbus calls, therefore the old one is

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-08-31 Thread Kai Uwe Broulik
broulik added inline comments. INLINE COMMENTS > jtamate wrote in kioexecd.cpp:58 > Why not? > It should be just removing the temporary file copy and it's backups. If you can be sure kioexec creates a folder per temp file (it might does), then this is probably fine. Also check kioexec what it

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-08-31 Thread Jaime Torres Amate
jtamate marked 2 inline comments as done. jtamate added inline comments. INLINE COMMENTS > broulik wrote in kioexecd.cpp:58 > I'm not sure that's a good idea Why not? It should be just removing the temporary file copy and it's backups. > broulik wrote in kioexecd.cpp:73 > You never unwatch the

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-08-31 Thread Kai Uwe Broulik
broulik added inline comments. INLINE COMMENTS > kioexecd.cpp:58 > qCDebug(KIOEXEC) << "About to delete" << parentDir << "containing" > << info.fileName(); > -QFile::remove(it.key()); > -QDir().rmdir(parentDir); > +QDir(parentDir).removeRecursively(); > }

D15180: kioexecd: watch for creations or modifications of the temporary files

2018-08-31 Thread Jaime Torres Amate
jtamate created this revision. jtamate added reviewers: Frameworks, broulik, ngraham, dfaure. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. jtamate requested review of this revision. REVISION SUMMARY When a non KIO friendly program opens a non local file,