[arkit] Fix crash in ARConfiguration.SupportedVideoFormats. Fixes #5347 #5348
Conversation
…arin#5347 Even if 'supportedVideoFormats' is static the type is abstract. > Important > ARConfiguration is an abstract base class, so its implementation of > this property always returns an empty array. Read this property from > the configuration subclass you plan to use for your AR session, such > as ARWorldTrackingConfiguration or ARFaceTrackingConfiguration. https://developer.apple.com/documentation/arkit/arconfiguration/2942261-supportedvideoformats?language=objc and this behave differently in Objective-C (than .net) as every (static) method will be different (and not a single implementation like C#). The existing implementation (as a property) calls `ARConfiguration` implementation which simply throws a (native) exception > NSInvalidArgumentException Reason: Supported video formats should be called on individual configuration class The solution is to obsolete the property (can't be subclassed in .net since it's static) and add, only on the subclasses, a method that call the 'supportedVideoFormats' selector on the current (not base) type. Added unit test to detect the addition of newer subclasses - since they will also need to expose this method. reference: xamarin#5347
|
Build failure Test results4 tests failed, 0 tests skipped, 77 tests passed.Failed tests
|
There was a problem hiding this comment.
Beautiful, it makes a lot of sense (:
Now 2 questions for you @spouliot
- Why did our tests not catch that crash earlier? Or the native exception
NSInvalidArgumentException Reason: Supported video formats should be called on individual configuration class. Prob something to look into. - I don't know how frequent that case is in our bindings (we could review), of an abstract type with a static method that forces us to move that method in every subclass, but maybe it's worth having a new attribute for that case.
So we'd keep the property in the base class with a special attribute that bookmarks the abstract type and write that property into every subclass (no subclass forgotten, your test won't be needed and no duplicated code). No?
Co-Authored-By: spouliot <sebastien.pouliot@gmail.com>
|
Build failure ✅ Build succeeded |
|
A1. Our existing tests did not caught it because there was no test before I added mine ;-) Do not assume intro runs the generated binding code - most of the generated code has too much prerequisite (e.g. arguments) to automate it. So in most cases intro checks if selectors respond (and it does in this case) and that the signature match (and that was also fine). Now running the code for static properties is possible (no arguments, no instance to create) and worth trying (given time) but it will likely cause a lot of false positives (that will need to be audited and ignored - like many intro tests). Even then (abstract or not) the base (overridden) code might not throw a native exception (first case I see) so it might not find any such case by doing so... A2. it's not frequent - but it happened a few times before (e.g. Intents). It's not really possible to detect from code (at least from a case like this one) since the non-override case is common (and works just like C#). So it falls to the human binder realize the bindings are (or need to be) duplicated in subclasses (either from docs, headers comments...). Also having an attribute work on subclasses is difficult since there's no ordering in generator (i.e. the subclass could already have been generated). |
|
Build success |
Even if 'supportedVideoFormats' is static the type is abstract.
and this behave differently in Objective-C (than .net) as every (static)
method will be different (and not a single implementation like C#).
The existing implementation (as a property) calls
ARConfigurationimplementation which simply throws a (native) exception
The solution is to obsolete the property (can't be subclassed in .net
since it's static) and add, only on the subclasses, a method that
call the 'supportedVideoFormats' selector on the current (not base)
type.
Added unit test to detect the addition of newer subclasses - since they
will also need to expose this method.
reference: #5347