[pulseaudio-discuss] [PATCH] desktop-notifications: Add the initial version of the desktop notifications module.

[Tanu Kaskinen]

On Tue, 2012-06-26 at 19:50 +0100, Colin Guthrie wrote:
> > 3. What is meant by "setting a card as default"?
> > 
> >    My suggestion here is to simply set the first sink and first source of the
> >    card as default.
> 
> Yeah this is a tricky one too. In the case of headsets, I think they can
> be handled as a single unit (as per my above example) but for other
> devices it might come down to handling them separately (two
> notifications) or only handling the sink side of things (as it's by far
> the more important for most users).
> 
> I'd also be somewhat wary of basing too much of the code around "cards"
> (as opposed to sinks and sources). While they are commonly used and
> probably covers 95% of what we would want this stuff for, various sinks
> and sources are created without cards (e.g. the network sinks, airtunes
> etc.). So in an ideal world, it wouldn't need a card.

Popping up dialogs based on new sinks and sources has its own problems.
If you change the card profile in pavucontrol, and it causes a new set
of sinks and sources to appear, would there be a dialog again asking
what to do with the "new" sinks and sources, even though nothing new has
actually been plugged in?

I'd say that ideally the dialog would be triggered when a new card is
seen. The various network sinks etc. should create a card object too.
Ideally questions like setting something as default should work at the
port level, not at sink and source level, because the difference between
two sinks and two ports of one sink is not very meaningful to the user.
Ports become visible immediately when a card is created, whereas sinks
and sources can appear later at pretty random times, like illustrated by
the pavucontrol example.

That said, there may be significant problems with implementing that
ideal solution currently, because there's not yet a concept of global
default port or even proper port-based routing. Therefore, I'm OK with
pretty much any compromise - you can improve things later. I just wanted
to make clear what my "ideal world" looks like, since it's conflicting
with what Colin said above.

> > +struct pa_notification {
> > +    char *app_name;
> > +    dbus_uint32_t replaces_id;
> > +    char *app_icon;
> > +    char *summary;
> > +    char *body;
> > +    char **actions;
> > +    size_t num_actions;
> > +    dbus_int32_t expire_timeout;
> > +};
> > +
> > +static pa_hook_result_t card_put_cb(pa_core *c, pa_card *card, void* userdata);
> > +static pa_dbus_pending* show_notification(DBusConnection *conn, pa_notification *n);
> > +
> > +pa_notification* pa_notification_new(size_t num_actions);
> > +void pa_notification_free(pa_notification *n);
> > +
> > +void pa_dbus_append_basic_variant_dict(DBusMessageIter *iter, const char** keys, int item_type, const void *data, unsigned n);
> 
> 
> While there is no hard and fast rule, most of the code seems to not use
> forward declarations like this - preferring instead to just define them
> inline and put the pa__init/pa__done at the end of the code. I'm not
> personally too bothered about this, so you don't need to change anything
> specifically unless you really feel it's better to remain consistent.
> 
> Also, things with the pa_* prefix on their name are typically reserved
> for concepts outside of modules. I do concede that in this case the
> pa_notification* classes will likely move to something central (i.e.
> pulsecore) eventually anyway and thus I think naming them
> pa_notification*  is sensible forward planning!

While I have a slightly different view about the pa_ prefix: it's used
everywhere except in the module-foo.c files. For example, the alsa and
bluetooth modules have separated part of their functionality into
utility libraries, and those libraries use pa_alsa_ and pa_bluetooth_
prefixes. The utility libraries are still not part of libpulsecore. They
are only linked to the modules that need them, so they can be considered
to be part of the modules.

Specifically about the pa_dbus_append_basic_variant_dict() function:
that should be added to pulsecore/dbus-util.[ch].

