GNOME Bugzilla – Bug 739036
Renovate the search UX
Last modified: 2014-12-18 07:29:49 UTC
Created attachment 289172 [details] search-entry-logic.py I told you several times that we need to rework the search entries a bit, I enumerated some items and the general idea but only on chat, I like to expand the idea in BZ so I can have some feedback. The main problem I can note is that the auto-completion of history (and favourites in the near future I think) doesn't play well together with the search results, it's like we two different UIs, one for completion and one for search results. I started to write some sort of Pythonic pseudocode (attached) to tell you how I think how search entry needs to behave, it has a high level of detail, I wasn't expecting that when I started to write it... perhaps it would have been better to write a patch instead but the mistake is already done!
Created attachment 289173 [details] search-entry-logic.py Reuploading the pseudocode as a patch so the review system works.
I am finding it hard to visualize what this would do. I would prefer visual mockups or a real patch. And pinging Andreas would be nice.
Created attachment 289631 [details] [review] Replace GtkEntryCompletion with SearchPopup This moves the completion functionality to the search popover instead of having a GtkEntryCompletion.
Created attachment 289632 [details] [review] Add recent icon to place store recent places
Created attachment 289634 [details] [review] Replace GtkEntryCompletion with SearchPopup This moves the completion functionality to the search popover instead of having a GtkEntryCompletion.
Created attachment 289635 [details] [review] Add recent icon to place store recent places
Created attachment 289636 [details] Screencast of popover as completion
Created attachment 289718 [details] [review] Replace GtkEntryCompletion with SearchPopup This moves the completion functionality to the search popover instead of having a GtkEntryCompletion.
Created attachment 289719 [details] [review] Replace GtkEntryCompletion with SearchPopup This moves the completion functionality to the search popover instead of having a GtkEntryCompletion.
Created attachment 289722 [details] Search hint How about a search hint like this? Not sure at all.
Created attachment 289865 [details] [review] Replace GtkEntryCompletion with SearchPopup This moves the completion functionality to the search popover instead of having a GtkEntryCompletion.
Created attachment 289866 [details] [review] Add 'press enter' hint to search completion
Created attachment 289867 [details] [review] Add recent icon to place store recent places
Created attachment 289868 [details] [review] Replace GtkEntryCompletion with SearchPopup This moves the completion functionality to the search popover instead of having a GtkEntryCompletion.
Created attachment 289869 [details] [review] Add 'press enter' hint to search completion
Created attachment 289870 [details] [review] Add recent icon to place store recent places
Created attachment 289873 [details] [review] Replace GtkEntryCompletion with SearchPopup This moves the completion functionality to the search popover instead of having a GtkEntryCompletion.
Created attachment 289874 [details] [review] Add 'press enter' hint to search completion
Created attachment 289875 [details] [review] Add recent icon to place store recent places
Created attachment 289927 [details] [review] Replace GtkEntryCompletion with SearchPopup This moves the completion functionality to the search popover instead of having a GtkEntryCompletion. ---- - Makes popover no modal so the place entry focus is clear to the user. - Switch focus between popup and entry using arrow keys.
Created attachment 289928 [details] [review] Add 'press enter' hint to search completion - Hide hint when popup gains focus.
Created attachment 289930 [details] [review] Add recent icon to place store recent places
Review of attachment 289927 [details] [review]: So I dislike this :( 1) I want to be able to keep typing while chaning rows in the treeview, like with the gtkentrycompletion. 2) Doing a pure grab_focus on the entry will select the text in the entry, which is bad. 3) It's messy with all these event grabbing in both classes. I will post a new version to see if you like that better.
Review of attachment 289928 [details] [review]: I don't see why we need to hide the hint, does it bother you so much, I think it is a nice hint on which mode we are in,
Created attachment 289968 [details] [review] Replace GtkEntryCompletion with SearchPopup This moves the completion functionality to the search popover instead of having a GtkEntryCompletion.
Created attachment 289969 [details] [review] Add 'press enter' hint to search completion
Created attachment 289970 [details] [review] Add recent icon to place store recent places
I think this behavior sort of matches how for instance the addressbar in firefox work.
(In reply to comment #24) > I don't see why we need to hide the hint, does it bother you so much, I think > it is a nice hint on which mode we are in, Yes, it's really annoying, I was thinking about put the hint in the bottom, but not sure. I was hiding the hint because when you have focus on the list (or have something selected in the list in this case), ENTER won't search but it will go to the selected place. Maybe when user selects something from the list we can change the hint to "Press enter to go to the selected place".
Review of attachment 289968 [details] [review]: Absolutely awesome! I really like this approach. Just one little nit, suppose that I cancel the searchpopup when there is possible placestore completion (i.e. by pressing ESC), could we show the searchpopup again when user press up/down, so user can access to completion again without modifying the content. This is also done by Firefox.
Review of attachment 289970 [details] [review]: Looks good!
Review of attachment 289969 [details] [review]: Looks good, we just need to discuss if change "Press enter to search" to "Press enter to go to selected place" when there is a selected item in search popup is a good approach (as I said in c#29).
Review of attachment 289968 [details] [review]: ::: src/searchPopup.js @@ +75,3 @@ + this._entry.connect('key-press-event', (function(widget, event) { + if (!this.get_mapped()) + return false; Just curious, what is this for?
Created attachment 291207 [details] [review] PlaceStore: Add recent icon
Created attachment 291208 [details] [review] Replace GtkEntryCompletion with SearchPopup
See branch wip/completion for a version based on the favorites stuff as well.
Created attachment 291341 [details] [review] PlaceListRow: Add type property
Created attachment 291342 [details] [review] Replace GtkEntryCompletion with SearchPopup
Created attachment 291344 [details] [review] Replace GtkEntryCompletion with SearchPopup
Created attachment 291536 [details] [review] Replace GtkEntryCompletion with SearchPopup
So the issue I have with this atm is at if you exit Maps while the searchPopup is visible you get the warnings: "GtkApplicationWindow 0x1e82270 is mapped but visible child Gjs_SearchPopup 0x2064700 is not mapped" This only happens when the searchPopup has its modal property set to false. No idea about what this is about.
Created attachment 291703 [details] [review] Replace GtkEntryCompletion with SearchPopup
Created attachment 291704 [details] [review] Replace GtkEntryCompletion with SearchPopup
Review of attachment 291704 [details] [review]: Nice work! ::: src/placeEntry.js @@ +100,3 @@ + this._filter = new Gtk.TreeModelFilter({ + child_model: Application.placeStore + }); Alignment? @@ +108,2 @@ this.connect('activate', this._onActivate.bind(this)); + this.connect('changed', (function() { Should be cancel the current nominatim search here, if any? A little refactorization in geocodeService to support cancellable would be needed. @@ +165,3 @@ + return false; + + if (!GLib.ascii_strncasecmp(name, key, key.length)) Just thinking if is a good idea to match key to any part of the place name, not just the beginning. ::: src/search-popup.ui @@ +16,2 @@ <child> + <object class="GtkRevealer" id="hintRevealer"> I tried this approach a couple of days ago and I like it, it's so fancy :D . Did you tried to move this hint to the bottom of the popup to avoid motion of the GtkListBox? could it look good? ::: src/searchPopup.js @@ +37,3 @@ + COMPLETION: 2, + RESULT: 3 +}; What about a short comment for each Mode? @@ +98,3 @@ + // This silents warning at Maps exit about this widget being + // visible but not mapped. + this.connect('unmap', function(popover) { popover.hide(); }); Off-topic: I think there is a similar problem with bubbles. @@ +113,3 @@ showResult: function() { + if (this._mode !== Mode.RESULT) + this._mode = Mode.RESULT; Maybe we can skip the "if" here... @@ +130,3 @@ + this._mode = Mode.IDLE; + return; + } Let me understand this part. When you activate a row, the entry will change its content with the place name, that will emit a "changed" event in the entry, so you want to avoid the execution of the completion stuff for that particular case. Am I right? @@ +141,3 @@ }, + Why this empty line? @@ +221,3 @@ + this._list.select_row(row); + return false; + } Let's leave an empty line as you did with key up if :) @@ +226,3 @@ + this._list.select_row(nRow); + } + else "else" must be along the brace. @@ +245,3 @@ + maxChars: this._maxChars, + can_focus: true }); + this._list.add(row); Maybe we should create an _addPlaceRow(place, type, searchString) (or _create... if we want more flexibility) to use it here and in updateResult.
Review of attachment 291704 [details] [review]: Thanks for review Damían! ::: src/placeEntry.js @@ +100,3 @@ + this._filter = new Gtk.TreeModelFilter({ + child_model: Application.placeStore + }); Sure. @@ +108,2 @@ this.connect('activate', this._onActivate.bind(this)); + this.connect('changed', (function() { We could, will add it as a patch on top of this. @@ +165,3 @@ + return false; + + if (!GLib.ascii_strncasecmp(name, key, key.length)) Yeah, I think we want to do better here. But not right now. We need some thought. We should file a bug to have better filter function for this and favourites. Maybe it can be easy, like just splitting on ',' and matching each sub word. If the key length is > n. But dunno, maybe we want to get fancy. I think it is an issue of its own. ::: src/search-popup.ui @@ +16,2 @@ <child> + <object class="GtkRevealer" id="hintRevealer"> :) I think I want to keep it above, since we are focusing on the entry while writing it seems odd to pop something up away from the entry. ::: src/searchPopup.js @@ +37,3 @@ + COMPLETION: 2, + RESULT: 3 +}; Sure. @@ +98,3 @@ + // This silents warning at Maps exit about this widget being + // visible but not mapped. + this.connect('unmap', function(popover) { popover.hide(); }); Yeah, might be gtk issue, I'll dig into it when I have the energy. @@ +113,3 @@ showResult: function() { + if (this._mode !== Mode.RESULT) + this._mode = Mode.RESULT; Yes! @@ +130,3 @@ + this._mode = Mode.IDLE; + return; + } You. Are. Right! @@ +141,3 @@ }, + Gone. @@ +221,3 @@ + this._list.select_row(row); + return false; + } Yep. @@ +226,3 @@ + this._list.select_row(nRow); + } + else Yep. @@ +245,3 @@ + maxChars: this._maxChars, + can_focus: true }); + this._list.add(row); Sure.
Created attachment 291962 [details] [review] placeListRow: Add type property
Created attachment 291963 [details] [review] Replace GtkEntryCompletion with SearchPopup
Created attachment 291964 [details] [review] Add cancellable to geocode search
Created attachment 291972 [details] [review] Replace GtkEntryCompletion with SearchPopup
Review of attachment 291704 [details] [review]: ::: src/search-popup.ui @@ +16,2 @@ <child> + <object class="GtkRevealer" id="hintRevealer"> Yeah, I realized a couple of days later that moving the hint to the bottom is an stupid decision :P
Review of attachment 291962 [details] [review]: Looks ok! (I know you infiltrated the place-list-row.ui changes in "Convert SearchPopup to GtkListBox" commit, you cannot cheat me, my friend :P)
Review of attachment 291972 [details] [review]: There are some behavioural problems here. 1) Let's say that for example, I write "mar" and I have completion of places "Mar del Plata" and "Mar de Ajó". If I clear the text pressing the clear button and press down/up arrow, the completion list keep showing "Mar del Plata" and "Mar de Ajó". We need to behave different in this case, so there are three approach I think: a) Show everything from place store b) Show a part of placestore (MRU?) c) Don't show popup at all Personally, I prefer (a) 2) When popup is hidden and user press down/up, we always must move the cursor list to the beginning. The behaviour is really odd and unpredictable if we don't do that. ::: src/placeEntry.js @@ +113,3 @@ + + /* Filter model based on input text */ + this._filter.refilter(); If we choose solution (1.a) (see review overview), we need to always refilter (even when placeEntry is empty which won't apply any filter at all), so I would move this to the beginning of the callback. @@ +155,3 @@ + return false; + + key = GLib.utf8_normalize (key, -1, GLib.NormalizeMode.ALL); Extra space before the parenthesis (we are not programming in C :D) @@ +159,3 @@ + return false; + + return false; Same here. @@ +168,3 @@ + return false; + + return false; If we choose solution (1.a) we need to return true when the search is empty, so the filter is not applied. Also, I still don't like this way to compare, I would prefer to execute GLib.utf8_normalize and toLowerCase in both name and key variables, and then use JS's "search" method. ::: src/searchPopup.js @@ +202,3 @@ + return true; + } else if (keyval === Gdk.KEY_KP_Up || keyval === Gdk.KEY_Up) { + let row = this._list.get_selected_row(); In case we want to implement (1.a) or (1.b) solution, we need to repopulate the completion list. This may require to have the GtkTreeFilterModel as a property of this class. Same applies in the down arrow key case.
Review of attachment 291964 [details] [review]: Looks ok! Do we want to have this in "reverse" method as well?
Review of attachment 291972 [details] [review]: Ok. So I will add a new version that should address this. Will unselect all rows on Escape. When we clear the list and then pres up/down, I will repopulate the list and it will be un-filtered in completion mode. In result mode it will show the results again. ::: src/placeEntry.js @@ +113,3 @@ + + /* Filter model based on input text */ + this._filter.refilter(); yep. @@ +155,3 @@ + return false; + + key = GLib.utf8_normalize (key, -1, GLib.NormalizeMode.ALL); nice catch. @@ +168,3 @@ + return false; + + }, Sure. ::: src/searchPopup.js @@ +202,3 @@ + return true; + } else if (keyval === Gdk.KEY_KP_Up || keyval === Gdk.KEY_Up) { + this.show(); Sure. We can have a vfunc_show that checks.
Created attachment 292736 [details] [review] placeListRow: Add type property
Created attachment 292737 [details] [review] Replace GtkEntryCompletion with SearchPopup
Created attachment 292738 [details] [review] Add cancellable to geocode search
Created attachment 292928 [details] [review] placeListRow: Add type property
Created attachment 292929 [details] [review] Replace GtkEntryCompletion with SearchPopup
Created attachment 292930 [details] [review] geocodeService: Add support for cancellable
Comment on attachment 292928 [details] [review] placeListRow: Add type property Thanks!