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 Extensible Clipboard Data Loading #86021

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Setadokalo
Copy link
Contributor

@Setadokalo Setadokalo commented Dec 11, 2023

(Note: This is very freshly written, so I haven't even implemented it on Windows yet.)

This mostly addresses godotengine/godot-proposals#7899 by adding several functions to DisplayServer:

  • clipboard_has_type, implemented per-platform, to test if a given type is available on the clipboard.
  • clipboard_add_type_handler and clipboard_remove_type_handler, for adding and removing type handlers (Callables which map the raw data to the correct type) from the internal list.
  • clipboard_get_raw_type, implemented per-platform, which is not exposed outside the engine and simply produces the raw bytes of a type from the clipboard in a PackedByteArray.
  • clipboard_get_type, implemented for all platforms, which takes the raw data from clipboard_get_raw_type and converts it using the appropriate Callable from the handler map to produce the correct type.

Something not explicit in the code is that p_type, the type name being requested by the user, is a MIME type, which should be directly supported on Linux and (as far as I can tell) Windows, and which should be convertable to the correct representation on Mac.

I still want to implement this for windows, and I don't know if the API is really what we'd want to expose for this, but I'm making this draft PR to let anyone who'd like to leave feedback on it.

@lostminds
Copy link

Nice to see you're working on this, some comments:

Perhaps a bit of a nitpick, but for me the naming get_type/get_raw_type feels wrong as it's not returning a type, but rather data or resources. If you feel clipboard_get_data_for_type is too long I'd propose getting rid of type instead, as in clipboard_get_data since it's the data we're getting and type is already a required variable to pass. Or perhaps clipboard_get_object for the parsed and processed resources and clipboard_get_raw_data for the raw data?

Something not explicit in the code is that p_type, the type name being requested by the user, is a MIME type

Regarding this there's also the case of custom types, one of the more important usecases for expanding the clipboard functionality in my mind. In other words getting and setting data to the clipboard in propriety formats used only in your own application. So for cases where the type isn't a MIME type that can be translated to something appropriate for the local platform the type string should be used as is.

And finally, regarding the image specific versions like clipboard_has_image() and clipboard_get_image() it looks like you've started implementing this as just checking for image/png data. I'm not sure about linux, but both Windows Clipboard and macOS NSPasteboard have dedicated functions for checking if there is image data on the clipboard and returning this image regardless of image format. Using these system features we can ensure that we can get access to any image on the pasteboard that the system supports, regardless of format.

@Setadokalo
Copy link
Contributor Author

I agree the naming of the functions is a bit awkward, I was aiming for consistency with the existing functions without being overly verbose. I don't love clipboard_get_object, since the data loaded can be of non-object type (i.e. String, Dictionary, int, etc). I'm also just not sure this is the right design - it feels inconsistent for the engine to register callbacks to handle clipboard parsing in this way. Maybe it might be better to use an API more like that of ResourceFormatLoader? Or is that overkill? I'm not sure.

@lostminds
Copy link

Regarding naming, perhaps the current clipboard_get/set/has() could just be used to get a shorter version, but with a new optional p_type variable to pass. If this is optional and defaults to a clipboard string type it would retain compatibility with the current clipboard_get/set/has() implementations for strings currently on the pasteboard? Only the return type of clipboard_get and first clipboard_set parameter would now be a Variant instead of String, which shouldn't break compatibility I think?

I also agree that the clipboard_add_type_handler seems a little inconsistent. I figured this was a design you liked, but to me it would be ok to just have the clipboard_get (or whatever they'll be called) return the data directly and skip the handlers. Either return raw as a PackedByteArray for further processing, or maybe with hints in clipboard_get for how the data should be parsed to basic types like String or Dictionary returned instead (se examples below).

To me I think the most important thing is to allow getting and setting custom and multiple specified types of data on the clipboard instead of the current just getting and setting single standard bitmap image or text string.

For example if I copy a graphical element in the graphics app I'm working on I'd like to place a bitmap image, an svg vector representation and my own custom format data structure all on the pasteboard. The two first for exporting to other apps that might prefer a bitmap images or vector svg graphics, and the third custom format data for internal use if the user pastes it in somewhere in my own app. Similarly when pasting something into the app I'd like to be able to check if there is data of different such supported types on the clipboard to determine which one should be used and pasted in.

So for these types of situations I imagined there would be need for something like this to access this data from the clipboard when pasting:

if DisplayServer.clipboard_has("my_custom_format"):
	#paste my custom data
	custom_data_dictionary = DisplayServer.clipboard_get("my_custom_format", ClipboardParseType.DICTIONARY)
elif DisplayServer.clipboard_has("image/svg+xml"):
	#get and parse svg data specifically
	svg_string = DisplayServer.clipboard_get("image/svg+xml", ClipboardParseType.STRING)
elif DisplayServer.clipboard_has_image():
	#get any available image data from pasteboard, converted to a Godot Image
	image = DisplayServer.clipboard_get_image()

And to set multiple format data on the clipboard when copying:

clipboard_set_image(image) #might set multiple image data formats for compatibility
clipboard_set(svg_string,"image/svg+xml")
clipboard_set(custom_data_dictionary,"my_custom_format")

@Setadokalo
Copy link
Contributor Author

I think removing the type handler callback system entirely and just providing engine methods for text and images solves the common use cases, and then an additional method to get arbitrary types as a PackedByteArray should allow users with less common uses to solve their issue. I think that design should adhere better to the Godot development philosophy, so I'll make the change soon.

@lostminds
Copy link

@Setadokalo any progress on this? Anything I can do to help? I've got a use case now in the project I'm working on where I'm copying graphical layer and I'd like to put both a string SVG representation and my own internal data format on the clipboard at the same time, and conversely also then examine the clipboard and see if there is data there in my internal format when pasting in.

Currently this is very tricky and doesn't really work as expected in all cases as I can only actually put strings on the clipboard with the current clipboard functionality and have to rely on my own faked internal clipboard for the custom data. So getting this functionality to put multiple data formats on the clipboard and look for and get specific custom formats would be very useful!

@Setadokalo
Copy link
Contributor Author

sorry, the thing that stopped me from progressing last time was actually a bug I noticed in the current implementation; when I get an image from the clipboard and it triggers the multipart mechanism, it also for some reason makes the window unable to receive focus anymore. I've meant to try to track that bug down, but haven't had the time.

@lostminds
Copy link

I see, I assume this input issue is on Windows, where the Clipboard seems more closely tied to windows? But still it seems unexpected that getting data from the clipboard should affect input. Could it be related to an unbalanced OpenClipboad / CloseClipboard call set (as described here)?

And also, does getting an image need to trigger a "multipart mechanism" of some kind? On Windows there seems to be a dedicated Clipboard.GetImage method that will allow getting an image from the Clipboard without needing to know what format it is in. On macOS there is a similar method via NSImage initWithPasteboard that can be used to create an NSImage (that we can then turn into PNG data for example) from the clipboard, without needing to know what formats images are stored on the clipboard.

Is the code updated with this latest version in this pr draft? If you update that I could take a look at the code there and see if I can find anything that looks like it could cause issues.

@Setadokalo
Copy link
Contributor Author

it's actually a linux issue - and increment transfer is chosen by the owner of the clipboard data, not the requesting client. I'm not sure if it's a godot, xwayland, or kde bug at this point - I haven't had the time and motivation to investigate it enough.

@lostminds
Copy link

actually a linux issue - and increment transfer is chosen by the owner of the clipboard data

I see, that's unfortunate. But that sounds like it wouldn't be a new issue or specific to this new extended multi-format clipboard support. But rather something more general with linux clipboard access in Godot? Does the current DisplayServer.clipboard_get_image() implementation suffer from the same issue? If not perhaps there is a solution in how that is implemented.

@Cykyrios
Copy link
Contributor

Cykyrios commented Oct 11, 2024

I don't know how helpful it can be, but since SDL 3 now has support for mime types in its clipboard implementation, maybe you could have a look there for inspiration? https://wiki.libsdl.org/SDL3/SDL_SetClipboardData
It would definitely be nice to see support for pasting images into the clipboard.

Edit: Direct link to clipboard.c: https://github.com/libsdl-org/SDL/blob/main/src/video/SDL_clipboard.c

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.

4 participants