Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash with QskSlider in gallery on Windows #471

Closed
wants to merge 2 commits into from
Closed

Fix crash with QskSlider in gallery on Windows #471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
05d3b15
QskSlider: Do not exceed boundaries
peter-ha Nov 12, 2024
42258fc
QskSlider: Make data field big enough on Windows
peter-ha Nov 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/controls/QskSlider.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class QskSlider::PrivateData
QPointF pressedPos;
qreal pressedValue;
bool tracking : 1;
Qt::Orientation orientation : 2;
Qt::Orientation orientation : 3;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need 3 bits ?

Copy link
Collaborator Author

@peter-ha peter-ha Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah of course - it has to be "uint orientation : 2;"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed the bitfield issues in QskPageIndicator and QskSlider.
The reason for the crash - and how to avoid it best - remains open.

};

QskSlider::QskSlider( QQuickItem* parent )
Expand Down
10 changes: 6 additions & 4 deletions src/controls/QskSliderSkinlet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,15 +136,17 @@ QRectF QskSliderSkinlet::sampleRect(

if( slider->orientation() == Qt::Horizontal )
{
r.setTop( r.center().y() - size.height() / 2 );
const auto y = r.center().y() - size.height() / 2;
r.setTop( qMax( 0.0, y ) );
const auto x = r.left() + slider->valueAsRatio( pos ) * r.width() - size.width() / 2;
r.setLeft( x );
r.setLeft( qMax( 0.0, x ) );
}
else
{
r.setLeft( r.center().x() - size.width() / 2 );
const auto x = r.center().x() - size.width() / 2;
r.setLeft( qMax( 0.0, x ) );
const auto y = r.bottom() - slider->valueAsRatio( pos ) * r.height() - size.height() / 2;
r.setTop( y );
r.setTop( qMax( 0.0, y ) );
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recently fixed a heap corruption from the box renderer: b5c56f7

Maybe this was also the reason for your crash ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

r.setHeight( size.height() );
Expand Down
Loading