Unnecessary check for Qt Svg

[Arfrever]

Qt Svg is not used anywhere in KVIrc. Nothing includes Qt Svg headers. Nothing is linked against libQt5Svg.so.

I suggest to comment out this check in CMakeLists.txt:

option(WANT_QTSVG "Whether to compile Qt SVG support" ON)
...
# Qt SVG support
if(WANT_QTSVG)
        find_package(Qt5Svg)
        if(Qt5Svg_FOUND)
                list(APPEND qt5_kvirc_modules Svg)
                include_directories(${Qt5Svg_INCLUDE_DIRS})
                set(COMPILE_SVG_SUPPORT 1)
        endif()
endif()

And to delete references to libqt5svg5-dev in dist/debian/control and doc/INSTALL.txt.

[un1versal]

not sure if this is valid, you remove this and KVIRc wont be able to work with svg, something we may want in future or currently, pretty sure I saw a fork using svg instead of pngs with no mods except referencing svg instead of png (or Im imagining things) besides with this we can have any svg graphics on scripts and other things.

[wodim]

@un1versal are you drunk?

[Arfrever]

If usage of Qt Svg is introduced in the future, then check for Qt Svg would be simply uncommented.

Checks for e.g. Qt D-Bus and Qt WebKit define COMPILE_DBUS_SUPPORT and COMPILE_WEBKIT_SUPPORT. COMPILE_DBUS_SUPPORT and COMPILE_WEBKIT_SUPPORT are used in some files (e.g. #ifdef COMPILE_DBUS_SUPPORT). But COMPILE_SVG_SUPPORT is not used anywhere.

Presence of check for unused library in CMakeLists.txt might deceive these packagers of KVirc who forget to grep sources of KVIrc (not me).

[un1versal]

@un1versal are you drunk?

Nope... https://github.com/logangorence/kvircx

[wodim]

@un1versal what is that?

[un1versal]

@un1versal what is that?

Are you drunk?

[ctrlaltca]

The cmake rule has been added by @Noldor73 in https://github.com/kvirc/KVIrc/commit/c0c7dfa7c9ef7952d264c4e6eac171c99fcfd226 as an optional component. The library is not used by KVIrc itself, so it should be safe for removal.

[ctrlaltca]

pretty sure I saw a fork using svg instead of pngs with no mods except referencing svg instead of png

And it doesn't work for sure. This is how icon loading wrks in KVIrc currently:

QString KviIconManager::getSmallIconResourceName(SmallIcon eIcon)
{
    QString szName("smallicons:kcs_");
    szName.append(g_szIconNames[eIcon]);
    szName.append(".png");
    return szName;
}

As you can see, the png extension is hardcoded.

[un1versal]

As you can see, the png extension is hardcoded.

Im talking about that kvircx fork which is where I got idea from.

personally I say leave it, it can be used externally by scripts/addons. but if its such a thorn remove it, Im fine with it either way.

[Arfrever]

Just linking against some library (libQt5Svg.so in this case) does not result in using any functionality provided by that library.

[un1versal]

I dont think its that simple, Im not an expert but if you look more deeply than some include you see quite a few references

like https://github.com/kvirc/KVIrc/blob/master/cmake/Doxyfile.cmake#L2190-L2200 theres further mentions of svg in that file (is this used some script or some other stuff??)

then we look at changelog this was added in 2007 and further reworked later, id it probaly was removed since then and this maybe the remnants of the removal. https://github.com/kvirc/KVIrc/blob/master/ChangeLog#L102 https://github.com/kvirc/KVIrc/blob/master/ChangeLog#L610

so I presume its related to https://github.com/kvirc/KVIrc/tree/master/data/icons/scalable and you have scripts in there https://github.com/kvirc/KVIrc/blob/master/data/icons/scalable/createpng.sh

the you have yet more references in https://github.com/kvirc/KVIrc/tree/master/dist to libqt5svg5-dev

in any case, removing this, is not just what you mentioned, you also need to cleanup INSTALL file which refers to libqt5svg5-dev (twice lol) and a bunch of other stuff where this is referred.

travis also refers to it https://github.com/kvirc/KVIrc/blob/master/.travis.yml#L33

removing just the cmake entries is a botched job and theres enough half done jobs in KVIrc so please do this properly whoever want to do this

TL;DR

@Arfrever if you make a PR and cleanup everything related to this, then we can perhaps get feedback from @pragmaware and @HelLViS69 who the ones that worked on this stuff.

thats not even a really decent search but if you want this removed then please no half jobs

[DarthGandalf]

Just linking against some library (libQt5Svg.so in this case) does not result in using any functionality provided by that library.

Usually this is correct. There are some special cases: a library can define _init function (or some other function with __attribute__((constructor))) in which it can register itself to some global variable, e.g. of type std::map<std::string, ImageLoader*>. I don't know whether Qt5 uses such mechanism for images or not.

travis also refers to it https://github.com/kvirc/KVIrc/blob/master/.travis.yml#L33

Of course it does, because otherwise build fails because CMake requires that library... This is what this issue is about.

[un1versal]

Of course it does, because otherwise build fails because CMake requires that library... This is what this issue is about.

yes, my comment was part of a cursory search which that was part of (I was merely pointing to that as something also possibly to remove), removing just the cmake entries would be wrong, as it requires more cleanup elsewhere which is not mentioned by OP.

So this needs to be properly investigated, properly searched and if needs be properly removed not just cmake entries.

I nominate @Arfrever for that job :smile:

[un1versal]

See https://github.com/kvirc/KVIrc/pull/1904

[Arfrever]

(I had mentioned 2 files other than CMakeLists.txt in my initial comment. I probably used * as argument of grep which failed to match .travis.yml...)

[pragmaware]

As far as I can remember SVG support was added together with WebKit support. I guess it's not linked directly to libQtWebKit because webkit can work also without it. So it's kind of optional.. though I don't know if some script uses *.svg somewhere.

[HelLViS69]

The porting from Qt3 to Qt4 was a hard job, tons of stuff broke and we did a lot of hours pursuing bugs. If I remember right, the inclusion of SVG support was a must-have to correctly include Webkit support, so Noldor added that check. Maybe Qt later fixed this behaviour and we didn't notice it, including all stuff for debugging. I think it's better to leave that support in KVIrc as a turned off option

[wodim]

Related to this, I think I remember the check for Qt5Multimedia is optional, but it should be mandatory because we include <QSound> which depends on Qt5Multimedia

[un1versal]

Can anyone reach Noldor? I sent an email re this subject but there's been no reply whatsoever.

[wodim]

Well, you mailed him about a change he made five years ago. I wouldn't expect an answer.

[un1versal]

So shall we remove or leave?

[un1versal]

Can we please vote on this? baring in mind scripts may use it independently

I vote leave it, Hellvisio said leave it, pragma suggested that this can be used by scripts so I imagine vote is to leave it

So thats 3 votes to leave it.

[staticfox]

Let's just leave it. Closing simply because there are no cunning arguments to remove it in the first place other than cleanup.