D13174: New layout for the search bar.

[Ahmad Samir]

ahmadsamir added a comment.


  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).

INLINE COMMENTS

> IncrementalSearchBar.cpp:116
>      optionsButton->setPopupMode(QToolButton::InstantPopup);
> -    optionsButton->setArrowType(Qt::DownArrow);
> -    optionsButton->setToolButtonStyle(Qt::ToolButtonTextOnly);
> +    optionsButton->setArrowType(Qt::NoArrow);
> +    optionsButton->setToolButtonStyle(Qt::ToolButtonIconOnly);

This is redundant as Qt::NoArrow is the default for QToolButton: http://doc.qt.io/qt-5/qtoolbutton.html#arrowType-prop

> IncrementalSearchBar.cpp:190
> +        _searchFromButton->setIcon(QIcon::fromTheme(QStringLiteral("go-top")));
> +
>      }

There is indeed an unnecessary extra new line here.

_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.

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.

REPOSITORY
  R319 Konsole

REVISION DETAIL
  https://phabricator.kde.org/D13174

To: tcanabrava, #konsole, hindenburg
Cc: safaalfulaij, ahmadsamir, rizzitello, ngraham, konsole-devel, herrold, maximilianocuria, hindenburg
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/konsole-devel/attachments/20180612/d03efa88/attachment-0001.html>