[WIP] AdvancedCollectionViewSource #349

tomzorz · 2016-09-18T20:01:29Z

This is a work in progress for issue #248

Right now I only did the suggested formattings to the original code.

Skipped for now:

grouping (I /* commented out */ the parts until we/I figure out how to proceed on this one)

Done:

filtering
sorting
selected value persists while using sort/filter
incremental item loading (inherited)
tests (maybe add more later)
sample

Edited: 2016.10.09., 2016.10.22., 2017.01.07.

Update from upstream

msftclas · 2016-09-18T20:01:34Z

Hi @tomzorz, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

msftclas · 2016-09-18T20:17:17Z

@tomzorz, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

deltakosh · 2016-09-19T17:38:32Z

This is REALLY cool. It is a big amount of work though. To all: Do not hesitate to use this issue (#248) to discuss about architecture or whatever else you may need.

tomzorz · 2016-09-19T18:05:05Z

Edited the original comment, I added the done section.

deltakosh · 2016-09-19T17:35:50Z

....Toolkit.Uwp.UI.Controls/AdvancedCollectionViewSource/AdvancedCollectionViewSource.Events.cs

+using Windows.Foundation.Collections;
+using Windows.UI.Xaml.Data;
+
+namespace Microsoft.Toolkit.Uwp.UI.Controls.AdvancedCollectionViewSource


I should not be in its own namespace

....nor in a UI.Controls namespace since it's neither

True. The built-in one is in .Xaml.Data - https://msdn.microsoft.com/library/windows/apps/br209833?f=255&MSPPError=-2147217396 - I'd move it to the .UI project under the Data namespace based on that.

.UI should be enough (no need for .Data)

I moved it.

deltakosh · 2016-09-19T17:37:25Z

Microsoft.Toolkit.Uwp.UI.Controls/AdvancedCollectionViewSource/AdvancedCollectionViewSource.cs

+            HandleSourceChanged();
+        }
+
+        public IEnumerator<object> GetEnumerator() => _view.GetEnumerator();


Properties order is not aligned to Stylecop checks here (You must have warnings for that when compiling)

dotMorten

Did a quick little initial review

dotMorten · 2016-10-06T05:42:01Z

....Toolkit.Uwp.UI.Controls/AdvancedCollectionViewSource/AdvancedCollectionViewSource.Events.cs

+using Windows.Foundation.Collections;
+using Windows.UI.Xaml.Data;
+
+namespace Microsoft.Toolkit.Uwp.UI.Controls.AdvancedCollectionViewSource


....nor in a UI.Controls namespace since it's neither

dotMorten · 2016-10-06T05:43:32Z

....Toolkit.Uwp.UI.Controls/AdvancedCollectionViewSource/AdvancedCollectionViewSource.Events.cs

+                return;
+            }
+
+            CurrentChanging?.Invoke(this, e);


Is this event needed? Wouldn't it be better to have oldValue and newValue in the event arguments of the CurrentChanged event?

Also why virtual? If the intent is to subclass and ovewrite this event to handle when the item is changing, generally this should be an empty method to override, so the content of this code still executes if the subclass doesn't call base.OnCurrentChanging (same for OnCurrentChanged and OnVectorChanged)

Is this event needed? Wouldn't it be better to have oldValue and newValue in the event arguments of the CurrentChanged event?

The implementations are this way because of ICollectionView's requirements. (They are only used for the defer notifications part.)

Also why virtual?

True. Removed virtual, and made it private.

dotMorten · 2016-10-06T05:47:23Z

Microsoft.Toolkit.Uwp.UI.Controls/AdvancedCollectionViewSource/AdvancedCollectionViewSource.cs

+                _sourceNcc = _source as INotifyCollectionChanged;
+                if (_sourceNcc != null)
+                {
+                    _sourceNcc.CollectionChanged += _sourceNcc_CollectionChanged;


I think this should be a weak listener

dotMorten · 2016-10-06T05:48:34Z

Microsoft.Toolkit.Uwp.UI.Controls/AdvancedCollectionViewSource/AdvancedCollectionViewSource.cs

+            MoveCurrentTo(currentItem);
+        }
+
+        private void _sourceNcc_CollectionChanged(object sender, NotifyCollectionChangedEventArgs e)


Isn't the naming of this method creating a warning?

Also fixed.

dotMorten · 2016-10-06T05:48:56Z

Microsoft.Toolkit.Uwp.UI.Controls/AdvancedCollectionViewSource/AdvancedCollectionViewSource.cs

