GNOME Bugzilla – Bug 652809
Add "search" entry
Last modified: 2012-06-11 18:04:20 UTC
People using the secondary icon to create search entries usually get them wrong. It would be very useful to have directly in GTK+.
This needs UI reviewing, and implementation (which should be straight forward enough), because there's apparently consistency problems as to which side icon should go in, etc. See bug 659284.
*** Bug 659284 has been marked as a duplicate of this bug. ***
Consistent search entries would be awesome. The only tricky thing I can see here is when the search icon is used to provide a menu, as in Banshee's case. It might be useful to think about alternative design patterns for that.
(In reply to comment #3) ... > The only tricky thing I can see here is when the search icon is used to provide > a menu, as in Banshee's case. It might be useful to think about alternative > design patterns for that. I just noticed - Evolution uses the same pattern as Banshee.
Any thoughts on how to handle the difference between search-as-you-type (like system settings) and search on activation (evolution, web browsers, anything remote)? I currently distinguish between them in rhythmbox by placing a button labeled 'search' next to search entries that require the user to hit enter or press the button rather than just stop typing. I'm not particularly delighted with that solution, but I think making the difference visible is worthwhile.
(In reply to comment #5) I'd be tempted to avoid having a search button outside of the search box itself. The search icon that is located inside the field could be used as a search button, of course.
Created attachment 215422 [details] [review] gtk: Add GtkSearchEntry Add a search entry widget with the recommended behaviour implemented. As used in gnome-control-center, Totem, gnome-documents and many others.
Review of attachment 215422 [details] [review]: Looks mostly good; I'm not 100% sure we still want to allow clients to be able to set the "secondary-icon-*" properties to something else, but I can't think of an easy way to avoid that. Some comments below ::: gtk/gtksearchentry.c @@ +1,2 @@ +/* GTK - The GIMP Toolkit + * Copyright (C) 2007 Red Hat, Inc. 2012 (here and below) @@ +32,3 @@ +#include "gtkadjustment.h" +#include "gtkintl.h" +#include "gtktooltip.h" You could probably remove these includes here. @@ +67,3 @@ + str = gtk_entry_get_text (entry); + + if (!g_strcmp0 (str, "")) I think you should special case a NULL value returned by gtk_entry_get_text() and treat it as empty text; I also usually like more an explicit (g_strcmp0 (foo, bar) == 0), since it can be easy to miss the '!' when refactoring code. @@ +69,3 @@ + if (!g_strcmp0 (str, "")) + { + g_object_set (G_OBJECT (entry), No need to cast to G_OBJECT here and below, as g_object_set() takes a gpointer. @@ +78,3 @@ + { + g_object_set (G_OBJECT (entry), + "secondary-icon-name", "edit-clear-symbolic", Wrong indentation
(In reply to comment #8) > Review of attachment 215422 [details] [review]: > > Looks mostly good; I'm not 100% sure we still want to allow clients to be able > to set the "secondary-icon-*" properties to something else, but I can't think > of an easy way to avoid that. Done by overriding the properties and popping out a warning if they try to override them. > Some comments below > > ::: gtk/gtksearchentry.c > @@ +1,2 @@ > +/* GTK - The GIMP Toolkit > + * Copyright (C) 2007 Red Hat, Inc. > > 2012 (here and below) Fixed. > @@ +32,3 @@ > +#include "gtkadjustment.h" > +#include "gtkintl.h" > +#include "gtktooltip.h" > > You could probably remove these includes here. Removed. > @@ +67,3 @@ > + str = gtk_entry_get_text (entry); > + > + if (!g_strcmp0 (str, "")) > > I think you should special case a NULL value returned by gtk_entry_get_text() > and treat it as empty text; I also usually like more an explicit (g_strcmp0 > (foo, bar) == 0), since it can be easy to miss the '!' when refactoring code. Fixed. > @@ +69,3 @@ > + if (!g_strcmp0 (str, "")) > + { > + g_object_set (G_OBJECT (entry), > > No need to cast to G_OBJECT here and below, as g_object_set() takes a gpointer. Done. > @@ +78,3 @@ > + { > + g_object_set (G_OBJECT (entry), > + "secondary-icon-name", "edit-clear-symbolic", > > Wrong indentation Fixed.
Created attachment 215442 [details] [review] gtk: Add GtkSearchEntry Add a search entry widget with the recommended behaviour implemented. As used in gnome-control-center, Totem, gnome-documents and many others.
Review of attachment 215442 [details] [review]: Looks almost ready; the GTK_IS_SEARCH_ENTRY special case in GtkEntry is a bit eekish but I can't think of any other better approach. ::: gtk/gtkentry.c @@ +8289,3 @@ g_return_if_fail (GTK_IS_ENTRY (entry)); g_return_if_fail (IS_VALID_ICON_POSITION (icon_pos)); + g_return_if_fail (verify_immutable_search_entry (entry, icon_pos)); Not 100% sure about adding this here and in the tooltip methods; at least for the tooltip, it might make sense for the application to set something custom there (since the subclass doesn't set any tooltip on its own)...What do you think? ::: gtk/gtksearchentry.c @@ +46,3 @@ + * been tailored for use as a search entry, with appropriate + * icons and behaviour. + */ The documentation should probably mention that the secondary-icon properties are "locked" for the subclass, and that setting them will be treated as a programmer error. @@ +76,3 @@ + GParamSpec *pspec) +{ + G_OBJECT_CLASS (gtk_search_entry_parent_class)->get_property (object, prop_id, value, pspec); I don't remember ever seeing a get_property implementation chaining up, but if it works...
(In reply to comment #11) > Review of attachment 215442 [details] [review]: > > Looks almost ready; the GTK_IS_SEARCH_ENTRY special case in GtkEntry is a bit > eekish but I can't think of any other better approach. Pretty ugly indeed. But now you know why it took me that long to get the patch cleaned up :) > ::: gtk/gtkentry.c > @@ +8289,3 @@ > g_return_if_fail (GTK_IS_ENTRY (entry)); > g_return_if_fail (IS_VALID_ICON_POSITION (icon_pos)); > + g_return_if_fail (verify_immutable_search_entry (entry, icon_pos)); > > Not 100% sure about adding this here and in the tooltip methods; at least for > the tooltip, it might make sense for the application to set something custom > there (since the subclass doesn't set any tooltip on its own)...What do you > think? I don't think that the application should set a tooltip on an icon it doesn't control. > ::: gtk/gtksearchentry.c > @@ +46,3 @@ > + * been tailored for use as a search entry, with appropriate > + * icons and behaviour. > + */ > > The documentation should probably mention that the secondary-icon properties > are "locked" for the subclass, and that setting them will be treated as a > programmer error. Will do. > @@ +76,3 @@ > + GParamSpec *pspec) > +{ > + G_OBJECT_CLASS (gtk_search_entry_parent_class)->get_property (object, > prop_id, value, pspec); > > I don't remember ever seeing a get_property implementation chaining up, but if > it works... You need a get_property implementation for the class_override to work (otherwise it complains of a missing get_property, and doesn't do anything).
Review of attachment 215442 [details] [review]: ::: gtk/gtkentry.c @@ +7795,3 @@ + const gchar *icon_name) +{ + g_return_if_fail (verify_immutable_search_entry (entry, icon_pos)); I don't like this verify business much at all, tbh. I'd rather just document that the secondary icon is used for internal purposes in search entries - we do the same for the capslock warning, and people seem to deal just fine. ::: gtk/gtksearchentry.c @@ +46,3 @@ + * been tailored for use as a search entry, with appropriate + * icons and behaviour. + */ More importantly, the docs should actually explain how one uses this thing. What is handled for me, what isn't ? Am I supposed to connect to "activate" to start a search ? What if the user cancels a search ? Embedding an example would be nice.
But you will still let me h
The gtk-demo search entry is kinda broken with this patch - if you enter a word, there is a menu on the clear icon that is somewhat unexpected. Also, it spits out warnings.
(In reply to comment #15) > The gtk-demo search entry is kinda broken with this patch - if you enter a > word, there is a menu on the clear icon that is somewhat unexpected. Also, it > spits out warnings. It's very broken. I broke it on purpose to check that my warnings were effective, and forgot to revert the changes.
(In reply to comment #13) > Review of attachment 215442 [details] [review]: > > ::: gtk/gtkentry.c > @@ +7795,3 @@ > + const gchar *icon_name) > +{ > + g_return_if_fail (verify_immutable_search_entry (entry, icon_pos)); > > I don't like this verify business much at all, tbh. > I'd rather just document that the secondary icon is used for internal purposes > in search entries - we do the same for the capslock warning, and people seem to > deal just fine. Removed. > ::: gtk/gtksearchentry.c > @@ +46,3 @@ > + * been tailored for use as a search entry, with appropriate > + * icons and behaviour. > + */ > > More importantly, the docs should actually explain how one uses this thing. > What is handled for me, what isn't ? Added. > Am I supposed to connect to "activate" to > start a search ? GtkEntry::activate says "Applications should not connect to it". I think we should fix that before trying to tell app developers to connect to it. > What if the user cancels a search ? I don't understand here. > Embedding an example would > be nice.
Created attachment 216117 [details] [review] gtk: Add GtkSearchEntry Add a search entry widget with the recommended behaviour implemented. As used in gnome-control-center, Totem, gnome-documents and many others.
Created attachment 216118 [details] [review] gtk: Add GtkSearchEntry Add a search entry widget with the recommended behaviour implemented. As used in gnome-control-center, Totem, gnome-documents and many others.
Review of attachment 216118 [details] [review]: Looks fine now. ::: gtk/gtksearchentry.h @@ +64,3 @@ + +GType gtk_search_entry_get_type (void) G_GNUC_CONST; +GtkWidget* gtk_search_entry_new (void); Please add GDK_AVAILABLE_IN_3_6 annotations to the new API here.
Moved the gtksearchentry.h in the right location in the Makefile.am (oops), and added the "available in 3.6" macros. Attachment 216118 [details] pushed as d704f2b - gtk: Add GtkSearchEntry