Don't even try to create icons with empty sizes
ClosedPublic

Authored by apol on Apr 19 2017, 11:17 AM.

Details

Summary

It's not enough to have QSize::isValid, as it needs both width and height
to be 0, so we explicitly check it.
Fixes a warning about writing into an empty image.

Diff Detail

Repository
R302 KIconThemes
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
apol created this revision.Apr 19 2017, 11:17 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 19 2017, 11:17 AM
kfunk added a subscriber: kfunk.Apr 19 2017, 12:13 PM
kfunk added inline comments.
src/kiconengine.cpp
87

size.isEmpty()?

markg requested changes to this revision.Apr 19 2017, 12:19 PM
markg added a subscriber: markg.
markg added inline comments.
src/kiconengine.cpp
87

It took me some minutes to figure this line out. I was "expecting" the isValid call to only return true if with and height are both > 0
So for a QSize(0, 1) i would have expected a false, but it returns true. 0,0 is apparently a valid size.

Then i figured out that isValid is the wrong function for the task you want to do.
You want:

if (size.isEmpty()) {
  // ...
}

From the Qt docs:

The isValid() function determines if a size is valid (a valid size has both width and height greater than or equal to zero). The isEmpty() function returns true if either of the width and height is less than, or equal to, zero, while the isNull() function returns true only if both the width and the height is zero.

And i tested it for you as well:

QSize a(0, 0);
QSize b(0, 1);
QSize c(1, 1);
QSize d(0, -1);

qDebug() << a.isEmpty(); // true
qDebug() << b.isEmpty(); // true
qDebug() << c.isEmpty(); // false
qDebug() << d.isEmpty(); // true

Exactly how i expect it.

This revision now requires changes to proceed.Apr 19 2017, 12:19 PM
apol updated this revision to Diff 13595.Apr 19 2017, 12:33 PM
apol edited edge metadata.

Readability

This revision is now accepted and ready to land.Apr 19 2017, 12:34 PM
This revision was automatically updated to reflect the committed changes.