GNOME Bugzilla – Bug 751913
GtkEntry: support setting the popup position for GtkEntryCompletion
Last modified: 2015-07-08 03:13:52 UTC
It'd be very nice if GtkEntryCompletion supports setting the position of the popup window relative to the entry. In fact, this is needed to fully implement GtkPlacesView. More discussion is certainly needed.
Created attachment 306758 [details] [review] entrycompletion: enable changing popup position GtkEntryCompletion only handles bottom popups, and it's enought for most cases. There is, however, a considerable number of situations where it is good to change the position of the popup window. The new GtkPlacesView widget, for example, requires that the popup window is placed above the entry, not below. Since the widget has a minimal space below, the popup window is narrowed down to a ridiculous height, only to fit the space below the entry. By adding a GtkEntryCompletion::popup-position property, GtkEntryCompletion is able to know where the user wants to place the popup window, and reposition it properly.
Created attachment 306760 [details] [review] entrycompletion: enable changing popup position Fix indentation.
Created attachment 306761 [details] [review] entrycompletion: enable changing popup position More indenting issues.
Review of attachment 306761 [details] [review]: ::: gtk/gtkentrycompletion.c @@ +420,3 @@ + g_object_class_install_property (object_class, + PROP_POPUP_POSITION, + g_param_spec_int ("popup-position", Please use g_param_spec_enum() here. set/get_property will need using g_value_set/get_enum then. @@ +703,3 @@ + case PROP_POPUP_POSITION: + gtk_entry_completion_set_popup_position (completion, you slipped a tab in there :). gtk+ files are supposed to be indented with spaces, even though usage is mixed at places. Would be good not to perpetuate that IMO. @@ +1607,3 @@ &popup_req, NULL); + above = FALSE; nitpick, "above" isn't quite descriptive anymore after this change. I also see that it effectively goes nowhere (no caller of _gtk_entry_completion_resize_popup() checks the return value). Would be really nice if you could clean that up and make that function return void, preferably in a separate commit. @@ +1640,3 @@ x = monitor.x + monitor.width - popup_req.width; + if (pos == GTK_POS_TOP) Hmm, without having tried it yet. This doesn't look quite alright to me. We enter the if() branch if pos == GTK_POS_TOP, but in the else on all other 3? Also, this code is meant to constrain the popup to the current monitor, we should be doing something like that in the X axis as well. @@ +2250,3 @@ + * + * Since: 3.18 + */ Nitpick, functions would need adding to docs/reference/gtk/gtk3-sections.txt :) @@ +2264,3 @@ + { + _gtk_entry_completion_popdown (completion); + gtk_entry_completion_popup(completion); Can't we just call _gtk_entry_completion_resize_popup() here? popping down and up again will bring in extra grabbing work, the selected option being unselected, etc.
Created attachment 306762 [details] [review] entrycompletion: no need to return boolean The boolean _gtk_entry_completion_resize_popup's return value is not used anywhere, and only adds more complexity for the method.
Created attachment 306764 [details] [review] entrycompletion: no need to return boolean The boolean _gtk_entry_completion_resize_popup's return value is not used anywhere, and only adds more complexity for the method.
Review of attachment 306764 [details] [review]: Looks good, thanks!
I'm not sure I follow the argument in the commit message. "a considerable number of cases" seems like an exaggeration - its only one case you mention. But even before that, I am not sure I understand why the current entry completion behavior is not sufficient. It already moves the popup to the top if there is not enough space at the bottom...
Created attachment 306804 [details] small popup window (In reply to Matthias Clasen from comment #8) > I'm not sure I follow the argument in the commit message. "a considerable > number of cases" seems like an exaggeration - its only one case you mention. You're totally right, I'll fix the commit message as well as the code. > But even before that, I am not sure I understand why the current entry > completion behavior is not sufficient. It already moves the popup to the top > if there is not enough space at the bottom... The image I'm attaching with this comment is the demonstration of what I'm trying to say. The popup window desperately tries to fit the small space below the entry, but it gets so small that not even 1 row is properly shown. The fact that there's not API to change that is what drives me to fix this bug.
Obviously, it's more than enought to fix that very code instead of adding more API to an already settled component.
(In reply to Georges Basile Stavracas Neto from comment #9) > Created attachment 306804 [details] > small popup window > > (In reply to Matthias Clasen from comment #8) > > I'm not sure I follow the argument in the commit message. "a considerable > > number of cases" seems like an exaggeration - its only one case you mention. > > You're totally right, I'll fix the commit message as well as the code. > > > But even before that, I am not sure I understand why the current entry > > completion behavior is not sufficient. It already moves the popup to the top > > if there is not enough space at the bottom... > > The image I'm attaching with this comment is the demonstration of what I'm > trying to say. The popup window desperately tries to fit the small space > below the entry, but it gets so small that not even 1 row is properly shown. > > The fact that there's not API to change that is what drives me to fix this > bug. That looks more like an empty popup than like a popup that is getting too small by trying to squeeze under the entry, tbh. The completion popup is not constrained by the widget or even the toplevel window it is attached to - it should size itself to fit on the screen, nothing more.
Created attachment 306926 [details] selected item in small popup window (In reply to Matthias Clasen from comment #11) > That looks more like an empty popup than like a popup that is getting too > small by trying to squeeze under the entry, tbh. I tested it multiple times here, and there are at least 2 items in this popup window (from the previous attached screenshot). There is even the dashed line for scrollable content, and I can select the items using keyboard (see the screenshot attached to this comment).
Ah, I think I know what is going on here: the scropled window no longer gets a guaranteed min height because of overlay scrollbars
Can this be reproduced on some branch ?
(In reply to Matthias Clasen from comment #14) > Can this be reproduced on some branch ? You can see it happening on my Gtk+'s branch wip/gbsneto/other-locations
ok, your problem turned out to be a different one: setting GtkEntryCompletion::text-column generically (e.g., from the ui file, or with g_object_set) does *not* have the same side-effect as gtk_entry_completion_set_text_column of adding a cell renderer. This patch fixes your completion popup: diff --git a/gtk/ui/gtkplacesview.ui b/gtk/ui/gtkplacesview.ui index 854eb30..4084766 100644 --- a/gtk/ui/gtkplacesview.ui +++ b/gtk/ui/gtkplacesview.ui @@ -15,6 +15,12 @@ <property name="text_column">1</property> <property name="inline_completion">True</property> <property name="inline_selection">True</property> + <child> + <object class="GtkCellRendererText"/> + <attributes> + <attribute name="text">1</attribute> + </attributes> + </child> </object> <object class="GtkPopover" id="recent_servers_popover"> <child> After playing with this for 5 minutes, I have to say that I find the combination of entry completion and the 'recent servers' popover strange and redundant. I think I'd prefer to just have the popover and a plain entry. Also: the placeholder text is too long... a bit ironic to have a placeholder text with an example that is almost completely ellipsized away. I'd suggest to just say "Server URL" instead of trying to fit a label+example in this cramped space.