Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add drag n drop support to x11 #187

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

BillyDM
Copy link
Contributor

@BillyDM BillyDM commented Apr 1, 2024

No description provided.

src/x11/xcb_connection/get_property.rs Outdated Show resolved Hide resolved
@@ -0,0 +1,169 @@
// Code copied and modifed from https://github.com/rust-windowing/winit/blob/master/src/platform_impl/linux/x11/util/window_property.rs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are going to do this, please use a permalink that includes the commit hash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, didn't know you could do that.

Although what do you think, should we have attributions like this or not?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer not to copy files from other projects in the first place, but I think that doing so and then not providing an attribution is usually a license violation. In fact, I think there might not be enough information in this attribution to satisfy the redistribution clause of Winit's Apache 2.0 license anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, ok. I didn't copy it one for one. I'm not sure what the rules are for "some code was copied but then it was tweaked to fit the API of the codebase".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made changes to the attribution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've taken the liberty to make further changes to the attribution/license headers. This new version should properly satisfy section 4 of winit's APACHE license by retaining all copyright and attribution notices (I used the standard APACHE license header for that), while making it clear that we are claiming copyright, redistributing and re-licensing the changed version of the code under APACHE/MIT (as allowed by the last paragraph of section 4 of the APACHE license, and required for us to integrate and properly distribute that code).

Of course I am not a lawyer, but I think this is enough to satisfy winit's APACHE license now.

@prokopyl
Copy link
Member

On top of making a merge commit to bring this up to date with main, I made a few fixes:

  • Properly flush the connection every time we send a XdndStatus message, to make sure the source application receives it and keeps sending us position events.
  • Also properly flush the connection when making a XConvertSelection request, to make sure the source application receives it and sends us the SelectionNotify reply.
  • Change the XConvertSelection request logic to only send a request if we haven't requested the data yet, instead of sending a request if we haven't received (or successfully parsed) the data yet. This fixes an issue where multiple requests would be needlessly sent if we received a new position event before the transfer was complete.
  • Track the timestamp of the XdndPosition event from which we made an XConvertSelection request, and match it in the SelectionNotify reply handler, to make sure we aren't reading stale replies.
  • Competely clear the cached drag-n-drop state and data when receiving a new XdndEnter event. This fixes some state/caching issues, but it is still a brittle fix that may cause issues if concurrent (or stale) drag sessions' events reach the application.

For good measure, I've also updated the femtovg example, so that the little orange square that follows the mouse also updates drop DragEnter/Position/Drop events. 🙂

Copy link
Member

@prokopyl prokopyl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now, despite the fixes I've made to make it work on my computer (™), I still have some issues with the current implementation as is.

The biggest of those issues is that, IMO, the current winit X11 DnD implementation that this is based on is very brittle:

  • All of the drag session's state is stored in a bunch of Options, which not only make it a pain to know the current state, but also make it really hard to guard against invalid or unexpected events.
  • The get_property implementation completely blocks the thread waiting for a reply. This can lead to the application hanging if the source application doesn't reply fast enough. We should at least block with a timeout, but ideally this should be asynchronous and let us process other events while waiting for the transfer to complete.
    EDIT: Nevermind, I misunderstood that part. The data transfer is actually done through the server using properties, it is not sent peer-to-peer. So even though this is still synchronous, the only way we could hang is if the X Server doesn't reply, not the source application.

These robustness issues are, IMO, more critical in drag-n-drop than in other parts of the X11 codebase, because we are exchanging messages with an arbitrary application, not the X server itself.
While e.g. Dolphin may be reliable enough, the same cannot be said for other applications (those issues are also mentioned in the spec).

Outside of the robustness of the winit-originating code, there are also a few other things I found in the current implementation (although those are more open questions):

  • Currently, the drop is accepted instantly without first trying to receive or parse the data. This can lead to surprising behavior if an error occurs during parsing/receiving where the user sees that the baseview app accepts the drag but doesn't redraw accordingly, and nothing happens on drop.
  • Right now, the XdndEnter handling code immediately emits a DragEnter event, without waiting to receive position or drop data first. As far as I've tested, this seems inconsistent with other platforms, and forces users to handle the case of not having a reliable position at first, and for drop data to "appear" later.
    I think a more robust approach would be to wait for the data to be received and parsed before notifying the WindowHandler of any DnD event. This is also how the spec suggests handling this, by specifying the "widget" should only be notified when it can "look at" the data.
    This would also fix potential issues where the WindowHandler could "miss" drops if the data isn't received in time.

I can also work on incorporating those changes myself into your PR, instead of asking you to make more back-and-forths, if you're okay with it. 🙂

@BillyDM
Copy link
Contributor Author

BillyDM commented Apr 19, 2024

Yeah, I think it would be a good idea to follow how the spec suggests handling it.

I'm okay with you working on it if you're inclined to do so.

@prokopyl
Copy link
Member

I just finished rewriting the logic to use a specific enum to keep track of the XDND session state. 🙂

Incidentally I ended up completely rewriting the drag_and_drop file, and there isn't a single line of code remaining from winit, so I removed the attribution and license header from that file. (It's still there on the get_property file though, I haven't touched it.)

This commit should fix all of the issues I raised in my previous review. 🙂

@prokopyl prokopyl requested a review from micahrj April 22, 2024 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants