<table><tr><td style="">kfunk accepted this revision.<br />kfunk added inline comments.<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/D4808" 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/D4808#inline-19447" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kossebau</span> wrote in <span style="color: #4b4d51; font-weight: bold;">grepoutputmodel.cpp:153</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">The Grep toolview has two modes: Find-only mode and Replace mode.</p>

<p style="padding: 0; margin: 8px;">The Find-only mode is the default mode and entered on doing the search.<br />
The Replace mode is triggered once one starts editing the "Replacement text" field.</p>

<p style="padding: 0; margin: 8px;">In the Replace mode all items get changed to isCheckable = true, resulting in all items being shown with a checkbox, so one can select by toggling the checkboxes which of the found items will be included when doing the actual replacement.</p>

<p style="padding: 0; margin: 8px;">So the isCheckable state reflects if in Find-only mode or Replace mode. The isChecked state only reflects if an item should be included in the replacement, it does not hint to the mode.</p>

<p style="padding: 0; margin: 8px;">The bug is this:<br />
Currently the tooltip only cares about the Replacement mode, previewing the version of the item with the replacement done. Which in Find-only mode does not make sense, especially as the replacement text is empty and thus the tooltip shows only incomplete content. <br />
Now one option would be to just not show the tooltip in Find mode. But it still serves a purpose there, when the matched line is much longer than what is shown in the view, so the tooltip allows to see the complete text (like done in similar tree views).<br />
So the fix here is to in Find mode (as reflected by the isCheckable == false) use the original text, not the replacement text, for the matched content in the line.</p>

<p style="padding: 0; margin: 8px;">Any proposal how to make that more clear in the in-code comment?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Okay, understood, that's what I /thought/ as well.</p>

<p style="padding: 0; margin: 8px;">My complaint is that using <tt style="background: #ebebeb; font-size: 13px;">isCheckable()</tt> isn't going far enough. It should be <tt style="background: #ebebeb; font-size: 13px;">isCheckable() && isChecked()</tt> -- since it only makes sense to show the to-be-replaced text when we actually want to replace it (IOW: when it is checked).</p>

<p style="padding: 0; margin: 8px;">Anyhow, much better then before already: so go for it. Thanks!</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R33 KDevPlatform</div></div></div><br /><div><strong>BRANCH</strong><div><div>smallfixestogrepview</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D4808" rel="noreferrer">https://phabricator.kde.org/D4808</a></div></div><br /><div><strong>EMAIL PREFERENCES</strong><div><a href="https://phabricator.kde.org/settings/panel/emailpreferences/" rel="noreferrer">https://phabricator.kde.org/settings/panel/emailpreferences/</a></div></div><br /><div><strong>To: </strong>kossebau, KDevelop, kfunk<br /><strong>Cc: </strong>kfunk, kdevelop-devel<br /></div>