GNOME Bugzilla – Bug 764086
Convert jobs API to async calls
Last modified: 2018-01-23 10:04:07 UTC
The jobs API are non-ideal since they have no mechanism to notify the caller of errors or cancellations. They are historical artefacts from its origins in gnome-documents. We should make them like the usual async APIs.
Created attachment 324701 [details] [review] create-collection-job: convert to async API
Created attachment 324703 [details] [review] organize-collection-view: Cancel create_collection_job on destruction
Created attachment 324704 [details] [review] delete-item-job: convert to async API
Created attachment 324705 [details] [review] base-item: Cancel delete-item-job during destruction
Created attachment 324706 [details] [review] fetch-collections-job: convert to async API
Created attachment 324708 [details] [review] fetch-collections-job: rename variable
Created attachment 324709 [details] [review] fetch-collection-state-job: Cancel fetch-collections-job during destruction
Created attachment 324710 [details] [review] fetch-ids-job: convert to async API
Created attachment 324711 [details] [review] create-collection-icon-job: convert to async API
Created attachment 324712 [details] [review] fetch-metas-job: Cancel create-collection-icon-job during destruction
Created attachment 324713 [details] [review] single-item-job: convert to async API
Created attachment 324714 [details] [review] update-mtime-job: convert to async API
Review of attachment 324701 [details] [review]: Thanks for the patches. They are mostly correct. Some comments: ::: src/photos-create-collection-job.c @@ +104,3 @@ + g_task_return_pointer (task, g_strdup (val), g_free); + else + g_task_return_pointer (task, NULL, NULL); We should g_task_return_new_error in this case. Otherwise, we will violate the error != NULL when return value is NULL constraint. Something like (PHOTOS_ERROR, 0, "Failed to parse GVariant"). @@ +213,3 @@ if (G_UNLIKELY (self->queue == NULL)) { + g_task_return_new_error (task, PHOTOS_ERROR, 0, "Failed to get a valid tracker queue"); It would be better to pass the error thrown by photos_tracker_queue_dup_singleton. See photos-camera-cache.c and photos-tracker-controller.c for examples. Remember that we need to copy the error since g_task_return_error will assume ownership of it. @@ +228,3 @@ g_object_unref); photos_query_free (query); + out: A newline before 'out:' would be nice. :) ::: src/photos-organize-collection-view.c @@ +118,3 @@ + PhotosSetCollectionJob *set_job = NULL; + PhotosOrganizeCollectionView *self; + PhotosOrganizeCollectionViewPrivate *priv; Nitpick: we usually keep self and priv on the first 2 lines. @@ +129,3 @@ + g_warning ("Unable to create collection: %s", error->message); + g_error_free (error); + goto out; This 'goto out' doesn't look right because 'error != NULL' is the same thing as 'created_urn == NULL' (except that when we introduce cancellation, we will have to short-circuit the G_IO_ERROR_CANCELLED case). This means that we are going to miss the photos_organize_collection_model_remove_placeholder call for errors. I think we should print the warning, free the error and continue forward.
Review of attachment 324703 [details] [review]: ::: src/photos-organize-collection-view.c @@ +129,3 @@ { + if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) + g_warning ("Unable to create collection: %s", error->message); We will need to separate the G_IO_ERROR_CANCELLED case a bit more because we want to completely short-circuit it, while for other errors we want to keep falling down to the self/priv assignment. See previous review.
Review of attachment 324704 [details] [review]: ::: src/photos-delete-item-job.c @@ +181,3 @@ if (G_UNLIKELY (self->queue == NULL)) { + g_task_return_new_error (task, PHOTOS_ERROR, 0, "Unable to get valid tracker queue"); It would be better to use the error thrown by photos_tracker_queue_dup_singleton. ::: src/photos-delete-item-job.h @@ -43,1 @@ Nitpick: would be nice to remove the newline also.
Created attachment 325472 [details] [review] create-collection-job: convert to async API Fixing raised points.
Created attachment 325473 [details] [review] organize-collection-view: Cancel create_collection_job on destruction Fix raised points.
Created attachment 325474 [details] [review] delete-item-job: convert to async API Fix raised points.
Review of attachment 325472 [details] [review]: Looks good to me.
Review of attachment 325473 [details] [review]: ::: src/photos-organize-collection-view.c @@ +135,1 @@ g_warning ("Unable to create collection: %s", error->message); Nitpick: would be nice to put this in an else branch, like we do elsewhere.
Created attachment 325872 [details] [review] Cancel create_collection_job during destruction Fixed and pushed.
Review of attachment 325474 [details] [review]: ::: src/photos-delete-item-job.c @@ +35,3 @@ #include "photos-search-context.h" #include "photos-tracker-queue.h" +#include "photos-utils.h" I forgot to point out that this #include is not needed.
Created attachment 325873 [details] [review] delete-item-job: Convert to async API Fixed and pushed.
Review of attachment 324705 [details] [review]: Now that I think about it, I am not so sure about this one. Cancelling the DeleteItemJob on destruction doesn't give us anything from the ref counting / lifecycle front, because we were just letting it run and die in the void. We didn't keep any ref on self or anything like that. On the flip side, it does expose us to cases where the BaseItem might not get removed from the database because for some reason the BaseItem got destroyed while the job was still running. That would be surprising for the user. Note that photos_base_item_trash is a fire-and-forget function, and not a typical async where the BaseItem would have been kept alive by the GTask during the lifetime of the operation. We could turn photos_base_item_trash into a full async with a _finish and everything, but then it would be up to the photos_base_item_trash caller to cancel the operation. I am also not sure if that would actually be useful in the end. What we really need is a way to flush the queue of pending queries in TrackerQueue when the application is about to die, but that is a topic for another bug. :)
Review of attachment 324706 [details] [review]: Thanks for the patches, Rafael. Some comments: ::: src/photos-fetch-collection-state-job.c @@ +184,3 @@ + g_warning ("Unable to fetch collections: %s", error->message); + g_error_free (error); + return; Returning in case of an error is subtly changing the logic. I think we should keep going. Otherwise we would never reach photos_fetch_collection_state_job_emit_callback. ::: src/photos-fetch-collections-job.c @@ +35,3 @@ #include "photos-search-context.h" #include "photos-tracker-queue.h" +#include "photos-utils.h" This won't be needed. See below. @@ +82,2 @@ + self = PHOTOS_FETCH_COLLECTIONS_JOB (g_task_get_source_object (task)); + cancellable = g_task_get_cancellable (task); The changes near this area appear to be a bit unrelated to this patch. eg., we can avoid most of it by moving these 2 lines to the top of the function. Note that since the lifetime of self is bound to that of task, we don't need to worry about self getting destroyed or G_IO_ERROR_CANCELLED. @@ +85,2 @@ + if (valid) + { This change, and removing the 'end:' look nice, but would be better to do them in a separate patch. @@ +127,3 @@ photos_fetch_collections_job_cursor_next, + g_object_ref (task)); + out: Nitpick: a newline before the out would be nice. @@ +228,3 @@ PhotosQuery *query; PhotosSearchContextState *state; + GTask *task; Nitpick: maybe put it between app and query to retain the ordering? @@ +235,3 @@ if (G_UNLIKELY (self->queue == NULL)) { + g_task_return_new_error (task, PHOTOS_ERROR, 0, "Unable to get tracker queue"); We should use the actual error from photos_tracker_queue_dup_singleton, as you did before. @@ +250,3 @@ g_object_unref); photos_query_free (query); + out: Nitpick: a new line would be nice.
Review of attachment 324708 [details] [review]: Looks good, but it would probably need a rebase after the previous patch is adjusted.
Review of attachment 324709 [details] [review]: ::: src/photos-fetch-collection-state-job.c @@ +298,3 @@ + self->cancellable, + photos_fetch_collection_state_job_job_collector, + data); We need to change photos_fetch_collection_state_job_data_new to not take a ref on self. Otherwise, self->cancellable will never be cancelled. Then we need to check for G_IO_ERROR_CANCELLED in photos_fetch_collection_state_job_job_collector.
Review of attachment 324710 [details] [review]: ::: src/photos-search-provider.c @@ -88,2 @@ - app = g_application_get_default (); - g_application_release (app); This change is not needed. See below. @@ +93,3 @@ + { + if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) + g_warning ("Unable to fetch ids: %s", error->message); Since photos_fetch_ids_job_run is not called with a GCancellable, we don't need to check for this. Actually, photos_fetch_ids_job_run should not be cancelled because this is the "server-side" of the gnome-shell search provider. As a DBus service it should stay alive while it handles the DBus call.
Review of attachment 324714 [details] [review]: ::: src/photos-update-mtime-job.c @@ +35,3 @@ #include "photos-search-context.h" #include "photos-tracker-queue.h" +#include "photos-utils.h" This won't be needed ... @@ +181,3 @@ if (G_UNLIKELY (self->queue == NULL)) { + g_task_return_new_error (task, PHOTOS_ERROR, 0, "Unable to get valid tracker queue"); ... because we should use the error thrown when creating the queue.
Review of attachment 324713 [details] [review]: ::: src/photos-single-item-job.c @@ +217,3 @@ if (G_UNLIKELY (self->queue == NULL)) { + g_task_return_new_error (task, PHOTOS_ERROR, 0, "Unable to get valid tracker queue"); Same thing about the error.
Created attachment 326031 [details] [review] fetch-collections-job: move some code around Addressing raised points.
Created attachment 326032 [details] [review] fetch-collections-job: convert to Async API Addressing raised points.
Created attachment 326033 [details] [review] fetch-collections-job: rename variable Rebase
Created attachment 326034 [details] [review] fetch-collections-job: make urn property gettable Needed by the collection-state-job patch getting rid of data.
Created attachment 326035 [details] [review] fetch-collection-state-job: get rid of job data
Created attachment 326036 [details] [review] fetch-collection-state-job: check for valid list before hashing
Created attachment 326037 [details] [review] fetch-collection-state-job: cancel fetch-collections-job during destruction Addressing raised points.
Created attachment 326038 [details] [review] fetch-ids-job: convert to async API
Created attachment 326039 [details] [review] create-collection-icon-job: convert to async API
Created attachment 326040 [details] [review] fetch-metas-job: cancel create-collection-icon-job during destruction
Created attachment 326041 [details] [review] single-item-job: convert to async API Addressing raised points.
Created attachment 326042 [details] [review] update-mtime-job: convert to async API Addressing raised points.
Review of attachment 326031 [details] [review]: ::: src/photos-fetch-collections-job.c @@ +106,3 @@ photos_fetch_collections_job_emit_callback (self); + + end: This bit shouldn't change because we want photos_fetch_collections_job_emit_callback to be called even when there is an error.
(In reply to Debarshi Ray from comment #43) > Review of attachment 326031 [details] [review] [review]: > > ::: src/photos-fetch-collections-job.c > @@ +106,3 @@ > photos_fetch_collections_job_emit_callback (self); > + > + end: > > This bit shouldn't change because we want > photos_fetch_collections_job_emit_callback to be called even when there is > an error. But if the error is G_IO_CANCELLED, than self is not valid anymore.
Review of attachment 326032 [details] [review]: Looks good, but will need a rebase after the previous patch is adjusted.
(In reply to Rafael Fonseca from comment #44) > (In reply to Debarshi Ray from comment #43) > > Review of attachment 326031 [details] [review] [review] [review]: > > > > ::: src/photos-fetch-collections-job.c > > @@ +106,3 @@ > > photos_fetch_collections_job_emit_callback (self); > > + > > + end: > > > > This bit shouldn't change because we want > > photos_fetch_collections_job_emit_callback to be called even when there is > > an error. > > But if the error is G_IO_CANCELLED, than self is not valid anymore. Two things: (a) We are not using a GCancellable, yet. It gets added in the next patch. (b) Even with a GCancellable, self's lifetime is bound to that of the GTask. As long as the GTask is alive, self is alive. When you call: my_object_action_async (obj, cancellable, callback, user_data) ... 'obj' will stay alive until 'callback' has been invoked because it will be bound to the GTask or GSimpleAsyncResult on the callee (or implementation) side. And the GTask needs to stay alive so that you can call my_object_action_finish. It is only the lifetime of the user_data that we need to worry about. In our case, where photos_fetch_collections_job_run is implemented as a wrapper around other async operations, the GTask used to implement photos_fetch_collections_job_run is passed around as the user_data, and we ensure that it stays alive. So, if you call: my_object_action_async (self->obj, cancellable, callback, self) ... and 'cancellable' is triggered, then self->obj will survive, but unless we kept a reference on self, there is no guarantee that it will be valid. It is a bit confusing, I know. :)
Created attachment 326329 [details] [review] fetch-collections-job: Move some code around Fixed and pushed.
Created attachment 326330 [details] [review] fetch-collections-job: Convert to async API Rebased and pushed.
Review of attachment 326033 [details] [review]: Looks good to me.
Review of attachment 326034 [details] [review]: ::: src/photos-fetch-collections-job.c @@ +161,3 @@ + { + case PROP_URN: + g_value_set_string (value, self->urn); The downside of the convention (in the GObject world) to implement get_property with g_value_set_string, is that the g_object_get caller needs to free the string. In most cases, a getter that returns a 'const gchar *' is more convenient (and easier on the memory allocator).
Review of attachment 326035 [details] [review]: I like this idea. ::: src/photos-fetch-collection-state-job.c @@ +155,3 @@ } + g_object_get (job, "urn", &urn, NULL); We need to g_free 'urn'. However, if we have a get method (see previous comment) then our lives will be easier.
(In reply to Debarshi Ray from comment #51) > Review of attachment 326035 [details] [review] [review]: > > I like this idea. > > ::: src/photos-fetch-collection-state-job.c > @@ +155,3 @@ > } > > + g_object_get (job, "urn", &urn, NULL); > > We need to g_free 'urn'. However, if we have a get method (see previous > comment) then our lives will be easier. Doesn't the hash table get ownership over it? Before it was being strdup'ed in the hash_table_insert call.
Review of attachment 326038 [details] [review]: ::: src/photos-fetch-ids-job.c @@ +34,3 @@ #include "photos-search-controller.h" #include "photos-tracker-queue.h" +#include "photos-utils.h" This won't be needed. See below. @@ +81,3 @@ + + if (valid) + { The changes in this area seem unrelated to this patch. Would be nice to split it into a separate patch. @@ +234,3 @@ if (G_UNLIKELY (self->queue == NULL)) { + g_task_return_new_error (task, PHOTOS_ERROR, 0, "Unable to get valid track queue"); It would be better to use the error from photos_tracker_queue_dup_singleton, like we do elsewhere. ::: src/photos-search-provider.c @@ -88,2 @@ - app = g_application_get_default (); - g_application_release (app); Would better to use a separate patch for moving it around. @@ +94,3 @@ + if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) + g_warning ("Unable to fetch ids: %s", error->message); + g_error_free (error); Now that we can actually get to the error, we should use g_dbus_method_invocation_take_error.
(In reply to Debarshi Ray from comment #28) > Review of attachment 324710 [details] [review] [review]: > @@ +93,3 @@ > + { > + if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) > + g_warning ("Unable to fetch ids: %s", error->message); > > Since photos_fetch_ids_job_run is not called with a GCancellable, we don't > need to check for this. Actually, photos_fetch_ids_job_run should not be > cancelled because this is the "server-side" of the gnome-shell search > provider. As a DBus service it should stay alive while it handles the DBus > call. Sorry, I was mistaken. We do cancel the previous invocation if another one arrived while it was running.
(In reply to Rafael Fonseca from comment #52) > (In reply to Debarshi Ray from comment #51) > > Review of attachment 326035 [details] [review] [review] [review]: > > > > I like this idea. > > > > ::: src/photos-fetch-collection-state-job.c > > @@ +155,3 @@ > > } > > > > + g_object_get (job, "urn", &urn, NULL); > > > > We need to g_free 'urn'. However, if we have a get method (see previous > > comment) then our lives will be easier. > > Doesn't the hash table get ownership over it? Before it was being strdup'ed > in the hash_table_insert call. Oh, yes, that is true. I didn't notice it. However, I slightly prefer to explicitly do the g_strdup while inserting it into the GHashTable because it is easier to follow the ownership.
Created attachment 326344 [details] [review] fetch-collections-job: make urn property gettable Now using getter with const gchar.
Created attachment 326345 [details] [review] fetch-collection-state-job: get rid of job data Using the new getter.
Created attachment 326346 [details] [review] fetch-collection-state-job: check for valid list before hashing Rebasing.
Created attachment 326347 [details] [review] fetch-collection-state-job: cancel job during destruction Rebasing.
Created attachment 326416 [details] [review] fetch-ids-job: rename variable Splitting previous version of the patch.
Created attachment 326417 [details] [review] fetch-ids-job: move some code around Splitting patch part 2.
Created attachment 326418 [details] [review] fetch-ids-job: convert to async API Fixing issues.
Review of attachment 326344 [details] [review]: Thanks Rafael.
Review of attachment 326345 [details] [review]: ::: src/photos-fetch-collection-state-job.c @@ -285,3 @@ job = photos_fetch_collections_job_new (urn); - data = photos_fetch_collection_state_job_data_new (self, urn); - photos_fetch_collections_job_run (job, NULL, photos_fetch_collection_state_job_job_collector, data); This is even wider than 120 characters. Let's break it into separate lines.
Created attachment 326472 [details] [review] fetch-collection-state-job: Get rid of job data Fixed and pushed.
Review of attachment 326039 [details] [review]: Looks good, except a few smaller things: ::: src/photos-create-collection-icon-job.c @@ +166,2 @@ /* TODO: build collection icon query */ + g_task_return_pointer (task, NULL, NULL); Here, I would use g_task_return_new_error to return a PHOTOS_ERROR to satisfy the "error != NULL when return value is NULL" invariant. ::: src/photos-fetch-metas-job.c @@ +115,3 @@ + g_warning ("Unable to create collection icon: %s", error->message); + g_error_free (error); + goto out; In its current form, the code expects to always call photos_fetch_metas_job_collector. Otherwise, the callback passed by the PhotosFetchMetasJob client will never be invoked. @@ +120,2 @@ self = PHOTOS_FETCH_METAS_JOB (g_task_get_source_object (task)); meta = (PhotosFetchMeta *) g_task_get_task_data (task); One option is to move this before the photos_create_collection_icon_job_finish call and ... @@ +129,2 @@ self->metas = g_list_prepend (self->metas, meta); photos_fetch_metas_job_collector (self); ... have the 'out' include these two lines.
Review of attachment 326040 [details] [review]: It would be better to first convert FetchMetasJob to use a GIO-like cancellable async API, like you have done to the others. Then, this change might not be required. When we call photos_fetch_metas_job_run, the important thing is that the FetchMetasJob instance has to stay alive until the callback is invoked. This needs to be guaranteed by the implementation of photos_fetch_metas_job_run. It is usually done by a GTask (or a GSimpleAsyncResult). It is upto the photos_fetch_metas_job_run caller to handle the lifetime of user_data. The caller can either take a ref and keep it alive until the callback is invoked, or it can cancel the operation when the user_data becomes invalid.
(In reply to Debarshi Ray from comment #67) > Review of attachment 326040 [details] [review] [review]: > When we call photos_fetch_metas_job_run, the important thing is that the > FetchMetasJob instance has to stay alive until the callback is invoked. This > needs to be guaranteed by the implementation of photos_fetch_metas_job_run. > It is usually done by a GTask (or a GSimpleAsyncResult). It is upto the > photos_fetch_metas_job_run caller to handle the lifetime of user_data. The > caller can either take a ref and keep it alive until the callback is > invoked, or it can cancel the operation when the user_data becomes invalid. Also see comment 46 (from (b) onwards) - I was trying to say the same thing but in a different way.
Created attachment 326487 [details] [review] create-collection-icon-job: convert to async API Fix issues.
Review of attachment 326487 [details] [review]: Looks good. Pushed.
Created attachment 326560 [details] [review] create-collection-icon-job: remove unused variable
Created attachment 326860 [details] [review] single-item-job: convert to async API
Review of attachment 326860 [details] [review]: Thanks for the new patch, Rafael. It looks fine except for a few ref-counting and similar issues. ::: src/photos-application.c @@ +649,3 @@ const gchar *identifier; + cursor = photos_single_item_job_finish (job, res, &error); We need to unref the cursor because we now have: g_task_return_pointer (task, g_object_ref (cursor), g_object_unref); The result_destroy (ie. g_object_unref) is only used if g_task_propagate_pointer (ie. photos_single_item_job_finish) is not called. This is a subtle change from the existing code, but this is a common convention with GAsyncResult / GTask, so I think it is fine. ::: src/photos-base-item.c @@ +553,3 @@ + TrackerSparqlCursor *cursor = NULL; + + cursor = photos_single_item_job_finish (job, res, &error); The cursor needs to be unreffed. ::: src/photos-collection-icon-watcher.c @@ +177,3 @@ + GError *error = NULL; + + cursor = photos_single_item_job_finish (job, res, &error); Ditto. ::: src/photos-fetch-metas-job.c @@ +163,3 @@ const gchar *title; + cursor = photos_single_item_job_finish (job, res, &error); Ditto. ::: src/photos-item-manager.c @@ +126,3 @@ + TrackerSparqlCursor *cursor = NULL; + + cursor = photos_single_item_job_finish (job, res, &error); Ditto. ::: src/photos-single-item-job.c @@ +73,3 @@ } else if (!valid) goto out; This bit is complicated because tracker_sparql_cursor_next_finish can return FALSE even without an error. It means that there are no more rows inside it. We need to ensure that we invoke the callback in this case too, like the existing code does. ie. we need to call one of the g_task_return_* functions. I suggest that we call g_task_return_pointer with a NULL result for the time being to match the existing behaviour. In a subsequent patch, we can change this to throw an error so that the callbacks don't need to handle error != NULL and cursor == NULL separately. @@ +80,1 @@ tracker_sparql_cursor_close (cursor); This tracker_sparql_cursor_close call is problematic. g_task_return_* will invoke the callback in the next iteration of the GMainLoop. That means that if the callback calls any of the tracker_sparql_cursor_get_* functions, it will be after the cursor has been closed. Strictly speaking this is illegal. With the current implementations of the various TrackerSparqlCursor sub-classes, this doesn't appear to be a problem, but that can change in future. Note that destroying a TrackerSparqlCursor by dropping the last reference is enough to close it. These explicit tracker_sparql_cursor_close calls are inherited from gnome-documents' JavaScript code. In JS, there is a worry about the garbage collector not running soon enough, which can cause resources (some cursors use a file descriptor) to be needlessly held up. We don't have that problem in C, so we can simply remove these calls. @@ +213,1 @@ return; We need a 'goto out' to unref the GTask.
Created attachment 330194 [details] [review] single-item-job: Remove redundant function call
Created attachment 330195 [details] [review] single-item-job: Convert to async API I took the liberty to make the above changes.
Review of attachment 326560 [details] [review]: Looks good to me. Pushed.
Review of attachment 326418 [details] [review]: ::: src/photos-fetch-ids-job.c @@ +39,3 @@ { GObject parent_instance; GCancellable *cancellable; This can be removed. In fact, you had already removed it in the previous iteration of the patch. :) @@ +43,2 @@ GPtrArray *ids; PhotosFetchIdsJobCallback callback; Ditto about callback and user_data. @@ +90,3 @@ + tracker_sparql_cursor_next_async (cursor, + self->cancellable, We can get the GCancellable from the GTask. @@ +241,1 @@ return; We need a 'goto out' to unref the GTask. @@ +254,2 @@ query->sparql, self->cancellable, Just 'cancellable', as in the previous version of this patch. ::: src/photos-search-provider.c @@ +96,3 @@ + { + g_dbus_method_invocation_take_error (invocation, error); + return; We need a 'goto out' to unref the GDBusMethodInvocation. See bug 767952 about improving the g_dbus_method_invocation_* documentation.
Review of attachment 326416 [details] [review]: Thanks for splitting it this out. Pushed.
Review of attachment 326417 [details] [review]: Looks good to me.
Created attachment 330202 [details] [review] fetch-ids-job: Remove redundant function call
Created attachment 330203 [details] [review] fetch-ids-job: Convert to async API I took the liberty to fix it up and pushed it to master.
Review of attachment 326042 [details] [review]: ::: src/photos-set-collection-job.c @@ +77,3 @@ + if (error != NULL) + { + g_warning ("Could not query mtime: %s", error->message); Nitpick: s/query/update/
Created attachment 330238 [details] [review] update-mtime-job: Convert to Async API Fixed and pushed.
Review of attachment 326346 [details] [review]: ::: src/photos-fetch-collection-state-job.c @@ +155,3 @@ + if (collections_for_item != NULL) + { Was this causing an actual bug? I wonder if this change has an effect on photos_fetch_collection_state_job_emit_callback. From a quick reading - probably not. If you really want to make this change, then I would suggest submitting a similar patch for gnome-documents so that the logic doesn't start diverging in subtle ways.
Review of attachment 326347 [details] [review]: Same comment as attachment 326040 [details] [review] - it will be better to first convert FetchCollectionStateJob to a GIO-like async API because then this might not even be needed.
As for pending items, I think FetchCollectionStateJob and FetchMetasJob need to be converted.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gnome-photos/issues/44.