Generate image thumbnails and use thumbnails in the media gallery#1568
Generate image thumbnails and use thumbnails in the media gallery#1568lissine0 wants to merge 5 commits intomonal-im:developfrom
Conversation
| return [AnyPromise promiseWithResolverBlock:^(PMKResolver resolve) { | ||
| AnyPromise* imagePromise = nil; | ||
| if(fileInfo.isSVGImage) | ||
| imagePromise = [HelperTools renderUIImageFromSVGURL:fileInfo.fileURL]; |
There was a problem hiding this comment.
The existing Monal code often uses PMKHang when loading SVGs. What does that do and should I use it here?
There was a problem hiding this comment.
Since ObjC doesn't have async/await, I'm using PMKHang to hang the current thread until the given promise is resolved. It should obviously only be used in a thread that is allowed to hang (never in the main thread for example).
See the implementation here: https://github.com/mxcl/PromiseKit/blob/2bc44395edb4f8391902a9ff7c220471882a4d07/Sources/hang.m#L10
There was a problem hiding this comment.
Ah wait, it can even be used in the main thread, because it spins the runloop while "hanging".
There was a problem hiding this comment.
It is the ObjC pendant of the async/await implementation for promises over here: https://github.com/mxcl/PromiseKit/blob/2bc44395edb4f8391902a9ff7c220471882a4d07/Sources/Async.swift
There was a problem hiding this comment.
Okay, another note: the swift version of "hang" says in the comments:
// hang doesn't make sense on threads that aren't the main thread.
// use `.wait()` on those threads.
So the swift version and the ObjC versions seem to be slightly different.
There was a problem hiding this comment.
To answer your question now: No, you don't need to use PMKHang here. It should only be used if you need the result of the svg promise in the following synchronous code (like in the notification manager) and it is okay to hang the current thread while waiting for the svg promise (the notification manager will hang the receive queue which is okay).
And some cleanups: - Use more concise code, now that we have thumbnails for both images and videos - Use the isAudio, isVideo and isImage MLFiletransferInfo properties - Use the cacheId directly as attachment id, rather than creating a UUID from it
fb03133 to
b7b785c
Compare
| unreachable(@"Trying to generate a thumbnail for a file that is neither an image nor a video"); | ||
| } | ||
|
|
||
| +(AnyPromise*) generateThumbnailFromImageFile:(MLFiletransferInfo*) fileInfo |
There was a problem hiding this comment.
Wouldn't it be better to move this method to MLFiletransferInfo? Maybe even make it a readonly property (hanging for the result rather than returning a promise).
There was a problem hiding this comment.
Or even better: remove it and use the HelperTools method addUploadItemPreviewForItem: as stated in a comment further down.
| else if(fileInfo.isVideo) | ||
| return [HelperTools generateThumbnailFromVideoFile:fileInfo.cacheFilePath havingMimeType:fileInfo.mimeType andFileExtension:fileInfo.fileExtension]; | ||
| else | ||
| unreachable(@"Trying to generate a thumbnail for a file that is neither an image nor a video"); |
There was a problem hiding this comment.
Maybe just return a SFSymbol image (document/file image) and use this method for every filetransfer.
The HelperTools method for handling a file shared with Monal does even have some thumbnail generating code that could be reused here, so every location showing some thumbnail would show the same thumbnail. That would be the best solution.
There was a problem hiding this comment.
That would be the HelperTools method addUploadItemPreviewForItem:, see my other comments.
There was a problem hiding this comment.
I'm aware of addUploadItemPreviewForItem. I intentionally didn't use it, for future-proofing purposes:
If we use it now to generate generic thumbnails for all kinds of files, in the future we may start to generate actual thumbnails for PDFs for example. But then pre-existing PDFs would keep showing the generic thumbnails that were already generated in the past. (because we cache the thumbnails on disk)
There was a problem hiding this comment.
We could easily clear the disk cache once we add new thumbnail types (e.g. PDF previews like in your example above).
Returning only a generic icon for videos on iOS could be fixed, I guess?
| } | ||
|
|
||
| +(AnyPromise*) generateVideoThumbnailFromFile:(NSString*) file havingMimeType:(NSString*) mimeType andFileExtension:(NSString* _Nullable) fileExtension | ||
| +(AnyPromise*) generateThumbnailFromFile:(MLFiletransferInfo*) fileInfo |
There was a problem hiding this comment.
Wouldn't it be better to move this method to MLFiletransferInfo? Maybe even make it a readonly property (hanging for the result rather than returning a promise).
There was a problem hiding this comment.
I can move this to MLFiletransferInfo, but I don't see the point in making it a readonly property.
The thumbnailURL property is already readonly, with automatic updates in the UI, and disk / memory cache. Its setter calls this method (generateThumbnailFromFile) if a thumbnail wasn't generated and cached previously.
There was a problem hiding this comment.
Okay, makes sense, I guess.
|
|
||
| -(NSURL*) thumbnailURL { | ||
| if(!self.isImage && !self.isVideo) | ||
| return nil; |
There was a problem hiding this comment.
Maybe always return a thumbnail and reuse the thumbnail generating code from the HelperTools method addUploadItemPreviewForItem: (see my other comment above)?
|
Regarding the SVG library choice: sure, lets switch to a better lib. Do you know of any (should ideally be lightweight). |
|
Yeah, Giphy is just a library of gif images / (sometimes) animated images, no rendering library. |

A few notes:
We may want to consider this as the Exyte library has poor SVG support. In fact doesn't even render the recently added Monal SVG logos correctly.
The removed Giphy library isn't useful in this context, as I'd imagine it works by just sending an image link from one phone to another, and the receiving end would download the image from the Giphy server. i.e. it wouldn't handle animated images sent from a non-giphy client like Gajim.
Therefore, if we want to display animated images as animated we'll have to patch our exyteChat fork and add another library. Note that FLAnimatedImage (used in the old chatView for GIF displaying) doesn't support WebP.
(Animated image support is out of scope of this PR, as it needs to be done in the exytechat fork anyway. I just wanted to discuss it.)