<table><tr><td style="">rkflx accepted this revision.<br />rkflx added a comment.<br />This revision is now accepted and ready to land.
</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/D8056" rel="noreferrer">View Revision</a></tr></table><br /><div><div><blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D8056#205595" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;" rel="noreferrer">D8056#205595</a>, <a href="https://phabricator.kde.org/p/simgunz/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;" rel="noreferrer">@simgunz</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>Fixed the problems reported by <a href="https://phabricator.kde.org/p/rkflx/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;" rel="noreferrer">@rkflx</a> hopefully without introducing new ones. I did some testing and now everything seems ok, but please check again.</p></div>
</blockquote>
<p>Thanks Simone, works great now.</p>
<p>Successfully tested opening a file with the following methods:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">mouse selection only</li>
<li class="remarkup-list-item">type a few characters, select with the mouse</li>
<li class="remarkup-list-item">type a few characers, hit <kbd style="display: inline-block; min-width: 1em; padding: 4px 5px 5px; font-weight: normal; font-size: 0.8rem; text-align: center; text-decoration: none; line-height: 0.6rem; border-radius: 3px; box-shadow: inset 0 -1px 0 rgba(71, 87, 120, 0.08); user-select: none; background: #f7f7f7; border: 1px solid #C7CCD9;">Enter</kbd></li>
<li class="remarkup-list-item">double click on entry</li>
<li class="remarkup-list-item">history dropdown</li>
<li class="remarkup-list-item">enable completion dropdown, open with custom bin in path</li>
<li class="remarkup-list-item">open button</li>
<li class="remarkup-list-item">paste custom bin not in path</li>
<li class="remarkup-list-item">autocomplete custom path</li>
<li class="remarkup-list-item"><kbd title="Down" style="display: inline-block; min-width: 1em; padding: 4px 5px 5px; font-weight: normal; font-size: 0.8rem; text-align: center; text-decoration: none; line-height: 0.6rem; border-radius: 3px; box-shadow: inset 0 -1px 0 rgba(71, 87, 120, 0.08); user-select: none; background: #f7f7f7; border: 1px solid #C7CCD9;">↓</kbd> to move focus (this breaks with the completion enabled, though)</li>
</ul>
<p>That's basically all the dialog should be able to do that I know of, therefore we can land it (once the tooltip is changed).</p>
<p>For improving the usability, I still want to open a task with some ideas to discuss them further (really soon™, the first step is done by listing the current functionality ↑↑↑). [My promise for Nate's <a href="https://phabricator.kde.org/D10245" class="remarkup-link" target="_blank" rel="noreferrer">https://phabricator.kde.org/D10245</a> is still open too…]</p>
<hr class="remarkup-hr" />
<blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D8056#205608" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;" rel="noreferrer">D8056#205608</a>, <a href="https://phabricator.kde.org/p/simgunz/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;" rel="noreferrer">@simgunz</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>The tooltip of the line edit now says: <tt style="background: #ebebeb; font-size: 13px;">Type some text to filter the application tree.\nPress down arrow to navigate the results tree.</tt><br />
I suggest to change it to: <tt style="background: #ebebeb; font-size: 13px;">Type to filter the application tree or type the name of a command.\nPress down arrow to navigate the results tree.</tt> just to avoid having a hidden feature, which is the possibility to type commands. Also because the completion mode default is now set to None, so there will not even be the completion that suggests the existence of this feature.</p></div>
</blockquote>
<p>You are right, the least we can do for now is to improve the tooltip. I don't think users know what a "tree" is, so how about this:</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);">Type to filter the applications below, or specify the name of a command.
Press down arrow to navigate the results.</pre></div>
<p>Use/adapt it if you like it, I'll wait with landing.</p>
<hr class="remarkup-hr" />
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>When you click <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">OK</span></span></span> the function <tt style="background: #ebebeb; font-size: 13px;">checkAccepted</tt> is called. To me it seems that it actually uses the service associated to the selected "Kate" entry (<tt style="background: #ebebeb; font-size: 13px;">kate -b %U</tt>) and does not execute the command <tt style="background: #ebebeb; font-size: 13px;">kate</tt>. <br />
(That function is quite complicated and I haven't really analyzed it completely)</p></blockquote>
<p>Yeah, that's not ideal (but not too bad since the completion is off by default). In my testing the service was also preferred over the executable, which is fine because you can explicitly specify the latter by typing the full path.</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/D8056#inline-49532" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kopenwithdialog.cpp:834-836</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">if</span> <span class="p">(</span><span class="n">typedExec</span><span class="p">.</span><span class="n">isEmpty</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">return</span> <span style="color: #304a96">false</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I checked this, but AFAIK it's fine to remove. <a href="https://phabricator.kde.org/p/dfaure/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;" rel="noreferrer">@dfaure</a> introduced this 10 years ago in <a href="https://phabricator.kde.org/R446:d6903613f07b05640f56bcbaa867c0b6441785bb" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;" rel="noreferrer">R446:d6903613f07b</a>, but I guess we are covered both by the disabled button for empty line edits as well as the checks below.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R241 KIO</div></div></div><br /><div><strong>BRANCH</strong><div><div>openwithdialog-filter-app-tree</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D8056" rel="noreferrer">https://phabricator.kde.org/D8056</a></div></div><br /><div><strong>To: </strong>simgunz, dfaure, Frameworks, VDG, ngraham, rkflx<br /><strong>Cc: </strong>rkflx, romangg, fabianr, abetts, ngraham, alexeymin, Frameworks, michaelh<br /></div>