Skip to content
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

[macOS] Adding QuickLook feature #7491

Merged
merged 9 commits into from Nov 4, 2022
Merged

[macOS] Adding QuickLook feature #7491

merged 9 commits into from Nov 4, 2022

Conversation

MatthiasWM
Copy link
Contributor

@MatthiasWM MatthiasWM commented Sep 14, 2022

  • Your pull request is confined strictly to a single module. That is, all the files changed by your pull request are either in App, Base, Gui or one of the Mod subfolders. If you need to make changes in several locations, make several pull requests and wait for the first one to be merged before submitting the next ones

The code is located in src/MacAppBundle/QuickLook so it does not interfere with other platforms or macOS command line builds

  • In case your pull request does more than just fixing small bugs, make sure you discussed your ideas with other developers on the FreeCAD forum

This PR is in relation to Forum: New feature: Quicklook for macOS

  • Your branch is rebased on latest master git pull --rebase upstream master
  • All FreeCAD unit tests are confirmed to pass by running ./bin/FreeCAD --run-test 0

This commit has no impact on the actual app. Also I will need hep at some point to build a full app on macOS Monterey.

  • All commit messages are well-written ex: Fixes typo in Draft Move command text
  • Your pull request is well written and has a good description, and its title starts with the module name, ex: Draft: Fixed typos

And please remember to update the Wiki with the features added or changed once this PR is merged.
Note: If you don't have wiki access, then please mention your contribution on the 1.0 Changelog Forum Thread.

Will do.

@berndhahnebach
Copy link
Contributor

pipeline status for feature branch PR_7491. Pipeline 640548795 was triggered at cb5bc5b. All CI branches and pipelines.

@MatthiasWM
Copy link
Contributor Author

This Pull Request adds macOS QuickLook previews and icons to FreeCAD.

The patch in Info.plist makes *.FCStd files known as filetype org.freecad.fcstd to the system. The actual QL plugin source code is located in src/MacAppBundle/QuickLook and is installed in FreeCAD.app/Contents/Library/QuickLook using CMake install in the file src/MacAppBundle/CMakeLists.txt.

FreeCAD needs to be built as a full macOS app (brew install --with-macos-app freecad@0.20.1) and the installed in the /Applications folder. A restart of Finder may be required to get QL icons.

@luzpaz
Copy link
Contributor

luzpaz commented Sep 15, 2022

@ipatch if you have any time, would you be available to test this PR ?

@luzpaz luzpaz added Feature FR for improvements or new features OS: macOS labels Sep 15, 2022
@ipatch
Copy link
Contributor

ipatch commented Sep 15, 2022

@ipatch if you have any time, would you be available to test this PR ?

sure, give me a few.

@MatthiasWM
Copy link
Contributor Author

MatthiasWM commented Sep 15, 2022

Looking at the CI output, I saw that the QuickLook/main.c source code is checked on Linux, not finding includes that only exist on macOS (CoreFoundations, etc.).

I did fix the tab and space errors that I found in the branch, so they can be pulled. The tab errors in main.c and both Info.plist files are generated by Apple tools. If this helps, I'll be happy to fix that manually, but at least for Info.plist, they will reappear as soon as those files are edited again.

Anything else I need to fix, please let me know.

Edit: Oh, and do you want me to do "Update branch" (in the GitHub PR page) with a merge commit or a rebase?

@berndhahnebach
Copy link
Contributor

pipeline status for feature branch PR_7491. Pipeline 641547865 was triggered at a95b59b. All CI branches and pipelines.

@ipatch
Copy link
Contributor

ipatch commented Sep 15, 2022

@MatthiasWM

per the github web ui recommendations for testing out this PR, i ran the below commands on my local build box (same box i use to build the bottles for the homebrew freecad tap). i'm using the macos mojave vm because it gives me the least amount friction when building an app bundle ie. a FreeCAD.app

git remote add MatthiasWM https://github.com/MatthiasWM/FreeCAD.git
git fetch MatthiasWM ql3
git switch --track MatthiasWM/ql3

i pushed your PR to my fork of freecad so you can see if might have messed something up, but so far from running those cmds on my setup cmake is giving me configuration incomplete message using the below cmake command

