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

[v11] 12. Raise Replace events for collection notifications #3254

Merged
merged 1 commit into from
Mar 24, 2023

Conversation

nirinchev
Copy link
Member

Description

Fixes #2854

This doesn't technically need v11, but it would have conflicted, so I decided to do it on top of the nullable annotations branch.

TODO

  • Changelog entry
  • Tests (if applicable)

@nirinchev nirinchev requested a review from papafe February 22, 2023 11:01
@cla-bot cla-bot bot added the cla: yes label Feb 22, 2023
@nirinchev nirinchev self-assigned this Feb 22, 2023
@coveralls
Copy link

coveralls commented Feb 22, 2023

Pull Request Test Coverage Report for Build 4513122303

  • 11 of 11 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 81.81%

Totals Coverage Status
Change from base Build 4513109284: 0%
Covered Lines: 5875
Relevant Lines: 7056

💛 - Coveralls


// Only raise specialized notifications if we have exactly one change type to report
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we compute the replaced notifications after added and removed so we don't need to raise Reset in case we have added + replaced or removed + replaced?
With replaced the index should be stable, so I suppose it could work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to avoid special-casing this - if you use foo[i] = new Obj(...), you'll get a replaced index. If you do foo.RemoveAt(i); foo.Insert(i, new Obj(...)), I'd be reluctant to translate that into a replacement, although maybe it'll work better with animations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you said, in the second case that will be a reset though, so the whole list will be updated unnecessarily. That's annoying especially if the index is out of the visible collection on screen, and there could be some usability issues if the user is currently interacting with the list.
Generally, we should try to avoid raising resets, but that would probably require to make this method much more complex.
We could also keep it as you wrote it now, and create another ticket to spend some time trying to reduce resets to the minimum.

@nirinchev nirinchev force-pushed the ni/v11-nullability-realm branch from 6502628 to 0f14da6 Compare February 24, 2023 21:58
@nirinchev nirinchev force-pushed the ni/v11-nullability-realm branch from 630f7ca to eb80d9d Compare March 24, 2023 05:46
Base automatically changed from ni/v11-nullability-realm to main March 24, 2023 16:05
@nirinchev nirinchev force-pushed the ni/replace-notifications branch from 58f92c1 to eb4db4c Compare March 24, 2023 16:06
@nirinchev nirinchev merged commit 800da2e into main Mar 24, 2023
@nirinchev nirinchev deleted the ni/replace-notifications branch March 24, 2023 17:11
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: NotifyCollectionChangedAction.Replace and Reset is not fired on RealmCollection
3 participants