search

lxqt / lxqt-qtplugin

LXQt Qt platform integration plugin
https://lxqt.github.io
GNU Lesser General Public License v2.1
24 stars 14 forks source link

Fix icon loading #9

Closed tsujan closed 8 years ago

tsujan commented 8 years ago

Fixes https://github.com/lxde/lxqt/issues/1048.

qiconloader has a bug, that breaks Breeze icon theme. It isn't limited to Breeze and can make trouble with any SVG icon set that has different icons for different sizes.

Here, QIconEngine *createIconEngine() is used in LXQtPlatformTheme, alongside a fixed qiconloader.cpp.

P.S. Breeze-5.4.x has a bug in 48-pix places icons. It's fixed in v5.21.0.

palinek commented 8 years ago

(just reviewing on th phone) But i think this is not a good approach (copy/paste the logic from qt - it will be the third imlementation already). Imo you should submit thepatch directly to the Qt and here we should either link to libqtxdg and use its engine (of ciurse with the needed fix) or move the whole engine from libqtxdg here...

@luis-pereira what do you think?

tsujan commented 8 years ago

Imo you should submit thepatch directly to the Qt and here we should either link to libqtxdg and use its engine

I'm a pragmatist and want the apps to work OK. On the other hand, the file could be manipulated later according to our needs. And this isn't the first time the "copy/paste" method is used in LXQt: qiconloader_p.h was already there, both qiconloader_p.h and qiconloader.cpp are in libqtxdg, and there are several other examples.

tsujan commented 8 years ago

BTW, correct directoryMatchesSize() in libqtxdg too if it's of any use!

luis-pereira commented 8 years ago

@tsujan libqtxdgs QIconLoaderFixed includes several fixes to make it compliant with the standard. My proposal is to create an XdgIconLoader inside within libqtxdg. It has two purposes:

lxqt-qtplugin would be dependent on the libqtxdg package but not on lQt5Xdg, only on XdgIconLoader.

@tsujan @palinek Comments are welcome.

tsujan commented 8 years ago

@luis-pereira Frankly speaking, I was motivated by the challenge of improving UX and removing an ugly bug. I agree about whatever method that might be chosen -- unless someone says that the problem shouldn't be fixed here because it's about a Qt bug.

luis-pereira commented 8 years ago

@tsujan IMO we are going to solve it also here. It has been done several times, that's why we have QIconLoaderFixed and XdgIcon. That doesn't invalidate the bug/fix being reported to the Qt guys.

tsujan commented 8 years ago

@luis-pereira Great! You and @palinek are masters here. I'll act as a tester :)

luis-pereira commented 8 years ago

@tsujan your patch would be applied in XdgIconLoader.

palinek commented 8 years ago

My proposal is to create an XdgIconLoader inside within libqtxdg. It has two purposes:

  • be used by XdgIcon
  • be used by lxqt-qtplugin

That's exacly what I meant. I just wasn't sure if it is feasible to make a separate "libxdgiconloader" library or move the logic directly into lxqt-qtplugin. Sure with the "split" we will provide the logic for use by any interested 3rd party.

luis-pereira commented 8 years ago

@palinek I'm already working on it. Maybe tomorrow we have something to test.

luis-pereira commented 8 years ago

Just made it work. Created an Qt5XdgIconLoader library in the Qt5Xdg package. lxqt-qtplugin links to Qt5XdgIconLoader. Every QIcon uses our IconLoader. Needs some cleanups before PR's.

PCMan commented 8 years ago

@luis-pereira Cool! Does this mean that the icon loader part becomes its own library living outside libqtxdg? If that is the case, all Qt programs requiring the fix can use it whether they require other xdg stuff or not.

luis-pereira commented 8 years ago

@tsujan Your patch should be applied at libqtxdg/xdgiconloader after https://github.com/lxde/libqtxdg/pull/84 and https://github.com/lxde/lxqt-qtplugin/pull/10

palinek commented 8 years ago

Cool! Does this mean that the icon loader part becomes its own library living outside libqtxdg? If that is the case, all Qt programs requiring the fix can use it whether they require other xdg stuff or not.

Yes, that's the case.... and if our qt platform theme plugin (lxqt-qtplugin) is used (which is the case for LXQt session), then every qt app gets the xdg iconloader engine for free (no need for any change in the code).

luis-pereira commented 8 years ago

@PCMan Sorry I forgot to reply.

palinek commented 8 years ago

We should close this and make the needed patch in libqtxdg.

@tsujan is the only change the one stated in https://github.com/lxde/libqtxdg/pull/84#issuecomment-220385769 or did you make any other changes as well? Will you push/PR the needed changes to libqtxdg or should we take care?

tsujan commented 8 years ago

is the only change the one stated in lxde/libqtxdg#84 (comment) or did you make any other changes as well?

As far as I remember, that change was enough; it corrected a typo.

Will you push/PR the needed changes to libqtxdg or should we take care?

Please do it yourselves!

palinek commented 8 years ago

done in lxde/libqtxdg@2a3833659d437d322574cad41874994da05113e3

tsujan commented 8 years ago

@palinek Did you also tell Qt devs?

palinek commented 8 years ago

Did you also tell Qt devs?

Not yet...

palinek commented 8 years ago

Did you also tell Qt devs?

Not yet...

FYI https://codereview.qt-project.org/#/c/159800/

tsujan commented 8 years ago

FYI https://codereview.qt-project.org/#/c/159800/

I should learn their method of PR -- there are a few other bugs that could be fixed easily -- but testing would be out of question ;)

palinek commented 8 years ago

I should learn their method of PR

Actualy, I'm also a completely newbie on that. But after the initial setup (following the wiki) it is quite easy to submit/push the commits for gerrit review.