cmake \
-DHOMEBREW_PREFIX=$bp \
-DPYTHON_EXECUTABLE="$bp/opt/python@3.10/bin/python3" \
-DPYTHON_INCLUDE_DIR="$bp/opt/python@3.10/Frameworks/Python.framework/Headers" \
-DCMAKE_PREFIX_PATH="$pthmedfile;$pthswig;$pthqt;$pthshiboken2;$pthpyside2;$pthvtk82;$pthocc;$pthicu4c" \
-DCMAKE_BUILD_TYPE=RelWithDebInfo \
-DBUILD_FEM_NETGEN=0 \
-DFREECAD_USE_3DCONNEXION=0 \
-DBUILD_QT5=1 \
-DBUILD_ENABLE_CXX_STD=C++17 \
-Wno-dev \
-DFREECAD_CREATE_MAC_APP=1 \
-DCMAKE_INSTALL_PREFIX="$code/freecad-git/installs/$(basename $PWD)" \
-DBUILD_ARCH=1 \
-DBUILD_DRAFT=1 \
-DBUILD_TUX=1 \
-DBUILD_OPENSCAD=1 \
-DBUILD_TECHDRAW=1 \
-DBUILD_SPREADSHEET=1 \
-DBUILD_DRAWING=1 \
\
../../freecad-src/

obviously the local shell variables are just that, local for my particular build setup on this box. however i'm seeing the below output after running that cmake command in my build dir. output has been trimmed. can provide full output of the cmake command if needed.

=================================================
Now run 'cmake --build /opt/code/freecad-git/builds/build.release.0.20.1.pr7491' to build FreeCAD
=================================================

-- Configuring incomplete, errors occurred!
See also "/opt/code/freecad-git/builds/build.release.0.20.1.pr7491/CMakeFiles/CMakeOutput.log".
See also "/opt/code/freecad-git/builds/build.release.0.20.1.pr7491/CMakeFiles/CMakeError.log".

and probably where cmake begins to fail,

-- Performing Test _flag_found - Failed
CMake Error at src/MacAppBundle/CMakeLists.txt:37 (string):
  string begin index: -1 is out of range 0 - 74


CMake Error at src/MacAppBundle/CMakeLists.txt:38 (string):
  string sub-command REGEX, mode MATCHALL needs at least 5 arguments total to
  command.


CMake Error at src/MacAppBundle/CMakeLists.txt:39 (string):
  string sub-command REGEX, mode REPLACE needs at least 6 arguments total to
  command.


CMake Error at src/MacAppBundle/CMakeLists.txt:40 (string):
  string sub-command REGEX, mode REPLACE needs at least 6 arguments total to
  command.


CMake Error at src/MacAppBundle/CMakeLists.txt:37 (string):
  string begin index: -1 is out of range 0 - 76


CMake Error at src/MacAppBundle/CMakeLists.txt:38 (string):
  string sub-command REGEX, mode MATCHALL needs at least 5 arguments total to
  command.


CMake Error at src/MacAppBundle/CMakeLists.txt:39 (string):
  string sub-command REGEX, mode REPLACE needs at least 6 arguments total to
  command.


CMake Error at src/MacAppBundle/CMakeLists.txt:40 (string):
  string sub-command REGEX, mode REPLACE needs at least 6 arguments total to
  command.


-- Checking for module 'icu-uc'
--   Found icu-uc, version 71.1
find: /usr/local/Cellar/nglib: No such file or directory
Quicklook: copy framework template from <</opt/code/freecad-git/freecad-src/src/MacAppBundle>>/QuickLook/ to <</opt/code/freecad-git/installs/build.release.0.20.1.pr7491/FreeCAD.app/Contents>>/Library/QuickLook
--
==============
Summary report
==============

i hope this helps. and let me know if i need to amend or update my PR branch at some point.

below is a link to my fork freecad containing a branch of this PR

https://github.com/ipatch/FreeCAD/tree/ipatch.builds.pr7491

cheers

@MatthiasWM
Copy link
Contributor Author

