Add share sheet support - #275
Conversation
dotMorten
left a comment
There was a problem hiding this comment.
Amazing work! This is a really cool addition, but see notes on where I think this should be put instead.
|
|
||
| // IID of DataTransferManager, passed as the riid to GetForWindow: | ||
| Guid dtmIid = new(0xa5caee9b, 0x8708, 0x49d1, 0x8d, 0x36, 0x67, 0xd2, 0x5a, 0x8d, 0xa0, 0x0c); | ||
| _dtm = WinRT.MarshalInterface<DataTransferManager>.FromAbi(interop.GetForWindow(hWnd, dtmIid)); |
There was a problem hiding this comment.
Could this be done on-the-fly in the extension method and cleaned up after instead?
There was a problem hiding this comment.
After ab23884, it is done on-the-fly, please see #275 (comment).
| } | ||
|
|
||
| dtm.DataRequested += handler; | ||
| window.ShowShareUI(); |
There was a problem hiding this comment.
Could there be a case here where the dtm/handler goes out of scope and gets garbage collected before the event fires?
There was a problem hiding this comment.
I think that won't happen because we are marshalling a COM object, and it is stable per window. Also, please correct me if I'm wrong, since we are keeping a reference of dtm in handler, it should not be recycled before the handler runs?
There was a problem hiding this comment.
Makes sense. Is there a case where the handler won't run, and thus the handler is now leaking? Or could multiple calls to this method in parallel cause the handler to trigger multiple times?
There was a problem hiding this comment.
For leaking, I think it has a risk of happening, though I'm not certain whether or how it can be triggered. The COM wrapper dtm and handlsr will be referencing each other until the event is fired for once. However, once the event is finally fired, the unregistration happens and they both get recycled. So the damage should be minimal.
As for multiple calls, I think it's safe, since each handler is designed to run only once and unregister itself.
There was a problem hiding this comment.
Actually we could probably just remove this helper function if you feel really worried. Don't feel like it is something that must be in this project.
There was a problem hiding this comment.
Would the datarequested call just not happen if the user cancels the dialog? There's also the chance that window.ShowShareUI throws and again would cause it to never unhook. I could imagine that happens with a stale/closed window instance.
There was a problem hiding this comment.
If it happens fast enough maybe? As for windows closed while there are uninvoked handlers, yeah if that happens it would be a leak without any way to recover... Might just should remove it in this case?
|
/api-diff |
API changesnamespace WinUIEx
{
public static class HwndExtensions
{
public DataTransferManager GetDataTransferManagerForWindow(nint hwnd);
public ShowShareUIForWindow(nint hwnd);
}
public static class WindowExtensions
{
public DataTransferManager GetDataTransferManager(Window window);
public Share(Window window, DataPackage data);
public ShowShareUI(Window window);
}
}
Generated with .NET Object Model Diagram Generator |
|
Looking at the public APIs added, I get the need for And should the Share method also be on HwndExtensions? |
Actually the other two APIs are the one provided by Microsoft,
It is actually how Microsoft expects us to use the API: https://learn.microsoft.com/en-us/windows/apps/develop/windows-integration/integrate-sharesheet-send#implement-share-for-desktop-apps-winui-3-wpf-winforms private void OnDataRequested(DataRequestedEventArgs args)
{
DataRequest request = args.Request;
DataPackage data = request.Data;
data.Properties.Title = "Share from my desktop app";
data.SetText("Shared content");
// For URLs:
// data.SetWebLink(new Uri("https://example.com"));
// For files:
// var item = await StorageFile.GetFileFromPathAsync(filePath);
// data.SetStorageItems(new[] { item });
}
// In your Share button handler:
private void ShareButton_Click()
{
var hWnd = WinRT.Interop.WindowNative.GetWindowHandle(this);
var interop = DataTransferManager.As<IDataTransferManagerInterop>();
interop.ShowShareUIForWindow(hWnd);
}I'm adding |
|
Thank you for the quick follow ups. I'm gonna do a little more research on this one and fully understand the share contracts. The thing that has me slightly concerned with the helper method, is the up-front creation of data that might not ever be requested if the user cancels the share UI. In the example it's a cheap call, but that could also be very heavy and large data objects, which leads me to think this should probably be more of a delegate parameter to return the data on-demand. The obvious solution is to not provide the helper, and just expose the manager, but then there really isn't much left, and I do like how simple the Share method is. So bottom line, let me think about it some more and play with it a little, and see if I can't come up with something that satisfies everything and gets the best of both worlds. I definitely like what you're adding here and appreciate all the time you put into it. I want to try and move this forward. |
No worries at all! |
|
A well-designed one-shot void Share(this Window window, DataPackage data);
void Share(this Window window, Action<DataRequestedEventArgs> onDataRequested);
void Share(this Window window, Func<DataRequest, Task> onDataRequestedAsync);The callback overload should receive |
Related Issue (required)
Closes #274
PR Type
Summary
Validation
Notes