After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 747962 - Rewrite search view
Rewrite search view
Status: RESOLVED FIXED
Product: gnome-calendar
Classification: Applications
Component: User Interface
3.16.x
Other Linux
: Normal normal
: 3.26
Assigned To: GNOME Calendar maintainers
GNOME Calendar maintainers
: 778889 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2015-04-16 04:18 UTC by Erick Perez Castellanos
Modified: 2017-05-04 12:00 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Don't show drop down patch. (2.15 KB, patch)
2017-03-16 12:05 UTC, Kevin Lopez
none Details | Review
Patch (2.15 KB, patch)
2017-03-17 00:28 UTC, Kevin Lopez
committed Details | Review
Patch (3.02 KB, patch)
2017-03-17 03:46 UTC, Kevin Lopez
none Details | Review
Patch (4.49 KB, patch)
2017-03-27 02:45 UTC, Kevin Lopez
none Details | Review
Video showing the behavior (1.33 MB, video/webm)
2017-03-28 11:28 UTC, Kevin Lopez
  Details
Patch (5.73 KB, patch)
2017-03-31 14:08 UTC, Kevin Lopez
none Details | Review
Patch (5.74 KB, patch)
2017-05-04 01:24 UTC, Kevin Lopez
none Details | Review
Patch (4.51 KB, patch)
2017-05-04 02:16 UTC, Kevin Lopez
committed Details | Review

Description Erick Perez Castellanos 2015-04-16 04:18:42 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.
Comment 1 Priyanshu Sinha 2017-02-24 16:35:09 UTC
2. Result drop down should disappear when any click is performed outside that drop down.
Comment 2 nikhilnandyala97 2017-02-25 06:49:31 UTC
This bug is duplicate of a bug which is already submitted https://bugzilla.gnome.org/show_bug.cgi?id=778890
Comment 3 nikhilnandyala97 2017-02-25 06:52:52 UTC
This bug is duplicate of a bug which is already submitted https://bugzilla.gnome.org/show_bug.cgi?id=778890
Comment 4 nikhilnandyala97 2017-02-25 06:53:06 UTC
This bug is duplicate of a bug which is already submitted https://bugzilla.gnome.org/show_bug.cgi?id=778890
Comment 5 Kevin Lopez 2017-03-16 12:05:43 UTC
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.
Comment 6 Georges Basile Stavracas Neto 2017-03-16 23:27:27 UTC
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()
Comment 7 Kevin Lopez 2017-03-17 00:28:23 UTC
Created attachment 348139 [details] [review]
Patch
Comment 8 Georges Basile Stavracas Neto 2017-03-17 00:30:56 UTC
Review of attachment 348139 [details] [review]:

Looks good, thanks!
Comment 9 Georges Basile Stavracas Neto 2017-03-17 00:32:41 UTC
Thanks for the patch. Pushed to master and gnome-3-24
Comment 10 Kevin Lopez 2017-03-17 00:45:32 UTC
(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!
Comment 11 Georges Basile Stavracas Neto 2017-03-17 00:47:15 UTC
The bug being fixed was a mistake from git-bz... Reopened it. There are many improvements left to do here.
Comment 12 Kevin Lopez 2017-03-17 03:46:16 UTC
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 !
Comment 13 Georges Basile Stavracas Neto 2017-03-23 01:55:14 UTC
Review of attachment 348144 [details] [review]:

This patch didn't apply against GNOME Calendar master. Please update it.
Comment 14 Kevin Lopez 2017-03-27 02:45:36 UTC
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!
Comment 15 Georges Basile Stavracas Neto 2017-03-28 03:03:07 UTC
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.
Comment 16 Kevin Lopez 2017-03-28 11:22:07 UTC
(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.
Comment 17 Kevin Lopez 2017-03-28 11:28:57 UTC
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.
Comment 18 Kevin Lopez 2017-03-31 14:08:12 UTC
Created attachment 349057 [details] [review]
Patch
Comment 19 Georges Basile Stavracas Neto 2017-04-03 20:21:58 UTC
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
Comment 20 Kevin Lopez 2017-04-14 01:06:51 UTC
(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!
Comment 21 Georges Basile Stavracas Neto 2017-04-28 13:57:55 UTC
(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.
Comment 22 Georges Basile Stavracas Neto 2017-04-28 20:53:27 UTC
*** Bug 778889 has been marked as a duplicate of this bug. ***
Comment 23 Kevin Lopez 2017-05-04 01:20:17 UTC
(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.
Comment 24 Kevin Lopez 2017-05-04 01:24:10 UTC
Created attachment 351017 [details] [review]
Patch
Comment 25 Kevin Lopez 2017-05-04 01:24:45 UTC
(In reply to Kevin Lopez from comment #24)
> Created attachment 351017 [details] [review] [review]
> Patch

Uploaded with the corrections.
Comment 26 Georges Basile Stavracas Neto 2017-05-04 01:43:17 UTC
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.
Comment 27 Kevin Lopez 2017-05-04 02:16:56 UTC
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!
Comment 28 Georges Basile Stavracas Neto 2017-05-04 11:54:10 UTC
Review of attachment 351019 [details] [review]:

LGTM
Comment 29 Georges Basile Stavracas Neto 2017-05-04 12:00:26 UTC
Attachment 351019 [details] pushed as ec3ef0d - search-view: hide if click is performed outside