Fix default values for touchbar APIs in NSTextViewDelegate.

[filipnavara]

Fixes #3824.

[chamons]

build

[chamons]

This looks like as reasonable change, the header file / documentation doesn't state either way and looks like our initial binding got it wrong. I'm somewhat surprised that it worked when I originally bound/tested it, but adding additional asserts is something that happens.

@spouliot - Can you confirm this is not an API breaking change? I'm pretty sure it is, but can't keep the rules in my head.

[chamons]

👍 conditionally on safety of default value change is confirmed and tests pass.

[chamons]

By the way - 🎉 thanks for the contribution!

[monojenkins]

Build success
Build comment file:

✅ Provisioning succeeded
✅ Build succeeded
✅ API Diff (from stable)
✅ API Diff (from PR only)
✅ Generator Diff
✅ Test run succeeded

---

[spouliot]

The attributes used do not change the public API, only the generated code.

@chamons you made a sample (or a test case) for the Touch Bar right ? can it be used/updated to verify the changes ?

[chamons]

Yeah, i know the attributes change the gen'ed code. My question was changing the default considered an "implicit" API break, and it sounds like the answer is no.

I hacked up this example:

https://github.com/xamarin/mac-samples/tree/master/TouchBarExample/TouchBarExample

however, it uses none of the affected APIs.

It would likely be an hour or so to extend it to to use the APIs in question. Thoughts?

[chamons]

We had QA effort in porting the "large" Apple sample, but I don't believe it was ever completed. :(

[filipnavara]

As long as you are fine with a sample and not an automated unit test then basically any app with NSTextView (and any registered event) should do. We only stumbled upon it because it was consistently crashing our app.

[dalexsoto]

I think I am fine with these changes, it does not break public API and while it is undocumented it makes sense this way. That said +1 if we can just verify that we won't throw an exception for not providing a DefaultValue but my gut says we won't.

[chamons]

So I hacked up a little test, and the only delegate method that i can confirm will not accept nil is:

- (NSArray<NSTouchBarItemIdentifier> *)textView:(NSTextView *)textView shouldUpdateTouchBarItemIdentifiers:(NSArray<NSTouchBarItemIdentifier> *)identifiers NS_AVAILABLE_MAC(10_12_2);

which gives:

2018-04-03 14:16:02.155 NSTextViewCrash[5362:421806] *** Assertion failure in -[NSTouchBar setDefaultItemIdentifiers:], /BuildRoot/Library/Caches/com.apple.xbs/Sources/AppKit/AppKit-1561.40.106/TouchBar.subproj/NSTouchBar.m:163

You can see this in this example: https://gist.github.com/chamons/91de78a7b60d1e3281aeb39ffedd044b

I am uncertain about the other three changes looking closer.

• textView:candidatesForSelectedRange accepts nil and header says "Returning nil from this delegate method allows NSTextView to query candidates from NSSpellChecker" and is marked up with nullable NSArray *
• textView:candidates:forSelectedRange: is not marked up with nullable NSArray * but does not assert or appear to misbehave if you return null
• textView:shouldSelectCandidateAtIndex doesn't care either way true of false returned.

[filipnavara]

The other three methods work with the default values that are currently in AppKit.cs, but at the same time it kills the default touch bar behaviour. When the methods are not implemented (eg. NoDefaultValue) then the touch bar contains an emoticon selection and spell checker corrections. When they are implemented the touch bar is empty.

Note that the whole interface was extended in macOS 10.12.x with the addition of touch bar support. All applications that were created before simply didn't have the selectors implemented because they didn't exist yet. The system provides its own default implementation.

[chamons]

When they are implemented the touch bar is empty. - I missed that when originally testing.