Extract CallbackBridge from Document.

[jverkoey]

This new class enables a generalized mechanism for connecting Objective-C and Swift types to GB_gameboy_t's callback mechanisms. It has been extracted from Document.m, and Document.m now uses this type to facilitate all callbacks.

Also as part of this change:

• I've verified that no remaining references to GB_get_user_data exist outside of CallbackBridge.m, as the GB_get_user_data value has changed from a Document to a CallbackBridge.
• I've made an effort to document the events and their expected behaviors + implementation considerations.

This change reduces the size of Document.m by 104 lines (from 2040 to 1936).

TODO

• CallbackBridgeDelegate and other exported symbols should have the GB prefix (e.g. GBCallbackBridgeDelegate). The Document class is an exception due to an Apple convention.
• gotNewSample is actually optional, it will only be called if you first set the sample rate
• initWithGB should be initWithGameBoy
• The { following a method definition belongs on its own line
• I don't think connectPrinter makes sense as a bridge API
• connectLinkCableWithPartner should be enableSerialCallbacks instead and should not have a partner parameter, as a Link Cable between two Game Boys is just one of many use cases of the serial API. Document.m should just call enableSerialCallbacks for both the master and slave Game Boys.

Part of #324

[LIJI32]

That's a good step in cleaning up the mess that is Document.m, thanks!

Few notes, mostly affecting coding style:

• CallbackBridgeDelegate and other exported symbols should have the GB prefix (e.g. GBCallbackBridgeDelegate). The Document class is an exception due to an Apple convention.
• nonnull and nullable should not be used – nullability is explicitly not used in SameBoy, because the Objective-C implementation of this feature encourages bad practices such as dangerous casting, while not providing much stability or security benefits and needlessly making the code awkward and longer
• gotNewSample is actually optional, it will only be called if you first set the sample rate
• initWithGB should be initWithGameBoy
• I don't think connectPrinter makes sense as a bridge API, connectLinkCableWithPartner should be enableSerialCallbacks instead and should not have a partner parameter, as a Link Cable between two Game Boys is just one of many use cases of the serial API. Document.m should just call enableSerialCallbacks for both the master and slave Game Boys.
• The { following a method definition belongs on its own line

[jverkoey]

CallbackBridgeDelegate and other exported symbols should have the GB prefix (e.g. GBCallbackBridgeDelegate). The Document class is an exception due to an Apple convention.

Ah! I was wondering about that; thank you for confirming. I will update to follow the standard prefix conventions here. Added a TODO.

[jverkoey]

nonnull and nullable should not be used – nullability is explicitly not used in SameBoy, because the Objective-C implementation of this feature encourages bad practices such as dangerous casting, while not providing much stability or security benefits and needlessly making the code awkward and longer

Is this something you'd be open to discussing further?

I'm using some of these APIs from Swift code and the nullability annotations make a big difference in ergonomics there. I've found that annotating the nullability of some of these APIs also helped better clarify the expected behavior (e.g. NULL has special meaning in some cases that isn't immediately apparent from the method signature alone).

I'd love to learn more about "bad practices such as dangerous casting" because this conflicts with Objective-C/Swift interop.

[jverkoey]

• gotNewSample is actually optional, it will only be called if you first set the sample rate
• initWithGB should be initWithGameBoy
• The { following a method definition belongs on its own line

Added TODOs. Thank you!

[jverkoey]

• I don't think connectPrinter makes sense as a bridge API, connectLinkCableWithPartner should be enableSerialCallbacks instead and should not have a partner parameter, as a Link Cable between two Game Boys is just one of many use cases of the serial API. Document.m should just call enableSerialCallbacks for both the master and slave Game Boys.

Ah this is good to know, I've added a TODO to clean this up.

[jverkoey]

I don't think connectPrinter makes sense as a bridge API

Agreed that this API felt a bit out of place. I've replaced the method with an externalized C callback that can be passed directly to GB_connect_printer instead. Let me know what you think.

[jverkoey]

Gently checking in here to see if there's anything remaining for me to clean up.

[LIJI32]

Sorry I haven't replied, I didn't have time working on SameBoy the last few weeks, I'll go over the PR again in a few days.

[jverkoey]

No worries! Thank you for the great feedback :)

[LIJI32]

Finally had time to go over the code, sorry for the delay! Other than my issue with nullability, everything looks fine.

My issues with with nullability are:

1. Objective-C provides no standard way to cast away nullable, leading to dangerous and messy casts all over the code when nullability is used. For example, if you have a nullable NSString *, and you already know it's non-null, you need to cast it like (NSString *)var to use it as a nonnull argument to method. The problem is that if var ever changes its typing, that cast will silent any type mismatch warnings and errors.
2. Objective-C's handling of nil is very different from how Swift handles it, making the already minimal benefits of null-safety have even less of an impact while potentially hiding pitfalls regarding Objective-C's handling of nil

For that reason I avoid nullability in my Objective-C codebases. Now, I understand this only affects the Swift integration but I'm afraid this will encourage further use of nullability in future 3rd-party contributions and forks.

My familiarity with Swift is fairly limited (I've mostly reversed Swift code and that was painful enough ><), but is there a way to handle this issue in the Swift bridging mechanism rather than the Objective-C headers? If that's not possible I'd settle for a comment in these two affected files explaining these are merely for allowing integration of this object into Swift codebase.