Audacious - OLD, PLEASE USE GITHUB DISCUSSIONS/ISSUES

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.

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

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.

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.

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.

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

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.

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.

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?

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

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.

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.

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.

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.

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.

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);

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.

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.

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.

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 ?

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.

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 ?

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.

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 ?

I believe Debian ships XFCE now by default.

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 !

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.

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

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")

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

Here is the corresponding change in rhythmbox.

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 *’

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.

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

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.

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

warning: 1 line adds whitespace errors.

Set your editor to remove trailing whitespace.

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).

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.

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.

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.

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).

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 !

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?

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