Created attachment 341947 [details] [review]
photos-searchbar: Port to GtkSearchBar

Review of attachment 341947 [details] [review]:

Thanks for taking this on!

Can't we delete even more lines of code? eg., here is the corresponding gnome-documents commit:
https://git.gnome.org/browse/gnome-documents/commit/?id=0b3aa6955f22fd105e5dd25fda9653a3628a87ea

Created attachment 349571 [details] [review]
photos-searchbar: Port to GtkSearchBar

Created attachment 349572 [details] [review]
searchbar: Stop using gd_entry_focus_hack()

It is no more needed.

Created attachment 349573 [details] [review]
searchbar: Enable/disable search action when required

Since the key events are now handled by GtkSearchbar, the
searchbar will now show/hide itself when it receives the
appropriate key event. However, a way to enable/disable the
search action is missing. Thus, we need to do it manually when
escape is pressed or when the searchbar has handeled the keyevent
but the search-mode is still not active.

Review of attachment 349571 [details] [review]:

Thanks for the updated patch, Alessandro.

I found that if I start typing right after launching the application, the search entry loses focus right after typing the first character. I think that's because the "app.search" GAction's state is getting changed to FALSE right after the first key press when the application switches to the SEARCH mode. Keeping the GAction in sync with the SearchBar::search-mode-enabled seems to address this.

::: src/photos-searchbar.c
@@ +97,2 @@
   if (event_device != NULL)
     gd_entry_focus_hack (priv->search_entry, event_device);

This gd_entry_focus_hack() can be removed.

@@ +146,2 @@
   g_signal_connect_swapped (priv->search_entry,
                             "search-changed",

We should also connect to notify::search-mode-enabled to update the GAction whenever Searchbar::search-mode-enabled changes. This solves the above problem.

::: src/photos-searchbar.h
@@ +37,3 @@
 struct _PhotosSearchbarClass
 {
+  GtkSearchBarClass parent_class;

The G_DECLARE_* line needs to be updated.

Created attachment 351732 [details] [review]
photos-searchbar: Port to GtkSearchBar

I took the liberty to make the above changes.

Review of attachment 349573 [details] [review]:

Is this really needed? I don't see any similar code in gnome-documents. It seems to already do the right thing. For example, if I press escape after typing the Searchbar is concealed and we are out of the SEARCH mode. I don't know how to get the SearchBar to handle a key press without the "app.search" being TRUE.

Review of attachment 349572 [details] [review]:

This is covered by attachment 351732 [details] [review]. See comment 6.

However, we do need to remove gd_entry_focus_hack from photos_overview_searchbar_active_changed. The corresponding gnome-documents commit is https://git.gnome.org/browse/gnome-documents/commit/?id=d5c1f35c1ad7a740fc69024308e80946c0564c25

Created attachment 351735 [details] [review]
photos-searchbar: Port to GtkSearchBar

Let's stop #including gd.h.

Created attachment 351737 [details] [review]
overview-searchbar: Stop using gd_entry_focus_hack

Many thanks for tackling this hairy beast, Alessandro!

Created attachment 351894 [details] [review]
searchbar: Avoid CRITICALs when typing inside the preview

(In reply to Debarshi Ray from comment #8)
> Review of attachment 349573 [details] [review] [review]:
> 
> Is this really needed? I don't see any similar code in gnome-documents. It
> seems to already do the right thing. For example, if I press escape after
> typing the Searchbar is concealed and we are out of the SEARCH mode. I don't
> know how to get the SearchBar to handle a key press without the "app.search"
> being TRUE.

Probably because the problem has been fixed by keeping the GAction in sync with the SearchBar::search-mode-enabled as you have done in attachment 351732 [details] [review]. My attempt was a workaround because I didn't fully understand the issue.

(In reply to Alessandro Bono from comment #14)
> (In reply to Debarshi Ray from comment #8)
> > Review of attachment 349573 [details] [review] [review] [review]:
> > 
> > Is this really needed? I don't see any similar code in gnome-documents. It
> > seems to already do the right thing. For example, if I press escape after
> > typing the Searchbar is concealed and we are out of the SEARCH mode. I don't
> > know how to get the SearchBar to handle a key press without the "app.search"
> > being TRUE.
> 
> Probably because the problem has been fixed by keeping the GAction in sync
> with the SearchBar::search-mode-enabled as you have done in attachment
> 351732 [details] [review]. My attempt was a workaround because I didn't
> fully understand the issue.

Ok, I see. Closing the bug as FIXED.