Review for my svg icon library

SGaist

Hi and welcome to devnet,

No problem with posting such things here.

IIRC, there's currently an effort to modernize a bit the QtSvg module, you might want to take a look there. There might be some stuff you could help doing :-)

That said, for your own code, you should consider using QLoggingCategory. You might also want to handle your error message as error and not warning. It feels a bit counterintuitive to see logError that prints a warning. Also, why make the SvgIcon class a QObject ? It seems to be a bit overkill since it's not using any QObject feature. You also lose the possibility to copy the class.

Hellorge

@SGaist Thank you for your feedback.

QLoggingCategory seems very useful,
SvgIcon is a QObject because next step is to add animations and transitions to the icon and it'll be beneficial to emit signals when things animate or let's say properties like color and size changes. SvgIcon class for now is like a placeholder, possibilities exist that I use QSvgWidget and QGraphicsSvgItem to completely convert a svg file's contents into Qt objects and work with them.

SGaist

You might want to rethink the naming of the classes then. They are a bit too close to their counterpart currently and change significantly in terms of usage if you take for example QIcon.

Hellorge

@SGaist Hello,

I understand your point but personally I don't find naming to be an issue, if possible can you suggest better naming options?

Also, SvgIcon class now inherits QSvgWidget and supports animations for color, size, image-scale and opacity. I have been thinking about seperating animations and heavier stuff into an addon which can be enabled when initiating SvgIconEngine instead of always being present with all SvgIcon instances. This can simply make it less heavier for cases when simple static icons are required without any post generation animation support.

sierdzio

Hi, nice project, good luck!

One small suggestion: if you want others to use it and contribute, you should include a license text in the repository. Without a license, nobody can (legally) use your code.

Some nice license candidates are: MIT license, GPL, LGPL, WTFPL.

Hellorge

@sierdzio Thank you, the repo is now under ~~GPLv3~~ LGPL license.

SGaist

@Hellorge for the class names, I would recommend following the class you are subclassing with maybe your own prefix.

SimonSchroeder

There is currently one problem with using SVG icons in Qt: I have two monitors with different display scaling. Our software uses SVG icons for the QToolButtons in the QToolBar. Moving the main window from one screen to the other rescales everything. We react to the event and then recreate all tool bars so they have the correct pixel size for the SVG. Maybe with what you are doing you can come up with a solution that works automatically, i.e. if the tool button resizes the icon (based on a SVG) also resizes.

Hellorge

@SimonSchroeder
Surely, minor issues like this will be addressed, but right now I'm stuck at converting my Icon class (SvgIcon inherits QSvgWidget) to QIcon, or simply put it can't be used at places expecting a QIcon, I have minor grasp of inheriting QIconEngine and using SvgIcons as source for painting, but I'm not sure about some other better approach or let's say reworking things already set to get a better solution. Any suggestions would be appreciated.

Well a quick update on this:

SvgIconEngine iconEngine(":/icons");
SvgIcon *icon = iconEngine.getIcon("regular", "flag");

QPushButton *btn = new QPushButton(icon->toQIcon(), "");
//toQIcon: QImage -> QPixmap -> QIcon

QObject::connect(icon, &SvgIcon::iconChanged, [=]() {
    btn->setIcon(icon->toQIcon());
    btn->update();
});

with this approach I still face issues like size and opacity stop animating on QPushButton, will look for a better solution.

Christian Ehrlicher

@SimonSchroeder I doubt this library can help here. Can you please provide a minimal, compilable example in a bug report? I can take a look on it. Sounds similar to https://bugreports.qt.io/browse/QTBUG-122403 which needs a QIcon instead a QPixmap to make scaling work for your situation.