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 - open files from Finder and other apps #18288

Merged
merged 10 commits into from
Jan 27, 2025

Conversation

michmill1970
Copy link
Contributor

This PR fixes the issue where files can't be opened in Darktable by Finder or other apps that use macOS Launch Services.

I've tested the repro steps in all issues related to #17298. All previously reported issues all work fine now.

Issues tested:
#17298 - passed
#17539 - passed
#17671 - passed
#17704 - passed
#18043 - passed

Tagging @zisoft as the macOS dev.

Cheers,
Mike

@zisoft zisoft self-requested a review January 25, 2025 14:55
@zisoft
Copy link
Collaborator

zisoft commented Jan 26, 2025

Works for me 👍

In src/gui/gtk.c we have a signal handler for NSApplicationOpenFile with the callback _osx_openfile_callback(). Can this be removed with this PR?

If I drop a single image from finder onto the lighttable of the running application, it gets imported but not switched to darkroom.

And I think @TurboGit will request some style changes :-)
(no space after for ( etc.)

@michmill1970
Copy link
Contributor Author

michmill1970 commented Jan 26, 2025

Works for me 👍

In src/gui/gtk.c we have a signal handler for NSApplicationOpenFile with the callback _osx_openfile_callback(). Can this be removed with this PR?

I don't think so. I tried to only touch things that would affect application launch, and nothing after darktable entered main(int argc, char *argv[]) (other than the #ifndefs that blocked the code from running in MAC_INTEGRATION mode).

If I drop a single image from finder onto the lighttable of the running application, it gets imported but not switched to darkroom.

I'll be honest, I don't know if I ever used drag/drop into the Lighttable view so I don't know the existing behavior. I did test my changes so that it didn't break anything. Did it go to darkroom before my changes? I can revert my private branch and test if you don't know off the top of your head. It sounds like going to the darkroom view is proper behavior, but did the macOS build follow this?

And I think @TurboGit will request some style changes :-) (no space after for ( etc.)

No worries. I'll fix whatever @TurboGit tells me to.

Cheers,
Mike

@zisoft
Copy link
Collaborator

zisoft commented Jan 26, 2025

Did it go to darkroom before my changes?

No, I thought this was another complaint by others. All good.

@TurboGit TurboGit added this to the 5.2 milestone Jan 26, 2025
Copy link
Member

@TurboGit TurboGit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a macOS guy, this is already validated so good to me.

Just some minor style to conform to current dt guideline. TIA.

src/osx/osx.mm Outdated Show resolved Hide resolved
src/osx/osx.mm Outdated Show resolved Hide resolved
@michmill1970 michmill1970 requested a review from TurboGit January 26, 2025 23:11
Copy link
Member

@TurboGit TurboGit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to me now, thanks!

@TurboGit
Copy link
Member

A side note, please never merge master branch into a topic branch like this one. I'll squash everything here, so no problem.

@TurboGit
Copy link
Member

@michmill1970 : on macOS 13 XCode 15.2 this is failing with:

/Users/runner/work/darktable/darktable/src/src/osx/osx.mm:307:17: fatal error: no matching member function for call to 'push_back'
    openedFiles.push_back([filename UTF8String]);
    ~~~~~~~~~~~~^~~~~~~~~
/Applications/Xcode_15.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk/usr/include/c++/v1/vector:591:62: note: candidate function not viable: no known conversion from 'NS_RETURNS_INNER_POINTER const char *' to 'const value_type' (aka 'const std::basic_string<char>') for 1st argument
    _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void push_back(const_reference __x);
                                                             ^
/Applications/Xcode_15.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk/usr/include/c++/v1/vector:593:62: note: candidate function not viable: no known conversion from 'NS_RETURNS_INNER_POINTER const char *' to 'value_type' (aka 'std::basic_string<char>') for 1st argument
    _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void push_back(value_type&& __x);
                                                             ^
1 error generated.

@zisoft
Copy link
Collaborator

zisoft commented Jan 27, 2025

I would suggest to use a simple GList here instead of std::vector

@michmill1970
Copy link
Contributor Author

michmill1970 commented Jan 27, 2025

I would suggest to use a simple GList here instead of std::vector

The issue isn't with the vector, but with the way earier versions of XCode casts the NSString to something that can be used by the std::string constructor. By being more explicit in the code casting and creating the std::string, the problem should be fixed.

The change has been committed and is awaiting your approval.

Cheers,
Mike

Copy link
Contributor Author

@michmill1970 michmill1970 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue was due to the way the older XCode compiler was casting the NSString when constructing the std::string. This should fix it by being more explicit in the code.

@TurboGit
Copy link
Member

Please DO NOT merge master into your topic branch. TIA.

@TurboGit TurboGit added the scope: macos support macos related issues and PR label Jan 27, 2025
Copy link
Member

@TurboGit TurboGit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@TurboGit TurboGit merged commit d251a5e into darktable-org:master Jan 27, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: macos support macos related issues and PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants