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

Use QSvgRenderer for SVG icons #247

Merged
merged 3 commits into from
Mar 24, 2021
Merged

Use QSvgRenderer for SVG icons #247

merged 3 commits into from
Mar 24, 2021

Conversation

tsujan
Copy link
Member

@tsujan tsujan commented Mar 18, 2021

QSvgRenderer is used with a cache for both ordinary and colorized SVG icons. In this way, LXQt's icon handling cannot be broken by intruding icon engines, which register themselves for "svg" (see #246). Moreover, it does not depend on a specific code structure of qtsvg or any other icon engine.

`QSvgRenderer` is used with a cache for both ordinary and colorized SVG icons. In this way, LXQt's icon handling cannot be broken by intruding icon engines, which register themselves for "svg" (see #246). Moreover, it does not depend on a specific code structure of `qtsvg` or any other icon engine.
Copy link
Contributor

@palinek palinek left a comment

Choose a reason for hiding this comment

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

Perfect... get rid of relying on private undocumented internals is a good way to go.

}
hCol = pal.highlight().color().name();
}
QString key = QLatin1String("lxqt_")
Copy link
Contributor

Choose a reason for hiding this comment

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

Just suggestion... wouldn't it be better to to use the same key structure for both ScalableFollowsColorEntry & ScalableEntry? Or be the one at least prefix of the extended?

Copy link
Member Author

Choose a reason for hiding this comment

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

The only difference is that, in ScalableFollowsColorEntry, we need three colors (not the whole palette) but we don't need them in ScalableEntry.

BTW, what do you think about increasing QPixmapCache limit? I didn't think that was needed but wanted to ask your opinion.

Copy link
Contributor

@palinek palinek Mar 19, 2021

Choose a reason for hiding this comment

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

but we don't need them in ScalableEntry.

Sure... but then wouldn't it be better to have the same fixed-width prefix... like:

      QString key = QLatin1String("lxqt_")
                    % filename
                    % HexString<int>(mode)
                    % HexString<int>(state)
                    % HexString<int>(size.width())
                    % HexString<int>(size.height())
                    % txtCol % bgCol % hCol;

..and can't the concatenation of *Col allow a collision?

what do you think about increasing QPixmapCache limit?

Never looked into that. But if we need to increase...then I would vote for allowing it to be configurable.

Copy link
Member Author

Choose a reason for hiding this comment

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

but then wouldn't it be better to have the same fixed-width prefix... like:

No objection from me if you prefer it. No collision will happen either. Will do it soon.

But if we need to increase...then I would vote for allowing it to be configurable

I agree. Haven't found a real use case yet; just an idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done; made the cache keys similar to each other.

Copy link
Contributor

@palinek palinek left a comment

Choose a reason for hiding this comment

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

Code review + simple test. Working as expected.

@tsujan tsujan merged commit 07bb453 into master Mar 24, 2021
@tsujan tsujan deleted the use_svg_renderer branch March 24, 2021 09:54
BLumia added a commit to BLumia/libqtxdg that referenced this pull request Feb 25, 2023
QSvgRender itself only support SVG 1.2 Tiny for rendering so SVGs
that more complex might not able to rendered properly. Thus, some
DE 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
lxqtGH-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 kepts the ability to make Qt image formats plugin to
work properly.

This patch originally provided by @zccrs
kegechen added a commit to kegechen/libqtxdg that referenced this pull request Dec 19, 2023
From: lxqt/libqtxdg#247
Use QImageReader instead of QSvgRender for XdgIconLoader
QSvgRender itself only support SVG 1.2 Tiny for rendering so SVGs
that more complex might not able to rendered properly. Thus, some
DE 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
lxqt/libqtxdg/pull/246 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 kepts the ability to make Qt image formats plugin to
work properly.

This patch originally provided by @zccrs

Issue: linuxdeepin/developer-center#6480
deepin-ci-robot pushed a commit to deepin-community/libqtxdg that referenced this pull request Dec 19, 2023
From: lxqt/libqtxdg#247
Use QImageReader instead of QSvgRender for XdgIconLoader
QSvgRender itself only support SVG 1.2 Tiny for rendering so SVGs
that more complex might not able to rendered properly. Thus, some
DE 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
lxqt/libqtxdg/pull/246 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 kepts the ability to make Qt image formats plugin to
work properly.

This patch originally provided by @zccrs

Issue: linuxdeepin/developer-center#6480
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants