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 clipboard_has/get_image methods to DisplayServer #63826

Merged
merged 1 commit into from
Jul 27, 2023

Conversation

deakcor
Copy link
Contributor

@deakcor deakcor commented Aug 2, 2022

pr_paste_image_preview

Following proposal #2949, added

clipboard_get_image() to get image from the clipboard

clipboard_has_image() to check if the clipboard has an image. It's faster than clipboard_get_image() so It may be useful to make a check (to disable a "paste" button for example)

Override of clipboard_has(), may be faster for a huge text.

They are only implemented on Windows and MacOS. Should be added on x11 (couldn't get a working result).

Here a code to try quickly (just add a texturerect in child) :

if DisplayServer.clipboard_has():
	print("clipboard txt : ",DisplayServer.clipboard_get())
if DisplayServer.clipboard_has_image():
	print("clipboard img : ",DisplayServer.clipboard_get_image().get_data())
	$TextureRect.texture=ImageTexture.create_from_image(DisplayServer.clipboard_get_image())

Also should be nice to rename clipboard_has() to clipboard_has_text() and clipboard_get() to clipboard_get_text(). Or make Variant as result of clipboard_get() and clipboard_has() should add an optional param to indicate what type it should check.

Production edit: Closes godotengine/godot-proposals#2949.

@akien-mga
Copy link
Member

I think that's pretty useful to support. We'll indeed need a X11 implementation too (could be in a follow-up PR if someone else wants to work on it), and assess other platforms (Android, iOS, Web).

Also should be nice to rename clipboard_has() to clipboard_has_text() and clipboard_get() to clipboard_get_text().

I agree, this should be done if this PR is accepted. That would also solve the awkwardness of clipboard_has() and clipboard_get() Yoda-speak, because then it would be more obvious that all those methods are clipboard_<method>.

There should also be support for clipboard_set_image() IMO, and clipboard_set() should be clipboard_set_text().
clipboard_set_image() could be quite convenient to let users grab a screenshot for easy sharing on social media / Discord, etc.

@deakcor
Copy link
Contributor Author

deakcor commented Aug 2, 2022

I began to replace "clipboard" methods with "text" at the end. And I saw clipboard_set_primary and clipboard_get_primary methods, it duplicates a bit all the methods only for x11 system. I don't know if it exists for the images the primary ? But if yes it can become a bit messy.

Shouldn't be easier to add this in clipboard_set_text method ? Like clipboard_set_text(text,primary=false). So that way you can do also clipboard_has_text(primary=false) without new methods.

@deakcor deakcor requested review from a team as code owners August 2, 2022 20:57
@deakcor deakcor requested a review from a team as a code owner August 3, 2022 07:46
@m4gr3d
Copy link
Contributor

m4gr3d commented Aug 15, 2022

I think that's pretty useful to support. We'll indeed need a X11 implementation too (could be in a follow-up PR if someone else wants to work on it), and assess other platforms (Android, iOS, Web).

Android provides the ability to copy/paste complex data structure, so this can be added as well as a follow-up PR once the initial one has landed.

@fritol
Copy link

fritol commented Feb 4, 2023

wen in the in the main?

@YuriSizov YuriSizov modified the milestones: 4.0, 4.x Feb 4, 2023
@Calinou
Copy link
Member

Calinou commented Feb 4, 2023

wen in the in the main?

4.0 is in feature freeze, so this will have to wait for a future 4.x release to be merged. Also, this pull request needs to be rebased against the latest master branch first.

radzo73 added a commit to radzo73/godot that referenced this pull request Apr 30, 2023
@Steveplays28
Copy link

Is there anything I could help with to get this merged?
Would be a really nice feature for making apps.

@Calinou
Copy link
Member

Calinou commented Jun 26, 2023

Is there anything I could help with to get this merged? Would be a really nice feature for making apps.

4.1 is currently in feature freeze, but you can test this PR locally and make sure it works as expected. We also recommend creating a publicly accessible testing project (cc @deakcor if they still have a copy of the project). That said, this PR needs to be rebased against the latest master branch before it can be merged in a future 4.x release.

I began to replace "clipboard" methods with "text" at the end. And I saw clipboard_set_primary and clipboard_get_primary methods, it duplicates a bit all the methods only for x11 system. I don't know if it exists for the images the primary ? But if yes it can become a bit messy.

Note that this particular part of the PR needs to be reverted, as we can't break compatibility anymore now that 4.0 is released.

@deakcor
Copy link
Contributor Author

deakcor commented Jul 10, 2023

Is there anything I could help with to get this merged? Would be a really nice feature for making apps.

4.1 is currently in feature freeze, but you can test this PR locally and make sure it works as expected. We also recommend creating a publicly accessible testing project (cc @deakcor if they still have a copy of the project). That said, this PR needs to be rebased against the latest master branch before it can be merged in a future 4.x release.

I began to replace "clipboard" methods with "text" at the end. And I saw clipboard_set_primary and clipboard_get_primary methods, it duplicates a bit all the methods only for x11 system. I don't know if it exists for the images the primary ? But if yes it can become a bit messy.

Note that this particular part of the PR needs to be reverted, as we can't break compatibility anymore now that 4.0 is released.

So I rebased and reverted the part with "_text" behind clipboard methods. If you validate the changes, let me know and I'll squash the commits into one

@akien-mga
Copy link
Member

Looks good to me overall. The classref needs to be updated, see the diff in https://github.com/godotengine/godot/actions/runs/5513089283/jobs/10050780703?pr=63826

CC @bruvzg to review Windows and macOS implementations.

platform/windows/display_server_windows.cpp Show resolved Hide resolved
platform/macos/display_server_macos.mm Outdated Show resolved Hide resolved
platform/macos/display_server_macos.mm Outdated Show resolved Hide resolved
platform/macos/display_server_macos.mm Outdated Show resolved Hide resolved
@deakcor
Copy link
Contributor Author

deakcor commented Jul 12, 2023

Strange, the checks succeeded before I squashed the commits but normally there isn't any issue. Let me know if I need to change anything.

@bruvzg
Copy link
Member

bruvzg commented Jul 12, 2023

Strange, the checks succeeded before I squashed the commits but normally there isn't any issue. Let me know if I need to change anything.

It's not related to PR and caused by #78749, I have restarted failed job.

servers/display_server.cpp Outdated Show resolved Hide resolved
platform/windows/display_server_windows.cpp Outdated Show resolved Hide resolved
platform/macos/display_server_macos.mm Show resolved Hide resolved
@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Jul 25, 2023
@YuriSizov YuriSizov changed the title Added clipboard has/get image methods Add clipboard_has/get_image methods to DisplayServer Jul 25, 2023
@YuriSizov YuriSizov merged commit 0c2399d into godotengine:master Jul 27, 2023
@YuriSizov
Copy link
Contributor

YuriSizov commented Jul 27, 2023

Thanks! This is great! Now we just need a volunteer to implement this for Linux :)

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.

Allow getting image data from the system clipboard
9 participants