macOS/QuickLook: support rich thumbnail #107072

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

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Member

Support thumbnail that shows the file contents instead
of the default blend file icon for all files in Finder.

blender-thumbnailer process is kept alive by the system
in the background and is used when needed.

It will NOT be interactive "Preview" that allows richer
interactions like Panning viewport, or rotating 3D objects.


This may need codesign changes in the buildbot.

Community request: https://blender.community/c/rightclickselect/d3cbbc/?sorting=hot

QLgenerator for older versions (not developed by me):
https://blenderartists.org/t/blender-quicklook-plugin/621221

Replaces the thumbnail from
image

To something to show the contents of the file as done on other platforms.

Finder icon mode and QuickLook:

image

Space bar on file:

image

Colors

image

System Preferences to toggle the behaviour: This
has been removed. Nothing will change in System Preferences

image

Support thumbnail that shows the file contents instead of the default blend file icon for all files in Finder. blender-thumbnailer process is kept alive by the system in the background and is used when needed. It will NOT be interactive "Preview" that allows richer interactions like Panning viewport, or rotating 3D objects. --------- This may need codesign changes in the buildbot. Community request: https://blender.community/c/rightclickselect/d3cbbc/?sorting=hot QLgenerator for older versions (not developed by me): https://blenderartists.org/t/blender-quicklook-plugin/621221 Replaces the thumbnail from ![image](/attachments/af801e08-b5b0-460d-9bc9-5452feccd3c3) To something to show the contents of the file as done on other platforms. Finder icon mode and QuickLook: ![image](/attachments/20f81268-fefe-48e2-8b4d-cb6b6939ee68) Space bar on file: ![image](/attachments/87fbe5c7-6eb6-46a7-a76c-3cdfb2134c66) Colors ![image](/attachments/3eba9b3e-957c-4b01-9b2f-72c670f675e3) ~~System Preferences to toggle the behaviour:~~ This has been removed. Nothing will change in System Preferences ![image](/attachments/a414d5e4-1bd6-4a0b-b263-58ab1b278d33)
Ankit Meel added 1 commit 2023-04-18 13:26:33 +02:00
Author
Member

@brecht @LazyDodo any preliminary comments, please? Cmake process or info.plist location etc. Further changes will only be in ThumbnailProvider.mm file

@brecht @LazyDodo any preliminary comments, please? Cmake process or info.plist location etc. Further changes will only be in ThumbnailProvider.mm file

Can you explain what this does, show a screenshot?

Can you explain what this does, show a screenshot?
Author
Member

Can you explain what this does, show a screenshot?

Updated description

> Can you explain what this does, show a screenshot? Updated description
Author
Member
Community request: https://blender.community/c/rightclickselect/d3cbbc/?sorting=hot QLgenerator for older versions (not mine): https://blenderartists.org/t/blender-quicklook-plugin/621221
Member

I'm a bit confused what this does? I assumed this was going to show the thumbnail embedded in the .blend file rather than the blender logo, just like it does on Windows.

before:

image

after:

image

You either chose poor examples or i don't understand what's happening there, but a giant white document doesn't seem better than a document with a blender logo to me?

I'm a bit confused what this does? I assumed this was going to show the thumbnail embedded in the .blend file rather than the blender logo, just like it does on Windows. before: ![image](/attachments/c930581c-caa3-42eb-919e-af183d2add50) after: ![image](/attachments/2a10dfa5-1e5d-4155-a124-f82907fdaba8) You either chose poor examples or i don't understand what's happening there, but a giant white document doesn't seem better than a document with a blender logo to me?

In terms of code organization, I'd prefer this to be more similar to the existing files for the regular Blender.app. So:

  • Move Info.plist to appropriate subdirectory of release/darwin/Blender.app
  • Move explanations about plist values values from ThumbnailProvider.mm into Info.plist
  • See the existing Info.plist for how to insert version numbers
  • Move ThumbnailExtensionMacOS.entitlements.Debug to release/darwin/thumbnail_entitlements.plist
  • Don't call lsregister as part of the build, nothing should be registered automatically on the build machine.

