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

Generalize syncer #907

Merged
merged 53 commits into from
Aug 5, 2024
Merged

Generalize syncer #907

merged 53 commits into from
Aug 5, 2024

Conversation

sunkup
Copy link
Member

@sunkup sunkup commented Jul 13, 2024

Purpose

The sync algorithm of the different syncer implementations is very similiar. One generalized sync algorithm in the abstract syncer, with implementation details in the implemented Syncers, is less error prone and easier to understand and test with unit tests.

Future error handling (#887) should become easier as well.

Short description

  • Passes only tasks authorities along, since the others can be inferred from chosen concrete syncer in BaseSyncWorker
  • Acquires generic content provider in abstract syncer and specific TaskProvider in TaskSyncer
  • Generalizes sync algorithm for the different resources (calendars, tasks, jtx collections, address books)
    • Provide collection to SyncManager #881 divided the process into four generic steps in every concrete syncer (CalendarSyncer, TasksSyncer, ...)
    • Now merges these steps in the abstract syncer and
      • calls abstract syncer methods implemented in extending syncers for the specific syncers resource details (i.e. create/delete/update a resource)
  • use repositories instead of DAOs
  • use new way to acquire logger
  • drop SyncerTest - Tests will be added in seperate PR
  • remove obsolete undocumented conditional (see 4e67988)
  • NOTE: TaskSyncer still needs to acquire its own TaskProvider, since finding the local sync enabled collections with DmfsTaskList.find() requires it, which would in turn require some more rewriting in ical4android.

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.

Depends on #909

@sunkup sunkup linked an issue Jul 13, 2024 that may be closed by this pull request
6 tasks
@sunkup sunkup force-pushed the 896-generalize-syncer branch from c38fd6d to 6911576 Compare July 14, 2024 09:34
@sunkup sunkup self-assigned this Jul 14, 2024
@sunkup sunkup force-pushed the 896-generalize-syncer branch from 6911576 to 32a5e5f Compare July 15, 2024 08:51
Copy link

This PR/issue depends on:

@sunkup sunkup force-pushed the 896-generalize-syncer branch from 5b61cb0 to f421ad1 Compare July 16, 2024 10:00
@rfc2822
Copy link
Member

rfc2822 commented Jul 21, 2024

Please rebase/merge from main-ose, changed a lot of things (mainly DI) that also affect Syncer constructors etc (but not its algorithm).

@sunkup sunkup force-pushed the 896-generalize-syncer branch from 79feede to 0d199a3 Compare July 22, 2024 14:52
@sunkup sunkup force-pushed the 896-generalize-syncer branch from 2f129dd to d56586b Compare August 1, 2024 12:10
@sunkup sunkup force-pushed the 896-generalize-syncer branch from d56586b to eceef11 Compare August 1, 2024 12:27
@sunkup
Copy link
Member Author

sunkup commented Aug 1, 2024

@rfc2822 Ready :)

I used a nullable string for the url property of LocalCollection, since JtxCollection requires it. We could make the change in ical4android of course, but I felt it was not really worth it, if we are going to change to the ID property soon anyways.

Also the TaskSyncer still needs to acquire TaskProvider, see the PR description for details.

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 already good with having the delete and url in the LocalCollection!

Did a few minor changes, and have some requests again.

@rfc2822 rfc2822 added the refactoring Internal improvement of existing functions label Aug 2, 2024
@rfc2822
Copy link
Member

rfc2822 commented Aug 5, 2024

I have created bitfireAT/ical4android#161 for future improvement :)

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 very good now! Hope that everything will work as expected.

@rfc2822 rfc2822 merged commit bf1bdfc into main-ose Aug 5, 2024
8 checks passed
@rfc2822 rfc2822 deleted the 896-generalize-syncer branch August 5, 2024 15:22
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.

Generalize Syncer
2 participants