D15450: Make sublime tool bar buttons shrinkable and elide text

[Friedrich W. H. Kossebau]

kossebau accepted this revision.
kossebau added a comment.
This revision is now accepted and ready to land.


  Some more nitpicks, but otherwise happy with this patch.
  So with the nitpicks resolved, I vote for adding this.

INLINE COMMENTS

> ideallayout.cpp:211
> +    if (shrink) {
> +        int smallItems = 0;
> +        const int surplus = std::accumulate(_items.begin(), _items.end(), 0, [maximumHeight, &smallItems](int acc, QLayoutItem* item) {

-> `smallItemCount`, 'count' makes it clear the variable is not a list of items.

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

`int h` -> `const int itemHeight` or `const int h`

> ideallayout.cpp:222
> +        Q_ASSERT(_items.size() != smallItems); // should be true since rect.width != sizeHint.width
> +        shrinkedHeight = maximumHeight + surplus / (_items.size() - smallItems);
> +    }

Please add a comment to what is done here, at least sleepy code reader does not instantly get what is done here:

  // distribute surplus height over large items

> ideallayout.cpp:268
> +    if (shrink) {
> +        int smallItems = 0;
> +        const int surplus = std::accumulate(_items.begin(), _items.end(), 0, [maximumWidth, &smallItems](int acc, QLayoutItem* item) {

smallItemCount

> ideallayout.cpp:270
> +        const int surplus = std::accumulate(_items.begin(), _items.end(), 0, [maximumWidth, &smallItems](int acc, QLayoutItem* item) {
> +            int w = item->sizeHint().width();
> +            if (w <= maximumWidth) {

const

> ideallayout.cpp:282
> +
> +    for (QLayoutItem *item : _items) {
> +        const QSize itemSizeHint = item->sizeHint();

`QLayoutItem *` -> QLayoutItem*`

> idealtoolbutton.cpp:1
> +
>  /*

Accidentally added newline? :)

> idealtoolbutton.cpp:103
>  void IdealToolButton::paintEvent(QPaintEvent *event)
>  {
> +    QStylePainter painter(this);

Now that the argument event is no longer used, please add a line

  Q_UNUSED(event);

so the compiler knows it should not complain.

> idealtoolbutton.cpp:112
> +        if (toolButtonStyle() != Qt::ToolButtonTextOnly && !option.icon.isNull())
> +            iconWidth = option.iconSize.width();
> +

Please wrap the single line body with {}:

  if (toolButtonStyle() != Qt::ToolButtonTextOnly && !option.icon.isNull()) {
      iconWidth = option.iconSize.width();
  }

> idealtoolbutton.cpp:121
> +        int iconHeight = 0;
> +        if (toolButtonStyle() != Qt::ToolButtonTextOnly && !option.icon.isNull())
> +            iconHeight = option.iconSize.height();

Please wrap the single line branch body with {}:

  if (toolButtonStyle() != Qt::ToolButtonTextOnly && !option.icon.isNull()) {
      iconHeight = option.iconSize.height();
  }

> amhndu wrote in idealtoolbutton.cpp:46
> Although I've added this, it doesn't seem to work.

What exactly was the intent with this code? Given the layout code in the IdealButtonBarLayout class does not take the size policy or minimum size into account in the current logic, this would not work.

So unless the button is going to be inserted elsewhere, this added code can be removed again, or?

REPOSITORY
  R32 KDevelop

BRANCH
  buttonbar-shrink

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

To: amhndu, #kdevelop, kossebau
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/20180914/64c9652e/attachment-0001.html>