GNOME Bugzilla – Bug 636341
bad handling when typing multiple searches in the search box in overview
Last modified: 2011-05-27 20:29:20 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.)
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.
this is still valid in latest shell, btw. just re-tested.
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.
yup, behaviour's never changed for me.
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.
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).
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.
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.
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 ;)
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.
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)
Created attachment 185240 [details] [review] Fix comments and git commit message
*** Bug 646873 has been marked as a duplicate of this bug. ***
Created attachment 185488 [details] [review] Comments improved
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.
Review of attachment 185488 [details] [review]: Looks good to me
Attachment 185488 [details] pushed as r35c85d9fac - Fix search handling when typing multiple searches in the search box
*** Bug 648809 has been marked as a duplicate of this bug. ***
*** Bug 651281 has been marked as a duplicate of this bug. ***