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

Remove custom NSErrorException in favor of built-in ObjCErrorException #666

Conversation

darronschall
Copy link
Contributor

Instead of introducing our own NSErrorException to propagate NSErrors to Swift (that I introduced in #598), we change on approach to instead rely on the built-in (albeit internal) ObjCErrorException via https://github.com/JetBrains/kotlin/blob/b05604058fa3c480030267629cc70cf180a1263d/kotlin-native/runtime/src/main/kotlin/kotlin/native/internal/ObjCExportUtils.kt#L274

Previously, we needed to create a helper on the Swift side of things to extract the nested NSError details when throwing NSErrorExceptions in our code:

extension Error {
    var wrappedNSError: NSError? {
        let kotlinException = (self as NSError).userInfo["KotlinException"] as? KotlinException
        let nsErrorException = kotlinException as? NSErrorException
        return nsErrorException?.nsError as? NSError
    }
}

Downstream users had to rely on the helper extension to read the error details:

do {
    //
} catch {
    if let wrappedError = error.wrappedNSError {
        print("wrappedError domain: \(wrappedError.domain)")
        // etc.
    }
}

With this updated approach leveraging ObjCErrorException, the error in the Swift catch block is already an NSError containing the correct error information. We can simply read the data from it, no Error extension required:

do {
    //
} catch {
    let nsError = error as NSError
    print("nsError domain: \(nsError.domain)")
}

This simplifies things for downstream users.

Using this updated approach, here's what I see when purposefully generating an error scenario (calling registerFont() on the same font multiple times):

do {
    try fontResource.registerFont()
    try fontResource.registerFont() // force a failure to demonstrate improved error handling
} catch {
    let nsError = error as NSError
    print("nsError: \(nsError)")
}
nsError: Error Domain=com.apple.CoreText.CTFontManagerErrorDomain Code=105 "Font registration was unsuccessful." UserInfo={NSLocalizedDescription=Font registration was unsuccessful., CTFontManagerErrorFontURLs=(
    "[trimmed]/OpenSans-Regular.ttf"
)}

…tion`

Instead of introducing our own `NSErrorException` to propagate `NSError`s to Swift, we rely on the built-in (albeit _internal_) `ObjCErrorException` via https://github.com/JetBrains/kotlin/blob/b05604058fa3c480030267629cc70cf180a1263d/kotlin-native/runtime/src/main/kotlin/kotlin/native/internal/ObjCExportUtils.kt#L274

Previously, we needed to create a helper on the Swift side of things to extract the nested `NSError` details:

```swift
extension Error {
    var wrappedNSError: NSError? {
        let kotlinException = (self as NSError).userInfo["KotlinException"] as? KotlinException
        let nsErrorException = kotlinException as? NSErrorException
        return nsErrorException?.nsError as? NSError
    }
}
```

And would use the helper extension like:

```swift
do {
    //
} catch {
    if let wrappedError = error.wrappedNSError {
        print("wrappedError domain: \(wrappedError.domain)")
        // etc.
    }
}
```

With this updated approach leveraging `ObjCErrorException`, the error in the Swift catch block is _already_ an `NSError` containing the correct error information. We can simply read the data from it, no `Error` extension required:

```swift
do {
    //
} catch {
    let nsError = error as NSError
    print("nsError domain: \(nsError.domain)")
}
```

This simplifies things for downstream users.
@Alex009 Alex009 added this to the 0.24.0 milestone Apr 18, 2024
@Alex009 Alex009 merged commit 9eb3be1 into icerockdev:develop Apr 19, 2024
24 of 30 checks passed
@darronschall darronschall deleted the remove-unnecessary-nserrorexception-for-apple-register-font branch April 19, 2024 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants