<table><tr><td style="">rjvbb marked 3 inline comments as done.<br />rjvbb added inline comments.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D7742" rel="noreferrer">View Revision</a></tr></table><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D7742#inline-32100" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">abstractfilemanagerplugin.cpp:120</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">if the setting has changed, you still may have a watcher from before which you want to delete here. I don't think you should handle the setting, but rather make the code fail gracefully if no watcher exists for the given project:</p>

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">delete m_watchers.value(project, nullptr);
m_watchers.remove(project);</pre></div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Your code snippet looks like it achieves the same thing as my code. I take it your main suggestion was to remove the check for watchAllProjectDirectories()?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D7742#inline-32099" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">abstractfilemanagerplugin.cpp:396</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">if the setting has changed, you still may have a watcher from before which you want to stop here, see above</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Question is, should the watcher be deleted here if dirwatching is turned off?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D7742#inline-32101" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">abstractfilemanagerplugin.cpp:410</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">dito, but slightly changed - if the setting changed, did you recreate the watchers?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">The answer to that is "no, not unless the project was reloaded". Dirwatcher creation remains coupled to project import, and the checkbox callback doesn't trigger one. Just like the "parse all project files" it only affects future project imports.</p>

<p style="padding: 0; margin: 8px;">So this check must stay.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D7742#inline-32103" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">projectcontroller.cpp:916</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">this should be a separate patch, it has nothing to do with dirwatches</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Exactly what should be a separate patch, in fact? The whole timing logic and KDEV_DONT_DEFER_PROJECT_PARSING option is not intended for committing. Unless someone wants it to go in, in that case we can make it a separate commit of course.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D7742#inline-32104" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">projectcontroller.cpp:1288</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">this looks wrong, add the API to IProject and call the virtual directly, obsoleting this whole method</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Adding it to IProject would mean accessing the higher level IProjectFileManager class from there. That seems wrong too, so I've made a <tt style="background: #ebebeb; font-size: 13px;">virtual bool IProjectFileManager createWatcher(IProject*)</tt> method.</p>

<p style="padding: 0; margin: 8px;">Should I check the return value from <tt style="background: #ebebeb; font-size: 13px;">IProject::projectFileManager()</tt>?</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R32 KDevelop</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D7742" rel="noreferrer">https://phabricator.kde.org/D7742</a></div></div><br /><div><strong>To: </strong>rjvbb, KDevelop, mwolff, brauch<br /><strong>Cc: </strong>mwolff, kossebau, arrowdodger, brauch, zhigalin, kdevelop-devel, geetamc, Pilzschaf, akshaydeo, surgenight<br /></div>