Code signing will have to be done on the buildbot. What is not clear to me if it's needed as well to get this to work on your own machine, or it can just not be signed for that case.

In terms of code organization, I'd prefer this to be more similar to the existing files for the regular Blender.app. So: * Move `Info.plist` to appropriate subdirectory of `release/darwin/Blender.app` * Move explanations about plist values values from `ThumbnailProvider.mm` into `Info.plist` * See the existing `Info.plist` for how to insert version numbers * Move `ThumbnailExtensionMacOS.entitlements.Debug` to `release/darwin/thumbnail_entitlements.plist` * Don't call `lsregister` as part of the build, nothing should be registered automatically on the build machine. Code signing will have to be done on the buildbot. What is not clear to me if it's needed as well to get this to work on your own machine, or it can just not be signed for that case.
Author
Member

tsk.. I forgot to hit save. Since the thumbnail generation code is noop as of now, nothing is created yet. WIP.

I'll use the the same functions to generate PNG and fill it in a buffer and pass it to the system, so output on OSes will be the same too.

tsk.. I forgot to hit save. Since the thumbnail generation code is noop as of now, nothing is created yet. WIP. I'll use the the same functions to generate PNG and fill it in a buffer and pass it to the system, so output on OSes will be the same too.
Author
Member

Don't call lsregister as part of the build, nothing should be registered automatically on the build machine.

It happens in Xcode also, that's where I took the command from.
Same can be achieved with launching Blender.app. I'll add that in a comment.

Code signing will have to be done on the buildbot. What is not clear to me if it's needed as well to get this to work on your own machine, or it can just not be signed for that case.

Yes that is needed locally also or else QL will mark it as invalid generator.

Xcode runs the command too.

>Don't call lsregister as part of the build, nothing should be registered automatically on the build machine. It happens in Xcode also, that's where I took the command from. Same can be achieved with launching Blender.app. I'll add that in a comment. > Code signing will have to be done on the buildbot. What is not clear to me if it's needed as well to get this to work on your own machine, or it can just not be signed for that case. Yes that is needed locally also or else QL will mark it as invalid generator. Xcode runs the command too.

It happens in Xcode also, that's where I took the command from.
Same can be achieved with launching Blender.app. I'll add that in a comment.

I'd rather we do not do this even if Xcode does. Having the system configuration affected every time you run a build just seems wrong to me.

> It happens in Xcode also, that's where I took the command from. > Same can be achieved with launching Blender.app. I'll add that in a comment. I'd rather we do not do this even if Xcode does. Having the system configuration affected every time you run a build just seems wrong to me.
Ankit Meel added 1 commit 2023-04-18 23:14:01 +02:00
Ankit Meel added 1 commit 2023-04-19 11:35:55 +02:00
Ankit Meel changed title from WIP: macOS/QuickLook: support rich thumbnail to macOS/QuickLook: support rich thumbnail 2023-04-19 11:42:07 +02:00
Ankit Meel requested review from Brecht Van Lommel 2023-04-19 11:42:45 +02:00
Ankit Meel requested review from Julian Eisel 2023-04-19 11:42:45 +02:00
Ankit Meel added this to the User Interface project 2023-04-19 11:42:58 +02:00
Ankit Meel added the
Module
User Interface
Platform
macOS
Interest
User Interface
labels 2023-04-19 11:43:39 +02:00
Author
Member

We may change the behaviour of "space bar on file" to make a full screen image later. Would need another appex file and a high res image.

We may change the behaviour of "space bar on file" to make a full screen image later. Would need another appex file and a high res image.
Brecht Van Lommel requested changes 2023-04-19 13:03:03 +02:00
Brecht Van Lommel left a comment
Owner

Getting a bigger preview to work may be impractical, and not worth the code complexity to get working. We don't want to make .blend files too big with big previews images, and opening .blend files specifically for previews is not something we should do.

Is there a way to display the preview inside a different icon, without the document shape? It looks a bit weird to me. Maybe the cup .blend just happens to be poorly aligned.

