Created attachment 342571 [details] [review]
item-manager: Added condition to check if URI is NULL

`uri` of an item is not relevant in case of albums
as we depend only on tracker's urn for album's stuff.

(In reply to Umang Jain from comment #0)
> Stack trace reveals that photos_item_manager_created_executed tries to get
> `uri` of newly created item (i.e. album) but turns out to be NULL. As one
> can simply argue `uri` of an item is not relevant in case of albums as we
> depend only on tracker's urn for album's stuff.

Good catch! This is a fallout from:

commit fa1232fec2f8b7ed4511c5d744f270479b7c6f96
Author: Debarshi Ray <debarshir@gnome.org>
Date:   Thu Sep 8 14:17:38 2016 +0200

    item-manager: Add photos_item_manager_wait_for_changes_async
    
    This is meant to let us work around the URN possibly changing due to
    an atomic file update. It will wait for any relevant changes and return
    the new URN, if any. This is only needed for LocalItems since the
    other BaseItem sub-classes are not backed by a file on the local hard
    disk.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=770267
    https://bugzilla.gnome.org/show_bug.cgi?id=771042

Review of attachment 342571 [details] [review]:

Thanks for the patch, Kartikeya! Some comments below:

::: src/photos-item-manager.c
@@ +186,3 @@
   uri = tracker_sparql_cursor_get_string (cursor, PHOTOS_QUERY_COLUMNS_URI, NULL);
+  if (uri != NULL)  /*in case of new album uri is NULL*/
+    photos_item_manager_check_wait_for_changes (self, id, uri);

It will be nicer to make these three lines conditional on photos_item_manager_cursor_is_collection. There is another photos_item_manager_check_wait_for_changes call later in the file. For that you will have to use photos_base_item_is_collection.

That should fix this particular crash, but we should make the photos_item_manager_wait_for_changes_async code more robust. In two separate patches, you can do:

(a) Make photos_item_manager_wait_for_changes_async return early if it is a collection. See how we return early if it is not a local item.

(b) Add a 'g_return_if_fail (uri != NULL && uri[0] != '\0');' at the beginning of photos_item_manager_check_wait_for_changes. Same for 'id'. That is basically an assertion which will log a CRITICAL if we don't handle collections correctly. We should never see the CRITICAL if our code is correct.

Created attachment 343260 [details] [review]
item-manager: Added conditions to check if item is collection.

before calling photos_item_manager_wait_for_changes_async we should check
if the item is collection, in that case we don't need to call this function.

Created attachment 343261 [details] [review]
item-manager: Added conditions to check if item is collection.

before calling photos_item_manager_wait_for_changes_async we should check
if the item is collection, in that case we don't need to call this function.

Created attachment 343263 [details] [review]
item-manager: Added assertions at the the start of check_wait_for_changes.

these assertions will fail if we don't handle collections correctly.

Review of attachment 343261 [details] [review]:

Thanks for the new patch!

The summary of the commit message doesn't end in a full-stop, but the body should start with a capital letter. Secondly, we should describe the motivation behind the change, instead of literally describing the changes line by line. eg., 'item-manager: Don't look for collections in wait_for_changes_table' could be a good summary. You can use the body to further explain why we don't have collections in the wait_for_changes_table GHashTable.

::: src/photos-item-manager.c
@@ +190,3 @@
     }
 
+  if (cursor == NULL || photos_item_manager_cursor_is_collection (cursor))

The _is_collection check should only cover the 3 statements involved in photos_item_manager_check_wait_for_changes.

@@ +247,2 @@
       object = photos_base_manager_get_object_by_id (PHOTOS_BASE_MANAGER (self), change_urn);
+      if (object != NULL && !photos_base_item_is_collection (PHOTOS_BASE_ITEM (object)))

Ditto. The _is_collection check should only cover the 2 lines involved in photos_item_manager_check_wait_for_changes.

Created attachment 343269 [details] [review]
item-manager: Don't look for collections in wait_for_changes_table

we don't have collections in the wait_for_changes_table GHashTable, because
collections are stored locally, and are not going to change over time
so, we don't need to keep track of them.

Created attachment 343270 [details] [review]
item-manager: Don't look for collections in wait_for_changes_table

We don't have collections in the wait_for_changes_table GHashTable, because
collections are stored locally, and are not going to change over time
so, we don't need to keep track of them.

Created attachment 343271 [details] [review]
item-manager: Added assertions at the the start of check_wait_for_changes

These assertions will fail if collections are not handled correctly.

Created attachment 343298 [details] [review]
item-manager: Assert valid ID and URI in check_wait_for_changes

Created attachment 343299 [details] [review]
item-manager: Don't look for collections in wait_for_changes_table

Created attachment 343300 [details] [review]
item-manager: Short-circuit collections in wait_for_changes_async

I pushed your patches with some small tweaks. Thanks for your contribution!

The crashes are not specific to the selection mode. Updating the title accordingly.