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

[Push] Upon notification, only enqueue sync for the respective service type #1175

Conversation

ArnyminerZ
Copy link
Member

Purpose

When receiving a push notification, do not enqueue sync for all authorities, schedule only for the collection type's authority.

Short description

After receiving a push notification, after the collection and service are loaded, the collection type is checked. Those authorities are scheduled depending on the type:

Type Authority
TYPE_ADDRESSBOOK at.bitfire.davdroid.addressbooks (R.string.address_books_authority)
TYPE_CALENDAR CalendarContract.AUTHORITY
TYPE_WEBCAL CalendarContract.AUTHORITY

Also, if the collection supports VJOURNAL or VTODO, a sync is enqueued for the current tasks provider, if any.

If the type received is unknown or invalid, no sync is scheduled. We could change this behavior to sync everything, whatever you think suits best.

Checklist

  • The PR has a proper title, description and label.
  • I have self-reviewed the PR.
  • I have added documentation to complex functions and functions that can be used by other modules.
  • I have added reasonable tests or consciously decided to not add tests.

@ArnyminerZ ArnyminerZ self-assigned this Dec 16, 2024
@ArnyminerZ ArnyminerZ added enhancement New feature or request refactoring Internal improvement of existing functions and removed enhancement New feature or request labels Dec 16, 2024
@ArnyminerZ
Copy link
Member Author

I have still not tried it, the emulator is not working on my laptop for some reason (not enough RAM I think). I will test this ASAP.

Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@rfc2822 rfc2822 force-pushed the 1152-push-upon-notification-only-enqueue-sync-for-the-respective-service-type branch from d5be215 to f3d8911 Compare December 27, 2024 08:06
@rfc2822
Copy link
Member

rfc2822 commented Dec 27, 2024

Since #1177, the sync workers are per SyncDataType and not per authority.

@ArnyminerZ Can you please adapt the PR accordingly? It should be much shorter now.

Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@ArnyminerZ ArnyminerZ marked this pull request as ready for review December 27, 2024 09:01
@ArnyminerZ ArnyminerZ requested a review from rfc2822 December 27, 2024 09:02
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@ArnyminerZ ArnyminerZ requested a review from rfc2822 December 27, 2024 12:01
Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@ArnyminerZ ArnyminerZ requested a review from rfc2822 December 27, 2024 16:28
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

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

Looks good! Just two style comments:

  • In this project curly braces are usually not used for one-line statements. I know that usage of then-blocks with/out curly braces is a matter of dispute, but I generally prefer the variant without extra curly braces because it makes it more hard to read for me personally. Of course there may be some special cases where curly braces are useful to make clear what's going on even if not strictly necessary (nested ifs etc).
  • Instead of add, we also can use +=. Makes it a bit shorter.

Signed-off-by: Arnau Mora Gras <arnyminerz@proton.me>
@ArnyminerZ ArnyminerZ requested a review from rfc2822 December 28, 2024 16:47
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

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

Tested with an event and works very well!

@rfc2822 rfc2822 merged commit 794007f into main-ose Dec 29, 2024
8 checks passed
@rfc2822 rfc2822 deleted the 1152-push-upon-notification-only-enqueue-sync-for-the-respective-service-type branch December 29, 2024 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Internal improvement of existing functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Push] Upon notification, only enqueue sync for the respective service type
2 participants