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

Don't call MapDataWithEditsSource listeners if nothing changed #4472

Merged
merged 5 commits into from
Oct 14, 2022

Conversation

Helium314
Copy link
Collaborator

fixes #4077

This is a longer version of what was linked in #4077 (comment), but it's not doing more work and I think it's easier to read that way.

Copy link
Member

@westnordost westnordost left a comment

Choose a reason for hiding this comment

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

Some questions... also, should add tests as this is a critical section of code

Also:

but it's not doing more work and I think it's easier to read that way.

Thank you, yes!

Co-authored-by: Tobias Zwick <newton@westnordost.de>
@westnordost
Copy link
Member

What should be tested:

  • does not call onUpdated when all deleted elements are already deleted
  • does not call onUpdated when all updated elements stayed the same
  • does not call onUpdated when all updated elements stayed the same except for version and timestamp
  • does call onUpdated when not all deleted elements are already deleted
  • does call onUpdated when not all updated elements stayed the same

@westnordost
Copy link
Member

In terms of numbers, I guess it is reasonable to claim 2x faster quest solving (with auto-sync on) and 2x faster upload times?

westnordost added a commit that referenced this pull request Oct 9, 2022
necessary because edits do not refer to specific elements they edit anymore (i.e. they are more of a black box)

incidentally this actually reimplements  #4472
@Helium314
Copy link
Collaborator Author

I added the tests, but the last one fails.
I think this is because using mapDataChangesAre the changes are not actually edits applied on top of some initial elements... how do I do that?

@westnordost
Copy link
Member

Uhm, not sure I understand the problem. But mapDataChangesAre is just a helper function. You can create a different helper function as you like you fix it if it does not do what one would expect.

@westnordost
Copy link
Member

westnordost commented Oct 13, 2022

Maybe I ran into the same problem as you when adding tests to the now dismissed #4481 PR. Shouldn't you be able to use editsControllerNotifiesMapDataChangesAdded?

It simulatest that the EditsController notifies the MapDataWithEditsSource class about new changes that have been added.

@Helium314
Copy link
Collaborator Author

Thanks! editsControllerNotifiesMapDataChangesAdded and originalElementsAre made it work as intended.

@westnordost westnordost merged commit 2499d77 into streetcomplete:master Oct 14, 2022
@westnordost westnordost deleted the Helium314-patch-2 branch October 14, 2022 09:45
@FloEdelmann FloEdelmann added the hacktoberfest-accepted pull request that should be treated as eligible for Hacktoberfest event label Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted pull request that should be treated as eligible for Hacktoberfest event
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve performance on upload: Notice if nothing changed
3 participants