> Comments on the below code are going to be tricky for me as I'm pretty
> clueless about dbus, but I'll ask some questions and hopefully you can
> find the right answers. If in doubt Tanu is a good person to poke about
> dbus stuff :D
> 
> > +static pa_hook_result_t card_put_cb(pa_core *c, pa_card *card, void *userdata) {
> > +    char *card_name;
> > +    DBusError err;
> > +    pa_dbus_connection *conn;
> > +    pa_notification *n;
> > +
> > +    card_name = pa_proplist_gets(card->proplist, PA_PROP_DEVICE_DESCRIPTION);
> > +    pa_log_debug("Card detected: %s.", card_name);
> > +
> > +
> > +    dbus_error_init(&err);
> > +    conn = pa_dbus_bus_get(c, DBUS_BUS_SESSION, &err);

Connecting to the bus should be done when the module is loaded. Since
the connection is kept alive as long as there is at least one user for
it, and there are other users than your module, this probably doesn't
have any practical effect, but still... If it happened that your module
was the only user of the session bus, you would set up and tear down the
connection over and over again. Connecting to the bus is a relatively
heavy operation, and involves synchronous communication with the dbus
daemon. Synchronous communication with external processes should be
avoided due to the possible delays that it introduces.

> > +
> > +    n = pa_notification_new(0);
> > +    n->summary = "A new card has been detected.";
> > +    n->body = pa_sprintf_malloc("%s has been detected. Would you like to set it as default?", card_name);
> > +
> > +    show_notification(pa_dbus_connection_get(conn), n);
> > +
> > +    pa_notification_free(n);
> > +    pa_dbus_connection_unref(conn);
> > +    dbus_error_free(&err);
> > +
> > +    return PA_HOOK_OK;
> > +}
> 
> 
> I'm thinking that the pa_notification stuff generally should maybe
> abstract away anything related to dbus where it's used (i.e. dbus
> becomes an implementation detail of pa_notification* code).
> 
> > +static pa_dbus_pending* show_notification(DBusConnection *conn, pa_notification *n) {
> > +    DBusMessage* msg;
> > +    DBusMessageIter args;
> > +    DBusPendingCall* pending;
> > +
> > +    msg = dbus_message_new_method_call(
> > +            "org.freedesktop.Notifications",
> > +            "/org/freedesktop/Notifications",
> > +            "org.freedesktop.Notifications",
> > +            "Notify"
> > +    );
> > +
> > +    /* FIXME: free memory when there's an error? */
> > +    pa_assert_se(NULL != msg);

If you check the documentation for dbus_message_new_method_call(),
you'll see that the only possible error is that there's not enough
memory. If that happens, then it's OK to crash, so pa_assert_se() is the
right thing to do.

I'd be OK also with

    pa_assert_se(msg = dbus_message_new_method_call("org.freedesktop.Notifications", "/org/freedesktop/Notifications", "org.freedesktop.Notifications", "Notify"));

despite the fact that it's a bit long line. That's the format that is
used everywhere else too. But I don't mind the formatting that you've
done either.

I don't like "Yoda conditionals": "NULL is message" is an awkward way to
say "message is NULL".

> > +
> > +    dbus_message_iter_init_append(msg, &args);
> > +
> > +    pa_assert_se(dbus_message_iter_append_basic(&args, DBUS_TYPE_STRING, (void *) &n->app_name));
> > +    pa_assert_se(dbus_message_iter_append_basic(&args, DBUS_TYPE_UINT32, (void *) &n->replaces_id));
> > +    pa_assert_se(dbus_message_iter_append_basic(&args, DBUS_TYPE_STRING, (void *) &n->app_icon));
> > +    pa_assert_se(dbus_message_iter_append_basic(&args, DBUS_TYPE_STRING, (void *) &n->summary));
> > +    pa_assert_se(dbus_message_iter_append_basic(&args, DBUS_TYPE_STRING, (void *) &n->body));
> > +
> > +    pa_dbus_append_basic_array(&args, DBUS_TYPE_STRING, (void *) n->actions, n->num_actions);
> > +    pa_dbus_append_basic_variant_dict(&args, NULL, DBUS_TYPE_STRING, NULL, 0);
> > +
> > +    pa_assert_se(dbus_message_iter_append_basic(&args, DBUS_TYPE_INT32, (void *) &n->expire_timeout));
> > +
> > +
> > +    pa_assert_se(dbus_connection_send_with_reply (conn, msg, &pending, -1));
> > +    pa_assert_se(pending != NULL);

pa_assert_se() is used for expressions that have side effects, or when
you want to make it impossible to disable the assertion. "pending !=
NULL" doesn't have side effects, and I don't see why this would be such
a special assertion that it needs to be impossible to disable, so please
use pa_assert() instead.

> > +
> > +    dbus_connection_flush(conn);
> > +
> > +    return pa_dbus_pending_new(conn, msg, pending, NULL, NULL);
> > +}
> 
> 
> I'm not sure how dbus replies are handled, but I presume there is some
> kind of loop for dbus and some way to get the results of the user
> interaction. This could mean blocking and waiting or it could be some
> kind of async event coming back at a later time - I have no idea. But
> one thing is likely for certain: You should not block when processing
> notifications from a callback function (card_put_cb() in this case).
> 
> This might require spawning a separate thread for notifications and
> "handing over" the details of the notification to that thread which will
> in turn send all the relevant notifications and deal with the replies.
> This probably means defining some kind of callback to be called with the
> notification itself which can then take care of doing the right thing
> with the user response.

pa_dbus_bus_get() takes care of integrating the D-Bus connection with
the event loop, so there's no need for extra threads. Actually I believe
it's not entirely safe (even though it "should" be) to use the D-Bus
connection from multiple threads, so you'd have to create a
"private" (i.e. non-shared) connection for the desktop-notifications
module, which is wasteful.

The code is fine in this regard, because
dbus_connection_send_with_reply() creates a pending call, i.e. it's
asynchronous. I believe it's intentional that the reply is not yet
handled in this patch. Handling it would be only a matter of setting the
callback with dbus_pending_call_set_notify().

The code leaks pa_dbus_pending objects, however, which needs to be
fixed.

-- 
Tanu