Created attachment 272045 [details] [review]
Allows quit from search preferences by clicking outside or typing the ESC key

Usability issues shown at bug https://bugzilla.gnome.org/show_bug.cgi?id=725622 suggest who implement this behavior could improve the search preference's usage.

The attached patch implements a solution for the ESC key.
The click outside also works, but an issue still persists: The documents list use a collection view and Gd.MainWidget, and disallows direct button click signals, so the Gtk.ApplicationWindow can't receive the click signal. So, click at document lists does not close the search preferences.

Review of attachment 272045 [details] [review]:

Thanks for the patch, Saulo. See my comments below.

::: src/searchbar.js
@@ +233,3 @@
+
+                if (keyval == Gdk.KEY_Escape) {
+                    this.emit('item-activated');

Couldn't you just call hide() on the dropdown here instead?

@@ +240,3 @@
+            }));
+
+                    this.emit('item-activated');

This is wrong - you're essentially eating all the mouse click events on the window forever after the dropdown has been created.

That doesn't matter much though, as the click handling should really be done with a GtkPopover; it's too late now for 3.12 though, so let's use this patch only for the Escape key and take the rest off.

Note that we'll branch off gnome-3-12 pretty soon now, so patches to port the dropdown to GtkPopover are more than welcome!

@@ +339,3 @@
             }));
 
+        this._dropdownButton.connect('key-press-event', Lang.bind(this,

Is this really needed? It's very unusual to catch key presses on a GtkButton.

Created attachment 272176 [details] [review]
The new attachment solves the ESC key problem. Some comments:

> Couldn't you just call hide() on the dropdown here instead?

It's not enough. The dropdown button receives that signal, changes his state and hides the box. Just call hide() don't change the button state.

> This is wrong - you're essentially eating all the mouse click events on the window forever after the dropdown has been created.

You're right. I removed that code.

> Is this really needed? It's very unusual to catch key presses on a GtkButton.

The idea was about if the button is focused (and the ESC key does not work). But my solution was wrong, and I changed it for this patch.

The new patch is attached. The old one is now obsolete.

Review of attachment 272176 [details] [review]:

::: src/searchbar.js
@@ +234,3 @@
+
+                if (keyval == Gdk.KEY_Escape) {
+                    this.emit('item-activated');

Hmm I get your point about the dropdown button not being synchronized in that case.
I think a better approach would be binding the visibility of the dropdown in a stronger way to the toggled state of the button, e.g. by using g_object_bind_property().
In any case let's leave it at this for now.

@@ +328,3 @@
                 let active = this._dropdownButton.get_active();
                 this._dropdown.widget.reveal_child = active;
+                this._dropdown.widget.get_children()[0].grab_focus();

I wonder if you couldn't just grab_focus() on this._dropdown.widget instead? I really don't like the get_children[0] here.

Created attachment 272296 [details] [review]
GtkPopover allows hiding interface elements out of the way.  The problem of click handling for dropdown menu is solved by using GtkPopover.

I played a little bit with the GtkPopover. It is a really nice, new feature, so I hope we can manage to implement it for gnome-3.12.

Cheers ;)

Review of attachment 272296 [details] [review]:

Thanks Marta, this looks mostly good. I have some comments below.

::: src/searchbar.js
@@ +212,2 @@
     _init: function() {
+        this.parent({ height_request: 240,

Why the hardcoded minimum height?

@@ +247,3 @@
     },
 
+    setState: function(state) {

I'd rather you removed this function and just called show() and hide() directly from OverviewSearchbar instead.

@@ +254,3 @@
     }
 });
 Signals.addSignalMethods(Dropdown.prototype);

If you extend from a GObject, I think you should make 'item-activated' a native signal I think... otherwise, you will be overriding the native connect/emit signals with this.
Alternatively, you can do what the application class does - see https://git.gnome.org/browse/gnome-documents/tree/src/application.js#n601

@@ +289,3 @@
         this._searchEntry.connect('tag-button-clicked',
             Lang.bind(this, this._onTagButtonClicked));
+        this._dropdown.set_relative_to(this._searchEntry);

Hrm, doesn't this make the popover point to the entry instead of the button?

Nice patch Marta! Some of my test results:
After apply attachment 272296 [details] [review], while running outside of Gnome (Fluxbox, to be exact), the GtkPopover isn't rendered and I get this error at terminal output:

(gnome-documents:11999): Gtk-CRITICAL **: gtk_render_frame_gap: assertion 'xy0_gap >= 0' failed

The bug isn't only with the GtkPopover, since I can write simple applications who works here. Only gnome-documents with the patch does not works. Is this a GTK bug?

Seems that the GtkPopover's closed event is not working.

Created attachment 272534 [details] [review]
While GtkPopover's version is being reviewed, I'm attaching an update for the GtkRevealer's version.

The ugly get_children()[0] code was removed. By usability questions, a grab_focus() for _searchEntry was added.
A focus-out event keeps the focus at search box while it's opened, to avoid ESC key do not work if the user clicks outside.

Created attachment 272565 [details] [review]
Updated version.

Changes:
- setState() is removed and show()/hide() are used directely.
- A native signals have been overridden while using addSignalMethods() (when extending from a GObject), so instead addJSSignalMethods() is used, because it adds methods with different names explicitly for JS signals.

The reason I hardcoded minimum height is because the Gtk.TreeView does not expand Gtk.Popover. The other solution that I have in mind could be to calculate the value (max number of rows) and set it as this.height_request.

Isn't the popover supposed to point to the _searchEntry as it is related to it and it should be positioned under the _searchEntry, not the _dropdownButton?!

Using a popover seems like the right approach here. It would be nice to get this landed.

Review of attachment 272565 [details] [review]:

Finally got back at investigating this. Turns out the height request problem is likely a bug in GTK - let's see if we can fix that properly before hardcoding a height here.

Other than that, the patch looks good, except a couple of minor details below.

::: src/searchbar.js
@@ +212,3 @@
     _init: function() {
+        this.parent({ height_request: 240,
+                      opacity: 0.9 });

No opacity should be needed here - the new mockups have the dropdown completely opaque.

See bug 728228, which I filed against GTK, for the height request problem.

@@ +245,3 @@
     }
 });
+Utils.addJSSignalMethods(Dropdown.prototype);

Sorry for being picky on this one, but I'd rather you used real signals here instead of the JSSignals hack.

@@ +252,3 @@
 
+    _init: function() {
+        this._dropdown = new Dropdown();

You can create this in createSearchWidgets() instead, and pass the relative_to widget directly between the construct params instead of later separately.

@@ +309,3 @@
+                    this._dropdown.hide();
+            }));
+        this._dropdown.connectJS('item-activated', Lang.bind(this,

See comment above about signals.

Thanks for the review Cosimo. 

I'm thinking if 'item-activated' is even needed in this implementation where we use Gtk.Popover? Whenever we activate an item in dropdown the signal 'closed' is being emitted, as the Gtk.Popover widget is unmapped on click (isn't that the primary use of Gtk.Popover). This means that we wouldn't need to connect 'item-activated', neither emit.

Yes, if that's the case, I completely agree with you.

Created attachment 274403 [details] [review]
Ports Dropdown of the Overview Searchbar to GtkPopover

GtkPopover allows hiding interface elements out of the way.
The problem of click handling for dropdown menu is solved by using
GtkPopover.
Minimum height is hardcoded until the bug in GTK is fixed.

Attachment 274403 [details] pushed as 06b7534 - Ports Dropdown of the Overview Searchbar to GtkPopover

I now pushed a patchset that implements this using GActions instead of a treeview - probably a good idea anyway - which doesn't hit the GTK bug with the popover sizing.
Closing as FIXED, thanks to everyone involved.