D7995: KDevelop: address dirwatching inefficiency (WIP/PoC)

[René J.V. Bertin]

rjvbb marked 11 inline comments as done.
rjvbb added inline comments.

INLINE COMMENTS

> mwolff wrote in abstractfilemanagerplugin.cpp:130
> delete + take like before
> 
> not sure this comment was seen, phab says the above comment isn't saved/submitted. No clue how to resubmit it...

It's done locally but I'm waiting for more info to include other requested changes in the next revision too.

> mwolff wrote in abstractfilemanagerplugin.cpp:297
> This patch should target master, not 5.2. It's highly experimental.
> 
> Then see above, remove the dead code in master in a separate patch, but keep this part of the loop. Then rebase your patch remove the rest of this loop, too.

Please be more specific exactly what I have to remove and what loop you're referring to. There are 2 columns with line numbers I can choose from.

Are you saying this won't make it into 5.2? As a Mac user and thus concerned directly by the current inefficiency that would cool off my enthusiasm to invest in this significantly.

> rjvbb wrote in abstractfilemanagerplugin.cpp:409
> As stated in the comment, I don't know if we (read I) can be certain the directory to restart will always be monitored already.
> 
> I suspect there is a slightly bigger chance with the proposed dirwatching model that we end here for a new directory that somehow hasn't been processed by the dirty handler yet. I'll put a qWarning and run that locally for a bit to see if it ever triggers.
> 
> I made this change because it seemed like a simple and cheap failsafe, using a KDW feature that was undoubtedly implemented for a reason.

Doh, I had already added that warning message.

> mwolff wrote in abstractfilemanagerplugin.cpp:409
> Well, then do it in a separate patch - I'm not going to oppose it. But it's not related to the bigger change set here at all.

direct commit or review first? I'd prefer the former, evidently.

> mwolff wrote in projectwatcher.cpp:33
> Can you please clarify this:
> 
> Are you saying different KDirWatcher instances share the same QFileSystemWatcher? If so, that needs to be changed upstream, but I'd be OK with working around that for now.

Indeed, that's my understanding from looking over kdirwatch.cpp (which is a mess to read for outsiders).

@dfaure : do you know if there was a reason to use a single QFSW instance and if that reason is still valid?

I could easily imagine (for instance) that it'd be a good idea to use a single QFSW instance per volume (disk, partition) and that in practice that boils down to using a single QFSW instance.

I'll have to check if the Mac FSEvents backend uses a single native instance. I should also try to understand why its own internal mutex didn't prevent the concurrency issues I mentioned above (emphasis on "try" :-/)

> mwolff wrote in projectwatcher.h:38
> Sure, but it's against our policy. We only pimpl public stuff.

Understood. It will be removed but maybe not yet in the next patch revision.

REPOSITORY
  R32 KDevelop

REVISION DETAIL
  https://phabricator.kde.org/D7995

To: rjvbb, #kdevelop, mwolff
Cc: dfaure, mwolff, brauch, kdevelop-devel, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20170927/67e9ae6f/attachment-0001.html>