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 725622 - Can't quit search preferences by pressing outside the slide down box
Can't quit search preferences by pressing outside the slide down box
Status: RESOLVED FIXED
Product: gnome-documents
Classification: Core
Component: general
3.12.x
Other Linux
: Normal normal
: ---
Assigned To: GNOME documents maintainer(s)
GNOME documents maintainer(s)
ready
Depends on: 728228
Blocks:
 
 
Reported: 2014-03-03 22:19 UTC by Kat
Modified: 2014-07-30 13:20 UTC
See Also:
GNOME target: ---
GNOME version: 3.11/3.12


Attachments
Allows quit from search preferences by clicking outside or typing the ESC key (2.29 KB, patch)
2014-03-15 23:23 UTC, Saulo Toledo
needs-work Details | Review
The new attachment solves the ESC key problem. Some comments: (1.79 KB, patch)
2014-03-17 15:50 UTC, Saulo Toledo
reviewed Details | Review
GtkPopover allows hiding interface elements out of the way. The problem of click handling for dropdown menu is solved by using GtkPopover. (5.31 KB, patch)
2014-03-18 16:07 UTC, Marta Milakovic
needs-work Details | Review
While GtkPopover's version is being reviewed, I'm attaching an update for the GtkRevealer's version. (2.21 KB, patch)
2014-03-21 02:12 UTC, Saulo Toledo
rejected Details | Review
Updated version. (5.45 KB, patch)
2014-03-21 13:44 UTC, Marta Milakovic
reviewed Details | Review
Ports Dropdown of the Overview Searchbar to GtkPopover (5.24 KB, patch)
2014-04-15 22:25 UTC, Marta Milakovic
committed Details | Review

Description Kat 2014-03-03 22:19:25 UTC
Search preferences should be quitable by pressing outside the slide down box that has the preferences in it. It would also be nice if it was quitable by hitting the Esc key.

FWIW, it's also very difficult to open the prefs with keyboard navigation: it's not possible to tab into it from the search box.
Comment 1 Saulo Toledo 2014-03-15 23:23:14 UTC
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.
Comment 2 Saulo Toledo 2014-03-15 23:34:46 UTC
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.
Comment 3 Cosimo Cecchi 2014-03-16 03:47:03 UTC
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.
Comment 4 Saulo Toledo 2014-03-17 15:50:21 UTC
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.
Comment 5 Cosimo Cecchi 2014-03-17 18:56:33 UTC
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.
Comment 6 Marta Milakovic 2014-03-18 16:07:52 UTC
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 ;)
Comment 7 Cosimo Cecchi 2014-03-20 21:48:48 UTC
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?
Comment 8 Saulo Toledo 2014-03-20 22:20:38 UTC
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.
Comment 9 Saulo Toledo 2014-03-21 02:12:18 UTC
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.
Comment 10 Marta Milakovic 2014-03-21 13:44:47 UTC
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?!
Comment 11 Allan Day 2014-03-28 17:40:21 UTC
Using a popover seems like the right approach here. It would be nice to get this landed.
Comment 12 Cosimo Cecchi 2014-04-15 00:27:39 UTC
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.
Comment 13 Marta Milakovic 2014-04-15 20:48:55 UTC
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.
Comment 14 Cosimo Cecchi 2014-04-15 20:56:58 UTC
Yes, if that's the case, I completely agree with you.
Comment 15 Marta Milakovic 2014-04-15 22:25:18 UTC
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.
Comment 16 Cosimo Cecchi 2014-07-30 13:20:13 UTC
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.