GNOME Bugzilla – Bug 770342
Remove the mode-specific filtering from PhotosViewModel
Last modified: 2018-01-22 10:32:06 UTC
Now that ItemManager has separate child BaseManagers for each mode (bug 770157), we can greatly simplify ViewModel by removing the mode-specific filtering logic. This was needed earlier when we had one single global bucket inside ItemManager where the results of the mode-specific queries, and we had to filter the contents of the bucket for each ViewModel. Not any more. From now on, ViewModel can simply mirror the contents of each mode-specific child BaseManager. A simplified ViewModel will help us with bug 770267 and in porting to GtkFlowBox in the future.
Created attachment 334089 [details] [review] item-manager: Add photos_item_manager_get_for_mode
Created attachment 334090 [details] [review] Don't add a collection while we are browsing another one
Created attachment 334091 [details] [review] item-manager: Fix logic to check if a cursor is a collection
Created attachment 334092 [details] [review] item-manager: Be more strict about what is acceptable
Created attachment 334093 [details] [review] Remove the mode-specific filtering from PhotosViewModel
Review of attachment 334092 [details] [review]: ::: src/photos-item-manager.c @@ +746,3 @@ + is_collection = photos_item_manager_cursor_is_collection (cursor); + g_return_if_fail ((is_collection && (mode == PHOTOS_WINDOW_MODE_COLLECTIONS || mode == PHOTOS_WINDOW_MODE_SEARCH)) + || (!is_collection && mode != PHOTOS_WINDOW_MODE_COLLECTIONS)); The second part is too restrictive. An item that isn't a collection can be part of the COLLECTION mode if we are browsing the contents of a collection.
Review of attachment 334093 [details] [review]: ::: src/photos-tracker-controller.c @@ +290,3 @@ + + item_mngr_chld = photos_item_manager_get_for_mode (PHOTOS_ITEM_MANAGER (priv->item_mngr), priv->mode); + photos_base_manager_clear (item_mngr_chld); I think we should update the 0th child BaseManager (ie. the union of all the children) when we clear a child. The trade-off is between more aggressive caching of BaseItems versus dropping some unused resources like GeglBuffers, GdkPixbufs, etc.. Given that we don't have any cache invalidation for those objects with pixel data, I am leaning towards the second option.
Created attachment 334515 [details] [review] Don't add a collection while we are browsing another one
Created attachment 334516 [details] [review] item-manager: Be more strict about what is acceptable
Created attachment 334517 [details] [review] item-manager: Add photos_item_manager_clear
Created attachment 334518 [details] [review] Remove the mode-specific filtering from PhotosViewModel
Leave a comment if you spot any regression or bugs in the above patches.
Comment on attachment 334517 [details] [review] item-manager: Add photos_item_manager_clear >+ if (item != NULL) >+ { >+ const gchar *id1; >+ >+ id1 = photos_filterable_get_id (PHOTOS_FILTERABLE (item)); >+ g_assert_cmpstr (id, ==, id1); >+ >+ photos_base_manager_remove_object_by_id (self->item_mngr_chldrn[0], id); >+ } >+ } >+ >+ photos_base_manager_clear (item_mngr_chld); >+} >+ >+ This does not play good with photos_item_manager_get_object_by_id as functions tend to query self->item_mngr_chldrn[0] for lookup. I can give a use case which describes this. 1) Add some images into a new Album. 2) Switch over to "Albums" tab and browse the collection. 3) Switch back to "Overview" and try to preview the images which were added in albums earlier. This is also described in 770824 Turns out we are trying to query the object in photos_view_container_item_activated with the same "id" which was removed in self->item_mngr_chldrn[0] by photos_item_manager_clear. Thus, the object is returned as NULL.
Created attachment 334852 [details] [review] item-manager: Remove from the 0th child if it doesn't exist anywhere
Created attachment 334853 [details] [review] item-manager: Assert an invariant
(In reply to Umang Jain from comment #13) > Comment on attachment 334517 [details] [review] [review] > This does not play good with photos_item_manager_get_object_by_id as > functions tend to query self->item_mngr_chldrn[0] for lookup. I can give a > use case which describes this. > 1) Add some images into a new Album. > 2) Switch over to "Albums" tab and browse the collection. > 3) Switch back to "Overview" and try to preview the images which were added > in albums earlier. > > This is also described in 770824 > > Turns out we are trying to query the object in > photos_view_container_item_activated with the same "id" which was removed in > self->item_mngr_chldrn[0] by photos_item_manager_clear. Thus, the object is > returned as NULL. Good catch. There was typo in the patch. It should have checked for NULL, not non-NULL. Thanks!
*** Bug 770824 has been marked as a duplicate of this bug. ***
Closing again. :)
Review of attachment 334518 [details] [review]: ::: src/photos-item-manager.c @@ +344,3 @@ + id = photos_filterable_get_id (PHOTOS_FILTERABLE (item)); + updated_item = PHOTOS_BASE_ITEM (photos_base_manager_get_object_by_id (PHOTOS_BASE_MANAGER (self), id)); + g_return_if_fail (updated_item == item); This assertion is triggered when favoriting an item while browsing a collection: CRITICAL **: photos_item_manager_info_updated: assertion 'updated_item == item' failed
Created attachment 337527 [details] [review] item-manager: Avoid CRITICALs when favouriting contents of collections
Created attachment 367168 [details] [review] item-manager: Stop listening to updates if an item was cleared
Review of attachment 367168 [details] [review]: +++
Comment on attachment 367168 [details] [review] item-manager: Stop listening to updates if an item was cleared Many thanks for taking!