D15450: Make sublime tool bar buttons shrinkable and elide text

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Very clean implementation, I like this :) And from what I tested this works nicely.
  
  Some code style feedback for now, will think through the logic tonight, but so far looks fine.

INLINE COMMENTS

> idealbuttonbarwidget.cpp:255
>      QString buttonId = id(button);
>      if (!m_buttonsOrder.contains(buttonId)) {
> +        m_buttonsOrder.push_back(buttonId);

Why did you remove the variant for the bottom docker? I have no idea why that exists :) but then I also do not see why it should be removed, as it changes behaviour people have got used to, no?

> idealbuttonbarwidget.h:97
>      QStringList m_buttonsOrder;
> +    QLayout* m_buttonsLayout;
>  };

Even while not really needed for the API used, let's use the final type `IdealButtonBarLayout`, otherwise the code reader could assume that potentially different subclasses of QLayout might be used with this member.

> ideallayout.cpp:25
>  
> +#include <numeric>
> +

Please move to be the last include. While inconsistent throughout kdevelop codebase, the recommended order is own->kf->qt->std

> ideallayout.cpp:201
>  
> -    for (QLayoutItem *item : _items) {
> -        const QSize itemSizeHint = item->sizeHint();
> -        if (y + itemSizeHint.height() + b > rect.height()) {
> -            int newX = x + currentLineWidth + buttonSpacing;
> -            if (newX + itemSizeHint.width() + r <= rect.width())
> +    if (_items.empty())
> +        return x + currentLineWidth + r;

Please also wrap single lines with {}:

  f (_items.empty()) {
      return x + currentLineWidth + r;
  }

> ideallayout.cpp:204
> +
> +    QSize preferredSize = sizeHint();
> +    bool shrink = rect.height() < preferredSize.height();

Make const, or perhaps avoid intermediate var and use directly in estimation of `shrink`.

> ideallayout.cpp:205
> +    QSize preferredSize = sizeHint();
> +    bool shrink = rect.height() < preferredSize.height();
> +

const

> ideallayout.cpp:207
> +
> +    int maximumHeight = rect.height() / _items.size(),
> +        surplus,

please explicit type definition for each variable on its own line, const for those which will not be changed later.
Same for rest of code.

> ideallayout.cpp:208
> +    int maximumHeight = rect.height() / _items.size(),
> +        surplus,
> +        shrinkedHeight = -1;

surplus is only used inside the if(shrink), so move declaration of it there,

> ideallayout.cpp:212
> +    if (shrink)
> +    {
> +        int smallItems = 0;

`{` please in same line as `if()`, here and below

> ideallayout.cpp:215
> +        surplus = std::accumulate(_items.begin(), _items.end(), 0, [maximumHeight, &smallItems](int acc, QLayoutItem* item) {
> +            int h = item->sizeHint().height();
> +            if (h <= maximumHeight)

Please tag as const to make clear this will not be changed, use decriptive variable name, not single char:

-> `const int itemHeight`

> ideallayout.cpp:288
> +
> +    for (QLayoutItem *item : _items)
> +    {

{ on same line as for, please.

> idealtoolbutton.cpp:89
>          }
> +
>      }

Linebreak sneaked in?

> idealtoolbutton.cpp:115
> +
> +        option.text = fontMetrics().elidedText(text(), Qt::ElideRight, contentsRect().width() - iconWidth - 4);
> +        painter.drawComplexControl(QStyle::CC_ToolButton, option);

Please document the magic number 4 :)

> idealtoolbutton.cpp:123
> +
> +        QString textToDraw = fontMetrics().elidedText(text(), Qt::ElideRight, contentsRect().height() - iconHeight - 4);
>  

const

REPOSITORY
  R32 KDevelop

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

To: amhndu, #kdevelop
Cc: kossebau, kdevelop-devel, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20180913/b1af283d/attachment-0001.html>