Created attachment 313616 [details] [review]
GtkTreeView: Use more consistant search window naming

Instead of alterating between search dialog and search window, use
search window everywhere.

Created attachment 313617 [details] [review]
wayland: Make window type conditions switches

Created attachment 313618 [details] [review]
wayland: Don't try to use subsurfaces as popup parents

If a GtkMenu (or something else that is mapped as a xdg_popup) tries to
use a subsurface window as a parent, it will be terminated by the
compositor due to protocol violation. So to avoid this, if a parent
window is not a xdg_popup or xdg_surface, i.e. a wl_subsurface, then
traverse up the trasient parents until we find the right popup parent.

Created attachment 313619 [details] [review]
GtkTreeView: Make a search window destroy helper

Created attachment 313620 [details] [review]
wayland: Map UTILITY hinted windows as subsurfaces

Currently used by GtkTreeView to map windows without changing focus. We
can't map this as a popup, because popup implies focus change.

Created attachment 313621 [details] [review]
GtkTreeView: Rework the search window hack so it also works on Wayland

The search window of a tree view was implemented by showing without
making it visible by by positioning it outside the screen edge. This is
not possible on Wayland, so implement another method for being able to
enter text into a non-visible entry.

The new method is implemented by, before showing the window, pass the
key event directly to the IM context backing the entry. If the key
event triggered the context to commit new text or change the preedit
content, the search window is shown, and from that point the key events
are forwarded directly to the entry widget.

Created attachment 313829 [details] [review]
Ensure the search_window exists

The call to gtk_tree_view_ensure_interactive_directory is what
ensures that tree_view->priv->search_window is not NULL, so don't
stash it away in a local variable beforehand.

Needed this fixup on top to avoid critical warnings.

Review of attachment 313616 [details] [review]:

sure

Review of attachment 313616 [details] [review]:

Typo in the commit message though: alternating, not alterating

Review of attachment 313616 [details] [review]:

oh, and consistent, not consistant

Review of attachment 313617 [details] [review]:

Would be nice to say something like: this will make changes in future commits easier.

Review of attachment 313618 [details] [review]:

typo in the commit message: transient, not trasient. Looks fine otherwise

Review of attachment 313618 [details] [review]:

typo in the commit message: transient, not trasient. Looks fine otherwise

Review of attachment 313619 [details] [review]:

ok

Review of attachment 313620 [details] [review]:

did you look if anything else is using this window type, and will be ok with this change ?

Review of attachment 313621 [details] [review]:

::: gtk/gtktreeview.c
@@ +6035,2 @@
       gtk_tree_view_ensure_interactive_directory (tree_view);
 

The fixup I attached separately should probably be squashed here.

@@ +11168,3 @@
   gtk_window_set_modal (GTK_WINDOW (tree_view->priv->search_window), TRUE);
+  gtk_window_set_transient_for (GTK_WINDOW (tree_view->priv->search_window),
+                                GTK_WINDOW (toplevel));

Is this related to the key event handling changes ? looks unrelated to me.

@@ +11279,3 @@
+  /* Grab focus without selecting all the text. */
+  _gtk_entry_grab_focus (GTK_ENTRY (tree_view->priv->search_entry), FALSE);
+

We have public GtkEntry api for this now:  gtk_entry_grab_focus_without_selecting().
Might as well use that here, to reduce our reliance on private APIs.

