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 719920 - allow enter to activate first search item
allow enter to activate first search item
Status: RESOLVED FIXED
Product: gnome-documents
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: GNOME documents maintainer(s)
GNOME documents maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2013-12-05 16:33 UTC by William Jon McCann
Modified: 2014-03-17 09:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
allow enter to activate first search item (1.07 KB, patch)
2014-02-22 10:58 UTC, Pranav Kant
needs-work Details | Review
allow enter to activate first search item (5.18 KB, patch)
2014-02-23 12:35 UTC, Pranav Kant
needs-work Details | Review
allow enter to activate first search item (5.25 KB, patch)
2014-02-24 11:28 UTC, Pranav Kant
reviewed Details | Review
searchbar: Allow enter to activate first search item (5.48 KB, patch)
2014-02-24 14:02 UTC, Debarshi Ray
committed Details | Review
embed: Not every toolbar has a searchbar (1.07 KB, patch)
2014-03-14 10:05 UTC, Debarshi Ray
committed Details | Review
view: Add a method to get the first document and keep the model private (3.70 KB, patch)
2014-03-15 09:07 UTC, Debarshi Ray
committed Details | Review

Description William Jon McCann 2013-12-05 16:33:41 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.
Comment 1 Pranav Kant 2014-02-18 16:00:44 UTC
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.
Comment 2 Debarshi Ray 2014-02-18 16:19:24 UTC
(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.
Comment 3 Pranav Kant 2014-02-22 10:58:04 UTC
Created attachment 269986 [details] [review]
allow enter to activate first search item
Comment 4 Debarshi Ray 2014-02-22 19:11:04 UTC
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.
Comment 5 Pranav Kant 2014-02-23 12:35:13 UTC
Created attachment 270051 [details] [review]
allow enter to activate first search item

Patch updated accordingly following suggestions from Comment 4.
Comment 6 Debarshi Ray 2014-02-24 10:13:29 UTC
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.
Comment 7 Pranav Kant 2014-02-24 11:28:12 UTC
Created attachment 270110 [details] [review]
allow enter to activate first search item

Fixed as per Comment 6.
Comment 8 Debarshi Ray 2014-02-24 14:01:00 UTC
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.
Comment 9 Debarshi Ray 2014-02-24 14:02:03 UTC
Created attachment 270131 [details] [review]
searchbar: Allow enter to activate first search item
Comment 10 Debarshi Ray 2014-02-24 14:03:03 UTC
Thanks for the patches, Pranav!
Comment 11 Debarshi Ray 2014-03-14 09:56:25 UTC
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.
Comment 12 Debarshi Ray 2014-03-14 10:05:28 UTC
Created attachment 271846 [details] [review]
embed: Not every toolbar has a searchbar
Comment 13 Cosimo Cecchi 2014-03-14 17:39:24 UTC
Review of attachment 271846 [details] [review]:

OK
Comment 14 Cosimo Cecchi 2014-03-14 17:41:26 UTC
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()?
Comment 15 Debarshi Ray 2014-03-15 09:07:26 UTC
Created attachment 271990 [details] [review]
view: Add a method to get the first document and keep the model private
Comment 16 Cosimo Cecchi 2014-03-16 03:10:16 UTC
Review of attachment 271990 [details] [review]:

Thanks, looks good.
Comment 17 Debarshi Ray 2014-03-17 09:46:50 UTC
Comment on attachment 271990 [details] [review]
view: Add a method to get the first document and keep the model private

Thanks for the review.