MissingFieldException in fragment and activity

[binoypatel]

Hi

After upgrading to rx 7.1, I am getting following exception when WireUpControls called:

1. In fragment:
   System.MissingFieldException: Failed to wire up the Property View to a View in your layout with a corresponding identifier

2. In activity:
   System.MissingFieldException: Failed to wire up the Property CurrentFocus to a View in your layout with a corresponding identifier

As you can see that "View" and "CurrentFocus" both are type of Android.Views.View properties and I believe it should not be in the collection in ControlFetcherMixin.WireUpControls. You can reproduce this issue in Playground-Android. Let me know if I am doing something wrong? I have attached MainActivity.cs from Playground-Android for your reference:
MainActivity.txt

Thanks,
Binoy

[kentcb]

Seems likely that this was brought about by #1217. Perhaps @Qonstrukt has some thoughts on this?

[Qonstrukt]

I'll try and reproduce this today.

[Qonstrukt]

So, this bug got introduced by #1217 but I'd like some ideas on how we might fix this, because I see a couple of options.
We could just revert #1217 of course, but this issue also happens with descendants when you use properties for dynamically created Views for example.
I'd say we need a way to have this fail silently, or just throw a warning like we do with POCO properties.

@reactiveui/reviewers-android Do you guys think it's necessary to add a parameter to WireUpControls to triggers warnings instead of exceptions, or could we make this the default behaviour? Or do you think we shouldn't touch this at all and just revert #1217?

[binoypatel]

I could not quite understand the changes made in #1217 but here is what I suggest:

public static void WireUpControls(this Activity This, bool viewWithsubClass = false)
{
     IEnumerable<PropertyInfo> members;
     if (viewWithsubClass)
     {
         members = This.GetType().GetRuntimeProperties()
         .Where(m => typeof(View).IsAssignableFrom(m.PropertyType));
      }
     else
     {
         members = This.GetType().GetRuntimeProperties()
         .Where(m => m.PropertyType.IsSubclassOf(typeof(View)));
     }...

reset of the method is same. We can do the same for all methods.

@Qonstrukt Can you please share some code where you need IsAssignableFrom(), that will help me understand the benefit of it?

Kind regards,
Binoy

[Qonstrukt]

The change in #1217 was purely made to allow View properties to be wired up. IsSubclassOf only works for descendants like TextView. Let's say I have a spacer of some kind, which is just a plain view with AXML defined layout properties, there's no reason to subclass these. WireUpControls wouldn't work on such elements.

I didn't run into this exception because we were already catching the System.MissingFieldException exceptions. We often use dynamically created views in the app that I have tested this with, that's why we needed those try-catch blocks.

Looking at how this works, it's kind of a bare-bones implementation anyway. We had our own solution before using RxUI which depended on Property attributes to define that a property had to be wired up automatically.
I think RxUI could use such a solution as well, and to be honest, I think this feature isn't used that much yet, so I might as well propose a new way of how WireUpControls should work, and add some tests to it while I'm at it.

[binoypatel]

Cool thanks for the detailed reply, I definitely agree that it requires better way of doing this but I believe we should avoid try/catch as not everyone need to pay the performance cost in regular scenarios. I will be happy to test your changes to make sure it cover regular scenarios.

Thanks,
Binoy

[Qonstrukt]

I'll try and propose a code change this weekend and will reference you, so you can have a look.

[makon]

IMHO I don't see the need of wiring up controls of type View in daily scenarios. Even when I am using a view as a spacer it's a real corner case to try to change its size or some other properties dynamically afterwards and there is still the workaround by using FindViewById<View> directly.

Failing silently just because of a corner case does not feel like the right solution. I feel like it would somehow break the convention and I would not get an immediate exception if I did some typo or did some refactoring. Failing silently or even logging warnings would just postpone the exception to a NullReferenceException when trying to access the View property.

I would disagree that WireUpControls isn't used that much. It's integrated in LayoutViewHost so it's an essential part of every ReactiveListAdapter and therefore deeply integrated into ReactiveUI.Android. Another indicator is that already 3 different issues were raised all related to this change #1227, #1231, #1234. In my opinion if there should be a change at all, it should not break the api and do not affect everyone. The proposed solution of @binoypatel seems like a good trade-off. Also a quick note that the default behavior does not work for View would be really nice.

We could just revert #1217 of course, but this issue also happens with descendants when you use properties for dynamically created Views for example.

@Qonstrukt I do not really understand the issue can you maybe provide an example to clearify the use case?

[Qonstrukt]

Thank you for pointing out the use in LayoutViewHost, I'm not familiar with that functionality yet.

An example is very easy, say you have a property in a base Fragment representing a Toolbar for example:

public Toolbar DefaultToolbar { get; set; }

This Toolbar is dynamically created, it's not in the layout, but created in OnViewCreated for example. Or there's cases where it's not created at all. Other fragments inherit from this base Fragment. Then it's impossible to use WireUpControls because it expects the DefaultToolbar to be there in the layout.

Sure you can work around this by falling back to FindViewById, but I despise having to find all my views manually, cluttering my OnViewCreated methods.
Since you pointed out the use in LayoutViewHost, I can imagine this method was introduced with that functionality in mind. It's there for everybody to use, but I think it needs some tweaks for it to be really useful.

One option would be to have it find only View descendants in it's own class and not the classes it's inheriting from. This would be a rather simple improvement, adding an attribute to WireUpControls with a default fallback to the old behaviour. How's everybody feeling about that?

[Qonstrukt]

This is fixed in 7.2.0 with #1250. Thanks @binoypatel and @orzech85 for reporting, and others for thinking on a solution.

[sushihangover]

Just so I clear 😕 , On an ReactiveAppCompatActivity with the use of ResolveStrategy, you basically have to use ExplicitOptIn and apply attributes to your opt-in properties.

Any other strategy/attribute combo results in the Failed to wire up the Property CurrentFocus... as CurrentFocus has a non-overridable Set, so an IgnoreResource on a fake CurrentFocus property an not be used...

Thanks.

[Qonstrukt]

I haven't encountered this in my own projects, but I'm assuming you're referring to the CurrentFocus property inside AppCompatActivity which is of type View. This means you can use ResolveStrategy.Implicit as well, which doesn't wire up View type properties, precisely for this reason.

[sushihangover]

@Qonstrukt Thanks for the quick reply. (yes, I am referring to CurrentFocus property inside AppCompatActivity)

Ah... said the now caffeinated brain... after adding the rxUI source, I realized that ResolveStrategy.Implicit is trying to wire up all view properties to the layout, not all the layout ids to view properties...

I refactored a couple activities to use Implicit or ExplicitOptIn where removing all the view properties was not worth it and re-did a handful of Views (were Forms-based), solved a ton of Forms' bugs and speed problems in at least those refactored views. (The WireUpControls is nice now that I am am using it correctly 😆, but will be sticking w/ the Fody-addin I have to avoid the runtime overhead). With the native-UI speed improvements over Forms but with the Reactive obs/sub overhead moving the pointer in the other direction, rxUI is not going to save this Form's project from going to a different group to be green-fielded in Kotlin and Swift... 😞