Changes-tracking collection for list views #111

shana · 2015-10-05T22:43:38Z

TrackingCollection gets items from an observable sequence and updates its contents in such a way that two updates to the same object (as defined by an Equals call) will result in one object on the list being updated (as opposed to having two different instances of the object added to the list).

This allows us to grab a bunch of objects from cache, load a collection with them, and then request updated versions of the objects from the server, which when returned will update the collection in place (and the UI with it), seamlessly.

The collection supports ordering and filtering, and filtering includes enough information to allow limiting the number of items to return (and display). It can be resorted and refiltered in place.

This PR also adds the IPullRequestModel model interface, because that's where this list will be used and might as well test things on it.

Also now that I've tagged 1.0.15, we can put these into master. Any further bug fixes on the 1.0 series can be done on a branch if needed.

TrackingCollection gets items from an observable sequence and updates its contents in such a way that two updates to the same object (as defined by an Equals call) will result in one object on the list being updated (as opposed to having two different instances of the object added to the list). This allows us to grab a bunch of objects from cache, load a collection with them, and then request updated versions of the objects from the server, which when returned will update the collection in place (and the UI with it), seamlessly. The collection supports ordering and filtering, and filtering includes enough information to allow limiting the number of items to return (and display). It can be resorted and refiltered in place.

haacked · 2015-10-07T16:04:54Z

Is TrackingCollection similar to TrackedRepositories that we have in GitHub for Windows?

haacked · 2015-10-07T16:06:51Z

src/GitHub.Exports.Reactive/Collections/TrackingCollection.cs

+
+namespace GitHub.Collections
+{
+    public class TrackingCollection<T> : ObservableCollection<T>, ITrackingCollection<T>


Let's document this class like you did in the PR.

haacked · 2015-10-07T17:05:30Z

src/GitHub.Exports.Reactive/Collections/TrackingCollection.cs

+                {
+                    OnPropertyChanged(new PropertyChangedEventArgs("Item[]"));
+                    OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Move, this[start], start, idxSmallest));
+                }


If I understand correctly, this method does a completely new sort, right? Could we just call list.Sort and then raise the NotifyCollectionChangedAction.Reset event rather than raise an event for each item?

Yeah, list.Sort makes sense here.

haacked · 2015-10-07T22:56:46Z

Ok, I'm done with my first pass. 🍨

shana · 2015-10-22T21:46:50Z

It's GREEN!

All the tests except for two timing tests are running on ci. The timings tests are iffy and likely to fail on a server, so better not have that stuff there.

Also, looks like we can't use your string interpolation thing on the server. Not sure why, but it just won't build and I'm not in the mood to fight it any more.

📦

haacked · 2015-10-22T22:56:42Z

Excellent! I'll do another round of reviews tonight!

haacked · 2015-10-22T23:52:28Z

src/GitHub.Exports.Reactive/Collections/TrackingCollection.cs

+            data = CheckFilter(data);
+            data = CalculateIndexes(data);
+            data = SortedRemove(data);
+            data = FilteredRemove(data);


data isn't returned. So why the assignments? Could these be void methods? Or should data be returned?

These methods are part of a pipeline of methods that get called sequentially and that process an item (see Listen method). They might or might not return a different ActionData object as part of the processing. SortedRemove and FilteredRemove aren't technically a part of the pipeline of the Listen method (because they'd always be noop there), but for all intents and purposes they follow the same pattern.
Just pretend these 4 calls are each a Select call on an observable 😄

I debated whether to return the removed item (hence the final assignment doing nothing). Might stil do that. In the meantime, the final assignment is just keeping the pattern.

If it's a pipeline, shouldn't each method return a new instance of ActionData instead of mutating the passed in instance? I think this calling pattern is really confusing.

Also, SortedRemove and FilteredRemove actually mutate the current TrackingCollection and do not mutate data. They only read from it. They are very different from the methods called before them, which do some calculations and return an ActionData with the calculations. I think that difference and intent would be better communicated by making those two methods return void.

