I don't know what a "messaging indicator" is. Is there any documentation on what libnotify does and how to use it? A quick search doesn't turn up anything useful.

libindicate (not libnotify) is a library for indicators, and the messaging indicator is the first of those. Ted Gould <ted.gould@can...cal.com> would be the best person to speak with. He says there's some documentation in the package in 9.04, not much more to go on, sorry.

We're always happy to try and improve empathy's user experience. The information you've given is a bit limited though. ``Some documentation in the package in 9.04'' doesn't really help me that much (what's 9.04 anyway?)

Maybe you or Ted could explain how libindicate/indicators would improve things for Empathy? Screenshots/documentations/mockups/examples are appreciated, patches even more so :)

Also is libindicate something more gnome applications might start using in the near future? Or would it be an external dependency for gnome just for empathy's benefit ?

In the latest version of Ubuntu we've been working on the notification and interaction with IM clients.  We've currently adapated a plugin for Pidgin based on the code that we've written, one of those is libindicate.  This is a library that we've written and are currently working on cleaning up the spec for to submit as an FD.o specification.

A basic introduction of what we're doing with regards to the messaging indicator can be found in my blog here:

http://gould.cx/ted/blog/Where_are_my_messages_

Some flash movies of the notifications themselves can be found here:

http://www.markshuttleworth.com/archives/253

To look at the patch we have made to Pidgin-libnotify plugin you can see that code here also.

http://package-import.ubuntu.com/p/pidgin-libnotify/jaunty/annotate/head%3A/debian/patches/indicate.patch

Created attachment 132157 [details] [review]
First stab at indicator support for empathy

Hi,

Here's the start of a patch to add support for libindicate to
empathy. There are surely some bugs hiding, it's perhaps not
complete, depending on the desired behaviour, and there are some
niggles that would be great to fix.

You can see a quick demo of what this looks like at:

  http://jameswestby.net/images/empathy-indicator.ogv

There are 4 places where indicator support is added:

  1) in the status icon, which doesn't actually show the icon if
     indicators are enabled.
  2) reacting to events, so that you get an indicator if you receive
     a message for a conversation that hasn't yet started.
  3) in the chat window, so that you get an indicator if you receive
     a message to an unfocused conversation.
  4) in the main window so that you get transient indicators when
     someone becomes available.

The list is in descending probability of working, with the last not working
as it should.

A brief overview, there are two things created the first is a "server",
which in essence corresponds to an app that may produce indicators. This
is what shows the app name in the menu, and callbacks on that trigger
the show/hide.

The second thing is the indicators themselves, which are similar to 
notifications. You create one with appropriate information, and libindicate
makes it available over dbus, where it can be queried and displayed.
There isn't much to them, but tracking them is where most of the
work comes from.

I'd appreciate a first review on the direction that is being taken to
add this support. It should almost no changes when libindicate is
not enabled (where there is a change it's not really to do with libindicate,
explained below).

