Code review comment for lp://staging/~larsu/indicator-messages/watch-desktop-files

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

> Overall this is a good patch as usual. :)
>
> Needs Fixing:
>
> * in app-section.c, the file-scope variable destroy_count should be static

Fixed in r311. (you meant destroy_signal, right?)

> * the new function app_section_get_name() is not used anywhere, was this
> included by accident?

That's a very old function. Removed it anyway in r312.

> Comment:
>
> * in remove_application(), the new call to g_hash_table_remove() added when
> (section == NULL) should be removed.

Right, fixed in r313.

> * remove_section() is wrapped in a g_hash_table_lookup() safety check when
> called from remove_application(), but not when called from the destroy
> callback. Both this and the previous bullet point could be addressed by moving
> the g_hash_table_lookup() + g_warning() from remove_application() to
> remove_section().

I didn't do this because remove_section is called from the "destroy" signal from the AppSection. There's no need to look up the AppSection again in the hash table, because we already have it. That said, are you okay with keeping this as it is for now? I plan on refactoring thatq bit starting next cycle anyway.

Thanks for reviewing!

« Back to merge proposal