Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
dsmitchell opened this issue Apr 2, 2019 · 3 comments · Fixed by #13607
Labels
breaking-change If an issue or a pull request represents a breaking change bug If an issue is a bug or a pull request a bug fix iOS Issues affecting iOS
Milestone

Comments

@dsmitchell
Copy link

dsmitchell commented Apr 2, 2019

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)
  1. 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
Copy link
Member

There is some history here:

xamarin-macios/src/uikit.cs

Lines 5957 to 5962 in f2f4133

// HACK: those members are not *required* in ObjC but we made them
// abstract to have them inlined in UITextField and UITextView
// Even more confusing it that respondToSelecttor return NO on them
// even if it works in _real_ life (compare unit and introspection tests)
[Protocol]
interface UITextInputTraits {

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.

@rolfbjarne rolfbjarne added this to the XAMCORE_4_0 milestone Apr 2, 2019
@rolfbjarne rolfbjarne added breaking-change If an issue or a pull request represents a breaking change bug If an issue is a bug or a pull request a bug fix external-xamarin-vs Issues affecting the Xamarin in Visual Studio and are not specific to Xamarin.iOS or Xamarin.Mac iOS Issues affecting iOS and removed external-xamarin-vs Issues affecting the Xamarin in Visual Studio and are not specific to Xamarin.iOS or Xamarin.Mac labels Apr 2, 2019
@chamons chamons mentioned this issue Apr 2, 2019
44 tasks
@dsmitchell
Copy link
Author

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
Copy link
Member

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).

rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Dec 20, 2021
…n#5831.

Once upon a long time ago we decided to mark the properties in the
UITextInputTraits protocol as required in our API definition, because that way
we'd inline these properties in any class that implemented the
UITextInputTraits protocol, which made calling these properties much easier.

At a later point, we implemented better support for protocols, and now we
automatically generate extension methods for such properties (a corresponding
Get/Set method for the get/set property accessors), so we don't need these
inlined properties anymore.

However, removing them would be a breaking change, so we were stuck with these
redundant inlined properties, until .NET came along.

Ref: xamarin/maccore@0e80570

Fixes xamarin#5831.
rolfbjarne added a commit that referenced this issue Dec 22, 2021
…#13607)

Once upon a long time ago we decided to mark the properties in the
UITextInputTraits protocol as required in our API definition, because that way
we'd inline these properties in any class that implemented the
UITextInputTraits protocol, which made calling these properties much easier.

At a later point, we implemented better support for protocols, and now we
automatically generate extension methods for such properties (a corresponding
Get/Set method for the get/set property accessors), so we don't need these
inlined properties anymore.

However, removing them would be a breaking change, so we were stuck with these
redundant inlined properties, until .NET came along.

Ref: xamarin/maccore@0e80570

Fixes #5831.
@ghost ghost locked as resolved and limited conversation to collaborators May 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking-change If an issue or a pull request represents a breaking change bug If an issue is a bug or a pull request a bug fix iOS Issues affecting iOS
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants