GNOME Bugzilla – Bug 700787
Add GtkSearchBar widget
Last modified: 2013-05-31 15:10:06 UTC
This adds a search bar widget. Having this in GTK+ would avoid perpetuating hacks like the one removed from gtktreeview.c (and used in gd-entry-focus-hack.c), or reimplementing tricky code like the preedit querying. The documentation isn't great (should have code examples) and we might want to add a property to mimic "reveal-child" ("search-mode"?)
Created attachment 244963 [details] [review] Add GtkSearchBar widget This widget is a toolbar that will popup automatically when searches should be started, and dismissed when they are finished.
Created attachment 244964 [details] [review] GtkEntry: Focus the entry without selecting the text Focusing the text entry without selecting all the text is needed in some places (GtkTreeView, and some uses of GtkSearchEntry) so create a private helper to avoid replicating the hacks.
Created attachment 244965 [details] [review] GtkTreeView: Use GtkEntry private function to remove hack
Created attachment 244966 [details] [review] GtkSearchEntry: Don't wait for timeout when emptying When the text entry gets cleared, emit the "changed" signal straight away. This avoids a lag when dismissing a search.
Created attachment 244981 [details] [review] Add GtkSearchBar widget This widget is a toolbar that will popup automatically when searches should be started, and dismissed when they are finished. https://bugzilla.gnome.org/show_bug.cgi?id=700787 v2: - Make it possible to set search_entry and container after creation - Make gtk_container_add() work as expected (so it works in GtkBuilder)
Review of attachment 244964 [details] [review]: I suspect in those cases, we want the cursor to be moved to the end ?
Review of attachment 244964 [details] [review]: .
Review of attachment 244965 [details] [review]: nice cleanup
Review of attachment 244966 [details] [review]: makes sense
Review of attachment 244964 [details] [review]: No, this simply makes the entry take keyboard focus. There are no functional changes in this patch.
Review of attachment 244981 [details] [review]: Can we make this have a GtkRevealer as an internal child, instead of deriving from GtkRevealer ? ::: gtk/gtksearchbar.c @@ +43,3 @@ + * #GtkSearchBar is toolbar which will show when the application + * is in search mode, and hide when it's not. It will also handle + * focusing itself if key presses are made in the application's view. This needs quite a bit more information how this is supposed to be used. An example, at least. I wouldn't be sure, myself - stuff your content in an overview and put the overlay in that ? Also, the introduction should say something about how key events are collected and handled - after all, thats the 'special sauce' here. @@ +138,3 @@ + keyval == GDK_KEY_Up || + keyval == GDK_KEY_KP_Up || + keyval == GDK_KEY_Up || you've got your ups doubled, here @@ +154,3 @@ + keyval == GDK_KEY_KP_Page_Down || + ((state & (GDK_CONTROL_MASK | GDK_MOD1_MASK)) != 0)) + return TRUE; I think a fixed list of keysyms is a somewhat limiting approach, that we won't be able to get right - e.g. this list misses f10 (focus menubar). You probably have to tailor this list per-platform, as well. Also, why is this needed in the first place ? @@ +176,3 @@ + return FALSE; + + //FIXME search-mode? What does this mean ? @@ +204,3 @@ + * Returns: %TRUE if the key press event resulted in text being + * entered in the search entry, %FALSE otherwise. + **/ Hmm, I don't really like the manual nature of this. Maybe add a gtk_search_bar_set_toplevel that does the right thing without the app having to manually juggle key events ? It could also use connect-after, and avoid the need to do this imperfect filtering of keynav and other events @@ +221,3 @@ + /* Exit early if the search bar is already shown, + * the event doesn't contain a key press, + * or the event is a navigation or space bar key press */ Pet peeve: move the */ to the next line and line it up with the leading *s @@ +322,3 @@ + GTK_TYPE_WIDGET, + G_PARAM_READWRITE | G_PARAM_CONSTRUCT)); + I don't like exposing these as properties. Widget-valued properties are somewhat iffy, in particular writable properties. If we need these exposed, we should add getters. I could see that for the entry, but why the container ? Without saying anything about the nature of the container (hbox, vbox, grid ?) it seems of very limited use. If you need it, you can always call get_parent on the entry. @@ +372,3 @@ + "container", container, + NULL)); +} I find it really ugly to pass in a half-constructed widget tree like that. What are the use cases that require us to a) make the entry replaceable b) make this open-ended extensible by passing a container ?
(In reply to comment #11) > Review of attachment 244981 [details] [review]: > > Can we make this have a GtkRevealer as an internal child, instead of deriving > from GtkRevealer ? I'm not sure how to do that. > ::: gtk/gtksearchbar.c > @@ +43,3 @@ > + * #GtkSearchBar is toolbar which will show when the application > + * is in search mode, and hide when it's not. It will also handle > + * focusing itself if key presses are made in the application's view. > > This needs quite a bit more information how this is supposed to be used. An > example, at least. I wouldn't be sure, myself - stuff your content in an > overview and put the overlay in that ? Also, the introduction should say > something about how key events are collected and handled - after all, thats the > 'special sauce' here. There's documentation in v3 of the patch. > @@ +138,3 @@ > + keyval == GDK_KEY_Up || > + keyval == GDK_KEY_KP_Up || > + keyval == GDK_KEY_Up || > > you've got your ups doubled, here They were copy/pasted from gnome-documents, it has that bug indeed. > @@ +154,3 @@ > + keyval == GDK_KEY_KP_Page_Down || > + ((state & (GDK_CONTROL_MASK | GDK_MOD1_MASK)) != 0)) > + return TRUE; > > I think a fixed list of keysyms is a somewhat limiting approach, that we won't > be able to get right - e.g. this list misses f10 (focus menubar). > You probably have to tailor this list per-platform, as well. > > Also, why is this needed in the first place ? So that navigation events don't trigger the search entry. > @@ +176,3 @@ > + return FALSE; > + > + //FIXME search-mode? > > What does this mean ? I was wondering whether to add a search-mode property. I've added it in v3 of the patch. > @@ +204,3 @@ > + * Returns: %TRUE if the key press event resulted in text being > + * entered in the search entry, %FALSE otherwise. > + **/ > > Hmm, I don't really like the manual nature of this. > Maybe add a gtk_search_bar_set_toplevel that does the right thing without the > app having to manually juggle key events ? > It could also use connect-after, and avoid the need to do this imperfect > filtering of keynav and other events This is needed so that applications can choose whether to automatically popup the search entry. In Totem for example, the same top-level window is used for the overview, and for playback. I don't want a search to start when pressing keys that would be used to control the playback. > @@ +221,3 @@ > + /* Exit early if the search bar is already shown, > + * the event doesn't contain a key press, > + * or the event is a navigation or space bar key press */ > > Pet peeve: move the */ to the next line and line it up with the leading *s Sure, fixed in v3. > @@ +322,3 @@ > + GTK_TYPE_WIDGET, > + G_PARAM_READWRITE | > G_PARAM_CONSTRUCT)); > + > > I don't like exposing these as properties. Widget-valued properties are > somewhat iffy, in particular writable properties. > > If we need these exposed, we should add getters. I could see that for the > entry, but why the container ? Without saying anything about the nature of the > container (hbox, vbox, grid ?) it seems of very limited use. If you need it, > you can always call get_parent on the entry. I've removed the container widget property in v3. I added the GtkContainer ->add() support after that. > @@ +372,3 @@ > + "container", container, > + NULL)); > +} > > I find it really ugly to pass in a half-constructed widget tree like that. > > What are the use cases that require us to > > a) make the entry replaceable > b) make this open-ended extensible by passing a container The child widget usually contains an entry (or sub-class of an entry) usually along with drop-downs, bubbles for tags, etc. A simple GtkSearchEntry is inadequate for more complicated uses.
Created attachment 245028 [details] [review] Add GtkSearchBar widget This widget is a toolbar that will popup automatically when searches should be started, and dismissed when they are finished. https://bugzilla.gnome.org/show_bug.cgi?id=700787 v2: - Make it possible to set search_entry and container after creation - Make gtk_container_add() work as expected (so it works in GtkBuilder) v3: - Remove unneeded forward declaration by moving get/set props - Fold one-liner "grab focus" helper into calling func - Add search-mode property - Better documentation with examples - Add toggle button in example, and center the search widget - Remove duplicate GDK_Key_Up - Fix comment style - Remove container property (we use the container class' ->add() instead)
Created attachment 245041 [details] [review] Add GtkSearchBar widget This widget is a toolbar that will popup automatically when searches should be started, and dismissed when they are finished. https://bugzilla.gnome.org/show_bug.cgi?id=700787 v2: - Make it possible to set search_entry and container after creation - Make gtk_container_add() work as expected (so it works in GtkBuilder) v3: - Remove unneeded forward declaration by moving get/set props - Fold one-liner "grab focus" helper into calling func - Add search-mode property - Better documentation with examples - Add toggle button in example, and center the search widget - Remove duplicate GDK_Key_Up - Fix comment style - Remove container property (we use the container class' ->add() instead) v4: - Descend from a GtkBin as recommended - Use widget templates to create the composite widget
Created attachment 245057 [details] [review] Add GtkSearchBar widget This widget is a toolbar that will popup automatically when searches should be started, and dismissed when they are finished. https://bugzilla.gnome.org/show_bug.cgi?id=700787 v2: - Make it possible to set search_entry and container after creation - Make gtk_container_add() work as expected (so it works in GtkBuilder) v3: - Remove unneeded forward declaration by moving get/set props - Fold one-liner "grab focus" helper into calling func - Add search-mode property - Better documentation with examples - Add toggle button in example, and center the search widget - Remove duplicate GDK_Key_Up - Fix comment style - Remove container property (we use the container class' ->add() instead) v4: - Descend from a GtkBin as recommended - Use widget templates to create the composite widget v5: - Add get/set accessors for the entry property
Created attachment 245161 [details] [review] Add GtkSearchBar widget This widget is a toolbar that will popup automatically when searches should be started, and dismissed when they are finished. https://bugzilla.gnome.org/show_bug.cgi?id=700787 v2: - Make it possible to set search_entry and container after creation - Make gtk_container_add() work as expected (so it works in GtkBuilder) v3: - Remove unneeded forward declaration by moving get/set props - Fold one-liner "grab focus" helper into calling func - Add search-mode property - Better documentation with examples - Add toggle button in example, and center the search widget - Remove duplicate GDK_Key_Up - Fix comment style - Remove container property (we use the container class' ->add() instead) v4: - Descend from a GtkBin as recommended - Use widget templates to create the composite widget v5: - Add get/set accessors for the entry property v6: - Add show-close-button property (as used in epiphany's search toolbar)
Attachment 244964 [details] pushed as ffbe7f6 - GtkEntry: Focus the entry without selecting the text Attachment 244965 [details] pushed as 1bfbfbc - GtkTreeView: Use GtkEntry private function to remove hack Attachment 244966 [details] pushed as 536fc22 - GtkSearchEntry: Don't wait for timeout when emptying
Created attachment 245479 [details] [review] Add GtkSearchBar widget This widget is a toolbar that will popup automatically when searches should be started, and dismissed when they are finished. https://bugzilla.gnome.org/show_bug.cgi?id=700787 v2: - Make it possible to set search_entry and container after creation - Make gtk_container_add() work as expected (so it works in GtkBuilder) v3: - Remove unneeded forward declaration by moving get/set props - Fold one-liner "grab focus" helper into calling func - Add search-mode property - Better documentation with examples - Add toggle button in example, and center the search widget - Remove duplicate GDK_Key_Up - Fix comment style - Remove container property (we use the container class' ->add() instead) v4: - Descend from a GtkBin as recommended - Use widget templates to create the composite widget v5: - Add get/set accessors for the entry property v6: - Add show-close-button property (as used in epiphany's search toolbar) v7: - Add example for more advanced search widget - Add accessors for show-close-button and search-entry properties
Review of attachment 245479 [details] [review]: ::: gtk/gtksearchbar.c @@ +184,3 @@ + * </example> + * + * Returns: %TRUE if the key press event resulted in text being should be "Return value", and probably you want to use %GDK_EVENT_PROPAGATE and %GDK_EVENT_STOP instead of the boolean constants. @@ +187,3 @@ + * entered in the search entry (and revealing the search bar if + * necessary), %FALSE otherwise. + **/ missing Since: annotation. @@ +284,3 @@ + +static void +gtk_search_bar_finalize (GObject *object) if there isn't anything to finalize, just don't override finalize. spares us a type check, a function call, and a pointer redirection. @@ +352,3 @@ + container_class->add = gtk_search_bar_add; + + g_object_class_install_property (object_class, missing documentation annotation. @@ +358,3 @@ + P_("The entry widget in the search bar"), + GTK_TYPE_ENTRY, + G_PARAM_READWRITE | G_PARAM_CONSTRUCT)); why are these properties G_PARAM_CONSTRUCT? there is no constructor for them, and you don't override the constructed or constructor virtual functions. @@ +360,3 @@ + G_PARAM_READWRITE | G_PARAM_CONSTRUCT)); + + g_object_class_install_property (object_class, same as above. @@ +362,3 @@ + g_object_class_install_property (object_class, + PROP_SEARCH_MODE, + g_param_spec_boolean ("search-mode", search-mode sounds awfully like a modal control; those kinds of properties that we usually control with an enumeration. maybe "enable-search-mode" is a better name. or "search-mode-enabled". @@ +368,3 @@ + G_PARAM_READWRITE | G_PARAM_CONSTRUCT)); + + g_object_class_install_property (object_class, same as above. @@ +370,3 @@ + g_object_class_install_property (object_class, + PROP_SHOW_CLOSE_BUTTON, + g_param_spec_boolean ("show-close-button", the idiomatic way to install properties is to define a global array of GParamSpecs and then use g_object_class_install_properties() and g_object_notify_by_pspec(), e.g.: enum { PROP_0, PROP_ENTRY, PROP_SEARCH_MODE, PROP_SHOW_CLOSE_BUTTON, LAST_PROPERTY }; static GParamSpec *widget_props[LAST_PROPERTY] = { NULL, }; ... widget_props[PROP_ENTRY] = g_param_spec_object ("entry", ...); widget_props[PROP_SEARCH_MODE] = g_param_spec_boolean ("search-mode", ...); g_object_class_install_properties (object_class, LAST_PROPERTY, widget_props); @@ +416,3 @@ +gtk_search_bar_new (void) +{ + return GTK_WIDGET (g_object_new (GTK_TYPE_SEARCH_BAR, no need to cast the return value: g_object_new() returns a gpointer, and the constructor cannot fail. @@ +440,3 @@ + +/** + * gtk_search_bar_set_entry: I'm moderately puzzled as to how this widget should be used just by reading at its documentation, to be honest. the search bar acquires a pointer (not even a reference) to a GtkEntry which is not its child. what happens if the GtkEntry is destroyed before the GtkSearchBar? at the very least, the SearchBar should acquire either a strong reference (and watch the ::destroy signal) or a weak pointer to NULL-ify the priv->entry pointer, otherwise we may get segfaults later on. but in general, this API sounds odd to me. the search entry is not a child of the search bar? the example in gtk-demo seems to be packing the entry in a container and then setting the search entry of the search bar; feels a bit contrived to me. can you explain a bit of the use case, here? also, the documentation is *far* too sparse if you expect developers to understand the implications of calling gtk_search_bar_set_entry(). @@ +472,3 @@ + * gtk_search_bar_get_search_mode: + * @bar: a #GtkSearchBar + * missing description. @@ +490,3 @@ + * @search_mode: the new state of the search mode + * + * Switch the search mode on or off. Switches. @@ +506,3 @@ + * gtk_search_bar_get_show_close_button: + * @bar: a #GtkSearchBar + * missing description. ::: gtk/gtksearchbar.h @@ +48,3 @@ +typedef struct _GtkSearchBarClass GtkSearchBarClass; + +struct _GtkSearchBar missing gtk-doc annotation.
Created attachment 245552 [details] [review] Add GtkSearchBar widget This widget is a toolbar that will popup automatically when searches should be started, and dismissed when they are finished. https://bugzilla.gnome.org/show_bug.cgi?id=700787 v2: - Make it possible to set search_entry and container after creation - Make gtk_container_add() work as expected (so it works in GtkBuilder) v3: - Remove unneeded forward declaration by moving get/set props - Fold one-liner "grab focus" helper into calling func - Add search-mode property - Better documentation with examples - Add toggle button in example, and center the search widget - Remove duplicate GDK_Key_Up - Fix comment style - Remove container property (we use the container class' ->add() instead) v4: - Descend from a GtkBin as recommended - Use widget templates to create the composite widget v5: - Add get/set accessors for the entry property v6: - Add show-close-button property (as used in epiphany's search toolbar) v7: - Add example for more advanced search widget - Add accessors for show-close-button and search-entry properties v8: - Fix review comments
(In reply to comment #19) > Review of attachment 245479 [details] [review]: > > ::: gtk/gtksearchbar.c > @@ +184,3 @@ > + * </example> > + * > + * Returns: %TRUE if the key press event resulted in text being > > should be "Return value", and probably you want to use %GDK_EVENT_PROPAGATE and > %GDK_EVENT_STOP instead of the boolean constants. Good point, done. > @@ +187,3 @@ > + * entered in the search entry (and revealing the search bar if > + * necessary), %FALSE otherwise. > + **/ > > missing Since: annotation. Added. > @@ +284,3 @@ > + > +static void > +gtk_search_bar_finalize (GObject *object) > > if there isn't anything to finalize, just don't override finalize. spares us a > type check, a function call, and a pointer redirection. Removed. > @@ +352,3 @@ > + container_class->add = gtk_search_bar_add; > + > + g_object_class_install_property (object_class, > > missing documentation annotation. Added. > @@ +358,3 @@ > + P_("The entry widget > in the search bar"), > + GTK_TYPE_ENTRY, > + G_PARAM_READWRITE | > G_PARAM_CONSTRUCT)); > > why are these properties G_PARAM_CONSTRUCT? there is no constructor for them, > and you don't override the constructed or constructor virtual functions. You only need to override those functions if you can't do that job in the property setter. In this case, it's not required, but it's needed so that the close button is shown by default. > @@ +360,3 @@ > + G_PARAM_READWRITE | > G_PARAM_CONSTRUCT)); > + > + g_object_class_install_property (object_class, > > same as above. Done. > @@ +362,3 @@ > + g_object_class_install_property (object_class, > + PROP_SEARCH_MODE, > + g_param_spec_boolean ("search-mode", > > search-mode sounds awfully like a modal control; those kinds of properties that > we usually control with an enumeration. > > maybe "enable-search-mode" is a better name. or "search-mode-enabled". I've changed it to search-mode-enabled. > @@ +368,3 @@ > + G_PARAM_READWRITE | > G_PARAM_CONSTRUCT)); > + > + g_object_class_install_property (object_class, > > same as above. Done. > @@ +370,3 @@ > + g_object_class_install_property (object_class, > + PROP_SHOW_CLOSE_BUTTON, > + g_param_spec_boolean ("show-close-button", > > the idiomatic way to install properties is to define a global array of > GParamSpecs and then use g_object_class_install_properties() and > g_object_notify_by_pspec(), e.g.: > > enum { > PROP_0, > PROP_ENTRY, > PROP_SEARCH_MODE, > PROP_SHOW_CLOSE_BUTTON, > LAST_PROPERTY > }; > > static GParamSpec *widget_props[LAST_PROPERTY] = { NULL, }; > > ... > widget_props[PROP_ENTRY] = > g_param_spec_object ("entry", ...); > > widget_props[PROP_SEARCH_MODE] = > g_param_spec_boolean ("search-mode", ...); > > g_object_class_install_properties (object_class, LAST_PROPERTY, > widget_props); It's not what a majority of (historical) widgets do, but I'll change it. Recommended, or required rather than idiomatic. > @@ +416,3 @@ > +gtk_search_bar_new (void) > +{ > + return GTK_WIDGET (g_object_new (GTK_TYPE_SEARCH_BAR, > > no need to cast the return value: g_object_new() returns a gpointer, and the > constructor cannot fail. Fixed. > @@ +440,3 @@ > + > +/** > + * gtk_search_bar_set_entry: > > I'm moderately puzzled as to how this widget should be used just by reading at > its documentation, to be honest. There's example code at the top of the file, in the section header. This is not clearly visible when reading the source code (as opposed to reading the API docs in devhelp or the website). > the search bar acquires a pointer (not even a reference) to a GtkEntry which is > not its child. what happens if the GtkEntry is destroyed before the > GtkSearchBar? at the very least, the SearchBar should acquire either a strong > reference (and watch the ::destroy signal) or a weak pointer to NULL-ify the > priv->entry pointer, otherwise we may get segfaults later on. > > but in general, this API sounds odd to me. the search entry is not a child of > the search bar? It doesn't have to be a direct child. See the example at the top of the file. > the example in gtk-demo seems to be packing the entry in a > container and then setting the search entry of the search bar; feels a bit > contrived to me. can you explain a bit of the use case, here? It's a very simple one. Every search entry that needs one or more buttons next to the search entry (Web's up/down arrows). Or composite widgets that use an entry and a source drop-down (gnome-documents' source selection). > also, the documentation is *far* too sparse if you expect developers to > understand the implications of calling gtk_search_bar_set_entry(). It's explained at the top of the file. > @@ +472,3 @@ > + * gtk_search_bar_get_search_mode: > + * @bar: a #GtkSearchBar > + * > > missing description. Done. > @@ +490,3 @@ > + * @search_mode: the new state of the search mode > + * > + * Switch the search mode on or off. > > Switches. Done. > @@ +506,3 @@ > + * gtk_search_bar_get_show_close_button: > + * @bar: a #GtkSearchBar > + * > > missing description. Done. > ::: gtk/gtksearchbar.h > @@ +48,3 @@ > +typedef struct _GtkSearchBarClass GtkSearchBarClass; > + > +struct _GtkSearchBar > > missing gtk-doc annotation. /*< private >*/ ? Done.
Review of attachment 245479 [details] [review]: ::: gtk/gtksearchbar.c @@ +120,3 @@ + keyval == GDK_KEY_Page_Down || + keyval == GDK_KEY_KP_Page_Down || + ((state & (GDK_CONTROL_MASK | GDK_MOD1_MASK)) != 0)) I'm not very comfortable with this, still. It misses tons of possible keynav - e.g. what about arrow keys, backspace, enter ? @@ +362,3 @@ + g_object_class_install_property (object_class, + PROP_SEARCH_MODE, + g_param_spec_boolean ("search-mode", Or search-bar-visible ?
(In reply to comment #22) > Review of attachment 245479 [details] [review]: > > ::: gtk/gtksearchbar.c > @@ +120,3 @@ > + keyval == GDK_KEY_Page_Down || > + keyval == GDK_KEY_KP_Page_Down || > + ((state & (GDK_CONTROL_MASK | GDK_MOD1_MASK)) != 0)) > > I'm not very comfortable with this, still. > > It misses tons of possible keynav - e.g. what about arrow keys, backspace, > enter ? I'm not sure that keynav needs to be specifically handled actually, it will be ignored by the IM set on the entry. > @@ +362,3 @@ > + g_object_class_install_property (object_class, > + PROP_SEARCH_MODE, > + g_param_spec_boolean ("search-mode", > > Or search-bar-visible ? I've just changed it to search-mode-enabled, should I change it again?
(In reply to comment #23) > (In reply to comment #22) > > Review of attachment 245479 [details] [review] [details]: > > > > ::: gtk/gtksearchbar.c > > @@ +120,3 @@ > > + keyval == GDK_KEY_Page_Down || > > + keyval == GDK_KEY_KP_Page_Down || > > + ((state & (GDK_CONTROL_MASK | GDK_MOD1_MASK)) != 0)) > > > > I'm not very comfortable with this, still. > > > > It misses tons of possible keynav - e.g. what about arrow keys, backspace, > > enter ? > > I'm not sure that keynav needs to be specifically handled actually, it will be > ignored by the IM set on the entry. Let me add that arrow keys are already there.
Created attachment 245563 [details] [review] Add GtkSearchBar widget This widget is a toolbar that will popup automatically when searches should be started, and dismissed when they are finished. https://bugzilla.gnome.org/show_bug.cgi?id=700787 v2: - Make it possible to set search_entry and container after creation - Make gtk_container_add() work as expected (so it works in GtkBuilder) v3: - Remove unneeded forward declaration by moving get/set props - Fold one-liner "grab focus" helper into calling func - Add search-mode property - Better documentation with examples - Add toggle button in example, and center the search widget - Remove duplicate GDK_Key_Up - Fix comment style - Remove container property (we use the container class' ->add() instead) v4: - Descend from a GtkBin as recommended - Use widget templates to create the composite widget v5: - Add get/set accessors for the entry property v6: - Add show-close-button property (as used in epiphany's search toolbar) v7: - Add example for more advanced search widget - Add accessors for show-close-button and search-entry properties v8: - Fix review comments v9: - Remove entry property - Add other widget in example code - Rename _set_entry() to _connect_entry() and fix refcounting of the search widget - Special case GtkEntry as the only child
Created attachment 245733 [details] [review] Add GtkSearchBar widget This widget is a toolbar that will popup automatically when searches should be started, and dismissed when they are finished. https://bugzilla.gnome.org/show_bug.cgi?id=700787 v2: - Make it possible to set search_entry and container after creation - Make gtk_container_add() work as expected (so it works in GtkBuilder) v3: - Remove unneeded forward declaration by moving get/set props - Fold one-liner "grab focus" helper into calling func - Add search-mode property - Better documentation with examples - Add toggle button in example, and center the search widget - Remove duplicate GDK_Key_Up - Fix comment style - Remove container property (we use the container class' ->add() instead) v4: - Descend from a GtkBin as recommended - Use widget templates to create the composite widget v5: - Add get/set accessors for the entry property v6: - Add show-close-button property (as used in epiphany's search toolbar) v7: - Add example for more advanced search widget - Add accessors for show-close-button and search-entry properties v8: - Fix review comments v9: - Remove entry property - Add other widget in example code - Rename _set_entry() to _connect_entry() and fix refcounting of the search widget - Special case GtkEntry as the only child v10: - 2 liner in ui file to fix sizing problems
Review of attachment 245733 [details] [review]: It would be nice to get the a11y team to look at this widget with orca eyes - but that doesn't block merging it. Remaining comments are just cosmetics (see below). I think it is good to merge with those comments addressed, unless ebassi has more complaints. ::: gtk/gtksearchbar.c @@ +48,3 @@ + * For keyboard presses to start a search, events will need to be forwarded + * from the top-level window that contains the search bar. See + * gtk_search_bar_handle_event() for example code. It might be good to mention gtk_search_bar_connect_entry somewhere here in the overview. Forgetting that is a pretty easy trap to fall into, otherwise. Maybe the code should even warn if you forward key events to the search bar, but don't have an entry connected ? @@ +128,3 @@ + + /* Other navigation events should get automatically + * ignored as they will not change the content of the entry */ Pet peeve, line up the */ in the next line @@ +134,3 @@ + +static gboolean +is_space_event (guint keyval) is_space_event sounds just weird, in particular since you don't pass an event. @@ +147,3 @@ + + if (!gdk_event_get_keyval (event, &keyval) || + keyval != GDK_KEY_Escape) ... and here you put the keyval comparison inline. I think you shoul dprobably do the same for space and ditch is_space_event @@ +261,3 @@ + + if (reveal_child) + _gtk_entry_grab_focus (GTK_ENTRY (bar->priv->entry), FALSE); do you also need to move the cursor to the end here ? or maybe gtk_entry_grab_focus (...FALSE) should do that @@ +360,3 @@ + { + g_signal_handlers_disconnect_by_func (bar->priv->entry, entry_key_pressed_event_cb, bar); + g_object_weak_unref (G_OBJECT (bar->priv->entry), entry_vanished, bar); I think technically, this belongs in dispose, not in finalize
Review of attachment 245733 [details] [review]: minor cosmetic issue here as well. ::: gtk/gtksearchbar.c @@ +478,3 @@ + { + bar->priv->entry = GTK_WIDGET (entry); + g_object_weak_ref (G_OBJECT (bar->priv->entry), entry_vanished, bar); you can get away with g_object_add_weak_pointer() instead of a weak_ref() + function.
(In reply to comment #27) > Review of attachment 245733 [details] [review]: > > It would be nice to get the a11y team to look at this widget with orca eyes - > but that doesn't block merging it. > > Remaining comments are just cosmetics (see below). I think it is good to merge > with those comments addressed, unless ebassi has more complaints. > > ::: gtk/gtksearchbar.c > @@ +48,3 @@ > + * For keyboard presses to start a search, events will need to be forwarded > + * from the top-level window that contains the search bar. See > + * gtk_search_bar_handle_event() for example code. > > It might be good to mention gtk_search_bar_connect_entry somewhere here in the > overview. Done. > Forgetting that is a pretty easy trap to fall into, otherwise. Maybe the code > should even warn if you forward key events to the search bar, but don't have an > entry connected ? Added a warning and a link to _connect_entry(). > @@ +128,3 @@ > + > + /* Other navigation events should get automatically > + * ignored as they will not change the content of the entry */ > > Pet peeve, line up the */ in the next line Done. > @@ +134,3 @@ > + > +static gboolean > +is_space_event (guint keyval) > > is_space_event sounds just weird, in particular since you don't pass an event. > > @@ +147,3 @@ > + > + if (!gdk_event_get_keyval (event, &keyval) || > + keyval != GDK_KEY_Escape) > > ... and here you put the keyval comparison inline. I think you shoul dprobably > do the same for space and ditch is_space_event Ditched. > @@ +261,3 @@ > + > + if (reveal_child) > + _gtk_entry_grab_focus (GTK_ENTRY (bar->priv->entry), FALSE); > > do you also need to move the cursor to the end here ? or maybe > gtk_entry_grab_focus (...FALSE) should do that The cursor will automatically be moved by the fact that we're entering text. > @@ +360,3 @@ > + { > + g_signal_handlers_disconnect_by_func (bar->priv->entry, > entry_key_pressed_event_cb, bar); > + g_object_weak_unref (G_OBJECT (bar->priv->entry), entry_vanished, bar); > > I think technically, this belongs in dispose, not in finalize Done. (In reply to comment #28) > Review of attachment 245733 [details] [review]: > > minor cosmetic issue here as well. > > ::: gtk/gtksearchbar.c > @@ +478,3 @@ > + { > + bar->priv->entry = GTK_WIDGET (entry); > + g_object_weak_ref (G_OBJECT (bar->priv->entry), entry_vanished, bar); > > you can get away with g_object_add_weak_pointer() instead of a weak_ref() + > function. Done.
Created attachment 245746 [details] [review] Add GtkSearchBar widget This widget is a toolbar that will popup automatically when searches should be started, and dismissed when they are finished. https://bugzilla.gnome.org/show_bug.cgi?id=700787 v2: - Make it possible to set search_entry and container after creation - Make gtk_container_add() work as expected (so it works in GtkBuilder) v3: - Remove unneeded forward declaration by moving get/set props - Fold one-liner "grab focus" helper into calling func - Add search-mode property - Better documentation with examples - Add toggle button in example, and center the search widget - Remove duplicate GDK_Key_Up - Fix comment style - Remove container property (we use the container class' ->add() instead) v4: - Descend from a GtkBin as recommended - Use widget templates to create the composite widget v5: - Add get/set accessors for the entry property v6: - Add show-close-button property (as used in epiphany's search toolbar) v7: - Add example for more advanced search widget - Add accessors for show-close-button and search-entry properties v8: - Fix review comments v9: - Remove entry property - Add other widget in example code - Rename _set_entry() to _connect_entry() and fix refcounting of the search widget - Special case GtkEntry as the only child v10: - 2 liner in ui file to fix sizing problems v11: - Fix review comments
Attachment 245746 [details] pushed as dbbea8b - Add GtkSearchBar widget