RxUI6: Additional interfaces cleanup #448

Merged
merged 31 commits into from Mar 21, 2014

Projects

None yet

3 participants

@jlaanstra
ReactiveUI member

This PR removes the untyped IReactiveNotifyPropertyChanged, IReactiveNotifyCollectionItemChanged, IReactiveNotifyCollectionChanged interfaces, makes ObservedChange<T> immutable and continues the cleanup started in #432.

What is this PR changing?

  • IReactiveObjectExtension is now IReactiveObject and contains the two untyped interfaces that are left.
  • ExtensionState class has been extended significantly to make it generic. It now also contains a reference to the object it extends. This does not leak because ExtensionState would be kept alive as long as the extended object lives anyway.
  • IRNPCObservableForProperty now uses the extension methods on IReactiveObject and has been renamed.
  • ReactiveList<T> is now also IReactiveObject since it provided the same functionality already anyway.
  • No more untyped interfaces that have typed equivalents. The untyped equivalents were not used much anymore.
  • ObservedChange<T> is immutable. This removes fillInValue() which was using a cast that was not type safe. Almost all instances of fillInValue() could be written without.

Other small fixes

  • IReactiveDerivedList<T>is now also covariant.
  • IRoutableViewModel no longer inherit IReactiveNotifyPropertyChanged, but IReactiveObject.
  • Fixes to comments which were incorrect because of #432.

TODO

  • Discuss the new changes.
  • The Xamarin stuff is still on my list, so those projects probably need to be fixed.
@paulcbetts
ReactiveUI member

No more untyped interfaces that have typed equivalents. The untyped equivalents were not used much anymore.

If you can make this work, that's 👍 but it ended up being super inconvenient for things like View Location

ObservedChange is immutable. This removes fillInValue() which was using a cast that was not type safe. Almost all instances of fillInValue() could be written without.

👍

IRoutableViewModel no longer inherit IReactiveNotifyPropertyChanged. I believe this should not be part of their contract anyway.

Nuke it from IRoutingState, but please leave it on IRoutableViewModel, while it's not technically necessary, it's important to flag that this object should be change-notifying

ReactiveObject is now ReactiveObject where T should be a ReactiveObject

This is hella weird, I don't think that anybody's going to get this...

@jlaanstra
ReactiveUI member

No more untyped interfaces that have typed equivalents. The untyped equivalents were not used much anymore.

If you can make this work, that's 👍 but it ended up being super inconvenient for things like View Location

I only removed the untyped interfaces for the collection and change notifications. IViewForis still there. Do I miss something?

This is hella weird, I don't think that anybody's going to get this...

That might be true, although the result is simply

public class TestViewModel : ReactiveObject<TestViewModel> { }

Nuke it from IRoutingState, but please leave it on IRoutableViewModel, while it's not technically necessary, it's important to flag that this object should be change-notifying.

So would someone ever implement IRoutableViewModel without using ReactiveObject? It will make this interface generic and I wanted to avoid that, because nothing on the interface itself is generic.

@paulcbetts
ReactiveUI member

I only removed the untyped interfaces for the collection and change notifications. IViewForis still there. Do I miss something?

Nope, I'm the one who missed something :)

So would someone ever implement IRoutableViewModel without using ReactiveObject?

They shouldn't, no. I guess they theoretically could, so you could make it derive from INotifyPropertyChanged...

@jlaanstra
ReactiveUI member

They shouldn't, no. I guess they theoretically could, so you could make it derive from INotifyPropertyChanged...

Hmm I actually agree with you that IRoutableViewModel should implement IReactiveNotifyPropertyChanged<T>. I updated the PR.

@onovotny
ReactiveUI member

Is this PR totally nuking the non-generic ReactiveObject? This is gonna be really painful if so as that means that all code has to be updated immediately.

At the very least, the non-generic version should be kept around for awhile, even if it's marked as obsolete.

@paulcbetts
ReactiveUI member

Why don't we just remove Sender from IObservedChange? Sender is kind of the dumbest thing in the world and if we nuked it, we wouldn't need ReactiveObject<T>

@onovotny
ReactiveUI member

@paulcbetts Not sure I follow that last part-- not needing ReactiveObject, or do you mean ReactiveObject<T>?

@paulcbetts
ReactiveUI member

@onovotny I got Markup Censored, I meant, not needing ReactiveObject<T>

@jlaanstra
ReactiveUI member

Why don't we just remove Sender from IObservedChange? Sender is kind of the dumbest thing in the world and if we nuked it, we wouldn't need ReactiveObject<T>

I think sender can actually be quite useful in a WhenAny(x => x.A, x => x.Parent.A) when we for example want to set the selected item based on the sender of the changes.

Instead we could add a ReactiveObject : ReactiveObject<ReactiveObject> class and have it for backwards compatibility?

@jlaanstra
ReactiveUI member

I would still like to have some of this in RxUI 6. Any new thoughts on these changes? I will fix this up against the latest rxui6-master.

@paulcbetts
ReactiveUI member

We gotta ditch the ReactiveObject<TMyOwnClass> stuff somehow, it's too ugly. If we were getting a ton out of it in terms of user scenarios I might be willing to roll the hard six, but afaik, this is a big breaking change that doesn't make any user scenario better / easier / faster.

I know you put a ton of work into this, is there any way to get some of this cleanup without the type system hack?

@jlaanstra
ReactiveUI member

We gotta ditch the ReactiveObject stuff somehow, it's too ugly.

Agreed. I have some time at the end of the week to clean it up.

@jlaanstra
ReactiveUI member

The PR has been updated. The changes are significant to account for the changes made in #502.

We gotta ditch the ReactiveObject stuff somehow, it's too ugly.

It is gone :)

@paulcbetts
ReactiveUI member

Cool! I'll have a look over this soonish

@paulcbetts
ReactiveUI member

A few things before we can merge this:

  • None of the Xamarin stuff builds, can you fix it up?
  • Can you make RaisePropertyChang[ing/ed] an explicit interface implementation on all of the concrete objects, so people don't try to call them outside of the object? Allowing people to call myObject.RaisePropertyChanged is something I've always tried to avoid happening, because it's almost always wrong
@jlaanstra
ReactiveUI member

I'll be on it at the end of this week. Busy week. The Xamarin stuff is gonna be tricky though, because Xamarin is still on my list of things to do.

@jlaanstra
ReactiveUI member

• Can you make RaisePropertyChang[ing/ed] an explicit interface implementation on all of the concrete objects.

Done.

• None of the Xamarin stuff builds, can you fix it up?

Got my Xamarin license today, so now I actually can :). It is gonna take me a while to get familiar with all the ins and outs.

@jlaanstra
ReactiveUI member

Just a heads up, I am still working on this. Will probably be ready early next week.

@paulcbetts
ReactiveUI member

@jlaanstra 🆒

@jlaanstra
ReactiveUI member

:shipit:

@jlaanstra
ReactiveUI member

Some remarks on the Xamarin stuff:

  • I'm seeing the System.Runtime.InteropServices.WindowsRuntime issue described here Reactive-Extensions/Rx.NET#2 for the Monodroid test project.
  • It seems like I am missing a Sherlock assembly. Where do I get it?
  • Should the iOSPlayground and AndroidPlayground projects be renamed to MobileSample-iOS and MobileSample-Android?
@onovotny
ReactiveUI member

For now you can get them here:
https://www.dropbox.com/s/62qhbea0bdt2slf/WindowsFacades.zip

Those are installed on the Mac but accidently left out of the Windows installers. I'm working with Xamarin support to rectify this issue. There's a readme in the zip file that tells you where to put the files.

@paulcbetts
ReactiveUI member

It seems like I am missing a Sherlock assembly. Where do I get it?

We're ditching ActionBarSherlock, if you see it, nuke it

Should the iOSPlayground and AndroidPlayground projects be renamed to MobileSample-iOS and MobileSample-Android?

👍

@paulcbetts
ReactiveUI member

This is a huge amount of work @jlaanstra, thanks for hacking on this! I'll review this week and we'll get this merged

paulcbetts added some commits Mar 21, 2014
@paulcbetts paulcbetts Merge remote-tracking branch 'origin/rxui6-master' into additional-in…
…terfaces-cleanup

Conflicts:
	AndroidPlayground/Resources/Resource.designer.cs
	ReactiveUI.Platforms/Android/ReactiveActivity.cs
	ReactiveUI.Platforms/Android/ReactiveFragment.cs
8c65015
@paulcbetts paulcbetts Fix up the new Cocoa classes to use IReactiveObject ed26411
@paulcbetts paulcbetts :sigh: 8bbda78
@paulcbetts paulcbetts merged commit 0a15158 into rxui6-master Mar 21, 2014
@paulcbetts paulcbetts deleted the additional-interfaces-cleanup branch Mar 21, 2014
@paulcbetts
ReactiveUI member

Oof, I need to revert some of e0d69c9, you can't have UIKit objects that are generic, the runtime blows up. I'll fix this up

@paulcbetts paulcbetts added a commit that referenced this pull request Mar 21, 2014
@paulcbetts paulcbetts Make Cocoa view sources non-generic
While the design in #448 is far-and-away better from a code perspective, it
doesn't work because Cocoa objects can't be generic (i.e. anything that
exposes an interface to Objective C). Peep
http://docs.xamarin.com/guides/ios/advanced_topics/limitations for the
details.
d099607
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment