<table><tr><td style="">ervin requested changes to this revision.<br />ervin added a comment.<br />This revision now requires changes to proceed.
</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/D8332" rel="noreferrer">View Revision</a></tr></table><br /><div><div><p>All the fiddling with URLs makes me wonder if that wouldn't be better done on the KIO implementations side... but that's out of scope for that patch I think.</p></div></div><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/D8332#inline-37271" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kfileplacesmodeltest.cpp:98</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// disable baloo</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">KConfig</span> <span style="color: #004012">config</span><span class="p">(</span><span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">"baloofilerc"</span><span class="p">));</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Shouldn't we have tests verifying the extra URLs are properly inserted when baloo is enabled? Seems like it's trying to avoid testing the behavior change coming from the rest of the commit.</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/D8332#inline-37278" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kfileplacesmodel.cpp:68</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">bookmarkManager</span><span class="p">(</span><span style="color: #aa4000">nullptr</span><span class="p">),</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">fileIndexingEnabled</span><span class="p">(</span><span class="n">isFileIndexingEnabled</span><span class="p">())</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Do we really want to keep that state? It's never reevaluated so could be a const if we keep it.</p>
<p style="padding: 0; margin: 8px;">Asks the question of what happens if the setting changes at runtime though.</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/D8332#inline-37277" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kfileplacesmodel.cpp:499</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">bool</span> <span class="n">allowedHere</span> <span style="color: #aa2211">=</span> <span class="n">appName</span><span class="p">.</span><span class="n">isEmpty</span><span class="p">()</span> <span style="color: #aa2211">||</span> <span class="p">(</span><span class="n">appName</span> <span style="color: #aa2211">==</span> <span class="n">QCoreApplication</span><span style="color: #aa2211">::</span><span class="n">instance</span><span class="p">()</span><span style="color: #aa2211">-></span><span class="n">applicationName</span><span class="p">());</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">bool</span> <span class="n">allowBalooUrl</span> <span style="color: #aa2211">=</span> <span class="n">isBalooUrl</span><span class="p">(</span><span class="n">url</span><span class="p">)</span> <span style="color: #aa2211">?</span> <span style="color: #a0a000">fileIndexingEnabled</span> <span class="p">:</span> <span style="color: #304a96">true</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I find that naming confusing I mean, semantically fileIndexingEnabled would be your "allowBalooUrl". Could we come up with something better? Like "isSupportedUrl" perhaps? This is what it is about somewhat, we support all schemes except for the baloo ones depending on fileIndexingEnabled.</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/D8332#inline-37281" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kfileplacesview.cpp:1301</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">const</span> <span class="n">QString</span> <span class="n">path</span> <span style="color: #aa2211">=</span> <span class="n">url</span><span class="p">.</span><span class="n">toDisplayString</span><span class="p">(</span><span class="n">QUrl</span><span style="color: #aa2211">::</span><span class="n">PreferLocalFile</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">path</span><span class="p">.</span><span class="n">endsWith</span><span class="p">(</span><span class="n">QLatin1String</span><span class="p">(</span><span style="color: #766510">"yesterday"</span><span class="p">)))</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">const</span> <span class="n">QDate</span> <span class="n">date</span> <span style="color: #aa2211">=</span> <span class="n">QDate</span><span style="color: #aa2211">::</span><span class="n">currentDate</span><span class="p">().</span><span class="n">addDays</span><span class="p">(</span><span style="color: #aa2211">-</span><span style="color: #601200">1</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Don't you want to match /yesterday and so on here? Like you're matching /documents and so on for the method below.</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/D8332#inline-37280" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kfileplacesview.cpp:1329</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">day</span> <span style="color: #aa2211">></span> <span style="color: #601200">0</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">date</span> <span style="color: #aa2211">=</span> <span class="n">date</span><span class="p">.</span><span class="n">arg</span><span class="p">(</span><span class="n">QString</span><span class="p">(</span><span style="color: #766510">"-%1"</span><span class="p">).</span><span class="n">arg</span><span class="p">(</span><span class="n">day</span><span class="p">,</span> <span style="color: #601200">2</span><span class="p">,</span> <span style="color: #601200">10</span><span class="p">,</span> <span class="n">QChar</span><span class="p">(</span><span style="color: #766510">'0'</span><span class="p">)));</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">}</span> <span style="color: #aa4000">else</span> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This is unusual, why not concatenate + arg() in that case instead of the nested arg calls?</p>
<p style="padding: 0; margin: 8px;">Would allow you to drop the else part too.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R241 KIO</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D8332" rel="noreferrer">https://phabricator.kde.org/D8332</a></div></div><br /><div><strong>To: </strong>renatoo, Frameworks, Dolphin, KDE Applications, dvratil, VDG, ngraham, ervin<br /><strong>Cc: </strong>ervin, usta, mlaurent, dvratil, ngraham, Frameworks<br /></div>