So this method would look like.

var data = new ActionData(TheAction.Remove, item, null, position - 1, position, original); data = CheckFilter(data); data = CalculateIndexes(data); SortedRemove(data); FilteredRemove(data);

Where data is never mutated. Thoughts?

If it's a pipeline, shouldn't each method return a new instance of ActionData instead of mutating the passed in instance? I think this calling pattern is really confusing.

Yes, if this was a purely functional thing, it would return a new ActionData for every call. I don't do that because I don't want to be creating thousands of objects and putting pressure on the GC when I don't have to.

ActionData has non-mutable data, which is the data regarding the item that's being processed and where it is in the sorted, unfiltered list (the item, it's position in the list, where it was before, and a reference to the list itself). This data is built on ProcessItem. After that, other information is added to ActionData so I don't have to keep looking it up, like whether it's visible according to the current filter, it's index in the filtered list, etc. All this (mutable) information is related to the filtered list (the one that is visible via the Items property of the base class).

I tried to strike a balance between immutability and the fact that every item processed would have at least 3 "updates" to it's ActionData object (or more), and go through 5 methods, and I don't like needlessly throwing pressure at the GC. Whenever data that is essential to the object is calculated, a new ActionData is created. Other data merely updates the existing ActionData (for instance, if there's no filter, only the data for the unfiltered list is needed)

Also, SortedRemove and FilteredRemove actually mutate the current TrackingCollection and do not mutate data.

Neither do the other SortedXXX and FilteredXXX methods that are called via the Select calls in the Listen method. By the time they're called, the data in ActionData is ready for processing by these methods, so they merely use it and pass it through.

In a future where I can figure out how to include Remove actions into the pipeline that's set up in the Listen methods, SortedRemove and FilteredRemove will have to conform to the same pattern as the other SortedXX and FilteredXX methods. The only reason they're not included in the Listen method list of Select calls right now is because they would be noops and I don't see the point in having method calls that currently are known to be noops.

I change this so that RemoveItem actually returns the item that was removed. It's a common pattern and useful thing to have (because the item you pass in might not be the exact same instance that is removed). It returns null when it couldn't find anything to remove, so that's a nice thing to have too.

and I don't like needlessly throwing pressure at the GC

But you wouldn't be if ActionData was a struct, right? It really feels like a struct to me. It's just a value type that defines what action should happen.

But you wouldn't be if ActionData was a struct, right? It really feels like a struct to me. It's just a value type that defines what action should happen.

You're absolutely correct, it should be a struct, and that makes things make more sense. I'll fix that up right away.

haacked · 2015-10-23T21:58:19Z

Ugh, TrackingCollectionsTests are too big to render in the PR diff so I can't provide comments and feedback on it.

haacked · 2015-10-23T22:03:57Z

Just a little bit more to go! Back to you @shana. 😄

…ion)

shana · 2015-10-26T16:33:13Z

Ugh, TrackingCollectionsTests are too big to render in the PR diff so I can't provide comments and feedback on it.

I really don't know what to do about this. I can't understand why we have a limit on PR size without a way to see the full diff on the website, it makes it really pointless for feature work. 😢

I'm hacking on a diff commenter in VS so you can comment there on the PR, but can't make that work fast enough to get this thing out the door. If only we had PR reviewing in VS... 😜

haacked · 2015-10-27T17:42:23Z

src/GitHub.Exports.Reactive/Collections/TrackingCollection.cs

+                IsIncluded = other.IsIncluded;
+            }
+
+            public ActionData(ActionData other, int position)


This ctor is never used. Should we remove it?

It seems like some of these ctors could be defined in terms of the others.

