<table><tr><td style="">kossebau added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D15625">View Revision</a></tr></table><br /><div><div><blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D15625#332028" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D15625#332028</a>, <a href="https://phabricator.kde.org/p/amhndu/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@amhndu</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>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.</p></blockquote>
<p>From <a href="http://doc.qt.io/qt-5/qlayout.html#details" class="remarkup-link" target="_blank" rel="noreferrer">http://doc.qt.io/qt-5/qlayout.html#details</a>:</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>You should also implement minimumSize() to ensure your layout isn't resized to zero size if there is too little space.</p></blockquote>
<p>And <a href="http://doc.qt.io/qt-5/qlayout.html#SizeConstraint-enum" class="remarkup-link" target="_blank" rel="noreferrer">http://doc.qt.io/qt-5/qlayout.html#SizeConstraint-enum</a></p>
<p>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.<br />
Though I'm not against doing a <tt style="background: #ebebeb; font-size: 13px;">rect.width() < minimumSize().width</tt> check in the <tt style="background: #ebebeb; font-size: 13px;">doHorizontalLayout</tt>, even if redundant, which should be cheap.</p></div>
</blockquote>
<p>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.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>I am slighty uneasy with the explosion of code we add now, where ideally we could just delegate things to existing Qt code.</p></blockquote>
<p>If IdealLayout were to inherit from QBoxLayout, we might be able to delegate minimumSize and sizeHint. And maybe even setGeometry.</p></blockquote>
<p>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.</p>
<p>I just did a quick test by replacing IdealButtonBarLayout with a QBoxLayout in IdealButtonBarWidget (-> <tt style="background: #ebebeb; font-size: 13px;">m_buttonsLayout = new QBoxLayout(orientation()==Qt::Vertical?QBoxLayout::TopToBottom:QBoxLayout::LeftToRight, this)</tt>) and behaviour was as expected.<br />
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.</p></div></div><br /><div><strong>REPOSITORY</strong><div><div>R32 KDevelop</div></div></div><br /><div><strong>BRANCH</strong><div><div>segfault-fix</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D15625">https://phabricator.kde.org/D15625</a></div></div><br /><div><strong>To: </strong>amhndu, KDevelop, kossebau, rjvbb<br /><strong>Cc: </strong>kossebau, rjvbb, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd<br /></div>