unoplatform / uno

Open-source platform for building cross-platform native Mobile, Web, Desktop and Embedded apps quickly. Create rich, C#/XAML, single-codebase apps from any IDE. Hot Reload included! 90m+ NuGet Downloads!!
https://platform.uno
Apache License 2.0
8.89k stars 720 forks source link

Ditch Android & iOS control constructors #1385

Closed weitzhandler closed 5 years ago

weitzhandler commented 5 years ago

What would you like to be added:

The Uno runtime, generates special constructors for each control in Android and iOS heads. Those constructors tunnel down to the base constructors and requires such to be present.

I'm talking about these constructors (see actual example in the RxUI library here):

//Android
protected ReactivePage(IntPtr javaReference, Android.Runtime.JniHandleOwnership transfer)
    : base(javaReference, transfer)
{
    AttachHandlers();
}

//iOS
protected ReactivePage(IntPtr handle)
    : base(handle)
{
    AttachHandlers();
}

When implementing a control library for Uno, even such that has all its controls based on Uno.UI, this can become tedious because it then those generators are not around.

Why is this needed:

My suggestion, is that it might be good if instead of these constructors, we can have another means of injecting those important stuff so it's easier to port UWP libraries to uno by just adding Uno.UI and have the control depend on it. I know it tunnels down to mono, but maybe there can be a way to have those resolved statically in one of the low-level types.

For which Platform:

davidjohnoliver commented 5 years ago

because then those generators are not around

This isn't the case, the source generation applies equally to control libraries. You can check by creating a new library with the default Uno template.

I'm not sure why the source generation isn't working inside ReactiveUI.

weitzhandler commented 5 years ago

This isn't the case, the source generation applies equally to control libraries.

The ReactivePage was created declaratively, just ported the UWP version, Uno complained about the missing base constructors for iOS and Android, which we later added manually.

davidjohnoliver commented 5 years ago

This isn't the case, the source generation applies equally to control libraries.

The ReactivePage was created declaratively, just ported the UWP version, Uno complained about the missing base constructors for iOS and Android, which we later added manually.

We need to investigate why it's not working in the ReactiveUI project. @jeromelaban may have some more insights when he's back.

I had a quick look, but I get a number of build errors for ReactiveUI.Uno like the following:

1>C:\Users\david.oliver\.nuget\packages\msbuild.sdk.extras\2.0.31\Build\LanguageTargets\CheckMissing.targets(36,5): error : The specified language targets for Xamarin.iOS10 is missing. Ensure correct tooling is installed for 'Xamarin.iOS'. Missing: 'C:\Program Files (x86)\Microsoft Visual Studio\2019\Preview\MSBuild\Xamarin\iOS\Xamarin.iOS.CSharp.targets'
1>Done building project "ReactiveUI.csproj" -- FAILED.
1>You are using a preview version of .NET Core. See: https://aka.ms/dotnet-core-preview
1>C:\Users\david.oliver\.nuget\packages\msbuild.sdk.extras\2.0.31\Build\LanguageTargets\CheckMissing.targets(36,5): error : The specified language targets for uap10.0.16299 is missing. Ensure correct tooling is installed for 'uap'. Missing: 'C:\Program Files (x86)\Microsoft Visual Studio\2019\Preview\MSBuild\Microsoft\WindowsXaml\v16.0\Microsoft.Windows.UI.Xaml.CSharp.targets'
1>Done building project "ReactiveUI.csproj" -- FAILED.

(Building with VS 2019, 16.3 preview 2)

weitzhandler commented 5 years ago

On my machine it was built successfully (16.2.2).

jeromelaban commented 5 years ago

Generated native constructors are a critical part of memory management for iOS and Android, and won't be removed. There should not be any need to care about those generated constructors.

Those constructors are automatically generated for any native-inherited types, as soon as the Uno.UI package is referenced in a project.

Could you explain where that causes an issue? I fail to see where the friction is. If the constructors are missing, they are most likely generated, but in an incorrect namespace. Have a look at the obj\xxx\g folder to see if they are generated properly.

weitzhandler commented 5 years ago

The Uno.UI was referenced in the ReactiveUI.Uno project but the constructors weren't there.

jeromelaban commented 5 years ago

Thanks, I'll be taking a look at what Uno does in the ReactiveUI repo. Will let you know, but overall those constructors will stay :)

jeromelaban commented 5 years ago

So from what I can see, the generators are working properly, generating the ctors where there should be: image With those files in the generated folder: image

Where I see you may have an issue is that you're invoking AttachHandlers() from there, and there's no way to hook on that from the non-generated code. That could be done through partials.

The reason those partials don't exist is that if those constructors ever get called, you're generally in a case where the new instance will get GC'ed very soon, and it won't be used anywhere in managed code.

So overall, the generators work as they should. Could you point me to a branch/code that has an issue with those missing constructors ?

weitzhandler commented 5 years ago

So overall, the generators work as they should. Could you point me to a branch/code that has an issue with those missing constructors ?

For some reason, before I created those constructors by hand and referenced the RxUI repo in my project, my project's generated constructors couldn't find the base constructors. That was before the AttachHandlers.

See https://github.com/reactiveui/ReactiveUI/pull/2116.

weitzhandler commented 5 years ago

Please check out my PR, that can make things easier. But we need to find a way to allow constructors with parameters, to enable DI.