Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Range Manipulation APIs to Collection<T> and ObservableCollection<T> #23018

Merged
merged 17 commits into from Mar 8, 2019

Conversation

Projects
None yet
5 participants
@ahoefling
Copy link
Contributor

ahoefling commented Mar 5, 2019

Added Range Manipulation APIs to Collection and ObservableCollection.

Part of : dotnet/corefx#10752
Part of: dotnet/corefx#35772

Added the following new API that was approved:

    // Adds a range to the end of the collection.
    // Raises CollectionChanged (NotifyCollectionChangedAction.Add)
    public void AddRange(IEnumerable<T> collection) => InsertItemsRange(0, collection);

    // Inserts a range
    // Raises CollectionChanged (NotifyCollectionChangedAction.Add)
    public void InsertRange(int index, IEnumerable<T> collection) => InsertItemsRange(index, collection);

    // Removes a range.
    // Raises CollectionChanged (NotifyCollectionChangedAction.Remove)
    public void RemoveRange(int index, int count) => RemoveItemsRange(index, count);

    // Will allow to replace a range with fewer, equal, or more items.
    // Raises CollectionChanged (NotifyCollectionChangedAction.Replace)
    public void ReplaceRange(int index, int count, IEnumerable<T> collection)
    {
         RemoveItemsRange(index, count);
         InsertItemsRange(index, collection);
    }

    #region virtual methods
    protected virtual void InsertItemsRange(int index, IEnumerable<T> collection);
    protected virtual void RemoveItemsRange(int index, int count);
    #endregion

Looking forward to everyone's feedback on this

Added new Range Manipulation APIs for Collection<T> which propogate u…
…p to ObservableCollection<T>. AddRange, InsertRange, RemoveRange and Replace Range
@ahsonkhan

This comment has been minimized.

Copy link
Member

ahsonkhan commented Mar 5, 2019

Based on dotnet/corefx#10752 (comment), you are missing ReplaceItemsRange

@ahoefling

This comment has been minimized.

Copy link
Contributor Author

ahoefling commented Mar 5, 2019

Based on dotnet/corefx#10752 (comment), you are missing ReplaceItemsRange

Thanks for pointing this out, I was going off the update at the top and missed this API. I'll get this added in the next update and include tests in the associated PR for CoreFX

ahoefling added some commits Mar 5, 2019

ahoefling added some commits Mar 5, 2019

ahoefling added some commits Mar 5, 2019

Updated RemoveRange invocation of RemoveAt to use index instead of i,…
… because the array changes with each iteration of the for loop and will cause side-effects which may include index out of range exceptions. This needs to be index because as the array shrinks the index is always at the correct position.
Updated RemoveAt->RemoveItem. This change removes redundent validatio…
…n checks that happen using the RemoveAt API. RemoveItem gives us direct access to invoke the command
Updated InsertItemsRange to simplify the expression and added perform…
…ance improvements. If the underlying `items` is using List<T> we should use it's InsertRange method since it is optimized, othersie we use InsertItem
Added new RemoveRange validation check to see if the resulting range …
…(index + count) > items.Count and if it is true throw ArgumentException

ahoefling added some commits Mar 6, 2019

@ahsonkhan
Copy link
Member

ahsonkhan left a comment

Some leftover comments. Otherwise, LGTM.

@safern

safern approved these changes Mar 6, 2019

Copy link
Member

safern left a comment

LGTM once @ahsonkhan's last comments are solved.

@safern safern merged commit d29c78b into dotnet:master Mar 8, 2019

24 checks passed

CentOS7.1 x64 Checked Innerloop Build and Test Build finished.
Details
CentOS7.1 x64 Debug Innerloop Build Build finished.
Details
OSX10.12 x64 Checked Innerloop Build and Test Build finished.
Details
Ubuntu arm Cross Checked Innerloop Build and Test Build finished.
Details
Ubuntu arm Cross Checked crossgen_comparison Build and Test Build finished.
Details
Ubuntu arm Cross Checked no_tiered_compilation_innerloop Build and Test Build finished.
Details
Ubuntu arm Cross Release crossgen_comparison Build and Test Build finished.
Details
Ubuntu x64 Checked CoreFX Tests Build finished.
Details
Ubuntu x64 Checked Innerloop Build and Test Build finished.
Details
Ubuntu x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0) Build finished.
Details
Ubuntu x64 Formatting Build finished.
Details
Windows_NT x64 Checked CoreFX Tests Build finished.
Details
Windows_NT x64 Checked Innerloop Build and Test Build finished.
Details
Windows_NT x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0) Build finished.
Details
Windows_NT x64 Formatting Build finished.
Details
Windows_NT x64 Release CoreFX Tests Build finished.
Details
Windows_NT x64 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x64 min_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 Checked Innerloop Build and Test Build finished.
Details
Windows_NT x86 Checked Innerloop Build and Test (Jit - TieredCompilation=0) Build finished.
Details
Windows_NT x86 Release Innerloop Build and Test Build finished.
Details
Windows_NT x86 full_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
Windows_NT x86 min_opt ryujit CoreCLR Perf Tests Correctness Build finished.
Details
license/cla All CLA requirements met.
Details
@safern

This comment has been minimized.

Copy link
Member

safern commented Mar 8, 2019

Thanks @ahoefling now we can move to corefx's PR once CoreCLR flows there 😄

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Mar 8, 2019

@safern Try to hit squash & merge next time ... I know it is easy to press the wrong button by accident.

@safern

This comment has been minimized.

Copy link
Member

safern commented Mar 8, 2019

@safern Try to hit squash & merge next time ... I know it is easy to press the wrong button by accident.

Yeah I just realized I rebased and merge. Sorry about that, my default was "Squash and merge" I don't know how it was changed 😭

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.