Comment on attachment 323362 [details] [review]
Don't reset the drag status if not needed

Thanks for the patch, I'm however not sure that's the right approach, that partly breaks the contract of GTK_DEST_DEFAULT_MOTION (which is issuing gdk_drag_status calls automatically).

I've been looking these last days into GtkNotebook tab DnD and also run into this, what's really happening here is that we possibly issue multiple gdk_drag_status() calls (with different drag actions) when processing the GDK_DRAG_MOTION event. This burst of action changes actually trigger a renegotiation, with signals emitted to both the drag source and destination. The destination side queues quickly get flooded by the many .set_action requests done and .action events received.

I think the solution is to coalesce these calls, so only one happens per GDK_DRAG_MOTION event. Because of the bottom to top approach in gtk_drag_find_widget(), we will be consistently sending the same status as long as the pointer sits in the same area, so we will be triggering a lot less noise at the protocol level.

I'm attaching a couple of patches with the fix I came up with.

Created attachment 323526 [details] [review]
gdkdnd: Add private gdk_drag_context_commit_drag_status() call

The way gdk_drag_status() may be called multiple times during the
processing of GDK_DRAG_MOTION events throughout the widget hierarchy
brings some superfluous messaging going in, esp. when it's the last
request the one we want to honor, yet we emit messaging requests on
all.

This is barely appreciable in the X11 backend, but due to the design
of the wayland protocol, quick series of changes like this it have
some self-amplificating consequences which may end up flooding the
connection.

Adding a gdk_drag_context_commit_drag_status() call, we ensure we
only do this once per GDK_DRAG_MOTION event, and only with the final
status, so we don't trigger spurious changes on the compositor
and the other peer.

Created attachment 323527 [details] [review]
gtkdnd: Use gdk_drag_context_commit_drag_status()

This way the status can just be pushed once to the windowing
per GDK_DRAG_MOTION event.

Review of attachment 323526 [details] [review]:

While looking at this I started wondering if we shouldn't avoid this private call nonsense by just calling this on the gdk side, at the bottom of _gdk_event_emit.

Created attachment 323608 [details] [review]
gdkdnd: Add private means to commit the drag status

The way gdk_drag_status() may be called multiple times during the
processing of GDK_DRAG_MOTION events throughout the widget hierarchy
brings some superfluous messaging going in, esp. when it's the last
request the one we want to honor, yet we emit messaging requests on
all.

This is barely appreciable in the X11 backend, but due to the design
of the wayland protocol, quick series of changes like this it have
some self-amplificating consequences which may end up flooding the
connection.

We can delegate this to a late "commit" call, performed within GDK
event management. This way gdk_drag_status() calls may be cached
and only result in windowing messaging once per GDK_DRAG_MOTION
event. Emitting the final status will also avoid spurious action
changes on the compositor and the other peer.

Didn't think at first of that one, much better than adding private calls around indeed.

Here is another concern I came upwith this morning:

The gdk dnd api lets you pretty freely call gdk_drag_status from whereever. With this change, I'm afraid that this might not work anymore for Wayland, since gdk_drag_status will not have any effect unless it is in response to a drag motion event.

Now, I don't know if such 'freestanding gdk_drag_status calls ever worked with Wayland in the first place...

(In reply to Matthias Clasen from comment #7)
> Here is another concern I came upwith this morning:
> 
> The gdk dnd api lets you pretty freely call gdk_drag_status from whereever.

Hmmm, wherever in response to DnD events, or crazier things like timeouts, etc? I thought the docs seemed clear enough (even if outdated/non-generic):

"This function is called by the drag destination in response to gdk_drag_motion() called by the drag source."

It's right I'm missing GDK_DRAG_ENTER in the switch though.

> With this change, I'm afraid that this might not work anymore for Wayland,
> since gdk_drag_status will not have any effect unless it is in response to a
> drag motion event.
> 
> Now, I don't know if such 'freestanding gdk_drag_status calls ever worked
> with Wayland in the first place...

According to the wayland DnD protocol, the wl_data_offer.set_actions request (which gdk_drag_status/commit_drag_status boil down to) are allowed "during dnd" (typically in response to wl_data_device.enter/motion) and once after wl_data_device.drop, only if an "ask" action was agreed previously. I guess we should also enforce the latter more pervasively.

The example I was thinking of is from the GtkWidget::drag-data-received docs:

  * The ::drag-data-received signal is emitted on the drop site when the
   * dragged data has been received. If the data was received in order to
   * determine whether the drop will be accepted, the handler is expected
   * to call gdk_drag_status() and not finish the drag.

Oh, right. We should be supporting that too. The wayland protocol also caters for the possibility of inspecting the data early before issuing wl_data_offer.set_actions. Looks like we should also trigger this after selection events.

Review of attachment 323362 [details] [review]:

.

Created attachment 323875 [details] [review]
gdkdnd: Add private means to commit the drag status

The way gdk_drag_status() may be called multiple times during the
processing of GDK_DRAG_MOTION events throughout the widget hierarchy
brings some superfluous messaging going in, esp. when it's the last
request the one we want to honor, yet we emit messaging requests on
all.

This is barely appreciable in the X11 backend, but due to the design
of the wayland protocol, quick series of changes like this it have
some self-amplificating consequences which may end up flooding the
connection.

We can delegate this to a late "commit" call, performed within GDK
event management. This way gdk_drag_status() calls may be cached
and only result in windowing messaging once per GDK_DRAG_MOTION
event. Emitting the final status will also avoid spurious action
changes on the compositor and the other peer.

Created attachment 323889 [details] [review]
gdkdnd: Add private means to commit the drag status

The way gdk_drag_status() may be called multiple times during the
processing of drag and drop events throughout the widget hierarchy
brings some superfluous messaging going in, esp. when it's the last
request the one we want to honor, yet we emit messaging requests on
all.

This is barely appreciable in the X11 backend, but due to the design
of the wayland protocol, quick series of changes like this it have
some self-amplificating consequences which may end up flooding the
connection.

We can delegate this to a late "commit" call, performed within GDK
event management. This way gdk_drag_status() calls may be cached
and only result in windowing messaging once per ::drag-motion or
::drag-data-received event. Emitting the final status will also
avoid spurious action changes on the compositor and the other peer.

This last patch fixes the issue raised in comment #9. It's now pushed to master.

Attachment 323889 [details] pushed as 2923f69 - gdkdnd: Add private means to commit the drag status