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 image data get/set methods #79561

Closed
wants to merge 19 commits into from

Conversation

radzo73
Copy link
Contributor

@radzo73 radzo73 commented Jul 16, 2023

Closes godotengine/godot-proposals#2949.

Explanation of implementations:

  • Windows (get): First checks if there is a PNG in the clipboard, then if there is a DIBv5 in the clipboard, and then if there is a DIB in the clipboard.
  • Windows (set): Loads as a PNG to the clipboard, and as a DIBv5.
  • Mac (get): Converts to a PNG and returns it.
  • Mac (set): Loads the Image (thank you _convert_to_nsimg(), very cool) to the clipboard.
  • iOS (get): Converts to a PNG and returns it.
  • iOS (set): N/A (wish you were here, _convert_to_nsimg())

Unsupported (because I don't know how to):

  • Web
  • Android
  • X11
  • Wayland (cc @Riteo how's it going)
  • UWP

radzo73 and others added 10 commits April 29, 2023 20:52
boy am I glad I test things
perhaps they work?
I trust that at bruvzg knows better than me

Co-authored-by: bruvzg <7645683+bruvzg@users.noreply.github.com>
Co-authored-by: bruvzg <7645683+bruvzg@users.noreply.github.com>
Co-authored-by: bruvzg <7645683+bruvzg@users.noreply.github.com>
Gee golly I hope this works
@radzo73 radzo73 requested review from a team as code owners July 16, 2023 21:01
@KoBeWi
Copy link
Member

KoBeWi commented Jul 16, 2023

How is this PR related to #63826? (aside from implementing set method)

@radzo73
Copy link
Contributor Author

radzo73 commented Jul 16, 2023

Main difference is that my PR has the set_clipboard_image() method.
My initial PR was #76603 (which I started because #63826 hadn't had any progress since August 2022), which didn't end out working.
(I actually had no idea @deakcor was still working on their branch)

@Riteo
Copy link
Contributor

Riteo commented Jul 17, 2023

Wayland (cc @Riteo how's it going)

Hi, it's going well! I can implement this without trouble in my PR once this or the other get merged, it should be pretty doable. Implementing it here wouldn't make sense as there's no Wayland code in the main branch yet.

(I actually had no idea @deakcor was still working on their branch)

That's a bummer. What do we do now? :/

This PR adds a set method but the other one was much more reviewed and approved... Perhaps the two could be merged together, implementing a set method there?

@deakcor
Copy link
Contributor

deakcor commented Jul 17, 2023

This PR adds a set method but the other one was much more reviewed and approved... Perhaps the two could be merged together, implementing a set method there?

Yes this PR seems an improvement of mine (#63826), there is similar base for a lot of code I think (?). All the checks passed mine and it was validated so maybe we could merge then rebase this one to get rid of duplicated code. If you have an other idea which require me to do an action let me know.

@KoBeWi
Copy link
Member

KoBeWi commented Jul 17, 2023

maybe we could merge then rebase this one to get rid of duplicated code

Makes sense.
@radzo73 You could give the other PR a review.

@YuriSizov
Copy link
Contributor

#63826 was merged. This can now be rebased.

@radzo73
Copy link
Contributor Author

radzo73 commented Jul 27, 2023

#63826 got merged, which means it's time for me to rebase
gonna re-add the set methods
Also, what are the thoughts on merging String clipboard_get_text() and Image clipboard_get_image() into Variant clipboard_get()? (and, likewise, the setmethods)

@YuriSizov
Copy link
Contributor

Also, what are the thoughts on merging String clipboard_get_text() and Image clipboard_get_image() into Variant clipboard_get()? (and, likewise, the setmethods)

I don't think merging them would be extremely useful. If there is a demand, we can add a generic method in addition to these more specific ones, but there is no good reason to replace them.

I never know what I'm doing
plus updating other files to use new methods, and xml tweaks
also iOS
@Zireael07
Copy link
Contributor

Is this merged or not? The site seems to be confused...

@KoBeWi
Copy link
Member

KoBeWi commented Jul 29, 2023

Another similar PR was merged.

@lostminds
Copy link

Also, what are the thoughts on merging String clipboard_get_text() and Image clipboard_get_image() into Variant clipboard_get()? (and, likewise, the setmethods)

@radzo73 If you're interested in further improving pasteboard support in Godot perhaps you have some thoughts on this proposal godotengine/godot-proposals#7899 . In short basically proposing to instead move the access of clipboard data into a new separate global Clipboard object, to enable more general access of multiple parallell data types on the clipboard. Basically to have it work more like macOS NSPasteboard and Windows system Clipboard class.

On a more specific note regarding the macOS implementation, I think it would be worth moving away from the specific data formats specified now like [pasteboard availableTypeFromArray:[NSArray arrayWithObjects:NSPasteboardTypeTIFF, NSPasteboardTypePNG, nil]]; etc. And instead use the NSImage method [NSImage canInitWithPasteboard:[NSPasteboard generalPasteboard]] to check if it's possible to create an NSImage from whatever data or formats are available on clipboard and then create it with nsImage = [[NSImage alloc] initWithPasteboard:[NSPasteboard generalPasteboard]].

This would mean it'll support all image formats the system can handle. However it also means the NSImage might not be a bitmap format, so it might not have a NSBitmapImageRep. However, all NSImages can be converted to tiff bitmap data using tiffData = [nsImage TIFFRepresentation] (sadly not png directly), and then you can use that to create a NSBitmapImageRep with nsbitmapImageRep = [NSBitmapImageRep imageRepWithData: tiffData]. And then finally get the png data from that just like the current implementation using [nsbitmapImageRep representationUsingType:NSPNGFileType]

For setting image data to the pasteboard on macOS (and iOS) you shouldn't need to do a conversion to an NSImage (as the _convert_to_nsimg above implies). Just using NSPasteboard setData:data forType:NSPasteboardTypePNG or the corresponding UIPasteboard setData:forPasteboardType: method for iOS should let you set the raw PNG data directly to the pasteboard. This is assuming you can get the png data you can get from Godots Image.save_png_to_buffer into an NSData object, but that might be done somewhere else in the codebase.

@radzo73
Copy link
Contributor Author

radzo73 commented Oct 17, 2023

Closing this in favor of a new PR inspired by godotengine/godot-proposals#7899 whenever I get around to working on it.

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