GNOME Bugzilla – Bug 765003
DND to overview is broken on wayland
Last modified: 2017-03-13 11:24:24 UTC
E.g. start a drag of a file in nautilus, then just move your mouse cursor in the upper left corner. The activities overview will open, but the cursor doesn't indicate the ongoing drag operation anymore and selecting another window results in weird ongoing-but-not-working-grab behavior.
(In reply to Timm Bäder from comment #0) > E.g. start a drag of a file in nautilus, then just move your mouse cursor in > the upper left corner. The activities overview will open Up until here, the behavior is intentional (see bug 601731). > but the cursor doesn't indicate the ongoing drag operation anymore and > selecting another window results in weird ongoing-but-not-working-grab > behavior. Yeah, that's broken on wayland.
*** Bug 765524 has been marked as a duplicate of this bug. ***
This problem continues in GS 3.22. While testing Fedora Workstation 25 Beta I found a significant difference between Xorg and Wayland drag and drop behavior. The Xorg drag and drop is intuitive and works fine in GS 3.22. The Wayland drag and drop works workspace to workspace but it is not obvious what to do. You must drag the object into the hot corner. When the Overview is displayed, move cursor into the Overview. The icon is detached from the cursor and remains at the corner. You then release the mouse button and move the cursor to the workspace with the target. Press and release the mouse button to select the target. The target will be moved from the workspace to the body of the Overview. Move the mouse button to the target and press and release the mouse button. The Overview will close and the icon appears and reconnects to the cursor. Click the target and the object will be dropped. I have placed a compressed file with two short videos of Xorg and Wayland drags from an application to an application in a different workspace on my google-drive at: http://tinyurl.com/hvlebtw
Moving to mutter. It's not completely broken, its just that the DND feedback (the drag icon) is not updated during compositor grabs. If you continue your drag after having changed worskpace or whatever, and returned to not-overview mode it should already work.
There is one markedly different aspect to the DND behavior between Xorg and Wayland in the Overview. In Wayland you must release the mouse button to select the workspace and click the target application in the Overview to select and exit the Overview. In Xorg you must not release the mouse button until you reach the target. In Xorg dragging the icon into the workspace will switch it and then continue dragging the icon to the target application in the Overview will select and exit the Overview it. I certainly could be wrong but this seems more than simply the loss of visual feedback.
Created attachment 339499 [details] [review] wayland: emit plugin-dnd-{begin/position-changed/end} as the equivalent of xdnd signals
Created attachment 339500 [details] [review] shell-global: Add meta plugin property
Created attachment 339501 [details] [review] xdndhandler: handle the DND signals from wayland compositor
gnome-shell cannot receive xdnd signals anymore when it runs as a wayland compositor. The substitute of xdnd signals are needed to notify the dnd situation to gnome-shell directly. I thought that meta-plugin is a good point for these kinds of direct communication, because gnome-shell is a plugin for mutter. The patches implements the emission of plugin-dnd-{begin/position-changed/end} as the equivalent of xdnd-{enter/position-changed/leave} in mutter side, and the handling of these signals in gnome-shell side. This patches are not perfect. It cannot handle when the drag ends at app icon in overview mode, and there can be other trivial bugs. But because I am pretty new to gnome-shell/mutter, I want to hear some opinion about this approach before refine the patch more. Thanks.
*** Bug 775716 has been marked as a duplicate of this bug. ***
I have three comments about the approach in these patches: 1. I don't want to add any per-display-protocol code paths in gnome-shell unless we really need to. In this case, I think it would be much better to add a generic API similat to the one added in this patch set and have the shell just connect to one. On the mutter side we'd add the X11 vs Wayland abstraction needed. 2. Not so fond of placing anything on the plugin. AFAIK the plugin thing is eventually going away (being replaced with a pure library kind of API, replacing anything that had to do with having todays "plugins"). Would have been nice if we had a MetaSeat thing now to put this in. The next best thing I guess is MetaDisplay. 3. Not so sure how this should fit with the "ClutterGrab" kind of thing that is being baked. CC:ing Carlos to see if he has any comments on that. With that in place, this would be done pretty differently (not by hooking onto any stage signals, but instead having a "dnd drag" grab that interacts with other grabs. From a shell point of view I suspect it might look the same though, i.e. a separate set of signals that the "dnd drag" grab would deal with.
(In reply to Jonas Ådahl from comment #11) > I have three comments about the approach in these patches: > > 1. I don't want to add any per-display-protocol code paths in gnome-shell > unless we really need to. In this case, I think it would be much better to > add a generic API similat to the one added in this patch set and have the > shell just connect to one. On the mutter side we'd add the X11 vs Wayland > abstraction needed. There's actually nothing xdnd specific in the ShellGlobal::xdnd-* signals themselves, they perhaps could do with some renaming but we may want to preserve compatibility for plugins here. > > 2. Not so fond of placing anything on the plugin. AFAIK the plugin thing is > eventually going away (being replaced with a pure library kind of API, > replacing anything that had to do with having todays "plugins"). Would have > been nice if we had a MetaSeat thing now to put this in. The next best thing > I guess is MetaDisplay. > > 3. Not so sure how this should fit with the "ClutterGrab" kind of thing that > is being baked. CC:ing Carlos to see if he has any comments on that. With > that in place, this would be done pretty differently (not by hooking onto > any stage signals, but instead having a "dnd drag" grab that interacts with > other grabs. From a shell point of view I suspect it might look the same > though, i.e. a separate set of signals that the "dnd drag" grab would deal > with. Hmm, I find this kind of an special case for a couple of reasons: - In X11, we have no compositor grab at all. There is the drag source client holding a pointer grab, so gnome-shell just forwards DnD ClientMessages sent by it. In this regard, I think we unfortunately can't do without the x11-specific ShellGlobal code handling this. - ClutterGrab is being designed around being stackable, but there is only one active grab (the topmost) at a time, the grabs that become shadowed can only jump ship or resist inactive until the grabs stacked on top are popped, but their event handlers don't run in the mean time. So it seems what we want here is 2 grabs to work together, regardless the order of precedence. I guess this affinity could be made explicit, since I think this is the right implicit behavior anyways.
(In reply to Carlos Garnacho from comment #12) > (In reply to Jonas Ådahl from comment #11) > > I have three comments about the approach in these patches: > > > > 1. I don't want to add any per-display-protocol code paths in gnome-shell > > unless we really need to. In this case, I think it would be much better to > > add a generic API similat to the one added in this patch set and have the > > shell just connect to one. On the mutter side we'd add the X11 vs Wayland > > abstraction needed. > > There's actually nothing xdnd specific in the ShellGlobal::xdnd-* signals > themselves, they perhaps could do with some renaming but we may want to > preserve compatibility for plugins here. The ShellGlobal::xdnd-* things seems to be completely inside gnome-shell, so I don't see anything that needs to be preserved, or am I missing something? > > > > > 2. Not so fond of placing anything on the plugin. AFAIK the plugin thing is > > eventually going away (being replaced with a pure library kind of API, > > replacing anything that had to do with having todays "plugins"). Would have > > been nice if we had a MetaSeat thing now to put this in. The next best thing > > I guess is MetaDisplay. > > > > 3. Not so sure how this should fit with the "ClutterGrab" kind of thing that > > is being baked. CC:ing Carlos to see if he has any comments on that. With > > that in place, this would be done pretty differently (not by hooking onto > > any stage signals, but instead having a "dnd drag" grab that interacts with > > other grabs. From a shell point of view I suspect it might look the same > > though, i.e. a separate set of signals that the "dnd drag" grab would deal > > with. > > Hmm, I find this kind of an special case for a couple of reasons: > > - In X11, we have no compositor grab at all. There is the drag source client > holding a pointer grab, so gnome-shell just forwards DnD ClientMessages sent > by it. In this regard, I think we unfortunately can't do without the > x11-specific ShellGlobal code handling this. Can't we move this abstraction, including the Xdnd things in shell-global.c, into mutter, hiding it behind some abstraction? Something like a MetaDnd with one implementation in each backend? > > - ClutterGrab is being designed around being stackable, but there is only > one active grab (the topmost) at a time, the grabs that become shadowed can > only jump ship or resist inactive until the grabs stacked on top are popped, > but their event handlers don't run in the mean time. > > So it seems what we want here is 2 grabs to work together, regardless the > order of precedence. I guess this affinity could be made explicit, since I > think this is the right implicit behavior anyways. Right, if grabs including a Dnd grab having been "overtaken" by an overview grab can still receive motion events, it can connect a MetaDnd and emit signals for the shell (currently xdndHandler.js) to listen to directly. I guess before we have ClutterGrab and friends we can still have a MetaDnd to abstract it for gnome-shell?
(In reply to Jonas Ådahl from comment #13) > (In reply to Carlos Garnacho from comment #12) > > (In reply to Jonas Ådahl from comment #11) > > > I have three comments about the approach in these patches: > > > > > > 1. I don't want to add any per-display-protocol code paths in gnome-shell > > > unless we really need to. In this case, I think it would be much better to > > > add a generic API similat to the one added in this patch set and have the > > > shell just connect to one. On the mutter side we'd add the X11 vs Wayland > > > abstraction needed. > > > > There's actually nothing xdnd specific in the ShellGlobal::xdnd-* signals > > themselves, they perhaps could do with some renaming but we may want to > > preserve compatibility for plugins here. > > The ShellGlobal::xdnd-* things seems to be completely inside gnome-shell, so > I don't see anything that needs to be preserved, or am I missing something? The ShellGlobal::xdnd-* are not specific to x11 and only inside gnome-shell. As Carlos said, we may rename them (to more inclusive one) so that gnome-shell can receive the same name signals in both X11 and wayland. > > > > > > > > > 2. Not so fond of placing anything on the plugin. AFAIK the plugin thing is > > > eventually going away (being replaced with a pure library kind of API, > > > replacing anything that had to do with having todays "plugins"). Would have > > > been nice if we had a MetaSeat thing now to put this in. The next best thing > > > I guess is MetaDisplay. As I didn't know that the plugin is temporary, I thought that mutter might have not only gnome-shell but also another plugin. So, mutter should have generic signals for those plugins. Yes. If the plugin is temporary, we might find another place to put this in. > > > > > > 3. Not so sure how this should fit with the "ClutterGrab" kind of thing that > > > is being baked. CC:ing Carlos to see if he has any comments on that. With > > > that in place, this would be done pretty differently (not by hooking onto > > > any stage signals, but instead having a "dnd drag" grab that interacts with > > > other grabs. From a shell point of view I suspect it might look the same > > > though, i.e. a separate set of signals that the "dnd drag" grab would deal > > > with. > > > > Hmm, I find this kind of an special case for a couple of reasons: > > > > - In X11, we have no compositor grab at all. There is the drag source client > > holding a pointer grab, so gnome-shell just forwards DnD ClientMessages sent > > by it. In this regard, I think we unfortunately can't do without the > > x11-specific ShellGlobal code handling this. > > Can't we move this abstraction, including the Xdnd things in shell-global.c, > into mutter, hiding it behind some abstraction? Something like a MetaDnd > with one implementation in each backend? > > > > > - ClutterGrab is being designed around being stackable, but there is only > > one active grab (the topmost) at a time, the grabs that become shadowed can > > only jump ship or resist inactive until the grabs stacked on top are popped, > > but their event handlers don't run in the mean time. > > > > So it seems what we want here is 2 grabs to work together, regardless the > > order of precedence. I guess this affinity could be made explicit, since I > > think this is the right implicit behavior anyways. > > Right, if grabs including a Dnd grab having been "overtaken" by an overview > grab can still receive motion events, it can connect a MetaDnd and emit > signals for the shell (currently xdndHandler.js) to listen to directly. I > guess before we have ClutterGrab and friends we can still have a MetaDnd to > abstract it for gnome-shell? In X11, xdnd event starts from X11 backend, when xserver sends the event 'XdndEnter' when gnome-shell is getting focus by starting to show the overview. So we might hide the Xdnd things in the plugin by implementing MetaDnd and using it in x11 backend code. The stack in X11 is like below: (mutter) x_event_source_dispatch handle_host_xevent meta_plugin_manager_xevent_filter _meta_plugin_xevent_filter (gnome-shell) gnome_shell_plugin_xevent_filter Also in wayland, dnd event starts from wayland backend(the function meta_wayland_data_device_start_drag). We could store MetaDnd started or not in some variable and check to emit 'MetaDnd start signal' when the modal begins at gnome-shell. Because in X11 XdndEnter event happens only when gnome-shell is getting focus by starting to show the overview, this additional connect behavior would be needed in wayland. With this modification, I think that we could remove the xdnd code in the plugin and implement them in each backend using MetaDnd. Also I agree that it would be much better than now. Plus, I never heard about ClutterGrab before. So I don't know how ClutterGrab can help this implementation. Can I see the code in a repository somewhere?
(In reply to Hyungwon Hwang from comment #14) > (In reply to Jonas Ådahl from comment #13) > > (In reply to Carlos Garnacho from comment #12) > > > (In reply to Jonas Ådahl from comment #11) > > > > I have three comments about the approach in these patches: > > > > > > > > 1. I don't want to add any per-display-protocol code paths in gnome-shell > > > > unless we really need to. In this case, I think it would be much better to > > > > add a generic API similat to the one added in this patch set and have the > > > > shell just connect to one. On the mutter side we'd add the X11 vs Wayland > > > > abstraction needed. > > > > > > There's actually nothing xdnd specific in the ShellGlobal::xdnd-* signals > > > themselves, they perhaps could do with some renaming but we may want to > > > preserve compatibility for plugins here. > > > > The ShellGlobal::xdnd-* things seems to be completely inside gnome-shell, so > > I don't see anything that needs to be preserved, or am I missing something? > > The ShellGlobal::xdnd-* are not specific to x11 and only inside gnome-shell. > As Carlos said, we may rename them (to more inclusive one) so that > gnome-shell can receive the same name signals in both X11 and wayland. Right. We can rename the signals and just use one set. What I meant was that we don't need to care about breaking other plugins/compositors than gnome-shell here, since this so far has been internal to gnome-shell only. > > > > > > > > > > > > > > 2. Not so fond of placing anything on the plugin. AFAIK the plugin thing is > > > > eventually going away (being replaced with a pure library kind of API, > > > > replacing anything that had to do with having todays "plugins"). Would have > > > > been nice if we had a MetaSeat thing now to put this in. The next best thing > > > > I guess is MetaDisplay. > > As I didn't know that the plugin is temporary, I thought that mutter might > have not only gnome-shell but also another plugin. So, mutter should have > generic signals for those plugins. Yes. If the plugin is temporary, we might > find another place to put this in. I don't think it's documented anywhere, so I don't blame you for not knowing :) It's not about only allowing gnome-shell, more about moving away from a libmutter vs mutter+plugin hybrid API to a pure libmutter API. > > > > > > > > > 3. Not so sure how this should fit with the "ClutterGrab" kind of thing that > > > > is being baked. CC:ing Carlos to see if he has any comments on that. With > > > > that in place, this would be done pretty differently (not by hooking onto > > > > any stage signals, but instead having a "dnd drag" grab that interacts with > > > > other grabs. From a shell point of view I suspect it might look the same > > > > though, i.e. a separate set of signals that the "dnd drag" grab would deal > > > > with. > > > > > > Hmm, I find this kind of an special case for a couple of reasons: > > > > > > - In X11, we have no compositor grab at all. There is the drag source client > > > holding a pointer grab, so gnome-shell just forwards DnD ClientMessages sent > > > by it. In this regard, I think we unfortunately can't do without the > > > x11-specific ShellGlobal code handling this. > > > > Can't we move this abstraction, including the Xdnd things in shell-global.c, > > into mutter, hiding it behind some abstraction? Something like a MetaDnd > > with one implementation in each backend? > > > > > > > > - ClutterGrab is being designed around being stackable, but there is only > > > one active grab (the topmost) at a time, the grabs that become shadowed can > > > only jump ship or resist inactive until the grabs stacked on top are popped, > > > but their event handlers don't run in the mean time. > > > > > > So it seems what we want here is 2 grabs to work together, regardless the > > > order of precedence. I guess this affinity could be made explicit, since I > > > think this is the right implicit behavior anyways. > > > > Right, if grabs including a Dnd grab having been "overtaken" by an overview > > grab can still receive motion events, it can connect a MetaDnd and emit > > signals for the shell (currently xdndHandler.js) to listen to directly. I > > guess before we have ClutterGrab and friends we can still have a MetaDnd to > > abstract it for gnome-shell? > > In X11, xdnd event starts from X11 backend, when xserver sends the event > 'XdndEnter' when gnome-shell is getting focus by starting to show the > overview. So we might hide the Xdnd things in the plugin by implementing > MetaDnd and using it in x11 backend code. > > The stack in X11 is like below: > (mutter) x_event_source_dispatch > handle_host_xevent > meta_plugin_manager_xevent_filter > _meta_plugin_xevent_filter > (gnome-shell) gnome_shell_plugin_xevent_filter > > Also in wayland, dnd event starts from wayland backend(the function > meta_wayland_data_device_start_drag). We could store MetaDnd started or not > in some variable and check to emit 'MetaDnd start signal' when the modal > begins at gnome-shell. Because in X11 XdndEnter event happens only when > gnome-shell is getting focus by starting to show the overview, this > additional connect behavior would be needed in wayland. > > With this modification, I think that we could remove the xdnd code in the > plugin and implement them in each backend using MetaDnd. Also I agree that > it would be much better than now. One issue is we don't have proper "X11 window manager" vs "Display server" abstraction. I'd suggest to introduce a MetaDnd (as a GObject based API; maybe make MetaBackend own it for now, until we have something better) and have a both an X11 implementation and a "display server" implementation in the same file behind a "meta_is_wayland_compositor()". We can't really put it in the x11 vs native backends, because it should work the same not depending on the backend but in what mode mutter is run (i.e. even if mutter runs nested (using the x11 backend) it should still behave the same as when it is run on the native backend). For X11, have the MetaDnd deal X events (moving that part from ShellGlobal), and for Wayland, have the Wayland data device stuff notify MetaDnd directly with the equivalent events while for now hooking into the stage or something (what you do in this patch set) until we have the grab chaining that ClutterGrab will provide. Does this sound reasonable? > > Plus, I never heard about ClutterGrab before. So I don't know how > ClutterGrab can help this implementation. Can I see the code in a repository > somewhere? It's just a design that has been discussed and aims to replace the multitude of different ways to grab input in mutter/gnome-shell (I think we have 3 or 4 different ways right now, all not working very well together). I'm not sure about the current implementation status.
(In reply to Jonas Ådahl from comment #15) > (In reply to Hyungwon Hwang from comment #14) > > (In reply to Jonas Ådahl from comment #13) > > > (In reply to Carlos Garnacho from comment #12) > > > > (In reply to Jonas Ådahl from comment #11) > > > > > I have three comments about the approach in these patches: > > > > > > > > > > 1. I don't want to add any per-display-protocol code paths in gnome-shell > > > > > unless we really need to. In this case, I think it would be much better to > > > > > add a generic API similat to the one added in this patch set and have the > > > > > shell just connect to one. On the mutter side we'd add the X11 vs Wayland > > > > > abstraction needed. > > > > > > > > There's actually nothing xdnd specific in the ShellGlobal::xdnd-* signals > > > > themselves, they perhaps could do with some renaming but we may want to > > > > preserve compatibility for plugins here. > > > > > > The ShellGlobal::xdnd-* things seems to be completely inside gnome-shell, so > > > I don't see anything that needs to be preserved, or am I missing something? > > > > The ShellGlobal::xdnd-* are not specific to x11 and only inside gnome-shell. > > As Carlos said, we may rename them (to more inclusive one) so that > > gnome-shell can receive the same name signals in both X11 and wayland. > > Right. We can rename the signals and just use one set. What I meant was that > we don't need to care about breaking other plugins/compositors than > gnome-shell here, since this so far has been internal to gnome-shell only. Yup, that's what I meant :), the signal signatures are generic enough. I'm not particularly fond of X event handling code in gnome-shell, so I'm definitely not against moving it to mutter domain. > > > > > > > > > > > > > > > > > > > > 2. Not so fond of placing anything on the plugin. AFAIK the plugin thing is > > > > > eventually going away (being replaced with a pure library kind of API, > > > > > replacing anything that had to do with having todays "plugins"). Would have > > > > > been nice if we had a MetaSeat thing now to put this in. The next best thing > > > > > I guess is MetaDisplay. > > > > As I didn't know that the plugin is temporary, I thought that mutter might > > have not only gnome-shell but also another plugin. So, mutter should have > > generic signals for those plugins. Yes. If the plugin is temporary, we might > > find another place to put this in. > > I don't think it's documented anywhere, so I don't blame you for not knowing > :) It's not about only allowing gnome-shell, more about moving away from a > libmutter vs mutter+plugin hybrid API to a pure libmutter API. > > > > > > > > > > > > > 3. Not so sure how this should fit with the "ClutterGrab" kind of thing that > > > > > is being baked. CC:ing Carlos to see if he has any comments on that. With > > > > > that in place, this would be done pretty differently (not by hooking onto > > > > > any stage signals, but instead having a "dnd drag" grab that interacts with > > > > > other grabs. From a shell point of view I suspect it might look the same > > > > > though, i.e. a separate set of signals that the "dnd drag" grab would deal > > > > > with. > > > > > > > > Hmm, I find this kind of an special case for a couple of reasons: > > > > > > > > - In X11, we have no compositor grab at all. There is the drag source client > > > > holding a pointer grab, so gnome-shell just forwards DnD ClientMessages sent > > > > by it. In this regard, I think we unfortunately can't do without the > > > > x11-specific ShellGlobal code handling this. > > > > > > Can't we move this abstraction, including the Xdnd things in shell-global.c, > > > into mutter, hiding it behind some abstraction? Something like a MetaDnd > > > with one implementation in each backend? > > > > > > > > > > > - ClutterGrab is being designed around being stackable, but there is only > > > > one active grab (the topmost) at a time, the grabs that become shadowed can > > > > only jump ship or resist inactive until the grabs stacked on top are popped, > > > > but their event handlers don't run in the mean time. > > > > > > > > So it seems what we want here is 2 grabs to work together, regardless the > > > > order of precedence. I guess this affinity could be made explicit, since I > > > > think this is the right implicit behavior anyways. > > > > > > Right, if grabs including a Dnd grab having been "overtaken" by an overview > > > grab can still receive motion events, it can connect a MetaDnd and emit > > > signals for the shell (currently xdndHandler.js) to listen to directly. I > > > guess before we have ClutterGrab and friends we can still have a MetaDnd to > > > abstract it for gnome-shell? > > > > In X11, xdnd event starts from X11 backend, when xserver sends the event > > 'XdndEnter' when gnome-shell is getting focus by starting to show the > > overview. So we might hide the Xdnd things in the plugin by implementing > > MetaDnd and using it in x11 backend code. > > > > The stack in X11 is like below: > > (mutter) x_event_source_dispatch > > handle_host_xevent > > meta_plugin_manager_xevent_filter > > _meta_plugin_xevent_filter > > (gnome-shell) gnome_shell_plugin_xevent_filter > > > > Also in wayland, dnd event starts from wayland backend(the function > > meta_wayland_data_device_start_drag). We could store MetaDnd started or not > > in some variable and check to emit 'MetaDnd start signal' when the modal > > begins at gnome-shell. Because in X11 XdndEnter event happens only when > > gnome-shell is getting focus by starting to show the overview, this > > additional connect behavior would be needed in wayland. > > > > With this modification, I think that we could remove the xdnd code in the > > plugin and implement them in each backend using MetaDnd. Also I agree that > > it would be much better than now. > > One issue is we don't have proper "X11 window manager" vs "Display server" > abstraction. I'd suggest to introduce a MetaDnd (as a GObject based API; > maybe make MetaBackend own it for now, until we have something better) and > have a both an X11 implementation and a "display server" implementation in > the same file behind a "meta_is_wayland_compositor()". We can't really put > it in the x11 vs native backends, because it should work the same not > depending on the backend but in what mode mutter is run (i.e. even if mutter > runs nested (using the x11 backend) it should still behave the same as when > it is run on the native backend). > > For X11, have the MetaDnd deal X events (moving that part from ShellGlobal), > and for Wayland, have the Wayland data device stuff notify MetaDnd directly > with the equivalent events while for now hooking into the stage or something > (what you do in this patch set) until we have the grab chaining that > ClutterGrab will provide. > > Does this sound reasonable? IMHO it does. I think such abstract interface is good to have despite ClutterGrab anyway, as otherwise gnome-shell has to poke the current grab stack, check there is a DnD one, figure out its state, and emit the appropriate high-level signals. This, apropos, reminds me we need a generic mutter abstraction for selections (perhaps also DnDs?) in order to allow the compositor to be source (or destination) of selections without loopbacks into X11. This is obviously not something to address in this bug, but this MetaDnD sounds logically related, so would be great if the API is designed with these future extensions in mind. > > > > > Plus, I never heard about ClutterGrab before. So I don't know how > > ClutterGrab can help this implementation. Can I see the code in a repository > > somewhere? > > It's just a design that has been discussed and aims to replace the multitude > of different ways to grab input in mutter/gnome-shell (I think we have 3 or > 4 different ways right now, all not working very well together). I'm not > sure about the current implementation status. It's still in kind of early stages... I've got the basic semantics working and some simple cases adapted, which pretty much makes it "yet another impl" atm. I'll feel more confident to push to a branch when I've ported more (complicated) cases. So I won't let this deter the nice patches from Hwang :), I think an specific abstraction is due anyway.
(In reply to Carlos Garnacho from comment #16) > (In reply to Jonas Ådahl from comment #15) ... snip ... > > > > One issue is we don't have proper "X11 window manager" vs "Display server" > > abstraction. I'd suggest to introduce a MetaDnd (as a GObject based API; > > maybe make MetaBackend own it for now, until we have something better) and > > have a both an X11 implementation and a "display server" implementation in > > the same file behind a "meta_is_wayland_compositor()". We can't really put > > it in the x11 vs native backends, because it should work the same not > > depending on the backend but in what mode mutter is run (i.e. even if mutter > > runs nested (using the x11 backend) it should still behave the same as when > > it is run on the native backend). > > > > For X11, have the MetaDnd deal X events (moving that part from ShellGlobal), > > and for Wayland, have the Wayland data device stuff notify MetaDnd directly > > with the equivalent events while for now hooking into the stage or something > > (what you do in this patch set) until we have the grab chaining that > > ClutterGrab will provide. > > > > Does this sound reasonable? > > IMHO it does. I think such abstract interface is good to have despite > ClutterGrab anyway, as otherwise gnome-shell has to poke the current grab > stack, check there is a DnD one, figure out its state, and emit the > appropriate high-level signals. Agreed. A MetaDnd still makes sense to sit between any ClutterGrab/whatever-we-have-now thing and gnome-shell. > > This, apropos, reminds me we need a generic mutter abstraction for > selections (perhaps also DnDs?) in order to allow the compositor to be > source (or destination) of selections without loopbacks into X11. This is > obviously not something to address in this bug, but this MetaDnD sounds > logically related, so would be great if the API is designed with these > future extensions in mind. Yea, and agreed, its out of scope for this bug. > > > > > > > > > Plus, I never heard about ClutterGrab before. So I don't know how > > > ClutterGrab can help this implementation. Can I see the code in a repository > > > somewhere? > > > > It's just a design that has been discussed and aims to replace the multitude > > of different ways to grab input in mutter/gnome-shell (I think we have 3 or > > 4 different ways right now, all not working very well together). I'm not > > sure about the current implementation status. > > It's still in kind of early stages... I've got the basic semantics working > and some simple cases adapted, which pretty much makes it "yet another impl" > atm. I'll feel more confident to push to a branch when I've ported more > (complicated) cases. > > So I won't let this deter the nice patches from Hwang :), I think an > specific abstraction is due anyway. Agreed!
Created attachment 342577 [details] [review] Rename the signals which have been used to handle XDnd events to more inclusive ones. So that these signals can be used to handle the DnD events in Wayland.
Created attachment 342578 [details] [review] xdnd: Remove XDnD handling code and receive DnD signals
Created attachment 342579 [details] [review] dnd: Implement MetaDnd
Created attachment 342580 [details] [review] dnd: Implement DnD handling code in Wayland
As we discussed, I reworked on the patches. With all your comments, it was really helpful to refine them. Thanks again! With the introduction of MetaDnd, the code seems much better than before. As move the event handling code from gnome-shell to mutter, XDnD timestamp propagation is removed. This code was there from when DnD code is implemented at first. But the propagation seems not have enough reason for existing (https://bugzilla.gnome.org/show_bug.cgi?id=601731). The testing was OK in my dual monitors. But we needs to discuss it if needed.
Review of attachment 342579 [details] [review]: Overall looks nice! Comments are mostly nitpicking style and bikeshedding where to put signals. ::: src/backends/meta-backend.c @@ +404,3 @@ NULL, NULL, NULL, G_TYPE_NONE, 1, G_TYPE_INT); + I wonder, should we put these in the MetaDnd object instead and expose that to gnome-shell? @@ +425,3 @@ + NULL, NULL, NULL, + G_TYPE_NONE, 0); + nit: Stray newline. ::: src/backends/x11/meta-backend-x11.c @@ +275,3 @@ { MetaCompositor *compositor = display->compositor; + meta_plugin_manager_xevent_filter (compositor->plugin_mgr, event); We should probably still bypass_clutter = TRUE here (as otherwise we are changing API). ::: src/compositor/meta-dnd.c @@ +18,3 @@ + */ + +#include <config.h> #include "config.h" @@ +23,3 @@ +#include "meta-backend-private.h" +#include "compositor-private.h" +#include "display-private.h" These are purely my own preferences, it's pretty mixed how it looks right now: Always use "" for our own headers (clutter, meta/, ...) and always paths relative to src/. @@ +27,3 @@ +struct _MetaDndPrivate +{ + gulong handler_id[3]; I'd prefer having three handler_id's, so we know what they do and what we have. It'd also be better to add these in the next commit, when they are being used for the first time. @@ +38,3 @@ +{ + G_OBJECT_CLASS (meta_dnd_parent_class)->finalize (object); +} Looks unnecessary? @@ +56,3 @@ + priv->handler_id[i] = 0; + + priv->compositor = NULL; No need to set anything to 0 or NULL here, that's done by g_object_new(). @@ +60,3 @@ + +static inline void +meta_dnd_notify_dnd_enter () Pass the MetaDnd here instead, so we don't have to look up the global over and over. A pipe dream I have is to one day remove our globals. @@ +66,3 @@ + +static inline void +meta_dnd_notify_dnd_position_changed (int x, int y) inline is superfluous (here and elsewhere), the compiler will probably ignore it. Coding style: function declaration/definition arguments on their own lines, with the variable names aligned. @@ +87,3 @@ +gboolean +meta_dnd_handle_xdnd_event (MetaCompositor *compositor, MetaDisplay *display, + XEvent *xev) Same here about coding style (one argument per line, align variable names). ::: src/meta/meta-dnd.h @@ +55,3 @@ +}; + +GType meta_dnd_get_type (void); Just replace all this with: G_DECLARE_FINAL_TYPE (MetaDnd, meta_dnd, META, DND, GObject) and move the struct _MetaDnd { ...} into the .c file. @@ +59,3 @@ +gboolean +meta_dnd_handle_xdnd_event (MetaCompositor *compositor, MetaDisplay *display, + XEvent *xev); In the header files we mostly have the return type on the same line as the function name, and also 1 line - 1 argument + aligned.
Review of attachment 342579 [details] [review]: Couple of more nits. ::: src/compositor/meta-dnd.c @@ +29,3 @@ + gulong handler_id[3]; + + MetaCompositor *compositor; Needs to be behind a #ifdef HAVE_WAYLAND and should be added to the next patch. ::: src/meta/meta-dnd.h @@ +60,3 @@ +meta_dnd_handle_xdnd_event (MetaCompositor *compositor, MetaDisplay *display, + XEvent *xev); +#endif Missing newline before last #endif, and missing /* META_DND_H */
Review of attachment 342579 [details] [review]: ::: src/compositor/meta-dnd.c @@ +29,3 @@ + gulong handler_id[3]; + + MetaCompositor *compositor; Sorry, ignore this comment.
Review of attachment 342580 [details] [review]: ::: src/compositor/meta-dnd.c @@ +145,3 @@ +static void +meta_dnd_wayland_position_change_notify (ClutterActor *actor, ClutterEvent *event, + MetaDndPrivate *priv) Coding style nits: argument placement and alignment. Also, I'd prefer not to pass around the priv, but the actual MetaDnd. @@ +163,3 @@ +static void +meta_dnd_wayland_end_notify (ClutterActor *actor, ClutterEvent *event, + MetaDndPrivate *priv) Coding style nits, same as above. @@ +175,3 @@ + break; + + case CLUTTER_BUTTON_RELEASE: nit: If you put the "end drag + disconnect handler" in a helper, you don't need to rely on the switch-case-break-through thing to avoid duplication. Hmm. Should probably count the number of pressed buttons? Or how do we handle cases where more than one button is held? @@ +181,3 @@ + g_signal_handler_disconnect (priv->compositor->stage, priv->handler_id[i]); + priv->handler_id[i] = 0; + } Coding style: - put 'int i' on the top of the block (not because of compiler compatibility, but for consistency). - { indented 2 spaces, then the actual code 2 more spaces (i.e. 4 compared to the "for ..." line. @@ +192,3 @@ + +void +meta_dnd_wayland_enter_check (MetaCompositor *compositor) naming nit: This function handles the "modal grabs", so maybe name it something related to that? maybe meta_dnd_wayland_handle_begin_modal() or something? @@ +200,3 @@ + if (priv->handler_id[0] == 0 && + meta_wayland_data_device_get_current_grab (&wl_compositor->seat->data_device) != NULL) + { Coding style: { indented 2 spaces, the code below two more. @@ +218,3 @@ + "key-press-event", + G_CALLBACK (meta_dnd_wayland_end_notify), + priv); Probably want liston for touch things here too. ::: src/meta/meta-dnd.h @@ +63,3 @@ +#ifdef HAVE_WAYLAND +void +meta_dnd_wayland_enter_check (MetaCompositor *compositor); coding style: void on the same line as meta_dnd... ::: src/wayland/meta-wayland-data-device.c @@ +819,3 @@ +void +meta_wayland_drag_grab_update_feedback_actor (MetaWaylandDragGrab *drag_grab, + ClutterEvent *event) style nit: align argument names. ::: src/wayland/meta-wayland-data-device.h @@ +140,3 @@ +void +meta_wayland_drag_grab_update_feedback_actor (MetaWaylandDragGrab *drag_grab, + ClutterEvent *event); void on the same line as meta_wayland_...., align argument names.
Review of attachment 342580 [details] [review]: ::: src/compositor/meta-dnd.c @@ +175,3 @@ + break; + + if (current_grab) I think I couldn't understand the meaning completely. Would it be better to put "end drag + disconnect handler" to meta_dnd_notify_dnd_leave()? In overview mode with DnD, would it be there some multiple button-pressed cases which we need to handle? Can a user do something except canceling the overview mode by pressing ESC key in keyboard?
Created attachment 342718 [details] [review] dnd: Implement MetaDnd
Created attachment 342719 [details] [review] dnd: Implement DnD handling code in Wayland
Created attachment 342720 [details] [review] xdnd: Remove XDnD handling code and receive DnD signals
Review of attachment 342718 [details] [review]: Mostly good. A few more things. ::: src/compositor/meta-dnd.c @@ +29,3 @@ +struct _MetaDndClass +{ + /*< private >*/ Well, this is in a non-introspected C file isn't it, so of course its private :P @@ +37,3 @@ + GObject parent; + + MetaDndPrivate *priv; This is not needed. @@ +46,3 @@ +}; + +G_DEFINE_TYPE_WITH_PRIVATE (MetaDnd, meta_dnd, G_TYPE_OBJECT); Just define it with G_DEFINE_TYPE () here. You can add the _WITH_PRIVATE in the patch you start adding used things to the private. @@ +83,3 @@ +meta_dnd_notify_dnd_enter (MetaDnd *dnd) +{ + g_signal_emit_by_name (dnd, "dnd-enter"); nit: Use a signal enum, as done elsewhere in mutter, so you can emit without using any strings. In mutter we only emit-by-string when we otherwise can't (e.g. because its in a D-Bus signal and hidden in the generated code). ::: src/meta/meta-dnd.h @@ +35,3 @@ + MetaCompositor *compositor, + MetaDisplay *display, + XEvent *xev); This should be put in a src/backend/meta-dnd-private.h as it should not be exposed outside of mutter. The same with the X related includes above.
Review of attachment 342719 [details] [review]: Mostly looks good. Just a couple of more nits. ::: src/compositor/meta-dnd.c @@ +206,3 @@ + meta_wayland_data_device_end_drag (&priv->wl_compositor->seat->data_device); + + for (i = 0; i < 3; i++) If you insist not using three explicitly named listeners (i.e. gulong motion_event_handler_id; gulong button_release_handler_id; gulong key_press_handler_id;) then at least use G_N_ELEMENTS () here. @@ +254,3 @@ + priv->handler_id[0] = g_signal_connect (compositor->stage, + "motion-event", + G_CALLBACK (meta_dnd_wayland_position_change_notify), Just use the same scheme as below, and call this meta_dnd_wayland_on_motion_event.
I'll leave the gnome-shell parts to give someone who knows the gnom-shell code better, but at a glance, they look good.
Review of attachment 342720 [details] [review]: I agree the gnome-shell bits look good, great to see that code go away. Consider this a-c-n when the other issues have been resolved.
Created attachment 344031 [details] [review] dnd: Implement MetaDnd
Created attachment 344032 [details] [review] dnd: Implement DnD handling code in Wayland
Created attachment 344033 [details] [review] xdnd: Rename XDnD handling signals to more inclusive ones
Created attachment 344034 [details] [review] xdnd: Remove XDnD handling code and receive DnD signals
I reflected things which Jonas reviewed such as: 1. public & private things are separated (meta_dnd_handle_xdnd_event & meta_dnd_wayland_handle_begin_modal in meta-dnd-private.h and MetaDnd itself in meta-dnd.h). 2. enum is used for signals, and g_signal_emit is used instead of g_signal_emit_by_name. Plus, I modified the signal mutter emits dnd-{enter/position-changed/leave} to dnd-{entered/position-changed/left} both in mu.tter and gnome-shell (which is the only thing changed from the previous patchset)
> Plus, I modified the signal mutter emits dnd-{enter/position-changed/leave} > to dnd-{entered/position-changed/left} both in mu.tter and gnome-shell > (which is the only thing changed from the previous patchset) I meant that this is the only thing changed only in gnome-shell.
(In reply to Hyungwon Hwang from comment #39) ... > Plus, I modified the signal mutter emits dnd-{enter/position-changed/leave} > to dnd-{entered/position-changed/left} both in mu.tter and gnome-shell > (which is the only thing changed from the previous patchset) These should probably have the same naming convention as ClutterGrab will. Carlos?
(In reply to Jonas Ådahl from comment #41) > (In reply to Hyungwon Hwang from comment #39) > > ... > > > Plus, I modified the signal mutter emits dnd-{enter/position-changed/leave} > > to dnd-{entered/position-changed/left} both in mu.tter and gnome-shell > > (which is the only thing changed from the previous patchset) > > These should probably have the same naming convention as ClutterGrab will. > Carlos? ClutterGrab doesn't have such high-level hooks, so it will still be up for MetaDnD to provide those. So I've got nothing against a name change nor MetaDnD picking the names it considers more sensible. However, I'd personally prefer to have those made all infinitives (enter/position-change/leave) than past tense. The past tense strikes me as factually wrong, as the signals are emitted right at the moment it's happening in the compositor POV, not any later (not counting X11 IPC latency).
I see. Well, for the record, I'd prefer enter/leave/position-change(or even motion), as thats more true and more consistent with other things doing the same (mostly the Wayland protocols I guess).
Created attachment 344232 [details] [review] xdnd: Rename XDnD handling signals to more inclusive ones
Created attachment 344233 [details] [review] xdnd: Remove XDnD handling code and receive DnD signals
Created attachment 344234 [details] [review] dnd: Implement MetaDnd
Created attachment 344235 [details] [review] dnd: Implement DnD handling code in Wayland
I am OK, only if they are all infinitives or all past tense. So I renamed them again to past tense as you all prefer. I also rebased the patches on top of the recent Jonas' patches in mutter.
Review of attachment 344234 [details] [review]: Looks good. Just a typo and a missing Makefile.am addition. No need to re-upload for that though. it can be fixed when landing. ::: src/Makefile.am @@ +172,3 @@ compositor/meta-cullable.c \ compositor/meta-cullable.h \ + compositor/meta-dnd.c \ missing compositor/meta-dnd-private.h ::: src/backends/meta-dnd-private.h @@ +18,3 @@ + */ + +#ifndef META_DND_PRIVATE__H typo: __ -> _
Review of attachment 344235 [details] [review]: Looks good to me.
Review of attachment 344232 [details] [review]: Sure.
Review of attachment 344233 [details] [review]: Very nice to see this gone from here!
Comment on attachment 344032 [details] [review] dnd: Implement DnD handling code in Wayland Marking the old patch as obsolete.
Seems this was not landed; I suspect you don't have commit access, Hyungwon? We are now in API freeze, and this is an API change, but might be worth requesting a freeze exception for this.
Attachment 344234 [details] pushed as 5fafaf9 - dnd: Implement MetaDnd Attachment 344235 [details] pushed as 65e9c89 - dnd: Implement DnD handling code in Wayland
Yes, I have no privilege to commit. Thanks for reviewing and landing them, Jonas & Carlos.