Created attachment 278699 [details] [review]
base-item: Mark it as an abstract type

Created attachment 278700 [details] [review]
Turn the PhotosBaseItem sub-classes into extensions

Created attachment 278755 [details] [review]
item-manager: Make use of extensions to create item

Fixes: https://bugzilla.gnome.org/731865

Review of attachment 278700 [details] [review]:

What if we provide the macro for extension point names instead of hard coding the string ?

(In reply to comment #4)
> Review of attachment 278700 [details] [review]:
> 
> What if we provide the macro for extension point names instead of hard coding
> the string ?

Yes, we should eventually do something about that. The reason I did not do it now is that we use strings like "flickr" all over the codebase for all sorts of reasons, and I was not sure what would be the best way to do it that would address most of these cases.

Maybe you have some ideas.

Otherwise we can do that once we have addressed the bigger issues one by one.

Review of attachment 278699 [details] [review]:

OK

Comment on attachment 278699 [details] [review]
base-item: Mark it as an abstract type

Thanks for the review, Pranav.

Review of attachment 278700 [details] [review]:

look ok.

Review of attachment 278755 [details] [review]:

Thanks for the patch, Pranav. This is exactly what I had in mind. Some minor comments.

::: src/photos-facebook-item.c
@@ -279,3 @@
-                       "failed-thumbnailing", FALSE,
-                       NULL);
-}

You should also remove the function prototypes from the headers.

::: src/photos-flickr-item.c
@@ -381,3 @@
-                       "failed-thumbnailing", FALSE,
-                       NULL);
-}

Ditto.

::: src/photos-item-manager.c
@@ +38,3 @@
 #include "photos-tracker-change-event.h"
 #include "photos-tracker-change-monitor.h"
+#include "photos-utils.h"

You removed an extra newline.

@@ +310,3 @@
   gchar *identifier = NULL;
+  gchar **split_identifier = NULL;
+  gchar *extension_name = NULL;

extension_name should be a 'const gchar *' because it is not supposed to be freed. Nothing wrong with what you have done. It is just a matter of convention in GLib code / API.

@@ +316,2 @@
   identifier = g_strdup (tracker_sparql_cursor_get_string (cursor, PHOTOS_QUERY_COLUMNS_IDENTIFIER, NULL));
+  extension_name = "local";

You could just initialize it to "local" instead of NULL in the definition above.

@@ +334,3 @@
 
+ final:
+  extension_point = g_io_extension_point_lookup (PHOTOS_BASE_ITEM_EXTENSION_POINT_NAME);

Instead of doing it here, I think it would be better to have a priv->extension_point which is assigned to in photos_item_manager_init. Even though g_io_extension_point_lookup is only as expensive as a hashtable lookup, photos_item_manager_create_item will be called for each item in the application. So it is better to keep it as lean as possible.

@@ +335,3 @@
+ final:
+  extension_point = g_io_extension_point_lookup (PHOTOS_BASE_ITEM_EXTENSION_POINT_NAME);
+  extension = g_io_extension_point_get_extension_by_name (extension_point, extension_name);

I think that it would be better to have a g_warning for the 'if (G_UNLIKELY (extension == NULL))' case. It shouldn't happen, unless say local items start having identifiers like 'foo:bar:baz:...' for some reason. Would be good to have some indication in the logs if that occurs.

@@ +337,3 @@
+  extension = g_io_extension_point_get_extension_by_name (extension_point, extension_name);
+  if (extension != NULL)
+    ret_val = PHOTOS_BASE_ITEM (g_object_new (g_io_extension_get_type (extension),

Can you please split the g_io_extension_get_type into a separate statement?

Created attachment 279000 [details] [review]
item-manager: Make use of extensions to create item

Fixes: https://bugzilla.gnome.org/731865

Created attachment 279022 [details] [review]
item-manager: Make use of extensions to create items

Committed after tweaking the error message to contain the name of the extension and the identifier.

Created attachment 279044 [details] [review]
properties-dialog: do item related tasks in respective subclasses.

Review of attachment 279044 [details] [review]:

Perfect.

Created attachment 279673 [details] [review]
Use ProviderName from the GoaObject whenever applicable

Review of attachment 279673 [details] [review]:

looks OK.

Comment on attachment 279673 [details] [review]
Use ProviderName from the GoaObject whenever applicable

Thanks for the review, Pranav.

Created attachment 280381 [details] [review]
src: Cleanup unnecessary inclusion of header files

Review of attachment 280381 [details] [review]:

Thanks for the patch, Saurav. Perfect!

Created attachment 285760 [details] [review]
application: Make use of extensions to run the miners

We have taken care of most of the big issues here. We can use separate bugs for further adjustments.