GNOME Bugzilla – Bug 757015
Clicking the favorite button doesn't update the UI immediately
Last modified: 2015-10-30 10:23:12 UTC
If you click the favorite button in the selection toolbar it takes a while before the star emblem shows up and the item is shown in the favorites tab. This is caused by all the asynchronous operations involved in the process: a) photos_utils_set_favorite queues a SPARQL update that is processed after all pending updates/queries b) eventually TrackerChangeMonitor::changes-pending is emitted c) photos_base_item_refresh queues a SPARQL query to fetch the updated meta-data d) once that is processed, photos_base_item_populate_from_cursor updates the meta-data inside BaseItem, sorts out the thumbnail to use (which can involve some more async calls), and then emits BaseItem::info-updated This is also why the favorite button in the preview toolbar gets confused if you press it in quick succession. The value of BaseItem:favorite doesn't stay in sync with the user's expectation. We can do better. We can update BaseItem:favorite immediately inside photos_base_item_default_set_favorite and emit infp-updated before starting the chain of asynchronous operations.
Created attachment 313950 [details] [review] base-item: Anticipate BaseItem:favorite update to improve responsiveness
Review of attachment 313950 [details] [review]: Thanks, Alessandro. At first glance it looks good to me, but I don't have a way to test it right now, so I will have another look when I am able to do that.
Review of attachment 313950 [details] [review]: While this takes care of updating the favorites button in preview, it still takes a while for the star emblem to show up if you are using the selection toolbar. The reason is that the emblem is only updated at the end of the thumbnailing step. We need to short-circuit that too.
Review of attachment 313950 [details] [review]: ::: src/photos-base-item.c @@ +409,3 @@ + priv->favorite = favorite; + g_signal_emit (self, signals[INFO_UPDATED], 0); + photos_utils_set_favorite (priv->id, favorite); Since we are now emitting info-updated while looping over the selection in photos_selection_toolbar_favorite_clicked, there is a chance that the selection will get modified while we are iterating over it. eg., if you are un-favoriting an item, then it will get removed from the favorites view, which lead to the item being removed from the ViewModel for favorites, and change the selection. To avoid crashing we need to copy the selection like we do when deleting. eg., http://paste.fedoraproject.org/283661/44585663/
Created attachment 314167 [details] [review] base-item: Anticipate BaseItem:favorite update to improve responsiveness
Review of attachment 314167 [details] [review]: Nit: the subject of the commit message shouldn't exceed 72 characters. How about: "Improve the responsiveness of the favorite button"? Since it touches multiple files we can drop the usual prefix. ::: src/photos-base-item.c @@ +406,3 @@ { + PhotosBaseItemPrivate *priv = self->priv; + Since we are touching this function, it would be nice to have a check like this at the top of the function: if (favorite == priv->favorite) return; Something to be done in a separate patch, though. :) @@ +409,3 @@ + priv->favorite = favorite; + photos_base_item_check_effects_and_update_info (self); + g_signal_emit (self, signals[INFO_UPDATED], 0); check_effects_and_update_info will already emit info-updated as long as priv->original_icon != NULL. That should always be the case, unless: a) We have closed the window before the first call to photos_base_item_create_placeholder_icon and it returns NULL b) The icon theme doesn't have content-loading-symbolic or image-x-generic-symbolic and we can't look up the placeholder icons We don't need to worry about (a). If there is no window, then a missing info-updated emission is not an issue. Right now, I would say (b) is a broken system. I would prefer to avoid an extra info-updated (and waking up GtkIconView, which is already slow), or complicate the code to deal with missing icons, unless we see a actual/valid problem. So, just drop the g_signal_emit.
Created attachment 314443 [details] [review] Improve the responsiveness of the favorite button
Created attachment 314444 [details] [review] base-item: Add short-circuit to photo_base_item_default_set_favorite
Review of attachment 314443 [details] [review]: ::: src/photos-selection-toolbar.c @@ +161,2 @@ item = PHOTOS_BASE_ITEM (photos_base_manager_get_object_by_id (priv->item_mngr, urn)); + items = g_list_prepend (items, g_object_ref (item)); Oops, I forgot to mention that items is not being freed.
Created attachment 314445 [details] [review] Improve the responsiveness of the favorite button Pushed after addressing the above comment.
Review of attachment 314444 [details] [review]: Nice. Pushed after tweaking the commit message a bit.