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.

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

SvgIcon: add new class to work around issues with SVG QIcons in Plasma/KStatusNotifierItem #3359

Merged

Conversation

@davidebeatrici
Copy link
Member

commented Mar 2, 2018

This commit adds new class SvgIcon which allows you to create a QPixmap-backed QIcon
from an SVG.

We need this to fix #3124.

Basically, SVG-backed QIcons returns an empty list for QIcon::availableSizes().
There is some confusion as to whether this is the expected behavior or not.
(Tracked in #3374).

However: Plasma/KStatusNotifierItem currently expects QIcon::availableSizes()
to return a valid list of available sizes. For our SVG icons, that is not the case.

This commit works around this by constructing a QIcon backed by pixmaps, such that
QIcon::availableSizes() returns something sensible.

This is implemented via the static method, SvgIcon::addSvgPixmapsToIcon(), which renders
SVG icon in various common sizes (including 22x22 and 44x44, the Plasma notification area
size, and its @2x size).

@mkrautz
Copy link
Member

left a comment

I am not convinced this is a good workaround.
Do you know which Qt versions this is broken on?
It's obviously not broken on our own Qt 5.6 LTS branch.
Is it also broken on Qt 5.9, which is also LTS? And Qt 5.10, 5.11?
Or is it only 5.7 and 5.8?

If we DO need a workaround for some versions of Qt, I'd be more inclined to refactor the code you removed from MainWindow.cpp:

	QSvgRenderer svg(QLatin1String("skin:mumble.svg"));
	QPixmap original(512,512);
	original.fill(Qt::transparent);

	QPainter painter(&original);
	painter.setRenderHint(QPainter::Antialiasing);
	painter.setRenderHint(QPainter::TextAntialiasing);
	painter.setRenderHint(QPainter::SmoothPixmapTransform);
	painter.setRenderHint(QPainter::HighQualityAntialiasing);
	svg.render(&painter);

	for (int sz=8;sz<=256;sz+=8)
		qiIcon.addPixmap(original.scaled(sz,sz, Qt::IgnoreAspectRatio, Qt::SmoothTransformation));

to create QIcons that include rasterized PNGs for various sizes.

@@ -42,7 +42,7 @@ AboutDialog::AboutDialog(QWidget *p) : QDialog(p) {
QWidget *about = new QWidget(qtwTab);

QLabel *icon = new QLabel(about);
icon->setPixmap(g.mw->qiIcon.pixmap(g.mw->qiIcon.actualSize(QSize(128, 128))));
icon->setPixmap(g.mw->qiIcon.scaled(QSize(128, 128), Qt::KeepAspectRatio, Qt::SmoothTransformation));

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 3, 2018

Member

Seems like an unrelated change? Welcome, but unrelated. Confirm?

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Mar 3, 2018

Author Member

QPixmap doesn't have a pixmap() function.

@@ -70,8 +70,8 @@ class MainWindow : public QMainWindow, public MessageHandler, public Ui::MainWin
QMenu *qmChannel;
QMenu *qmDeveloper;
QMenu *qmTray;
QIcon qiIcon, qiIconMutePushToMute, qiIconMuteSelf, qiIconMuteServer, qiIconDeafSelf, qiIconDeafServer, qiIconMuteSuppressed;
QIcon qiTalkingOn, qiTalkingWhisper, qiTalkingShout, qiTalkingOff;
QPixmap qiIcon, qiIconMutePushToMute, qiIconMuteSelf, qiIconMuteServer, qiIconDeafSelf, qiIconDeafServer, qiIconMuteSuppressed;

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 3, 2018

Member

These should have the qp prefix is they're pixmaps. That, or refactor to use the m_ prefix.

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Mar 3, 2018

Author Member

Right.

QIcon qiIcon, qiIconMutePushToMute, qiIconMuteSelf, qiIconMuteServer, qiIconDeafSelf, qiIconDeafServer, qiIconMuteSuppressed;
QIcon qiTalkingOn, qiTalkingWhisper, qiTalkingShout, qiTalkingOff;
QPixmap qiIcon, qiIconMutePushToMute, qiIconMuteSelf, qiIconMuteServer, qiIconDeafSelf, qiIconDeafServer, qiIconMuteSuppressed;
QPixmap qiTalkingOn, qiTalkingWhisper, qiTalkingShout, qiTalkingOff;

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 3, 2018

Member

Ditto.

for (int sz=8;sz<=256;sz+=8)
qiIcon.addPixmap(original.scaled(sz,sz, Qt::IgnoreAspectRatio, Qt::SmoothTransformation));

qiIcon.load(QLatin1String("skin:mumble.svg"));

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 3, 2018

Member

It doesn't seem to me that the code is equivalent to the code above.
The old code ensures we have high quality scaling across all icon sizes. The new code uses a pixmap of an unknown size (probably based on the baseline size specified in the SVG? who knows?)

This comment has been minimized.

Copy link
@davidebeatrici

davidebeatrici Mar 3, 2018

Author Member

I tested the QPixmap code with and without QSvgRenderer, the results appear to be the same.

Maybe it's worth creating an application to test various cases.

qiTalkingOn.addFile(QLatin1String("skin:talking_on.svg"));
qiTalkingShout.addFile(QLatin1String("skin:talking_alt.svg"));
qiTalkingWhisper.addFile(QLatin1String("skin:talking_whisper.svg"));
qiIconMuteSelf.load(QLatin1String("skin:muted_self.svg"));

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 3, 2018

Member

The old code uses SVG + QIcon to ensure proper vector scaling across different screen sizes, scaling factors, etc.
You're replacing that scaling with a raster icon of an unknown, but pre-determined size (probably based on the baseline size in the SVG?).
I can't say I'm a fan.

@davidebeatrici

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2018

This is broken on Qt 5.7, 5.8 and 5.9.

I don't know about Qt 5.10.

@mkrautz

This comment has been minimized.

Copy link
Member

commented Mar 4, 2018

IMHO, the correct fix for this would be to define a new loading function/class, like:

From SvgIcon.cpp:

/// addSvgPixmapsToIcon renders the SVG file at |fn| in various
/// sizes from 8x8 up to 512x512 and adds the resulting rasterized
/// pixmaps to |icon|.
void SvgIcon::addSvgPixmapsToIcon(QIcon &icon, QString fn) {
	QSvgRenderer svg(fn);
	QPixmap original(512, 512);
	original.fill(Qt::transparent);

	QPainter painter(&original);
	painter.setRenderHint(QPainter::Antialiasing);
	painter.setRenderHint(QPainter::TextAntialiasing);
	painter.setRenderHint(QPainter::SmoothPixmapTransform);
	painter.setRenderHint(QPainter::HighQualityAntialiasing);
	svg.render(&painter);

	for (int size = 8; size <= 256; size += 8) {
		QPixmap scaledPixmap = original.scaled(size, size, Qt::IgnoreAspectRatio, Qt::SmoothTransformation);
		icon.addPixmap(scaledPixmap);
	}
}

In my mind, your current code relies on QPixmap::load rendering a reasonably sized QPixmap from the SVG. We don't know how it determines what size to use. My guess would be that it would use the baseline size specified in the SVG itself. However, that's not documented. Creating a QPixmap from an SVG could produce a 4096x4096 pixmap, or an 8k one. Who knows?

As such, I'd prefer to be explicit. and use a routine tuned for icons like I suggest above.

WDYT?

@davidebeatrici

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2018

I agree.

It's still a workaround instead of a fix, because we don't load the SVG file directly and let QIcon render the image at the exact resolution, but it's definitely better than using QPixmap.

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:qpixmap-qsystemtrayicon branch from 25585ac to 65a33a7 Mar 5, 2018

@davidebeatrici

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2018

Problem:
screenshot_20180305_020632

Loading the SVG file directly leads to the same result.
The only way of showing the image correctly is to use QPixmap.

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:qpixmap-qsystemtrayicon branch from 65a33a7 to 3228f3d Mar 11, 2018

@mkrautz mkrautz changed the title Use QPixmap instead of QIcon for QSystemTrayIcon Add SvgIcon helper class to work around Plasma Kstatusnotifieritem Mar 18, 2018

@mkrautz mkrautz changed the title Add SvgIcon helper class to work around Plasma Kstatusnotifieritem Add SvgIcon helper class to work around Plasma KStatusNotifierItem not rendering SVG-backed QIcons Mar 18, 2018

SvgIcon: add new class to work around issues with SVG QIcons in Plasm…
…a/KStatusNotifierItem

This commit adds new class SvgIcon which allows you to create a QPixmap-backed QIcon
from an SVG.

We need this to fix #3124.

Basically, SVG-backed QIcons returns an empty list for QIcon::availableSizes().
There is some confusion as to whether this is the expected behavior or not.
(Tracked in #3374).

However: Plasma/KStatusNotifierItem currently expects QIcon::availableSizes()
to return a valid list of available sizes. For our SVG icons, that is not the case.

This commit works around this by constructing a QIcon backed by pixmaps, such that
QIcon::availableSizes() returns something sensible.

This is implemented via the static method, SvgIcon::addSvgPixmapsToIcon(), which renders
SVG icon in various common sizes (including 22x22 and 44x44, the Plasma notification area
size, and its @2x size).

@mkrautz mkrautz force-pushed the davidebeatrici:qpixmap-qsystemtrayicon branch from 416c9dc to 004a105 Mar 18, 2018

@mkrautz mkrautz changed the title Add SvgIcon helper class to work around Plasma KStatusNotifierItem not rendering SVG-backed QIcons SvgIcon: add new class to work around issues with SVG QIcons in Plasma/KStatusNotifierItem Mar 18, 2018

@davidebeatrici davidebeatrici merged commit b25db3e into mumble-voip:master Mar 18, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.