Manually unset XdndProxy ("Stop deleting dragged items, browser panel, pretty please")

GeorgesStavracas commented 3 years ago

Description

The explanation for this issue, and the subsequent fix I hereby propose, is long, too long. For that, my apologies.

When a browser panel is created on an X11 environment, it does so passing a CefWindowInfo set up through windowInfo.SetAsChild(windowId, rect). CEF then creates an X11 Window that is child of this windowId to render into. In addition to that, when the CEF window is shown, it walks up the window tree, and sets the XdndProxy atom of the topmost window to its own render X11 window. CEF does so to sneakily steal drag events from the toplevel.

However, this behavior is problematic for OBS Studio.

When OBS Studio has custom browser panels added and visible, and these panels are attached to the main window, the CEF widgetry is created and added to the main window. CEF then happily walks up the window tree, and sets the XdndProxy property of OBS Studio's main window.

This behavior is innocuous when the browser panel is in a detached dock, since CEF will set the XdndProxy atom of the dock window instead of OBS Studio's main window. CEF is also not aware of the dock window being attached to the main window, and won't try and reset the XdndProxy atom when it happens.

Having the XdndProxy atom set in the main window is the root of all evil we've seen so far. That's because when this atom is set, the xdndProxy() function in QXcbDrag reads the proxy window from OBS Studio's main window, and returns it. This function normally returns 0.

In particular, when xdndProxy() is called inside QXcbDrag::move() and returns a non-zero value, the if (!proxy_target) condition right after it isn't hit, making Qt use the CEF window as a drag proxy. The CEF window is then propagated to the current_proxy_target class member.

Then, when releasing the dragged item, QXcbDrag::drop() is called, and tries to find its own internal representation of the current_proxy_target window, which at this point is set to CEF's window - which Qt5 knows nothing about, and thus returns nullptr. This ends up skipping calling QXcbDrag::handleDrop() for OBS Studio's main window, sending the dragged item into the void, never to be seen again. Sorry about this terrible fate, dragged item 😢

Fix this whole mess by manually inspecting the toplevel window after setting up each CEF browser, and deleting the XdndProxy atom if it exists.

Fixes obsproject/obs-studio#4488

Motivation and Context

obsproject/obs-studio#4488

How Has This Been Tested?

Just try and reproduce obsproject/obs-studio#4488 - it shouldn't be reproducible anymore with this PR.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

[x] My code has been run through clang-format.
[x] I have read the contributing document.
[x] My code is not on the master branch.
[x] The code has been tested.
[x] All commit messages are properly formatted and commits squashed where appropriate.
[x] I have included updates to all appropriate documentation.

mihawk90 commented 3 years ago

So, thanks for you ping on Discord yesterday, I kinda forgot about testing this :)

Tested it today and I can indeed say it appears to fix the issue completely. I tried with a new scene in my existing scene collection and profile (logged into Twitch, Docks opened on start), and moving sources as well as scenes worked with no issues. Moving scenes into a newly created Group also worked. Also tested with a new config as per the repro steps, and it worked flawlessly as well.

Thanks for the work on this!

WizardCM commented 3 years ago

Thank you for spending the time tracking down this issue and coming up with a working solution.

I know you've already mentioned on Discord that this is likely one of the only solutions, but I wanted to document some of my thoughts here.

This seems like a strange behaviour on CEF's side to me. Yes, CEF is often used as the core GUI for apps (see Spotify), but it seems strange to me that this isn't a behaviour that can be avoided without a repetitive check to strip the XdndProxy. Ideally some sort of launch flag or toggle somewhere, or a different method of attaching CEF to a parent window (or a window in-between that ignores passing the XdndProxy up the chain?).

I think this is a good enough solution for now, but would like to potentially reach out to Marshall to see if there are cleaner methods of doing this.

One small request - could you add a comment by the unsetToplevelXdndProxy() function definition explaining the gist of why the code is necessary so that if another developer stumbles across it, they don't have to dig through git blame to see why it exists. I wouldn't want us to accidentally delete it in the future.

jp9000 commented 3 years ago

Damn this certainly is a nasty situation. This is a seriously clutch fix though, good work figuring it out, must have been a pain to figure out.

obsproject / obs-browser