[Fix] Add missing selectors for NSExpression. #57
Conversation
mandel-macaque
changed the title
| @@ -3153,14 +3153,14 @@ public interface NSException : NSCoding, NSCopying { | |||
| } | |||
|
|
|||
| public delegate void NSExpressionHandler (NSObject evaluatedObject, NSExpression [] expressions, NSMutableDictionary context); | |||
|
|
|||
mandel-macaque
force-pushed
the
bug-35012
branch
3 times, most recently
from
f98e34b
to
05dcd9b
Compare
| namespace XamCore.Foundation { | ||
|
|
||
| public partial class NSExpression { | ||
| bool hasFunction = false; |
There was a problem hiding this comment.
The problem with managed state is that it might not be set if the instance is surfaced from native code, i.e. when using the .ctor(IntPtr). In that case it would mean the Block would be null when it should not be.
From your commit message I assume that calling Block crash unless there's one ?
There was a problem hiding this comment.
Yes, not having a flag will crash the app.
|
Given that the API could not have worked with that return, I think we can make the change in the function declaration. But the delegate might have been used for other purposes, so I suggest we introduce a new delegate name, and use that new delegate name in the public API. |
|
Pretty much every time we think it's OK to break binary compatibility we end up bitten, so imho it would be best to maintain it. In particular in this case it's fairly easy: just add a new delegate with the correct name, and for the function with the return value, bind it as a different name (and make it |
|
Lets add the new methods and add a deprecation warning to all of the ones that are incorrect. Does that sound good? |
mandel-macaque
force-pushed
the
bug-35012
branch
5 times, most recently
from
8a9b9ef
to
f62faf4
Compare
|
This is ready to be merge (after any possible issues you might see). The tests show the types and the supported properties per type. I could not reproduce the creation of a Conditional or a Subquery expression so we might have a bug in one of the properties, but it is a huge improvement compared to what we had in the binding. |
|
Build success |
1 similar comment
|
Build success |
| NSExpression ExpressionValueWithObject (NSObject object1, NSMutableDictionary context); | ||
|
|
||
| [Export ("expressionValueWithObject:context:")] | ||
| NSObject EvaluateWith ([NullAllowed] NSObject object1, [NullAllowed] NSMutableDictionary context); |
There was a problem hiding this comment.
object1 is kind of ugly, maybe use @object or obj instead.
There was a problem hiding this comment.
Just followed the present pattern, but sure.
|
@mandel-macaque you told me it's missing a few corner cases right ? Can you list them in TODO comments, maybe in |
Added the missing static factory methods and the missing property. In order to give a clean API a new flag was added to the NSExpression class to track if the Block property does return a block or a null ptr. The idea is to avoid user from seeing an obj-c exception. This commit fixes bug #35012: https://bugzilla.xamarin.com/show_bug.cgi?id=35012 The evaluation of the NSExpression and the defition of the NSExpressionHandler have also been fixed since both should be using NSObjects.
|
LGTM, please double check |
|
Build success |
New commits in spouliot/Touch.Unit: * xamarin/Touch.Unit@6c5bb93 [Touch.Client] Add net5 project files. (xamarin#65) * xamarin/Touch.Unit@ef92ff9 Add tvOS version of Touch.Client. (xamarin#64) * xamarin/Touch.Unit@705964d [Touch.Client] Add a client that uses MonoTouch.Dialog and NUnitLite from NuGet. (xamarin#63) * xamarin/Touch.Unit@fbf9f30 Port Touch.Unit to the Unified profile, and remove the NUnitLite tests from the monotouch.tests project. (xamarin#62) * xamarin/Touch.Unit@dfbf8cb [TouchRunner] Update the current element even if showing a sub element with failure info. (xamarin#61) * xamarin/Touch.Unit@d5b1caa [TouchRunner] TestFinished may be called on a background thread, so make sure to execute any UI logic on the main thread. (xamarin#60) * xamarin/Touch.Unit@ba1196f Convert existing projects to Xamarin.iOS/Unified. (xamarin#58) * xamarin/Touch.Unit@f6958a2 Remove code to be compatible with MonoTouch (Classic Xamarin.iOS). (xamarin#59) * xamarin/Touch.Unit@6a10d44 Remove CLSCompliant attribute which causes compiler warnings. (xamarin#57) Diff: https://github.com/spouliot/Touch.Unit/compare/9db795d50d9fe4ac5df77d3f0d85c1b84d32ce8c..6c5bb930b35cf326d1f650eac0a5ac9e679a4e09
New commits in spouliot/Touch.Unit: * xamarin/Touch.Unit@6c5bb93 [Touch.Client] Add net5 project files. (xamarin#65) * xamarin/Touch.Unit@ef92ff9 Add tvOS version of Touch.Client. (xamarin#64) * xamarin/Touch.Unit@705964d [Touch.Client] Add a client that uses MonoTouch.Dialog and NUnitLite from NuGet. (xamarin#63) * xamarin/Touch.Unit@fbf9f30 Port Touch.Unit to the Unified profile, and remove the NUnitLite tests from the monotouch.tests project. (xamarin#62) * xamarin/Touch.Unit@dfbf8cb [TouchRunner] Update the current element even if showing a sub element with failure info. (xamarin#61) * xamarin/Touch.Unit@d5b1caa [TouchRunner] TestFinished may be called on a background thread, so make sure to execute any UI logic on the main thread. (xamarin#60) * xamarin/Touch.Unit@ba1196f Convert existing projects to Xamarin.iOS/Unified. (xamarin#58) * xamarin/Touch.Unit@f6958a2 Remove code to be compatible with MonoTouch (Classic Xamarin.iOS). (xamarin#59) * xamarin/Touch.Unit@6a10d44 Remove CLSCompliant attribute which causes compiler warnings. (xamarin#57) Diff: https://github.com/spouliot/Touch.Unit/compare/9db795d50d9fe4ac5df77d3f0d85c1b84d32ce8c..6c5bb930b35cf326d1f650eac0a5ac9e679a4e09
New commits in spouliot/Touch.Unit: * xamarin/Touch.Unit@6c5bb93 [Touch.Client] Add net5 project files. (#65) * xamarin/Touch.Unit@ef92ff9 Add tvOS version of Touch.Client. (#64) * xamarin/Touch.Unit@705964d [Touch.Client] Add a client that uses MonoTouch.Dialog and NUnitLite from NuGet. (#63) * xamarin/Touch.Unit@fbf9f30 Port Touch.Unit to the Unified profile, and remove the NUnitLite tests from the monotouch.tests project. (#62) * xamarin/Touch.Unit@dfbf8cb [TouchRunner] Update the current element even if showing a sub element with failure info. (#61) * xamarin/Touch.Unit@d5b1caa [TouchRunner] TestFinished may be called on a background thread, so make sure to execute any UI logic on the main thread. (#60) * xamarin/Touch.Unit@ba1196f Convert existing projects to Xamarin.iOS/Unified. (#58) * xamarin/Touch.Unit@f6958a2 Remove code to be compatible with MonoTouch (Classic Xamarin.iOS). (#59) * xamarin/Touch.Unit@6a10d44 Remove CLSCompliant attribute which causes compiler warnings. (#57) Diff: https://github.com/spouliot/Touch.Unit/compare/9db795d50d9fe4ac5df77d3f0d85c1b84d32ce8c..6c5bb930b35cf326d1f650eac0a5ac9e679a4e09
Added the missing static factory methods and the missing property. In
order to give a clean API a new flag was added to the NSExpression class
to track if the Block property does return a block or a null ptr. The
idea is to avoid user from seeing an obj-c exception.
This commit fixes bug #35012:
https://bugzilla.xamarin.com/show_bug.cgi?id=35012