xamarin / xamarin-macios

.NET for iOS, Mac Catalyst, macOS, and tvOS provide open-source bindings of the Apple SDKs for use with .NET managed languages such as C#
Other
2.48k stars 514 forks source link

IUITextInputTraits incorrectly marks members as "IsRequired" when in reality they are all optional #5831

Closed dsmitchell closed 2 years ago

dsmitchell commented 5 years ago

Steps to Reproduce

  1. In Visual Studio, go to definition of UIKit's IUITextInputTraits (see that several members are marked IsRequired=true)
  2. In Xcode, go to definition of UITextInputTraits.h (see that all members are marked @optional)
  3. Note that IUIKeyInput, which inherits from IUITextInputTraits, has 3 required members. But that shouldn't affect IUITextInputTraits

Expected Behavior

When implementing IUIKeyInput in a custom UIControl, I expect not to be forced to implement placeholders for all members of IUITextInputTraits

Actual Behavior

I am required to implement placeholder values, and force the OS to query my class for values I do not intend to override

Note that by forcing me to implement these members, NSObject's respondsToSelector will also return 'true' instead of 'false', which means that the OS, subclasses of my class, or other code that examines the class, may behave differently as a result

Environment

=== Visual Studio Community 2017 for Mac ===

Version 7.8.3 (build 2)
Installation UUID: a6743886-21f4-4c35-abcc-53881466b920
    GTK+ 2.24.23 (Raleigh theme)
    Xamarin.Mac 5.0.0.0 ( / b40230c0)

    Package version: 516010000

=== Mono Framework MDK ===

Runtime:
    Mono 5.16.1.0 (2018-06/a76b50e5faa) (64-bit)
    Package version: 516010000

=== NuGet ===

Version: 4.8.2.5835

=== .NET Core ===

Runtime: /usr/local/share/dotnet/dotnet
Runtime Versions:
    2.1.9
    2.1.8
    2.1.7
    2.1.2
    2.1.1
SDK: /usr/local/share/dotnet/sdk/2.1.505/Sdks
SDK Versions:
    2.1.505
    2.1.504
    2.1.503
    2.1.302
    2.1.301
MSBuild SDKs: /Library/Frameworks/Mono.framework/Versions/5.16.1/lib/mono/msbuild/15.0/bin/Sdks

=== Xamarin.Profiler ===

Version: 1.6.4
Location: /Users/microsoft/Applications/Xamarin Profiler.app/Contents/MacOS/Xamarin Profiler

=== Updater ===

Version: 11

=== Apple Developer Tools ===

Xcode 10.2 (14490.120)
Build 10E125

=== Xamarin.Mac ===

Version: 5.2.1.16 (Visual Studio Community)
Hash: 2dc06c71
Branch: 
Build date: 2019-02-12 23:09:50-0500

=== Xamarin.iOS ===

Version: 12.2.1.16 (Visual Studio Community)
Hash: 2dc06c71
Branch: d15-9
Build date: 2019-02-12 23:09:50-0500

=== Build Information ===

Release ID: 708030002
Git revision: fd02e670fdd6b101bc9c08b1cc5b7710d9f58cd8
Build date: 2019-03-08 10:30:21+00
Build branch: release-7.8
Xamarin extensions: 1f72f0a3737128552336f27e189d3c4f0cdebd00

=== Operating System ===

Mac OS X 10.14.4
Darwin 18.5.0 Darwin Kernel Version 18.5.0
    Mon Mar 11 20:40:32 PDT 2019
    root:xnu-4903.251.3~3/RELEASE_X86_64 x86_64

Build Logs

No build logs necessary to see this problem

Example Class

public class CustomControl : UIControl, IUIKeyInput
{
    public CustomControl(IntPtr handle) : base(handle) { }

    public string Text { get; set; }

    // NOTE: Xamarin SDK has incorrectly marked these as required. So for now we have to implement some basic values
    public UITextAutocapitalizationType AutocapitalizationType { get; set; } = UITextAutocapitalizationType.None;
    public UITextAutocorrectionType AutocorrectionType { get; set; } = UITextAutocorrectionType.No;
    public UIKeyboardType KeyboardType { get; set; } = UIKeyboardType.Default;
    public UIKeyboardAppearance KeyboardAppearance { get; set; } = UIKeyboardAppearance.Default;
    public UIReturnKeyType ReturnKeyType { get; set; } = UIReturnKeyType.Default;
    public bool EnablesReturnKeyAutomatically { get; set; } = true;
    public bool SecureTextEntry { get; set; } = false;
    public UITextSpellCheckingType SpellCheckingType { get; set; } = UITextSpellCheckingType.No;

    // These are correctly marked required as part of IUIKeyInput
    public bool HasText
    {
        get => !string.IsNullOrEmpty(Text);
    }

    public void InsertText(string text)
    {
        Text += text;
    }

    public void DeleteBackward()
    {
        if (Text.Length > 0)
        {
            Text = Text.Substring(0, Text.Length - 1);
        }
    }
}

VS bug #835770

rolfbjarne commented 5 years ago

There is some history here: https://github.com/xamarin/xamarin-macios/blob/f2f41334f4d5c773ebccd0763c1aec515c7ac1c6/src/uikit.cs#L5957-L5962

Looking further back, I ran into https://github.com/xamarin/maccore/commit/0e8057086362f7f9f22b68b9d174e8568b11bf7b. This is from when the source code was private, but this is the commit message:

UITextInputTraits: ignore Apple's optional settings, and force the methods as required

While this does not match Apple's definition, the situation is this:

    * Client code expects to be able to call the methods in this
          class, and since a lot of them are properties and we lack
          extension properties, we do not have a way of surfacing the API.

    * There is no public code that adopts this protocol on GitHub,
          or anywhere that I could find, it is as far as I can tell a
          protocol that is accessed, but never implemented.

For our users, the downside is that we would be forcing them to
implement more methods than they would have wanted to when adopting
the IUITextInputTraits C# interface (UITextInputTraits protocol) by
forcing a number of methods to be implemented.

But this basically does not seem to happen in the wild, so the risk of
annoying someone is minimal, while the benefits are large.

This is from August 2014, so things have obviously changed since then.

The problem is that changing this now is not possible, since it would be a breaking change, and we need a very good reason to do breaking changes (this doesn't fulfill those requirements).

However, we might have a chance to do breaking changes in the future, so I'm keeping this open so that we can evaluate at that time if we can/want to do this change or not.

dsmitchell commented 5 years ago

Thanks for the quick update. While this case may be mostly benign, there are examples in macOS/iOS where not marking optional methods and properties correctly would lead to undesirable behavior with the underlying framework.

For example, NSFetchedResultsController will assume you want to enable change tracking the moment your class implements at leasts one of four particular optional methods in NSFetchedResultsControllerDelegate. If those had been marked incorrectly (thankfully they're not) you'd be stuck dealing with a datasource that wants to change the UI when the rest of your code may not

rolfbjarne commented 5 years ago

Yes, we know that some API really must be optional, since its presence is checked at runtime and can cause difference in behavior, so with any new bindings we'll bind it correctly. In this particular case the binding is almost 5 years old, and at that time our support for protocols wasn't as complete as it's today, so we had to find non-optimal solutions in some cases.

On another note, Apple has also changed the required-ness of API, making previously required API optional (and vice versa), and in that case we can't update our bindings since it would be a breaking change (work is in progress to fix this, but it requires updates to the runtime and the C# compiler, so it takes a while to complete).