<table><tr><td style="">dfaure added a comment.
</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/D23384">View Revision</a></tr></table><br /><div><div><p>Why does KRun duplicate all of the (new) code from DesktopExecParser, when DesktopExecParser is actually a helper class for KRun?<br />
I would expect it to have solved all this already, unless I'm missing something about the various code paths.<br />
The code you changed in krun.cpp was supposed to simply resolve desktop:/foo to file:///bleh but everything else about %u/%f is done by DesktopExecParser.</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/D23384#inline-145168">View Inline</a><span style="color: #4b4d51; font-weight: bold;">desktopexecparser.cpp:311</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: #74777d">// Check if we need kioexec</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">bool</span> <span class="n">useKioexec</span> <span style="color: #aa2211">=</span> <span style="color: #304a96">false</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">, or KIOFuse</tt></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/D23384#inline-145160">View Inline</a><span style="color: #4b4d51; font-weight: bold;">desktopexecparser.cpp:317</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">struct</span> <span class="n">MountRequest</span> <span class="p">{</span> <span class="n">QDBusPendingReply</span><span style="color: #aa2211"><</span><span class="n">QString</span><span style="color: #aa2211">></span> <span class="n">reply</span><span class="p">;</span> <span style="color: #aa4000">int</span> <span class="n">urlIndex</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">QVector</span><span style="color: #aa2211"><</span><span class="n">MountRequest</span><span style="color: #aa2211">></span> <span class="n">requests</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">if</span> <span class="p">(</span><span style="color: #aa2211">!</span><span class="n">mx1</span><span class="p">.</span><span class="n">hasUrls</span><span class="p">)</span> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">requests.reserve(d->urls.count());</tt></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/D23384#inline-145161">View Inline</a><span style="color: #4b4d51; font-weight: bold;">desktopexecparser.cpp:319</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">if</span> <span class="p">(</span><span style="color: #aa2211">!</span><span class="n">mx1</span><span class="p">.</span><span class="n">hasUrls</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span style="color: #aa4000">for</span> <span class="p">(<span class="bright"></span></span><span class="bright"></span><span style="color: #aa4000"><span class="bright">const</span></span><span class="bright"> </span><span class="n"><span class="bright">QUrl</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">&</span></span><span class="bright"></span><span style="color: #a0a000"><span class="bright">url</span></span><span class="bright"> </span><span class="p"><span class="bright">:</span></span><span class="bright"> </span><span class="n"><span class="bright">qAsConst</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="n">d</span><span style="color: #aa2211">-></span><span class="n">urls<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">))</span></span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">for</span> <span class="p">(<span class="bright"></span></span><span class="bright"></span><span style="color: #aa4000"><span class="bright">int</span></span><span class="bright"> </span><span class="n"><span class="bright">i</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">=</span></span><span class="bright"> </span><span style="color: #601200"><span class="bright">0</span></span><span class="bright"></span><span class="p"><span class="bright">;</span></span><span class="bright"> </span><span class="n"><span class="bright">i</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright"><</span></span><span class="bright"> </span><span class="n">d</span><span style="color: #aa2211">-></span><span class="n">urls<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="bright"></span><span class="n"><span class="bright">length</span></span><span class="bright"></span><span class="p"><span class="bright">();</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">++</span></span><span class="bright"></span><span class="n"><span class="bright">i</span></span><span class="bright"></span><span class="p"><span class="bright">)</span></span><span class="bright"> </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">QUrl</span> <span class="n">url</span> <span style="color: #aa2211">=</span> <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">urls</span><span class="p">[</span><span class="n">i</span><span class="p">];</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Minor: can you use count() (Qt-like) or size() (STL-like)? In 20 years I never saw code that uses length() for a vector or a list, it's just very unusual in Qt code :-)</p>
<p style="padding: 0; margin: 8px;">[occurs twice]</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/D23384#inline-145162">View Inline</a><span style="color: #4b4d51; font-weight: bold;">desktopexecparser.cpp:320</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(251, 175, 175, .7);"> <span style="color: #aa4000">for</span> <span class="p">(<span class="bright"></span></span><span class="bright"></span><span style="color: #aa4000"><span class="bright">const</span></span><span class="bright"> </span><span class="n"><span class="bright">QUrl</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">&</span></span><span class="bright"></span><span style="color: #a0a000"><span class="bright">url</span></span><span class="bright"> </span><span class="p"><span class="bright">:</span></span><span class="bright"> </span><span class="n"><span class="bright">qAsConst</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="n">d</span><span style="color: #aa2211">-></span><span class="n">urls<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">))</span></span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">for</span> <span class="p">(<span class="bright"></span></span><span class="bright"></span><span style="color: #aa4000"><span class="bright">int</span></span><span class="bright"> </span><span class="n"><span class="bright">i</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">=</span></span><span class="bright"> </span><span style="color: #601200"><span class="bright">0</span></span><span class="bright"></span><span class="p"><span class="bright">;</span></span><span class="bright"> </span><span class="n"><span class="bright">i</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright"><</span></span><span class="bright"> </span><span class="n">d</span><span style="color: #aa2211">-></span><span class="n">urls<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="bright"></span><span class="n"><span class="bright">length</span></span><span class="bright"></span><span class="p"><span class="bright">();</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">++</span></span><span class="bright"></span><span class="n"><span class="bright">i</span></span><span class="bright"></span><span class="p"><span class="bright">)</span></span><span class="bright"> </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">QUrl</span> <span class="n">url</span> <span style="color: #aa2211">=</span> <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">urls</span><span class="p">[</span><span class="n">i</span><span class="p">];</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span style="color: #aa4000">if</span> <span class="p">(</span><span style="color: #aa2211">!</span><span class="n">url</span><span class="p">.</span><span class="n">isLocalFile</span><span class="p">()</span> <span style="color: #aa2211">&&</span> <span style="color: #aa2211">!</span><span class="n">hasSchemeHandler</span><span class="p">(</span><span class="n">url</span><span class="p">))</span> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Use d->urls.at(i) to avoid a detach given that d->urls isn't const here.</p>
<p style="padding: 0; margin: 8px;">[occurs twice]</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/D23384#inline-145176">View Inline</a><span style="color: #4b4d51; font-weight: bold;">desktopexecparser.cpp:323</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">// Lets try a KIOFuse mount instead.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">requests</span><span class="p">.</span><span class="n">push_back</span><span class="p">({</span> <span class="n">kiofuse_iface</span><span class="p">.</span><span class="n">mountUrl</span><span class="p">(</span><span class="n">url</span><span class="p">.</span><span class="n">toString</span><span class="p">()),</span> <span class="n">i</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;">Could we have a kill switch for KIOFuse if it fails to work properly in some release?</p>
<p style="padding: 0; margin: 8px;">E.g. some <tt style="background: #ebebeb; font-size: 13px;">export KIO_FUSE=0</tt> or <tt style="background: #ebebeb; font-size: 13px;">export KIO_FUSE_DISABLE=1</tt>.<br />
Or a KConfig entry (use <tt style="background: #ebebeb; font-size: 13px;">KSharedConfig::openConfig()->group("KIO")</tt> for best performance and flexibility -- then it can be disabled for one calling application or for all)</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/D23384#inline-145174">View Inline</a><span style="color: #4b4d51; font-weight: bold;">desktopexecparser.cpp:330</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">// NOTE: Some non-KIO apps may support the URLs (e.g. VLC supports smb://)</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// but will struggle with passwords.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// Hence convert URL to KIOFuse equivalent in case there is a password.</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">"Struggle" sounds like the apps are limited/buggy.</p>
<p style="padding: 0; margin: 8px;">But if what you mean is that the password (e.g. stored in kpasswdserver after the user typed it in the KDE dialog asking for it) won't be available to non-KIO applications, that seems logical and I do accept that argument. It's just the writing that is surprising.</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/D23384#inline-145175">View Inline</a><span style="color: #4b4d51; font-weight: bold;">desktopexecparser.cpp:331</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">// but will struggle with passwords.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// Hence convert URL to KIOFuse equivalent in case there is a password.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// @see https://pointieststick.com/2018/01/17/videos-on-samba-shares/</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">The comment says "in case there is a password", the code doesn't.<br />
Probably historical, please fix :)</p>
<p style="padding: 0; margin: 8px;">On this topic I saw earlier comments in the merge request which I found confusing.<br />
It's very rare for an application to have the password stored inside the URL itself. That would mean the user typing it in clear in a location bar or in a terminal, bad idea. Instead, the user typically types a URL like ftp://user@host/ and the kioslave asks for the password, and stores it in kpasswdserver.</p>
<p style="padding: 0; margin: 8px;">Also you wrote "we strip out the stuff that's in userInfo()". (where QUrl::userInfo is username+password). But surely, while we might strip out passwords for security reasons, we never strip out the username, do we?</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/D23384#inline-145169">View Inline</a><span style="color: #4b4d51; font-weight: bold;">desktopexecparser.cpp:348</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">// Lets try a KIOFuse mount instead.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">requests</span><span class="p">.</span><span class="n">push_back</span><span class="p">({</span> <span class="n">kiofuse_iface</span><span class="p">.</span><span class="n">mountUrl</span><span class="p">(</span><span class="n">url</span><span class="p">.</span><span class="n">toString</span><span class="p">()),</span> <span class="n">i</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;">This duplicates line 344.</p>
<p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">if (a) { if (!b) { foo } } else { foo }</tt><br />
is equivalent to<br />
<tt style="background: #ebebeb; font-size: 13px;">if (!a || !b) { foo }</tt></p>
<p style="padding: 0; margin: 8px;">In your case <tt style="background: #ebebeb; font-size: 13px;">a</tt> is <tt style="background: #ebebeb; font-size: 13px;">http || https</tt> which makes this a big uglier, but you could use what other pieces of code do and write url.scheme().startsWith(QLatin1String("http")).</p>
<p style="padding: 0; margin: 8px;">So this becomes <tt style="background: #ebebeb; font-size: 13px;">if (!url.scheme().startsWith(QLatin1String("http")) || !supportedProtocols(d->service).contains(url.scheme())) {</tt></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/D23384#inline-145165">View Inline</a><span style="color: #4b4d51; font-weight: bold;">desktopexecparser.cpp:355</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(251, 175, 175, .7);"> <span class="bright"> </span><span style="color: #aa4000"><span class="bright">for</span></span><span class="bright"> </span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #aa4000"><span class="bright">const</span></span><span class="bright"> </span><span class="n"><span class="bright">QUrl</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">&</span></span><span class="bright"></span><span style="color: #a0a000"><span class="bright">url</span></span><span class="bright"> </span><span class="p"><span class="bright">:</span></span><span class="bright"> </span><span class="n"><span class="bright">qAsConst</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">d</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">-></span></span><span class="bright"></span><span class="n"><span class="bright">urls</span></span><span class="bright"></span><span class="p"><span class="bright">))</span></span><span class="bright"> </span><span class="p"><span class="bright">{</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"> </span><span style="color: #aa4000"><span class="bright">if</span></span><span class="bright"> </span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">!</span></span><span class="bright"></span><span class="n"><span class="bright">isProtocolInSupportedList</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">url</span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span class="n"><span class="bright">appSupportedProtocols</span></span><span class="bright"></span><span class="p"><span class="bright">)</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">&&</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">!</span></span><span class="bright"></span><span class="n"><span class="bright">hasSchemeHandler</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">url</span></span><span class="bright"></span><span class="p"><span class="bright">)</span>)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span style="color: #74777d"><span class="bright">// Main thing that we want is to send the mount requests without blocking.</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">for</span></span><span class="bright"> </span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #aa4000"><span class="bright">auto</span></span><span class="bright"> </span><span style="color: #a0a000"><span class="bright">request</span></span><span class="bright"> </span><span class="p"><span class="bright">:</span></span><span class="bright"> </span><span class="n"><span class="bright">requests</span></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">request</span><span class="p">.</span><span class="n">reply</span><span class="p">.</span><span class="n">waitForFinished</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">auto &request</tt> to avoid copying</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/D23384#inline-145166">View Inline</a><span style="color: #4b4d51; font-weight: bold;">desktopexecparser.cpp:356</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(251, 175, 175, .7);"> <span class="bright"> </span><span style="color: #aa4000"><span class="bright">if</span></span><span class="bright"> </span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">!</span></span><span class="bright"></span><span class="n"><span class="bright">isProtocolInSupportedList</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">url</span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span class="n"><span class="bright">appSupportedProtocols</span></span><span class="bright"></span><span class="p"><span class="bright">)</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">&&</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">!</span></span><span class="bright"></span><span class="n"><span class="bright">hasSchemeHandler</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">url</span></span><span class="bright"></span><span class="p"><span class="bright">)</span>)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">for</span></span><span class="bright"> </span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #aa4000"><span class="bright">auto</span></span><span class="bright"> </span><span style="color: #a0a000"><span class="bright">request</span></span><span class="bright"> </span><span class="p"><span class="bright">:</span></span><span class="bright"> </span><span class="n"><span class="bright">requests</span></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">request</span><span class="p">.</span><span class="n">reply</span><span class="p">.</span><span class="n">waitForFinished</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">request</span><span class="p">.</span><span class="n">reply</span><span class="p">.</span><span class="n">isError</span><span class="p">())</span> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This blocks.</p>
<p style="padding: 0; margin: 8px;">I don't mind much myself, but I know some people had a mandate to remove as many blocking calls as possible from KRun and related code. Or was that only avoiding blocking I/O because of network mounts? [which this patch is all about adding more of...]. <a href="https://phabricator.kde.org/p/broulik/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@broulik</a> ?</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/D23384#inline-145177">View Inline</a><span style="color: #4b4d51; font-weight: bold;">desktopexecparser.cpp:378</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">return</span> <span class="n">result</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 style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// At this point we know we're not using kioexec, so feel free to replace</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This nesting under <tt style="background: #ebebeb; font-size: 13px;">else</tt> is unnecessary given the <tt style="background: #ebebeb; font-size: 13px;">return</tt> just above. It's even a bit confusing that some code is in <tt style="background: #ebebeb; font-size: 13px;">else</tt> and then some code is outside the <tt style="background: #ebebeb; font-size: 13px;">if/else</tt>, but the '}' could be anywhere in between, that would make no difference.</p>
<p style="padding: 0; margin: 8px;">I would suggest to just remove the <tt style="background: #ebebeb; font-size: 13px;">else</tt>.</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/D23384#inline-145167">View Inline</a><span style="color: #4b4d51; font-weight: bold;">desktopexecparser.cpp:381</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">// KIO URLs with their KIOFuse local path.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">for</span> <span class="p">(</span><span style="color: #aa4000">auto</span> <span style="color: #a0a000">request</span> <span class="p">:</span> <span class="n">requests</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">d</span><span style="color: #aa2211">-></span><span class="n">urls</span><span class="p">[</span><span class="n">request</span><span class="p">.</span><span class="n">urlIndex</span><span class="p">]</span> <span style="color: #aa2211">=</span> <span class="n">QUrl</span><span style="color: #aa2211">::</span><span class="n">fromLocalFile</span><span class="p">(</span><span class="n">request</span><span class="p">.</span><span class="n">reply</span><span class="p">.</span><span class="n">value</span><span class="p">());</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">for (const auto & request : qAsConst(requests))</tt></p>
<p style="padding: 0; margin: 8px;">or use std::vector if you find qAsConst ugly :-)</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/D23384#inline-145173">View Inline</a><span style="color: #4b4d51; font-weight: bold;">CMakeLists.txt:80</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; ">qt5_add_dbus_interface(kiowidgets_SRCS org.kde.kuiserver.xml kuiserver_interface)
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">qt5_add_dbus_interface(kiowidgets_SRCS org.kde.KIOFuse.VFS.xml kiofuse_interface)
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Just use ../core/org.kde.KIOFuse.VFS.xml instead of duplicating the file.</p>
<p style="padding: 0; margin: 8px;">But even better would be to not need it in KRun, see separate comment.</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/D23384#inline-145170">View Inline</a><span style="color: #4b4d51; font-weight: bold;">krun.cpp:581</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">QVector</span><span style="color: #aa2211"><</span><span class="n">MountRequest</span><span style="color: #aa2211">></span> <span class="n">requests</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">for</span> <span class="p">(</span><span style="color: #aa4000">int</span> <span class="n">i</span> <span style="color: #aa2211">=</span> <span style="color: #601200">0</span><span class="p">;</span> <span class="n">i</span> <span style="color: #aa2211"><</span> <span class="n">urls</span><span class="p">.</span><span class="n">length</span><span class="p">();</span> <span style="color: #aa2211">++</span><span class="n">i</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">QUrl</span> <span class="n">url</span> <span style="color: #aa2211">=</span> <span class="n">urls</span><span class="p">[</span><span class="n">i</span><span class="p">];</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">count or size</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/D23384#inline-145171">View Inline</a><span style="color: #4b4d51; font-weight: bold;">krun.cpp:582</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">for</span> <span class="p">(</span><span style="color: #aa4000">int</span> <span class="n">i</span> <span style="color: #aa2211">=</span> <span style="color: #601200">0</span><span class="p">;</span> <span class="n">i</span> <span style="color: #aa2211"><</span> <span class="n">urls</span><span class="p">.</span><span class="n">length</span><span class="p">();</span> <span style="color: #aa2211">++</span><span class="n">i</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">QUrl</span> <span class="n">url</span> <span style="color: #aa2211">=</span> <span class="n">urls</span><span class="p">[</span><span class="n">i</span><span class="p">];</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #74777d">// NOTE: Some non-KIO apps may support the URLs (e.g. VLC supports smb://)</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">.at(i)</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/D23384">https://phabricator.kde.org/D23384</a></div></div><br /><div><strong>To: </strong>feverfew, fvogt, davidedmundson, dfaure, ngraham<br /><strong>Cc: </strong>alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, michaelh, bruns<br /></div>