Removing Carbon from GHOST Cocoa (MacOS) System
Needs RevisionPublic
Actions

Authored by Sam Miller (samuelmiller) on Feb 9 2021, 3:11 PM.

Details

Reviewers

Brecht Van Lommel (brecht)

Group Reviewers

Platform: macOS

Summary

This patch removes code that relies on the Carbon framework. Some deleted code explicitly mentions older versions of Mac (10.6) which are no longer supported (minimum 10.13). Some functions are shared between Cocoa and Carbon and can be imported from other Apple libraries.

I am not sure if there are edge cases for getButtons, but it seems like Cocoa handles the input elsewhere. There may be other areas which are not functionally valuable, but this patch is focused on eliminating importing Carbon\Carbon.h and the "GHOST Hack" workaround in creator.c.

Diff Detail

Event Timeline

Minor typo.

Ankit Meel (ankitm) added a project: Platform: macOS.Feb 9 2021, 4:02 PM

When it's no longer a work-in-progress, you should add reviewers.

Thank you. I added the MacOS project, but please let me know if there's someone/something else I should add for review.

Sam Miller (samuelmiller) edited the summary of this revision. (Show Details)Feb 10 2021, 2:49 PM

Overall, getting rid of Carbon here seems like good idea. I'm just not sure if copying over the enums is the right way to do this. If there is no alternative (other than keeping the include) this is fine by me.

I agree copying the enum is not ideal. Almost all the Cocoa code I've found that uses these key codes imports Carbon. I've only found one example on Github that uses these key codes without Carbon and it basically defines the enum. I appreciate the feedback and maybe someone else has a better answer.

Can -framework Carbon now be removed from platform_apple.cmake?

@Brecht Van Lommel (brecht) Yes, with this patch I am able to build without that flag.

I'm pretty sure this will break support for non-English keyboards, which is not very good as it is, see for example D5011.

Sam Miller (samuelmiller) added a comment.Feb 18 2021, 12:35 AM

This comment was removed by Sam Miller (samuelmiller).

Brecht Van Lommel (brecht) requested changes to this revision.Feb 22 2021, 6:10 PM

Brecht Van Lommel (brecht) added inline comments.

intern/ghost/intern/GHOST_SystemCocoa.mm
58	Adding these enums directly is fine with me.
278–304	As mentioned by @Yevgeny Makarov (jenkm) this probably breaks something, code needs to be replaced, not just removed.
879–886	This code can't just be removed, it needs a replacement.
source/creator/creator.c
352	It seems this is a special argument passed to Carbon processes. I think to remove this, we probably can't use any Carbon functionality at all and not link to the framework. Also needs to be tested that launching Blender from Finder in fact no longer passes this argument after the changes.

This revision now requires changes to proceed.Feb 22 2021, 6:10 PM

Thank you for the comments. It seems like there is dead code here, but maybe I am misreading. I also do not have an international keyboard so I am not the best one to address concerns about that.

It seems that the Text Input System functions like TISInputSourceRef are only available in Carbon. There does not seem to be a workaround without that framework at this point.

I'll try to find a better workaround for the process serial number issue. It seems to persist in some cases, but I'm not sure how closely that's related to Carbon. https://github.com/godotengine/godot/pull/37719#issuecomment-611945632

Corbin Dunn (corbin) added a subscriber: Corbin Dunn (corbin).Mar 3 2021, 3:02 PM

Corbin Dunn (corbin) added inline comments.Mar 3 2021, 3:35 PM

intern/ghost/intern/GHOST_SystemCocoa.mm
59	Unfortunately, there is no AppKit equivalent for virtual key codes. Apple strives to provide equivalency in AppKit without depending on Carbon; can you log a bug on this for Apple/AppKit? bugreport.apple.com
305	The proper way to do this is to just use charactersIgnoringModifiers (from an AppKit perspective, which I used to work on). I'd be surprised if this did something different. I think this code should just be removed. To test: Go to Preferences -> Keyboard -> Input Sources, and add some other input sources. Test typing some of the remapped characters and make sure they work correctly.
886	Use NSEvent's class property: /* mouse buttons currently down. Returns indices of the mouse buttons currently down. 1 << 0 corresponds to leftMouse, 1 << 1 to rightMouse, and 1 << n, n >= 2 to other mouse buttons. This returns the state of devices combined with synthesized events at the moment, independent of which events have been delivered via the event stream, so this method is not suitable for tracking. */ @property (class, readonly) NSUInteger pressedMouseButtons API_AVAILABLE(macos(10.6));
source/creator/creator.c
352	I can confirm that this was old carbon stuff not present in apps that use Cocoa/NSApplicationMain. Still, it would be good to test it by launching a Blender by dropping a file on Blender.app in Finder.