<table><tr><td style="">ahmadsamir 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/D13174">View Revision</a></tr></table><br /><div><div><p>I haven't tested this, but does it make sense to just make the searchBar part of the TerminalDisplay Class and do away the TerminalWidget concept? theoretically, in my mind anyway, it would make this patch a lot smaller as calls to TerminalDisplay objects wouldn't need to be changed then. (Just an untested thought, I'll hopefully try and test this tomorrow).</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/D13174#inline-70543">View Inline</a><span style="color: #4b4d51; font-weight: bold;">IncrementalSearchBar.cpp:116</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 class="n">optionsButton</span><span style="color: #aa2211">-></span><span class="n">setPopupMode</span><span class="p">(</span><span class="n">QToolButton</span><span style="color: #aa2211">::</span><span class="n">InstantPopup</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="n">optionsButton</span><span style="color: #aa2211">-></span><span class="n">setArrowType</span><span class="p">(</span><span class="n">Qt</span><span style="color: #aa2211">::<span class="bright"></span></span><span class="bright"></span><span class="n"><span class="bright">Down</span>Arrow</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="n">optionsButton</span><span style="color: #aa2211">-></span><span class="n">setToolButtonStyle</span><span class="p">(</span><span class="n">Qt</span><span style="color: #aa2211">::</span><span class="n">ToolButton<span class="bright">Text</span>Only</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">optionsButton</span><span style="color: #aa2211">-></span><span class="n">setArrowType</span><span class="p">(</span><span class="n">Qt</span><span style="color: #aa2211">::<span class="bright"></span></span><span class="bright"></span><span class="n"><span class="bright">No</span>Arrow</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">optionsButton</span><span style="color: #aa2211">-></span><span class="n">setToolButtonStyle</span><span class="p">(</span><span class="n">Qt</span><span style="color: #aa2211">::</span><span class="n">ToolButton<span class="bright">Icon</span>Only</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This is redundant as Qt::NoArrow is the default for QToolButton: <a href="http://doc.qt.io/qt-5/qtoolbutton.html#arrowType-prop" class="remarkup-link" target="_blank" rel="noreferrer">http://doc.qt.io/qt-5/qtoolbutton.html#arrowType-prop</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/D13174#inline-70544">View Inline</a><span style="color: #4b4d51; font-weight: bold;">IncrementalSearchBar.cpp:190</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">_searchFromButton</span><span style="color: #aa2211">-></span><span class="n">setIcon</span><span class="p">(</span><span class="n">QIcon</span><span style="color: #aa2211">::</span><span class="n">fromTheme</span><span class="p">(</span><span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">"go-top"</span><span class="p">)));</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="p">}</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">There is indeed an unnecessary extra new line here.</p>
<p style="padding: 0; margin: 8px;">_searchFromButton->setIcon() should be in the same order in the if{}else{}, right after the _searchFrom->setToolTip() call, and before the other two setIcon() calls, just so that it's all consistent.</p>
<p style="padding: 0; margin: 8px;">go-previous and go-next look a bit wrong, as they are left/right pointing arrows, respectively, and ideally one searches up/down in a document... etc (it's more logical somehow :)). IINM you changed the icons so as to differentiate between the searchFromButton arrow icon the other two arrow icons; but I suggest using go-up and go-down, respectively.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R319 Konsole</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D13174">https://phabricator.kde.org/D13174</a></div></div><br /><div><strong>To: </strong>tcanabrava, Konsole, hindenburg<br /><strong>Cc: </strong>safaalfulaij, ahmadsamir, rizzitello, ngraham, konsole-devel, herrold, maximilianocuria, hindenburg<br /></div>