lxqt / libqtxdg

Qt implementation of freedesktop.org xdg specs
https://lxqt.github.io
GNU Lesser General Public License v2.1
72 stars 35 forks source link

Status Notifier not showing correct icons having dash in their names #260

Closed ahsand97 closed 3 years ago

ahsand97 commented 3 years ago
Expected Behavior

All applications that use appindicator should display their icon correctly in the panel.

Current Behavior

Some applications don't display their appindicator icon, it just shows the generic application-x-executable icon, Remmina is one of them, if I install Remmina normally and run it, it doesn't show the appindicator icon but it does display the icon correctly with the package Remmina-appindicator which is a Remmina version compiled with its own libappindicator as far as I know. I'm developing some apps in Python and Java with GTK3 and have tested on both languages libraries that creates appindicators and same thing happens, no icon showing just the generic application-x-executable.

Remmina package: (it shows the generic application-x-executable icon for the appindicator) image

Remmina-appindicator package: (it shows correctly its icon for the appindicator) image

JappIndicator library to create AppIndicators in Java: (I've tested this on XFCE and GNOME and both works perfectly) image

SystemTray library to create AppIndicators in Java: (tested on both XFCE and GNOME too and it works) image

The only log I see when I run lxqt-panel from console and run those apps is this (don't really know if packages are missing):

Systray started
()
Error on DBus request(org.kde.StatusNotifierItem-1207-2,/StatusNotifierItem): QDBusError(org.freedesktop.DBus.Error.UnknownProperty, Property org.kde.StatusNotifierItem.IconThemePath was not found in object /StatusNotifierItem)
Error on DBus request(org.kde.StatusNotifierItem-1208-1,/StatusNotifierItem): QDBusError(org.freedesktop.DBus.Error.UnknownProperty, Property org.kde.StatusNotifierItem.IconThemePath was not found in object /StatusNotifierItem)
Error on DBus request(:1.66,/org/ayatana/NotificationItem/example_simple_client): QDBusError(org.freedesktop.DBus.Error.InvalidArgs, No existe la propiedad «ToolTip»)
Error on DBus request(:1.66,/org/ayatana/NotificationItem/example_simple_client): QDBusError(org.freedesktop.DBus.Error.InvalidArgs, No existe la propiedad «OverlayIconName»)
Error on DBus request(:1.66,/org/ayatana/NotificationItem/example_simple_client): QDBusError(org.freedesktop.DBus.Error.InvalidArgs, No existe la propiedad «OverlayIconPixmap»)
Possible Solution
Steps to Reproduce (for bugs)
  1. Install Remmina or use this libraries for java to create appindicators (JappIndicator or SystemTray)
  2. Run Remmina or the test examples given by those libraries
  3. The apps run perfectly but their appindicator in the panel shows the generic application-x-executable and not the desire icon.
Context

This is really annoying cause I really like appindicators over systray icons and since I develop desktop software I really wanted to use appindicators on lxqt but for some reason no matter what language or library I use same thing happens, it doesn't show the chosen icon.

System Information
ahsand97 commented 3 years ago

@slidinghotdog, thanks for looking into it!

tested a bit more and this looks like a Qt bug: QIcon::hasThemeIcon("org.remmina.Remmina-symbolic") is false

Here, it's true, as it should be. If you're sure that it's false on your system when the icon is really installed and the icon cache is updated, then we might be able to narrow it down by comparing our systems (not an easy task though).

Hey I did some more debug and it looks like the solutions given by @slidinghotdog work but for other appindicators still showing generic icon. I did some prints here and there and it looks like the apps that aren't showing the appindicator are not following KDE's specification but XDG's in the StatusNotifierItem object, so, the property "IconThemePath" since is only KDE's then for appindicators not following that specification its value is empty, while the path for the icon is in the property "org.freedesktop.StatusNotifierItem.IconName". I see that in the code here is only looking for property "IconThemePath" so if I'm not wrong that means that every appIndicator that don't follow KDE's StatusNofitierItem specification is not gonna have the icon (?)

elviosak commented 3 years ago

@slidinghotdog, thanks for looking into it!

tested a bit more and this looks like a Qt bug: QIcon::hasThemeIcon("org.remmina.Remmina-symbolic") is false

Here, it's true, as it should be. If you're sure that it's false on your system when the icon is really installed and the icon cache is updated, then we might be able to narrow it down by comparing our systems (not an easy task though).

this probably depends on theme used, so adwaita has the same results as yours, papirus does not. Also it's not a qt bug, QIcon::fromTheme includes fallbackSearchPaths() while hasThemeIcon does not.

so i think the issue here is (if you DO have issues):

so changing QIcon::hasThemeIcon to QIcon::fromTheme and checking icon null would fix the first, or checking for different extensions (on the following if blocks that check for a png file)would fix the second.

tsujan commented 3 years ago

this probably depends on theme used

But it shouldn't. All icon themes should inherit hicolor and the icon cache should be automatically updated whenever an icon is added by installation. Doesn't Papirus inherit hicolor? (I don't have Papirus.) If it doesn't, the issue should be reported to its developers. We could also make sure that our icon engine searches in hicolor, regardless of icon themes.

elviosak commented 3 years ago

I see that in the code here is only looking for property "IconThemePath" so if I'm not wrong that means that every appIndicator that don't follow KDE's StatusNofitierItem specification is not gonna have the icon (?)

i don't think so, IconThemePath is just used as a fallback if we can't find from the default paths.

ahsand97 commented 3 years ago

I see that in the code here is only looking for property "IconThemePath" so if I'm not wrong that means that every appIndicator that don't follow KDE's StatusNofitierItem specification is not gonna have the icon (?)

i don't think so, IconThemePath is just used as a fallback if we can't find from the default paths.

IconThemePath is used as the path for the icon (the fallback is the "application-x-executable" icon here) if I'm not wrong (I'm not an LXQT developer) but at least that's what I see so apps not following KDE's specifitacion are not gonna have that property but I see that printing the property IconName (which is XDG's) it has the correct path for the icon for those apps.

I mean I'm not sure but I guess it is that cuz with your solutions I still see generic icons

elviosak commented 3 years ago

Doesn't Papirus inherit hicolor?

it does Inherits=breeze-dark,hicolor on Papirus-Dark/index.theme

checking with with Qt QIcon::themeName() returns "Papirus-Dark" QIcon::fallbackThemeName() returns hicolor

tsujan commented 3 years ago

it does

Yes, I also checked its Arch package.

A daft question: Do you have gtk-update-icon-cache?

elviosak commented 3 years ago

I see that in the code here is only looking for property "IconThemePath" so if I'm not wrong that means that every appIndicator that don't follow KDE's StatusNofitierItem specification is not gonna have the icon (?)

i don't think so, IconThemePath is just used as a fallback if we can't find from the default paths.

IconThemePath is used as the path for the icon (the fallback is the "application-x-executable" here) if I'm not wrong (I'm not an LXQT developer) but at least that's what I see so apps not following KDE's specifitacion are not gonna have that property but I see that printing the property IconName (which is XDG's) it has the correct path for the icon for those apps.

here's where the value from IconThemePathis used, so if we find the icon before that, it's ignored.

elviosak commented 3 years ago

it does

Yes, I also checked its Arch package.

A daft question: Do you have gtk-update-icon-cache?

yes

tsujan commented 3 years ago

@slidinghotdog

QIcon::hasThemeIcon() seems to be just this (→ Qt → qicon.cpp) :

bool QIcon::hasThemeIcon(const QString &name)
{
    QIcon icon = QIcon::fromTheme(name);
    return icon.name() == name;
}
elviosak commented 3 years ago

@slidinghotdog

QIcon::hasThemeIcon() seems to be just this (→ Qt → qicon.cpp) :

bool QIcon::hasThemeIcon(const QString &name)
{
    QIcon icon = QIcon::fromTheme(name);
    return icon.name() == name;
}

huh, it has no mention of it on Qt docs so i assumed them to be different. tested this:

    auto ico = QIcon::fromTheme("org.remmina.Remmina-symbolic");
    qDebug() << "fromTheme is Null:" << ico.isNull() << ico.name() << "hasThemeIcon" << QIcon::hasThemeIcon("org.remmina.Remmina-symbolic");

and it returns fromTheme is Null: false "org.remmina.Remmina" hasThemeIcon false

so the -symbolic is causing the difference in behavior

tsujan commented 3 years ago

tested this:...

Here, it returns:

fromTheme is Null: false "org.remmina.Remmina-symbolic" hasThemeIcon true

OK, I believe you and you believe me. Let's find where this "magic" comes from!

elviosak commented 3 years ago

if we make this change https://github.com/lxqt/lxqt-panel/issues/1623#issuecomment-853505048 it would have the same behavior in both our machines, probably.

elviosak commented 3 years ago

also the themePath search only searches for png, shouldn't svg and xpm also be accepted?

tsujan commented 3 years ago

if we make this change lxqt/lxqt-panel#1623 (comment)...

That would be a workaround. We need to find the root cause, IMO.

Papirus has org.remmina.Remmina. It's the "dash fallback" of org.remmina.Remmina-symbolic (which is found on my system). As far as I remember, I made sure that our icon engine searched for "dash fallbacks" only after searching real names (contrary to what Freedesktop.org wrongly recommended). Now, apparently, that doesn't happen here.

tsujan commented 3 years ago

Haha... I removed dashFallback from libqtxdgxdgiconloader.cppXdgIconLoader::findIconHelper and Remmina's tray icon started to show up with Papirus.

Therefore, my "anti-Freedesktop.org" approach isn't complete; dash fallbacks still cause annoying problems — more annoying than I thought!

elviosak commented 3 years ago

That would be a workaround. We need to find the root cause, IMO.

I think it's also an improvement, since it "normalizes" the behavior from different themes. Also shouild we use XdgIcon::fromTheme instead of QIcon::fromTheme?

elviosak commented 3 years ago

Haha... I removed dashFallback from libqtxdgxdgiconloader.cppXdgIconLoader::findIconHelper and Remmina's tray icon started to show up with Papirus.

Therefore, my "anti-Freedesktop.org" approach isn't complete; dash fallbacks still cause annoying problems — more annoying than I thought!

i didn't know about "dash fallback", so thats why i had some issues with icons with dash in the middle.

tsujan commented 3 years ago

I think it's also an improvement, since it "normalizes" the behavior from different themes.

This is only one example. If we don't eradicate the cause (which isn't in Status Notifier), annoying issues might happen again.

i didn't know about "dash fallback", so thats why i had some issues with icons with dash in the middle.

Yes, dash fallbacks are a curse ;) I've told about their drawbacks here and there (a recent comment of mine: https://github.com/lxqt/libqtxdg/pull/259#issuecomment-845174096). But this report showed me that they can be even worse.

I'm rereading libqtxdg to see what I've missed.

tsujan commented 3 years ago

Found the culprit, al last: https://github.com/lxqt/libqtxdg/blob/750380b4072885c3e8053701440ddabfb24851fc/src/xdgiconloader/xdgiconloader.cpp#L343

That line ruins everything and should be removed. I don't know why it was added in the first place. Will make a PR.

tsujan commented 3 years ago

I reopened and transferred this issue to where it belongs.

ahsand97 commented 3 years ago

You can download this .jar to test and run it with java -jar JAppIndicator.jar

I just wanna let you know that AppIndicators created with libraries or .AppImages (the icons are in the /tmp folder) those StatusNotifierItem in their IconName property there's a full path to the icon but this line adds an additional .png to that property (also it shouldn't look for more icon extensions?).

