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

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D15625#331307 <https://phabricator.kde.org/D15625#331307>, @amhndu wrote:
  
  > Re-implemented Ideal Layout's minimumSize which accumulates the newly added
  >  minimumSize in Ideal Button.
  >  The previous buggy implementation of minimumSize in the Ideal Layout called the doVerticalLayout
  >  with a zero rect, although the height is passed from a property, but it is never set to anything
  >  non-zero, which is what caused problems with some QtCurve configs.
  
  
  Seems the use of setHeight was dropped in R32:0b00f84feb7e2351d8aef0600ab018694d41c030 <https://phabricator.kde.org/R32:0b00f84feb7e2351d8aef0600ab018694d41c030> where the code had been trying to keep the value updated on resizeevents.
  
  I am slighty uneasy with the explosion of code we add now, where ideally we could just delegate things to existing Qt code.
  
  Though I see how the old IdealButtonBarLayout::minimumSize() is surely broken and should be fixed, and that our custom rendering in IdealToolButton should also have a matching IdealToolButton::minimumSizeHint().
  
  Still, the crash as I have understood is roots in `IdealButtonBarLayout::setGeometry(QRect)` being called with some zero rect. Which could happen independently of us providing proper minimumSizeHints, by code that is broken or confused (think custom styles) or otherwise forced to go beyond our hints. The API dox of QLayoutItem::setGeometry(const QRect &r) <http://doc.qt.io/qt-5/qlayoutitem.html#setGeometry> does not forbid to set rects smaller than the minimumSizeHint().
  
  So for the actual crash, IMHO we better guard against 0 width or 0 height in doVerticalLayout() & doHorizontalLayout() in this very patch.
  
  And do the fixing of the minimumSizeHint() as a separate code sanitizing patch, ideally with some unit test to get more future proof.
  
  > I've also checked and it seems that setHeight and height are no longer used anymore in Ideal Layout
  >  or outside, should I go ahead and remove it avoid confusion from a dead property ?
  
  Once no longer used, yes, let's drop dead code. One advantage of KDevelop still is not having to maintain ABI across minor release versions, only keep it stable in the released branch :)
  
  BTW, given you are adding substantial code to the source, you should add your name to the copyright header  of those files :)
  
  Summary: please harden doVerticalLayout() & doHorizontalLayout()  against possible parameters. Do fixing the minimumSizeHint methods in a separate patch.

INLINE COMMENTS

> ideallayout.cpp:97
> +            if (first) {
> +                crossSize = crossSizeHere;
> +                first = false;

While this is inspired by the `sizeHint()` method, I wonder if we should not rather do a `qMax()`over all crossSizes. Just to be safe against someone starting to add a non-IdealToolButton one day. Not that expensive to do.
If agreed, in a later separate commit we could do the same for `sizeHint()`.

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/20180925/fe2cd601/attachment.html>