D15625: Sublime: Fix crash caused when all tool view items are small

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Looks good to me in general,  from what I tested works as intended (though I cannot test the crash as before, never had it reproduced)
  Some code tweaking might be nice to have, see inline comments.
  
  When it comes to updating the spacing on style changes, something like this should work:
  
    void IdealButtonBarLayout::changeEvent(QEvent* event)
    {
        QBoxLayout::changeEvent(event);
        if (event->type() == QEvent::StyleChange) {
            setSpacing(buttonSpacing());
        }
    }

INLINE COMMENTS

> ideallayout.cpp:50
>  
> -void IdealButtonBarLayout::setHeight(int height)
> +IdealButtonBarLayout::~IdealButtonBarLayout()
>  {

`IdealButtonBarLayout::~IdealButtonBarLayout() = default;` please

> idealtoolbutton.cpp:100
> +{
> +    QStyleOptionToolButton opt;
> +    initStyleOption(&opt);

We might better be safe and call `ensurePolished();` as well here at the begin.

> idealtoolbutton.cpp:115
> +
> +        return {iconWidth + 1, iconHeight + 1};
> +    } else {

Why the +1 ? To make up for any stuff style adds?

For that it might be better in any case to do at the end of the method some call like

  sizeHint = style()->sizeFromContents(QStyle::CT_ToolButton, &opt, QSize(w, h), this).
  expandedTo(QApplication::globalStrut());

Cmp. also implemenation of QToolButton::sizeHint()  e.g. to be seen at https://code.woboq.org/qt5/qtbase/src/widgets/widgets/qtoolbutton.cpp.html#_ZNK11QToolButton8sizeHintEv

And yes, in a follow-up commit we should also update the `sizeHint()` implementation to Qt5 style API.

> idealtoolbutton.cpp:116
> +        return {iconWidth + 1, iconHeight + 1};
> +    } else {
> +        // if no icon, set an arbitrary minimum size

No `else` after a branch ending in `return`, please.

> idealtoolbutton.cpp:122
> +            return minimumSize;
> +        } else {
> +            return {minimumSize.height(), minimumSize.width()};

No `else` after branch ending in `return`.

> idealtoolbutton.cpp:123
> +        } else {
> +            return {minimumSize.height(), minimumSize.width()};
> +        }

Please `return minimumSize.transposed();`

REPOSITORY
  R32 KDevelop

BRANCH
  segfault-fix

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

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