Getting a bigger preview to work may be impractical, and not worth the code complexity to get working. We don't want to make .blend files too big with big previews images, and opening .blend files specifically for previews is not something we should do. Is there a way to display the preview inside a different icon, without the document shape? It looks a bit weird to me. Maybe the cup .blend just happens to be poorly aligned.
@ -0,0 +45,4 @@
<key>NSExtensionPrincipalClass</key>
<!-- Must be the same as the class implementing the reply method. -->
<string>ThumbnailProvider</string>
<!-- Shows checkbox in System Preferences. -->

Is this required, and common for these kinds of thumbnails generators? I don't see it for any other apps, but maybe I just happen to not have any apps that do this.

Is this required, and common for these kinds of thumbnails generators? I don't see it for any other apps, but maybe I just happen to not have any apps that do this.
Author
Member

Not required and not sure about commonness. But since there's no other way to disable it, I added it.

I could remove the key from plist and let the system decide.

Not required and not sure about commonness. But since there's no other way to disable it, I added it. I could remove the key from plist and let the system decide.

I'd leave it out and let the system decide.

I'd leave it out and let the system decide.
@ -0,0 +6,4 @@
<key>com.apple.security.app-sandbox</key>
<true/>
<key>com.apple.security.files.user-selected.read-only</key>
<true/>

Inconsistent indentation.

Inconsistent indentation.
ankitm marked this conversation as resolved
@ -41,2 +41,3 @@
endif()
else()
if(UNIX AND NOT APPLE)

Code is more clearly mutually exclusive if you use:

if(WIN32)
..
elseif(APPLE)
...
elseif(UNIX)
...
endif()
Code is more clearly mutually exclusive if you use: ``` if(WIN32) .. elseif(APPLE) ... elseif(UNIX) ... endif() ```
ankitm marked this conversation as resolved
@ -57,0 +70,4 @@
"-e _NSExtensionMain"
"-framework QuickLookThumbnailing"
)
if(DEFINED PTHREADS_LIBRARIES)

This is never defined on macOS, not needed.

This is never defined on macOS, not needed.
ankitm marked this conversation as resolved
@ -0,0 +1,10 @@
<?xml version="1.0" encoding="UTF-8"?>

This file should be in release/darwin too.

Personally I'm also fine with not having the debugging entitlement at all, I just disable system integrity protection to be able to debug any application.

This file should be in `release/darwin` too. Personally I'm also fine with not having the debugging entitlement at all, I just disable system integrity protection to be able to debug any application.
Author
Member

I'll remove it.. debugging is only print debugging anyway.

I just disable system integrity protection to be able to debug any application.

Whoa

I'll remove it.. debugging is only print debugging anyway. > I just disable system integrity protection to be able to debug any application. Whoa
ankitm marked this conversation as resolved
@ -0,0 +5,4 @@
<key>com.apple.security.app-sandbox</key>
<true/>
<key>com.apple.security.files.user-selected.read-only</key>
<true/>

Inconsistent indentation.

Inconsistent indentation.
ankitm marked this conversation as resolved
@ -0,0 +1,13 @@
#pragma once

snake_case for filenames

snake_case for filenames
ankitm marked this conversation as resolved
@ -0,0 +1,159 @@

snake_case for filenames

snake_case for filenames
ankitm marked this conversation as resolved
@ -0,0 +8,4 @@
#include "BLI_fileops.h"
#include "BLI_filereader.h"
#include "blendthumb.hh"

Add newline before this.

Add newline before this.
ankitm marked this conversation as resolved
@ -0,0 +80,4 @@
}
};
static NSError *createErrorFromStr(NSString *errorStr)

snake_case for function names

`snake_case` for function names
Author
Member

Java habit from work ..

Java habit from work ..
ankitm marked this conversation as resolved
@ -0,0 +88,4 @@
userInfo:@{NSLocalizedDescriptionKey : errorStr}];
}
static NSImage *generateNSImageForFile(const char *src_blend_path, NSError **error)

snake_case for function names

`snake_case` for function names
ankitm marked this conversation as resolved
@ -0,0 +90,4 @@
static NSImage *generateNSImageForFile(const char *src_blend_path, NSError **error)
{

Remove empty line

Remove empty line
ankitm marked this conversation as resolved
@ -0,0 +92,4 @@
{
/* Open source file `src_blend`. */
FileReaderRAII src_file_fd = FileReaderRAII(BLI_open(src_blend_path, O_BINARY | O_RDONLY, 0));

This name is confusing to me, it's not RAII for the FileReader type below. Would also be better to put BLI_open inside the class constructor I think.

This name is confusing to me, it's not RAII for the `FileReader` type below. Would also be better to put `BLI_open` inside the class constructor I think.
ankitm marked this conversation as resolved
@ -0,0 +123,4 @@
CGImageRef image = CGImageCreateWithPNGDataProvider(
provider, nullptr, true, kCGRenderingIntentDefault);
CGDataProviderRelease(provider);
NSImage *uiimage = [[NSImage alloc] initWithCGImage:image size:NSZeroSize];

Not sure why this is called uiimage, maybe copied from example code?

Not sure why this is called `uiimage`, maybe copied from example code?
ankitm marked this conversation as resolved
@ -0,0 +125,4 @@
CGDataProviderRelease(provider);
NSImage *uiimage = [[NSImage alloc] initWithCGImage:image size:NSZeroSize];
CGImageRelease(image);
CGDataProviderRelease(provider);

Don't call CGDataProviderRelease twice

Don't call `CGDataProviderRelease` twice
ankitm marked this conversation as resolved
@ -1355,1 +1365,3 @@
DESTINATION Blender.app/Contents/MacOS/
DESTINATION Blender.app/Contents/Plugins
)
set(BL_CODESIGN_ENTITLEMENTS "${OSX_APP_SOURCEDIR}/../thumbnail_entitlements.plist")

Not using relative path is more clear I think, their relative location has no particular importance: ${CMAKE_SOURCE_DIR}/release/darwin/thumbnail_entitlements.plist.

Don't invent new BL_ prefix for variable names, suggest to use THUMBNAIL_ENTITLEMENTS

Not using relative path is more clear I think, their relative location has no particular importance: `${CMAKE_SOURCE_DIR}/release/darwin/thumbnail_entitlements.plist`. Don't invent new `BL_` prefix for variable names, suggest to use `THUMBNAIL_ENTITLEMENTS`
ankitm marked this conversation as resolved
@ -1356,0 +1365,4 @@
DESTINATION Blender.app/Contents/Plugins
)
set(BL_CODESIGN_ENTITLEMENTS "${OSX_APP_SOURCEDIR}/../thumbnail_entitlements.plist")
install(CODE

Does this now run on every make install? That would slow down incremental builds. Is there a way to make it run only when the appex is updated?

Does this now run on every `make install`? That would slow down incremental builds. Is there a way to make it run only when the appex is updated?
Author
Member
time codesign --entitlements release/darwin/thumbnail_entitlements.plist --force --deep --sign - ../build_darwin_debug_lite/bin/Blender.app/Contents/Plugins/blender-thumbnailer.appex
../build_darwin_debug_lite/bin/Blender.app/Contents/Plugins/blender-thumbnailer.appex: replacing existing signature
codesign --entitlements release/darwin/thumbnail_entitlements.plist --force    0.02s user 0.02s system 72% cpu 0.053 total

insignificant
Even my poor machine makes it unnoticeable. We aren't signing the full blender.app.

``` time codesign --entitlements release/darwin/thumbnail_entitlements.plist --force --deep --sign - ../build_darwin_debug_lite/bin/Blender.app/Contents/Plugins/blender-thumbnailer.appex ../build_darwin_debug_lite/bin/Blender.app/Contents/Plugins/blender-thumbnailer.appex: replacing existing signature codesign --entitlements release/darwin/thumbnail_entitlements.plist --force 0.02s user 0.02s system 72% cpu 0.053 total ``` insignificant Even my poor machine makes it unnoticeable. We aren't signing the full blender.app.
Author
Member

Image in Blender file browser. First I thought it wasn't scaling properly but that's not the case either.

image

Image in Blender file browser. First I thought it wasn't scaling properly but that's not the case either. ![image](/attachments/1935ab52-af4f-40af-bd6f-84a0386a96c1)
Ankit Meel added 1 commit 2023-04-19 14:45:09 +02:00
Ankit Meel added 1 commit 2023-04-19 14:55:40 +02:00
Author
Member

Is there a way to display the preview inside a different icon, without the document shape? It looks a bit weird to me. Maybe the cup .blend just happens to be poorly aligned.

Numbers, CSV and html work this way too
image

The square can be made rectangle by adding a "BLEND" banner under it

> Is there a way to display the preview inside a different icon, without the document shape? It looks a bit weird to me. Maybe the cup .blend just happens to be poorly aligned. Numbers, CSV and html work this way too ![image](/attachments/d1b569b8-8c3d-44f6-90e1-54dce799a801) The square can be made rectangle by adding a "BLEND" banner under it
Ankit Meel added 1 commit 2023-04-19 21:38:32 +02:00
snake_case
Some checks failed
buildbot/vexp-code-patch-coordinator Build done.
a4f987cb66

Is there a way to display the preview inside a different icon, without the document shape? It looks a bit weird to me. Maybe the cup .blend just happens to be poorly aligned.

Numbers, CSV and html work this way too

The square can be made rectangle by adding a "BLEND" banner under it

Ok, USDZ files seem to show similarly as a document so it's consistent, even if a bit strange to me.

> > Is there a way to display the preview inside a different icon, without the document shape? It looks a bit weird to me. Maybe the cup .blend just happens to be poorly aligned. > > Numbers, CSV and html work this way too > > The square can be made rectangle by adding a "BLEND" banner under it Ok, USDZ files seem to show similarly as a document so it's consistent, even if a bit strange to me.

I guess this will need buildbot code signing changes, but we can test what it does without them:

@blender-bot package macos

I guess this will need buildbot code signing changes, but we can test what it does without them: @blender-bot package macos
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR107072) when ready.
Ankit Meel added 1 commit 2023-04-20 18:37:02 +02:00
Brecht Van Lommel approved these changes 2023-04-20 19:40:54 +02:00
Brecht Van Lommel left a comment
Owner

Thumbnails don't seem to be working with the buildbot package, which is not surprising.

The code looks good to me, but this will need to wait to land until I have time to make the buildbot changes.

Thumbnails don't seem to be working with the buildbot package, which is not surprising. The code looks good to me, but this will need to wait to land until I have time to make the buildbot changes.
Author
Member

Thumbnails don't seem to be working with the buildbot package, which is not surprising.

There's a large number of things to go wrong. Listed in troubleshooting section in .mm file.
If you have time, it can be made to work locally.

  • codesigning appex locally with entitlements.
  • launching blender at least once
> Thumbnails don't seem to be working with the buildbot package, which is not surprising. There's a large number of things to go wrong. Listed in troubleshooting section in .mm file. If you have time, it can be made to work locally. - codesigning appex locally with entitlements. - launching blender at least once
Ankit Meel added 1 commit 2023-04-20 19:47:22 +02:00
Ankit Meel added 1 commit 2023-04-23 07:36:25 +02:00
Ankit Meel added 1 commit 2023-04-23 07:39:59 +02:00
Member

I know too little about macOS development to be useful here, so removing myself from the reviewer list. The feature itself seems good to have.

I know too little about macOS development to be useful here, so removing myself from the reviewer list. The feature itself seems good to have.
Julian Eisel refused to review 2023-05-01 11:56:18 +02:00
This pull request has changes conflicting with the target branch.
  • source/creator/CMakeLists.txt

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u ankitm/2ql:ankitm-ankitm/2ql
git checkout ankitm-ankitm/2ql
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#107072
No description provided.