Client: Linux: Tray icon not taken from current icon theme anymore

[andia89]

Expected behaviour

If icon state-ok exists in icon theme (like for Papirus or Numix icon theme) the icon from the theme should be taken instead of the hardcoded icon. This worked in earlier versions of the owncloud-client but not anymore in 2.3.3

Actual behaviour

The hardcoded icon is used in the tray

Steps to reproduce

1. Install owncloud-client
2. Open it

Client configuration

Client version:
2.3.3

Operating system:
Ubuntu 16.04 LTS

OS language: De

Qt version used by client package (Linux only, see also Settings dialog):
Qt 5.6.2

Client package (From ownCloud or distro) (Linux only):
2.3.3 installed per PPA

Installation path of client:
/opt/ownCloud

EDIT: I guess the problem is that owncloud uses its own Qt libraries /opt/owncloud/qt-5.6.2 which do not detect the current icon theme properly...
Becuase programming wise everything should be taken from the current icon theme
https://github.com/owncloud/client/blob/709aa27031f07ffbc21e0478ae0da220885a4cf3/src/libsync/theme.cpp#L132

[hodyroff]

Hm, interesting one from a CI perspective. The monochrome icon is not good enough for you?
It is kind of by purpose that we like to install a try icon which looks like ownCloud and is the official icon by us ...
@jnweiger

[jnweiger]

Testing on Mint 18.1 switching to the "HighContrast" icon theme, owncloud-client does not pick up the theme. But the behaviour is the same with version-2.3.2 using system qt-5.5.1 and with version 2.3.3 using our own qt-5.6.2

Can you tell, if owncloud-client picked up system themes in the past?

[andia89]

Yes I can confirm that it works with the version from the official Ubuntu Repo. Owncloud version 2.1.1. Third icon from the right. Arguably looks much more consistent than the shipped monochrome icon or even worse the colourful one.

@jnweiger: Are you sure the icons exist in HighContrast (state-ok for example). For me there are none of these icons present in the HighContrast icon theme. (Numix, Papirus or Breeze do have them though)

[ogoffart]

@jnweiger Did you not explicitly disable that line in a patch? (in client/src/libsync/theme.cpp )

[Foggalong]

@hodyroff It's not so much that it's not good enough, it's that if someone is using a theme it's expected that it also look there for ownCloud branded tray icons. For example, a theme may provide smaller icons than standard, or in a slightly different art style, or in a different colour (i.e. a black tray icon for light panels) which just relying on the inbuilt tray icon won't do.

You can still include your official tray icon as a fallback if a theme in use doesn't cover it (most themes won't unless it's requested by their users), but it allows the user to make sure that ownCloud properly fits into their system style if they so wish. For example at the moment my ownCloud tray icon sticks out like a sore thumb in the panel because it's both slightly bigger than the other icons and a different shade of white.

[ogoffart]

I bet the problem is because of the patch that changes the client/src/libsync/theme.cpp to disable the theme icons.

(the no_icon_substitution.diff )

[andia89]

Yes it seems so. Can we change that back?

[michaelstingl]

@jnweiger Please make sure branded clients always use branding icons. Unbranded ones could use the theme that's activated on the system.

[Foggalong]

@michaelstingl @jnweiger Not gonna lie that yeah having the ability to theme generally is a nice bonus, but it feels a bit short-sighted (no pun intended) to ignore the accessibility issues. As the examples above, a user might need to use a high contrast theme or otherwise smaller/larger than standard icons.

[michaelstingl]

@Foggalong good point – thanks a lot for your feedback!

[jnweiger]

@ogoffart you loose your bet as this issue is about Ubuntu. The no_icon_substitution.diff is only applied with openSUSE 42.2 and up.

[guruz]

This needs to be handled by @jnweiger @michaelstingl @dschmidt

[ogoffart]

After a long investigation i found out what the problem is.

Ubuntu comes with a theme plugin called appmenu-qt5, and that's this plugin which sets the right icon theme (http://bazaar.launchpad.net/~indicator-applet-developers/appmenu-qt5/trunk/view/head:/src/appmenuplatformmenubar.cpp#L282)

In Qt, normally, it is the GTK3 theme that is supposed to do the same (https://code.woboq.org/qt5/qtbase/src/plugins/platformthemes/gtk3/qgtk3theme.cpp.html#133)

However, the gtk3 style is not bundled with the ubnuty package. But that's only bundled with Qt since Qt 5.7.

I see the following solutions:

1. Upgrade our self compiled to Qt 5.9
2. build the appmenu-qt5 plugin for our qt, and ship it.
3. make a workaround in the code to do QIcon::setThemeName(gtkSetting("gtk-icon-theme-name") when using qt < 5.7 and it is a gtk based desktop. (but that implies linking against gtk+3)

[michaelstingl]

@jnweiger how do you like solution 2. ? (build the appmenu-qt5 plugin for our qt, and ship it.)

[guruz]

Solution 2 would be nice as it's not blocking the 2.4 release since we can just update the Linux packages.

[ogoffart]

Note that on Ubuntu 16.4, one can work around the missing plugin simply by doing

ln -s /usr/lib/x86_64-linux-gnu/qt5/plugins/platformthemes/ /opt/ownCloud/qt-5.6.2/lib/x86_64-linux-gnu/qt5/plugins/

That should works fine if you use a png theme.

But actually, most gnome theme are actually SVG theme, and QtSvg is not included.
So we also need to compile QtSvg to support SVG icon themes.

[guruz]

@michaelstingl @jnweiger I'm tempted to move this to 2.4.1 so we can get 2.4.0 out before christmas.

[ogoffart]

@jnweiger any progress? (QtSvg needs to be included in the packages)

[jnweiger]

The installs currently say
Recommended packages: ocqt562+240-libqt5svg5
so, we have the package, but we don't depend on it, we only recommend that.

Sorry, I lost track of this. Thanks for pinging again.

https://build.opensuse.org/package/show/isv:ownCloud:Qt562:2.4.0/ocqt562+240-qt5-qtsvg

[jnweiger]

@ogoffart How do we make use of the QtSvg lib?

Just add it as a runtime dependency to the 2.5.0 packages, or should we add it as a build time dependency and switch on some configure items?

[ogoffart]

It is a runtime dependency to be able to display the svg icon from the theme.

[jnweiger]

@ogoffart The new daily builds in https://software.opensuse.org//download.html?project=isv%3AownCloud%3Adesktop%3Adaily%3Amaster&package=owncloud-client installs
/opt/ownCloud/qt-5.10.0/lib/x86_64-linux-gnu/libQt5Svg.so.5
/opt/ownCloud/qt-5.10.0/lib/x86_64-linux-gnu/libQt5Svg.so.5.10
/opt/ownCloud/qt-5.10.0/lib/x86_64-linux-gnu/libQt5Svg.so
via the new ocqt5100-qtsvg5 package. But still
ldd /usr/bin/owncloud | grep -i svg
reports nothing.

-- Do you have a test case to see if this is sufficient?

[ogoffart]

@jnweiger In a Ubuntu 16.4 VM, change the config for a theme which has the state-ok image, and make sure it has the right image in the system tray.
In addition, you might need to do the ln command i mention earlier.

Edit: this needs to be a svg theme, to test that the svg plugin gets loaded

[jnweiger]

I am pretty sure, that build would work with such a test. Just found that it loads
testpilot 12228 testy mem REG 8,8 359160 22256091 /opt/ownCloud/qt-5.10.0/lib/x86_64-linux-gnu/libQt5Svg.so.5.10.0
I consider my svg part done.

[guruz]

Assuming this works. @Foggalong and @wa4557 can test with 2.4.1 to be released in the next weeks.

[andia89]

@guruz with 2.5.0daily there is no difference to before, running Numix (an svg theme) still not the theme icons are taken

[jnweiger]

I have another change in the DEB-packaging, the -*qtsvg5-dev package was not correctly separated from the main *qtsvg5 package resulting in uninstallable branded clients.
I am fixing this at the root of the issue, which means, that all my qtsvg packages are currently being rebuilt, any version any branch. This should also affect the 2.4.1-rc1

Reassigning back to @ogoffart : what next?
( Hope all this packaging hassle is actually related to the issue in the end)

[guruz]

This should also affect the 2.4.1-rc1

That's good because we didn't announce it yet :)

[ogoffart]

normally, 2.5 should come with Qt 5.10

(We really should not ship our own Qt when the system's Qt version is higher than the one we should ship)