+            switch (e.Action)
+            {
+                case NotifyCollectionChangedAction.Add:
+                    if (e.NewItems.Count == 1)


NewItems could be null

dotMorten · 2016-10-06T06:00:40Z

Microsoft.Toolkit.Uwp.UI.Controls/AdvancedCollectionViewSource/AdvancedCollectionViewSource.cs

+        /// </summary>
+        /// <param name="item">item</param>
+        /// <returns>success of operation</returns>
+        public bool MoveCurrentTo(object item) => item == CurrentItem || MoveCurrentToIndex(IndexOf(item));


Why have this when you already have the CurrentItem property that is get and set?

ICollectionView implementation.

dotMorten · 2016-10-06T06:02:19Z

Microsoft.Toolkit.Uwp.UI.Controls/AdvancedCollectionViewSource/AdvancedCollectionViewSource.cs

+        /// <exception cref="NotImplementedException">Not implemented yet...</exception>
+        public IAsyncOperation<LoadMoreItemsResult> LoadMoreItemsAsync(uint count)
+        {
+            throw new NotImplementedException("todo...");


Needs to be completed. I assume it's just a matter of checking if the source implements ISupportIncrementalLoading and call LoadMoreItemsAsync on that ?

dotMorten · 2016-10-06T06:04:15Z

Microsoft.Toolkit.Uwp.UI.Controls/AdvancedCollectionViewSource/AdvancedCollectionViewSource.cs

+        /// <summary>
+        /// Gets a value indicating whether the source has more items
+        /// </summary>
+        public bool HasMoreItems => false; // todo


Isn't this just:

public bool HasMoreItems => (_source as ISupportIncrementalLoading)?.HasMoreItems ?? false;

Yep, it is - added.

dotMorten · 2016-10-06T06:06:48Z

Microsoft.Toolkit.Uwp.UI.Controls/AdvancedCollectionViewSource/AdvancedCollectionViewSource.cs

+        /// <summary>
+        /// Occurs when the vector changes.
+        /// </summary>
+        public event VectorChangedEventHandler<object> VectorChanged;


Why not INotifyCollectionChanged instead?

This is required IObservableVector implementation.

dotMorten · 2016-10-06T06:08:47Z

Microsoft.Toolkit.Uwp.UI.Controls/AdvancedCollectionViewSource/VectorChangedEventArgs.cs

+    /// <summary>
+    /// Vector changed EventArgs
+    /// </summary>
+    public class VectorChangedEventArgs : IVectorChangedEventArgs


I think this class can be kept internal. It's enough to have IVectorChangedEventArgs public which is already part of WinRT

tomzorz · 2016-10-08T00:25:45Z

Thanks for the reviews - I'll try to work on the issues this weekend.

Pull from main MS repo

…ixes and other misc. changes.

tomzorz · 2016-10-09T00:13:54Z

Aaand, third time's the charm :) I left some questions above before proceeding with certain fixes.

deltakosh · 2016-10-10T17:26:45Z

This is really good work! It 's gonna be a truly awesome addition to the toolkit

deltakosh · 2016-10-18T22:37:18Z

Hey! How things are going here?

tomzorz · 2016-10-19T01:02:17Z

@deltakosh got an exam tomorrow - I'm planning on finishing the remaining issues later this week :)

deltakosh · 2016-10-19T17:46:01Z

Ouch!! good luck :)

Getting new changes

deltakosh · 2016-10-31T15:15:32Z

Ok keep me posted. There is no problem to flag it for 1.3

tomzorz · 2016-11-01T21:26:51Z

@deltakosh Let's flag it for 1.3 - I want to do this right, but I don't have time for right until 1.2 :D (maybe I'll have some down time during the summit)

deltakosh · 2016-11-01T21:30:16Z

Completely aligned!

tomzorz · 2016-12-02T10:49:52Z

I haven't forgot about the PR - definitely planning on to finish it before 1.3 :)

deltakosh · 2016-12-02T13:39:09Z

OK thanks:)

jgiacomini · 2016-12-27T13:00:56Z

Microsoft.Toolkit.Uwp.UI/AdvancedCollectionViewSource/ICollectionViewEx.cs

+    /// <summary>
+    /// Extended ICollectionView with filtering and sorting
+    /// </summary>
+    public interface ICollectionViewEx : ICollectionView


Why not call it "IAdvancedCollectionView" to match the class name.
Or "ICollectionViewSource" if the class name is named "CollectionViewSource"

Agreed, I'll do that.

jgiacomini · 2016-12-27T13:21:06Z

Microsoft.Toolkit.Uwp.UI/AdvancedCollectionViewSource/AdvancedCollectionViewSource.cs

+        /// <summary>
+        /// Gets or sets the predicate used to filter the visisble items
+        /// </summary>
+        public Predicate<object> Filter


I think we have to create a delegate type rather than use Predicate.
If we want to add new behavior on the filtering it will be easy without make breaking changes.
In the Silverlight implementation of CollectionViewSource they create a delegate called FilterEventHandler.

I'd personally go with the functional-ish/linq-ish style of the predicate, rather than have events instead. Also we can always ease-in breaking changes by marking things obsolete first. But I guess this is open for discussion.

jgiacomini · 2016-12-27T14:55:53Z

Very good work. I cant wait to have it in the toolkit. :)

