Don't display [None] twice.

[grokys]

The empty list item was getting added twice: once by the XAML and once in PullRequestListViewModel. This adds a new overload of TrackingCollection.CreateListenerCollection so that the logical can be carried out at the VM layer instead of being mixed between VM and view.

Fixes #390.

[shana]

This fix is not correct. Selecting [None] should remove it from the list and reset the filter to nothing, so everything shows up. [None] should only be visible when there is a selection in the list. The StickieListItemConverter is there to return null if [None] is set (so that the filter is reset), and the interaction behaviour is there to add/remove the [None] item on top of the list, depending on the selection.

[grokys]

Ok, I'd misunderstood the intention then. Will try and work out the correct fix.

[shana]

Initially, [None] should not be on the list, the interaction behaviour should be removing it when things load. It could be the IAccount type comparison not detecting that [None] and [None] are the same, and failing to initially remove it, which might explain why it looks duplicated. A quick unit test using the same data and asserting identity comparisons might work. Maybe I'm doing == instead of Equals in the logic (I've tweaked comparison implementations a bit, so that might have broken it)

[paladique]

👍

[shana]

I added two tests that clarify the behaviour this should have when initially created and when the TrackingCollection is reset. They're failing 😉

Also, the [None] selection is broken:
Nevermind the gif above, my checkout had some stale code!

[shana]

Seeing as the SelectedAssignee and SelectedAuthor are being set twice (once with None and then with null), we should probably filter out the None value when updating the filter, there's no point in filtering twice. Something like this perhaps?

this.WhenAny(x => x.SelectedAssignee, x => x.Value)
                 .Where(x => x != EmptyAssignee)
                 .Where(x => PullRequests != null)
                 .Subscribe(a => UpdateFilter(SelectedState, a, SelectedAuthor));

this.WhenAny(x => x.SelectedAuthor, x => x.Value)
                 .Where(x => x != EmptyAuthor)
                 .Where(x => PullRequests != null)
                 .Subscribe(a => UpdateFilter(SelectedState, SelectedAssignee, a));

[shana]

How about we just do a result.FirstOrDefault() == stickieItemOnTop instead of tracking with a flag? We know it's always going to be either the first or not there at all, and that way we don't have to keep state (and the first element look up is fast and doesn't consume the whole collection). That would work equally well to replace the hasSelection flag, too (or maybe not entirely for that flag, I'm trying to follow all the edge cases in my head here but brain is tired)

[grokys]

Yep, I was wondering about that, but assumed you had a scenario in mind when writing it like that.

[shana]

This doesn't need to be set.

[shana]

✨