GNOME Bugzilla – Bug 747962
Rewrite search view
Last modified: 2017-05-04 12:00:30 UTC
We have plans of rewriting the search view we have now. There's some other bugs related to the current implementation. I'll add here some other notes we might want to fix. 1. No need to show the results drop down when the search field is empty.
2. Result drop down should disappear when any click is performed outside that drop down.
This bug is duplicate of a bug which is already submitted https://bugzilla.gnome.org/show_bug.cgi?id=778890
Created attachment 348075 [details] [review] Don't show drop down patch. (In reply to Erick Perez Castellanos from comment #0) > 1. No need to show the results drop down when the search field is empty. I also deleted the binding property between the search_button and the search_view, to hide the drop down when the search button is toggled. I'm not sure if this is the correct behavior, if i have to change that only tell me.
Review of attachment 348075 [details] [review]: A couple of minor improvements. ::: src/gcal-window.c @@ +1198,3 @@ { GcalWindow *window = GCAL_WINDOW (user_data); + gint no_of_words; Better name it 'search_length' @@ +1204,3 @@ + /* perform the search or hide when the field is empty */ + + no_of_words = strlen (gtk_entry_get_text (GTK_ENTRY (window->search_entry))); GtkEntry has API for that. You should use gtk_entry_get_text_length()
Created attachment 348139 [details] [review] Patch
Review of attachment 348139 [details] [review]: Looks good, thanks!
Thanks for the patch. Pushed to master and gnome-3-24
(In reply to Georges Basile Stavracas Neto from comment #9) > Thanks for the patch. Pushed to master and gnome-3-24 I saw, that the bug now is marked as fixed, but I'm currently thinking what will be the best approach to solve the second issue (Result drop down should disappear when any click is performed outside that drop down.). Regards!
The bug being fixed was a mistake from git-bz... Reopened it. There are many improvements left to do here.
Created attachment 348144 [details] [review] Patch (In reply to Priyanshu Sinha from comment #1) > 2. Result drop down should disappear when any click is performed outside > that drop down. Hope it is ok. Regards !
Review of attachment 348144 [details] [review]: This patch didn't apply against GNOME Calendar master. Please update it.
Created attachment 348764 [details] [review] Patch (In reply to Georges Basile Stavracas Neto from comment #13) > Review of attachment 348144 [details] [review] [review]: > > This patch didn't apply against GNOME Calendar master. Please update it. Sure! To solve this bug I'm not sure if I take the best approach. I propagate the button-press events in the views, to allow the GtkWindow catch them, and next decide what to do. Tell me what do you think ? Regards!
Review of attachment 348764 [details] [review]: This patch looks good, but introduces a new problem. After the search is hidden, it doesn't show again. I'm not sure this approach is the best one. What do you think about adding a new function to GcalSearchView that handles that, instead of GcalWindow? We could then track the focus of the search entry, and show the search popover again when the entry is focused.
(In reply to Georges Basile Stavracas Neto from comment #15) > Review of attachment 348764 [details] [review] [review]: > > This patch looks good, but introduces a new problem. After the search is > hidden, it doesn't show again. > > I'm not sure this approach is the best one. What do you think about adding a > new function to GcalSearchView that handles that, instead of GcalWindow? We > could then track the focus of the search entry, and show the search popover > again when the entry is focused. In GcalSearchView, I think that we cannot track the focus of the search entry, since GCalSearchView is the class for only the GtkPopover, so we don't have access to the search_entry widget.
Created attachment 348881 [details] Video showing the behavior Tell me what do you think about the behavior of the search_view showed in the attachment. When the search_view is hided, the user can click another time in the search_entry to show the popover again.
Created attachment 349057 [details] [review] Patch
Review of attachment 349057 [details] [review]: Some nitpicks ::: src/gcal-window.c @@ +120,3 @@ + gint click_outside_handler_id; + gint focus_handler_id; + You should also disconnect in finalize() @@ +183,3 @@ + search_view_window = gtk_widget_get_window (GTK_WIDGET (popover)); + + GtkPopover *popover) Typo: remove the 'he' @@ +187,3 @@ + { + gtk_popover_popdown (popover); + GdkWindow *search_view_window; You don't need to put the {} for single-line if()s @@ +1141,3 @@ + + /* When the search button is toogled we connect the signal */ + window->click_outside_handler_id = g_signal_connect (G_OBJECT (window), Don't need to cast to GObject @@ +1144,3 @@ + "button-press-event", + G_CALLBACK (hide_search_view_on_click_outside), + (gpointer) window->search_view); No need for the (gpointer) casts @@ +1149,3 @@ + "button-press-event", + G_CALLBACK (show_search_view), + "button-press-event", Ditto @@ +1155,3 @@ + /* When the search mode is false we disconnect the handler */ + g_signal_handler_disconnect ((gpointer) window, window->click_outside_handler_id); + Ditto
(In reply to Georges Basile Stavracas Neto from comment #19) > Review of attachment 349057 [details] [review] [review]: > > Some nitpicks > > ::: src/gcal-window.c > @@ +120,3 @@ > + gint click_outside_handler_id; > + gint focus_handler_id; > + > > You should also disconnect in finalize() Thanks for the review!. I have one doubt, why we need to disconnect the handlers also in finalize() ? I've disconnected the handlers in finalize() and when I close gnome-calendar, these errors appears: GLib-GObject: WARNING: /home/kevin/Jhbuild/checkout/glib/gobject/gsignal.c:2641: instance '0x1d5f0a0' has no handler with id '6562' GLib-GObject: WARNING: invalid (NULL) pointer instance GLib-GObject: CRITICAL: g_signal_handler_disconnect: assertion 'G_TYPE_CHECK_INSTANCE (instance)' failed Regards!
(In reply to Kevin Lopez from comment #20) > Thanks for the review!. > I have one doubt, why we need to disconnect the handlers also in finalize() ? > > I've disconnected the handlers in finalize() and when I close > gnome-calendar, these errors appears: > > GLib-GObject: WARNING: > /home/kevin/Jhbuild/checkout/glib/gobject/gsignal.c:2641: instance > '0x1d5f0a0' has no handler with id '6562' > GLib-GObject: WARNING: invalid (NULL) pointer instance > GLib-GObject: CRITICAL: g_signal_handler_disconnect: assertion > 'G_TYPE_CHECK_INSTANCE (instance)' failed > > > Regards! So I guess that's not correct, and you shouldn't disconnect in finalize() indeed.
*** Bug 778889 has been marked as a duplicate of this bug. ***
(In reply to Georges Basile Stavracas Neto from comment #21) > (In reply to Kevin Lopez from comment #20) > So I guess that's not correct, and you shouldn't disconnect in finalize() > indeed. I was debugging that issue with cgdb and one problem is if we disconnect the handlers in finalize method and the user don't toggle the search button and close the app , a warning happens since the handlers are only initialized if the user toggle the search button. So I think that as you said we don't have to disconnect the handlers in finalize.
Created attachment 351017 [details] [review] Patch
(In reply to Kevin Lopez from comment #24) > Created attachment 351017 [details] [review] [review] > Patch Uploaded with the corrections.
Review of attachment 351017 [details] [review]: One last nitpick. ::: src/gcal-window.c @@ +228,3 @@ + /* If the event is not produced in the search view widget, we hide it */ + if (event->window != search_view_window) + gtk_popover_popdown (popover); Instead of just hiding the popover, this should toggle the search mode entirely.
Created attachment 351019 [details] [review] Patch (In reply to Georges Basile Stavracas Neto from comment #26) > Review of attachment 351017 [details] [review] [review]: > > One last nitpick. > > ::: src/gcal-window.c > @@ +228,3 @@ > + /* If the event is not produced in the search view widget, we hide it */ > + if (event->window != search_view_window) > + gtk_popover_popdown (popover); > > Instead of just hiding the popover, this should toggle the search mode > entirely. Makes sense ! One note about this patch : Hiding the popover also before we hide the search bar produces a more nice animation at least for me instead of only hiding the search bar. Regards!
Review of attachment 351019 [details] [review]: LGTM
Attachment 351019 [details] pushed as ec3ef0d - search-view: hide if click is performed outside