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 636341 - bad handling when typing multiple searches in the search box in overview
bad handling when typing multiple searches in the search box in overview
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
2.91.x
Other Linux
: Normal normal
: ---
Assigned To: Florian Müllner
gnome-shell-maint
: 646873 648809 651281 (view as bug list)
Depends on:
Blocks: GnomeShell301
 
 
Reported: 2010-12-03 03:13 UTC by Adam Williamson
Modified: 2011-05-27 20:29 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed bug fix (675 bytes, patch)
2011-03-30 06:50 UTC, Nohemi
needs-work Details | Review
Correct search bar display (3.96 KB, patch)
2011-04-05 00:18 UTC, Nohemi
needs-work Details | Review
Improve search handling based on recommendations (842 bytes, patch)
2011-04-05 21:39 UTC, Nohemi
reviewed Details | Review
Fix comments and git commit message (1.25 KB, patch)
2011-04-05 23:00 UTC, Nohemi
none Details | Review
Comments improved (1.34 KB, patch)
2011-04-07 23:19 UTC, Nohemi
committed Details | Review

Description Adam Williamson 2010-12-03 03:13:19 UTC
Go to the overview

Type in some search term (the search box is already selected by default, good)

Decide you want to search for something else; hold down shift-home, and start typing something else, for example, 'monkeys'

Note that the resulting search is in fact for 'onkeys'

What happens is that after doing shift-home, the next character you type is prepended to the 'Search your computer' guide text, so it becomes (in our example) 'mSearch your computer'. This is then replaced by the rest of the characters you type - 'mSearch your computer' disappears and is replaced by 'o', 'on', 'onk', 'onke' and so on, until you hit 'onkeys'.

(the whole experience is also quite slow and doesn't feel nice; perhaps the 'search as you type' could be slightly delayed so it doesn't feel creaky when you're quickly typing a whole word.)
Comment 1 António Lima 2011-01-27 22:37:10 UTC
I also find the search functionality in the overview mode very slow. In my older computed (a AMD athlon 2x 2GZ with integrated nvidia card and proprietary drivers) it's very frustrating to use, although the overall gnome-shell experience is good (the animation to/from overview is also a bit slow but I'm using a 22'' screnn).

With a more recent laptop the animations are quite fluid but the search is slow as well. Especially when using backspace to delete what I have typed in the search box.
Comment 2 Adam Williamson 2011-01-28 00:06:32 UTC
this is still valid in latest shell, btw. just re-tested.
Comment 3 Stéphane Maniaci 2011-03-28 18:09:22 UTC
This is still valid in latest shell from Fedora 15. 

1. Type a search ;
2. Decide something else, so use Ctrl+A to select all and type again ;
3. First character is eaten.
Comment 4 Adam Williamson 2011-03-28 19:38:48 UTC
yup, behaviour's never changed for me.
Comment 5 Nohemi 2011-03-30 06:50:00 UTC
Created attachment 184660 [details] [review]
Proposed bug fix

This simple fix solves the problem of the missing letters. To restore the hint text you must click away from the Search Box.
Comment 6 Florian Müllner 2011-03-30 08:02:25 UTC
Review of attachment 184660 [details] [review]:

The patch has undesired side effects, most importantly:
- enter a search
- leave the overview (hitting super, clicking activities, using the hot corner)
- enter the overview again

The previously entered search term is still there.

::: js/ui/viewSelector.js
@@ +168,1 @@
     },

