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

[Friedrich W. H. Kossebau]

kossebau added a comment.


  In D15625#332028 <https://phabricator.kde.org/D15625#332028>, @amhndu wrote:
  
  > > 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) does not forbid to set rects smaller than the minimumSizeHint() or even a 0 size rect.
  >
  > From http://doc.qt.io/qt-5/qlayout.html#details:
  >
  > > You should also implement minimumSize() to ensure your layout isn't resized to zero size if there is too little space.
  >
  > And http://doc.qt.io/qt-5/qlayout.html#SizeConstraint-enum
  >
  > From my understanding, if we set the size constraint in the layout (analogous to how we set it in the tool button, but I missed setting it here in layout) appropriately, Qt classes will follow it. And like the original bug, would resize the window if necessary.
  >  Though I'm not against doing a `rect.width() < minimumSize().width` check in the `doHorizontalLayout`, even if redundant, which should be cheap.
  
  
  While in the initial feature review I was reluctant with checking against incoming values, given the crashes we now saw I might be overcautious now in exchange :) But yes, if it's cheaply doable, let's better be safe than sorry, this is API which can be called by code not under our control, so having a protection layer is better.
  
  >> I am slighty uneasy with the explosion of code we add now, where ideally we could just delegate things to existing Qt code.
  > 
  > If IdealLayout were to inherit from QBoxLayout, we might be able to delegate minimumSize and sizeHint. And maybe even setGeometry.
  
  Oh, inheriting from QBoxLayout seems a good idea. On a quick look at the current code, there seems no special behaviour which isn't also shared with QBoxLayout.
  
  I just did a quick test by replacing IdealButtonBarLayout with a QBoxLayout in IdealButtonBarWidget (-> `m_buttonsLayout = new QBoxLayout(orientation()==Qt::Vertical?QBoxLayout::TopToBottom:QBoxLayout::LeftToRight, this)`) and behaviour was as expected.
  So yes, IMHO inheriting from QBoxLayout is something worth to explore, it could spare lots of code to maintain, and instead we could concentrate on completing IdealToolButton to have a complete implementation as expected by Qt Widgets design.

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/8d891f32/attachment-0001.html>