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

No Replacement for deprecated TraitCollectionDidChange #19410

Closed
tipa opened this issue Nov 8, 2023 · 16 comments · Fixed by #19411
Closed

No Replacement for deprecated TraitCollectionDidChange #19410

tipa opened this issue Nov 8, 2023 · 16 comments · Fixed by #19411
Labels
bug If an issue is a bug or a pull request a bug fix
Milestone

Comments

@tipa
Copy link

tipa commented Nov 8, 2023

I was using traitcollectiondidchange to detect changes of the dark/light theme. That method has been deprecated with iOS 17

Screenshot 2023-11-08 at 1 27 24 PM

The suggested alternative by Apple is registerfortraitchanges, which can be called on objects that implement UITraitChangeObservable (IUITraitChangeObservable interface in C#). UIView and UIViewController seem to implement that interface, but this isn't reflected in the C# bindings - the method RegisterForTraitChanges doesn't exist for UIView and UIViewController. How can I use this new API in .NET?

Screenshot 2023-11-08 at 1 25 32 PM

Environment

.NET 8 RC2.2

@rolfbjarne
Copy link
Member

If you have a UIView or UIViewController instance, you can get an object that implements IUITraitChangeObservable like this:

using ObjCRuntime;

UIView view = ...;
var observable = Runtime.GetINativeObject<IUITraitChangeObservable> (view.Handle, false);
observable.RegisterForTraitChanges (...);

@tipa
Copy link
Author

tipa commented Nov 8, 2023

Thanks. I tried the following but it crashes with a SIGSEGV at RegisterForTraitChanges:

var observable = Runtime.GetINativeObject<IUITraitChangeObservable>(this.Handle, false); // "this" is a UIViewController
observable.RegisterForTraitChanges(new[] { new UITraitUserInterfaceStyle() }, (env, coll) =>
{

});

I am not sure if I translated the UITraitUserInterfaceStyle.self part into C# code correctly.

@rolfbjarne
Copy link
Member

Could you create a test project that shows the crash?

rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Nov 8, 2023
…IWindowScene implement the UITraitChangeObservable protocol. Fixes xamarin#19410.

Fixes xamarin#19410.
@tipa
Copy link
Author

tipa commented Nov 8, 2023

crash.zip

=================================================================
	Native stacktrace:
=================================================================
	0x10c5a61d5 - /Users/timopartl/Library/Developer/CoreSimulator/Devices/C5E39504-07E4-4553-9FD3-20AA0931CCE8/data/Containers/Bundle/Application/5CFBD26F-9ED1-4CAF-8F35-A52D31FD1275/crash.app/libmonosgen-2.0.dylib : mono_dump_native_crash_info
	0x10c544b6e - /Users/timopartl/Library/Developer/CoreSimulator/Devices/C5E39504-07E4-4553-9FD3-20AA0931CCE8/data/Containers/Bundle/Application/5CFBD26F-9ED1-4CAF-8F35-A52D31FD1275/crash.app/libmonosgen-2.0.dylib : mono_handle_native_crash
	0x10c497aff - /Users/timopartl/Library/Developer/CoreSimulator/Devices/C5E39504-07E4-4553-9FD3-20AA0931CCE8/data/Containers/Bundle/Application/5CFBD26F-9ED1-4CAF-8F35-A52D31FD1275/crash.app/libmonosgen-2.0.dylib : mono_sigsegv_signal_handler_debug
	0x121ea55ed - /usr/lib/system/libsystem_platform.dylib : _sigtramp
	0x0 - Unknown
	0x1191dc256 - /Library/Developer/CoreSimulator/Volumes/iOS_21A342/Library/Developer/CoreSimulator/Profiles/Runtimes/iOS 17.0.simruntime/Contents/Resources/RuntimeRoot/System/Library/Frameworks/Foundation.framework/Foundation : NSStringFromClass
	0x130b46536 - /Library/Developer/CoreSimulator/Volumes/iOS_21A342/Library/Developer/CoreSimulator/Profiles/Runtimes/iOS 17.0.simruntime/Contents/Resources/RuntimeRoot/System/Library/PrivateFrameworks/UIKitCore.framework/UIKitCore : _UITraitTokenForTraitLocked
	0x1305ee729 - /Library/Developer/CoreSimulator/Volumes/iOS_21A342/Library/Developer/CoreSimulator/Profiles/Runtimes/iOS 17.0.simruntime/Contents/Resources/RuntimeRoot/System/Library/PrivateFrameworks/UIKitCore.framework/UIKitCore : __64+[UITraitCollection _traitTokensIncludingPlaceholdersForTraits:]_block_invoke
	0x130b450a3 - /Library/Developer/CoreSimulator/Volumes/iOS_21A342/Library/Developer/CoreSimulator/Profiles/Runtimes/iOS 17.0.simruntime/Contents/Resources/RuntimeRoot/System/Library/PrivateFrameworks/UIKitCore.framework/UIKitCore : _UIPerformWithTraitLock
	0x1305ee666 - /Library/Developer/CoreSimulator/Volumes/iOS_21A342/Library/Developer/CoreSimulator/Profiles/Runtimes/iOS 17.0.simruntime/Contents/Resources/RuntimeRoot/System/Library/PrivateFrameworks/UIKitCore.framework/UIKitCore : +[UITraitCollection _traitTokensIncludingPlaceholdersForTraits:]
	0x1301be0e8 - /Library/Developer/CoreSimulator/Volumes/iOS_21A342/Library/Developer/CoreSimulator/Profiles/Runtimes/iOS 17.0.simruntime/Contents/Resources/RuntimeRoot/System/Library/PrivateFrameworks/UIKitCore.framework/UIKitCore : -[UIViewController registerForTraitChanges:withHandler:]
	0x10b96f069 - /Users/timopartl/Library/Developer/CoreSimulator/Devices/C5E39504-07E4-4553-9FD3-20AA0931CCE8/data/Containers/Bundle/Application/5CFBD26F-9ED1-4CAF-8F35-A52D31FD1275/crash.app/libxamarin-dotnet-debug.dylib : xamarin_dyn_objc_msgSend
	0x179ab3c3a - Unknown

@rolfbjarne
Copy link
Member

Our bindings are wrong :/ The first argument to RegisterForTraitChanges is a list of types, not objects.

@rolfbjarne
Copy link
Member

This might work as a workaround (at least it doesn't crash anymore, although the callback isn't called):

	IUITraitChangeRegistration RegisterForTraitChanges (UIViewController vc)
	{
		var traits = new Type [] {
			typeof (UITraitVerticalSizeClass),
			typeof (UITraitHorizontalSizeClass),
		};
		return RegisterForTraitChanges (vc, traits);
	}

	IUITraitChangeRegistration RegisterForTraitChanges (UIViewController vc, params Type[] traits)
	{
		var classes = new Class [traits.Length];
		for (var i = 0; i < classes.Length; i++)
			classes [i] = new Class (traits [i]);
		using var traitArray = NSArray.FromNSObjects (classes);

		var registrationHandle = NativeHandle_objc_msgSend_NativeHandle_NativeHandle_NativeHandle (vc.Handle, Selector.GetHandle ("registerForTraitChanges:withTarget:action:"), traitArray.Handle, this.Handle, Selector.GetHandle ("traitChangeCallback:collection:"));
		return Runtime.GetINativeObject<IUITraitChangeRegistration> (registrationHandle, false);
	}

	[DllImport ("/usr/lib/libobjc.dylib", EntryPoint="objc_msgSend")]
	extern static NativeHandle NativeHandle_objc_msgSend_NativeHandle_NativeHandle_NativeHandle (IntPtr receiver, IntPtr selector, NativeHandle arg1, NativeHandle arg2, NativeHandle arg3);

	[Export ("traitChangeCallback:collection:")]
	void TraitChangeCallback (NSObject sender, UITraitCollection collection)
	{
		Console.WriteLine ($"TraitChangeCallback");
	}

Full AppDelegate.cs file: https://gist.github.com/rolfbjarne/042dc7b97e97c458ae08290a80e301df

Note that more workarounds are likely needed for other API, any new API that takes a UITrait in the signature is likely wrong (we bound using UIKit.IUITraitDefinition, while we should have bound using ObjCRuntime.Class).

@rolfbjarne rolfbjarne added the bug If an issue is a bug or a pull request a bug fix label Nov 9, 2023
@rolfbjarne rolfbjarne added this to the Future milestone Nov 9, 2023
@tipa
Copy link
Author

tipa commented Nov 10, 2023

Thanks a lot! For me, your workaround code works and the callback is called when a trait changes (e.g. dark/light theme switch when using typeof(UITraitUserIntertfaceStyle). Or when the device is rotated in your example with typeof(UITraitVerticalSizeClass)

rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Nov 14, 2023
…IWindowScene implement the UITraitChangeObservable protocol. Fixes xamarin#19410.

Fixes xamarin#19410.
@marcoburato-ecutek
Copy link

I'm also affected.

Once the bindings are fixed, what would be the proper way to both use traitCollectionDidChange for iOS < 17 and registerForTraitChanges for iOS >= 17?

We could override traitCollectionDidChange and, after calling the base implementation, only invoke our logic on iOS < 17. However, calling the base implementation would trigger the deprecation warning. So, it looks like we'd have to disable the warning on that piece of code.

@tipa
Copy link
Author

tipa commented Nov 29, 2023

@marcoburato-ecutek I did something like this (simply not calling the base implementation):

public override void TraitCollectionDidChange(UITraitCollection oldTraits)
{
        if (OperatingSystem.IsIOSVersionAtLeast(17)) return;
        base.TraitCollectionDidChange(oldTraits);
        if (TraitCollection.UserInterfaceStyle != oldTraits.UserInterfaceStyle) { UpdateTheme(); }
}

@marcoburato-ecutek
Copy link

@tipa I don't think that's a good idea. We don't know what the base implementation does, not calling it may or may not cause problems.

For instance, Apple's documentation says:

At the beginning of your implementation, call super to ensure that interface elements higher in the view hierarchy have an opportunity to adjust their layout first.

I imagine that, if there are nested view controllers (basically always the case with UIKit nav controller, tab controller, etc) the parent could not get notified if it still relies on the deprecated method.

I'd rather be safe and always call the base method.

rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Dec 7, 2023
…IWindowScene implement the UITraitChangeObservable protocol. Fixes xamarin#19410.

Fixes xamarin#19410.
rolfbjarne added a commit that referenced this issue Jan 3, 2024
There are a few issues with the bindings, because the typedef `UITrait`
in the headers is defined like this:

```objective-c
typedef Class<UITraitDefinition> UITrait
```

which means: "A Class that implements the UITraitDefinition protocol",
and not "The UITraitDefinition" protocol", which is how it was bound.

This means the corresponding bindings are incorrect, so fix them. In
some cases it's not possible to fix the API, so new ones had to be
implemented in order to maintain backwards compatibility.

Fixes #19410.
vs-mobiletools-engineering-service2 pushed a commit to vs-mobiletools-engineering-service2/xamarin-macios that referenced this issue Jan 3, 2024
…IWindowScene implement the UITraitChangeObservable protocol. Fixes xamarin#19410.

Fixes xamarin#19410.
@priyank459
Copy link

Hi @rolfbjarne ,
Along with registerForTraitChanges I also need to use viewDidLayoutSubviews.
Can you please help me with exporting logic for the same.

Thanks for the workaround on registerForTraitChanges , it helped me to solve part of the issue.

@rolfbjarne
Copy link
Member

@priyank459 we've already bound [UIViewController viewDidLayoutSubviews] as UIViewController.ViewDidLayoutSubviews, so I'm not sure what you need? Can you show a code sample of what doesn't work?

dalexsoto pushed a commit that referenced this issue Jan 4, 2024
…#19410. (#19730)

There are a few issues with the bindings, because the typedef `UITrait`
in the headers is defined like this:

```objective-c
typedef Class<UITraitDefinition> UITrait
```

which means: "A Class that implements the UITraitDefinition protocol",
and not "The UITraitDefinition" protocol", which is how it was bound.

This means the corresponding bindings are incorrect, so fix them. In
some cases it's not possible to fix the API, so new ones had to be
implemented in order to maintain backwards compatibility.

Fixes #19410.


Backport of #19411

---------

Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
@priyank459
Copy link

Thank you @rolfbjarne and @dalexsoto. I am able to solve my issue.

@tipa
Copy link
Author

tipa commented Jan 18, 2024

I just updated to the new version and observing a probably unintended behavior. Something like this (written in a Viewcontroller or View) fails to build:

RegisterForTraitChanges<UITraitUserInterfaceStyle>((env, traits) => { });

Screenshot 2024-01-18 at 11 12 18 AM

I need to specifically cast to IUITraitChangeObservable in order to get it to work

((IUITraitChangeObservable)this).RegisterForTraitChanges<UITraitUserInterfaceStyle>((env, traits) => { });

Probably not obvious to new developers. Wouldn't it be best to just remove the old methods, which were non-functional anyways? It would be a breaking change API-wise but I doubt that any developers used those methods as they were crashing

@rolfbjarne
Copy link
Member

I just updated to the new version and observing a probably unintended behavior. Something like this (written in a Viewcontroller or View) fails to build:

RegisterForTraitChanges<UITraitUserInterfaceStyle>((env, traits) => { });

Screenshot 2024-01-18 at 11 12 18 AM

I need to specifically cast to IUITraitChangeObservable in order to get it to work

((IUITraitChangeObservable)this).RegisterForTraitChanges<UITraitUserInterfaceStyle>((env, traits) => { });

Good catch, I think I know how to fix this.

Probably not obvious to new developers. Wouldn't it be best to just remove the old methods, which were non-functional anyways? It would be a breaking change API-wise but I doubt that any developers used those methods as they were crashing

We try very hard to not make breaking changes, because we've run into trouble before removing APIs that were 100% incorrect and never called (because it would crash the process). It turned out third-party libraries had references to said API in code paths that were never executed - and when we removed that API, we suddently got a lot of bug reports about breaking the build for consumers of the third-party library - it turned out the linker will fail if running into a reference to a method that doesn't exist.

In any case it doesn't look like removing the old API will fix the problem, because this isn't a failure to find a specific method overload - the problem is that methods on an interface are not directly accessible from types that implement that interface. The fix would be to add proxy implementations on all the types that implement the interface.

@rolfbjarne
Copy link
Member

rolfbjarne commented Mar 6, 2024

In any case it doesn't look like removing the old API will fix the problem, because this isn't a failure to find a specific method overload - the problem is that methods on an interface are not directly accessible from types that implement that interface. The fix would be to add proxy implementations on all the types that implement the interface.

For several reasons this turned out to be cumbersome to implement at this point, but it looks like it'll be much easier once we add support for default interface members in the generator (#13294), so I'm postponing until then.

I've filed #20265 to keep track of it.

rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Mar 6, 2024
…that implement it.

This also required fixing a bug in the static registrar where we'd ignore the
generic parameters in a method when looking for methods implementing an
interface.

Ref: xamarin#19410 (comment)
rolfbjarne added a commit that referenced this issue Nov 21, 2024
…that implement it. Fixes #20265.

This also required fixing:

* A bug in the static registrar where we'd ignore the generic parameters in a method
  when looking for methods implementing an interface.

* A bug in the generator where we'd throw You_Should_Not_Call_base_In_This_Method()
  in some cases where we shouldn't.

Also:

* Enable nullability and fix any resulting issues.
* Clean up some legacy code we don't need anymore.

Ref: #19410 (comment)

Fixes #20265.
mandel-macaque pushed a commit that referenced this issue Nov 26, 2024
…that implement it. Fixes #20265. (#21676)

This also required fixing:

* A bug in the static registrar where we'd ignore the generic parameters
in a method
  when looking for methods implementing an interface.

* A bug in the generator where we'd throw
You_Should_Not_Call_base_In_This_Method()
  in some cases where we shouldn't.

Also:

* Enable nullability and fix any resulting issues.
* Clean up some legacy code we don't need anymore.

Ref:
#19410 (comment)

Fixes #20265.

---------

Co-authored-by: GitHub Actions Autoformatter <github-actions-autoformatter@xamarin.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug If an issue is a bug or a pull request a bug fix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants