On 03/18/2013 03:39 PM, Tanu Kaskinen wrote:
> On Mon, 2013-03-18 at 14:00 +0100, David Henningsson wrote:
>> On 02/20/2013 07:23 PM, Tanu Kaskinen wrote:
>>> ---
>>> src/modules/dbus/iface-core.c | 173 +++++++++++++++++------------------------
>>> 1 file changed, 72 insertions(+), 101 deletions(-)
>>>
>>> diff --git a/src/modules/dbus/iface-core.c b/src/modules/dbus/iface-core.c
>>> index a9716bc..4621726 100644
>>> --- a/src/modules/dbus/iface-core.c
>>> +++ b/src/modules/dbus/iface-core.c
>>> @@ -108,9 +108,8 @@ struct pa_dbusiface_core {
>>> pa_hashmap *modules;
>>> pa_hashmap *clients;
>>>
>>> - pa_sink *fallback_sink;
>>> - pa_source *fallback_source;
>>> -
>>> + pa_hook_slot *default_sink_changed_slot;
>>> + pa_hook_slot *default_source_changed_slot;
>>> pa_hook_slot *sink_put_slot;
>>> pa_hook_slot *sink_unlink_slot;
>>> pa_hook_slot *source_put_slot;
>>> @@ -677,13 +676,12 @@ static void handle_get_fallback_sink(DBusConnection *conn, DBusMessage *msg, voi
>>> pa_assert(msg);
>>> pa_assert(c);
>>>
>>> - if (!c->fallback_sink) {
>>> - pa_dbus_send_error(conn, msg, PA_DBUS_ERROR_NO_SUCH_PROPERTY,
>>> - "There are no sinks, and therefore no fallback sink either.");
>>> + if (!c->core->default_sink) {
>>> + pa_dbus_send_error(conn, msg, PA_DBUS_ERROR_NO_SUCH_PROPERTY, "No fallback sink set.");
>>> return;
>>> }
>>
>> Is there a reason for using core->default_sink instead of
>> pa_namereg_get_default_sink?
>
> There's this reason: the property change signals should match what
> getting the property would return, at least in my opinion. If we'd call
> pa_namereg_get_default_sink() here, then the signals would indicate that
> no fallback has been set, but getting the fallback would indicate that
> there's some fallback set.
>
> It would definitely be good to have a comment about this here.
What about caching the result of pa_namereg_get_default_sink in the
beginning, and only send out a dbus signal if the result of the function
has actually changed?
>
>> E g, the native protocol (e g pactl stat) uses
>> pa_namereg_get_default_sink instead of the direct pointer.
>>
>> As for the other dbus patches (and "core: Add hooks for default device
>> changes"), I looked them briefly through and saw nothing to remark on.
>> With the disclaimer that I don't know the dbus stuff very well, but I'd
>> just say it's a very welcome fix, if it fixes the occasional crashes on
>> device unplug.
>
> OK, if you ack my explanation above, I'll push patches 1-4 (Arun wants
> to check patch 5 before I push it). Thanks for the review!
>
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