jgiacomini · 2016-12-29T09:14:33Z

Microsoft.Toolkit.Uwp.UI/AdvancedCollectionViewSource/ICollectionViewEx.cs

+        bool CanFilter { get; }
+
+        /// <summary>
+        /// Gets or sets the predicate used to filter the visisble items


In the summary block => "visisble" must be "visible"

deltakosh · 2017-01-03T18:55:06Z

Up? :) and Happy new year 💃

tomzorz · 2017-01-03T19:08:12Z

@deltakosh Yes! Sorry, planning on wrapping it up this weekend.

deltakosh · 2017-01-03T19:09:21Z

You rock!

Merge from MS

# Conflicts: # UnitTests/UnitTests.csproj

tomzorz · 2017-01-07T18:48:19Z

So theoretically this is it, pinging @deltakosh for a final re-check.

hermitdave · 2017-01-08T09:25:38Z

Microsoft.Toolkit.Uwp.UI/AdvancedCollectionView/SortDescription.cs

+    /// <summary>
+    /// Sort description
+    /// </summary>
+    public class SortDescription


Why have you named it SortDescription ? shouldn't it be SortDirection?

It's not just a direction - it describes a sorting step: a property to sort on plus the direction. So you can have more of these, and for example it'll sort by prop A asc, then by prob B desc.

Whilst its specifying a sort, calling it a SortDescription is misleading. I think you should call it SortDirection, SortEntry or something else. yes I get that you can add more than one

Maybe SortLevel, to match the phrasing that Excel uses?

I'm for SortDescription actually. It makes sense to me

deltakosh · 2017-01-10T17:07:31Z

Microsoft.Toolkit.Uwp.SampleApp/Microsoft.Toolkit.Uwp.SampleApp.csproj.DotSettings

@@ -0,0 +1,2 @@
+<wpf:ResourceDictionary xml:space="preserve" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:s="clr-namespace:System;assembly=mscorlib" xmlns:ss="urn:shemas-jetbrains-com:settings-storage-xaml" xmlns:wpf="http://schemas.microsoft.com/winfx/2006/xaml/presentation">


This file should not be in the repo

deltakosh · 2017-01-10T17:08:57Z

Microsoft.Toolkit.Uwp.UI.Controls/Microsoft.Toolkit.Uwp.UI.Controls.csproj

@@ -273,7 +273,7 @@
    <CodeAnalysisRuleSet>microsoft.toolkit.uwp.ui.controls.ruleset</CodeAnalysisRuleSet>
  </PropertyGroup>
  <Import Project="$(MSBuildExtensionsPath)\Microsoft\WindowsXaml\v$(VisualStudioVersion)\Microsoft.Windows.UI.Xaml.CSharp.targets" />
-  <!-- To modify your build process, add your task inside one of the targets below and uncomment it. 


not necessary update ;)

Hah, apparently VS was automatically removing that space from the end of the line any time I saved... had to edit it back in VSCode :)

deltakosh

Small changes only..Really good job

tomzorz · 2017-01-10T18:41:32Z

@deltakosh done :) Thanks!

deltakosh · 2017-01-10T23:12:16Z

@dotMorten @hermitdave Fancy doing a second check? Thanks!

hermitdave · 2017-01-10T23:32:06Z

@deltakosh I already went through the code.. I don't see any significant changes since then.. maybe @dotMorten can take a look.. good from my side

deltakosh · 2017-01-11T00:43:02Z

Do you mind flagging it as approved?

tomzorz added 3 commits September 18, 2016 21:34

AdvancedCollectionViewSource WIP #1

242c3be

Merge pull request #1 from Microsoft/dev

e711138

Update from upstream

Merge branch 'dev' into collectionviewsource

bd2e459

msftclas added the cla-required label Sep 18, 2016

msftclas added cla-signed and removed cla-required labels Sep 18, 2016