That's the same as removing the function all together (so that the implementation in the parent prototype is called directly).
Comment 7 Nohemi 2011-03-31 01:28:14 UTC
After reviewing the code for hint_text selection I think I may have noted a bug in the logic behind its execution. What I think is happening (please correct me if I'm wrong) is the following:
When pressing *combo of keys to make empty* and then another key the entry becomes empty for a moment in the program thereby triggering set_hint_text method and resetting the search. At the same time you have already pressed the key into the search, so it gets locked into the search bar then reset raising the bug.
Now, if my thoughts are correct I'm not exactly sure where to start with a fix since this is a logic error, if not then I'd love to know what is really going on behind the scenes.
Comment 8 Nohemi 2011-04-05 00:18:28 UTC
Created attachment 185157 [details] [review]
Correct search bar display

Selecting all text and re-entering, as described above, is now working. In my implementation, however, I do require the user to click away from an empty search to reset the hint. Navigating away and back from searchTab also resets.
Comment 9 Florian Müllner 2011-04-05 20:18:26 UTC
Review of attachment 185157 [details] [review]:

(In reply to comment #8)
> In my implementation, however, I do require the user to click away from an empty
> search to reset the hint. Navigating away and back from searchTab also resets.

This is a slightly changed behavior, but not a big deal in my opinion in case of an empty search - it basically means that hitting backspace puts you back where you start when activating the entry with the mouse.

What I don't like is the behavior when switching tabs with a search term entered - it's just weird:
 1. enter a search term
 2. click "Windows" -> tab is switched, but focus remains in the entry
 3. click "Applications" -> tab is switched, entry is reset to inactive

With regard to the code, I'm not too fond of boolean parameters which change behavior of an action if it's possible to avoid them - something like doFoo(false) is not at all obvious, and requires searching for the function definition to see what the parameter is about (on the other hand, something like setFooEnabled(true) is perfectly clear).

::: js/ui/viewSelector.js
@@ +64,3 @@
                            onComplete: Lang.bind(this,
                                function() {
+                                   this.page.hide(true);

this.page is a ClutterActor, so the parameter is wrong

@@ +155,3 @@
         this._entry.connect('secondary-icon-clicked', Lang.bind(this,
             function() {
+                this._reset(true);

Woops - that signal connection should not exist in the first place :(

Filed separately as bug 646855.

@@ -227,3 @@
                 this._iconClickedId = this._entry.connect('secondary-icon-clicked',
                     Lang.bind(this, function() {
-                        this.reset();

Woops again. Can you do a separate patch for the reset() -> _reset() fix?

@@ +387,2 @@
     _addTab: function(tab) {
+        tab.page.hide(true);

Again, tab.page is a ClutterActor, so no parameter.

@@ +415,3 @@
             if (this._searchTab.visible) {
+		// Need to keep focus on searchTab while resetting it
+                this._searchTab.hide(false);

So - it results that this is the only place where the search tab's hide function is actually called (all other places where you changed hide() to hide(true) are calls to clutter_actor_hide()) - you always end up calling this._reset(false) in SearchTab.hide(), so the additional parameter is not needed at all.

Now, as mentioned in the summary, the behavior when switching tabs is still odd with that change. So rather than never resetting the focus, I think it's better to do something like

this._reset(this._text.text != '');

That way, we still reset the entry fully when there is an active search term, but still catch the case where clutter sends a 'text-changed' signal for '' when killing a selected search term (it also applies to the case where the search term is cleared with backspace, but as mentioned above, I don't think this is necessarily a bad thing).

Now, when looking at _reset(), this is what it does when passing false for resetFocus:
 1. set the entry's text to '' => not necessary if the text already is ''
 2. reset the selection => not necessary either if the text is ''
 3. force cursor visibility => not necessary if the entry has focus

So I'd suggest getting rid of the resetFocus parameter to _reset() as well, and write

  if (this._text.text != '')
      this._reset();

in SearchTab.hide (and as the condition is not particularly obvious, add a comment explaining what this is fixing).

@@ +433,3 @@
                                    onComplete: Lang.bind(this,
                                        function() {
+                                           Main.overview.workspaces.actor.hide(true);

Not a tab either ;)
Comment 10 Nohemi 2011-04-05 21:39:07 UTC
Created attachment 185234 [details] [review]
Improve search handling based on recommendations

I have added the comment and have updated to your recommendations. Hope all is looking good now.
Comment 11 Florian Müllner 2011-04-05 22:32:12 UTC
Review of attachment 185234 [details] [review]:

The commit message needs to be more verbose, see 
http://lists.cairographics.org/archives/cairo/2008-September/015092.html.

In general, it should have the following structure:

subject: what changes

A short explanation why the change was made.

https://bugzilla.gnome.org/show_bug.cgi?id=123456

(You may also want to check out http://blog.fishsoup.net/2008/11/16/git-bz-bugzilla-subcommand-for-git because it's awesome)

::: js/ui/viewSelector.js
@@ +168,3 @@
 
+	// Catch the case where clutter sends a 'text-changed' signal for ''
+	// when attempting to replace a selected search term

Not exactly true - this is the case which needs fixing, but it also affects other cases (where we don't mind the slightly different behavior).

Maybe something like:

// Leave the entry focused when it doesn't have any text;
// this works around a bug where events are lost when replacing a selected search term
// (https://bugzilla.gnome.org/show_bug.cgi?id=636341)

(might be worth mentioning the 'text-changed' signal, and how it is send twice in the case of the buggy behavior; not sure ... getting late here)
Comment 12 Nohemi 2011-04-05 23:00:44 UTC
Created attachment 185240 [details] [review]
Fix comments and git commit message
Comment 13 Florian Müllner 2011-04-06 06:17:02 UTC
*** Bug 646873 has been marked as a duplicate of this bug. ***
Comment 14 Nohemi 2011-04-07 23:19:17 UTC
Created attachment 185488 [details] [review]
Comments improved
Comment 15 Florian Müllner 2011-04-12 17:45:30 UTC
Review of attachment 185488 [details] [review]:

Looks good (except for the "events are lost" part in the commit message - my suggestion if I recall correctly, sorry about that; the replacing character is not actually lost, but prepended to the hint text).
No need to attach another version, the message can be fixed before pushing.
Comment 16 Owen Taylor 2011-04-15 17:35:24 UTC
Review of attachment 185488 [details] [review]:

Looks good to me
Comment 17 Florian Müllner 2011-04-15 17:48:15 UTC
Attachment 185488 [details] pushed as r35c85d9fac - Fix search handling when typing multiple searches in the search box
Comment 18 Rui Matos 2011-04-27 21:33:04 UTC
*** Bug 648809 has been marked as a duplicate of this bug. ***
Comment 19 Shaun McCance 2011-05-27 20:29:20 UTC
*** Bug 651281 has been marked as a duplicate of this bug. ***