GNOME Bugzilla – Bug 719920
allow enter to activate first search item
Last modified: 2014-03-17 09:47:02 UTC
While watching rishi do a demo today I noticed him do a search that returned one result. It was weird that he had to use the mouse to open the search result. It would be nice to just hit enter - just like we do in the shell search.
Hi, I am newcomer and would like to work on this. Should arrow keys also be made functional after a particular item is searched ? I am thinking of automatically selecting the first document upon a search. Upon hitting enter, the selected item will open or the user can select other documents using arrow keys and then hit 'Enter' on whichever document he wants to open.
(In reply to comment #1) > I am thinking of automatically selecting the first document upon a search. Upon > hitting enter, the selected item will open or the user can select other > documents using arrow keys and then hit 'Enter' on whichever document he wants > to open. Just making ENTER open the first document would be a good first start. We can then mimic the behaviour of gnome-shell's search bar for the arrow keys.
Created attachment 269986 [details] [review] allow enter to activate first search item
Review of attachment 269986 [details] [review]: Thanks for the patch. You get bonus points for writing a well formatted commit message! ::: src/searchbar.js @@ +76,3 @@ + } + return true; + } This is the correct place in the code to plug in the change. However this is not a foolproof way to get the first item in the view. There is no guarantee that the first item returned by getItems is the first item in the view. I would suggest looking at the "id" of the first row in ViewModel and then use it to get the item out of the documentManager.
Created attachment 270051 [details] [review] allow enter to activate first search item Patch updated accordingly following suggestions from Comment 4.
Review of attachment 270051 [details] [review]: Great! This is much better. A few minor points: ::: src/embed.js @@ +280,3 @@ + function(model, path, iter) { + let id = model.get_value(iter, imports.gi.Gd.MainColumns.ID); + Application.documentManager.setActiveItem(Application.documentManager.getItemById(id)) Don't use tabs for indentation. @@ +360,3 @@ } + this._toolbar.searchbar.connect('enter-hit', + Lang.bind(this, this._onEnterHit)); Don't use tabs for indentation. ::: src/searchbar.js @@ +70,3 @@ } + else if (keyval == Gdk.KEY_Return) { + this.emit('enter-hit'); I think 'activate-result' is a better name. The fact that ENTER was hit is an implementation detail, and it would also match with the name of the signal that is emitted by the shell search provider.
Created attachment 270110 [details] [review] allow enter to activate first search item Fixed as per Comment 6.
Review of attachment 270110 [details] [review]: Much better. This is good to go apart from two minor issues that I forgot to point out earlier. Sorry about that. I will not harass you any further and fix them myself. ::: src/embed.js @@ +277,3 @@ + _onActivateResult: function() { + this._view.model.model.foreach(Lang.bind(this, function(model, path, iter) { It is better to use get_iter_first because it makes it obvious to the casual reader. @@ +278,3 @@ + _onActivateResult: function() { + this._view.model.model.foreach(Lang.bind(this, function(model, path, iter) { + let id = model.get_value(iter, imports.gi.Gd.MainColumns.ID); Define 'const Gd = imports.gi.Gd;' at the top and use 'Gd.MainColumns.ID' here.
Created attachment 270131 [details] [review] searchbar: Allow enter to activate first search item
Thanks for the patches, Pranav!
Oops! We did not test this code in edit mode: (gnome-documents:2324): Gjs-WARNING **: JS ERROR: Exception in callback for signal: window-mode-changed: TypeError: this._toolbar.searchbar is undefined Embed<._onWindowModeChanged@/opt/share/gnome-documents/js/embed.js:361 wrapper@resource:///org/gnome/gjs/modules/lang.js:169 _emit@resource:///org/gnome/gjs/modules/signals.js:124 ModeController<.setWindowMode@/opt/share/gnome-documents/js/windowMode.js:60 wrapper@resource:///org/gnome/gjs/modules/lang.js:169 EditView<._init/<@/opt/share/gnome-documents/js/edit.js:80 start@/opt/share/gnome-documents/js/main.js:29 @<command line>:1 Reopening.
Created attachment 271846 [details] [review] embed: Not every toolbar has a searchbar
Review of attachment 271846 [details] [review]: OK
Review of attachment 270131 [details] [review]: ::: src/embed.js @@ +278,3 @@ + _onActivateResult: function() { + let iter = this._view.model.model.get_iter_first()[1]; Ugh - can you please change this part of the patch to keep view.model private, and add a method to the view such as getFirstDocument()/getFirstDocumentId()?
Created attachment 271990 [details] [review] view: Add a method to get the first document and keep the model private
Review of attachment 271990 [details] [review]: Thanks, looks good.
Comment on attachment 271990 [details] [review] view: Add a method to get the first document and keep the model private Thanks for the review.