Cleaned up the constructors. The ctor above is being used now (before it was passing default args and hitting another ctor, that's fixed now)

Did you remember to push those changes? The only change I see is a merge to master. The constructors haven't changed.

I did, but it went to the wrong branch because the deskop app is just not liking me today. All fixed, everything pushed correctly now :P

shana · 2015-10-27T18:32:34Z

Back to you! 🍍

haacked · 2015-10-27T21:57:48Z

This is great! I know it's been a long trudge, but I appreciate you humoring my nitpicky reviews.

Changes-tracking collection for list views

Add IPullRequestModel

fa47a25

shana assigned haacked Oct 5, 2015

shana force-pushed the feature/pr/tracking-list branch from 8fd274c to ae0cf35 Compare October 5, 2015 23:22

shana mentioned this pull request Oct 6, 2015

Pull request list apis with cache #118

Merged

shana force-pushed the feature/pr/tracking-list branch from ae0cf35 to 7d40d74 Compare October 6, 2015 14:41

Clean up

a0ce2e1

shana mentioned this pull request Oct 6, 2015

Pull Request list view and viewmodel #119

Merged

31 tasks

haacked reviewed Oct 7, 2015
View reviewed changes

🎨 Sort and remove usings

73298d1

haacked reviewed Oct 7, 2015
View reviewed changes

haacked assigned shana and unassigned haacked Oct 7, 2015

Lots of cleanup, refactor and testing

b476146

shana added 2 commits October 22, 2015 23:25

Convince it to use C# 6...

2de84b5

Yeah, this doesn't work on this framework.

a21c91c

haacked assigned haacked and unassigned shana Oct 22, 2015

haacked reviewed Oct 22, 2015
View reviewed changes

🎨 Just removing redundant shit

a1dc147

haacked assigned shana and unassigned haacked Oct 23, 2015

haacked and others added 3 commits October 23, 2015 15:05

🎨 Define variable in the scope it's used

c41d2d8

Return the removed item (or null if item doesn't exist in the collect…

e02a182

…ion)

Remove useless return

c10f9ed

shana assigned haacked and unassigned shana Oct 26, 2015

shana added 2 commits October 27, 2015 15:17

Fix build

ab5c076

Make ActionData a struct

ba26db7

haacked reviewed Oct 27, 2015
View reviewed changes

Merge master into feature/pr/tracking-list

a9ff71b

Cleanup ActionData

f13cac6

haacked added a commit that referenced this pull request Oct 27, 2015

Merge pull request #111 from github/feature/pr/tracking-list

6bd8ca0

Changes-tracking collection for list views

haacked merged commit 6bd8ca0 into master Oct 27, 2015

haacked deleted the feature/pr/tracking-list branch October 27, 2015 21:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes-tracking collection for list views #111

Changes-tracking collection for list views #111

shana commented Oct 5, 2015

haacked commented Oct 7, 2015

haacked Oct 7, 2015

haacked Oct 7, 2015

shana Oct 7, 2015

haacked commented Oct 7, 2015

shana commented Oct 22, 2015

haacked commented Oct 22, 2015

haacked Oct 22, 2015

shana Oct 23, 2015

haacked Oct 23, 2015

shana Oct 26, 2015

shana Oct 26, 2015

haacked Oct 26, 2015

shana Oct 27, 2015

haacked commented Oct 23, 2015

haacked commented Oct 23, 2015

shana commented Oct 26, 2015

haacked Oct 27, 2015

haacked Oct 27, 2015

shana Oct 27, 2015

haacked Oct 27, 2015

shana Oct 27, 2015

shana commented Oct 27, 2015

haacked commented Oct 27, 2015

Changes-tracking collection for list views #111

Changes-tracking collection for list views #111

Conversation

shana commented Oct 5, 2015

haacked commented Oct 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haacked commented Oct 7, 2015

shana commented Oct 22, 2015

haacked commented Oct 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haacked commented Oct 23, 2015

haacked commented Oct 23, 2015

shana commented Oct 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shana commented Oct 27, 2015

haacked commented Oct 27, 2015