Code review comment for lp://staging/~xnox/ubuntu/raring/liferea/messaging-menu

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

It is roughly correct, but only very roughly ;)

You don't get a callback when the user clicks the application name any more. Instead, the app is launched again through its desktop file. This means that all applications that support the messaging menu must be single instance (which liferea is). Thus, on_indicator_server_clicked can be removed.

I recommend against hooking on_indicator_clicked up to the detailed signals only to pass the node as user data (if you're doing it like that, you would also need to disconnect the signals in the handler). Instead, hook the function up once, and lookup the node through its id in on_indicator_clicked.

The arguments to on_indicator_clicked are wrong, they should be

  static void
  on_indicator_clicked (MessagingMenuApp *mmapp, const gchar *source_id, gpointer user_data)

What did you intend to do here:

  messaging_menu_app_remove_source (indicator_priv->server, (char *)(indicator));

Casting the MessagingMenuApp pointer to a string?! You probably want 'source_id' in there.

review: Needs Fixing

« Back to merge proposal