> 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.
> 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: application( ), the new call to g_hash_ table_remove( ) added when
>
> * in remove_
> (section == NULL) should be removed.
Right, fixed in r313.
> * remove_section() is wrapped in a g_hash_ table_lookup( ) safety check when application( ), but not when called from the destroy table_lookup( ) + g_warning() from remove_ application( ) to
> called from remove_
> callback. Both this and the previous bullet point could be addressed by moving
> the g_hash_
> 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!