[Sync] Replace extras Bundle by explicit arguments
#1475
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the sync managers by removing the outdated extras parameter and replacing it with a ResyncType parameter to standardize re-synchronization logic. The changes span across sync workers, syncers, sync managers, and associated tests, ensuring consistent behavior when triggering re-sync events.
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| AccountSettingsModel.kt | Updated resyncCalendars and resync calls to use ResyncType instead of boolean flags. |
| SyncWorkerManager.kt | Replaced old extras/resync parameter with ResyncType and renamed upload parameter to syncFrameworkUpload. |
| BaseSyncWorker.kt | Adjusted syncer creation to pass resyncType and syncFrameworkUpload to relevant syncers. |
| TasksSyncManager.kt, TaskSyncer.kt, JtxSyncer.kt, etc. | Updated constructors and factory methods to remove extras and use ResyncType consistently. |
| SyncManager.kt, ContactsSyncManager.kt, CalendarSyncer.kt, etc. | Removed extras parameter in favor of resync, with corresponding documentation updates. |
| Test Files | Updated test setup code to reflect removal of extras and setting resync to null where appropriate. |
rfc2822
changed the title
extras Bundle by explicit arguments
| package at.bitfire.davdroid.sync | ||
|
|
||
| /** | ||
| * Used to signal that re-synchronization is requested during a sync. |
There was a problem hiding this comment.
I know calling it "re-synchronization" is a relic from the sync framework, but I have always found it very confusing, so maybe we can change the term to "sync-size" or something similar?
I don't know of a better way to indicate that this enum and sentence is about the amount of data (partial or total/full) being synchronized. As it stands now, one (including me) might think that it speaks about signaling a new sync request while there is one running already.
Since SyncType would be confusing with the sync data type, we could call the enum SyncSize or SyncAmount instead of ResyncType?
Or just writing: "Used to signal that partial or total re-synchronization is requested during a sync." would help a lot already.
If you don't want to, then we don't need to update it in all places/usages/kdoc, but here would be nice, because, yeah I find the term "re-synchronization" alone very confusing everytime I see it, until I remind myself, that it's actually about partial/full synchronization. 😅
There was a problem hiding this comment.
I know calling it "re-synchronization" is a relic from the sync framework, but I have always found it very confusing, so maybe we can change the term to "sync-size" or something similar?
No, the normal synchronization is without re-synchronization, so resync = null.
Re-synchronization is not related to the sync framework and really means that the sync algorithm should query (and download) the entries again and not skip synchronization when the sync-token stayed the same since the last sync.
I think SyncSize or amount would make it less clear what it is.
There was a problem hiding this comment.
Ah too bad then, but I can see there could also be confusion with the other names.
app/src/main/kotlin/at/bitfire/davdroid/sync/worker/BaseSyncWorker.kt
Outdated
Show resolved
Hide resolved
Purpose
Currently, we're using an
extrasBundleto encode and pass around sync arguments:This is not clean because it's not explicit what arguments can be passed over a
Bundleand what arguments then are actually used. Also there can be conversion errors when converting from/to aBundle.Short description
This PR removes the
Bundleand replaces it by explicit parameters.There's now an explicit
ResyncTypethat explains the two types of re-synchronization.Checklist