Project

General

Profile

Feature #206

Add better integration in Gnome-Shell via the notify plugin

Added by Jean-Alexandre Anglès d'Auriac over 11 years ago. Updated over 10 years ago.

Status:
Closed
Priority:
Minor
Assignee:
-
Category:
plugins/notify
Target version:
Start date:
November 09, 2012
Due date:
% Done:

100%

Estimated time:
Affects version:

Description

Hi.

I modified the notify plugin to provide better integration into Gnome-Shell, mainly by copying the behavior of Rhythmbox's notifications.
What I did was mainly to make the notifications persistent and to add actions onto them. This makes the notification act in some way like the old status icon, and allow to control Audacious even when the screen is locked, if the option is activated.

The result can be viewed here :
http://www.youtube.com/watch?v=m8T84TVH234

The code is available here :
http://buuficontheme.free.fr/notify-gs.tar.gz

I currently have two issues :
- I have relatively frequent Gnome-Shell crashes that seems to be related to the use of this plugin, but I've been unable to track down what exactly is causing them
- I don't display the album art. The problem here is that audgui_pixbuf_for_current() returns a GdkPixmap, that is then used with notify_notification_set_image_from_pixbuf() to set it in the notification, but this has terrible performance. I don't know whether is it inherent to the design or bad implementation on libnotify or gnome-shell, but there is a 2 to 4 second lag before the notification is updated. On the other hand, Rhythmbox is just using notify_notification_set_hint() with the "image_path" hint that provides the path to the file, and this solution doesn't introduce any lag. To fix that, I should either transform the gdkpixbuf into a file, or see with upstream if notify_notification_set_image_from_pixbuf() can be improved.

event.patch (2.93 KB) event.patch Jean-Alexandre Anglès d'Auriac, November 10, 2012 00:45
osdh.patch (410 Bytes) osdh.patch Jean-Alexandre Anglès d'Auriac, November 10, 2012 00:45
osdc.patch (8.68 KB) osdc.patch Jean-Alexandre Anglès d'Auriac, November 10, 2012 00:45
event.c.patch (3.06 KB) event.c.patch Jean-Alexandre Anglès d'Auriac, November 22, 2012 12:26
notify.c.patch (1.62 KB) notify.c.patch Jean-Alexandre Anglès d'Auriac, November 22, 2012 12:26
osd.c.patch (6.77 KB) osd.c.patch Jean-Alexandre Anglès d'Auriac, November 22, 2012 12:26
osd.h.patch (514 Bytes) osd.h.patch Jean-Alexandre Anglès d'Auriac, November 22, 2012 12:26
osd.c.patch (6.77 KB) osd.c.patch Jean-Alexandre Anglès d'Auriac, November 24, 2012 01:48
eventc.patch (3 KB) eventc.patch Jean-Alexandre Anglès d'Auriac, November 24, 2012 11:11
gitdiff.patch (12.1 KB) gitdiff.patch Jean-Alexandre Anglès d'Auriac, November 24, 2012 18:01
gitdiff.patch (14.3 KB) gitdiff.patch Jean-Alexandre Anglès d'Auriac, November 27, 2012 04:02
gitdiff.patch (12.9 KB) gitdiff.patch Jean-Alexandre Anglès d'Auriac, November 27, 2012 04:07
gitdiff.patch (12.9 KB) gitdiff.patch Jean-Alexandre Anglès d'Auriac, November 27, 2012 16:46
gitdiff.patch (12.7 KB) gitdiff.patch Jean-Alexandre Anglès d'Auriac, November 27, 2012 16:47
gitdiff.patch (12.8 KB) gitdiff.patch Jean-Alexandre Anglès d'Auriac, November 28, 2012 18:04
gitdiff.patch (13.3 KB) gitdiff.patch Jean-Alexandre Anglès d'Auriac, December 08, 2012 02:32
gitdiff.patch (13.4 KB) gitdiff.patch Jean-Alexandre Anglès d'Auriac, December 31, 2012 16:54
gitdiff.patch (13.6 KB) gitdiff.patch Jean-Alexandre Anglès d'Auriac, February 04, 2013 16:18
gitdiff.patch (13.5 KB) gitdiff.patch Jean-Alexandre Anglès d'Auriac, February 17, 2013 02:10
pixbuf.patch (13 KB) pixbuf.patch Patch using pixbuf, for performance testing Jean-Alexandre Anglès d'Auriac, February 18, 2013 02:45
gitdiff.patch (13.2 KB) gitdiff.patch Jean-Alexandre Anglès d'Auriac, February 18, 2013 04:29
gitdiff.patch (12.9 KB) gitdiff.patch Jean-Alexandre Anglès d'Auriac, March 12, 2013 18:19
notifyactionsupport.patch (13 KB) notifyactionsupport.patch Jean-Alexandre Anglès d'Auriac, March 16, 2013 20:01
notifyactionsupport.patch (13 KB) notifyactionsupport.patch Jean-Alexandre Anglès d'Auriac, April 01, 2013 17:19
notifyactionsupport.patch (13 KB) notifyactionsupport.patch Jean-Alexandre Anglès d'Auriac, April 01, 2013 17:58
notifyactionsupport.patch (13.3 KB) notifyactionsupport.patch Jean-Alexandre Anglès d'Auriac, April 08, 2013 03:01
notifyactionsupport.patch (13.3 KB) notifyactionsupport.patch Jean-Alexandre Anglès d'Auriac, April 10, 2013 02:40
notifyactionsupport.patch (13.2 KB) notifyactionsupport.patch Jean-Alexandre Anglès d'Auriac, April 10, 2013 03:04
notifyactionsupport.patch (13.1 KB) notifyactionsupport.patch Jean-Alexandre Anglès d'Auriac, April 14, 2013 00:19
notifyactionsupport.patch (13.1 KB) notifyactionsupport.patch Jean-Alexandre Anglès d'Auriac, April 20, 2013 23:34
notifyactionsupport.patch (13.1 KB) notifyactionsupport.patch Jean-Alexandre Anglès d'Auriac, April 20, 2013 23:36
notifyactionsupport2.patch (12.5 KB) notifyactionsupport2.patch Jean-Alexandre Anglès d'Auriac, April 22, 2013 02:10
notifyactionsupport.patch (20.3 KB) notifyactionsupport.patch Jean-Alexandre Anglès d'Auriac, May 22, 2013 16:53

History

#1 Updated by John Lindgren over 11 years ago

A "notifications" plugin by nature is not supposed to control the player, only notify you when a song changes. Also, I believe there is already an add-on for Gnome Shell that controls MPRIS-compatible media players.

#2 Updated by John Lindgren over 11 years ago

Also, please submit proposed changes in diff -u format so that we can see what you have changed more quickly.

#3 Updated by Jean-Alexandre Anglès d'Auriac over 11 years ago

Well, it is Gnome decision to use notification as the mean to control the player when the screen is locked, and to replace what used to be provided by status icon. It's not my idea, and it's not the way I would have implemented it, but that's what they choose.
The extensions, while some are pretty good, are just expansions. That mean they are not installed by default, and sometimes don't get updated when the new Gnome-Shell come out, and are not necessarily perfectly integrated, and everything. For instance, I'm not one hundred percent sure, but I really think that with the way the lock screen is currently implemented makes it impossible for an extension that has not been installed systemwide (i.e. any extension installed directly from the official website https://extensions.gnome.org/ , for instance) to modify it to provide media player control.

I should add some configuration options to allow going back to the previous behavior with no control on the notification, though. And possibly to allow tweaking which actions are available, and stuff like that. But before going further, I wanted to know your reaction to this idea, and to fix the two problems I mentioned.

(The diff format might be useful for the change to event.c and osd.h, but osd.c was extensively modified to be as close as possible to Rhythmbox code so that I could better understand the difference in behavior between the two that I witnessed, so I guess it's easier to just read the new file without bothering about the old one.)

#4 Updated by John Lindgren over 11 years ago

If the added controls were made optional, I think they would be okay. But your patch needs some work. First of all, don't move code around arbitrarily. It makes it difficult to see what you have changed. Also, try to respect the way that the original author of the plugin organized the code into separate source files. Code that decides when a notification should be shown and retrieves the needed information from Audacious should be in event.c. Code that does the dirty work of actually showing the notification should be in osd.c. Separating things out this way may mean a little more work right now, but it saves time in the long run.

#5 Updated by Jean-Alexandre Anglès d'Auriac over 11 years ago

I still have trouble understanding why Gnome-Shell still crashes sometimes and why it behave strangely, but I have a lots of problems that seem to be related to DBus in some way, with Gnome-Do crashing while complaining about DBus, or even GDM not loading itself properly and also complaining about DBus…

Is that better ?

#6 Updated by John Lindgren over 11 years ago

I get an error when trying to compile now:

osd.c: In function ‘osd_show’:
osd.c:119:49: error: ‘title2’ undeclared (first use in this function)
osd.c:119:49: note: each undeclared identifier is reported only once for each function it appears in

FYI, you can use "git diff" to produce a single patch rather than four separate ones.

#7 Updated by Jean-Alexandre Anglès d'Auriac over 11 years ago

Oups, last minute code cleanup and I forgot to test-compile it, sorry.
I don't use git, I just download the archive, but if that's more convenient to you, I can switch to git, or just merge every patch in one big patch.

#8 Updated by John Lindgren over 11 years ago

You took out the "if not playing then don't show notification" check. Why? I don't want to be shown a notification unless a song is playing.

#9 Updated by Jean-Alexandre Anglès d'Auriac over 11 years ago

Well, I guess with actions, it can be useful to relaunch playback or to raise the Audacious window.
So, new version that shows a notification only if actions are available or if the notification has been explicitly asked by the user.

#10 Updated by Jean-Alexandre Anglès d'Auriac over 11 years ago

Ok, I made a git diff patch :

#11 Updated by John Lindgren over 11 years ago

Doesn't compile again.

osd.c: In function ‘osd_show’:
osd.c:119:49: error: ‘title2’ undeclared (first use in this function)
osd.c:119:49: note: each undeclared identifier is reported only once for each function it appears in

event.c: In function ‘update’:
event.c:38:9: warning: implicit declaration of function ‘aud_get_bool’ [-Wimplicit-function-declaration]
event.c:39:13: error: too few arguments to function ‘osd_show’
In file included from event.c:32:0:
osd.h:27:6: note: declared here

#12 Updated by Jean-Alexandre Anglès d'Auriac over 11 years ago

Yes, I noticed that. It doesn't compile against git version. By some strange coincidence, I just discovered that less than one hour ago, and I've just finished fixing that. But since I also discovered that you added aud_art_request_file which will help me display the cover in the notification, I'll just finish implementing that before uploading a working patch. I feel a little stupid though.

#13 Updated by Jean-Alexandre Anglès d'Auriac over 11 years ago

Ok, so now it compiles and I have the right behavior, but if Audacious is launched with the plugin activated, Gnome-Shell completely freeze for, like, about 20 seconds, and give some error message like :
(gnome-shell:20395): Clutter-CRITICAL **: clutter_text_get_text: assertion `CLUTTER_IS_TEXT (self)' failed
It doesn't happen if Audacious is launched with the plugin deactivated and the plugin is activated later on. I'll investigate on that more, but right now it's late and I'm going to sleep.
Any suggestion on this strange behavior would be extremely appreciated though ;).

#14 Updated by Jean-Alexandre Anglès d'Auriac over 11 years ago

Oops I did remove the localization for the About string of the plugin. Same patch, with the localization put back in.

#15 Updated by Jean-Alexandre Anglès d'Auriac over 11 years ago

Err, I forgot a closing parenthesis before going to sleep :|.

#17 Updated by John Lindgren over 11 years ago

All right, it compiles now. That's a start. Here are some more problems ...

There's a warning when applying the patch:
src/notify/gitdiff.patch:249: trailing whitespace.

There's also a warning when compiling:
event.c: In function ‘update’:
event.c:66:27: warning: initialization discards ‘const’ qualifier from pointer target type [enabled by default]

And it still shows me a notification even when Audacious is not playing. ("Stopped. Audacious is not playing.") That's not useful information and there should not be a notification for it.

#18 Updated by Jean-Alexandre Anglès d'Auriac over 11 years ago

I have absolutely no idea why I have “aud_art_get_file() is deprecated. Use aud_art_request_file() instead.” message, because I don't use aud_art_get_file(), just aud_art_request_file(). Any idea ?

#19 Updated by John Lindgren over 11 years ago

There's another warning now:
src/notify/gitdiff.patch:386: trailing whitespace.

Also, on osd.c:166 you set the timeout to one millisecond. Why?

#20 Updated by John Lindgren over 11 years ago

Regarding the aud_art_get_file() warning, have you updated the other plugins recently? mpris2 used to use aud_art_get_file().

#21 Updated by John Lindgren over 11 years ago

As a "heads up": I made some changes to the notify plugin in Git. It doesn't use the deprecated album art API any more.

#22 Updated by Jean-Alexandre Anglès d'Auriac over 11 years ago

Actually, I have no idea how to use git, so I just used the command to get the code that you put on the website, and I never updated anything.
I'm currently investigating on a bug I have with the plugin on gnome-shell. For some strange reason, sometimes calling notify_get_server_caps() makes gnome-shell freeze for about 30 seconds (during which the only thing that can change on the X display is the position of the cursor) and the return value is then NULL instead of the correct GList. I think it may be linked to calling the function at two almost simultaneous times due to two hooks being raised at almost the same time, but I couldn't reproduced the problem in some standalone code yet.

#23 Updated by Jean-Alexandre Anglès d'Auriac over 11 years ago

The bug disappeared, thanks to some DBus update I guess. I also managed to update via git, and I've rewritten a little my code to use pooled strings (event thought I'm not very sure about what it implies, and if it is actually useful). Should I also react to the "current art ready" hook.

#24 Updated by Jean-Alexandre Anglès d'Auriac about 11 years ago

After testing it for a while, you were right, it's better to never show a notification when nothing is playing. I also corrected some trailing whitespace.

#25 Updated by Jean-Alexandre Anglès d'Auriac about 11 years ago

Hi.
New version of my patch that works with the latest git version, and I tried to improve a few stuff, based on your commits to the original code. I'm still not completely sure if I need to catch the "current art ready" hook and how to do it right. What does aud_art_request_file returns when the file is not ready ? The url in which the file is going to be stored, or null ?

#26 Updated by John Lindgren about 11 years ago

You are still setting the timeout to 1 millisecond, which is unacceptable. The notification barely produces a flicker before it disappears again.

aud_art_request_file() returns null if the file is not ready. You should be using aud_art_request_data(), though. EDIT: Even better, use audgui_pixbuf_request_current(). That allows the image data to be decoded only once and shared.

#27 Updated by Jean-Alexandre Anglès d'Auriac about 11 years ago

When I pass a GdkPixbuf to the notification instead of a file, it did cause huge performance problems. By huge performance problems, I mean like a few dozen of seconds of total Gnome-Shell freeze on my laptop. That's why I decided to simply use filepaths.
However, I can provide a patch that does use GdkPixbuf, so that we can check on another machine and see if the problem is also present there, if you think it's not normal.

#28 Updated by John Lindgren about 11 years ago

If Gnome Shell truly cannot handle pixbufs, I'm surprised that no one else has reported the problem, since that's the way things are done already. Bug fixes (or workarounds) should generally not be mixed with patches to implement new features, at any rate.

#29 Updated by John Lindgren about 11 years ago

Two more problems:
1) You removed the code that checked whether the song information changed. Now the notification shows whenever I refresh the playlist.
2) Text passed to gettext() must be in American English. gettext() will translate it to the appropriate language.

#30 Updated by John Lindgren about 11 years ago

Also, this code is criminal:

NotifyNotification * notif = notify_notification_new (...);
hook_associate ("notify shutdown", (HookFunction) clean, notif);
hook_call ("notify shutdown", NULL);
hook_dissociate ("notify shutdown", (HookFunction) clean);

Hooks are not storage space for pointers. Do this:

static NotifyNotification * notif;
notif = notify_notification_new (...);
clean (notif);

#31 Updated by Jean-Alexandre Anglès d'Auriac about 11 years ago

After doing some testing around, I remembered why I made the change.
The huge dozen-of-second freeze is when I don't resize the pixbuf like on the original code. But even if I do resize the pixbuf, there is still about half a second before the notification is updated. This is no problem without actions and persistent notifications : the notification just shows up about half a second later, no problem. However, with persistent notification and actions, it means the play/pause button won't get updated fast enough. The result being not only that the notification doesn't update as fast (which is much more visible this way), but also that if the user clicks two time very fast one the play/pause button, the first click is registered, but the second one is not.
Really, even with pixbuf resizing, using a file path makes controlling Audacious from the notification or the lock screen much more responsive !

I attached a patch with pixbuf resizing. If you can get a gnome-shell environment, try both versions, and you should notice the filepath one being much more responsive. If it's not the case, I will refine the pixbuf version, but even though I don't know much about D-Bus, it doesn't seem so unnatural that transferring the whole picture through it does cost more than just transferring a filepath.

#32 Updated by Jean-Alexandre Anglès d'Auriac about 11 years ago

- Added back the check if the notification text changed. I have to call notify_notification_show much more often when actions are enabled because it's the only way to update the action buttons, but I made sure that it doesn't trigger when not needed.
- Sorry about that French string. It slipped through unnoticed. Fixed now.
- I have no idea what's good or bad use for hooks. I've been told before that using static variable was a bad practice so I thought it would be good to remove them. But if you say it's in fact the opposite… New version with static variables ! Any link to some resource on good usage of hooks ?

It's the version with filepath, because it has much better performance, but I can post an upgraded version with pixbuf too if you want.

#33 Updated by John Lindgren about 11 years ago

You say "transferring a filepath" like it was nothing. Don't you realize that Audacious has to save a temporary image file and the the notification service has to load it, decode it, and then scale it again? If that is faster then tranferring an already-decoded pixbuf then there is a bug somewhere.

#34 Updated by John Lindgren about 11 years ago

Also, don't ever avoid using a certain programming technique or language feature simply because you heard it was "bad practice". Good programming means knowing the appropriate technique to accomplish the task at hand. Any given technique is good practice in some situations and bad practice in others.

Hooks are used to allow responding to program-wide events. For example, you would connect to the "playback begin" hook if you have some code that needs to run when playback is started. Cleaning up data within a single plugin is not a program-wide event, so the use of hooks is inappropriate.

#35 Updated by John Lindgren about 11 years ago

Just to be clear, I don't care if we cause gnome-shell to lock up hard for ten minutes. Transferring a pixbuf causes no performance problems in XFCE. If gnome-shell can't handle the same operation, then go and fix gnome-shell.

#36 Updated by Jean-Alexandre Anglès d'Auriac about 11 years ago

I don't know how DBus works, but if it's very bad at transferring big data, that might explain the problem. Or else, maybe it's just Gnome-Shell. I'll fill a bug in the Gnome bugzilla.

In the meantime, what about using the filepath only if the notification server is gnome-shell, and when the problem is fixed in Gnome-Shell, restrict that workaround to version of Gnome-Shell lower than the one which fixed the problem ? Would that be okay with you ?

#37 Updated by John Lindgren about 11 years ago

Anne O'neam wrote:

In the meantime, what about using the filepath only if the notification server is gnome-shell, and when the problem is fixed in Gnome-Shell, restrict that workaround to version of Gnome-Shell lower than the one which fixed the problem ? Would that be okay with you ?

No.

#38 Updated by Jean-Alexandre Anglès d'Auriac about 11 years ago

It seems that this bug is fixed in the development version of Gnome-Shell. So passing the pixbuf via DBus should work perfectly in Gnome-Shell 3.8.
If you don't want the workaround to be used for Gnome-Shell 3.6, what would you advise doing in that situation ? Not showing the cover ? Not allowing to use the actions ? Or something else completely ?

#39 Updated by John Lindgren about 11 years ago

Anne O'neam wrote:

If you don't want the workaround to be used for Gnome-Shell 3.6, what would you advise doing in that situation ? Not showing the cover ? Not allowing to use the actions ? Or something else completely ?

Personally, I'd suggest using a more stable desktop environment like XFCE. If you're set on using Gnome Shell and the lagging bug is that much of an issue to you, then update or patch Gnome Shell. Changing Audacious is not the right way to fix a bug in your desktop environment.

#40 Updated by Jean-Alexandre Anglès d'Auriac about 11 years ago

It's not about me. If it was about me, I could simply use the code I already written which works fine. It's about other users. Some distribution, like Debian, could keep an old version of Gnome-Shell for a very long time. If a user of said distribution wants to use this plugin, what should I do ? Just leave the bug as it is ? Warn him ? Use a workaround ? Not display the cover ?

#41 Updated by John Lindgren about 11 years ago

I believe Debian ships XFCE now by default.

#42 Updated by Jean-Alexandre Anglès d'Auriac about 11 years ago

I'm sorry, I don't understand what you mean by “Debian ships XFCE by default”. There are a number of different ways to install Debian, and I never heard one of them was considered the “default” one. Many of them ask you to explicitly install a desktop environment yourself, and other includes downloading and burning one or many different desktop environments on a CD, DVD or USB stick. I specifically searched on Google, and I mainly got articles which said that Debian “switched back to Gnome by default”. I guess this means that Gnome is the desktop environment included in the ISO which size makes them most popular, as it is designed to fill the most common CD format. Here is a sample of the article claiming that Gnome is the default desktop environment on Debian : http://www.neowin.net/news/debian-drops-xfce-reinstates-gnome-as-default-desktop .

Still, I don't see how the default desktop environment of one specific distribution I took as an example is relevant to the question I'm asking. Especially since most people that install Debian are sufficiently familiar with Linux to already have an opinion of the different desktop environments, quite often a strong and opinionated one, and therefore are likely to choose their desktop environment by themselves rather than relying on a default choice anyway. Given than both Debian, Gnome and Audacious are quite popular, I'm absolutely certain that there are people that use Audacious on Debian with Gnome-Shell. And Debian was just one possible example, not the sole and only case where some user could be stuck with Gnome 3.6 for a long time.

I totally understand why it was definitely an error to use a process that was slowing Audacious with every other notification servers, and while I don't understand what the problem is with using a fallback only when the notification server in Gnome-Shell 3.6, I respect your authority on that matter. I'm only asking you which way of dealing with this Gnome-Shell 3.6 bug you wouldn't have problems with. I personally think that the fallback is the best way to deal with this bug, but since you don't agree with it, I think the second best solution is to prevent the Gnome-Shell 3.6 users to activate the actions and to write a warning about the bug on stderr. If you think I'm wrong, please tell me so, and give me directions on how to handle this right !

#43 Updated by John Lindgren about 11 years ago

It's a bug in Gnome Shell, so the best way to fix it (in my opinion) is to patch or update Gnome Shell. Since you seem to consider it your personal responsibility to make sure that no one else is affected by the bug, I can only recommend that you go and nag the various Gnome Shell packagers (for Debian or whatever other distribution) to release an patched or updated version. You're wasting your time with me.

#44 Updated by Jean-Alexandre Anglès d'Auriac about 11 years ago

Ok, no problem.
Do I need to unref or free or anything on the pixbuf I get from audgui_pixbuf_request_current() ?

#45 Updated by John Lindgren about 11 years ago

Yes, you have to use g_object_unref(), just as the plugin does without your patch.

#47 Updated by Meister P almost 11 years ago

Thanks for your patch. Much appreciated.

To show the full notification in >=gnome-shell-3.8 you need to add

notify_notification_set_category (notification, "x-gnome.music")

otherwise the control buttons are missing.

See the changes to gnome-shell's js/ui/notificationDaemon.js in commit 8cb3884fa

Here is the corresponding change in rhythmbox.

#48 Updated by Jean-Alexandre Anglès d'Auriac almost 11 years ago

Thank you a lot for your feedback, Meister P.
Referring to the specifications ( https://developer.gnome.org/notification-spec/#categories ), it seems the category is likely to change from “x-gnome.music” to “music” soon…
Why didn't they just wait a little more to get an official music category ?

#49 Updated by Meister P almost 11 years ago

I forgot to mention yesterday that your patch introduces a warning:

osd.c: In function ‘osd_show’:
osd.c:166:9: warning: passing argument 2 of ‘notify_notification_set_image_from_pixbuf’ discards ‘const’ qualifier from pointer target type [enabled by default]
In file included from /usr/include/libnotify/notify.h:27:0,
                 from osd.c:26:
/usr/include/libnotify/notification.h:135:21: note: expected ‘struct GdkPixbuf *’ but argument is of type ‘const struct GdkPixbuf *’

#50 Updated by Jean-Alexandre Anglès d'Auriac almost 11 years ago

How did I let this slip through ?

#51 Updated by John Lindgren almost 11 years ago

With the current version of this patch:

1. The "show on-screen display" hotkey configured in the Global Hotkeys plugin no longer works.
2. A notification for the current song is displayed when I press "stop".

Please fix these regressions.

#52 Updated by Thomas Lange almost 11 years ago

You should also use g_error_free() to prevent memory leaks.

#53 Updated by Jean-Alexandre Anglès d'Auriac almost 11 years ago

1 should be fixed. I couldn't reproduce 2, I'm working on it.

#54 Updated by John Lindgren almost 11 years ago

Anne O'neam wrote:

1 should be fixed. I couldn't reproduce 2, I'm working on it.

Well, you connected "playback stop" to update_button(), which calls osd_update_button(), which calls notify_notification_show(). Naturally, that shows a notification when "playback stop" is called.

#55 Updated by Jean-Alexandre Anglès d'Auriac almost 11 years ago

osd_update_button() should not call notify_notification_show() (or do anything else) if actions_available is not set to true.
Do XFCE notify server support actions ? Maybe osd_update_button() should only do something if the server supports BOTH actions AND persistance.
Tell me if this is better.

#56 Updated by Jean-Alexandre Anglès d'Auriac almost 11 years ago

Or rather, this, with actual support for actions without persistence.
By the way, is there a hook called when the preferences are updated ?

#57 Updated by John Lindgren almost 11 years ago

$ git apply /tmp/notifyactionsupport.patch
/tmp/notifyactionsupport.patch:383: trailing whitespace.

warning: 1 line adds whitespace errors.

Set your editor to remove trailing whitespace.

#58 Updated by John Lindgren almost 11 years ago

John Lindgren wrote:

1. The "show on-screen display" hotkey configured in the Global Hotkeys plugin no longer works.
2. A notification for the current song is displayed when I press "stop".

These two issues seem to be fixed now (good). However, the notification no longer shows any album art (bad).

#59 Updated by Jean-Alexandre Anglès d'Auriac almost 11 years ago

I think the problem might have been that, when update() was called because the “current art ready” hook was called, update just noticed the title and message didn't change, so it didn't update the notification.
This should fix it.

#60 Updated by John Lindgren almost 11 years ago

Why did you remove the initial update() call? Now, if I enable the plugin while a song is playing, it is in an inconsistent state (last_title and last_message are null). If I then press F5 to refresh the playlist, a notification is shown.

#61 Updated by Jean-Alexandre Anglès d'Auriac almost 11 years ago

Ok, putting it back.
Is there a hook called when the preferences are updated ? Or how should I handle to change the behavior when the user ticks or unticks the option to use actions in the preference window ?

#63 Updated by John Lindgren almost 11 years ago

Anne O'neam wrote:

Is there a hook called when the preferences are updated ? Or how should I handle to change the behavior when the user ticks or unticks the option to use actions in the preference window ?

There is a callback pointer in the PreferencesWidget struct.

#64 Updated by John Lindgren almost 11 years ago

A few questions:
- Why are you merging reshow() into update()?
- Why rename the "explicit" parameter to "forcedisplay"?
- Why separate out update_explicit() from update() when update() already has an "explicit" parameter?

I just don't see the point of these changes.

#65 Updated by Jean-Alexandre Anglès d'Auriac almost 11 years ago

I have no idea. I guess I thought I could use some better way, and it didn't worked out, so I tried to fix it, and I didn't notice I had essentially gone back to the previous behavior without noticing it. Anyway, here is a new version that takes back all those changes, and finally change the behavior right after the user modify the setting. I could find how to use the callback from PreferencesWidget, but adding an .apply in PluginPreferences seems to work perfectly.

#66 Updated by John Lindgren almost 11 years ago

  • Category set to plugins/notify
  • Status changed from New to Closed
  • Target version set to 3.4
  • % Done changed from 0 to 100

Thanks for the hard work. I've made some further changes to the patch and finally committed it. Probably should have sat down for a few hours and done this some time ago, sorry.
https://github.com/audacious-media-player/audacious-plugins/commit/ec8042013b1afa0dd05f225fcbd6b738830ebc4a

#67 Updated by Jean-Alexandre Anglès d'Auriac almost 11 years ago

Thanks, that awesome. A few questions, though :
- Why did you remove “notify_notification_set_category (notification, "x-gnome.music");” ? It kind of defeat the original purpose of the patch
- Why did you remove the “previous” button ?
- Why do you have a “icon” argument to osd_show ? Why set the icon to NULL when pixbuf is not NULL ? I think the icon should always be set to "audacious". I don't see why it is set to “audio-x-generic” when Audacious is not playing, and setting it to NULL when there is a cover make the cover take the place of the icon in Gnome-Shell, so it get really small (and yes, I know that's something I should bug-report to Gnome, but having a consistent icon representing the application that created the notification seems logic).

#68 Updated by Jean-Alexandre Anglès d'Auriac almost 11 years ago

Also, in Gnome-Shell 3.8 the notification doesn't make the whole Shell freeze, but it's still way slower than with path-sharing.
By the way, if XFCE support actions within notifications, could you please run this little test program ?
http://pastebin.com/7Wy27JWa
Just call the program with both possible argument, and click as quickly as possible on the “play”/“pause” button.
Thanks !

#69 Updated by Meister P almost 11 years ago

Anne O'neam wrote:

Thanks, that awesome. A few questions, though :
- Why did you remove “notify_notification_set_category (notification, "x-gnome.music");” ? It kind of defeat the original purpose of the patch
- Why did you remove the “previous” button ?
- Why do you have a “icon” argument to osd_show ? Why set the icon to NULL when pixbuf is not NULL ? I think the icon should always be set to "audacious". I don't see why it is set to “audio-x-generic” when Audacious is not playing, and setting it to NULL when there is a cover make the cover take the place of the icon in Gnome-Shell, so it get really small (and yes, I know that's something I should bug-report to Gnome, but having a consistent icon representing the application that created the notification seems logic).

@John Lindgren: ping?

@Anne O'neam: Could you post a new version of your patch that applies to current git master?

#70 Updated by Jean-Alexandre Anglès d'Auriac almost 11 years ago

Meister, are you using the latest git version of Audacious for your day to day uses ?
This should just revert to my version.

I managed to get a grip on a XFCE. By trying my test example, I found out that using pixbuf-sharing really didn't cause any performance issue. But I wasn't able to test path-sharing at all, as I couldn't make it work. May be related to the fact that xfce notifyd claims to support only the 0.9 version of the notification protocol (while the current version is 1.2).

#71 Updated by Jean-Alexandre Anglès d'Auriac over 10 years ago

Ok, I opened a new bug report for the icon stuff. I guess I'll make a new one for the category when it's become part of the standard or something.
http://redmine.audacious-media-player.org/issues/312

Also available in: Atom PDF