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

[René J.V. Bertin]

rjvbb requested changes to this revision.
rjvbb added a comment.
This revision now requires changes to proceed.


  It works and can go in with just one minor change request.
  
  I still need the fallback protection though. I added some additional debug output (see below) which shows what is really going on for me: maximumWidth is calculated to be 0. I haven't tried to figure out if that's an anomaly, but it explains why I'm getting only small items.
  
  This happens only with my QtCurve style; if you want to bite in deeper I can share my settings (and fonts, if needed).
  
    diff --git a/kdevplatform/sublime/ideallayout.cpp b/kdevplatform/sublime/ideallayout.cpp
    index 8bc43479197dfb1b018b780c116034c3f1fc7e69..4157743641fc8024e1d5b8813ccac52b66e71a5c 100644
    --- a/kdevplatform/sublime/ideallayout.cpp
    +++ b/kdevplatform/sublime/ideallayout.cpp
    @@ -22,6 +22,8 @@
     
     #include "ideallayout.h"
     
    +#include <debug.h>
    +
     #include <QStyle>
     #include <QWidget>
     
    @@ -202,25 +204,36 @@ int IdealButtonBarLayout::doVerticalLayout(const QRect &rect, bool updateGeometr
             return x + currentLineWidth + r;
         }
     
    -    const bool shrink = rect.height() < sizeHint().height();
    +    bool shrink = rect.height() < sizeHint().height();
     
    -    const int maximumHeight = rect.height() / _items.size();
    +    // space left per button after removing available space for buttonSpacing
    +    const int maximumHeight = (rect.height() - buttonSpacing * (_items.size() - 1)) / _items.size();
         int shrinkedHeight = -1;
     
         if (shrink) {
             int smallItemCount = 0;
    -        const int surplus = std::accumulate(_items.begin(), _items.end(), 0, [maximumHeight, &smallItemCount](int acc, QLayoutItem* item) {
    +        int minHeight = _items.at(0)->sizeHint().height();
    +        const int surplus = std::accumulate(_items.begin(), _items.end(), 0, [maximumHeight, &smallItemCount, &minHeight](int acc, QLayoutItem* item) {
                 const int itemHeight = item->sizeHint().height();
                 if (itemHeight <= maximumHeight) {
                     acc += maximumHeight - itemHeight;
                     ++smallItemCount;
                 }
    +            if (itemHeight < minHeight) {
    +                minHeight = itemHeight;
    +            }
                 return acc;
             });
     
    -        Q_ASSERT(_items.size() != smallItemCount); // should be true since rect.width != sizeHint.width
    -        // evenly distribute surplus height over large items
    -        shrinkedHeight = maximumHeight + surplus / (_items.size() - smallItemCount);
    +        // Sanity check to prevent against a divide-by-zero due to some latent miscalculations
    +        if (_items.size() != smallItemCount) {
    +            // evenly distribute surplus height over large items
    +            shrinkedHeight = maximumHeight + surplus / (_items.size() - smallItemCount);
    +        } else {
    +            qCDebug(SUBLIME) << "Expected at least one large item, none found, possible erraneous shrink predicate or sizeHint";
    +            qCDebug(SUBLIME) << "\tthreshold height=" << maximumHeight << "min width found:" << minHeight;
    +            shrink = false;
    +        }
         }
     
         for (QLayoutItem* item : _items) {
    @@ -260,25 +273,36 @@ int IdealButtonBarLayout::doHorizontalLayout(const QRect &rect, bool updateGeome
             return y + currentLineHeight + b;
         }
     
    -    const bool shrink = rect.width() < sizeHint().width();
    +    bool shrink = rect.width() < sizeHint().width();
     
    -    const int maximumWidth = rect.width() / _items.size();
    +    // space left per button after removing available space for buttonSpacing
    +    const int maximumWidth = (rect.width() - buttonSpacing * (_items.size() - 1)) / _items.size();
         int shrinkedWidth = -1;
     
         if (shrink) {
             int smallItemCount = 0;
    -        const int surplus = std::accumulate(_items.begin(), _items.end(), 0, [maximumWidth, &smallItemCount](int acc, QLayoutItem* item) {
    +        int minWidth = _items.at(0)->sizeHint().width();
    +        const int surplus = std::accumulate(_items.begin(), _items.end(), 0, [maximumWidth, &smallItemCount, &minWidth](int acc, QLayoutItem* item) {
                 const int itemWidth = item->sizeHint().width();
                 if (itemWidth <= maximumWidth) {
                     acc += maximumWidth - itemWidth;
                     ++smallItemCount;
                 }
    +            if (itemWidth < minWidth) {
    +                minWidth = itemWidth;
    +            }
                 return acc;
             });
     
    -        Q_ASSERT(_items.size() != smallItemCount); // should be true since rect.width != sizeHint.width
    -        // evenly distribute surplus width on the large items
    -        shrinkedWidth = maximumWidth + surplus / (_items.size() - smallItemCount);
    +        // Sanity check to prevent against a divide-by-zero due to some latent miscalculations
    +        if (_items.size() != smallItemCount) {
    +            // evenly distribute surplus width on the large items
    +            shrinkedWidth = maximumWidth + surplus / (_items.size() - smallItemCount);
    +        } else {
    +            qCDebug(SUBLIME) << "Expected at least one large item, none found, possible erraneous shrink predicate or sizeHint";
    +            qCDebug(SUBLIME) << "\tthreshold width=" << maximumWidth << "min width found:" << minWidth;
    +            shrink = false;
    +        }
         }
     
         for (QLayoutItem* item : _items) {

INLINE COMMENTS

> ideallayout.cpp:293
> +        } else {
> +            qCWarning(SUBLIME) << "Expected at least one large item, none found, possible erraneous shrink predicate or sizeHint";
> +            shrink = false;

Please make this a qCDebug; no need to bother the user with this information because it will be printed really lots of times if the situation arises.

REPOSITORY
  R32 KDevelop

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/20180924/093ee81e/attachment.html>