<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#331307" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D15625#331307</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);"><p>Re-implemented Ideal Layout's minimumSize which accumulates the newly added<br />
minimumSize in Ideal Button.<br />
The previous buggy implementation of minimumSize in the Ideal Layout called the doVerticalLayout<br />
with a zero rect, although the height is passed from a property, but it is never set to anything<br />
non-zero, which is what caused problems with some QtCurve configs.</p></div>
</blockquote>
<p>Seems the use of setHeight was dropped in <a href="https://phabricator.kde.org/R32:0b00f84feb7e2351d8aef0600ab018694d41c030" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">R32:0b00f84feb7e2351d8aef0600ab018694d41c030</a> where the code had been trying to keep the value updated on resizeevents.</p>
<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>
<p>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().</p>
<p>Still, the crash as I have understood is roots in <tt style="background: #ebebeb; font-size: 13px;">IdealButtonBarLayout::setGeometry(QRect)</tt> 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 <a href="http://doc.qt.io/qt-5/qlayoutitem.html#setGeometry" class="remarkup-link" target="_blank" rel="noreferrer">QLayoutItem::setGeometry(const QRect &r)</a> does not forbid to set rects smaller than the minimumSizeHint().</p>
<p>So for the actual crash, IMHO we better guard against 0 width or 0 height in doVerticalLayout() & doHorizontalLayout() in this very patch.</p>
<p>And do the fixing of the minimumSizeHint() as a separate code sanitizing patch, ideally with some unit test to get more future proof.</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>I've also checked and it seems that setHeight and height are no longer used anymore in Ideal Layout<br />
or outside, should I go ahead and remove it avoid confusion from a dead property ?</p></blockquote>
<p>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 :)</p>
<p>BTW, given you are adding substantial code to the source, you should add your name to the copyright header of those files :)</p>
<p>Summary: please harden doVerticalLayout() & doHorizontalLayout() against possible parameters. Do fixing the minimumSizeHint methods in a separate patch.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D15625#inline-85147">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ideallayout.cpp:97</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span class="n"><span class="bright">m_min</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">=</span></span><span class="bright"> </span><span class="n"><span class="bright">QSize</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span style="color: #601200"><span class="bright">0</span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span style="color: #601200"><span class="bright">0</span></span><span class="bright"></span><span class="p"><span class="bright">);</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span style="color: #aa4000"><span class="bright">for</span></span><span class="bright"> </span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">QLayoutItem</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">*</span></span><span class="bright"> </span><span style="color: #a0a000"><span class="bright">item</span></span><span class="bright"> </span><span class="p"><span class="bright">:</span></span><span class="bright"> </span><span class="n"><span class="bright">_items</span></span><span class="bright"></span><span class="p"><span class="bright">)</span></span><span class="bright"> </span><span class="p"><span class="bright">{</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span class="n"><span class="bright">m_min</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">=</span></span><span class="bright"> </span><span class="n"><span class="bright">m_min</span></span><span class="bright"></span><span class="p"><span class="bright">.</span></span><span class="bright"></span><span class="n"><span class="bright">expandedTo</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">item</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">-></span></span><span class="bright"></span><span class="n"><span class="bright">minimumSize</span></span><span class="bright"></span><span class="p"><span class="bright">())</span>;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"> </span><span style="color: #aa4000"><span class="bright">if</span></span><span class="bright"> </span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">first</span></span><span class="bright"></span><span class="p"><span class="bright">)</span></span><span class="bright"> </span><span class="p"><span class="bright">{</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"> </span><span class="n"><span class="bright">crossSize</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">=</span></span><span class="bright"> </span><span class="n"><span class="bright">crossSizeHere</span></span><span class="bright"></span><span class="p"><span class="bright">;</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="bright"> </span><span class="n"><span class="bright">first</span></span><span class="bright"> </span><span style="color: #aa2211"><span class="bright">=</span></span><span class="bright"> </span><span style="color: #304a96"><span class="bright">false</span></span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">While this is inspired by the <tt style="background: #ebebeb; font-size: 13px;">sizeHint()</tt> method, I wonder if we should not rather do a <tt style="background: #ebebeb; font-size: 13px;">qMax()</tt>over all crossSizes. Just to be safe against someone starting to add a non-IdealToolButton one day. Not that expensive to do.<br />
If agreed, in a later separate commit we could do the same for <tt style="background: #ebebeb; font-size: 13px;">sizeHint()</tt>.</p></div></div></div></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>