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 710640 - Follow the standard search design pattern
Follow the standard search design pattern
Status: RESOLVED FIXED
Product: gnome-software
Classification: Applications
Component: General
3.10.x
Other Linux
: Normal normal
: ---
Assigned To: GNOME Software maintainer(s)
GNOME Software maintainer(s)
: 719870 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-10-22 11:58 UTC by Allan Day
Modified: 2016-05-31 16:36 UTC
See Also:
GNOME target: ---
GNOME version: Unversioned Enhancement


Attachments
[PATCH] Replace custom search bar with GtkSearchBar (12.43 KB, patch)
2014-05-15 21:55 UTC, Elad Alfassa
none Details | Review
Replace custom search bar with GtkSearchBar (11.77 KB, patch)
2014-06-27 15:42 UTC, Elad Alfassa
needs-work Details | Review

Description Allan Day 2013-10-22 11:58:31 UTC
We tried to do something a bit different for search with Software, but the standard design pattern we're using elsewhere has some advantages and will be more consistent.

This means:

 * Show a search button in the header bar (to the right)
 * Show the search bar when the user types, presses Ctrl+F or presses the search button
 * Display search results as the user types

The one variation I think we want here is that we should show the search bar when Software is launched. Navigating to a different page would then hide it.
Comment 1 André Klapper 2013-10-22 12:58:55 UTC
On which wikipage is the standard design pattern defined, for future reference?
Comment 2 Allan Day 2013-10-24 11:26:42 UTC
(In reply to comment #1)
> On which wikipage is the standard design pattern defined, for future reference?

There's some material here - https://wiki.gnome.org/Design/HIG/Search

We also have a search bar widget in GTK+ now and the other apps serve as a good reference point.
Comment 3 Richard Hughes 2014-04-07 15:36:52 UTC
*** Bug 719870 has been marked as a duplicate of this bug. ***
Comment 4 Elad Alfassa 2014-05-15 21:55:54 UTC
Created attachment 276630 [details] [review]
[PATCH] Replace custom search bar with GtkSearchBar

Patch attached.

This simplifies some code paths, removes a lot of lines from the .ui file, and makes the searching experience much nicer.
Comment 5 Elad Alfassa 2014-06-12 20:55:13 UTC
Ugh. This patch was sitting here so long it can no longer be applied to master, and I don't know any way to rebase it without writing it from scratch :(

Any tips on how to rebase this patch without having to write it from scratch (which is something I don't have the motivation to do)?
Comment 6 Kalev Lember 2014-06-13 14:19:58 UTC
'patch -F3' might help.
Comment 7 Elad Alfassa 2014-06-13 14:59:40 UTC
That worked.

Still trying to figure out how to make the search bar visible on startup AND keep type-to-search working (since my patch uses gtk_search_bar_handle_event() to make the search bar open when you start typing, if you start typing when it's already open AND the entry is not focused it does nothing, which is an expected behavior). Do we want the users to click the bar to start searching? I don't think the entry widget should be focused by default, and if it's not focused the input handler will remain as complex as it is before this patch.
Comment 8 Elad Alfassa 2014-06-27 15:42:30 UTC
Created attachment 279420 [details] [review]
Replace custom search bar with GtkSearchBar

Updated, search bar is now visible on startup.
Comment 9 Kalev Lember 2014-07-03 00:56:36 UTC
Review of attachment 279420 [details] [review]:

Thanks for the patch update. My main concern here is that it looks visually less pleasing than the current search.

"Before" screenshot: http://kalev.fedorapeople.org/gnome-software-search1.png
"After" screenshot: http://kalev.fedorapeople.org/gnome-software-search2.png

 1) The upper end of the scrollbar in the overview page floats weirdly in the air

 2) Now that the darker background behind the scrollbar (from the toolbar) is gone, the padding before the "Featured" headline and the featured tile looks off

 3) Switching between pages triggers the scrollbar animation, which looks out of place

 4) Switching between the "All" and "Installed" pages replaces the search icon with a tick mark icon in the exact same place, which looks odd

To address the issues above, my recommendation here would be to (1) (2) add back the GtkToolBar as a child of GtkSearchEntry, until Allan or someone else comes up with a new mockup how it should look; (3) remove the animation when switching pages; (4) drop the headerbar search icon for now.

I'm personally planning on adding search filtering to the Installed page (or if you want to do that, that would be great too!). Once we have search there too, then the search icon in the headerbar starts making sense, but not now when search is only available on the overview page and the search bar is always visible.

::: src/gs-shell-overview.c
@@ +256,3 @@
+		widget = GTK_WIDGET (gtk_builder_get_object (priv->builder, "button_search"));
+		gtk_widget_show (widget);
+		gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON (widget), TRUE);

Missing space before opening bracket.

@@ +259,3 @@
 		widget = GTK_WIDGET (gtk_builder_get_object (priv->builder, "entry_search"));
 		gtk_entry_set_text (GTK_ENTRY (widget), "");
+		gtk_widget_grab_focus(widget);

Same here, missing space before opening bracket.

::: src/gs-shell.c
@@ +153,3 @@
+	/* Only hide search bar if target mode is not search or details, so entry text is not deleted */
+	if (mode != GS_SHELL_MODE_SEARCH && mode != GS_SHELL_MODE_DETAILS)
+		gtk_search_bar_set_search_mode ( GTK_SEARCH_BAR (gtk_builder_get_object (priv->builder, "search_bar")), FALSE);

This triggers the search bar unreveal animation when switching between gnome-software pages. I don't think this is wanted here; can you just hide the search_bar instead of animating it?

@@ +395,3 @@
 		handled = GDK_EVENT_STOP;
+		bar = GTK_SEARCH_BAR (gtk_builder_get_object (shell->priv->builder, "search_bar"));
+		gtk_search_bar_set_search_mode(bar, TRUE);

Missing space before opening bracket.

@@ +425,3 @@
+	g_object_set (entry, "editable", FALSE, NULL);
+	gtk_widget_grab_focus (GTK_WIDGET (entry));
+	g_object_set (entry, "editable", TRUE, NULL);

Can you explain why this is needed? Why are you clearing and setting the "editable" property here?
Comment 10 Elad Alfassa 2014-07-03 10:12:06 UTC
(In reply to comment #9)
> Review of attachment 279420 [details] [review]:
> 
> Thanks for the patch update. My main concern here is that it looks visually
> less pleasing than the current search.
> 
> "Before" screenshot: http://kalev.fedorapeople.org/gnome-software-search1.png
> "After" screenshot: http://kalev.fedorapeople.org/gnome-software-search2.png
> 
>  1) The upper end of the scrollbar in the overview page floats weirdly in the
> air
> 
>  2) Now that the darker background behind the scrollbar (from the toolbar) is
> gone, the padding before the "Featured" headline and the featured tile looks
> off
> 

I think this is a theme bug, GtkSearchBar used to have a background before the major theme changes. Better ask the designers.

>  3) Switching between pages triggers the scrollbar animation, which looks out
> of place
> 
>  4) Switching between the "All" and "Installed" pages replaces the search icon
> with a tick mark icon in the exact same place, which looks odd

We can have a search icon which is always visible but is disabled when you are in a page where search is not supported.

> 
> To address the issues above, my recommendation here would be to (1) (2) add
> back the GtkToolBar as a child of GtkSearchEntry, until Allan or someone else
> comes up with a new mockup how it should look; (3) remove the animation when
> switching pages; (4) drop the headerbar search icon for now.
>
Without a GtkSearchBar and the search button the situation will be exactly the same as it is now - apart from those two UI elements, nothing changed.
 
> I'm personally planning on adding search filtering to the Installed page (or if
> you want to do that, that would be great too!). Once we have search there too,
> then the search icon in the headerbar starts making sense, but not now when
> search is only available on the overview page and the search bar is always
> visible.

Agreed.

> 
> @@ +425,3 @@
> +    g_object_set (entry, "editable", FALSE, NULL);
> +    gtk_widget_grab_focus (GTK_WIDGET (entry));
> +    g_object_set (entry, "editable", TRUE, NULL);
> 
> Can you explain why this is needed? Why are you clearing and setting the
> "editable" property here?

Yes. When you grab the focus of an entry, it selects the text, unless you use this "editable" hack. Focusing the entry allows you, for example, to select its text by doing ctrl+a or shift+right/left arrows. Focusing the entry was crucial when I used gtk_search_bar_handle_event(), but since I'm not using that any more those 3 lines are merely a minor enhancement that can be removed. I'm also unsure if we should/can keep them if we have the search bar visible on startup, as per bug 729483 comment 14