@ipatch Thanks for trying my code. I have not touched lines 37 to 40 in MacAppBundle/CMakeLists.txt. My changes are in lines 128 to 144. The lines handle some Python code location fixes and are unrelated to my changes.

I have generally trouble compiling FreeCAD into a bundle. I am using brew install -v --HEAD --keep-tmp --with-macos-app mcad@0.20.1 where mcad@0.20.1 is a modified version of freecad@0.20.1, pointing to my repo and branch instead of freecad.master . I was never able to use cmake directly. The resulting .app bundle is bad and can not be moved without breaking. I'd love to know how the weekly (daily) M1 builds are actually made.

Not sure if this is related at all, but some CMake parts expect HomeBrew to be in /usr/local which are in /opt/homebrew/ in a fresh homeprew setup: Homebrew Patch

@ipatch
Copy link
Contributor

ipatch commented Sep 15, 2022

@ipatch Thanks for trying my code. I have not touched lines 37 to 40 in MacAppBundle/CMakeLists.txt. My changes are in lines 128 to 144. The lines handle some Python code location fixes and are unrelated to my changes.

thanks. yeah i just popped this tab open again to update my response. indeed those lines are related to collecting / gathering the .pth files contained with the python3.x site-packages dir and adding those additional site-packages ie. shiboken2 and pyside2 and possible matplotlib if installed, and others as glob looks for homebrew* related .pth files. i had manually created a .pth file both shiboken2 and pyside2 and sure enough i just renamed files for temp purposes and the cmake step finishes without errors. 👍

all that brings another issue though, i think these checks should be a tad bit more robust, but that's separate issue for another day. as my manually added .pth files only contain a directory path such as the one below,

/Users/brewmaster/homebrew/opt/shiboken2@5.15.5/lib/python3.10/site-packages

I have generally trouble compiling FreeCAD into a bundle. I am using brew install -v --HEAD --keep-tmp --with-macos-app mcad@0.20.1 where mcad@0.20.1 is a modified version of freecad@0.20.1, pointing to my repo and branch instead of freecad.master . I was never able to use cmake directly. The resulting .app bundle is bad and can not be moved without breaking. I'd love to know how the weekly (daily) M1 builds are actually made.

i believe the weekly builds are made using conda. which is something we should look into. as it looks this plugin might only be applicable for homebrew builds freecad which would be unfortunate for those who are using the conda build and would enjoy using this plugin. me personally i'm not familiar with the conda build system so you'll have to ping somebody who is familiar with that stuff.

Not sure if this is related at all, but some CMake parts expect HomeBrew to be in /usr/local which are in /opt/homebrew/ in a fresh homeprew setup: Homebrew Patch

actually the freecad source was setup a while ago where somebody (too lazy, to look through the git blame at the moment) but they added a useful cmake var where you add a HOMEBREW_PREFIX as you've pointed out, i believe the default $bp (thats my short abbr for HOMEBREW_PREFIX), points to /usr/local for the default install of homebrew on intel where as the default install path on an m1 box i believe is in the /opt/homebrew as you've probably found out. now homebrew does support custom install paths, as you probably read in my cmake command. this useful for a multiuser system, especially so where the github CI only works with a homebrew install in /usr/local for intel and the entire setup is tore down after each run (oooffff 😖). and you've also figured out the --keep-tmp flag for brew for not having go through arduous process over and over again. kudos.


all that said 😥, i'll finish the cmake configure, then run through make make install and upload the app bundle to the homebrew-freecad if everything works out. and thanks for making this plugin. i enjoy using quicklook myself from time to time. 🎉

@MatthiasWM
Copy link
Contributor Author

MatthiasWM commented Sep 15, 2022

all that brings another issue though, i think these checks should be a tad bit more robust, but that's separate issue for another day. as my manually added .pth files only contain a directory path such as the one below,

/Users/brewmaster/homebrew/opt/shiboken2@5.15.5/lib/python3.10/site-packages

Well, at least we get stuck at the same places. Same happens to me if I use cmake directly, but not if I use homebrew. So even with --keep-tmp, I still have to rebuild from scratch and through github, but at least I can see what cmake generates.

