Code review comment for lp:~larsu/indicator-sound/use-bus-watch-namespace

Revision history for this message
Lars Karlitski (larsu) wrote :

> bus-watch-namespace is a nice piece of work. I only have three issues -- a
> bug, a typo, and a question:
>
> * watcher->name_space is leaked
>
> * trivial style: ";;" used after g_cancellable_new()

Nice catches, thanks! Fixed in revisions 366 and 367.

> * the API contract is a little odd in that user_data can be
> destroyed before bus_unwatch_namespace() is called by the client.
> This isn't an issue in i-sound, which doesn't use the argument,
> but was it intentional in the contract?

Yes, it is intentional. The contract is basically: as long as we call your callbacks, your user data will be alive. Since in some cases (e.g. no bus connection) there's no way those callbacks ever get called, I just free the user data directly.

I should probably document this better, thanks for raising it.

> The changes to media-player-list.vala are straightforward and fine.

:)

« Back to merge proposal