So, I would happily iterate on this patch, but to continue I need to know exactly what is left to change if we don't use GtkSearchBar and the search button, because that's basically the entirety of the patch :)
Comment 11 Kalev Lember 2014-07-03 11:52:28 UTC
(In reply to comment #10)
> I think this is a theme bug, GtkSearchBar used to have a background before the
> major theme changes. Better ask the designers.

Ah, that makes sense, thanks. If the theming reappears, is it going to draw a border too or is it something we have to handle ourselves?


> We can have a search icon which is always visible but is disabled when you are
> in a page where search is not supported.

Sure, I suppose that's fine too as a temporary measure. I don't really want to do a 3.13.4 release with an insensitive search button though, so if we don't manage to get search filtering in other pages in time for the 3.13.4 release, I can kill the search button right before the release.


> > @@ +425,3 @@
> > +    g_object_set (entry, "editable", FALSE, NULL);
> > +    gtk_widget_grab_focus (GTK_WIDGET (entry));
> > +    g_object_set (entry, "editable", TRUE, NULL);
> > 
> > Can you explain why this is needed? Why are you clearing and setting the
> > "editable" property here?
> 
> Yes. When you grab the focus of an entry, it selects the text, unless you use
> this "editable" hack. Focusing the entry allows you, for example, to select its
> text by doing ctrl+a or shift+right/left arrows. Focusing the entry was crucial
> when I used gtk_search_bar_handle_event(), but since I'm not using that any
> more those 3 lines are merely a minor enhancement that can be removed. I'm also
> unsure if we should/can keep them if we have the search bar visible on startup,
> as per bug 729483 comment 14

Ahh, I see. If it's not needed for the current patch, it's probably best to remove it, yes.


> So, I would happily iterate on this patch, but to continue I need to know
> exactly what is left to change if we don't use GtkSearchBar and the search
> button, because that's basically the entirety of the patch :)

No, please do keep GtkSearchBar :)

What I wanted to suggest is that you don't have to pack a GtkSearchEntry directly in a GtkSearchBar. It's possible to add other container widgets in between to make it look nicer. See e.g. the example in https://git.gnome.org/browse/gtk+/tree/examples/search-bar.c that packs a search entry in a box which is in turn packed in a search bar. And then uses gtk_search_bar_connect_entry() to tell the search bar which one of the children is the search entry widget.


Thanks for working on this by the way, good stuff!
Comment 12 Elad Alfassa 2014-08-17 21:34:20 UTC
Your comment was lost in the bugmail and I only saw it now.

Since the UI freeze starts today, I guess I won't have time to finish this for 3.14 :(

I'll try to work on this sometime this week, so we'll at least have it for 3.16.

Anyway, one important point I forgot to mention that really bothers me every time I search is that right now trying to search anything with space in it is impossible without clicking on the search entry. The workaround for this would be to grab the focus when typing starts, so I do think we should keep the gtk_widget_grab_focus thing. Since we are doing type-to-find anyway it won't seem as if we are yanking the focus from whatever the user was focused on, because the entire view changes.

What benefit does packing the search entry in another widget inside the searchbar give us?

Sorry for forgetting about this bug.
Comment 13 Richard Hughes 2014-10-13 14:52:05 UTC
What's the status of this bugs guys? Thanks.
Comment 14 Elad Alfassa 2014-10-13 14:57:13 UTC
Hey,
I was going to work on this targeting 3.16, but I haven't had enough time for FOSS related stuff recently and I don't know when I'll have time to work on it :(
Comment 15 Matthias Clasen 2015-08-08 16:46:06 UTC
Allan, Jakub, whats the current design thinking on this: keep the search entry permanently visible, or hide it initially ?
Comment 16 Jakub Steiner 2015-08-31 09:46:56 UTC
The search pattern is well established and described in the HIG now, I'd go for a regular (hidden by default) behavior.
Comment 17 Rafal Luzynski 2015-09-01 01:07:06 UTC
Actually I'd ask what is the future of this patch. Elad, do you have time to work on it now? Is somebody else going to continue this task? I think we are too close to the end to just abandon it.
Comment 18 Elad Alfassa 2015-09-01 19:08:14 UTC
I can't promise I'll have time to work on this. At least not in the next two weeks.

If anyone else wants to continue this work, feel free to do so. If nobody does, I might do it myself in few weeks - but I'm not promising anything so don't count on me doing that.

Sorry.
Comment 19 Richard Hughes 2016-05-31 16:36:10 UTC
commit 3299b31df7d95bd645af4abc6382e246c6d3fad7
Author: Richard Hughes <richard@hughsie.com>
Date:   Tue May 31 16:45:51 2016 +0100

    Do not show the search bar by default
    
    Thes user has to start typing or click the search button to show it.
    
    Fixes: https://bugzilla.gnome.org/show_bug.cgi?id=710640

:100644 100644 857ded6... 4207bd6... M  src/gnome-software.ui
:100644 100644 9025f39... 435514a... M  src/gs-shell-overview.c
:100644 100644 62ce9b8... aa470bc... M  src/gs-shell-overview.ui
:100644 100644 09b1b07... 382d11f... M  src/gs-shell.c