i believe the weekly builds are made using conda. which is something we should look into. as it looks this plugin might only be applicable for homebrew builds freecad which would be unfortunate for those who are using the conda build and would enjoy using this plugin. me personally i'm not familiar with the conda build system so you'll have to ping somebody who is familiar with that stuff.

I was missing files in the repo to even try conda, but then again, I know nothing about condo. I'll see if I find someone who does. Conda does build a bundle, and I hope (assumed) that it is based on cmake as well, so QuickLook would be part of the release. I'll follow up on that.

@MatthiasWM
Copy link
Contributor Author

MatthiasWM commented Sep 15, 2022

You are right, the Conda script does indeed include its own Info.plist wich does not have the changes to register the file type.

Script
Info.plist template

@looooo Hi Lorenz. Would it make sense and be possible to take the Info.plist from the main repo and also include my patches? I am not familiar with Condo, but the CMake setup has the basics to build app bundles and make QuickLook work.

@ipatch
Copy link
Contributor

ipatch commented Sep 15, 2022

your PR build finished. 👍

ran make install got the usual error message i've seen the past.

-- Installing: /opt/code/freecad-git/installs/pr7491/FreeCAD.app/Contents/Library/QuickLook/QuicklookFCStd.qlgenerator/Contents/Info.plist
-- Installing: /opt/code/freecad-git/installs/pr7491/FreeCAD.app/Contents/Library/QuickLook/QuicklookFCStd.qlgenerator/Contents/MacOS/QuicklookFCStd
-- Making bundle relocatable...
-- INFO: Analyzing bundle dependencies...
-- ERROR: Unable to find LC_DYLD_LOAD entry: @rpath/libc++abi.1.dylib
-- ERROR: Failed to resolve dependency in /Users/brewmaster/homebrew/opt/llvm/lib/libc++.1.dylib
Traceback (most recent call last):
  File "/opt/code/freecad-git/freecad-src/src/Tools/MakeMacBundleRelocatable.py", line 391, in <module>
    main()
  File "/opt/code/freecad-git/freecad-src/src/Tools/MakeMacBundleRelocatable.py", line 374, in main
    build_deps_graph(graph, bundle_path, dir_filter, search_paths)
  File "/opt/code/freecad-git/freecad-src/src/Tools/MakeMacBundleRelocatable.py", line 241, in build_deps_graph
    deps = create_dep_nodes(list_install_names(k2), s_paths)
  File "/opt/code/freecad-git/freecad-src/src/Tools/MakeMacBundleRelocatable.py", line 166, in create_dep_nodes
    raise LibraryNotFound(lib_name + " not found in given search paths")
__main__.LibraryNotFound: libc++abi.1.dylib not found in given search paths
-- Installing: /opt/code/freecad-git/installs/pr7491/FreeCAD.app/Contents/share/License.txt
-- Installing: /

which is unrelated to your PR.

however, is there post installation step that is required after building the FreeCAD.app bundle in order to get the quicklook feature working?

so i'm seeing this using quicklook with finder

image

@MatthiasWM
Copy link
Contributor Author

MatthiasWM commented Sep 15, 2022

The bundle must be moved to /Applications. This should be enough for macOS to pick up on the file type. You can check that by running

qlmanage -m | grep freecad
  org.freecad.fcstd -> /Applications/FreeCAD.app/Contents/Library/QuickLook/QuicklookFCStd.qlgenerator (0)

If it does not, use Finder to throw the bundle in the Trash and then move it back into /Applications. If it still has not picked up on the new type, a reboot may help. If it still isn't there, /Applications/FreeCAD.app/Contents/Info.plist wasn't installed correctly by the CMake script.

If the manager was found, you may need to restart Finder (Option-Rightclick, and select "Relaunch" in the menu).

If all that still fails, you can at least check with qlmanage -t some.FCStd and qlmanage -p some.FCStd if it created thumbnails and previews.

Oh, and not all .FCStd files actually have an embedded preview file. Older versions of FreeCAD did not generate thise, and some versions just generated the FC logo as a thumbnail (unzip the FCStd file and search for thumbnails/Thumbnail.png). My code will not launch FreeCAD to actually generate a thumbnail if there is none in the file.

