Differential D19256

[RFC] height or width of FrameSvgItem can be set to a negative number, handle it..
Needs ReviewPublic
Actions

Authored by benjaminrobin on Feb 23 2019, 4:43 PM.

Details

Reviewers
None
Group Reviewers
Plasma
Summary

As explained in the bug report this is maybe not the right fix. It's hide the true problem.
In the case of the bug report, a QQmlJavaScriptExpression generate a value equal to "nan" which
is then converted to an integer, which gives -2147483648

BUG: 403978

Diff Detail

Repository
R242 Plasma Framework (Library)
Lint
Lint Skipped
Unit
Unit Tests Skipped
benjaminrobin created this revision.Feb 23 2019, 4:43 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 23 2019, 4:43 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
benjaminrobin requested review of this revision.Feb 23 2019, 4:43 PM
benjaminrobin added a reviewer: Plasma.
benjaminrobin edited the summary of this revision. (Show Details)Feb 23 2019, 4:48 PM
apol added a subscriber: apol.Feb 23 2019, 11:55 PM

+1 makes sense to me

ngraham added a subscriber: ngraham.Feb 24 2019, 4:07 PM

Why is this number negative though? That seems wrong. The bug report seems to indicate that Qt is returning a garbage value: https://bugreports.qt.io/browse/QTBUG-73691

There's even a patch: https://codereview.qt-project.org/#/c/254226/

Feels like we should close this as well as the bug report and focus on the Qt fix (assuming that's the right approach).

ngraham edited the summary of this revision. (Show Details)Feb 24 2019, 4:07 PM
benjaminrobin added a comment.Feb 24 2019, 4:27 PM
In D19256#418577, @ngraham wrote:

Feels like we should close this as well as the bug report ...

I am agree with you, but this patch still fix 2 very minor issues :

  • resizeFrame(QSize( was used instead of resizeFrame(QSizeF(
  • In the creation of the variable QSize frameSize the width() and height() were implicitly converted from qreal to int (QtCreator is not happy with it)

So I can update this patch to only reflect these 2 minors changes, or forget them...

ngraham added a comment.Feb 24 2019, 8:04 PM

Updating the patch to only do that makes sense, but putting this minimum-value-is-always-zero guard in there too may also make sense just as a safety valve. I'll leave it up to the Plasma folks at this point.

Revision Contents
Changeset List

Diff 52387

View Options

src/declarativeimports/core/framesvgitem.cpp

Loading...