GNOME Bugzilla – Bug 764083
Add guards to methods in BaseItem which do not operate on collections
Last modified: 2016-04-05 09:47:43 UTC
Some methods are undefined when the BaseItem refers to a collection. We should add a g_return_if_fail guard in such methods. Some of the probable candidates are: * photos_base_item_create_preview * photos_base_item_get_bbox_edited * photos_base_item_get_bbox_source * photos_base_item_get_surface * photos_base_item_get_width * photos_base_item_load_async * photos_base_item_open * photos_base_item_operation_add * photos_base_item_operation_get * photos_base_item_operation_remove * photos_base_item_operations_revert * photos_base_item_pipeline_save_async * photos_base_item_pipeline_snapshot * photos_base_item_print * photos_base_item_process_async * photos_base_item_save_async * photos_base_item_save_guess_sizes_async
Created attachment 324947 [details] [review] base-item: add BaseItem checks for public methods
Created attachment 324948 [details] [review] base-item: add checks for functions which do not work on collections
Review of attachment 324947 [details] [review]: Thanks for the patch, Rafael. ::: src/photos-base-item.c @@ +2206,3 @@ va_list ap; + g_return_if_fail (PHOTOS_IS_BASE_ITEM (self)); Should be g_return_val_if_fail.
Created attachment 325088 [details] [review] base-item: Add PHOTOS_IS_BASE_ITEM guards in public functions Fixed and pushed
Review of attachment 324948 [details] [review]: ::: src/photos-base-item.c @@ +1697,2 @@ g_return_val_if_fail (PHOTOS_IS_BASE_ITEM (self), FALSE); + g_return_val_if_fail (photos_base_item_is_collection (self), FALSE); Shouldn't it be the other way round? ie.: g_return_val_if_fail (!photos_base_item_is_collection (self), FALSE); Also, I prefer to directly check priv->collection as we do with the other member variables. @@ +1749,2 @@ g_return_val_if_fail (PHOTOS_IS_BASE_ITEM (self), NULL); + g_return_val_if_fail (photos_base_item_is_collection (self), NULL); Ditto. @@ +1889,2 @@ g_return_val_if_fail (PHOTOS_IS_BASE_ITEM (self), FALSE); + g_return_val_if_fail (photos_base_item_is_collection (self), FALSE); Ditto, etc..
Created attachment 325092 [details] [review] base-item: add checks for functions which don't work on collections
Review of attachment 325092 [details] [review]: ::: src/photos-base-item.c @@ +1697,2 @@ g_return_val_if_fail (PHOTOS_IS_BASE_ITEM (self), FALSE); + g_return_val_if_fail (!self->priv->collection, FALSE); Better to move this after the "priv = self->priv". It is a purely nitpicky thing at the moment, but when we start using G_DECLARE_DERIVABLE_TYPE, we will no longer have a separate priv pointer in the _PhotosBaseItem structure. We will have to use the generated photos_base_item_get_instance_private function to get the priv pointer. So things like self->priv->... won't work any more. Modern GObject best practices recommend against having the priv pointer in the structure because it wastes one pointer for each type in the hierarchy multiplied by the number of instances. It is argued that the generated function only does pointer arithmetic and likely to get inlined, and hence a better speed vs. memory trade-off.
Created attachment 325100 [details] [review] base-item: Add guards for functions which do not work on collections Fixed and pushed.
I see that there are a few more public methods without the PHOTOS_IS_BASE_ITEM guards. Are you planning to add those too? Or do you want to close this bug?
Created attachment 325101 [details] [review] base-item: Add guard in remaining functions
Review of attachment 325101 [details] [review]: Looks good to me.