Created attachment 62156 [details] [review]
Initial patch

Shows a notification bubble when the song changes.

Thanks to Sebastian Dröge for the help and the bindings code.

Created attachment 62157 [details]
Screenshot

Screenshot of a notification bubble.

Hey! Just a comment, it compiles fine without libnotify installed, then fails hard when you try to play a song with muine:

Unhandled Exception: System.DllNotFoundException: notify
in (wrapper managed-to-native) Muine.TrayIcon:notify_init (string)
in [0x00005] (at /usr/local/src/muine-notification/plugins/TrayIcon.cs:310) Muine.TrayIcon:Notify (System.String summary, System.String message, Gdk.Pixbuf cover, Gtk.Widget widget)
in [0x00054] (at /usr/local/src/muine-notification/plugins/TrayIcon.cs:259) Muine.TrayIcon:OnSongChangedEvent (ISong song)
in (wrapper delegate-invoke) System.MulticastDelegate:invoke_void_ISong (Muine.PluginLib.ISong)
  .
  .

Try-Catch?

Besides that it looks great to me. Nice patch! :)

I'm not really familiar with C# and Mono. Do you know how to wrap the DllImport and the method declarations in a try/catch statement?

The almighty latexer said the following in #muine about the patch:
06:45 <latexer> and yeah, since it's all pinvoked stuff, you can throw the
                whole thing in a try/catch and be done with it.
06:45 <latexer> i suggest checking some bool before the try and, setting it to
                true in the catch.
06:45 <latexer> so you avoid the pinvoke overhead each time, you only do it
                once.

I'm no good with the pinvoke stuff either, but I guess he meant doing something like:

  private Boolean notifyDllMissing = false; // Assume it works


    // In the song changed part:
    if (!notifyDllMissing && song != null) {
       Notify(....
    }


  // In notify
  public static void Notify(string summary, string message,
                            Pixbuf cover, Widget widget)
      {
      Try {
           if(notify_init("Muine"))
           { 
               /* Show the notification */
           }
      } Catch(DllNotFoundException) {
          notifyDllMissing = true;  // The dll is missing
      }
  }

I could be wrong of course, can't test until I get home from work.
/Claes

Created attachment 62189 [details] [review]
Improved patch

This improved patch drastically reduces the PInvoke overhead by only trying to initialize once.

Thanks for the comments, I think it's even better now :)

Created attachment 62190 [details] [review]
Improved patch

Don't crash if libnotify was not found.

Created attachment 62191 [details] [review]
Even better!

This patch is almost the same as the previous version, but adds two <dllmap> entries to TrayIcon.dll.config (libnotify and libgobject-2.0)

Created attachment 62236 [details] [review]
Same patch as above, but without the a/ b/ paths

The patch above had some strange dirs that I removed. Otherwise it seems good.

Would it be possible to delay the notification a few seconds? With the current one, if you skip forward a few songs, the notification popup is showing all the time, even if the song hasn't actually started to play.
/Claes

The patch from #8 is working fine here (Mandriva Cooker, libnotify 0.3.2, notification-daemon 0.3.4).

I've been using Muine with notifications for a while now. The current patch shows:

>  Wolfgang Amadeus Mozart - Benedictus
>  Requiem

My local version shows:

>  Now playing: Benedictus
>  by Wolfgang Amadeus Mozart

This looks very friendly to me and I think it's better than the original version (despite the album name not being shown)

Created attachment 62673 [details] [review]
Other strings in notification bubble

This is the patch with the strings from my comment #11. Fiddle with patch -p1/-p0 to trim the a/ and b/ parts (those are added by bazaar-ng)

Created attachment 62674 [details] [review]
Other strings in notification bubble take 2

Woops, there was a spurious |colordiff pipe in my patch :P

I found another issue: titles with an ampersand (&) are not displayed. I guesss some sort of escaping (XML entities?) should be done before passing the string to the notification API.

17:45:32 <@latexer> uws: you probably need to use GLib.Markup.EscapeText on the string first.

Created attachment 63306 [details] [review]
Correctly escape strings

Updated the patch to correctly escape the strings before passing them to libnotify.

Created attachment 63609 [details]
Shot of album cover being cut off by notification bubble.

Comment on attachment 63609 [details]
Shot of album cover being cut off by notification bubble.

I'm a fan of this patch, but as you can see in the attached ss, it cuts off the bottom of the album cover.  I'm running Ubuntu Dapper with libnotify 0.3.2 and notification-daemon 0.3.4.

Wow, you actually see a cover! I've never managed to get one to show in one of my bubbles... I can't seem to figure out why, though.

About the cover being cut off: I think it's a libnotify bug, since we have no control over the size of the bubble. I think the notification bubble should somehow respect the size of the pixbuf passed to it.

(In reply to comment #18)
> (From update of attachment 63609 [details] [edit])
> I'm a fan of this patch, but as you can see in the attached ss, it cuts off the
> bottom of the album cover.  I'm running Ubuntu Dapper with libnotify 0.3.2 and
> notification-daemon 0.3.4.
> 

It is a notification-daemon bug, and I'll be fixing it for the next release. I've just been pretty busy with work stuff and a Galago release lately, but I'll soon be getting back to notification-daemon and releasing a new version with several fixes.

Ok. Great to know you're working on it.

Christian, any clue why I cannot any pixbuf to show up in my bubbles? It works for other people (see comment #17) and I made sure the pixbuf != null.

(In reply to comment #21)
> Christian, any clue why I cannot any pixbuf to show up in my bubbles? It works
> for other people (see comment #17) and I made sure the pixbuf != null.
> 

Are you using the Standard theme, or the Bubble theme? If Bubble, try switching to Standard. I may be removing Bubble soon, since it's fundamentally broken.

Also, what version of D-BUS are you using?

I am using the standard theme. D-Bus version is 0.61, but I run DBUS HEAD too sometimes.

The NEWS file for notification-daemon version 0.3.5 claims to fix the image cut-off:

	* Fixed a problem where icons were being clipped in notifications.
	  Patch by M.S. (Bug #21)

Jonah, can you please test this?

Created attachment 64405 [details]
Notifications working with latest notification-daemon

Wouter:  The latest version of notification-daemon does indeed fix the cut-off problem.  There might still be some alignment issues, but those I'd consider purely cosmetic.  Cheers.

It seems mandriva has shipped muine with the last patch applied (see http://cvs.mandriva.com/cgi-bin/cvsweb.cgi/contrib-SPECS/muine/ for details).

If I add support for a hidden gconf key that defaults to "off", will the maintainers consider the notification for inclusion?

Ping...?

I suppose it is not a good idea to ship this as an integral part of muine - first, it adds a bunch of dependancies (libnotify, notification-manager et al.) as well as it would certainly annoy many people - I for one know what songs are playing ;) However, it would be sweet if this feature gets implemented as a plugin.

Andrei, the trayicon already is a plugin and this code patches that plugin. So it's totally optional (and not even installed by default). My comment #26 is still unanswered, though...

Regarding the libnotify dependencies: it's not a hard dependency, but merely a "recommends" (in Debian lingo). The libnotify detection is done at runtime and the functionality is just disabled if it's not found.

*** Bug 352097 has been marked as a duplicate of this bug. ***

Osmo Salomaa in bug #352097:

> It would be useful if the tray icon tooltip would
> display album cover art. Technically that would show
> a window instead of a normal tooltip when the mouse
> pointer hovers over the tray icon.

I'm not sure if this is a good idea...

Created attachment 73696 [details] [review]
Update to CVS HEAD

This is the same patch, applying cleanly against current CVS HEAD.

Changing component. Filter "uws doing muine component triaging" to get rid of the spam. Thanks!

go go uws!

This is probably not be muine default behaviour. I'm still waiting for answer to my question in comment #26.

Hey Wouter,

Sorry for the delay in getting back to you. I am all for getting this in, once we get a gconf key, defaulted to false, for enabling/disabling the feature. Thanks for the patience and hard work!

any news on this ?

I'm slacking, but if there's any interest I'll try to come up with an updated patch that has the Gconf stuff as well.

Is muine maintained?

Wouter: yeah update your patch and get it going to svn.
Diego: I don't know but at least some of us are trying :).

In that case also check bug #308208.

(In reply to comment #39)
> Is muine maintained?

Basically not.

(In reply to comment #38)
> I'm slacking, but if there's any interest I'll try to come up with an updated
> patch that has the Gconf stuff as well.

Done :)
See attachment.

Patch add GConf key (off by default) and actually installs the tray icon plugin :)

Created attachment 119061 [details] [review]
As above patches but with GConf key and installs the plugin

Haven't tested it, but just let's do it. Please commit your patch with a decent ChangeLog message.

Thanks for the work!

Committed to trunk...