All of the above is usually automatic when a user drags the FreeCAD.app file into the /Applications folder, or if we were to build the app using Xcode.

@MatthiasWM MatthiasWM marked this pull request as draft September 16, 2022 00:52
@MatthiasWM MatthiasWM marked this pull request as ready for review September 16, 2022 00:56
@berndhahnebach
Copy link
Contributor

pipeline status for feature branch PR_7491. Pipeline 641904435 was triggered at fbd46a0. All CI branches and pipelines.

@looooo
Copy link
Contributor

looooo commented Sep 16, 2022

You are right, the Conda script does indeed include its own Info.plist wich does not have the changes to register the file type.

Script Info.plist template

@looooo Hi Lorenz. Would it make sense and be possible to take the Info.plist from the main repo and also include my patches? I am not familiar with Condo, but the CMake setup has the basics to build app bundles and make QuickLook work.

I am not sure if this works. For the conda-based bundles the build and bundle process are two separate tasks. Are there any conditions for the Info.plist to be build?

@ipatch
Copy link
Contributor

ipatch commented Sep 16, 2022

@MatthiasWM

rebuilding your PR now, (i saw you force pushed some changes since my last build) BTW it looks your located in Germany, if my detective work 🕵🏻‍♂️ is correct. i'm located in merica 🦅, so if there is a delay in my responses it is mostly due to time zones. ie. me being US CST. ie. UTC -5. i generally mix in my computer work from afternoon to about 6PM (ie. freecad related when i can).


also saw you're mixing tabs and spaces in your main.c file in this PR. do you think it'd be best to stick with one of the other. personally i prefer spaces. (not trying to start holy war or anything).

@ipatch
Copy link
Contributor

ipatch commented Sep 16, 2022

@MatthiasWM

finished rebuilding your PR, and produced an app bundle. 👍

  • copied the bundle to the /Applications dir.

ran the the below commands.

brewmaster@vmmojave ~/D/freecad.docs> qlmanage -t bs1.FCStd
Testing Quick Look thumbnails with files:
        bs1.FCStd
2022-09-16 15:42:44.288 qlmanage[40630:14060285] *** CFMessagePort: bootstrap_register(): failed 1100 (0x44c) 'Permission denied', port = 0x8b1f, name = 'com.apple.tsm.portname'
See /usr/include/servers/bootstrap_defs.h for the error codes.
* No thumbnail created for /Users/brewmaster/Documents/freecad.docs/bs1.FCStd
Done producing thumbnails

i'm seeing the below in my mojave vm.

image


do you mind attaching a file that does contain a thumbnail to this PR via a comment. and i can download that file and test?

@ipatch
Copy link
Contributor

ipatch commented Sep 16, 2022

attached is a file I created with your app bundle FreeCAD.ap using this PR. I'm still not seeing a quicklook preview.

bss1.FCStd.zip


below is a screen grab of what the file looks like,

image

@ipatch
Copy link
Contributor

ipatch commented Sep 16, 2022

all that brings another issue though, i think these checks should be a tad bit more robust, but that's separate issue for another day. as my manually added .pth files only contain a directory path such as the one below,

/Users/brewmaster/homebrew/opt/shiboken2@5.15.5/lib/python3.10/site-packages

Well, at least we get stuck at the same places. Same happens to me if I use cmake directly, but not if I use homebrew.

all that above should probably be opened in a separate issue, but add a link here for completeness sake,

a2127e0#r84213038

@MatthiasWM
Copy link
Contributor Author

@ipatch The file you attached has no Thumbnail.png. You can enable this feature using the FreeCAD preferences. I attached a file that does have the thumbnail (just rename the FCStd file to something.zip and unzip it. Inside should be a folder named "thumbnails", containing a file named "Thumbnail.png").

Reminder to self: final output should be sized using the png size; output image could contain a note stating there was no Thumbnail; can we suggest that the user changes the preferences? ; should we start the entire FreeCAD app in the background to generate the Thumbnail.png (not good IMHO as FC will be launched for each preview or icon)?

