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

feat: more flexible WireUpControls implementation #1250

Merged
merged 8 commits into from Feb 20, 2017

Conversation

Projects
None yet
6 participants
@Qonstrukt
Copy link
Member

Qonstrukt commented Jan 29, 2017

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
This is an improvement upon WireUpControls and #1217, but this time without introducing the problems from #1234, #1231 and #1227.

What is the current behavior? (You can also link to an open issue here)
WireUpControls doesn't allow you to specify how it'll find members currently. It'll always find all properties which are subclasses of View, which isn't always what you want.

For example, when you have a Toolbar property in your Fragment which you initialise dynamically in OnViewCreated, you cannot use WireUpControls anymore because it's unable to wire up the property with a view in your layout. This counts for the fragment itself or any subclasses thereof.

What is the new behavior (if this is a feature change)?
This PR allows you to switch from just implicit resolving, to two additional ways to explicitly wire up properties to views in your layout, by using opt-in or opt-out.

When using implicit, it'll default to the way WireUpControls has always worked.

When using explicit opt-in, you should use the WireUpResource attribute on your properties to explicitly define you want it wired up.

When using explicit opt-out, it introduces the behaviour of #1217 with the added bonus of opting out certain properties with the IgnoreResource attribute.
This is currently not very useful in activities and fragments, because it'll also introduce the problems with #1217, but it can certainly be handy in views for example.

Does this PR introduce a breaking change?
In contrary to #1217 it shouldn't. Unfortunately there doesn't seem to be any testing set-up for Android currently (amongst other platforms), but I will be using this in production in several ways in the upcoming week.

Please check if the PR fulfills these requirements

Other information:

  • I'd like feedback from @reactiveui/reviewers-android, @binoypatel and @makon on this. Am I missing something? Is the naming alright? The obvious.
  • I'll be testing this thoroughly in the next couple of days.
  • I'll whip up a page in the docs after I've had the green light on this.
@kentcb

This comment has been minimized.

Copy link
Contributor

kentcb commented Jan 30, 2017

@orzech85 @binoypatel @bidy are you guys able to pull this down and verify it resolves your issue?

@binoypatel

This comment has been minimized.

Copy link

binoypatel commented Jan 30, 2017

@kentcb Sure I will review this and get back to you by this week

@makon

This comment has been minimized.

Copy link

makon commented Feb 1, 2017

At first sorry, for the late response. Unfortunately I am quite busy this week...

Thank you for pointing out the issue with the Toolbar. I haven't thought about that but you are totally right. The current solution does not fit this cases properly. It seems like we identified two separate issues with the current implementation

  • The need to wire up controls of type View
  • The need to exclude properties which are programmatically created

Please correct me if I am missing some other point.

I really like the idea of having the possibility to define which properties should be included or excluded. It is definitely an advantage compared to the current behavior. I was thinking about a similar solution but without using attributes for it. Something like the following pseudo code:

// excluding properties adresing issue #2
this.WireUpControls(options => options.Exclude(x => x.Toolbar)
                                      .Exclude(x => x.CurrentFocus));

// include properties adresing issue #2
this.WireUpControlsExplicit(options => options.Include(x => x.Toolbar));

// determining the resolve strategy adresing issue #1
this.WireUpControls(Action<ExcludeOptions> options, ResolveStrategy strategy = ResolveStrategy.IsSubclassOfView);

this.WireUpControlsExplicit(Action<IncludeOptions> options, ResolveStrategy strategy = ResolveStrategy.IsSubclassOfView);

public enum ResolveStrategy
{
    IsSubclassOfView,
    IsAssignableFromView
}

This solution would have the advantage to be able to include or exclude properties from base types which are defined in third part libraries where I cannot add any attribute (e. g. Activity, Fragment, etc.). It would also provide the flexibility to exclude a property just in a given scenario. I am not an RxUI expert but I have a slight feeling that it would better fit to the remaining API because I would not expect the need to use Attributes with RxUI. But this is a highly personal impression.

What do you think about this? Do you think it is worth the overhead on work? It would be great if you could give me some feedback. If there is some interest I could give it a shot. But as I already mentioned your solution is totally an improvement and I am also fine with it.

@Qonstrukt

This comment has been minimized.

Copy link
Member Author

Qonstrukt commented Feb 2, 2017

Thank you for your insights!

The idea you've come up with is actually quite compelling, I agree it fits better in a Rx context. However I don't think I would apply it much to my use cases. Having to exclude certain Views every time (like CurrentFocus) gets cumbersome quick, so I would quickly fall back to the Include strategy. And then your solution would require me to define a query inside my code blocks again which feels nearly the same as using FindViewById.

Having the possibility to use attributes keeps such housekeeping rules from my code which I prefer personally, but I'd really like to hear other people's opinions about this.

I'm going to rename ResolveMembers to ResolveStrategy btw, as that's a much better name. 😄

@Qonstrukt

This comment has been minimized.

Copy link
Member Author

Qonstrukt commented Feb 6, 2017

We're a week down the road and I've pushed two improvements;

  • It's now possible to define a custom resource name so your property names don't necessarily need to correspond with the resource names.
  • I've added the possibility to use a WireUpResource attribute with the Implicit resolve strategy, so you can add View resolving on a per-property basis.

Besides that, I've renamed ResolveMembers to ResolveStrategy as I think it's a much better name, credit to @makon.

Is there anyone in @reactiveui/reviewers-android group that wants to share their opinion?

I've been using this in two separate projects with different resolve strategies now without any problems.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 6, 2017

Coverage Status

Coverage remained the same at 65.574% when pulling 709673c on Qonstrukt:improve-wire-up-controls into a7c7f9c on reactiveui:develop.

@Qonstrukt

This comment has been minimized.

Copy link
Member Author

Qonstrukt commented Feb 13, 2017

Another week, and I've used my latest implementation in two bigger projects without problems. I'll write up some draft docs to explain how it works exactly.

@kentcb

This comment has been minimized.

Copy link
Contributor

kentcb commented Feb 19, 2017

@Qonstrukt are you happy/keen for this to be merged and included in the next release?

@Qonstrukt

This comment has been minimized.

Copy link
Member Author

Qonstrukt commented Feb 19, 2017

Personally I am, this time I've also made it with 100% backward compatibility in mind. So it should be 'safe".

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 20, 2017

Coverage Status

Coverage remained the same at 65.938% when pulling c5e6dc4 on Qonstrukt:improve-wire-up-controls into bf6e1a4 on reactiveui:develop.

@kentcb kentcb removed the in progress label Feb 20, 2017

@kentcb kentcb added this to the vNext milestone Feb 20, 2017

@kentcb kentcb changed the title Allow to define which members WireUpControls finds feat: more flexible WireUpControls implementation Feb 20, 2017

@kentcb kentcb merged commit 01c6af2 into reactiveui:develop Feb 20, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage remained the same at 65.938%
Details
@kentcb

This comment has been minimized.

Copy link
Contributor

kentcb commented Feb 20, 2017

Excellent! Thanks @Qonstrukt!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment