Created attachment 335396 [details]
maps widget is misplaced after scrolling

Created attachment 335397 [details]
maps widget is misplaced completely (see comment #2 for details)

When following these steps
1. open gnome-contacts
2. select a contact with an address and some other contact details so that the contacts details can be scrolled
3. scroll down
4. minimize window or bring other window to front or go to gnome-shell overview mode

What happens:
Sometimes the maps widget is shown at pretty weird places

This looks pretty wild. Are you sure that when you updated libchamplain there wasn't some other system (e.g. wayland) update that could cause this? I'm not aware of any change in 0.12.14 that could possibly cause this and it shouldn't be possible to draw anything outside the application window unless the window manager is broken.

Wayland: no, wayland doesn't have code for that I think. And yes, applications can draw outside of windows, just think of menus, tooltips, popovers, etc. That's a change from X11 though, where X11 clients couldn't draw outside of their window.

Clutter: maybe. Or Gtk+ (Gdk). Or libchamplain. I don't know.

This issue is present for some time on wayland, including on 0.12.13, but as far as I know it was present even more than one year ago, so not a result of any recent change.

(In reply to Christian Stadelmann from comment #4)
> Wayland: no, wayland doesn't have code for that I think. And yes,
> applications can draw outside of windows, just think of menus, tooltips,
> popovers, etc. 

Those are basically separate windows, aren't they? I'm not familiar with Wayland backend of Gtk but I doubt it allows drawing things outside a window. If something is placed inside a GtkWidget, there should be no way for it to get completely outside it.

> This issue is present for some time on wayland, including on 0.12.13, but as
> far as I know it was present even more than one year ago, so not a result of
> any recent change.

If I have time, I can try to install Fedora with Wayland and test it. But even if I'm able to reproduce, I don't quite know what to change because this bug seems to be somewhere else.

gnome-contacts doesn't use linchamplain-gtk embed widget, it uses a libchamplain view directly into a clutter-gtk-embed

On Wayland, it uses a subsurface which explains why the map can appear outside of the toplevel window, because there is no clipping applied there (a Wayland subsurface is different from a X11 child window).

That said, it means it's an issue between clutter-gtk and gtk.

Amazingly, if the pointer hovers the scrollbar on the left of the gnome-contact window, the clutter-gtk subsurface gets moved at its expected location (wrt scrolling)...

Meanwhile, moving to clutter-gtk

We need to split this in two IMHO:

1. Moving the ClutterEmebd, that relies on configure events that gtk+/gdk doesn't emit when the window scrolls

2. Clipping, that would need to be done in clutter itself.

=> Moving to gtk+ as this is what I have for now...

Created attachment 337074 [details] [review]
[PATCH] gdkwindow: configure native windows when scrolling

ClutterEmbed on Wayland uses a subsurface and relocate it on a configure
handler, but when placed within a scrolled window, no configure event is
emitted and the CLutterEmbed subsurface remains static.

Emit a configure event for native windows in GdkWindow's internal
move_native_children() so that custom widgets relying on configure
events such as ClutterEmbed can relocate their stuff.

Clipping is another story... Not sure how to tackle that in clutter, but for now, at least, the ClutterEmbded widget is moved as it should (even though it can end up oustide of its toplevel due to missing clipping)

Created attachment 337315 [details] [review]
[PATCH] gdkwindow: configure native windows in move_native_children()

Updated patch - This also soves bug 767713

Review of attachment 337315 [details] [review]:

::: gdk/gdkwindow.c
@@ +5989,3 @@
+
+  display = gdk_window_get_display (window);
+  gdk_display_put_event (display, event);

Can be just "gdk_event_put (event);" since you do pretty much what that function does here.

@@ +6013,3 @@
+        {
+	  configure_native_child (child);
+	  move_native_children  (child);

Can fix the coding style issue (double space) while at it.

@@ +6102,3 @@
 			       window->width, window->height);
     }
+  else

How come this is needed? Shouldn't we still skip moving if the absolute position didn't change?

Created attachment 337492 [details] [review]
[PATCH v2] gdkwindow: configure native windows in move_native_children()

(In reply to Jonas Ådahl from comment #11)
> Review of attachment 337315 [details] [review] [review]:
> 
> ::: gdk/gdkwindow.c
> @@ +5989,3 @@
> +
> +  display = gdk_window_get_display (window);
> +  gdk_display_put_event (display, event);
> 
> Can be just "gdk_event_put (event);" since you do pretty much what that
> function does here.

Yeah good idea, and I was leaking the event as well (since it's a copy that gets added tothe queue,  v2 fixes that.
 
> @@ +6013,3 @@
> +        {
> +	  configure_native_child (child);
> +	  move_native_children  (child);
> 
> Can fix the coding style issue (double space) while at it.

Coding style is correct here, it's just that it uses a tab (8 char) so it shows weird in the patch. Anyhow, I replaced it with spaces, but there are tabs in many other places...
 
> @@ +6102,3 @@
>  			       window->width, window->height);
>      }
> +  else
> 
> How come this is needed? Shouldn't we still skip moving if the absolute
> position didn't change?

Yes, that's precisely the problem... abs_x/abs_y can be wrong when transitioning from/to fullscreen/maximized/normal because the shadows and header bar get added/removed.

You can try by applying the patch and leaving the test as-is, run totem, switch to maximized back and forth and the clutter-gtk embed widget will be misplaced (again).

Created attachment 337494 [details] [review]
[PATCH v2.1] gdkwindow: configure native windows in move_native_children()

Replace one more tab character remaining in the closing bracket...

(In reply to Olivier Fourdan from comment #12)
> Yes, that's precisely the problem... abs_x/abs_y can be wrong when
> transitioning from/to fullscreen/maximized/normal because the shadows and
> header bar get added/removed.

Just to clarify a bit here, this part there is to keep clutter-gtk's GtkClutterEmbed happy.

There are two things to consider, one is that clutter stage with gdk backend resizes on configure events whereas clutter-gtk GtkClutterEmbed widget reizes on size_allocate signal emitted by gtk+ (upper layer).

By putting traces in clutter gdk's backend _clutter_stage_gdk_notify_configure():

  https://git.gnome.org/browse/clutter/tree/clutter/gdk/clutter-stage-gdk.c#n282

And another in clutter-gtk gtk_clutter_embed_size_allocate():

  https://git.gnome.org/browse/clutter-gtk/tree/clutter-gtk/gtk-clutter-embed.c#n670

We can see exactly what happens with and without the patch when running totem while switchng to maximized state and back:

A) If we configure *only* if abs_x/abs_y have changed:

1. Start in normal state:

  Clutter-Message: _clutter_stage_gdk_notify_configure() window 0x24f2650 (26,70) [661x429]
  Clutter-Message: _clutter_stage_gdk_notify_configure() window 0x24f2650 (26,70) [661x429]
  Clutter-Gtk-Message: gtk_clutter_embed_size_allocate() window 0x24f2650 (0,0) [661x429]
  Clutter-Message: _clutter_stage_gdk_notify_configure() window 0x24f2650 (26,70) [661x429]

2. Switch to maximized state:

  Clutter-Gtk-Message: gtk_clutter_embed_size_allocate() window 0x24f2650 (0,0) [1972x1205]
  Clutter-Message: _clutter_stage_gdk_notify_configure() window 0x24f2650 (26,70) [1972x1205]
  Clutter-Gtk-Message: gtk_clutter_embed_size_allocate() window 0x24f2650 (0,0) [1920x1153]
  Clutter-Message: _clutter_stage_gdk_notify_configure() window 0x24f2650 (0,47) [1920x1153]
  Clutter-Message: _clutter_stage_gdk_notify_configure() window 0x24f2650 (0,47) [1972x1205]

3. Back to normal state:

  Clutter-Gtk-Message: gtk_clutter_embed_size_allocate() window 0x24f2650 (0,0) [609x377]
  Clutter-Message: _clutter_stage_gdk_notify_configure() window 0x24f2650 (0,47) [609x377]
  Clutter-Gtk-Message: gtk_clutter_embed_size_allocate() window 0x24f2650 (0,0) [661x429]
  Clutter-Message: _clutter_stage_gdk_notify_configure() window 0x24f2650 (26,70) [661x429]
  Clutter-Message: _clutter_stage_gdk_notify_configure() window 0x24f2650 (26,70) [609x377]

=> The final configure event gives (26,70) [609x377] which is too small

B) Now with sending a configure event even if the abs_x/abs_y didn't change:

1. Start in normal state:

  Clutter-Message: _clutter_stage_gdk_notify_configure() window 0x2a62650 (26,70) [661x429]
  Clutter-Message: _clutter_stage_gdk_notify_configure() window 0x2a62650 (26,70) [661x429]
  Clutter-Gtk-Message: gtk_clutter_embed_size_allocate() window 0x2a62650 (0,0) [661x429]
  Clutter-Message: _clutter_stage_gdk_notify_configure() window 0x2a62650 (26,70) [661x429]

2. Switch to maximized state:

  Clutter-Gtk-Message: gtk_clutter_embed_size_allocate() window 0x2a62650 (0,0) [1972x1205]
  Clutter-Message: _clutter_stage_gdk_notify_configure() window 0x2a62650 (26,70) [1972x1205]
  Clutter-Gtk-Message: gtk_clutter_embed_size_allocate() window 0x2a62650 (0,0) [1920x1153]
  Clutter-Message: _clutter_stage_gdk_notify_configure() window 0x2a62650 (0,47) [1920x1153]
  Clutter-Message: _clutter_stage_gdk_notify_configure() window 0x2a62650 (0,47) [1972x1205]
  Clutter-Message: _clutter_stage_gdk_notify_configure() window 0x2a62650 (0,47) [1972x1205]
  Clutter-Message: _clutter_stage_gdk_notify_configure() window 0x2a62650 (0,47) [1920x1153]
  Clutter-Message: _clutter_stage_gdk_notify_configure() window 0x2a62650 (0,47) [1920x1153]

3. Back to normal state:

  Clutter-Gtk-Message: gtk_clutter_embed_size_allocate() window 0x2a62650 (0,0) [609x377]
  Clutter-Message: _clutter_stage_gdk_notify_configure() window 0x2a62650 (0,47) [609x377]
  Clutter-Gtk-Message: gtk_clutter_embed_size_allocate() window 0x2a62650 (0,0) [661x429]
  Clutter-Message: _clutter_stage_gdk_notify_configure() window 0x2a62650 (26,70) [661x429]
  Clutter-Message: _clutter_stage_gdk_notify_configure() window 0x2a62650 (26,70) [609x377]
  Clutter-Message: _clutter_stage_gdk_notify_configure() window 0x2a62650 (26,70) [609x377]
  Clutter-Message: _clutter_stage_gdk_notify_configure() window 0x2a62650 (26,70) [661x429]
  Clutter-Message: _clutter_stage_gdk_notify_configure() window 0x2a62650 (26,70) [661x429]

=> The final configure event gives (26,70) [661x429] which is the right size for the clutter stage window

(In reply to Olivier Fourdan from comment #12)
> Created attachment 337492 [details] [review] [review]
> [PATCH v2] gdkwindow: configure native windows in move_native_children()
> 
> (In reply to Jonas Ådahl from comment #11)
> > Review of attachment 337315 [details] [review] [review] [review]:
> > 
> > ::: gdk/gdkwindow.c
> > @@ +5989,3 @@
> > +
> > +  display = gdk_window_get_display (window);
> > +  gdk_display_put_event (display, event);
> > 
> > Can be just "gdk_event_put (event);" since you do pretty much what that
> > function does here.
> 
> Yeah good idea, and I was leaking the event as well (since it's a copy that
> gets added tothe queue,  v2 fixes that.
>  
> > @@ +6013,3 @@
> > +        {
> > +	  configure_native_child (child);
> > +	  move_native_children  (child);
> > 
> > Can fix the coding style issue (double space) while at it.
> 
> Coding style is correct here, it's just that it uses a tab (8 char) so it
> shows weird in the patch. Anyhow, I replaced it with spaces, but there are
> tabs in many other places...
>  

I meant the double space between "move_native_children" and "(child);".

> > @@ +6102,3 @@
> >  			       window->width, window->height);
> >      }
> > +  else
> > 
> > How come this is needed? Shouldn't we still skip moving if the absolute
> > position didn't change?
> 
> Yes, that's precisely the problem... abs_x/abs_y can be wrong when
> transitioning from/to fullscreen/maximized/normal because the shadows and
> header bar get added/removed.

Ah, and at the time, the abs changes, the size hasn't caught up. I guess we could add 'did-things-change' guards (i.e. check whether the size actually changed before configuring) but I guess it doesn't hurt that much to let the callee do that.

I wonder if we can change this in gtk+4 so that 'configure' and gdkwindow's 'size' is always without shadow, then let the shadow expand the backing wl_surface size some other way, so we don't have all these races.

Review of attachment 337494 [details] [review]:

::: gdk/gdkwindow.c
@@ +6013,3 @@
+        {
+          configure_native_child (child);
+          move_native_children  (child);

As mentioned, the coding style fix I meant was the extra space between the function name and (.

Comment on attachment 337494 [details] [review]
[PATCH v2.1] gdkwindow: configure native windows in move_native_children()

attachment 337494 [details] [review] pushed with requested changes to git master as commit 9e2b1ad and to branch "gtk-3-22" as commit 12579fe - gdkwindow: configure native windows in move_native_children()

Positioning is solved on the gtk+ side with this bug here, but clipping is not implemented.

For this purpose, I filed bug 772840 against clutter to follow up on the clipping part.

I would reopen this bug, commit 9e2b1ad in master and commit 12579fe in branch gtk-3.22 are causing side effects (like in gvim, see bug 773387) so we would have to revert these changes eventually, I'm afraid.

Other side effects are listed in bug 767713.

So another possibility to address this issue would be to use a GDK_WINDOW_SUBSURFACE with the appropriate gdk API instead on implementing the subsurface directly, and link that to the gtk widget using gtk_widget_register_window() so that the subwindow gets moved along with the GtkClutterEmbed widget.

Moving to clutter-gtk

The issue with the maps widget location in gnome-contact would be fixed with the patches from bug 774534, bug 774546, bug 774915, bug 774917 and bug 774475

Positioning of the widget is now fixed by using gdk subsurfaces (bug 774546)