Fix crash with QskSlider in gallery on Windows #471
Conversation
| @@ -32,7 +32,7 @@ class QskSlider::PrivateData | |||
| QPointF pressedPos; | |||
| qreal pressedValue; | |||
| bool tracking : 1; | |||
| Qt::Orientation orientation : 2; | |||
| Qt::Orientation orientation : 3; | |||
There was a problem hiding this comment.
First, Qt::Horizontal and Qt::Vertical are defined as 1 and 2, not 0 and 1.
Then enums in Visual Studio C++ apparently are signed (as opposed to with gcc and clang), which means that Qt::Vertical (10) is interpreted as -2 and not 2.
So the following code on Windows:
QskSlider s(Qt::Vertical);
qDebug() << s.orientation();
... produces this output:
Qt::Orientation(-2)
In light of this I am not sure using bit fields for enumerations is worth it at all...
There was a problem hiding this comment.
Ah of course - it has to be "uint orientation : 2;"
There was a problem hiding this comment.
Yeah, but shouldn't we keep the type safety? We would have to always convert the types then:
C:\Users\peter\Documents\dev\qskinny\src\controls\QskSlider.cpp(80): error C2664: 'void QskSlider::orientationChanged(Qt::Orientation)': cannot convert argument 1 from 'uint' to 'Qt::Orientation'
C:\Users\peter\Documents\dev\qskinny\src\controls\QskSlider.cpp(80): note: Conversion to enumeration type requires an explicit cast (static_cast, C-style cast or parenthesized function-style cast)
There was a problem hiding this comment.
I agree, that this optimization is not that important - when not having more than one bit field it is even of no value. However it only requires one cast in the getter and isn't hard to achieve.
I noticed, that we have the same problem in QskPageIndicator.
There was a problem hiding this comment.
I fixed the bitfield issues in QskPageIndicator and QskSlider.
The reason for the crash - and how to avoid it best - remains open.
| const auto y = r.bottom() - slider->valueAsRatio( pos ) * r.height() - size.height() / 2; | ||
| r.setTop( y ); | ||
| r.setTop( qMax( 0.0, y ) ); |
There was a problem hiding this comment.
From the title of this pull request I guess it is about preventing a situation, where the application crashes. Wouldn't it be better to add defensive check where the crash happens instead of avoiding running into that code path ?
There was a problem hiding this comment.
Yeah apparently the crash happens when y is < 0, which is calculated in the line above.
This is triggered by the orientation being neither Vertical nor Horizontal, hence the size hint is 0. In theory it can still happen that y is < 0 with layouting etc; do you know how a defensive check could look like?
There was a problem hiding this comment.
There should be no crash only because assigning a value < 0 to a local variable. Guess the crash is because of processing the calculated rectangle somewhere else. Then the defensive check would be against the questionable rectangle, where the crash happens.
Actually it might be possible, that a scene graph ends up being outside of the bounding rectangle of the item - f.e when not having a proper layout code. Another hypothetical situation I can imagine is when working with transformations.
There was a problem hiding this comment.
So this seems to be more complicated than anticipated: The cause is not just the negative coordinate, but there is a heap corruption that happens only with negative coordinates and the width being the double of the coordinate and a box shape size of 100%, and this only happens on Windows.
Will investigate...
There was a problem hiding this comment.
I recently fixed a heap corruption from the box renderer: b5c56f7
Maybe this was also the reason for your crash ?
There was a problem hiding this comment.
I recently fixed a heap corruption from the box renderer: b5c56f7
Maybe this was also the reason for your crash ?
Yes, it turns out with this patch the crash does not appear anymore.
No description provided.