Hey Michael :), On Thu, Oct 1, 2015 at 12:43 AM, Michael Catanzaro <mcatanz...@igalia.com> wrote: > Reviewed-by: Michael Catanzaro <mcatanzaro at igalia.com> > > These are important fixes for the protocol that will allow drags to > work between GTK+ and Chrome. Thanks for working on this, Carlos!
Thanks for the review! > > Nit #1: the existing documentation doesn't use the DnD abbreviation, so > I would write out "drag-and-drop" in the documentation of the cancelled > event. > > Nit #2: You have a comma splice in the documentation for drag_finished; > you should change it to a semicolon, or split it into two sentences, or > add a conjunction. Right :), fixing these locally. Will send another patch when we've got an outcome for the question below. > > Minor issue: this and the next patch introduce the requirement that > accept is called on the data offer before the mouse grab is released. I > now see this has always been required by mutter -- not sure why -- but > it's new for Weston. (In fact, sending accepts has always been > optional.) The new restriction seems a bit arbitrary and could be hard > for some clients. Chrome's cross-platform DnD abstraction all but > requires drag data to have been received before it can determine > whether to accept a mime type, so Chrome doesn't send an accept until > it's done receiving. Previously, that just meant the cursor would not > be updated in response to motion events during that time, but now it > means the drop will fail. This is not a huge deal, and the other > changes in these patches are much more important, but it suggests we > shouldn't add this restriction unless there's a good reason behind > it... and I don't know of one, so I'd like to understand that. The > change isn't necessary for the protocol as-is, and doesn't seem > necessary for DnD actions, either? This is in order to be able to emit .cancelled if there is no agreement between source and destination about the data to be transferred. Otherwise every drop on any client with a wl_data_device would seem successful. I know .accept has had no functional role in the DnD state machine till now (besides UI feedback on the source-side, setting the appropriate cursor/dnd surfaces mostly), it made sense to me to reuse this though because compositor-side UI feedback and overall DnD outcome would be in sync for virtually every usecase I can imagine. I would expect .accept() to be called regularly on DnD motion over a surface, the pointer moves across inner widgets that accept different mimetypes or none. Actually, IMO there should be checks somewhere about the mimetype being one of the offered ones, in order to behave properly on wrong requests, although that means keeping the mimetype list in memory instead of just forwarding it. However, I can imagine other usecases where we don't know any concrete mimetype beforehand, eg. a "paste as html/plain text" popup menu on .drop_performed, there'd be no other way in these cases than preemptively picking one, as you say in your other mail. So perhaps it makes sense to split into a request with a boolean after all? Nonetheless, for the case of GTK+, both would be called from the same place (and .accept is already invariably called with the first mimetype from the list, hmm...) Cheers, Carlos _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
