Item Changing Zipped With Item Changed Test #508

Closed
wants to merge 5 commits into
from

Projects

None yet

3 participants

@damiensawyer

Failing Unit test demonstrating issue when zipping changed and changing observables from ReactiveList.

@jlaanstra
ReactiveUI member

You are retrieving both the old and the new value when the ItemChanged observable has fired and the item has already changed. Since GetValue is lazy, you get the current value twice.

This can be fixed by adding a .Select(old => old.Getvalue()) before the Zip()

@paulcbetts
ReactiveUI member

Oh duh, why didn't I spot that before. Yep, this is a bug in your code, though it's enough of a common request that we should probably just have an Extension method for Changing/Changed on RxObject and maybe a few common ones on RxList too. @jlaanstra, want to take a crack at it?

@damiensawyer

Ahh... thanks guys.

Paul, I'm happy to have a look at it if you want. It'd be good to get involved....

@paulcbetts
ReactiveUI member

@damiensawyer Sure! So I'd think about how to get an Old/New pair for a simple property first on a ViewModel object, that's the most common request.

@jlaanstra
ReactiveUI member

Guess I would add it as an extension method on the IReactiveNotifyPropertyChanged interface where the Changing/Changedobservables are defined.

@damiensawyer

Cool.... I'll check it out.

@damiensawyer

Here's my attempt as discussed. Put "ChangeWithHistory" on ReactiveObject and ReactlveList. I'm a little confused about something in the list and have flagged it with a 'todo' for you to look at.

@paulcbetts paulcbetts commented on the diff Feb 18, 2014
ReactiveUI/Interfaces.cs
@@ -49,6 +75,17 @@ public class ObservedChange<TSender, TValue> : IObservedChange<TSender, TValue>
}
/// <summary>
+ /// A data-only version of IObservedChangeWithHistory
+ /// </summary>
+ public class ObservedChangeWithHistory<TSender, TValue> : IObservedChangeWithHistory<TSender, TValue>
@paulcbetts
paulcbetts Feb 18, 2014

Don't create a new type for this, just put it in ObservedChange (i.e. just add another T that's OldValue)

@paulcbetts paulcbetts commented on the diff Feb 18, 2014
ReactiveUI/Interfaces.cs
@@ -141,6 +178,14 @@ public interface IReactiveNotifyPropertyChanged : INotifyPropertyChanged, INotif
IObservable<IObservedChange<object, object>> Changed { get; }
/// <summary>
+ /// Represents an Observable that fires *after* a property has changed and
+ /// provides both the old and new values for that property.
+ /// Note that this should not fire duplicate change notifications if a
+ /// property is set to the same value multiple times.
+ /// </summary>
+ IObservable<IObservedChangeWithHistory<object, object>> ChangedWithHistory { get; }
@paulcbetts
paulcbetts Feb 18, 2014

Probably shouldn't be on the interface itself, add it as an Extension method on IReactiveNotifyPropertyChanged

@paulcbetts paulcbetts commented on the diff Feb 18, 2014
ReactiveUI/ReactiveObject.cs
get { return this.getChangedObservable(); }
}
+ /// <summary>
+ /// Represents an observable that fires *after* a property has changed, providing access to both the old and new values.
+ /// </summary>
+ public IObservable<IObservedChangeWithHistory<object, object>> ChangedWithHistory
+ {
+ get
+ {
@paulcbetts
paulcbetts Feb 18, 2014

Code Styleeeeee

@paulcbetts
ReactiveUI member

Hey @damiensawyer, sorry for totally spacing on this - I think we can make this work, but your editor has run rampant over the code and trashed the style. Can you try redoing it? It looks like e7a2885 is the first time things go pear-shaped.

So, here's the way I'd go about fixing it:

git checkout zip-test
git remote add upstream https://github.com/reactiveui/ReactiveUI
git fetch upstream

## This will forget all of your commits, but leave your changes in the working directory
git reset upstream/master

## Think about the first discrete commit you want to make
git add -p  ## go through and say 'y' for any change related to that commit
git commit

### Repeat until there are only bogus changes left

git reset --hard HEAD  # Kill the bogus changes
@damiensawyer

Thanks @paulcbetts . I'll have a look. I mightn't be able to get onto it straight away though, having a crazy busy week.

Cheers.

@jlaanstra
ReactiveUI member

@damiensawyer are you still planning on working on this?

@damiensawyer

Sorry @jlaanstra, the ReactiveUI project I was on was suspended and I've been pulled on to other things. Let me have a look at working through Paul's 'git fix' suggestions. Let's give me a time limit of, say, 48 hours. If I've not pushed something by then assume that I gave up and am abandoning the pull request. Sorry to be a hassle!

@jlaanstra
ReactiveUI member

@damiensawyer No problem. We are just cleaning up the open issues.

@damiensawyer

Sweet. Hey, I've just had a look at this again. If it's ok, I think that I'll abandon it. I've not been using Rx or ReactiveUI since this patch and am a little rusty. There'd be a bit of a learning curve to pick up the ball again and I'm behind on other deadlines. Hope you understand and can perhaps use what's there anyway.
Cheers,
DS

@jlaanstra jlaanstra added this to the ReactiveUI 6.0 milestone Apr 27, 2014
@jlaanstra jlaanstra self-assigned this Apr 27, 2014
@jlaanstra
ReactiveUI member

@damiensawyer Thanks for letting us know. Reassigning this to myself.

@jlaanstra
ReactiveUI member

@paulcbetts This is ready to be merged in the reactiveui/zip-test branch.

@jlaanstra
ReactiveUI member

This is now tracked in #594.

@jlaanstra jlaanstra closed this May 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment