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

[v2/windows] implement full file path resolver #2774

Closed

Conversation

ayatkyo
Copy link

@ayatkyo ayatkyo commented Jul 16, 2023

Description

This PR add a feature for resolving the full path of js File.

Currently only tested in Windows 11 with WebView2 114.0.1823.79.

Fixes #1090 but only for Windows with WebView2 version >= 1.0.1774.30.

Usage Demo

Source code of this demo is on https://github.com/ayatkyo/wails-2-file-drop-test

2023-07-14_04-19-27.mp4

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Another Solution

The previous solution that I tried before using this is using an overlay view in front of the webview, so it is something like this:

  • Create a hidden Overlay view in front of the webview
  • In the JS side, listen to dragenter event then get the target element size and position using event.target.getBoundingClientRect()
  • Update Overlay size and position using that information
  • Show Overlay and handle file drop event
  • Send dropped file list to js side

But this solution will trigger dragleave to target the element on the JS side as soon as we show the Overlay view, but we can trigger dragover programmatically to webview with the mouse position (I have not tried this).

Checklist:

  • I have updated website/src/pages/changelog.mdx with details of this PR
  • My code follows the general coding style of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@ayatkyo ayatkyo marked this pull request as ready for review July 16, 2023 10:37
@leaanthony
Copy link
Member

I think we need to think about the scenario where the webview runtime doesn't support this feature. Any thoughts on how the developer would manage that?

return
}

file := *(**edge.ICoreWebView2File)(unsafe.Pointer(&_file))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to release this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no explanation about it in the docs. But I think it is better to release the resource to avoid memory leaks, or maybe it will be cleared by the garbage collector? honestly, I don't know much about Golang memory 😅.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid a memory leak we need to call Release here, because whenever COM returns a pointer to a COM Object the reference counter is increased. There's no special mentioning on the WebView2 docs, but it's a coding guideline when using COM on which all the WebView2 APIs are based. Managing the Lifetime of an COM-Object

Garbage Collector of Go can't do anything here, because the memory has been allocated in native COM code of which Go isn't aware of.

@ayatkyo
Copy link
Author

ayatkyo commented Jul 19, 2023

I think we need to think about the scenario where the webview runtime doesn't support this feature. Any thoughts on how the developer would manage that?

Maybe we can make a function to check if this feature is available with CanResolveFilePaths, so the developer can handle other case based on that value.

Because another solution is using a different way to get the file path, for example when using overlay as a drop target, we cannot use the same ResolveFilePaths function that accepts the file list, but we need to listen for file-drop event in js side.

Maybe it will be something like this:

  • If CanResolveFilePaths = true, handle file drop on the target element using ResolveFilePaths
  • Else, listen wails-file-drop event on the target element.

As far as I know, right now only WebView2 provides an API to resolve the js File path on the native side. WebView2 on another platform is on their Roadmap.

Copy link
Collaborator

@stffabi stffabi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome job @ayatkyo, thanks so much for all your time you have put into this 🙏

I've added some comments about COM memory management and pointer conversions.

return
}

file := *(**edge.ICoreWebView2File)(unsafe.Pointer(&_file))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid a memory leak we need to call Release here, because whenever COM returns a pointer to a COM Object the reference counter is increased. There's no special mentioning on the WebView2 docs, but it's a coding guideline when using COM on which all the WebView2 APIs are based. Managing the Lifetime of an COM-Object

Garbage Collector of Go can't do anything here, because the memory has been allocated in native COM code of which Go isn't aware of.

}

files := []File{}
count := *(*uint32)(unsafe.Pointer(&_count))
Copy link
Collaborator

@stffabi stffabi Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conversion seems somehow strange to me. According to the signature of GetCount() (*uint32, error) { _count is a *uint32, so we convert **uint32 to a *uint32 and dereference that.

I would suggest changing the signature of GetCount to return a uint32 instead of *uint32. AFAIK we never return pointers to primitive types in go-webview2

v2/internal/frontend/desktop/windows/frontend.go Outdated Show resolved Hide resolved
@ayatkyo
Copy link
Author

ayatkyo commented Jul 21, 2023

Awesome job @ayatkyo, thanks so much for all your time you have put into this 🙏

I've added some comments about COM memory management and pointer conversions.

Sorry for the late reply.

Thanks for the comments, I learn a lot from that, especially the pointer, I will update the code 🙏

If I not mistaken, we will need to update ICoreWebView2File.go to be something like this:

// ...
func (i *ICoreWebView2File) Release() error {
  return i.vtbl.CallRelease(unsafe.Pointer(i))
}
// ...

Then call the Release function using defer

file := (*edge.ICoreWebView2File)(unsafe.Pointer(_file))
defer file.Release()

Also for ICoreWebView2ObjectCollectionView.go

// ...
func (i *ICoreWebView2ObjectCollectionView) Release() error {
  return i.vtbl.CallRelease(unsafe.Pointer(i))
}
// ...
objs, err := args.GetAdditionalObjects()
defer objs.Release()
if err != nil {
  f.logger.Error(err.Error())
  return
}

@leaanthony
Copy link
Member

My suggestions for releasing would be:

  • Create a defer function for each item you know is allocated (not before the error condition
  • Pass in the thing you want to release as a parameter to that method:
defer func(itemToRelease *item) { 
   //release the item 
}(item)

I think that should work as it copies the reference.

@ayatkyo
Copy link
Author

ayatkyo commented Jul 24, 2023

I have updated the code, thanks a lot for guiding me on this PR @leaanthony @stffabi, I really appreciate that.

Copy link
Collaborator

@stffabi stffabi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for all your time you have put into this.

I suspect we also need to update go-webview2 with the changed method signature for GetCount. But this part looks good to me.

@ayatkyo
Copy link
Author

ayatkyo commented Jul 29, 2023

Thank you for your response. You are right. I have made some changes to the code and created a new PR for go-webview2.

The previous code caused the application to crash when using WebView2 SDK versions < 1.0.1774.30 due to the unavailability of GetAdditionalObjects.

@leaanthony
Copy link
Member

I've commented on the go-webview PR so I think we're pretty close to getting it in. One question: Am I right in thinking that if a webview runtime is installed which does not support GetAdditionalObjects,that means DnD is unavailable by default?

@ayatkyo
Copy link
Author

ayatkyo commented Aug 6, 2023

Thanks for the reply, let's move the discussion to go-webview PR, sorry for the late reply.

Am I right in thinking that if a webview runtime is installed which does not support GetAdditionalObjects,that means DnD is unavailable by default?

Yes, to get full file path, frontend needs to send the File object using postMessageWithAdditionalObjects then on the native side, we use GetAdditionalObjects alongside GetValueAtIndex to get the file object.

Unfortunately, we cannot simply just use window.chrome.webview.postMessageWithAdditionalObjects to check if this feature is supported, because I have checked that function is also available on WebView2 1.0.1722.45 which does not have GetAdditionalObjects.

@leaanthony
Copy link
Member

@ayatkyo - Do you still want to proceed with this PR? I think go-webview2 is ready for this now.

@leaanthony leaanthony added the awaiting feedback More information is required from the requestor label Sep 25, 2023
@leaanthony
Copy link
Member

Closing this due to no response from the Author. If anyone wants to pick it up, feel free 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting feedback More information is required from the requestor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT]: Native drap and drop for file and folder with complete filepath
3 participants