-
Notifications
You must be signed in to change notification settings - Fork 3
Native Inserter: Add initial Photos integration #203
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
Conversation
| } | ||
|
|
||
| func webView(_ webView: WKWebView, stop urlSchemeTask: WKURLSchemeTask) { | ||
| // Nothing to do here for simple file serving |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably want to store the task as a member of this class and cancel it from this method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a nearly-instant operation. It will never be cancelled in flight in practice.
| try? fileManager.createDirectory(at: self.rootURL, withIntermediateDirectories: true) | ||
| try? fileManager.createDirectory(at: self.uploadsDirectory, withIntermediateDirectories: true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these silently fail saveMediaData won't work right.
Probably best to just run these on every call to saveMediaData if needed – makes the error handling way more straightforward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea; updated.
4fbade0 to
611ac1f
Compare
| for scheme in CachedAssetSchemeHandler.supportedURLSchemes { | ||
| config.setURLSchemeHandler(schemeHandler, forURLScheme: scheme) | ||
| } | ||
| config.setURLSchemeHandler(MediaFileSchemeHandler(), forURLScheme: MediaFileSchemeHandler.scheme) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same approach we use for the assets. It's currently the only way to let WKWebView access the files on disk, but I hope we can simplify it in the future by extracting the bundled editor on disk and pointing WKWebView to that directly and not the readonly bundle.
|
|
||
| guard let data = try? JSONEncoder().encode(selection), | ||
| let string = String(data: data, encoding: .utf8) else { | ||
| debugPrint("Failed to serialize media array to JSON") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should never happen.
| onClick={ ( e ) => { | ||
| e.preventDefault(); | ||
|
|
||
| const blocks = serializeBlocksForNative( inserterItems ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the fix for CMM-903: Performance: Inserter recomputes the list of blocks on every change in the editor. Instead of useEffect, it now serializes blocks only when it's needed.
Ideally, I would also ensure useInsertionPoint isn't called so frequently, but it could be done later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. We can revisit other memoization ideas later. Maybe we can further reduce useInsertionPoint invocations.
| ); | ||
|
|
||
| if ( ! transformation ) { | ||
| debug( 'insertMedia: No matching transform found', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I focused only on the happy path and opened CMM-910: Add error handling for media insertion
611ac1f to
fd21214
Compare
fd21214 to
78df467
Compare
0ccfd38 to
728c04b
Compare
728c04b to
36b9906
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Selecting media feels very satisfying. Nicely done.
| onClick={ ( e ) => { | ||
| e.preventDefault(); | ||
|
|
||
| const blocks = serializeBlocksForNative( inserterItems ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. We can revisit other memoization ideas later. Maybe we can further reduce useInsertionPoint invocations.
What?
How?
I used onFilesDrop from use-on-block-drop component as a reference. It uses "transforms" to create blocks and then uses the existing insertion logic introduced in #199 to insert the blocks created by transforms.
This implementation should make sure that we support any media types, any transforms including third-party block ones, and any insertion points including nested blocks.
Testing Instructions
core/imageblock is added and the item is uploaded to the remoteNote: I focused on making the implementation simple and sound and plan to test it and fix any outstanding issues in future iterations.
Screenshots or screencast
Simulator.Screen.Recording.-.iPhone.17.Pro.-.2025-10-28.at.15.48.44.mov
Note: the first frame in the random video I uploaded is black, which is why there is no preview