tomzorz changed the title ~~AdvancedCollectionViewSource WIP~~ [WIP] AdvancedCollectionViewSource Sep 19, 2016

deltakosh added this to the v1.2 milestone Sep 26, 2016

deltakosh reviewed Oct 5, 2016

View reviewed changes

dotMorten reviewed Oct 6, 2016

View reviewed changes

tomzorz added 5 commits October 9, 2016 00:19

Merge pull request #2 from Microsoft/dev

a4d51b6

Pull from main MS repo

ISupportIncrementalLoading implemented, style fixes, property order f…

5865c80

…ixes and other misc. changes.

proper csproj

3cb8ee1

Merge branch 'dev' into collectionviewsource

7ae07c0

Proper headers added.

cbcef0d

tomzorz added 5 commits October 21, 2016 10:05

Merge pull request #3 from Microsoft/dev

e52b742

Getting new changes

Merge pull request #4 from Microsoft/dev

4b0c269

Getting new changes

Merge branch 'dev' into collectionviewsource

0c90652

added inheritdoc tags, and commented out the grouping parts for now

c717d39

moved the whole ACVS to UI

52da7d2

deltakosh modified the milestones: v1.3, v1.2 Nov 1, 2016

jgiacomini suggested changes Dec 27, 2016

View reviewed changes

jgiacomini reviewed Dec 29, 2016

View reviewed changes

tomzorz added 4 commits January 7, 2017 08:43

Merge pull request #5 from Microsoft/dev

91f09a8

Merge from MS

Merge branch 'dev' into collectionviewsource

4c887d5

# Conflicts: # UnitTests/UnitTests.csproj

Fixed the naming plus corrected a typo

0a5c6c9

Added AdvancedCollectionView sample

5ae03f6

hermitdave reviewed Jan 8, 2017

View reviewed changes

deltakosh reviewed Jan 10, 2017

View reviewed changes

deltakosh suggested changes Jan 10, 2017

View reviewed changes

Final requested changes

21a32b8

deltakosh approved these changes Jan 10, 2017

View reviewed changes

hermitdave approved these changes Jan 11, 2017

View reviewed changes

hermitdave merged commit 1d409d4 into CommunityToolkit:dev Jan 11, 2017

		@@ -0,0 +1,2 @@
		<wpf:ResourceDictionary xml:space="preserve" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:s="clr-namespace:System;assembly=mscorlib" xmlns:ss="urn:shemas-jetbrains-com:settings-storage-xaml" xmlns:wpf="http://schemas.microsoft.com/winfx/2006/xaml/presentation">

[WIP] AdvancedCollectionViewSource #349

[WIP] AdvancedCollectionViewSource #349

Conversation

tomzorz commented Sep 18, 2016 • edited

msftclas commented Sep 18, 2016

msftclas commented Sep 18, 2016

deltakosh commented Sep 19, 2016 • edited

tomzorz commented Sep 19, 2016

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

dotMorten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotMorten Oct 6, 2016 • edited

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

Choose a reason for hiding this comment

dotMorten Oct 6, 2016 • edited

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

dotMorten Oct 6, 2016 • edited

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

tomzorz commented Oct 8, 2016

tomzorz commented Oct 9, 2016

deltakosh commented Oct 10, 2016

deltakosh commented Oct 18, 2016

tomzorz commented Oct 19, 2016

deltakosh commented Oct 19, 2016

deltakosh commented Oct 31, 2016

tomzorz commented Nov 1, 2016

deltakosh commented Nov 1, 2016

tomzorz commented Dec 2, 2016

deltakosh commented Dec 2, 2016

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

jgiacomini commented Dec 27, 2016

Choose a reason for hiding this comment

deltakosh commented Jan 3, 2017

tomzorz commented Jan 3, 2017

deltakosh commented Jan 3, 2017

tomzorz commented Jan 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomzorz Jan 8, 2017 • edited

Choose a reason for hiding this comment

deltakosh Jan 10, 2017 • edited

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

deltakosh left a comment

Choose a reason for hiding this comment

tomzorz commented Jan 10, 2017

deltakosh commented Jan 10, 2017

hermitdave commented Jan 10, 2017

deltakosh commented Jan 11, 2017

tomzorz commented Sep 18, 2016 •

edited

deltakosh commented Sep 19, 2016 •

edited

dotMorten Oct 6, 2016 •

edited

dotMorten Oct 6, 2016 •

edited

dotMorten Oct 6, 2016 •

edited

tomzorz Jan 8, 2017 •

edited

deltakosh Jan 10, 2017 •

edited