blender/blender
172
532
Fork 807
Code Issues 6.5k Pull Requests 973 Projects 21 Wiki Activity

macOS/QuickLook: support rich thumbnail in Finder #107072

Open
Ankit Meel wants to merge 4 commits from ankitm/blender:ankitm/2ql into main
pull from: ankitm/blender:ankitm/2ql
merge into: blender:main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Conversation 43 Commits 4 Files Changed 7 +31 -33
3 changed files with 31 additions and 33 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 1b47e663e0 - Show all commits

0
release/darwin/Blender.app/Contents/Plugins/blender-thumbnailer.appex/Contents/Info.plist → release/darwin/Blender.app/Contents/PlugIns/blender-thumbnailer.appex/Contents/Info.plist
View File

2
source/blender/blendthumb/src/thumbnail_provider.h
View File

@ -1,8 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0-or-later */
#pragma once
#include <Foundation/Foundation.h>
#import <QuickLookThumbnailing/QuickLookThumbnailing.h>
ankitm marked this conversation as resolved
Jonas Holzman commented 2024-10-11 19:45:13 +02:00
Review
Copy Link

Similarly to the .mm file, #import should be used here

Similarly to the `.mm` file, `#import` should be used here
NS_ASSUME_NONNULL_BEGIN

62
source/blender/blendthumb/src/thumbnail_provider.mm
View File

@ -1,9 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0-or-later */
#include <AppKit/NSImage.h>
#include <CoreGraphics/CGDataProvider.h>
#include <CoreGraphics/CoreGraphics.h>
#include <Foundation/Foundation.h>
#import <AppKit/NSImage.h>
ankitm marked this conversation as resolved
Jonas Holzman commented 2024-10-11 19:39:31 +02:00
Review
Copy Link

CoreGraphics headers can be removed since they're not used anymore, also Objective-C headers should use #import instead of #include for correctness

