Add support for deferred SDL3 clipboard callbacks (and use it for images) #6254
Conversation
|
The This might not be due to not supporting the image format though. Here, I'd recommend integrating the logic I wrote in my latest commit to solve this issue. |
|
Seems to fail for BMP files on Wayland (PNG, JPEG work fine): |
If we're copying the image, we're going to allocate memory (and might do some processing to change the pixel format) -- might as well save it to the clipboard directly. |
I think I was for whatever reason thinking about returning the image in multiple different formats at the same time, but I guess it's unnecessary... |
|
That being said, it still fails for me. :( |
|
@FreezyLemon your logs are a bit concerning. |
Right, the callback can get called multiple times. SDL internally copies the BMP data on windows. |
This is completely valid, as you can paste the same image multiple times.
The pipe timeout does feel like an upstream bug, yeah. I'll try and create a repro and then report it. |
Seems to work fine on X. |
This is used for native callbacks that own the passed in managed object.
|
macOS should have full support for all clipboard data, but I'm mostly wondering which image formats are supported and compatible with other apps. Looking online, I see tiff, png and jpg mentioned. We can always put multiple formats on the clipboard 😉 diff --git a/osu.Framework/Platform/MacOS/MacOSGameHost.cs b/osu.Framework/Platform/MacOS/MacOSGameHost.cs
index 0cb6f2362..45657c24a 100644
--- a/osu.Framework/Platform/MacOS/MacOSGameHost.cs
+++ b/osu.Framework/Platform/MacOS/MacOSGameHost.cs
@@ -11,6 +11,10 @@
using osu.Framework.Input.Bindings;
using osu.Framework.Input.Handlers;
using osu.Framework.Input.Handlers.Mouse;
+using osu.Framework.Platform.SDL;
+using SixLabors.ImageSharp.Formats.Jpeg;
+using SixLabors.ImageSharp.Formats.Png;
+using SixLabors.ImageSharp.Formats.Tiff;
namespace osu.Framework.Platform.MacOS
{
@@ -35,7 +39,7 @@ public override IEnumerable<string> UserStoragePaths
}
}
- protected override Clipboard CreateClipboard() => new MacOSClipboard();
+ protected override Clipboard CreateClipboard() => new SDL3Clipboard(TiffFormat.Instance /* or PngFormat, JpegFormat */);
protected override ReadableKeyCombinationProvider CreateReadableKeyCombinationProvider() => new MacOSReadableKeyCombinationProvider();
|
| [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvCdecl) })] | ||
| private static unsafe IntPtr dataCallback(IntPtr userdata, byte* mimeType, UIntPtr* length) | ||
| { | ||
| var objectHandle = new ObjectHandle<ClipboardCallbackContext>(userdata, true); |
There was a problem hiding this comment.
We're passing true here which reads as if we should be cleaning this up, however that's not being done and appears incorrect to do in this callback. Should this be changed to false to prevent incorrect use?
| var objectHandle = new ObjectHandle<ClipboardCallbackContext>(userdata, true); | ||
|
|
||
| if (objectHandle.GetTarget(out var context)) | ||
| { | ||
| context.Dispose(); | ||
| objectHandle.Dispose(); | ||
| } |
There was a problem hiding this comment.
Any reason to not do using var objectHandle = ... here and above (with the appropriate false parameter)? Simply based on the principle of ensuring all IDisposable objects are disposed.
I suppose there should be no case where GetTarget() fails to resolve but the GCHandle is still allocated, though it strikes me as a bit odd to manually handle disposal like this.
| public void EnsureDataValid() | ||
| { | ||
| if (dataProvider == null) | ||
| { | ||
| Debug.Assert(Address != IntPtr.Zero); | ||
| Debug.Assert(DataLength != 0); | ||
| return; | ||
| } | ||
|
|
||
| var data = dataProvider(); | ||
| dataProvider = null!; | ||
| DataLength = (UIntPtr)data.Length; | ||
| memoryHandle = data.Pin(); | ||
| } |
There was a problem hiding this comment.
If this is pinned once and nulled afterwards, what's the purpose of this existing as a method as opposed to being done directly in the ctor and avoiding having to call this method/do the assertions?
There was a problem hiding this comment.
The idea is to make use of deferred rendering of clipboard content. If no application requests the clipboard data, this code never runs and we don't need to serialise it to bytes.
This isn't of much use in this PR as the image is immediately serialised.
| /// <summary> | ||
| /// Length of the <see cref="ReadOnlyMemory{T}"/> returned by the <see cref="dataProvider"/>. | ||
| /// </summary> | ||
| public UIntPtr DataLength; |
There was a problem hiding this comment.
How about { private set; }?
|
I've also tested macOS with |
|
On both Wayland and macOS, if I have something already in the clipboard,
Looks like an SDL issue. |
Definitely an SDL issue as |
Improvements on previous PR
SDL_HasClipboardDatais checked before getting the dataHow it works
All the required data for the clipboard callback is saved to a
ClipboardCallbackContextobject. AnObjectHandleis allocated for that object to keep it alive when it's owned by SDL (ownership is transferred upon successfully callingSDL_SetClipboardData).When another application (or we) request clipboard data, the clipboard content is generated and the resulting
ReadOnlyMemory<byte>is pinned (so the GC doesn't move it) and its pointer is passed to SDL.When new clipboard data is set, our cleanup callback gets called so we unpin the
ReadOnlyMemoryand free theObjectHandle.Tested on
WindowsClipbard)MacOSClipboard)Please make sure that
TestSceneClipboardpasses and that copying and pasting images to/from external apps also works.iOS and Android are not supported (clipboard text only)