[pulseaudio-discuss] [PATCH] desktop-notifications: Add the initial version of the desktop notifications module.
Colin Guthrie
gmane at colin.guthr.ie
Tue Jun 26 11:50:52 PDT 2012
Hi Ștefan
Sorry for the late reply.
Things are generally good and I've got a few comments below about how I
think things should go (above and beyond your patch) and some stylistic
comments about the code itself.
'Twas brillig, and Ștefan Săftescu at 22/06/12 02:33 did gyre and gimble:
> Hello. This is the patch I promised to submit early this week. I took a few days
> longer than expected, but here it is.
>
> The code will need some refactoring, as it is currently in one (relatively) big
> file. It shouldn't be so difficult once I decide what the general structure of
> the module will be.
>
> There are a few details that need to be sorted out now:
>
> 1. What actions should the user be able to invoke on the displayed notification?
>
> My suggestion is "Yes", "No", "Ask me later" (which is equivalent to ignoring
> the notification) as answers to the question "Would you like to set [this
> card] as default?". Do tell me if you have other ideas.
This is an interesting question. There will likely be some HIG about
whether "Ask me later" is a question that should be asked explicitly or
simply not asked (allowing whatever notification bubble or dialog is
shown to simply be closed), but this is a rather small point.
I suspect that the questions to be asked will ultimately need to be
fairly flexible rather than simple Yes/No questions.
For example, if I plug in a new USB Headset of pair a Bluetooth Headset,
I'd quite like the questions to be intelligent:
e.g.
/---------------------------------------------------------------------\
| X |
| |
| A new Headset has been detected. Would you like to use this device? |
| |
| /-----------------\ /---------------------------\ /--------------\ |
| | Use as default | | Use for VoIP applications | | Not just now | |
| \-----------------/ \---------------------------/ \--------------/ |
\---------------------------------------------------------------------/
(the above is an example of why I should not be allowed to do any UI stuff!)
Notice the contextual question in the middle. Because it's a Headset we
ask the user if they want to use it for VoIP applications as this is
quite a common desire. I will talk a little bit below about the
infrastructure needed for this to work (which I do not expect you to
work on - I have said for a long time I'll do it but I've just not found
the time and motivation to start - still a lot of plans have been made
and it's had a lot of thought put into it already).
Different questions might be asked of different types of devices. e.g.
for an Apple Airport device, it may ask "Use for Music applications" and
a HDMI device might ask "Use for Movies" (although this latter example
may be less of an obvious choice).
Regardless, I think the infrastructure in place to cope with this stuff
should allow this kind of flexibility if at all possible.
> 2. How should the module behave when there two cards that have been previously
> set as default? E.g.: the user has already previously set cards A and B as
> default. Currently, the system has card A plugged in and the user inserts
> card B. Should card B set as default? In what circumstances? If the user has
> set A and B as default independently, it's not really possible to determine
> which card the user prefers. However, if the user has set card B as default
> while card A was already plugged in, then the user probably prefers B over A.
>
> This, unfortunately, leads to some overly complicated relationships between
> cards, which would probably require some sort of UI for editing. That is why
> I suggest that, for the moment at least, the module should always set as
> default the latest plugged in card, so long as the user had already
> indicated it wants it as default.
This shouldn't be a something you should worry about for now, but your
input and thoughts on the topic would be most welcome in another thread
relating to priority lists more generally (see a link I'll post below).
To give a (brief) background, I have a plan to introduce priority list
routing in the core of PA. There already exists a module
(module-device-manager when loaded with the do_routing=1 argument) that
will implement a priority list of devices. The action of "setting a
default device" simply moves the device in question to the top of the
priority list. At present there are several, separate priority lists:
one per "role" (music, video, voip, a11y, notifications) and an overall
default list if no role is provided.
Thus this system should abstract away any questions you ask about how to
handle this. You simply set it as default and presume the rest of the
system will take care of things for you :)
There will be a lot more to priority lists than just the above -
including options to automatically change profile etc. (which will
require that streams have a priority associated with them so as to make
the choices deterministic!), but I want to try and keep this reply on
topic for now. For more background:
http://www.freedesktop.org/wiki/Software/PulseAudio/RFC/PriorityRouting
Also please feel free to discuss this any time with me on IRC.
As a small addendum to the above reply, I would also state that I think
this popup question should be asked only the first time a new device is
detected. It should be up to the desktop itself to buffer the
notification and allow the user to go back to it, or to provide
alternative mechanisms of configuring the devices through a
mixer/configuration interface. If the device is "known" to PA, then it
should no longer present such notifications when the card appears
(however, this could, if you like, be a module option that is off by
default - e.g. always_ask_user=1 or similar).
Again, as the priority list scheme will deal with storing history for a
device, I will propose to ensure that we set a property on the device
(i.e. in it's proplist) that indicates if we've never seen it before, or
if it has been plugged in previously.
> 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.
Hopefully that's addressed your specific questions, but if I've missed
something or you need clarification or you have some follup questions,
please do just ask :)
Now some comments on the code:
> diff --git a/src/modules/module-desktop-notifications.c b/src/modules/module-desktop-notifications.c
> new file mode 100644
> index 0000000..96ab4e5
> --- /dev/null
> +++ b/src/modules/module-desktop-notifications.c
> @@ -0,0 +1,206 @@
> +/***
> + This file is part of PulseAudio.
> +
> + Copyright 2012 Ștefan Săftescu
> +
> + PulseAudio is free software; you can redistribute it and/or modify
> + it under the terms of the GNU Lesser General Public License as published
> + by the Free Software Foundation; either version 2.1 of the License,
> + or (at your option) any later version.
> +
> + PulseAudio is distributed in the hope that it will be useful, but
> + WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public License
> + along with PulseAudio; if not, write to the Free Software
> + Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
> + USA.
> +***/
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <dbus/dbus.h>
> +
> +#include <pulse/proplist.h>
> +#include <pulse/xmalloc.h>
> +
> +#include <pulsecore/card.h>
> +#include <pulsecore/core.h>
> +#include <pulsecore/core-util.h>
> +#include <pulsecore/dbus-shared.h>
> +#include <pulsecore/dbus-util.h>
> +#include <pulsecore/log.h>
> +#include <pulsecore/macro.h>
> +#include <pulsecore/module.h>
> +
> +#include "module-desktop-notifications-symdef.h"
> +
> +PA_MODULE_AUTHOR("Ștefan Săftescu");
> +PA_MODULE_DESCRIPTION("Integration with the Desktop Notifications specification.");
> +PA_MODULE_VERSION(PACKAGE_VERSION);
> +PA_MODULE_LOAD_ONCE(TRUE);
> +
> +typedef struct pa_notification pa_notification;
> +
> +struct userdata {
> + pa_hook_slot *card_put_slot;
> +};
As said above, you maybe want to use the sink and source put slots, but
ultimately this isn't a major issue either way from the general
structure of the code (i.e. it shouldn't be crazy difficult to refactor
it to sinks+sources later). And again, this is only my opinion - there
could be very valid reasons other folk can offer to keep the trigger
based on cards.
> +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!
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);
> +
> + 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);
> +
> + 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);
> +
> + 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.
Another thing to consider is the use of libnotify rather than using dbus
directly. It might not be appropriate to do so, but it would be good if
you take a look at it and evaluate whether it makes more sense to do it
via libnotify API or via dbus and document your findings on this process.
I hope this has been useful feedback generally, but if not please poke
me and I'll try again :D Apologies for not knowing too much about the
dbus stuff, but then finding this stuff out is part of the fun too :)
I'll ask some folks more involved in UI stuff to see if they can comment
(at a high level) as to how user interaction might want to work.
Keep up the good work :)
Col
--
Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/
Day Job:
Tribalogic Limited http://www.tribalogic.net/
Open Source:
Mageia Contributor http://www.mageia.org/
PulseAudio Hacker http://www.pulseaudio.org/
Trac Hacker http://trac.edgewall.org/
More information about the pulseaudio-discuss
mailing list