Here's a list of some of the outstanding issues with the changes, in
no particular order. Also, some are things that will impact anything
wanting to add indicator support, and so a solution in empathy may
not be the way to do it.

  * Changing the preference currently doesn't affect most of the behavior,
    it only really controls whether the status icon is shown. Relatedly
    https://bugs.launchpad.net/indicator-applet/+bug/351537 means that
    you currently can't hide the server.

  * The hide/show behaviour is not very good. As the only implementation
    of a messages indicator listener is based on a menu it receives the
    focus when using it. This means that "GTK_WINDOW_ACTIVE" code
    currently used doesn't work. I had to make this just show and
    hide the window when visibility is toggled. Some better way to track
    whether to present or hide the window would be appreciated, and
    should be common across apps. (pidgin has code that tracks the gdk 
    information to see if the window is obscured. Using that may work,
    but I couldn't get the desired behaviour on a first try.

  * Presenting the preference in the UI is hard, because and "indicator"
    is not an obvious concept. The evolution support has "Indicate new
    messages in the panel", which is reasonable, but the panel isn't
    necessarily where they will be shown. This is something that should
    be common across apps.

  * There seems to be a chance that you could get multiple indicators
    for the same user showing up at the same time. For instance if
    someone logged on, and then sent you a message before that indicator
    had expired. Making the indicators unique per contact would seem
    to be sensible.

  * It uses the stock_person icon for those that don't have an avatar,
    but doesn't connect to icon theme changes.

The two changes that are made that aren't only enabled when libindicate
is available are:

  * Don't do anything with the status icon if it isn't visible,
    and don't iconify the main window if it isn't. The former is
    to avoid a crash, the latter to avoid window managers
    animating the iconify to the taskbar entry (at least compiz), because
    the taskbar entry is also disappearing, and conceptually
    the window is now inside the indicator applet, and we don't know
    where that is to associate with it.

  * If the notification server doesn't support actions then don't
    inhibit updates. This might be a bit heavy-handed. The issue
    is that the indicator code wants the updates so that it can
    keep the indicator up-to-date, so the notification code inhibiting
    it for everyone is a bit selfish. Fixing this by having the
    notification code ignore the event itself may work, but I wasn't
    sure about the original behaviour anyway, so just took the quick
    route for now.

I presume you would prefer to review the changes in broken up patches
when the time comes? When I find the time to fix the problems then I
can do that.

Thanks,

James

Created attachment 132158 [details] [review]
Tweaked first version of patch

Oops, a slightly better patch, which also makes the login
indicator much less broken.

Thanks,

James

libindicate is a Ubuntu-specific thing, this is not the right place to dicuss such changes.

Bruce,

libindicate is not Ubuntu-specific, anyone can install it.

True, Ubuntu is the only distro to package it so far
(that I know of at least), but that doesn't mean that
it is Ubuntu-specific.

Thanks,

James

Bruce,

Also, after we get some real world testing and a QT version written we hope that the indicator interface can become a Freedesktop.org thing.  That has always been our goal.  The more testing and issues that we can find (thanks James!) and fix before it becomes a difficult to change spec the better!  I'm very excited to find these before hand.

Thanks,
Ted

Is there a discussion somewhere to propose libindicate as external dep of GNOME 2.28? I think it's the very first step, I don't think it's wise to accept such patch before getting feedback from the GNOME community.

(In reply to comment #9)
> Bruce,
> 
> Also, after we get some real world testing and a QT version written we hope
> that the indicator interface can become a Freedesktop.org thing.  That has
> always been our goal.  The more testing and issues that we can find (thanks
> James!) and fix before it becomes a difficult to change spec the better!  I'm
> very excited to find these before hand.

Hi Ted,

The indicator seems useful and a good idea, and I'm glad you plan to make it a FreeDesktop standard. I think it could already benefit from a web page because it's hard to find out anything about it. I recommend you register a FreeDesktop project; you can submit the interface as a standard later.

However, I am disappointed that you've worked on this from scratch rather than working on improving the existing notifications interface. It seems like there's a huge overlap between the two efforts. I'd like to know more about how to two efforts co-exist. How does program support for the two standards interact?

(In reply to comment #10)
> Is there a discussion somewhere to propose libindicate as external 
> dep of GNOME 2.28? I think it's the very first step, I don't think 
> it's wise to accept suchpatch before getting feedback from the GNOME
> community.

There isn't currently.  My understanding of the GNOME release process is that the release team prefers to see a library used as a configure time option of several projects before accepting it as a dependency.  This removes the "I built a library that sounds good but no one really uses" problem of adding libraries to the platform.  For instance, libnotify has been used by many programs before it was a blessed dependency for 2.26.

(In reply to comment #11)
> However, I am disappointed that you've worked on this from scratch
> rather than working on improving the existing notifications interface.
> It seems like there's a huge overlap between the two efforts. I'd 
> like to know more about how to two efforts co-exist. How does program 
> support for the two standards interact?

That's a good point.  The interaction for the two is similar, but we felt it was different enough to warrant a distinct interface.  It's likely that application could, and would, use both.  But, the interaction model is different.

For notifications they are typically information that is sent the user that is useful if the user is sitting at the computer, and reading it right then, but doesn't have a long term impact.  Many programs actually use notifications in a "fire and forget" type of way, some even using 'notify-send' to create their notifications.

Indicators on the other hand are designed more to give the current status of the application.  I've got 10 unread messages.  20 people wanting to talk to me.  These are states instead of changes of state which is what a notification is.  So the way that a program would handle these objects is distinctly different than how they'd handle the notifications.

So, in a nutshell, it seemed that the different interaction models from the application's perspective justified creating two different interfaces.  There is some overlap, we could save a few bytes of memory by consolidating them, but it seemed like that'd create more confusion in the API than it'd really help application developers.

Created attachment 135876 [details] [review]
Adds message indicator

I updated the patch to apply to trunk

Thanks for your patch. Few comments:

	libindicate support.........:  yes
should be in the Features section. I'd prefer a more comprehensive message; something like "blabla (libindicate)".

empathy_window_iconify: why did you add this test? Some comments would be good.

Your patch doesn't contain new files: empathy-indicator-manager.[ch] empathy-indicator-manager.[ch]

Any chance you could submit a git branch?

Created attachment 135940 [details] [review]
Adds message indicator

Updated patch, puts the preference into the notifications tab

Also defaults the setting to on, but is ignored if the indicator isn't available

Created attachment 135957 [details] [review]
Adds message indicator

OK, this one is really good to go.  Struggling with git, the previous patch was missing some added files.

Could you please push your git branch somewhere? That would make testing and review lot easier for us.

I built this branch with libindicate support but the new option didn't appear in the preferences dialog.

You didn't fix this point:
        libindicate support.........:  yes
should be in the Features section. I'd prefer a more comprehensive message;
something like "blabla (libindicate)".

Neither:
empathy_window_iconify: why did you add this test? Some comments would be good.

Your branch doesn't pass "make check".

Wrap > 80 lines

Makefile.am
if Empathy is built without HAVE_LIBINDICATE, you should add new files to EXTRA_DIST to make "distcheck" happy.

empathy-chat-window.c
- Add a comment explaining the type and value in the  indicators hash table.

- You should use g_hash_table_new_full and pass g_object_unref as value_destroy_func

- priv->indicator_manager and priv->indicators should be destroyed in chat_window_finalize

New files should use tp-coding style. See the Style section on http://live.gnome.org/action/info/Empathy/

I am cleaning this up, my branch is currently at git://github.com/kenvandine/empathy.git

make check now passes cleanly and make distcheck is happy

I uploaded a HTML version of these comments for easier reading:
http://people.collabora.co.uk/~cassidy/libindicator%20review.html

• Please rebase on top of master and then fix "make check"
• Wrap > 80 lines
• Is there any doc about the libindicate API?

empathy.schemas.in
• remove the trailing space

empathy-ui-utils.c
• comment your change in empathy_window_iconify

empathy-chat-windows
• EmpathyChatWindowPriv: document the type of the key and the value	
• chat_window_finalize: you have to use g_hash_table_unref instead of g_object_unref on indicators (leads to crashes!!!)
• empathy_chat_window_init: priv->indicators should be created using g_hash_table_new_full (g_direct_hash, g_direct_equal, NULL, g_object_unref). That way you don't have to explicitely unref the indicactor when you remove it from the hash table
• chat_window_add_indicator: why are you reffing the indicator when adding it to the hash table. You just create a new one. Who borrow the first reference?
• chat_window_indicator_activate_cb: why are you unreffing the indicator here?
• Is the activate signal garanteed to be always fire once and only once?

src/Makefile.am
• you should add your files to $empathy_handwritten_source not $empathy_SOURCES (for coding style checks, etc)


empathy-indicator-manager
• empathy_indicator_manager_add_indicator  I'd rename it _create_indicator
• Both files (.c and .h) should contain an header with the licence, etc
• EMPATHY_INDICATOR_MANAGER_H should  be __EMPATHY_INDICATOR_MANAGER_H__
• Your coding style is wrong. New files should use the Telepathy coding style see http://telepathy.freedesktop.org/wiki/Style
∘ Don't use tabs
∘ function style is foo () not foo()
∘ function declarations and definitions are wrong
∘ don't mix variables declaration and code
∘ we tend to use "if (ptr != NULL)" instead of "if (ptr)"
• empathy_indicator_manager_add_login_indicator: why are you reffing the e_indicator you just created?
• empathy_indicator_manager_add_indicator: we usually don't check the return value of g_slice_new0
• I prefer having login_data_{new,free} functions
• What is this "login indicate" ?
• EmpathyIndicatorManagerPriv: document the type key and values of login_timeouts
• empathy_indicator_manager_add_login_indicator: use a #define instead of hardcoding 15 seconds
• empathy_indicator_manager_add_login_indicator: Why are you using a GValue to store the wrapped_timeout?
• empathy_indicator_manager_add_login_indicator: use tp_g_value_slice_new_uint (and tp_gvalue_slice_free when freeing the GValue) (but I don't think you need to use a GValue)
• empathy_indicator_manager_init: use g_hash_table_new_full and pass GDestroyNotify functions
• empathy_indicator_manager_set_server_visible: tell me more about this crash. How can we fix it?
• indicator_manager_finalize: don't g_object_unref hash table!!!!
• empathy_indicator_manager_init: don't harcode path. This won't work if Empathy is installed to another path
• indicate_login_timeout: you don't do anything with the indicate_login_timeout
• indicator_manager_event_updated_cb: remove "ret" and early return once you found the right element
• indicator_manager_event_removed_cb: same here
• indicator_manager_event_added_cb: empathy_indicator_new is not suppose to return NULL, no need to check (or then use g_assert)
• rename free_indicator_event  to indicator_event_free and create a indicator_event_new function (and use g_slice_new instead of g_malloc)
• indicator_manager_finalize: the indicator_events list is not freed


empathy-indicator
• TODO
• Both files (.c and .h) should contain an header with the licence, etc
• EMPATHY_INDICATOR_H should be __EMPATHY_INDICATOR_H__
• same comments about coding style
• empathy_indicator_constructor: doesn't do anything, you can remove it
• empathy_indicator_update: do you really need to dup the string passsed to indicate_indicator_set_property? That seems weird API to me
• empathy_indicator_new: this function should be a simple wrapper around g_object_new. Most of its code should be moved to _init or _constructed
• empathy_indicator_finalize: unrefing should be done in dispose, not finalize

empathy-main-window
• no need to include twice empathy-indicator-manager.h
• window->indicator_manager is never unreffed

empathy-status-icon
• prototype of indicate_server_activate_cb is useless. remove it
• notification_server_supports_actions: please comment this function; it's not clear what it's doing
• notification_server_supports_actions: use tp_strdiff instead of strcmp
• status_icon_notification_closed_cb: why did you remove the comment ?
• status_icon_set_use_libindicate: why are you hidding the status icon?
• Your changes in this file are not clear. Please comment
• priv->indicator_manager is never unreffed

Hi Guillaume,

Thanks for the detailed review, I've tried to address
most comments in

   	 git://github.com/james-w/empathy.git

but I don't think it's worth you reviewing it again yet.

Ken, has the libindicate API changed since I started this?

Thanks,

James

P.S. responses while still fresh in my head:

• Please rebase on top of master
done
 and then fix "make check"
not done
• Wrap > 80 lines
mostly done
• Is there any doc about the libindicate API?
I'm not sure, Ken?

empathy.schemas.in
• remove the trailing space
Done

empathy-ui-utils.c
• comment your change in empathy_window_iconify
done

empathy-chat-windows
• EmpathyChatWindowPriv: document the type of the key and the value  
done 
• chat_window_finalize: you have to use g_hash_table_unref instead of
g_object_unref on indicators (leads to crashes!!!)
Couldn't find this code in the latest?
• empathy_chat_window_init: priv->indicators should be created using
g_hash_table_new_full (g_direct_hash, g_direct_equal, NULL, g_object_unref).
That way you don't have to explicitely unref the indicactor when you remove it
from the hash table
Done (I think)
• chat_window_add_indicator: why are you reffing the indicator when adding it
to the hash table. You just create a new one. Who borrow the first reference?
No idea, fixed. I'm rather new at gtk in C, so you will have to forgive these
simple mistakes.
• chat_window_indicator_activate_cb: why are you unreffing the indicator
here?
fixed.
• Is the activate signal garanteed to be always fire once and only once?
no, probably needs a fix.

src/Makefile.am
• you should add your files to $empathy_handwritten_source not
$empathy_SOURCES (for coding style checks, etc)
done


empathy-indicator-manager
• empathy_indicator_manager_add_indicator  I'd rename it _create_indicator
done
• Both files (.c and .h) should contain an header with the licence, etc
done
• EMPATHY_INDICATOR_MANAGER_H should  be __EMPATHY_INDICATOR_MANAGER_H__
done
• Your coding style is wrong. New files should use the Telepathy coding style
see http://telepathy.freedesktop.org/wiki/Style
∘ Don't use tabs
∘ function style is foo () not foo()
∘ function declarations and definitions are wrong
∘ don't mix variables declaration and code
∘ we tend to use "if (ptr != NULL)" instead of "if (ptr)"
done (I think)
• empathy_indicator_manager_add_login_indicator: why are you reffing the
e_indicator you just created?
fixed.
• empathy_indicator_manager_add_indicator: we usually don't check the return
value of g_slice_new0
fixed.
• I prefer having login_data_{new,free} functions
done (I think)
• What is this "login indicate" ?
commented. It's a timed indicator when a contact comes online.
• EmpathyIndicatorManagerPriv: document the type key and values of
login_timeouts
forgot to do this
• empathy_indicator_manager_add_login_indicator: use a #define instead of
hardcoding 15 seconds
done
• empathy_indicator_manager_add_login_indicator: Why are you using a GValue
to store the wrapped_timeout?
No idea, still needs to be fixed
• empathy_indicator_manager_add_login_indicator: use
tp_g_value_slice_new_uint (and tp_gvalue_slice_free when freeing the GValue)
(but I don't think you need to use a GValue)
ditto
• empathy_indicator_manager_init: use g_hash_table_new_full and pass
GDestroyNotify functions
ditto
• empathy_indicator_manager_set_server_visible: tell me more about this
crash. How can we fix it?
It's a crash in gbus-glib or dbus itself, perhaps caused by API misuse.
Can be reproduced with a simple libindicate program, so not the fault of
empathy.
• indicator_manager_finalize: don't g_object_unref hash table!!!!
fixed
• empathy_indicator_manager_init: don't harcode path. This won't work if
Empathy is installed to another path
yeah, but I couldn't find an existing define to use, pulled to a #define
at the top of the file for now, still needs fixing properly.
• indicate_login_timeout: you don't do anything with the
indicate_login_timeout
I'm not sure what you mean by this.
• indicator_manager_event_updated_cb: remove "ret" and early return once you
found the right element
done
• indicator_manager_event_removed_cb: same here
done
• indicator_manager_event_added_cb: empathy_indicator_new is not suppose to
return NULL, no need to check (or then use g_assert)
done
• rename free_indicator_event  to indicator_event_free and create a
indicator_event_new function (and use g_slice_new instead of g_malloc)
done
• indicator_manager_finalize: the indicator_events list is not freed
done

empathy-indicato
• TODO
?
• Both files (.c and .h) should contain an header with the licence, etc
done
• EMPATHY_INDICATOR_H should be __EMPATHY_INDICATOR_H__
done
• same comments about coding style
done
• empathy_indicator_constructor: doesn't do anything, you can remove it
done
• empathy_indicator_update: do you really need to dup the string passsed to
indicate_indicator_set_property? That seems weird API to me
fixed (assuming the API isn't strange)
• empathy_indicator_new: this function should be a simple wrapper around
g_object_new. Most of its code should be moved to _init or _constructed
done (I think)
• empathy_indicator_finalize: unrefing should be done in dispose, not
finalize
done (I think)

empathy-main-window
• no need to include twice empathy-indicator-manager.h
done
• window->indicator_manager is never unreffed
done

empathy-status-icon
• prototype of indicate_server_activate_cb is useless. remove it
done
• notification_server_supports_actions: please comment this function; it's
not clear what it's doing
done
• notification_server_supports_actions: use tp_strdiff instead of strcmp
done (I think)
• status_icon_notification_closed_cb: why did you remove the comment ?
oops
• status_icon_set_use_libindicate: why are you hidding the status icon?
commented.
• Your changes in this file are not clear. Please comment
done
• priv->indicator_manager is never unreffed
done

enough for one day

Created attachment 143207 [details] [review]
Adds message indicator

Fixes:
- make check
- coding style fixes
- works with head

Created attachment 143322 [details] [review]
Adds message indicator

Fixed up ref/unref, use of hash table, correct gobject destruction

Created attachment 143414 [details] [review]
Adds message indicator

Fixes support for contact icons, builds with make check

Comment on attachment 143414 [details] [review]
Adds message indicator

Now that Empathy is living in the bring MC5 future, this should be done properly as an external application implementing an approver.

Ken is working on porting the indicator to be a proper Observer and Approver so this is not needed any more.