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.

Sign up for GitHub

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

Jump to bottom

Sync on collection update #1240

Open
wants to merge 7 commits into
base: main-ose
Choose a base branch
from
Open

Sync on collection update #1240

wants to merge 7 commits into from
+106 −18

Conversation

sunkup
Copy link
Member

@sunkup sunkup commented Jan 14, 2025 •
edited
Loading

Purpose

Nice to have for:

Short description

  • Pass account of the changed collection along when notifying listeners of collections change.
  • Then have SyncWorkerManager listen to any collection changes and use the account passed along to
  • then trigger a short delay-coroutine job, which resets itself on new change observations and after delay expiration
  • enqueues a one time sync of the whole account (not just the service which the collection belongs to - although that would prob be possible as well).

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.

@sunkup sunkup added the enhancement New feature or request label Jan 14, 2025
@sunkup sunkup self-assigned this Jan 14, 2025
This was linked to issues Jan 14, 2025
"Force read-only" setting not effective immediately #851
Open
Automatically sync when collections are (un)checked #1230
Open
@sunkup sunkup requested a review from ArnyminerZ January 14, 2025 15:23
@sunkup sunkup marked this pull request as ready for review January 14, 2025 15:23
rfc2822
rfc2822 reviewed Jan 18, 2025
View reviewed changes
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.

I wonder how this behaves when the services are refreshed. Doesn't it enqueue a lot of syncs during service detection (whenever a collection is updated)?

My ideas (may or may not make sense):

  • A delay of a few seconds between the collection change and the sync (like for the push registration) could at least reduce the number of synchronizations during service detection.
  • Or maybe pause it somehow?
  • Maybe separate callbacks for the actions we want to observe (update read-only, update sync flag)? Has advantages and disadvantages.

app/src/main/kotlin/at/bitfire/davdroid/sync/worker/SyncWorkerManager.kt Outdated Show resolved Hide resolved
@sunkup sunkup removed the request for review from ArnyminerZ January 20, 2025 16:07
@sunkup sunkup marked this pull request as draft January 20, 2025 16:07
@sunkup sunkup force-pushed the 851-force-read-only-setting-not-effective-immediately branch 2 times, most recently from 945029e to 7f58df8 Compare January 21, 2025 09:24
@sunkup sunkup force-pushed the 851-force-read-only-setting-not-effective-immediately branch from f9a0940 to 7d4bca1 Compare January 21, 2025 09:45
@sunkup sunkup requested a review from ArnyminerZ January 21, 2025 10:13
@sunkup sunkup marked this pull request as ready for review January 21, 2025 10:13
@rfc2822
Copy link
Member

rfc2822 commented Jan 27, 2025

Hm, because we're not sure about unwanted consequences (regarding service detection etc)

Maybe we could the sync from the UI? So that not the DB Collection is observed, but instead when the user changes the read-only / sync flag in the UI, it waits a few seconds and then starts a sync?

@sunkup
Copy link
Member Author

sunkup commented Jan 27, 2025 •
edited
Loading

Hm, because we're not sure about unwanted consequences (regarding service detection etc)

Maybe we could the sync from the UI? So that not the DB Collection is observed, but instead when the user changes the read-only / sync flag in the UI, it waits a few seconds and then starts a sync?

Sure, we should drop #1230 then though, since we don't want that feature anymore?

@rfc2822
Copy link
Member

rfc2822 commented Jan 27, 2025

Sure, we should drop #1230 then though, since we don't want that feature anymore?

I mean we could sync when they're (un)checked by the user in the UI, but not when the sync flag is modified by for instance some background code.

@rfc2822
Copy link
Member

rfc2822 commented Jan 27, 2025

But maybe let's do it after the other 4.5 things and could also be good to discuss (dis)advantages in a talk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rfc2822 rfc2822 rfc2822 left review comments

@ArnyminerZ ArnyminerZ Awaiting requested review from ArnyminerZ

At least 1 approving review is required to merge this pull request.

Assignees

@sunkup sunkup

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Automatically sync when collections are (un)checked "Force read-only" setting not effective immediately
2 participants
@sunkup @rfc2822