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

Implement clipboard_get/has_image for X11 #81439

Merged
merged 1 commit into from
Oct 20, 2023

Conversation

Setadokalo
Copy link
Contributor

Following up on the work done in #63826, this implements clipboard_has_image() and clipboard_get_image() on the X11 backend. This copies a lot of structure from the existing clipboard_get implementation.

This also only implements png-based clipboard transfers, but that seems to be how the windows implementation works too. Other file types shouldn't be too hard to implement, but every app I've tested with supports png target, even when the original file is a jpg or some other image type.

This is my first time working with X11 APIs, so testing from other linux users would be greatly appreciated!

@Setadokalo
Copy link
Contributor Author

With further testing, it seems particularly large images do trigger the incremental transfer mechanism (which I didn't experience in testing), and the incremental transfer does not work reliably. I'm looking into it now.

@Setadokalo
Copy link
Contributor Author

Setadokalo commented Sep 9, 2023

Large file transfers should now be fixed, but after fixing it I'm now almost certain the incremental handling for text clipboard data is and has been broken. Has it been tested? Unless someone asks me to, I won't fix it in this PR to keep the scope from creeping.
Testing the incremental transfer of text and it seems to work fine. Not sure how, but hey, as long as it works.

@Setadokalo Setadokalo force-pushed the clipboard_image branch 2 times, most recently from 1a6ddae to a22427c Compare September 15, 2023 00:49
@Setadokalo Setadokalo requested review from a team as code owners September 15, 2023 00:49
@Setadokalo Setadokalo force-pushed the clipboard_image branch 2 times, most recently from b82d675 to e4ccb10 Compare September 15, 2023 00:58
@AThousandShips AThousandShips removed request for a team September 15, 2023 09:53
@AThousandShips AThousandShips removed request for a team September 15, 2023 09:53
@akien-mga akien-mga changed the title Implement clipboard_get/has_image for X11 Implement clipboard_get/has_image for X11 Oct 10, 2023
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Style/code quality review. Looks great overall, at least as far as X11 API usage can look ;)

platform/linuxbsd/x11/display_server_x11.cpp Outdated Show resolved Hide resolved
platform/linuxbsd/x11/display_server_x11.cpp Show resolved Hide resolved
platform/linuxbsd/x11/display_server_x11.cpp Show resolved Hide resolved
platform/linuxbsd/x11/display_server_x11.cpp Outdated Show resolved Hide resolved
platform/linuxbsd/x11/display_server_x11.cpp Outdated Show resolved Hide resolved
platform/linuxbsd/x11/display_server_x11.cpp Outdated Show resolved Hide resolved
platform/linuxbsd/x11/display_server_x11.cpp Outdated Show resolved Hide resolved
platform/linuxbsd/x11/display_server_x11.cpp Outdated Show resolved Hide resolved
platform/linuxbsd/x11/display_server_x11.cpp Outdated Show resolved Hide resolved
platform/linuxbsd/x11/display_server_x11.cpp Outdated Show resolved Hide resolved
@YuriSizov
Copy link
Contributor

@Setadokalo Please let us know if you'll be able to address the review comments any time soon! Maintainers can finish it on your behalf if you are not available or don't respond.

@akien-mga
Copy link
Member

Could you squash the commits? See PR workflow for instructions.

@Setadokalo
Copy link
Contributor Author

I'm currently rewriting the last part of that function as I think you're right about it potentially double-freeing valid_targets. I'll squash it after I make sure it still works.

@akien-mga akien-mga merged commit 2f33c2b into godotengine:master Oct 20, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@Setadokalo Setadokalo deleted the clipboard_image branch October 26, 2023 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants