GNOME Bugzilla – Bug 774191
Turn PhotosBaseManager into a GListModel
Last modified: 2016-12-05 13:00:22 UTC
PhotosBaseManager, the class that we use as a model for tracking images, sources, share-points, etc., should offer the GListModel interface, so that we can replace our GtkIconView-based view with a GtkFlowBox-based one. It would also mean that we will no longer use PhotosViewModel (which is a GtkListStore) and the changes in bug 770342 have moved us closer towards this goal.
Summarizing my discussions with Alessandro for the sake of any onlookers... There is one bone of contention with GListModel-ifying PhotosBaseManager. The GListModel interface is primarily for rendering the contents of a BaseManager. eg., a GtkFlowBox to show all the images or collections or share-points. Rendering can involve having the objects sorted in a particular order. eg., for images and collections we want to show the most recent objects first. At the same time, we want to retain the ability of BaseManager to look up an object from its ID in O(1) time. This means that we cannot simply replace the internals of PhotosBaseManager with a GListStore (which is a GListModel implementation that comes with GLib). GListStore is very good at letting a GtkFlowBox render the objects, possibly sorted. It uses GSequence, which is a balanced binary tree, so it is easy and efficient to iterate over it like a GList, and given a GCompareDataFunc it can keep the tree sorted in O(nlogn). Random access is O(logn), which, I guess, is acceptable. However, looking up an object from its ID will be O(n), which is quite a loss from our current O(1) behaviour. This is because the GSequence (ie. the balanced binary tree) is sorted based on how the objects will be rendered, not based on their IDs. eg., it might sorted based on the images' mtimes, not their URNs. So, can we add a GListStore inside PhotosBaseManager alongside our existing GHashTable based implementation? Almost, but no. Things like photos_base_manager_remove_by_id are currently O(1). To keep the GListModel updated, we will have to iterate over the GListStore in O(n) time to find the object to remove. This would be more efficient if we had the GSequenceIter pointing to the actual node inside the GSequence. eg., our GHashTable could be a mapping from ID to (GObject, GSequenceIter). Unfortunately, GListStore doesn't expose its internal GSequence. Therefore, instead of using a GListStore inside BaseManager, we add a GSequence inside PhotosBaseManager and basically reimplement GListStore ourselves. It isn't as bad as it sounds. We don't need most of the GListStore modification API, because BaseManager already has its own. The guts of what we really need are in GSequence itself. Hence it is not a lot of code. Credit goes to Alessandro Bono for investigating this.
Created attachment 339604 [details] [review] base-manager: Stop using photos_base_manager_get_objects
Created attachment 339605 [details] [review] base-manager: Consolidate the removal logic in one place
Created attachment 339606 [details] [review] item-manager: Rename a variable
Created attachment 339607 [details] [review] base-manager: Directly access the GHashTable in remove_object_by_id
Created attachment 339608 [details] [review] Turn PhotosBaseManager and PhotosItemManager into GListModels
Review of attachment 339604 [details] [review]: ok
Review of attachment 339605 [details] [review]: Looks ok to me otherwise. ::: src/photos-base-manager.c @@ +483,3 @@ GHashTableIter iter; + GList *l; + GList *removed_ids = NULL; We could start using GSList (https://developer.gnome.org/glib/stable/glib-Singly-Linked-Lists.html) when we don't need a double linked structure. What do you think about it?
Review of attachment 339606 [details] [review]: ok
Review of attachment 339607 [details] [review]: ok
Comment on attachment 339604 [details] [review] base-manager: Stop using photos_base_manager_get_objects Pushed to master.
Comment on attachment 339606 [details] [review] item-manager: Rename a variable Pushed to master.
Comment on attachment 339607 [details] [review] base-manager: Directly access the GHashTable in remove_object_by_id Pushed to master.
(In reply to Alessandro Bono from comment #8) > Review of attachment 339605 [details] [review] [review]: Thanks for the review, Alessandro! > ::: src/photos-base-manager.c > @@ +483,3 @@ > GHashTableIter iter; > + GList *l; > + GList *removed_ids = NULL; > > We could start using GSList > (https://developer.gnome.org/glib/stable/glib-Singly-Linked-Lists.html) when > we don't need a double linked structure. What do you think about it? Sure, why not?
Created attachment 339912 [details] [review] base-manager: Consolidate the removal logic in one place Use GSList instead of GList.
Created attachment 339913 [details] [review] source-manager: Rearrange the code for subsequent changes
Created attachment 339914 [details] [review] share-point-manager: Rearrange the code for subsequent changes
Created attachment 339915 [details] [review] Turn PhotosBaseManager and PhotosItemManager into GListModels
Review of attachment 339912 [details] [review]: ok
Review of attachment 339913 [details] [review]: ok
Review of attachment 339914 [details] [review]: ok
Review of attachment 339915 [details] [review]: ::: src/photos-base-manager.c @@ +483,3 @@ g_clear_object (&priv->active_object); + + count = photos_base_manager_get_objects_count (self); Shouldn't we get the count before removing all the objects in the Hashtable? Even better if we get it before removing all the objects in the GSequence too.
Comment on attachment 339912 [details] [review] base-manager: Consolidate the removal logic in one place Pushed to master.
Comment on attachment 339913 [details] [review] source-manager: Rearrange the code for subsequent changes Pushed to master.
Comment on attachment 339914 [details] [review] share-point-manager: Rearrange the code for subsequent changes Pushed to master.
Created attachment 340227 [details] [review] Turn PhotosBaseManager and PhotosItemManager into GListModels Get the count before removing the contents of the GSequence and the GHashTable.
Review of attachment 340227 [details] [review]: ok
Comment on attachment 340227 [details] [review] Turn PhotosBaseManager and PhotosItemManager into GListModels Pushed to master.
Created attachment 341302 [details] [review] item-manager: Emit GListModel::items-changed
Comment on attachment 341302 [details] [review] item-manager: Emit GListModel::items-changed Pushed to master. Let me know if something is wrong.