feat: Add download started and download completed callbacks #530
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
amrbashir
requested changes
Apr 22, 2022
Also adds the complete and failed callbacks.
atlanticaccent
force-pushed
the
implement-download-event
branch
from
bf039b2 to
799da8e
Compare
Now has handler for download start/deny and download completed callback
This avoids the indirection of the closure builder pattern whilst still solving the lifetime issues.
9 tasks
|
What is this PR status? |
atlanticaccent
force-pushed
the
implement-download-event
branch
from
7e82e16 to
b600926
Compare
macOS implementation works, but may rely on an incorrect assumption - clicking the download link seems to return a random blob URL, so we just grab the first saved blob URL
|
@amrbashir I believe I've addressed your comments, plus cleaned some things up. Would you be able to give it another look over? |
amrbashir
requested changes
Sep 19, 2022
There was a problem hiding this comment.
Other than these two nit-picks, the Linux/Windows implementation looks good, as for macOS, I will leave it for @wusyong or @lucasfernog to approve.
amrbashir
previously approved these changes
Sep 19, 2022
amrbashir
requested changes
Sep 29, 2022
Comment on lines
+406
to
+407
| cls.add_ivar::<*mut c_void>("function"); | ||
| cls.add_ivar::<*mut c_void>("HasDownloadHandler"); |
There was a problem hiding this comment.
Can we extract these two strings into some consts so it is easier to track/change in the future? Actually any ivar used more than one time should be in a const
There was a problem hiding this comment.
There are many more ivar like this. I think we should do this in another PR.
wusyong
previously approved these changes
Oct 19, 2022
amrbashir
approved these changes
Oct 19, 2022
|
Thanks for this amazing PR @atlanticaccent ! |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What kind of change does this PR introduce?
Does this PR introduce a breaking change?
Checklist
fix: remove a typo, closes #___, #___)Other information
Windows works completely - the callbacks fire and the file downloads. The download location can even be changed through the crude API I've implemented (a
&mut Stringpassed to the download started callback). One note on changing download location paths is that Universal Naming Convention file paths do not work (they start with slashes and a question mark) - these are generated when callingcanonicalizeso an alternative solution is necessary.Linux seems to be working to some extent - both callbacks fire, however controlling where the file is saved seems to have no effect and causes the download to stall indefinitely. Not setting the download path causes bizarre behaviour where webkitgtk either claims that the download has succeeded (to where is unclear) or that the path (which is full of invalid characters) is occupied.
I attempted a macOS implementation however seem to have failed completely. I have aDownloadDelegatebeing created and set, and a couple of what I think are necessary methods implemented on theWKWebview, but nothing seems to get the actual delegate functions to fire.The macOS implementation is effectively on par with the linux implementation, in that all the callbacks now fire correctly, but setting the download location causes the entire download to fail, and not setting it does not seem to actually write out a download/file.