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

Fix: AdvancedCollection inserts at wrong position when adding to end of source collection #309

Merged
merged 6 commits into from
Mar 1, 2024
Merged

Fix: AdvancedCollection inserts at wrong position when adding to end of source collection #309

merged 6 commits into from
Mar 1, 2024

Conversation

Poker-sang
Copy link
Contributor

@Poker-sang Poker-sang commented Dec 30, 2023

Fixes

closes #307

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

What is the new behavior?

The new item is now added to the end of the collection.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
  • Based off latest main branch of toolkit
  • Tested code with current supported SDKs
  • New component
    • Documentation has been added
    • Sample in sample app has been added
    • Analyzers are passing for documentation and samples
    • Icon has been created (if new sample) following the Thumbnail Style Guide and templates
  • Tests for the changes have been added (if applicable)
  • Header has been added to all new source files
  • Contains NO breaking changes

Other information

@niels9001 niels9001 requested a review from Arlodotexe January 5, 2024 15:41
@Arlodotexe
Copy link
Member

Quickly testing here, list.Insert(list.Count - 1, newItem); would insert into the second-to-last position rather than the first.

There's no exceptions thrown when you list.Insert(list.Count, newItem); rather than calling list.Add().

Since the intention is to keep the List<object> _view field in sync with the IList _source field, this change should fix an issue where items aren't added to the end of the AdvancedCollectionView when the underlying source collection is ObservableCollection and updated.

I'll set up a quick minimal repro so we can observe the issue and make sure this fix does what we want.

@Arlodotexe Arlodotexe changed the title Fix: Wired logic in AdvancedCollectionView Fix: AdvancedCollection inserts at wrong position when adding to end of source collection Jan 26, 2024
@abdes
Copy link
Contributor

abdes commented Jan 26, 2024

The impact of this PR is really minimal, and it fixes an obvious bug. Please review and move forward.

@abdes
Copy link
Contributor

abdes commented Feb 28, 2024

@Arlodotexe what's the additional work needed here? Can we get this merged or can we get a review comment for what else needs to be done so we can get it done? This is a pretty bad bug as it breaks an important assumption of the AdvancedCollection staying in sync with the original collection.

@Lightczx
Copy link

Lightczx commented Feb 29, 2024

Honestly, I already copied the source code into my project, and even refactored it into generic compat
waiting for the package to get updated might take us several mouths.

@abdes
Copy link
Contributor

abdes commented Mar 1, 2024

Honestly, I already copied the source code into my project, and even refactored it into generic compat waiting for the package to get updated might take us several mouths.

Care to share the link to your improved version @Lightczx ?

@Poker-sang
Copy link
Contributor Author

Honestly, I already copied the source code into my project, and even refactored it into generic compat waiting for the package to get updated might take us several mouths.

Me too. But the new controls ItemsView&ItemsRepeater which can replace ListView&GridView, do not support ICollectionView. So, I dropped ICollectionView and reimplemented it based on ObservableCollection<T>.

@abdes
Copy link
Contributor

abdes commented Mar 1, 2024

Well, if that's the new model of Microsoft OpenSource projects, then so be it. We just copy the code, fix it an use it... Long live the MIT License.

Any of the maintainers care to comment on why Pull Requests submitted, even after thorough explanation, testing and due diligence by the submitter, get stuck at "Review Required" with no feedback?

@Arlodotexe
Copy link
Member

Arlodotexe commented Mar 1, 2024

Any of the maintainers care to comment on why Pull Requests submitted, even after thorough explanation, testing and due diligence by the submitter, get stuck at "Review Required" with no feedback?

We're still putting together the 8.1 update and in addition to features and bug fixes, we've been ironing out various tooling issues that have been cropping up. Adding net8-windows support to Wasdk caused significantly more trouble than we anticipated, PowerShell got updated underneath us and broke CI, the new ColorPicker package is behaving strangely when consumed in uwp, the list goes on.

You can follow along and see where we're at in the current iteration of our project board for 8.x.

We just recently closed off some of the larger issues we were having (again, see the .net8 prs [1] [2]), so I plan to start closing off the smaller bugfix PRs now (like this one).

Thanks for your patience 🙂

@abdes
Copy link
Contributor

abdes commented Mar 1, 2024

Thank you for the update.

@Arlodotexe
Copy link
Member

Arlodotexe commented Mar 1, 2024

@Poker-sang Good news! I've fleshed out our unit tests and was able to reproduce the issue described. I can confirm that the changes you've made fixes the broken test case, so we're good to close once CI passes 🚀

@Arlodotexe Arlodotexe enabled auto-merge (rebase) March 1, 2024 22:10
@Arlodotexe Arlodotexe added this to the 8.1 milestone Mar 1, 2024
@Arlodotexe Arlodotexe merged commit 7b8f241 into CommunityToolkit:main Mar 1, 2024
9 checks passed
@Poker-sang Poker-sang deleted the Poker-sang/advancedcollectionview-fixes branch March 2, 2024 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
5 participants