- Notifications
You must be signed in to change notification settings - Fork548
[coretext] Update for Xcode 11 beta 1-3#6562
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
includes unit tests
monojenkins commentedJul 11, 2019
Build failure Test results14 tests failed, 77 tests passed.Failed tests
|
| vardescriptor=(BlockLiteral*)block; | ||
| vardel=(CTFontManagerRequestFontsHandler)(descriptor->Target); | ||
| if(del!=null) | ||
| del(NSArray.ArrayFromHandle<CTFontDescriptor>(fontDescriptors)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I wonder if we would like to clean (release) theCTFontDescriptors individually after callingdel with them, at least it is not aCMSampleBuffer [] :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
code can keep reference to them, e.g in fieldsCMSampleBuffer is not a good example and requires extra care from callers (no nice way to handle it)
| staticexternunsafevoidCTFontManagerRegisterFontURLs(/* CFArrayRef */IntPtrfontUrls,CTFontManagerScopescope,boolenabled,void*registrationHandler); | ||
| [Watch(6,0),TV(13,0),Mac(10,15,onlyOn64:true),iOS(13,0)] | ||
| publicstaticvoidRegisterFonts(NSUrl[]fontUrls,CTFontManagerScopescope,boolenabled,CTFontRegistrationHandlerregistrationHandler) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The block code can be simplified somewhat with this pattern:https://github.com/xamarin/xamarin-macios/blob/c99ad292b2e357f4f2acb083e03281a323da44a2/src/CoreFoundation/DispatchIO.cs#L56-L84
Here it would become something like this:
[MonoPInvokeCallback(typeof(InnerRegistrationHandler))]staticboolTrampolineRegistrationHandler(IntPtrblock,/* NSArray */IntPtrerrors,booldone){vardel=BlockLiteral.GetTarget<CTFontRegistrationHandler>(block);returndel!=null?del(NSArray.ArrayFromHandle<NSError>(errors),done):true;}[Watch(6,0),TV(13,0),Mac(10,15,onlyOn64:true),iOS(13,0)][DllImport(Constants.CoreTextLibrary)]staticexternvoidCTFontManagerRegisterFontURLs(/* CFArrayRef */IntPtrfontUrls,CTFontManagerScopescope,boolenabled,refBlockLiteralregistrationHandler);[Watch(6,0),TV(13,0),Mac(10,15,onlyOn64:true),iOS(13,0)]publicstaticvoidRegisterFonts(NSUrl[]fontUrls,CTFontManagerScopescope,boolenabled,CTFontRegistrationHandlerregistrationHandler){using(vararr=EnsureNonNullArray(fontUrls,nameof(fontUrls))){if(registrationHandler==null){CTFontManagerRegisterFontURLs(arr.Handle,scope,enabled,null);return;}BlockLiteralblock_handler=newBlockLiteral();block_handler.SetupBlockUnsafe(callback,registrationHandler);CTFontManagerRegisterFontURLs(arr.Handle,scope,enabled,refblock_handler);block_ptr.CleanupBlock();}}
No unsafe code needed, and it's shorter, so easier to review and fewer lines to make mistakes in.
src/CoreText/CTFontManager.cs Outdated
| } | ||
| finally{ | ||
| // Copy/Create rule - we must release the CFArrayRef | ||
| CFObject.CFRelease(p); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IIRC this will crash ifp is IntPtr.Zero.
src/CoreText/CTFontManager.cs Outdated
| if(p==IntPtr.Zero) | ||
| returnnull; | ||
| // Copy/Create rule - dont't retain it inside the .ctor | ||
| returnnewCTFontDescriptor(p,owns:false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If you don't want to retain it inside the .ctor, you must passowns: true
src/CoreText/CTFontManager.cs Outdated
| } | ||
| finally{ | ||
| // Copy/Create rule - we must release the CFArrayRef | ||
| CFObject.CFRelease(p); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Crash ifp == IntPtr.Zero.
monojenkins commentedJul 12, 2019
Build failure Test results9 tests failed, 82 tests passed.Failed tests
|
| <ContentInclude="..\monotouch-test\Resources\fragmentShader.metal"> | ||
| <Link>Resources\fragmentShader.metal</Link> | ||
| </Content> | ||
| <ContentInclude="..\monotouch-test\Pacifico.ttf"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
These two files are already in the csproj, so this looks redundant.
- some (old) differences on macOS where not tested properly- tvOS and watchOS crash/hangs with `CTFontManagerRegisterFontDescriptors` (works on iOS)```The operation couldn’t be completed. (com.apple.CoreText.CTFontManagerErrorDomain error 500.```
monojenkins commentedJul 15, 2019
Build success |
includes unit tests