ayatana-commits team mailing list archive

[Ted Gould <ted@xxxxxxxx>]

On Tue, 2010-03-16 at 20:51 +0000, David Barth wrote:
> +/* Creating an instance of the status provider.  We set the variables
> +   and create an TpAccountManager object.  It does all the hard
> +   work in this module of tracking MissionControl and enumerating the
> +   accounts and all that jazz. */

Should fix this comment.

> +/* Unref the account manager and move on.  Sadly, we're
> +   leaving the show. */

This one too :)

> +/* Watch for Gwibber coming on and off the bus. */
> +static void
> +dbus_namechange (DBusGProxy * proxy, const gchar * name, const gchar * prev, const gchar * new, GwibberService * self)
> +{
> +	g_return_if_fail (IS_GWIBBER_SERVICE (self));
> +
> +	GwibberServicePrivate * priv = GWIBBER_SERVICE_GET_PRIVATE(self);
> +
> +	if (prev[0] == '\0' && g_strcmp0(name, GWIBBER_ADDRESS) == 0) {
> +		g_debug("Gwibber Service coming online");
> +		priv->status = GWIBBER_SERVICE_STATUS_RUNNING;
> +		if (priv->service_proxy == NULL)
> +			setup_service_proxy (self);
> +		query_account_manager (self);
> +	}
> +
> +	if (new[0] == '\0' && g_strcmp0(name, GWIBBER_ADDRESS) == 0) {
> +		g_debug("Gwibber Service going offline");
> +		priv->status = GWIBBER_SERVICE_STATUS_NOT_RUNNING;
> +		g_signal_emit (G_OBJECT (self), GWIBBER_SERVICE_SIGNAL_STATUS_CHANGED_ID, 0, priv->status, TRUE);
> +	}

Should you not be cleaning up the service_proxy here?  Setting it to
NULL and all that?

> +static void
> +get_accounts_response (DBusGProxy * proxy, DBusGProxyCall * call, gpointer data)
> +{
> +	GError *error = NULL;
> +	gchar  *accounts_string = NULL;
> +
> +	dbus_g_proxy_end_call (proxy, call, &error,
> +						   G_TYPE_STRING, &accounts_string,
> +						   G_TYPE_INVALID);
> +
> +	if (error != NULL) {
> +		g_error_free (error);
> +		g_warning ("GetAccounts: %s", error->message);
> +		return;
> +	}
> +
> +	/* don't print the accounts string, it contains passwords */
> +	/* g_debug ("GetAccounts: %s", accounts_string); */
> +
> +	g_return_if_fail (IS_GWIBBER_SERVICE (data));
> +	GwibberServicePrivate * priv = GWIBBER_SERVICE_GET_PRIVATE(data);
> +	g_return_if_fail (priv != NULL);
> +
> +	priv->status = GWIBBER_SERVICE_STATUS_RUNNING;
> +	priv->has_configured_accounts = g_strrstr (accounts_string, " \"send_enabled\": true,") ? TRUE : FALSE;
> +
> +	/* trigger a status update */
> +	g_signal_emit (G_OBJECT (data),
> +				   GWIBBER_SERVICE_SIGNAL_STATUS_CHANGED_ID,
> +				   0, priv->status, TRUE);
> +
> +	return;
> +}

You'll need to free accounts_string.  It seems like setting the status
in this function isn't good.  It should already be set, eh?

> +static void
> +setup_service_proxy (GwibberService *self)
> +{
> +	g_return_if_fail (IS_GWIBBER_SERVICE (self));
> +
> +	GwibberServicePrivate * priv = GWIBBER_SERVICE_GET_PRIVATE(self);
> +
> +	DBusGConnection * bus = dbus_g_bus_get(DBUS_BUS_SESSION, NULL);
> +	g_return_if_fail(bus != NULL); /* Can't do anymore DBus stuff without this,
> +	                                  all non-DBus stuff should be done */
> +
> +	if (priv->service_proxy == NULL) {
> +		priv->service_proxy = dbus_g_proxy_new_for_name(bus,
> +														GWIBBER_ADDRESS,
> +														GWIBBER_OBJECT,
> +														GWIBBER_INTERFACE);
> +	}
> +}

Ah, so you're not using an owner proxy, which invalidates my last
comment.  But it does make me wonder why you didn't just build it at
init then.  Seems like it'll always just hang out anyway.

> +  static GwibberService *singleton = NULL;
> +
> +  if (! singleton) {
> +	  singleton = gwibber_service_new ();
> +  }

Seems like just having the singleton in the function isn't going to work
great.  If you're going to have a singleton, probably best to make it
global to the file and have everyone use it.

-- 
https://code.launchpad.net/~dbarth/indicator-me/gwibber-service/+merge/21490
Your team ayatana-commits is subscribed to branch lp:indicator-me.