(In reply to Matthias Clasen from comment #8)
> Needed this fixup on top to avoid critical warnings.

Yes, I have it amended into the commit locally, but forgot to upload again after discovering it.

(In reply to Matthias Clasen from comment #16)
> Review of attachment 313620 [details] [review] [review]:
> 
> did you look if anything else is using this window type, and will be ok with
> this change ?

Initially I searched the gtk+ tree, and it seemed safe. Now I did a more broad search, and it seems to be assumed to be a regular toplevel window sometimes, and the couple of places I saw this it was a GDK_WINDOW_TOPLEVEL). I think to limit mapping it as a subsurface only if the window type is GDK_WINDOW_POPUP instead. If we want to be extra safe, I assume we can use the private set-subsurface API, but I wonder if any client using an utility window as a popup expects the grab behaviour that comes with a xdg_popup.

Would could also change the type hint in GtkTreeView to TOOLTIP, as it is a no-input window. This is what I did initially, but it seemed odd to make it a "tooltip" when it was clearly not one. Is there any expected semantics of an utility type hint? 

(In reply to Matthias Clasen from comment #17)
> Review of attachment 313621 [details] [review] [review]:
> 
> @@ +11168,3 @@
>    gtk_window_set_modal (GTK_WINDOW (tree_view->priv->search_window), TRUE);
> +  gtk_window_set_transient_for (GTK_WINDOW (tree_view->priv->search_window),
> +                                GTK_WINDOW (toplevel));
> 
> Is this related to the key event handling changes ? looks unrelated to me.

In order to map the window as a subsurface we need to make it transient to the parent. Without this, we'd fail to map.

(In reply to Jonas Ådahl from comment #18)
> (In reply to Matthias Clasen from comment #8)
> > Needed this fixup on top to avoid critical warnings.
> 
> Yes, I have it amended into the commit locally, but forgot to upload again
> after discovering it.
> 
> (In reply to Matthias Clasen from comment #16)
> > Review of attachment 313620 [details] [review] [review] [review]:
> > 
> > did you look if anything else is using this window type, and will be ok with
> > this change ?
> 
> Initially I searched the gtk+ tree, and it seemed safe. Now I did a more
> broad search, and it seems to be assumed to be a regular toplevel window
> sometimes, and the couple of places I saw this it was a
> GDK_WINDOW_TOPLEVEL). I think to limit mapping it as a subsurface only if
> the window type is GDK_WINDOW_POPUP instead. If we want to be extra safe, I
> assume we can use the private set-subsurface API, but I wonder if any client
> using an utility window as a popup expects the grab behaviour that comes
> with a xdg_popup.
> 
> Would could also change the type hint in GtkTreeView to TOOLTIP, as it is a
> no-input window. This is what I did initially, but it seemed odd to make it
> a "tooltip" when it was clearly not one. Is there any expected semantics of
> an utility type hint? 

Unfortunately, the ewmh does not say much about expected semantics of window types, beyond some general musings:

http://standards.freedesktop.org/wm-spec/wm-spec-latest.html#idm140200472629520

I think the best we can probably do is to make it clear how _we_ are interpreting the semantics and offer API to override individual aspects of the semantics (focus-on-map, subsurface, stacking,...)

> (In reply to Matthias Clasen from comment #17)
> > Review of attachment 313621 [details] [review] [review] [review]:
> > 
> > @@ +11168,3 @@
> >    gtk_window_set_modal (GTK_WINDOW (tree_view->priv->search_window), TRUE);
> > +  gtk_window_set_transient_for (GTK_WINDOW (tree_view->priv->search_window),
> > +                                GTK_WINDOW (toplevel));
> > 
> > Is this related to the key event handling changes ? looks unrelated to me.
> 
> In order to map the window as a subsurface we need to make it transient to
> the parent. Without this, we'd fail to map.

I see. Leave it as-is, then.

Review of attachment 313620 [details] [review]:

lets go with this, and see what complaints we get

Review of attachment 313621 [details] [review]:

since you explained the transient-for,

Pushed, but with the 'wayland: Map UTILITY hinted windows as subsurfaces' patch changed to only map as a subsurface if the gtk window type was GTK_WINDOW_POPUP.

Attachment 313617 [details] pushed as 4979875 - wayland: Make window type conditions switches
Attachment 313618 [details] pushed as e25ea62 - wayland: Don't try to use subsurfaces as popup parents
Attachment 313619 [details] pushed as 97dbef4 - GtkTreeView: Make a search window destroy helper
Attachment 313621 [details] pushed as aedd193 - GtkTreeView: Rework the search window hack so it also works on Wayland