mkiol / GNotifier

Thunderbird add-on that replaces built-in notifications with the OS native notifications
https://addons.mozilla.org/thunderbird/addon/gnotifier/
GNU General Public License v3.0
164 stars 25 forks source link

libnotify: Icon must be passed as a path #67

Closed sardemff7 closed 8 years ago

sardemff7 commented 8 years ago

In the specification (the updated one) it is explicit that app_icon (the direct paramater) and "image-path" (the hint) are icon names (for themes) or URIs and that only file:// URIs are supported.

In my daemon, I see the icon is passed as a simple path "/tmp/gnotifier-l7xukC" while is definitely should have the file:// scheme. Also, I believe you should pass it as "image-path" and not icon, and the icon should probably be "firefox".

Doing that allows the server to either display the image (if there is one), the icon (if there is no image, as a fallback) or both (for rare servers that support it, mine does).

I think it should be trivial to change, but I could not decide where to do that change (in linux.js I guess, but where exactly). I can make a pull request or provide a patch if you point me to the right place to change that.

mkiol commented 8 years ago

Thanks for sharing your findings. I've just committed the change (479f77f441e5ef05c6b5f4eeb30fc1a9c50d934b).

It's worth mentioning, that URI form works only when version of notification daemon is 1.2.

mkiol commented 8 years ago

Also, I believe you should pass it as "image-path" and not icon, and the icon should probably be "firefox".

I'm using libnotify and libnotify API is quite limited. Look at this function. There is only one icon param :-/.

sardemff7 commented 8 years ago

First, thanks for the fix! Then, sorry that I overlooked your code a bit.

In that case, you should use notify_notification_set_image_from_pixbuf() if possible. libnotify already pulls gdk-pixbuf as a dependency so it is merely a matter of accessing the API. This will use the image-data/image_data/icon_data hint, depending on the specification version.

Wait, I just remembered, you can add the hint directly if you want, using notify_notification_set_hint(), it makes less API to use, since creating the GVariant will require only one function g_variant_new_string() and even better, they use floating references (so you do not have to worry about freeing it). This one works on 1.2 only.

Again, if you want to point be at the relevant part of the code, I can provide a patch.

Side note: a server not using the 1.2 version of the spec should probably die quickly. Do you know any? A new specification is implemented in GTK+ and org.freedesktop.Notifications is being deprecated already. (At least for GNOME, not sure KDE ever really relied on it anyway, they have an internal one IIRC.)

mkiol commented 8 years ago

Thank you for clarification. Now I see that libnotify has everything what it's needed! Apparently I was blind earlier.

Yes, I would be very grateful if you could provide a patch. Feel free to change whatever you think is needed. IMHO, the best way would be to modify the notifyWithActions fun from linux.js.

Regarding servers using spec below version 1.2. Probably I wouldn't noticed it, but my distro (Opensuse 13.1 with KDE) uses 1.1. So, there is at least one that is alive :-)

sardemff7 commented 8 years ago

I just read the 1.1 specification and image_path (deprecated hint now renamed image-path) and app_icon should already be file:// URIs according to it.

What is your exact server implementation, maybe we can fix it?

mkiol commented 8 years ago

It's notification-daemon 0.7.6-4.1.3. Source on build.opensuse.org

Libnotify's notify_get_server_info returns this:

sardemff7 commented 8 years ago

This is strange. As visible here They were already using GFile with g_file_new_for_commandline_arg, so it should handle (for absolute paths) both file:// URI and straight paths.

Can you test this simple program just to be sure:

/*
 * Compile with: gcc -o test test.c $(pkg-config --cflags --libs gio-2.0)
 */

#include <glib.h>
#include <gio/gio.h>

int
main(int argc, char *argv[])
{
    if ( argc < 2 )
        return 1;

    GFile *file;
    file = g_file_new_for_commandline_arg(argv[1]);
    g_print("%s", g_file_get_path(file));

    return 0;
}

Thanks for investigating.

mkiol commented 8 years ago

Will test it, but tomorow.

mkiol commented 8 years ago

It works as expected. For example it converts file:///tmp/test to /tmp/test.

mkiol commented 8 years ago

Find below dbus-monitor log and screenshot of my KDE notification message. File URI is passed to notification daemon, and the result is as you see - icon is invalid.

method call sender=:1.950 -> dest=:1.18 serial=16 path=/org/freedesktop/Notifications; interface=org.freedesktop.Notifications; member=Notify
   string "Firefox"
   uint32 0
   string "file:///tmp/gnotifier-ydp0GB"
   string "Notification #1"
   string "This is the text body of the notification. 
Pretty cool, huh?"
   array [
      string "gnotifier_140610956506880_0"
      string "Otwórz"
   ]
   array [
   ]
   int32 -1
method return sender=:1.18 -> dest=:1.950 reply_serial=16
   uint32 50

img13

sardemff7 commented 8 years ago

I think you are not using notification-daemon but something integrated to KDE. I am trying to identify which KDE component is providing notifications, it seems it is a Plasma applet, but it is quite hard to locate the exact code.

sardemff7 commented 8 years ago

Ok, I found the code! You can see line 132 that is is checking for the file: scheme. But this code is not used for the app_icon param, which leads to your error. Used with the image_path hint (now image-path) it should work perfectly.

Your version is 2 years old, so I think we cannot patch it, but the code is still in use Plasma! There may be hope to fix that code and port it to 1.2…

Edit: BTW, the code in Plasma is the same, so it does not check for file:// in app_icon, and your code is currently “broken” then. I will try to fix #69 quickly, then everyone will be fine.