CoreGraphics headers can be removed since they're not used anymore, also Objective-C headers should use `#import` instead of `#include` for correctness
#include "BLI_fileops.h"
#include "BLI_filereader.h"
@ -110,36 +107,38 @@ static NSError *create_nserror_from_string(NSString *errorStr)
static NSImage *generate_nsimage_for_file(const char *src_blend_path, NSError **error)
{
/* Open source file `src_blend`. */
FileDescriptorRAII src_file_fd = FileDescriptorRAII(src_blend_path);
if (!src_file_fd.good()) {
*error = create_nserror_from_string(@"Failed to open blend");
return nil;
}
@autoreleasepool {
/* Open source file `src_blend`. */
FileDescriptorRAII src_file_fd = FileDescriptorRAII(src_blend_path);
if (!src_file_fd.good()) {
Brainzman marked this conversation as resolved Outdated
Jonas Holzman commented 2024-10-11 19:42:29 +02:00
Outdated
Review
Copy Link

For safety, this entire function should be wrapped in an @autoreleasepool since we're calling Cocoa code which might call autorelease internally

For safety, this entire function should be wrapped in an `@autoreleasepool` since we're calling Cocoa code which might call `autorelease` internally
Ankit Meel commented 2024-10-11 20:17:03 +02:00
Outdated
Review
Copy Link

I thought the external pool would take effect.
removed now

I thought the external pool would take effect. removed now
Jonas Holzman commented 2024-10-12 12:25:07 +02:00
Outdated
Review
Copy Link

The external pool does take effect yeah, but since we do not have a global pool to fallback to like in a normal Obj-C app, it's safer and simpler to wrap all Cocoa code in pools to prevent leaks later down the line when refactoring.
It also leads to way easier maintenance since you do not have to worry about figuring out which function is/isn't wrapped in a pool, and lets you safely call any Obj-C function from C/C++.

The external pool does take effect yeah, but since we do not have a global pool to fallback to like in a normal Obj-C app, it's safer and simpler to wrap all Cocoa code in pools to prevent leaks later down the line when refactoring. It also leads to way easier maintenance since you do not have to worry about figuring out which function is/isn't wrapped in a pool, and lets you safely call any Obj-C function from C/C++.
👍 1
*error = create_nserror_from_string(@"Failed to open blend");
return nil;
}
FileReader *file_content = BLI_filereader_new_file(src_file_fd.get());
if (file_content == nullptr) {
*error = create_nserror_from_string(@"Failed to read from blend");
return nil;
}
FileReader *file_content = BLI_filereader_new_file(src_file_fd.get());
if (file_content == nullptr) {
*error = create_nserror_from_string(@"Failed to read from blend");
return nil;
}
/* Extract thumbnail from file. */
Thumbnail thumb;
eThumbStatus err = blendthumb_create_thumb_from_file(file_content, &thumb);
if (err != BT_OK) {
*error = create_nserror_from_string(@"Failed to create thumbnail from file");
return nil;
}
/* Extract thumbnail from file. */
Thumbnail thumb;
eThumbStatus err = blendthumb_create_thumb_from_file(file_content, &thumb);
if (err != BT_OK) {
*error = create_nserror_from_string(@"Failed to create thumbnail from file");
ankitm marked this conversation as resolved Outdated
Jonas Holzman commented 2024-10-02 23:15:29 +02:00
Outdated
Review
Copy Link

This can be heavily simplified by using the [NSimage initWithData] method, removing the need for CoreGraphics altogether. In which case, the Cocoa calling code should be wrapped in an @autoreleasepool block.

This can be heavily simplified by using the `[NSimage initWithData]` method, removing the need for CoreGraphics altogether. In which case, the Cocoa calling code should be wrapped in an `@autoreleasepool` block.
return nil;
}
std::optional<blender::Vector<uint8_t>> png_buf_opt = blendthumb_create_png_data_from_thumb(
&thumb);
if (!png_buf_opt) {
*error = create_nserror_from_string(@"Failed to create png data from thumbnail");
return nil;
std::optional<blender::Vector<uint8_t>> png_buf_opt = blendthumb_create_png_data_from_thumb(
&thumb);
if (!png_buf_opt) {
*error = create_nserror_from_string(@"Failed to create png data from thumbnail");
return nil;
}
NSData *ns_data = [NSData dataWithBytes:png_buf_opt->data() length:png_buf_opt->size()];
NSImage *ns_image = [[NSImage alloc] initWithData:ns_data];
return ns_image;
}
NSData *ns_data = [NSData dataWithBytes:png_buf_opt->data() length:png_buf_opt->size()];
NSImage *ns_image = [[NSImage alloc] initWithData:ns_data];
return ns_image;
}
@implementation ThumbnailProvider
@ -152,7 +151,8 @@ static NSImage *generate_nsimage_for_file(const char *src_blend_path, NSError **
NSLog(@"Generating thumbnail for %@", request.fileURL.path);
@autoreleasepool {
Brainzman marked this conversation as resolved
Jonas Holzman commented 2024-10-11 19:44:38 +02:00
Review
Copy Link

Do we want to keep these logging statements or are they just for debugging purposes?

Do we want to keep these logging statements or are they just for debugging purposes?
Ankit Meel commented 2024-10-11 20:17:18 +02:00
Review
Copy Link

This is the only output generated by the tool visible in console.app and other reviewers signed off on it..
i doubt there's any downside to it

This is the only output generated by the tool visible in console.app and other reviewers signed off on it.. i doubt there's any downside to it
Jonas Holzman commented 2024-10-12 12:11:51 +02:00
Review
Copy Link

Alrighty makes sense

Alrighty makes sense
NSError *error = nil;
NSImage *ns_image = generate_nsimage_for_file(request.fileURL.path.UTF8String, &error);
NSImage *ns_image = generate_nsimage_for_file(request.fileURL.path.fileSystemRepresentation,
&error);
if (ns_image == nil) {
handler(nil, error);
return;