For example this one its IconName property is: image and the "LOOKING FOR" line is the path that is being used as the path for the icon

Same with AppIndicator displayed by an .AppImage file image

since the code adds that extra .png the path doesn't exist anymore, both cases show generic icon cuz the path to the icon is incorrect.

tsujan commented 3 years ago

@ahsand97, please don't mix different issues in the same report. If your problem with Java isn't fixed after the PR is merged (I'm preparing it), you could open a separate report for it.

ahsand97 commented 3 years ago

@ahsand97, please don't mix different issues in the same report. If your problem with Java isn't fixed after the PR is merged (I'm preparing it), you could open a separate report for it.

The problem is related with the SatusNotifier plugin since it adds an additional .png to the icon path for AppImages and portable things I guess, the ones that use appindicatos (cuz it creates its icons in the /tmp folder) , I know it's not related wih Java at all (I just pointed out that example cuz it's easy to test), you can check what I said with any AppImage that uses appindicators or any library (it doesn't matter if it's java, python).

You keep saying is not the statusnotifier but then why it works on other desktop environments? plus I'm showing in the pictures what I think is the problem....

tsujan commented 3 years ago

I know it's not related wih Java at all...

That makes no difference. Read the recent comments about dash fallbacks. Here, we're dealing with them, and nothing else: one report/PR for each problem.

I know that you couldn't guess it — who could? @slidinghotdog, @stefonarch and I spent a lot of time to find it — but that's the way issues and fixes are organized in LXQt.

ahsand97 commented 3 years ago

I know it's not related wih Java at all...

That makes no difference. Read the recent comments about dash fallbacks. Here, we're dealing with them, and nothing else: one report/PR for each problem.

I know that you couldn't guess it — who could? @slidinghotdog, @stefonarch and I spent a lot of time to find it — but that's the way issues and fixes are organized in LXQt.

I'm pretty sure that the additional .png I mentioned is ruining icons (at least the ones who already have the full icon path in their property IconName), how can I get that fixed?: Here's the proof adding the icon without that extra .png: image all AppIndicators work

Just by adding in here this fixes the problem but I don't know how to ask to be fixed haha

if (themeDir.exists(iconName))
{
    nextIcon.addFile(themeDir.filePath(iconName));
}

should I open another issue?

tsujan commented 3 years ago

The PR is here: https://github.com/lxqt/libqtxdg/pull/261

tsujan commented 3 years ago

should I open another issue?

Yes, please open a separate issue with an appropriate title at https://github.com/lxqt/lxqt-panel/issues. I changed the title of the current issue intentionally.

Just by adding in here this fixes the problem...

It should be investigated. A partial fix may cause new bugs.

stefonarch commented 3 years ago

Compiled #261 and remmina icon is fixed now after reloading panel, but solaar shows still the generic icon. What still doesn't work for both icons is opening the apps by left click.

tsujan commented 3 years ago

What still doesn't work for both icons is opening the apps by left click.

That's related to the app, not Status Notifier.

tsujan commented 3 years ago

@ahsand97, the second issue that you mentioned is fixed by https://github.com/lxqt/lxqt-panel/pull/1624

ahsand97 commented 3 years ago

Compiled #261 and remmina icon is fixed now after reloading panel, but solaar shows still the generic icon. What still doesn't work for both icons is opening the apps by left click.

Solaar's icons are in the folder /usr/share/solaar/icons , all of its icons are .svg files (also Remmina's ubicated in /usr/share/icons/hicolor/apps), the plugin only seems to look for .png files (I'm not sure) but adding this here partially fixes the issue even for remmina having the dash on its icon name, @tsujan could it be that? the lack of looking for .svg files?:

if (themeDir.exists(iconName + QStringLiteral(".svg")))
{
    nextIcon.addFile(themeDir.filePath(iconName + QStringLiteral(".svg")));
}
tsujan commented 3 years ago

@tsujan could it be that? the lack of looking for .svg files?

As I asked earlier, open reports, each for one issue.

tsujan commented 3 years ago

but solaar shows still the generic icon.

@stefonarch, please check https://github.com/lxqt/lxqt-panel/pull/1625 in addition to libqtxdg patch if by generic icon you meant application-x-executable.

stefonarch commented 3 years ago

Yep, that works for solaar icon (it was application-x-executable).

tsujan commented 3 years ago

Yep, that works for solaar icon

Thanks!

ahsand97 commented 3 years ago

Hey @tsujan the remmina icon worked for a while but my lxqt panel got updated by pacman and now I don't see icons for the applications variety and remmina

I pulled the 2 PR u made this and this and recompiled again still isn't working.

I put some prints while running lxqt-panel from console to debug:

solaar (it shows icon) image

image

remmina (it doesn't show icon) image

image

The icon is called remmina-status and is not being found tho it exists: image

variety (it doesn't show icon) image

image

The icon is called variety-indicator also not being found and it seems to exists too: image

tsujan commented 3 years ago

... but my lxqt panel got updated by pacman and now I don't see icons...

Not related to this. The PRs haven't been reviewed and merged; so, they weren't in your updates and later, you should have made some mistakes.

ahsand97 commented 3 years ago

... but my lxqt panel got updated by pacman and now I don't see icons...

Not related to this. The PRs haven't been reviewed and merged; so, they weren't in your updates and later, you should have made some mistakes.

Ok thanks, then I compiled libxdg and lxqt-panel manually with the 2 PRs u made and I still see no icon working for remmina and variety bu it works for solaar (which it wasn't working when I recently opened the issue)

tsujan commented 3 years ago

When a non-merged patch works for you and then it doesn't work after an upgrade, it should be about a local problem/mistake. Please don't complicate this page with those problems; it doesn't help at all.

ahsand97 commented 3 years ago

When a non-merged patch works for you and then it doesn't work after an upgrade, it should be about a local problem/mistake. Please don't complicate this page with those problems; it doesn't help at all.

I'm just tying to help or should I open new issues??? I downloaded the source code of both libxdg and lxqt-panel with your pull requests and after compiling them and install them the appindicators are still not showing. aren't users allowed to keep track of issues??? if I'm doing it is cuz I use LXQT on a daily basis and hate those bugs and those apps show their icon in other DEs it should work for LXQT as well.

tsujan commented 3 years ago

Ranting isn't helping. Wait until the PRs are merged. If, after that, you see a problem, open another issue.

stefonarch commented 3 years ago

I downloaded the source code of both libxdg and lxqt-panel with your pull requests and after compiling them and install them

Did you switch before compiling to the branches git checkout search_hicolor_before_dashfallbacks in libqtxdg and git checkout status_notifier_icon_extension ?

ahsand97 commented 3 years ago

Ranting isn't helping. Wait until the PRs are merged. If, after that, you see a problem, open another issue.

The bug affect me every day that's why I want it to be solved at least for me cuz I know it takes long to have those changes via pacman and I work with remmina everyday.

I downloaded the source code of both libxdg and lxqt-panel with your pull requests and after compiling them and install them

Did you switch before compiling to the branches git checkout search_hicolor_before_dashfallbacks in libqtxdg and git checkout status_notifier_icon_extension ?

Yes I downloaded those branches, compiled them and installed them and the icons weren't working out of the box.

After cloning the branch status_notificer_icon_extension which has the changes made by @tsujan and adding this changes from the branch statusnotifier-xdgicon got the problem solved.

statusnotifierbutton.cpp from the status_notifier_icon_extension branch

if (QIcon::hasThemeIcon(iconName))
    nextIcon = QIcon::fromTheme(iconName);
else

replacing those lines adding the statusnotifier-xdgicon branch changes solves the issue:

nextIcon = XdgIcon::fromTheme(iconName);
if (nextIcon.isNull())

I don't know if both changes are meant to be in a future release but thanks to both of you for helping solving this annoying issue. After compiling it having those changes I see appindicator for remmina, solaar, variety, and my own appindicator which uses a .jpg icon in a custom path for the icon: image

yan12125 commented 3 years ago

I'm not sure if I follow everything here, but seems the only remaining issue is for applications that use absolute paths as icons? (ex: *.appimage). If so, please try this patch:

diff --git a/plugin-statusnotifier/statusnotifierbutton.cpp b/plugin-statusnotifier/statusnotifierbutton.cpp
index 9aff4f08..67a1c8cf 100644
--- a/plugin-statusnotifier/statusnotifierbutton.cpp
+++ b/plugin-statusnotifier/statusnotifierbutton.cpp
@@ -166,7 +166,11 @@ void StatusNotifierButton::refetchIcon(Status status, const QString& themePath)
         QIcon nextIcon;
         if (!iconName.isEmpty())
         {
-            if (QIcon::hasThemeIcon(iconName))
+            if (iconName[0] == QChar::fromLatin1('/'))
+            {
+                nextIcon.addFile(iconName);
+            }
+            else if (QIcon::hasThemeIcon(iconName))
                 nextIcon = QIcon::fromTheme(iconName);
             else
             {
ahsand97 commented 3 years ago

@palinek I can't reply on this and thanks for the feedback, what I was trying to point out is that QIcon::hasThemeIcon(iconName) returns false for the icon remmina-status in the tests I did on my pc and in a virtual machine, so there's no appindicator for remmina even with the latest lxqt-panel and libqtxdg and somehow XdgIcon::fromTheme(iconName) it is able to find it, that's why I asked for that pull request cuz if that line is not added then I still see no appindicator for remmina.

Now that you point out that QIcon::fromTheme(iconName) can have a valid icon even with QIcon::hasThemeIcon(iconName) returning false I did some more tests and it turned out you were right, QIcon::hasThemeIcon("remmina-status") returns false but using QIcon::fromTheme("remmina-status"); I see the icon correctly.

changing here this (returns false for remmina-status icon)

if (QIcon::hasThemeIcon(iconName))
    nextIcon = QIcon::fromTheme(iconName);

for this:

nextIcon = QIcon::fromTheme(iconName);
if (nextIcon.isNull())
{
    QDir themeDir(themePath);

results in the icon being found correctly image

yan12125 commented 3 years ago

even if hasThemeIcon() is false, the fromTheme() can return a valid icon image.

Excellent point @palinek! Do you know when will it occur?

results in the icon being found correctly

Cool! I'm +1 for that change - get rid of hasThemeIcon() and just use fromTheme().

palinek commented 3 years ago

Do you know when will it occur?

If there is not the "exact match" (meaning the "dash fallback" is available only):

  /*!
      \since 4.6

      Returns \c true if there is an icon available for \a name in the
      current icon theme, otherwise returns \c false.

      \sa themeSearchPaths(), fromTheme(), setThemeName()
  */
  bool QIcon::hasThemeIcon(const QString &name)
  {
      QIcon icon = fromTheme(name);

      return icon.name() == name;
  }   

get rid of hasThemeIcon() and just use fromTheme().

I think that also...

palinek commented 3 years ago

IMO something like this should be used..

$ git diff
diff --git a/plugin-statusnotifier/statusnotifierbutton.cpp b/plugin-statusnotifier/statusnotifierbutton.cpp
index 9aff4f08e..1a5cecd6c 100644
--- a/plugin-statusnotifier/statusnotifierbutton.cpp
+++ b/plugin-statusnotifier/statusnotifierbutton.cpp
@@ -163,12 +163,10 @@ void StatusNotifierButton::refetchIcon(Status status, const QString& themePath)
     }

     interface->propertyGetAsync(nameProperty, [this, status, pixmapProperty, themePath] (QString iconName) {
-        QIcon nextIcon;
         if (!iconName.isEmpty())
         {
-            if (QIcon::hasThemeIcon(iconName))
-                nextIcon = QIcon::fromTheme(iconName);
-            else
+            QIcon nextIcon = QIcon::fromTheme(iconName);
+            if (nextIcon.isNull())
             {
                 QDir themeDir(themePath);
                 if (themeDir.exists())
yan12125 commented 3 years ago

IMO something like this should be used..

yeah, this is basically the same patch @ahsand97 tested above (https://github.com/lxqt/libqtxdg/issues/260#issuecomment-878046696)