test8.FCStd.zip -> rename to test8.FCStd to preview, or to test8.zip to see contents.

Screenshot 2022-09-17 at 13 19 09

@ipatch
Copy link
Contributor

ipatch commented Sep 17, 2022

that same document i added previously after turning on the option you mentioned above.

image

@MatthiasWM
Copy link
Contributor Author

That's how it is supposed to look. Great!

@berndhahnebach
Copy link
Contributor

pipeline status for feature branch PR_7491. Pipeline 645847973 was triggered at eb2017b. All CI branches and pipelines.

@MatthiasWM
Copy link
Contributor Author

@looooo I added a PR 136 to FreeCAD-Bundle, so QuickLook should work with Conda bundles as well.

@berndhahnebach
Copy link
Contributor

pipeline status for feature branch PR_7491. Pipeline 647973705 was triggered at 7dfabea. All CI branches and pipelines.

@luzpaz
Copy link
Contributor

luzpaz commented Oct 23, 2022

Is this ready to be reviewed ?

@MatthiasWM
Copy link
Contributor Author

Yes, it's ready

@berndhahnebach
Copy link
Contributor

pipeline status for feature branch PR_7491. Pipeline 674776242 was triggered at ac8fe0c. All CI branches and pipelines.

@luzpaz
Copy link
Contributor

luzpaz commented Oct 23, 2022

@ipatch can you re-test and review ?

@looooo there is FreeCAD/FreeCAD-Bundle#136 can we merge and have the macOS folk test it ? (bump the appimage weekly release to make that happen?)

Edit: @looooo after this PR is merged of course

@berndhahnebach
Copy link
Contributor

pipeline status for feature branch PR_7491. Pipeline 674782892 was triggered at b39ab4d. All CI branches and pipelines.

@chennes chennes self-requested a review October 24, 2022 04:39
@luzpaz
Copy link
Contributor

luzpaz commented Oct 31, 2022

@ipatch sorry for bugging you...any chance you can test this ?

@ipatch
Copy link
Contributor

ipatch commented Oct 31, 2022

@ipatch sorry for bugging you...any chance you can test this ?

sure give me a few.

ipatch added a commit to ipatch/FreeCAD that referenced this pull request Oct 31, 2022
@ipatch
Copy link
Contributor

ipatch commented Oct 31, 2022

i was able to build locally using my macos mojave vm. 👍

🎃

@freecadci
Copy link

pipeline status for feature branch PR_7491. Pipeline 681965791 was triggered at cbc6fcb. All CI branches and pipelines.

@chennes
Copy link
Member

chennes commented Nov 4, 2022

Since @ipatch reports success with this plugin above, I'm going to merge it in so we can get more OS X users in the loop, and can catch anything it's missing. This is a tough one to test!

@chennes chennes merged commit f33be68 into FreeCAD:master Nov 4, 2022
@ipatch
Copy link
Contributor

ipatch commented Nov 4, 2022

@chennes no need to revert any changes. i'm going to update the patch file for the freecad formula file in the homebrew-freecad tap to work with this recent merge. referenced this PR in the homebrew-freecad tap for posterity.

chennes pushed a commit to chennes/FreeCAD that referenced this pull request Nov 4, 2022
* [macOS] Adding QuickLook feature
* [macOS] Adding QuickLook support for Conda and Homebrew.
* [macOS] Support non-square app icons in thumbnails.
* [macOS] adding icon for .FCScript files
@MatthiasWM
Copy link
Contributor Author

Thanks!

@MatthiasWM MatthiasWM deleted the ql3 branch November 4, 2022 21:27
chennes pushed a commit to chennes/FreeCAD that referenced this pull request Nov 4, 2022
* [macOS] Adding QuickLook feature
* [macOS] Adding QuickLook support for Conda and Homebrew.
* [macOS] Support non-square app icons in thumbnails.
* [macOS] adding icon for .FCScript files
@luzpaz
Copy link
Contributor

luzpaz commented Nov 6, 2022

Thank you @MatthiasWM !

Edit: and @ipatch and @chennes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature FR for improvements or new features OS: macOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants