| kossebau | |
| rjvbb |
| KDevelop |
Fixes regression introduced in D15450 by making IdealButtonBarLayout derive from
QBoxLayout instead of QLayout and thus delete most operations to Qt instead
of manually handling them, removing the bugged implementation.
Added minimumSizeHint in IdealToolButton to prevent it from being resized
to zero.
IdealButtonBarLayout is now a child layout for vertical bars as well,
the top level layout is stretched and allows for context menu.
BUG: 399025
| No Linters Available |
| No Unit Test Coverage |
| Buildable 3240 | |
| Build 3258: arc lint + arc unit |
Please also fix the possibly actual bug, which is
const int maximumWidth = rect.width() / _items.size();
and its counterpart not taking the button spacing into account.
Updated diff solves the buttonSpacing issue, but leaving the check for any future edge-cases I might've missed
Looks good at first glance. The failsafe should work evidently but I'll have to test to be certain if the other fix prevents the situation I ran into. Maybe this afternoon, otherwise tomorrow.
| kdevplatform/sublime/ideallayout.cpp | ||
|---|---|---|
| 68 | Please add a comment that this is protective code against the chance there is some bug in our logic which would lead to divide-by-zero then. Also worth a qCWarning in the else branch, so we could collect info where this happens. Given the calculations above, there should be at least one too-large item. Otherwise we would have a bug in our sizeHint() code or in the what-and-how-to-shrink code above. Or the layouted items would suddenly start to report random sizeHint() values during the execution of this method, which would be even more unexpected. | |
| 68 | Same comment please. | |
Thanks. Patch fine with me, feature still works.
(and now that I know about it I also see the unused modulo pixels from the maxheight calculation ;) something to be solved on a boring day in need for entertainment)
Let's have @rjvbb give this a test as well before pushing, given he experienced the crash.
| kdevplatform/sublime/ideallayout.cpp | ||
|---|---|---|
| 73 | Wait, this should be the inverse: "Expected at least one too-large item" :) | |
Oh yes, sorry about that, they say misleading documentation is worse than no documentation :D
Hi @rjvbb Already got a chance to test this, and give your waited for +1, so it can be shipped?
Just making sure this one has not slipped your attention :)
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) {
| kdevplatform/sublime/ideallayout.cpp | ||
|---|---|---|
| 73 | 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. | |
How exactly is maximumWidth calculated to be 0? Which value triggers that?
Something is broken somewhere, we should uncover this instead of hiding it under undocumented protective code which needs maintainance in the future.
I see no such issues here with QtCurve on openSUSE TW. So please let's first make sure this is not some local issue.
How exactly is maximumWidth calculated to be 0?
How would I know? QtCurve is highly configurable, so it doesn't strike me as impossible that the value is actually correct, esp. since all items compared against it also have a zero width.
Even if it's a local issue it could be outside of our control. I've already run into unexpected situations with fonts, for instance. How do you handle that ... crash and tell the user to use a different font, or provide a protection. That's a question that's not even open to debate IMHO, not if there's a simple and evident fallback like here.
Any chance that the original bug (384711) is somehow related to QTBUG-16252 and its (reverted) fix in Qt 5.9.6?
What are the values of rect.width(), buttonSpacing and _items.size() in this calculation?
const int maximumWidth = (rect.width() - buttonSpacing * (_items.size() - 1)) / _items.size();
Interestingly, I'm getting the same issue on Linux (with the same QtCurve style though a slightly different font selection).
Only here it's maximumHeight that is concerned, not the width.
How ad-hoc is the threshold under which an item is considered small, and how much sense does it make to distinguish small vs. not-small when all items have exacty the same size (that includes the single-item case)?
What are the values of rect.width(), buttonSpacing and _items.size() in this calculation? `const int maximumWidth = (rect.width() - buttonSpacing * (_items.size() - 1)) / _items.size();`
Ah, that.
rect.height= 0 buttonSpacing= 1 _items.size()= 2
so, rect.width() == buttonSpace * (_items.size()-1), something that can happen for more than a few combinations of the variables involved.
It would be more interesting to know why items (and rect) could have 0 width or height, but even that doesn't strike me as illegal in a UI.
Another thought: are you sure the comparison should be itemWidth <= maximumWidth and not a simple < instead? That ought to address the situation I run into, though I still don't believe it'd be a guarantee that there will never be other situations where all items are considered to be small items.
are you sure the comparison should be itemWidth <= maximumWidth and not a simple < instead?
The reasoning was that Items that have maximumWidth size needn't be shrinked, as that's the maximum size that's allowable.
It would be more interesting to know why items (and rect) could have 0 width or height
rect shouldn't be, the (buggy, which should probably be fixed) IdealButtonBarLayout::minimumSize returns something non-zero.
Can you share your QtCurve config ?
Can you guarantee that rect.width (height) will never be equal to the RHS in the equation from my previous reply?
I’ll attach my qtcurve and other ui settings when I get back.
See also https://bugs.kde.org/show_bug.cgi?id=399025
~/.config/qtcurve/stylerc :
~/.config/kdeglobals
icon themes and colour scheme (just in case):
What if you turn that into a "this the minimum size where shrinking becomes necessary"? Without changing the equation that introduces a 1 pixel difference, I'm not certain if you're going to see that?
Here's a screenshot of what my situation with only small items looks like; to me it looks exactly the same as before the introduction of the regression (I never ran into the issue that was fixed by that change):
If I may: can someone please tell me why it should be impossible that only small items are found, IOW why there would also have to be something to shrink?
If I may: can someone please tell me why it should be impossible that only small items are found, IOW why there would also have to be something to shrink?
Ideally, Layout::sizeHint should tell us the total space the buttons would occupy (given that it sums up the button size hints), comparing that to the rect should make it make it possible to predict.
I've been assuming if the buttons were to all fit, rect.height() should be equal to sizeHint().height()
Then I don't understand the question.
I thought it was clear, but let me try to reword:
What proof is there that there must be at least 1 big item (= not small)?
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.
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 ?
Forgot to update one qCWarning into qCDebug.
@rjvbb can you test this with your theme ?
Looks like you nailed it this time.
However, maximumWidth|Height can still be 0 when rect.width()|height() == buttonSpacing * (_items.size() - 1) (and given the values I've seen that doesn't seem outlandish). And that's not even a requirement; if all items have 0 width or height you will still end up without any small items.
In other words, maybe you shouldn't justify the fallback with potential "latent miscalculations" but simply with the observation that you cannot guarantee that there will be at least 1 large item.
Some styling fixes.
However, maximumWidth|Height can still be 0 when rect.width()|height() == buttonSpacing * (_items.size() - 1) (and given the values I've seen that doesn't seem outlandish). And that's not even a requirement; if all items have 0 width or height you will still end up without any small items.
Because of the minimumSize, where we ensure the rect has to be at least the spacing required plus the non-zero minimumSize of the button.
Also need input on the dead height property in Ideal Layout, should I just remove it ?
Nobody, inside our out, seems to be using it.
Seems the use of setHeight was dropped in 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) does not forbid to set rects smaller than the minimumSizeHint() or even a 0 size rect.
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.
| kdevplatform/sublime/ideallayout.cpp | ||
|---|---|---|
| 97 ↗ | (On Diff #42280) | 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. |
I am slighty uneasy with the explosion of code we add now
Seems to me you're the one who asked for it ;)
Does this add anything over the original patch + the fallback/catch-all, aside from a lot of extra code? I don't *see* any differences (in the UI behaviour).
I did not ask for code explosion though. But as you can read by the next lines, I totally understand why the patches has been growing since (just that I now would like to see that minsizehint aspect solve differently, as it only fixes a trigger and does not prevent the general conceptually valid crash chance).
Does this add anything over the original patch + the fallback/catch-all, aside from a lot of extra code? I don't *see* any differences (in the UI behaviour).
Yes, it makes the code more sane and complete and less fragile. I did not see any crashes as well in the UI before, so by that argument this patch here would not have been necessary ;)
Still, the crash as I have understood is roots in IdealButtonBarLayout::setGeometry(QRect) being called with some zero rect.
Do fixing the minimumSizeHint methods in a separate patch.
The crash actually happens because the previous IdealButtonBarLayout::minimumSize() called doVerticalHeight with a zero rect. The height passed in this rect is the height property we talked about, which is never set and is thus zero. Sorry, I may not have been clear enough in my revision-edits / comments.
See: https://cgit.kde.org/kdevelop.git/tree/kdevplatform/sublime/ideallayout.cpp#n81
So, the minimumSize fix needs to be in this patch.
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.
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.
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.
IdealButtonBarLayout is now derived from QBoxLayout and delegates all operations to it.
In IdealButtonBarWidget, added a super-layout for all bars, to allow context
menu to work as previously.
I could not directly add a strecher to the button layout, adding new items keeps ruining
the order.
Looks good to me in general, from what I tested works as intended (though I cannot test the crash as before, never had it reproduced)
Some code tweaking might be nice to have, see inline comments.
When it comes to updating the spacing on style changes, something like this should work:
void IdealButtonBarLayout::changeEvent(QEvent* event)
{
QBoxLayout::changeEvent(event);
if (event->type() == QEvent::StyleChange) {
setSpacing(buttonSpacing());
}
}| kdevplatform/sublime/ideallayout.cpp | ||
|---|---|---|
| 50 | IdealButtonBarLayout::~IdealButtonBarLayout() = default; please | |
| kdevplatform/sublime/idealtoolbutton.cpp | ||
| 100 | We might better be safe and call ensurePolished(); as well here at the begin. | |
| 115 | Why the +1 ? To make up for any stuff style adds? For that it might be better in any case to do at the end of the method some call like sizeHint = style()->sizeFromContents(QStyle::CT_ToolButton, &opt, QSize(w, h), this). expandedTo(QApplication::globalStrut()); Cmp. also implemenation of QToolButton::sizeHint() e.g. to be seen at https://code.woboq.org/qt5/qtbase/src/widgets/widgets/qtoolbutton.cpp.html#_ZNK11QToolButton8sizeHintEv And yes, in a follow-up commit we should also update the sizeHint() implementation to Qt5 style API. | |
| 116 | No else after a branch ending in return, please. | |
| 122 | No else after branch ending in return. | |
| 123 | Please return minimumSize.transposed(); | |
Eek, missed that changeEvent(QEvent *event) is only added for QWidget, not QObject itself. We would need to install an event filter to the parent widget.
| kdevplatform/sublime/ideallayout.cpp | ||
|---|---|---|
| 63 ↗ | (On Diff #42280) | We have an issue here. buttonSpacing() looks for any parentWidget() to take the style from, otherwise defaults to 0. One solution might be to add an explicit property *QWidget* m_styleParentWidget` to |
Pass style parent in the constructor for Ideal Layout, and install event filter
on it to monitor style changes and set spacing.
Besides the two nitpicks, happy with the result, good work. Works here as intended, across multiple styles I tested (also at runtime).
So from my POV, time to land this in the 5.3 branch, the bug hopefully enjoyed its lifetime a bit, may it enjoy some reincarnation then somewhere else and ideally as feature :)
| kdevplatform/sublime/ideallayout.cpp | ||
|---|---|---|
| 275 ↗ | (On Diff #42280) | eventFilter reimplementation best also call the one of the base class, as one never knows if the base classes do not need to do some filtering as well. Might not be the case here currently, but let's be future-proof and base class co-operative by default :) Usually done by doing in the final return statement, that also spares a variable to remember the result (also drops the need for Q_UNUSED()): return QBoxLayout::eventFilter(watched, event); |
| kdevplatform/sublime/ideallayout.h | ||
| 36 ↗ | (On Diff #42426) | Please move the * to the type, while you touch the line/argument :) |