GNOME Bugzilla – Bug 786936
Have a separate PhotosWindowMode for viewing the contents of collections
Last modified: 2018-02-14 22:22:39 UTC
Showing the contents of a collection doesn't use the usual WindowMode transitions via ModeController::window-mode-changed, etc.. It is special-cased as "sub-modes" of the COLLECTIONS and SEARCH modes. This makes it harder to ensure the correctness of the internal state machine, and makes it harder to fix things like bug 771038. Let's regularize this by introducing a separate WindowMode. It will also make our lives easier when implementing support for importing from a removable device because the contents of the device are to be presented as a collection. We'll have to treat the device as a special collection, so let's not have two layers of special-casing.
Created attachment 358614 [details] [review] searchbar: Use the more appropriate GAction API
Created attachment 358615 [details] [review] embed: Remove redundant code
Created attachment 358616 [details] [review] embed, overview-searchbar: Simplify code
Created attachment 358617 [details] [review] embed: Simplify code
Created attachment 358618 [details] [review] embed: Untangle the notify::visible-child & window-mode-changed logic
Created attachment 358619 [details] [review] Simplify Searchbar handling and don't show it on key presses in PREVIEW
Created attachment 358620 [details] [review] embed: Don't keep the searchbar active merely because it has the focus
Created attachment 358621 [details] [review] Consolidate code to update MainToolbar on ItemManager::items-changed
Created attachment 358622 [details] [review] Add a new mode for viewing the contents of a collection
Review of attachment 358614 [details] [review]: ok
Review of attachment 358615 [details] [review]: ok
Review of attachment 358616 [details] [review]: ok
Review of attachment 358617 [details] [review]: ok
Review of attachment 358618 [details] [review]: ok
Review of attachment 358619 [details] [review]: ::: src/photos-main-toolbar.c @@ +651,2 @@ self->searchbar = g_object_ref_sink (photos_overview_searchbar_new ()); + gtk_container_add (GTK_CONTAINER (self), self->searchbar); Since the searchbar now is always present, is it possible to move it in photos-main-toolbar.ui?
Review of attachment 358620 [details] [review]: ok
Review of attachment 358621 [details] [review]: ok
Thanks for the reviews!
Created attachment 358787 [details] [review] Simplify Searchbar handling and don't show it on key presses in PREVIEW Use the XML template to add the OverviewSearchbar.
Created attachment 358788 [details] [review] Simplify Searchbar handling and don't show it on key presses in PREVIEW
*** Bug 787099 has been marked as a duplicate of this bug. ***
Review of attachment 358621 [details] [review]: ::: src/photos-application.c @@ +2018,3 @@ + "items-changed", + G_CALLBACK (photos_application_items_changed), + self); One of the case I hit upon is of photos_base_item_trash here (refer criticals below). Deleting(which is hiding initially) the item will emit "item-changed" signal and subsequently will lead to the 'for' loop in photos_application_items_changed (photos-application.c:408) photos_item_manager_hide_item will remove the item from the base manager and in the 'for' loop, we try to get_object_by_id the item (which now is absent in the base manager), hence giving the criticals. -- (gnome-photos:23926): gnome-photos-CRITICAL **: photos_base_item_can_trash: assertion 'PHOTOS_IS_BASE_ITEM (self)' failed (gnome-photos:23926): gnome-photos-CRITICAL **: photos_base_item_get_default_app_name: assertion 'PHOTOS_IS_BASE_ITEM (self)' failed Getting a few criticals while trying out wip/rishi/collection branch Steps to reproduce: 1) Select an image by right-click 2) Click "Trash" icon in the bar present below. Criticals triggered.
Created attachment 359007 [details] [review] Consolidate code to update MainToolbar on ItemManager::items-changed Avoid the CRITICALs by adding a NULL-check.
Created attachment 359076 [details] [review] Consolidate code to update MainToolbar on ItemManager::items-changed Added a comment to explain the NULL check.
Created attachment 359616 [details] [review] Listen to ItemManager::active-collection-changed only when needed This splits out some hunks from attachment 358622 [details] [review].
Created attachment 359617 [details] [review] main-toolbar: Remove redundant cod Some more splitting.
Created attachment 359618 [details] [review] Add a new mode for viewing the contents of a collection
Comment on attachment 359616 [details] [review] Listen to ItemManager::active-collection-changed only when needed I self-reviewed these once more, and couldn't spot any bugs. Of course, I could be wrong. However, we are at the start of a new development cycle, so it is better to push these sooner than later. That way we will get some extra testing in the wild.
Created attachment 367167 [details] [review] item-manager: Remove pre-COLLECTION_VIEW workaround Found another workaround from the time when collections and their contents shared the same view and mode.
Review of attachment 367167 [details] [review]: ++
Created attachment 368362 [details] [review] view-container: Unbreak progressive loading of collection contents