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

Fix warnings/tests with recent macOS and add more generics annotations #75

Merged
merged 9 commits into from
Nov 15, 2021

Conversation

qmathe
Copy link
Member

@qmathe qmathe commented Oct 29, 2021

  • Warnings were caused by Apple deprecating more APIs starting from macOS 10.9.

  • Test failure was caused by the default color space on macOS being changed from 'generic' (aka NSCalibratedRGBColorSpace) to 'sRGB'. I'm not sure in which version that was changed, but the change I made is backward-compatible, so it shouldn't matter. Here is the failing test ouput:

=== [TestValueTransformerAttribute testRoundTrip] ===
Etoile/Frameworks/CoreObject/Tests/Serialization/TestValueTransformerAttribute.m:101: warning: Failed, expected "sRGB IEC61966-2.1 colorspace 1 0 0 1", got "NSCalibratedRGBColorSpace 0.984314 0 0.0235294 1"

  • The new generics annotations allow to call more CoreObject APIs from Swift without casting.

  • For sanboxed apps on macOS, posting distributed notifications containing a user info dictionary (and also registering for distributed notifications without targeting a specific object) causes sandboxing violations:

Toucan *** attempt to post distributed notification 'COStorePersistentRootsDidChangeNotification' thwarted by sandboxing.

Date/Time: Fri Oct 29 15:10:26 2021
OS Version: 21A559
Application: Toucan

Backtrace:
0 CoreFoundation 0x00007ff80ddb75b8 __CFGenerateReport + 175
1 CoreFoundation 0x00007ff80dc4e9f8 _CFXNotificationPost + 1359
2 Foundation 0x00007ff80eb65f5b -[NSDistributedNotificationCenter postNotificationName:object:userInfo:options:] + 84

My solution to this is to use a custom distribution center that does nothing when sandboxing is enabled. The previous solution introduced for iOS doesn't work, because NSDistributedNotificationCenter is a public API on macOS unlike iOS, so the compiler ignores the compatibility alias.

@qmathe qmathe marked this pull request as ready for review October 29, 2021 13:59
@qmathe qmathe requested a review from ericwa October 29, 2021 13:59
@qmathe qmathe self-assigned this Oct 29, 2021
@qmathe qmathe changed the title Fix warnings/tests with recent macOS and add more generics annotations for Swift compatibility Fix warnings/tests with recent macOS and add more generics annotations Oct 29, 2021
Copy link
Member

@ericwa ericwa left a comment

Choose a reason for hiding this comment

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

I haven't run anything, but changes look fine!

@qmathe
Copy link
Member Author

qmathe commented Oct 30, 2021

Thanks for the review, Eric.

I took a look at PR #74 yesterday. Before merging, I'd like to take most changes from PR #74 and apply them on top of this PR, since both PRs are related. I could then close PR #74 without applying it. Any objections?

@ericwa
Copy link
Member

ericwa commented Oct 30, 2021

No objections!

@qmathe
Copy link
Member Author

qmathe commented Nov 4, 2021

Oops, I didn't run the tests with -DSANDBOXED=1 last time, so CODistributedNotificationCenter implementation was wrong and some tests were broken. I fixed it. The code now covers for the case where we sync two editing contexts backed by two distinct store instances pointing to the same underlying SQLite db on disk.

I also applied changes from PR #74 on top.

In COCrossPersistentRootDeadRelationshipCache, we have this comment:

        // FIXME: If we don't ditch 10.7 support, we need a reverse mapping
        // from each referringObject to a path set, that can be used to remove
        // the referring objects when their object graph context is discarded.

For me, it means _referringObjectToPaths can be removed if we support weak-zeroing references as it is the case on 10.8 and higher (not sure about GNUstep though). I tried to do it, but I get a test failure and some exceptions, so not sure what's going on here. I'll keep this for later, since I don't want to spend too much time on it for now.

@qmathe
Copy link
Member Author

qmathe commented Nov 8, 2021

@ericwa Any comments on my latest changes before I merge?

@ericwa
Copy link
Member

ericwa commented Nov 10, 2021

No special comments, looks fine.

@qmathe qmathe merged commit 8b3ce88 into master Nov 15, 2021
@qmathe qmathe deleted the swift-compatibility branch February 2, 2024 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants