Use QImageReader instead of QSvgRender for XdgIconLoader

[BLumia]

QSvgRender itself only support SVG 1.2 Tiny for rendering so SVGs that more complex might not able to rendered properly. Thus, some DEs like KDE and DDE provides their own Qt icon engine and registered them as for SVG icons, and seems that causes libqtxdg have issues, so #247 was there.

But user or DE might still want to install or provide Qt image formats plugins for better SVG files/icons rendering, using QSvgRender will stop the Qt image formats plugin from being used.

Using QImageReader will still allow us avoiding the usage of Qt icon engines, but kept the ability to make Qt image formats plugin to work properly.

---

This patch originally provided by @zccrs

This patch is intended to address the issue that incorrect SVG icon rendering under Archlinux DDE, and can be tested by installing DDE under Archlinux, then install deepin-icon-theme, deepin-camera and browse the deepin-camera's icon from launcher (the start menu thing). Since that step might be complicated if you don't want to install Archlinux or DDE, you can also https://github.com/BLumia/qt-icon-engine-tool, install deepin-qt5integration, select XdgIconProxyEngine and use the following attachment SVG image for testing (image extracted from the bloom theme provided by deepin-icon-theme):

If the icon is not black then it's working properly :)

[tsujan]

Thanks for the patch!

The purpose of #247 was using QSvgRenderer directly and getting rid of external hassles. QImageReader might do the opposite.

Anyway, at least to me, installing of DDE or deepin-qt5integration for testing isn't an option. If you know of an image-formats plugin that does the job without extra dependencies, please tell us. This should be tested thoroughly regarding extra system resources that it might use.

[BLumia]

The purpose of #247 was using QSvgRenderer directly and getting rid of external hassles. QImageReader might do the opposite.

Well, this patch didn't introduce any extra dependencies, and still avoiding affected by what #247 trying to avoided. It should work exactly the same as before. Just to be clear, deepin-qt5integration won't be a new dependency.

Anyway, at least to me, installing of DDE or deepin-qt5integration for testing isn't an option.

Since this patch is intended to address the incorrect SVG icon rendering issue under DDE (we use libqtxdg as a dependency to provide icon rendering related support), so doing the steps mentioned above can be used to check if the fix actually works. It should not affected LXQt in any form IMO so you can just check if the current SVG rendering stuff still works as intended under LXQt.

If you know of an image-formats plugin that does the job without extra dependencies, please tell us.

DDE provides the SVG imageformats in deepin-qt5integration and it will only be applied when using DDE so...

Still, you don't need them for testing and users won't get affected by them. The default SVG iconformats plugin provided by Qt will still be used in any other cases. You can just check if LXQt is affected if you don't want to install deepin-qt5integration.

[tsujan]

Qt plugins aren't limited to any DE. What I want to check is the probable impact on resource usage when there is a Qt image-formats plugin of that kind.

[BLumia]

Qt plugins aren't limited to any DE.

Yes. So that gives the ability to use supported SVG Qt imageformats plugin. We provided one in deepin-qt5integration and currently we don't know any other Qt imageformats plugin that provided SVG format support. So if you are not too keen to temporary install deepin-qt5integration for testing, then I don't really have any other known plugins to suggest.

By the way, since Qt doesn't provide a way to overwrite the load order of imageformats plugin, the plugin name is important if you decided to write a custom plugin for testing. (ref)

What I want to check is the probable impact on resource usage when there is a Qt image-formats plugin of that kind.

I think the default SVG imageformats plugin (provided by Qt itself) counts and can be used for testing resource usage too. If you apply that patch then it will be used by default, you don't need to do any other step to use the default one :)

If you are using Arch, the default one provided by Qt is in qt5-svg package (or qt6-svg for Qt 6), installed to usr/lib/qt/plugins/imageformats/libqsvg.so.

[tsujan]

I think you didn't get my concern.

By using QSvgRender, we guarantee the lightest possible solution. That's enough for reasonably complex SVG icons too.

Using QImageReader means leaving the job to a Qt image-formats plugin for SVG. Qt's default plugin does it as we do, at most; we've just added a layer to it. However, we can't guarantee that another plugin does the job correctly. That is my concern.

[BLumia]

By using QSvgRender, we guarantee the lightest possible solution. That's enough for reasonably complex SVG icons too.

Well, the reason we at DDE provide our own Qt imageformats plugin for SVG is, QSvgRender simply can't render SVG images properly if it beyond the capability of Qt SVG support. Qt SVG only supports the static features of SVG 1.2 Tiny standard, even Inkscape will easily produce an SVG image that is beyond that spec, which results in incorrect image rendering result. So in our case that's not enough to use QSvgRender. The camera icon is not the only one that cannot render correctly.

However, we can't guarantee that another plugin does the job correctly. That is my concern.

Thanks for clarifying. TBH, I think it's the user's responsibility to install and use any imageformats plugin that can render image correctly. I can update the patch to add a macro or CMake option if needed. 😂

[tsujan]

beyond the capability of Qt SVG support.

That's why I said "reasonably complex SVG icons".

btw I think it's the user's responsibility to install and use any imageformats plugin that can render image correctly.

Yes, but it shouldn't affect a basic thing like icons.

[BLumia]

Yes, but it shouldn't affect a basic thing like icons.

I think users should be able to control the system and make it have better SVG support if they want (by using a different icon engine, or using a different imageformats plugin).

While I'm 100% okay with we have different opinions here, does this mean this patch is not acceptable?

[BLumia]

I can update the patch to add a macro or CMake option if needed.

For example, maybe we can add an environment variable like QTXDG_DONT_FORCE_QTSVGRENDER to make it use QImageReader, and DEs can set this variable for this purpose?

[tsujan]

does this mean this patch is not acceptable?

No, it doesn't. Please don't close it!

I told my opinion and leave it to other LXQt developers.

[tsujan]

I think you and I both misunderstood something ;) I installed deepin-qt5integration from Arch, and this is what I see in the Qt5-based pcmanfm-qt without any patch:

This screenshot shows the difference in (Qt5-based) lximage-qt:

[tsujan]

OK, mystery solved. The above screenshots show thumbnails, not icons. libfm-qtcreates thumbnails by using QImage::loadFromData(), which consults libdsvg instead of libqsvg.

EDIT: My above-mentioned argument still holds.

[zccrs]

Thanks for the patch!

The purpose of #247 was using QSvgRenderer directly and getting rid of external hassles. QImageReader might do the opposite.

Anyway, at least to me, installing of DDE or deepin-qt5integration for testing isn't an option. If you know of an image-formats plugin that does the job without extra dependencies, please tell us. This should be tested thoroughly regarding extra system resources that it might use.

Hi, Before #247, ScalableEntry::pixmap used QIcon to load svg images, QIcon depended on Qt iconengine plugins, I think it is correct not to use QIcon in ScalableEntry::pixmap. But I suggest use QImageReader replace QIcon, although QImageReader will also uses Qt imageformat plugins, but these plugins are different from Qt iconengines, image plugins are heavily used (used by QImage and QPixmap), they will do less work than the iconengine plugins.

[tsujan]

Qt6's QSvgRender has been improved in this regard. This screenshot is taken with Qt 6.7.2 and without deepin-qt6integration:

Although it isn't perfect (and there's a high CPU usage in lximage-qt when scaling the image beyond some size), at least its appearance is much better than what Qt5 provided, especially for icons. More improvements might come.

[BLumia]

Yeah, we are also watching the changes made in the most recent Qt 6 releases and already did some sort of testing. There are still some known issues that need to be addressed like QTBUG-126771, QTBUG-127642(should be fixed in 6.8), QTBUG-128485, QTBUG-127018(should be fixed in 6.7.3) and some others, but anyway as the SVG support keep improving, we might have chance to be able to rely on QSvgRender-only in the future :)